Merge lp:~vishvananda/nova/fix-dhcpbridge into lp:~hudson-openstack/nova/trunk

[Vish Ishaya]

Fixes issues with allocation and deallocation of fixed and elastic addresses.

Fixes issues with allocation and deallocation of fixed and elastic addresses.

I ended up merging in fix-hostname while i was testing, so I'm resubmitting this branch with that one as a prereq.

[Jesse Andrews (anotherjesse)]

lgtm

review: Approve

[Devin Carlen (devcamcar)]

lgtm

review: Approve

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~vishvananda/nova/fix-dhcpbridge into lp:nova failed.Below is the output from the failed tests.

nova.tests.access_unittest
  AccessTestCase
    test_001_allow_all ... [OK]
    test_002_allow_none ... [OK]
    test_003_allow_project_manager ... [OK]
    test_004_allow_sys_and_net ... [OK]
    test_005_allow_sys_no_pm ... [OK]
nova.tests.api_unittest
  ApiEc2TestCase
    test_describe_instances ... [OK]
    test_get_all_key_pairs ... [OK]
nova.tests.auth_unittest
  AuthTestCase
    test_001_can_create_users ... [OK]
    test_002_can_get_user ... [OK]
    test_003_can_retreive_properties ... [OK]
    test_004_signature_is_valid ... [OK]
    test_005_can_get_credentials ... [OK]
    test_006_test_key_storage ... [OK]
    test_007_test_key_generation ... [OK]
    test_008_can_list_key_pairs ... [OK]
    test_009_can_delete_key_pair ... [OK]
    test_010_can_list_users ... [OK]
    test_101_can_add_user_role ... [OK]
    test_199_can_remove_user_role ... [OK]
    test_201_can_create_project ... [OK]
    test_202_user1_is_project_member ... [OK]
    test_203_user2_is_not_project_member ... [OK]
    test_204_user1_is_project_manager ... [OK]
    test_205_user2_is_not_project_manager ... [OK]
    test_206_can_add_user_to_project ... [OK]
    test_207_can_remove_user_from_project ... [OK]
    test_208_can_remove_add_user_with_role ... [OK]
    test_209_can_generate_x509 ... [OK]
    test_210_can_add_project_role ... [OK]
    test_211_can_list_project_roles ... [OK]
    test_212_can_remove_project_role ... [OK]
    test_214_can_retrieve_project_by_user ... [OK]
    test_299_can_delete_project ... [OK]
    test_999_can_delete_users ... [OK]
nova.tests.cloud_unittest
  CloudTestCase
    test_console_output ... [OK]
    test_instance_update_state ... [OK]
    test_run_instances ... ...

=== modified file 'bin/nova-dhcpbridge'
--- bin/nova-dhcpbridge	2010-08-12 21:22:40 +0000
+++ bin/nova-dhcpbridge	2010-08-12 21:22:40 +0000
@@ -69,7 +69,7 @@
     """Get the list of hosts for an interface."""
     net = model.get_network_by_interface(interface)
     res = ""
-    for address in net.address_objs:
+    for address in net.assigned_objs:
         res += "%s\n" % linux_net.host_dhcp(address)
     return res
 
 
=== modified file 'nova/endpoint/cloud.py'
--- nova/endpoint/cloud.py	2010-08-12 21:22:40 +0000
+++ nova/endpoint/cloud.py	2010-08-12 21:22:40 +0000
@@ -126,7 +126,7 @@
         else:
             keys = ''
 
-        address_record = network_model.Address(i['private_dns_name'])
+        address_record = network_model.FixedIp(i['private_dns_name'])
         if address_record:
             hostname = address_record['hostname']
         else:
@@ -311,7 +311,7 @@
 
     def _get_address(self, context, public_ip):
         # FIXME(vish) this should move into network.py
-        address = network_model.PublicAddress.lookup(public_ip)
+        address = network_model.ElasticIp.lookup(public_ip)
         if address and (context.user.is_admin() or address['project_id'] == context.project.id):
             return address
         raise exception.NotFound("Address at ip %s not found" % public_ip)
