From 73a68abb28b9ad5f4e9dee2781cdc5203bcb5937 Mon Sep 17 00:00:00 2001 From: Ryan Rossiter Date: Mon, 29 Jun 2015 13:19:07 +0000 Subject: [PATCH] Set vif and allocated when associating fixed ip Follow on to changes I84fa2f0926f719bff33ac43b39e161a4eb412f44 and I8cf5483982085da57ee470fa2753b0d0aebc12b3. This change removes a race when associating fixed IPs and pools. Originally, the fixed IPs were associated, and "virtual_interface_id" was updated and saved to the DB at a later time. This value is now sent into associate(_pool) and are tunneled down to the DB on associate, so now the value is saved to the DB at association. Change-Id: Ic37d96deba3a5ef51594733cc16091173a9655a4 Closes-Bug: #1249065 --- nova/db/api.py | 9 +++--- nova/db/sqlalchemy/api.py | 13 +++++--- nova/network/manager.py | 29 +++++++---------- nova/objects/fixed_ip.py | 20 +++++++----- nova/objects/floating_ip.py | 11 ++++--- nova/tests/unit/db/test_db_api.py | 24 +++++++++++++- nova/tests/unit/network/test_manager.py | 40 ++++++++++-------------- nova/tests/unit/objects/test_fixed_ip.py | 28 +++++++++++++++-- nova/tests/unit/objects/test_objects.py | 8 ++--- 9 files changed, 115 insertions(+), 67 deletions(-) diff --git a/nova/db/api.py b/nova/db/api.py index 30d1f794520b..d00aa1e0e5c7 100644 --- a/nova/db/api.py +++ b/nova/db/api.py @@ -488,25 +488,26 @@ def migration_get_all_by_filters(context, filters): def fixed_ip_associate(context, address, instance_uuid, network_id=None, - reserved=False): + reserved=False, virtual_interface_id=None): """Associate fixed ip to instance. Raises if fixed ip is not available. """ return IMPL.fixed_ip_associate(context, address, instance_uuid, network_id, - reserved) + reserved, virtual_interface_id) def fixed_ip_associate_pool(context, network_id, instance_uuid=None, - host=None): + host=None, virtual_interface_id=None): """Find free ip in network and associate it to instance or host. Raises if one is not available. """ return IMPL.fixed_ip_associate_pool(context, network_id, - instance_uuid, host) + instance_uuid, host, + virtual_interface_id) def fixed_ip_create(context, values): diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index 8fccc2894c82..9bb81b2e8e7c 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -1085,7 +1085,7 @@ def dnsdomain_get_all(context): @oslo_db_api.wrap_db_retry(max_retries=5, retry_on_deadlock=True, retry_on_request=True) def fixed_ip_associate(context, address, instance_uuid, network_id=None, - reserved=False): + reserved=False, virtual_interface_id=None): """Keyword arguments: reserved -- should be a boolean value(True or False), exact value will be used to filter on the fixed ip address @@ -1111,9 +1111,12 @@ def fixed_ip_associate(context, address, instance_uuid, network_id=None, raise exception.FixedIpAlreadyInUse(address=address, instance_uuid=instance_uuid) - params = {'instance_uuid': instance_uuid} + params = {'instance_uuid': instance_uuid, + 'allocated': virtual_interface_id is not None} if not fixed_ip_ref.network_id: params['network_id'] = network_id + if virtual_interface_id: + params['virtual_interface_id'] = virtual_interface_id rows_updated = model_query(context, models.FixedIp, session=session, read_deleted="no").\ @@ -1135,7 +1138,7 @@ def fixed_ip_associate(context, address, instance_uuid, network_id=None, @oslo_db_api.wrap_db_retry(max_retries=5, retry_on_deadlock=True, retry_on_request=True) def fixed_ip_associate_pool(context, network_id, instance_uuid=None, - host=None): + host=None, virtual_interface_id=None): if instance_uuid and not uuidutils.is_uuid_like(instance_uuid): raise exception.InvalidUUID(uuid=instance_uuid) @@ -1154,13 +1157,15 @@ def fixed_ip_associate_pool(context, network_id, instance_uuid=None, if not fixed_ip_ref: raise exception.NoMoreFixedIps(net=network_id) - params = {} + params = {'allocated': virtual_interface_id is not None} if fixed_ip_ref['network_id'] is None: params['network_id'] = network_id if instance_uuid: params['instance_uuid'] = instance_uuid if host: params['host'] = host + if virtual_interface_id: + params['virtual_interface_id'] = virtual_interface_id rows_updated = model_query(context, models.FixedIp, session=session, read_deleted="no").\ diff --git a/nova/network/manager.py b/nova/network/manager.py index babd768b3e59..86c1b9f26c58 100644 --- a/nova/network/manager.py +++ b/nova/network/manager.py @@ -910,10 +910,9 @@ class NetworkManager(manager.Manager): {'address': address, 'network': network['id'], 'cidr': network['cidr']}, instance=instance) - fip = objects.FixedIP.associate(context, - str(address), - instance_id, - network['id']) + fip = objects.FixedIP.associate( + context, str(address), instance_id, network['id'], + vif_id=vif.id) else: LOG.debug('Associating instance with fixed IP from pool ' 'in network %(network)s on subnet %(cidr)s.' % @@ -921,14 +920,12 @@ class NetworkManager(manager.Manager): 'cidr': network['cidr']}, instance=instance) fip = objects.FixedIP.associate_pool( - context.elevated(), network['id'], instance_id) + context.elevated(), network['id'], instance_id, + vif_id=vif.id) LOG.debug('Associated instance with fixed IP: %s', fip, instance=instance) address = str(fip.address) - fip.allocated = True - fip.virtual_interface_id = vif.id - fip.save() cleanup.append(functools.partial(fip.disassociate, context)) LOG.debug('Refreshing security group members for instance.', @@ -1998,23 +1995,21 @@ class VlanManager(RPCAllocateFixedIP, floating_ips.FloatingIP, NetworkManager): address = network['vpn_private_address'] fip = objects.FixedIP.associate(context, str(address), instance_id, network['id'], - reserved=True) + reserved=True, + vif_id=vif.id) else: address = kwargs.get('address', None) if address: fip = objects.FixedIP.associate(context, str(address), instance_id, - network['id']) + network['id'], + vif_id=vif.id) else: - fip = objects.FixedIP.associate_pool(context, - network['id'], - instance_id) + fip = objects.FixedIP.associate_pool( + context, network['id'], instance_id, + vif_id=vif.id) address = fip.address - fip.allocated = True - fip.virtual_interface_id = vif.id - fip.save() - if not kwargs.get('vpn', None): self._do_trigger_security_group_members_refresh_for_instance( instance_id) diff --git a/nova/objects/fixed_ip.py b/nova/objects/fixed_ip.py index e9a365e81b33..0779ef05a445 100644 --- a/nova/objects/fixed_ip.py +++ b/nova/objects/fixed_ip.py @@ -44,7 +44,8 @@ class FixedIP(obj_base.NovaPersistentObject, obj_base.NovaObject, # Version 1.11: Instance 1.21 # Version 1.12: Instance 1.22, FloatingIPList 1.9 # Version 1.13: Instance 1.23, FloatingIPList 1.10 - VERSION = '1.13' + # Version 1.14: Added vif_id kwarg to associate(_pool), FloatingIPList 1.11 + VERSION = '1.14' fields = { 'id': fields.IntegerField(), @@ -75,7 +76,7 @@ class FixedIP(obj_base.NovaPersistentObject, obj_base.NovaObject, 'network': [('1.0', '1.2')], 'virtual_interface': [('1.1', '1.0')], 'floating_ips': [('1.5', '1.7'), ('1.11', '1.8'), ('1.12', '1.9'), - ('1.13', '1.10')], + ('1.13', '1.10'), ('1.14', '1.11')], } def obj_make_compatible(self, primitive, target_version): @@ -153,18 +154,20 @@ class FixedIP(obj_base.NovaPersistentObject, obj_base.NovaObject, @obj_base.remotable_classmethod def associate(cls, context, address, instance_uuid, network_id=None, - reserved=False): + reserved=False, vif_id=None): db_fixedip = db.fixed_ip_associate(context, address, instance_uuid, network_id=network_id, - reserved=reserved) + reserved=reserved, + virtual_interface_id=vif_id) return cls._from_db_object(context, cls(context), db_fixedip) @obj_base.remotable_classmethod def associate_pool(cls, context, network_id, instance_uuid=None, - host=None): + host=None, vif_id=None): db_fixedip = db.fixed_ip_associate_pool(context, network_id, instance_uuid=instance_uuid, - host=host) + host=host, + virtual_interface_id=vif_id) return cls._from_db_object(context, cls(context), db_fixedip) @obj_base.remotable_classmethod @@ -225,7 +228,8 @@ class FixedIPList(obj_base.ObjectListBase, obj_base.NovaObject): # Version 1.11: FixedIP <= version 1.11 # Version 1.12: FixedIP <= version 1.12 # Version 1.13: FixedIP <= version 1.13 - VERSION = '1.13' + # Version 1.14: FixedIP <= version 1.14 + VERSION = '1.14' fields = { 'objects': fields.ListOfObjectsField('FixedIP'), @@ -235,7 +239,7 @@ class FixedIPList(obj_base.ObjectListBase, obj_base.NovaObject): ('1.3', '1.3'), ('1.4', '1.4'), ('1.5', '1.5'), ('1.6', '1.6'), ('1.7', '1.7'), ('1.8', '1.8'), ('1.9', '1.9'), ('1.10', '1.10'), ('1.11', '1.11'), - ('1.12', '1.12'), ('1.13', '1.13')], + ('1.12', '1.12'), ('1.13', '1.13'), ('1.14', '1.14')], } @obj_base.remotable_classmethod diff --git a/nova/objects/floating_ip.py b/nova/objects/floating_ip.py index 30ddbc3765f5..3d2030507454 100644 --- a/nova/objects/floating_ip.py +++ b/nova/objects/floating_ip.py @@ -35,7 +35,8 @@ class FloatingIP(obj_base.NovaPersistentObject, obj_base.NovaObject, # Version 1.7: FixedIP <= version 1.11 # Version 1.8: FixedIP <= version 1.12 # Version 1.9: FixedIP <= version 1.13 - VERSION = '1.9' + # Version 1.10: FixedIP <= version 1.14 + VERSION = '1.10' fields = { 'id': fields.IntegerField(), 'address': fields.IPAddressField(), @@ -51,7 +52,8 @@ class FloatingIP(obj_base.NovaPersistentObject, obj_base.NovaObject, obj_relationships = { 'fixed_ip': [('1.0', '1.1'), ('1.2', '1.2'), ('1.3', '1.3'), ('1.4', '1.4'), ('1.5', '1.5'), ('1.6', '1.6'), - ('1.7', '1.11'), ('1.8', '1.12'), ('1.9', '1.13')], + ('1.7', '1.11'), ('1.8', '1.12'), ('1.9', '1.13'), + ('1.10', '1.14')], } @staticmethod @@ -177,6 +179,7 @@ class FloatingIPList(obj_base.ObjectListBase, obj_base.NovaObject): # Version 1.8: FloatingIP 1.7 # Version 1.9: FloatingIP 1.8 # Version 1.10: FloatingIP 1.9 + # Version 1.11: FloatingIP 1.10 fields = { 'objects': fields.ListOfObjectsField('FloatingIP'), } @@ -184,9 +187,9 @@ class FloatingIPList(obj_base.ObjectListBase, obj_base.NovaObject): 'objects': [('1.0', '1.0'), ('1.1', '1.1'), ('1.2', '1.1'), ('1.3', '1.2'), ('1.4', '1.3'), ('1.5', '1.4'), ('1.6', '1.5'), ('1.7', '1.6'), ('1.8', '1.7'), - ('1.9', '1.8'), ('1.10', '1.9')], + ('1.9', '1.8'), ('1.10', '1.9'), ('1.11', '1.10')], } - VERSION = '1.10' + VERSION = '1.11' @obj_base.remotable_classmethod def get_all(cls, context): diff --git a/nova/tests/unit/db/test_db_api.py b/nova/tests/unit/db/test_db_api.py index cb50c1f10950..90e50797e506 100644 --- a/nova/tests/unit/db/test_db_api.py +++ b/nova/tests/unit/db/test_db_api.py @@ -4532,6 +4532,28 @@ class FixedIPTestCase(BaseInstanceTypeTestCase): self.ctxt, address, instance_uuid) self.assertEqual(1, mock_first.call_count) + def test_fixed_ip_associate_with_vif(self): + instance_uuid = self._create_instance() + network = db.network_create_safe(self.ctxt, {}) + vif = db.virtual_interface_create(self.ctxt, {}) + address = self.create_fixed_ip() + + fixed_ip = db.fixed_ip_associate(self.ctxt, address, instance_uuid, + network_id=network['id'], + virtual_interface_id=vif['id']) + + self.assertTrue(fixed_ip['allocated']) + self.assertEqual(vif['id'], fixed_ip['virtual_interface_id']) + + def test_fixed_ip_associate_not_allocated_without_vif(self): + instance_uuid = self._create_instance() + address = self.create_fixed_ip() + + fixed_ip = db.fixed_ip_associate(self.ctxt, address, instance_uuid) + + self.assertFalse(fixed_ip['allocated']) + self.assertIsNone(fixed_ip['virtual_interface_id']) + def test_fixed_ip_associate_pool_invalid_uuid(self): instance_uuid = '123' self.assertRaises(exception.InvalidUUID, db.fixed_ip_associate_pool, @@ -6143,7 +6165,7 @@ class NetworkTestCase(test.TestCase, ModelsObjectComparatorMixin): 'network_id': network.id, 'allocated': True, 'virtual_interface_id': virtual_interface.id}) db.fixed_ip_associate(self.ctxt, ip, instance.uuid, - network.id) + network.id, virtual_interface_id=virtual_interface['id']) return network, instance def test_network_get_associated_default_route(self): diff --git a/nova/tests/unit/network/test_manager.py b/nova/tests/unit/network/test_manager.py index a546c40b1aca..ec4ed43696c0 100644 --- a/nova/tests/unit/network/test_manager.py +++ b/nova/tests/unit/network/test_manager.py @@ -683,7 +683,8 @@ class FlatNetworkTestCase(test.TestCase): mock_associate.assert_called_once_with(self.context, '1.2.3.4', instance.uuid, - 1) + 1, + vif_id=1) @mock.patch('nova.objects.instance.Instance.get_by_uuid') @mock.patch('nova.objects.virtual_interface.VirtualInterface' @@ -736,11 +737,9 @@ class FlatNetworkTestCase(test.TestCase): '.get_by_instance_and_network') @mock.patch('nova.objects.fixed_ip.FixedIP.disassociate') @mock.patch('nova.objects.fixed_ip.FixedIP.associate_pool') - @mock.patch('nova.objects.fixed_ip.FixedIP.save') @mock.patch('nova.network.manager.NetworkManager._add_virtual_interface') def test_allocate_fixed_ip_create_new_vifs(self, mock_add, - mock_fixedip_save, mock_fixedip_associate, mock_fixedip_disassociate, mock_vif_get, @@ -749,7 +748,7 @@ class FlatNetworkTestCase(test.TestCase): fip = objects.FixedIP(instance_uuid='fake-uuid', address=address, - virtual_interface_id=1) + virtual_interface_id=1000) net = {'cidr': '24', 'id': 1, 'uuid': 'nosuch'} instance = objects.Instance(context=self.context) instance.create() @@ -881,10 +880,9 @@ class VlanNetworkTestCase(test.TestCase): mox.IgnoreArg(), mox.IgnoreArg(), network_id=mox.IgnoreArg(), - reserved=True).AndReturn(fixed) - db.fixed_ip_update(mox.IgnoreArg(), - mox.IgnoreArg(), - mox.IgnoreArg()) + reserved=True, + virtual_interface_id=vifs[0]['id'] + ).AndReturn(fixed) db.virtual_interface_get_by_instance_and_network(mox.IgnoreArg(), mox.IgnoreArg(), mox.IgnoreArg()).AndReturn(vifs[0]) db.instance_get_by_uuid(mox.IgnoreArg(), @@ -907,7 +905,6 @@ class VlanNetworkTestCase(test.TestCase): '_do_trigger_security_group_members_refresh_for_instance', lambda *a, **kw: None) self.mox.StubOutWithMock(db, 'fixed_ip_associate_pool') - self.mox.StubOutWithMock(db, 'fixed_ip_update') self.mox.StubOutWithMock(db, 'virtual_interface_get_by_instance_and_network') self.mox.StubOutWithMock(db, 'instance_get_by_uuid') @@ -917,10 +914,9 @@ class VlanNetworkTestCase(test.TestCase): db.fixed_ip_associate_pool(mox.IgnoreArg(), mox.IgnoreArg(), instance_uuid=mox.IgnoreArg(), - host=None).AndReturn(fixed) - db.fixed_ip_update(mox.IgnoreArg(), - mox.IgnoreArg(), - mox.IgnoreArg()) + host=None, + virtual_interface_id=vifs[0]['id'] + ).AndReturn(fixed) db.virtual_interface_get_by_instance_and_network(mox.IgnoreArg(), mox.IgnoreArg(), mox.IgnoreArg()).AndReturn(vifs[0]) db.instance_get_by_uuid(mox.IgnoreArg(), @@ -945,9 +941,8 @@ class VlanNetworkTestCase(test.TestCase): @mock.patch('nova.network.manager.VlanManager._add_virtual_interface') @mock.patch('nova.objects.instance.Instance.get_by_uuid') @mock.patch('nova.objects.fixed_ip.FixedIP.associate') - @mock.patch('nova.objects.fixed_ip.FixedIP.save') @mock.patch('nova.objects.VirtualInterface.get_by_instance_and_network') - def test_allocate_fixed_ip_return_none(self, mock_get, mock_save, + def test_allocate_fixed_ip_return_none(self, mock_get, mock_associate, mock_get_uuid, mock_add, mock_trigger, mock_validate, mock_setup): net = {'cidr': '24', 'id': 1, 'uuid': 'nosuch'} @@ -974,7 +969,6 @@ class VlanNetworkTestCase(test.TestCase): mock_add.assert_called_once_with(self.context_admin, instance.uuid, net['id']) - mock_save.assert_called_once_with() @mock.patch('nova.objects.instance.Instance.get_by_uuid') @mock.patch('nova.objects.fixed_ip.FixedIP.associate') @@ -992,7 +986,8 @@ class VlanNetworkTestCase(test.TestCase): mock_associate.assert_called_once_with(self.context, '1.2.3.4', instance.uuid, - 1) + 1, + vif_id=1) @mock.patch('nova.objects.instance.Instance.get_by_uuid') @mock.patch('nova.objects.fixed_ip.FixedIP.associate') @@ -1011,7 +1006,8 @@ class VlanNetworkTestCase(test.TestCase): mock_associate.assert_called_once_with(self.context, '1.2.3.4', instance.uuid, - 1, reserved=True) + 1, reserved=True, + vif_id=1) @mock.patch.object(db, 'virtual_interface_get_by_instance_and_network', return_value=None) @@ -1590,13 +1586,9 @@ class VlanNetworkTestCase(test.TestCase): self.mox.StubOutWithMock(db, 'fixed_ip_associate_pool') self.mox.StubOutWithMock(db, 'virtual_interface_get_by_instance_and_network') - self.mox.StubOutWithMock(db, 'fixed_ip_update') self.mox.StubOutWithMock(db, 'instance_get_by_uuid') self.mox.StubOutWithMock(self.network, 'get_instance_nw_info') - db.fixed_ip_update(mox.IgnoreArg(), - mox.IgnoreArg(), - mox.IgnoreArg()) db.virtual_interface_get_by_instance_and_network(mox.IgnoreArg(), mox.IgnoreArg(), mox.IgnoreArg()).AndReturn(vifs[0]) @@ -1605,7 +1597,9 @@ class VlanNetworkTestCase(test.TestCase): db.fixed_ip_associate_pool(mox.IgnoreArg(), mox.IgnoreArg(), instance_uuid=mox.IgnoreArg(), - host=None).AndReturn(fixed) + host=None, + virtual_interface_id=vifs[0]['id'] + ).AndReturn(fixed) db.network_get(mox.IgnoreArg(), mox.IgnoreArg(), project_only=mox.IgnoreArg() diff --git a/nova/tests/unit/objects/test_fixed_ip.py b/nova/tests/unit/objects/test_fixed_ip.py index fc03b0a8fb0f..1310f50a3650 100644 --- a/nova/tests/unit/objects/test_fixed_ip.py +++ b/nova/tests/unit/objects/test_fixed_ip.py @@ -163,7 +163,20 @@ class _TestFixedIPObject(object): fixedip = fixed_ip.FixedIP.associate(self.context, '1.2.3.4', 'fake-uuid') associate.assert_called_with(self.context, '1.2.3.4', 'fake-uuid', - network_id=None, reserved=False) + network_id=None, reserved=False, + virtual_interface_id=None) + self._compare(fixedip, fake_fixed_ip) + + @mock.patch('nova.db.fixed_ip_associate') + def test_associate_with_vif(self, associate): + associate.return_value = fake_fixed_ip + fixedip = fixed_ip.FixedIP.associate(self.context, '1.2.3.4', + 'fake-uuid', + vif_id=0) + associate.assert_called_with(self.context, '1.2.3.4', + 'fake-uuid', + network_id=None, reserved=False, + virtual_interface_id=0) self._compare(fixedip, fake_fixed_ip) @mock.patch('nova.db.fixed_ip_associate_pool') @@ -173,7 +186,18 @@ class _TestFixedIPObject(object): 'fake-uuid', 'host') associate.assert_called_with(self.context, 123, instance_uuid='fake-uuid', - host='host') + host='host', virtual_interface_id=None) + self._compare(fixedip, fake_fixed_ip) + + @mock.patch('nova.db.fixed_ip_associate_pool') + def test_associate_pool_with_vif(self, associate): + associate.return_value = fake_fixed_ip + fixedip = fixed_ip.FixedIP.associate_pool(self.context, 123, + 'fake-uuid', 'host', + vif_id=0) + associate.assert_called_with(self.context, 123, + instance_uuid='fake-uuid', + host='host', virtual_interface_id=0) self._compare(fixedip, fake_fixed_ip) @mock.patch('nova.db.fixed_ip_disassociate') diff --git a/nova/tests/unit/objects/test_objects.py b/nova/tests/unit/objects/test_objects.py index 1687ec65817e..c54e27e4d3b9 100644 --- a/nova/tests/unit/objects/test_objects.py +++ b/nova/tests/unit/objects/test_objects.py @@ -1176,12 +1176,12 @@ object_data = { 'EC2InstanceMapping': '1.0-a4556eb5c5e94c045fe84f49cf71644f', 'EC2SnapshotMapping': '1.0-47e7ddabe1af966dce0cfd0ed6cd7cd1', 'EC2VolumeMapping': '1.0-5b713751d6f97bad620f3378a521020d', - 'FixedIP': '1.13-b5818a33996228fc146f096d1403742c', - 'FixedIPList': '1.13-87a39361c8f08f059004d6b15103cdfd', + 'FixedIP': '1.14-53e1c10b539f1a82fe83b1af4720efae', + 'FixedIPList': '1.14-87a39361c8f08f059004d6b15103cdfd', 'Flavor': '1.1-b6bb7a730a79d720344accefafacf7ee', 'FlavorList': '1.1-52b5928600e7ca973aa4fc1e46f3934c', - 'FloatingIP': '1.9-52a67d52d85eb8b3f324a5b7935a335b', - 'FloatingIPList': '1.10-7f2ba670714e1b7bab462ab3290f7159', + 'FloatingIP': '1.10-52a67d52d85eb8b3f324a5b7935a335b', + 'FloatingIPList': '1.11-7f2ba670714e1b7bab462ab3290f7159', 'HostMapping': '1.0-1a3390a696792a552ab7bd31a77ba9ac', 'HVSpec': '1.1-6b4f7c0f688cbd03e24142a44eb9010d', 'ImageMeta': '1.7-642d1b2eb3e880a367f37d72dd76162d',