@@ -456,7 +456,7 @@
 
     def format_addresses(self, context):
         addresses = []
-        for address in network_model.PublicAddress.all():
+        for address in network_model.ElasticIp.all():
             # TODO(vish): implement a by_project iterator for addresses
             if (context.user.is_admin() or
                 address['project_id'] == context.project.id):
 
=== modified file 'nova/network/linux_net.py'
--- nova/network/linux_net.py	2010-08-12 21:22:40 +0000
+++ nova/network/linux_net.py	2010-08-12 21:22:40 +0000
@@ -116,7 +116,7 @@
         ' --pid-file=%s' % dhcp_file(net['vlan'], 'pid'),
         ' --listen-address=%s' % net.dhcp_listen_address,
         ' --except-interface=lo',
-        ' --dhcp-range=%s,static,600s' % net.dhcp_range_start,
+        ' --dhcp-range=%s,static,120s' % net.dhcp_range_start,
         ' --dhcp-hostsfile=%s' % dhcp_file(net['vlan'], 'conf'),
         ' --dhcp-script=%s' % bin_file('nova-dhcpbridge'),
         ' --leasefile-ro']
@@ -153,14 +153,10 @@
         #           correct dnsmasq process
         try:
             os.kill(pid, signal.SIGHUP)
+            return
         except Exception as exc:  # pylint: disable=W0703
             logging.debug("Hupping dnsmasq threw %s", exc)
 
-    # otherwise delete the existing leases file and start dnsmasq
-    lease_file = dhcp_file(network['vlan'], 'leases')
-    if os.path.exists(lease_file):
-        os.unlink(lease_file)
-
     # FLAGFILE and DNSMASQ_INTERFACE in env
     env = {'FLAGFILE': FLAGS.dhcpbridge_flagfile,
            'DNSMASQ_INTERFACE': network['bridge_name']}
 
=== modified file 'nova/network/model.py'
--- nova/network/model.py	2010-08-12 21:22:40 +0000
+++ nova/network/model.py	2010-08-12 21:22:40 +0000
@@ -143,25 +143,26 @@
                           network[start + FLAGS.network_size - 1])
 
 
-class Address(datastore.BasicModel):
+class FixedIp(datastore.BasicModel):
     """Represents a fixed ip in the datastore"""
-    override_type = "address"
 
     def __init__(self, address):
         self.address = address
-        super(Address, self).__init__()
+        super(FixedIp, self).__init__()
 
     @property
     def identifier(self):
         return self.address
 
+    # NOTE(vish): address states allocated, leased, deallocated
     def default_state(self):
-        return {'address': self.address}
+        return {'address': self.address,
+                'state': 'none'}
 
     @classmethod
     # pylint: disable=R0913
     def create(cls, user_id, project_id, address, mac, hostname, network_id):
-        """Creates an Address object"""
+        """Creates an FixedIp object"""
         addr = cls(address)
         addr['user_id'] = user_id
         addr['project_id'] = project_id
@@ -170,21 +171,22 @@
             hostname = "ip-%s" % address.replace('.', '-')
         addr['hostname'] = hostname
         addr['network_id'] = network_id
+        addr['state'] = 'allocated'
         addr.save()
         return addr
 
     def save(self):
         is_new = self.is_new_record()
-        success = super(Address, self).save()
+        success = super(FixedIp, self).save()
         if success and is_new:
             self.associate_with("network", self['network_id'])
 
     def destroy(self):
         self.unassociate_with("network", self['network_id'])
-        super(Address, self).destroy()
-
-
-class PublicAddress(Address):
+        super(FixedIp, self).destroy()
+
+
+class ElasticIp(FixedIp):
     """Represents an elastic ip in the datastore"""
     override_type = "address"
 
@@ -200,7 +202,7 @@
 class BaseNetwork(datastore.BasicModel):
     """Implements basic logic for allocating ips in a network"""
     override_type = 'network'
-    address_class = Address
+    address_class = FixedIp
 
     @property
     def identifier(self):
@@ -268,12 +270,12 @@
     # pylint: disable=R0913
     def _add_host(self, user_id, project_id, ip_address, mac, hostname):
         """Add a host to the datastore"""
-        Address.create(user_id, project_id, ip_address,
+        self.address_class.create(user_id, project_id, ip_address,
                        mac, hostname, self.identifier)
 
     def _rem_host(self, ip_address):
         """Remove a host from the datastore"""
-        Address(ip_address).destroy()
+        self.address_class(ip_address).destroy()
 
     @property
     def assigned(self):
@@ -322,7 +324,13 @@
 
     def lease_ip(self, ip_str):
         """Called when DHCP lease is activated"""
-        logging.debug("Leasing allocated IP %s", ip_str)
+        if not ip_str in self.assigned:
+            raise exception.AddressNotAllocated()
+        address = self.get_address(ip_str)
+        if address:
+            logging.debug("Leasing allocated IP %s", ip_str)
+            address['state'] = 'leased'
+            address.save()
 
     def release_ip(self, ip_str):
         """Called when DHCP lease expires
@@ -330,16 +338,23 @@
         Removes the ip from the assigned list"""
         if not ip_str in self.assigned:
             raise exception.AddressNotAllocated()
+        logging.debug("Releasing IP %s", ip_str)
         self._rem_host(ip_str)
         self.deexpress(address=ip_str)
-        logging.debug("Releasing IP %s", ip_str)
 
     def deallocate_ip(self, ip_str):
         """Deallocates an allocated ip"""
-        # NOTE(vish): Perhaps we should put the ip into an intermediate
-        #             state, so we know that we are pending waiting for
-        #             dnsmasq to confirm that it has been released.
-        logging.debug("Deallocating allocated IP %s", ip_str)
+        if not ip_str in self.assigned:
+            raise exception.AddressNotAllocated()
+        address = self.get_address(ip_str)
+        if address:
+            if address['state'] != 'leased':
+                # NOTE(vish): address hasn't been leased, so release it
+                self.release_ip(ip_str)
+            else:
+                logging.debug("Deallocating allocated IP %s", ip_str)
+                address['state'] == 'deallocated'
+                address.save()
 
     def express(self, address=None):
         """Set up network.  Implemented in subclasses"""
@@ -462,7 +477,7 @@
 class PublicNetworkController(BaseNetwork):
     """Handles elastic ips"""
     override_type = 'network'
-    address_class = PublicAddress
+    address_class = ElasticIp
 
     def __init__(self, *args, **kwargs):
         network_id = "public:default"
@@ -597,7 +612,7 @@
 
 def get_network_by_address(address):
     """Gets the network for a given private ip"""
-    address_record = Address.lookup(address)
+    address_record = FixedIp.lookup(address)
     if not address_record:
         raise exception.AddressNotAllocated()
     return get_project_network(address_record['project_id'])
@@ -613,6 +628,6 @@
 def get_public_ip_for_instance(instance_id):
     """Gets the public ip for a given instance"""
     # FIXME(josh): this should be a lookup - iteration won't scale
-    for address_record in PublicAddress.all():
+    for address_record in ElasticIp.all():
         if address_record.get('instance_id', 'available') == instance_id:
             return address_record['address']
 
=== modified file 'nova/network/service.py'
--- nova/network/service.py	2010-08-12 21:22:40 +0000
+++ nova/network/service.py	2010-08-12 21:22:40 +0000
@@ -226,13 +226,13 @@
         """Returns an ip to the pool"""
         return model.get_network_by_address(fixed_ip).deallocate_ip(fixed_ip)
 
-    def lease_ip(self, address):
+    def lease_ip(self, fixed_ip):
         """Called by bridge when ip is leased"""
-        return model.get_network_by_address(address).lease_ip(address)
+        return model.get_network_by_address(fixed_ip).lease_ip(fixed_ip)
 
-    def release_ip(self, address):
+    def release_ip(self, fixed_ip):
         """Called by bridge when ip is released"""
-        return model.get_network_by_address(address).release_ip(address)
+        return model.get_network_by_address(fixed_ip).release_ip(fixed_ip)
 
     def restart_nets(self):
         """Ensure the network for each user is enabled"""