From 481cb5ce04e220c26e5772f4253d63e212adca45 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Tue, 30 Apr 2019 17:45:32 -0400 Subject: [PATCH] Find instance in another cell during floating IP re-association When associating a floating IP to instance A but it was already associated with instance B, we try to refresh the info cache on instance B. The problem is the context is targeted to the cell for instance A and instance B might be in another cell, so we'll get an InstanceNotFound error trying to lookup instance B. This change tries to find the instance in another cell using its instance mapping, and makes the code a bit more graceful if instance B is deleted. Change-Id: I71790afd0784d98050ccd7cc0e046321da249cbe Closes-Bug: #1826472 --- devstack/nova-multi-cell-blacklist.txt | 3 - nova/network/neutronv2/api.py | 58 +++++++- nova/tests/unit/network/test_neutronv2.py | 168 ++++++++++++++++++++++ 3 files changed, 221 insertions(+), 8 deletions(-) diff --git a/devstack/nova-multi-cell-blacklist.txt b/devstack/nova-multi-cell-blacklist.txt index 87f07be42603..afda00fae1b2 100644 --- a/devstack/nova-multi-cell-blacklist.txt +++ b/devstack/nova-multi-cell-blacklist.txt @@ -4,6 +4,3 @@ # Exclude tempest.scenario.test_network tests since they are slow and # only test advanced neutron features, unrelated to multi-cell testing. ^tempest.scenario.test_network - -# Disable associate floating IP tests until bug 1826472 is fixed. -tempest.api.compute.floating_ips.test_floating_ips_actions.FloatingIPsAssociationTestJSON diff --git a/nova/network/neutronv2/api.py b/nova/network/neutronv2/api.py index a683d2d68450..64b300d323af 100644 --- a/nova/network/neutronv2/api.py +++ b/nova/network/neutronv2/api.py @@ -29,6 +29,7 @@ import six from nova.compute import utils as compute_utils import nova.conf +from nova import context as nova_context from nova import exception from nova.i18n import _ from nova.network import base_api @@ -2341,12 +2342,59 @@ class API(base_api.NetworkAPI): LOG.info('re-assign floating IP %(address)s from ' 'instance %(instance_id)s', msg_dict, instance=instance) - orig_instance = objects.Instance.get_by_uuid(context, - orig_instance_uuid) + orig_instance = self._get_instance_by_uuid_using_api_db( + context, orig_instance_uuid) + if orig_instance: + # purge cached nw info for the original instance; pass the + # context from the instance in case we found it in another cell + base_api.update_instance_cache_with_nw_info( + self, orig_instance._context, orig_instance) + else: + # Leave a breadcrumb about not being able to refresh the + # the cache for the original instance. + LOG.info('Unable to refresh the network info cache for ' + 'instance %s after disassociating floating IP %s. ' + 'If the instance still exists, its info cache may ' + 'be healed automatically.', + orig_instance_uuid, fip['id']) - # purge cached nw info for the original instance - base_api.update_instance_cache_with_nw_info(self, context, - orig_instance) + @staticmethod + def _get_instance_by_uuid_using_api_db(context, instance_uuid): + """Look up the instance by UUID + + This method is meant to be used sparingly since it tries to find + the instance by UUID in the cell-targeted context. If the instance + is not found, this method will try to determine if it's not found + because it is deleted or if it is just in another cell. Therefore + it assumes to have access to the API database and should only be + called from methods that are used in the control plane services. + + :param context: cell-targeted nova auth RequestContext + :param instance_uuid: UUID of the instance to find + :returns: Instance object if the instance was found, else None. + """ + try: + return objects.Instance.get_by_uuid(context, instance_uuid) + except exception.InstanceNotFound: + # The instance could be deleted or it could be in another cell. + # To determine if its in another cell, check the instance + # mapping in the API DB. + try: + inst_map = objects.InstanceMapping.get_by_instance_uuid( + context, instance_uuid) + except exception.InstanceMappingNotFound: + # The instance is gone so just return. + return + + # We have the instance mapping, look up the instance in the + # cell the instance is in. + with nova_context.target_cell( + context, inst_map.cell_mapping) as cctxt: + try: + return objects.Instance.get_by_uuid(cctxt, instance_uuid) + except exception.InstanceNotFound: + # Alright it's really gone. + return def get_all(self, context): """Get all networks for client.""" diff --git a/nova/tests/unit/network/test_neutronv2.py b/nova/tests/unit/network/test_neutronv2.py index 76ea4bff11dc..2cff523c9034 100644 --- a/nova/tests/unit/network/test_neutronv2.py +++ b/nova/tests/unit/network/test_neutronv2.py @@ -5753,6 +5753,174 @@ class TestNeutronv2WithMock(TestNeutronv2Base): mock_update_cache.assert_called_once_with( # from @refresh_cache self.api, ctxt, instance, nw_info=None) + @mock.patch('nova.network.base_api.update_instance_cache_with_nw_info') + def test_update_inst_info_cache_for_disassociated_fip_other_cell( + self, mock_update_cache): + """Tests a scenario where a floating IP is associated to an instance + in another cell from the one in which it's currently associated + and the network info cache on the original instance is refreshed. + """ + ctxt = context.get_context() + mock_client = mock.Mock() + new_instance = fake_instance.fake_instance_obj(ctxt) + cctxt = context.get_context() + old_instance = fake_instance.fake_instance_obj(cctxt) + fip = {'id': uuids.floating_ip_id, + 'port_id': uuids.old_port_id, + 'floating_ip_address': '172.24.5.15'} + # Setup the mocks. + with test.nested( + mock.patch.object(self.api, '_show_port', + return_value={ + 'device_id': old_instance.uuid}), + mock.patch.object(self.api, '_get_instance_by_uuid_using_api_db', + return_value=old_instance) + ) as ( + _show_port, _get_instance_by_uuid_using_api_db + ): + # Run the code. + self.api._update_inst_info_cache_for_disassociated_fip( + ctxt, new_instance, mock_client, fip) + # Assert the calls. + _show_port.assert_called_once_with( + ctxt, uuids.old_port_id, neutron_client=mock_client) + _get_instance_by_uuid_using_api_db.assert_called_once_with( + ctxt, old_instance.uuid) + mock_update_cache.assert_called_once_with( + self.api, cctxt, old_instance) + + @mock.patch('nova.network.neutronv2.api.LOG.info') + @mock.patch('nova.network.base_api.update_instance_cache_with_nw_info') + def test_update_inst_info_cache_for_disassociated_fip_inst_not_found( + self, mock_update_cache, mock_log_info): + """Tests the case that a floating IP is re-associated to an instance + in another cell but the original instance cannot be found. + """ + ctxt = context.get_context() + mock_client = mock.Mock() + new_instance = fake_instance.fake_instance_obj(ctxt) + fip = {'id': uuids.floating_ip_id, + 'port_id': uuids.old_port_id, + 'floating_ip_address': '172.24.5.15'} + # Setup the mocks. + with test.nested( + mock.patch.object(self.api, '_show_port', + return_value={ + 'device_id': uuids.original_inst_uuid}), + mock.patch.object(self.api, + '_get_instance_by_uuid_using_api_db', + return_value=None) + ) as ( + _show_port, _get_instance_by_uuid_using_api_db + ): + # Run the code. + self.api._update_inst_info_cache_for_disassociated_fip( + ctxt, new_instance, mock_client, fip) + # Assert the calls. + _show_port.assert_called_once_with( + ctxt, uuids.old_port_id, neutron_client=mock_client) + _get_instance_by_uuid_using_api_db.assert_called_once_with( + ctxt, uuids.original_inst_uuid) + mock_update_cache.assert_not_called() + self.assertEqual(2, mock_log_info.call_count) + self.assertIn('If the instance still exists, its info cache may ' + 'be healed automatically.', + mock_log_info.call_args[0][0]) + + @mock.patch('nova.objects.Instance.get_by_uuid') + @mock.patch('nova.objects.InstanceMapping.get_by_instance_uuid') + def test_get_instance_by_uuid_using_api_db_current_cell( + self, mock_get_map, mock_get_inst): + """Tests that _get_instance_by_uuid_using_api_db finds the + instance in the cell currently targeted by the context. + """ + ctxt = context.get_context() + instance = fake_instance.fake_instance_obj(ctxt) + mock_get_inst.return_value = instance + inst = self.api._get_instance_by_uuid_using_api_db(ctxt, instance.uuid) + self.assertIs(inst, instance) + mock_get_inst.assert_called_once_with(ctxt, instance.uuid) + mock_get_map.assert_not_called() + + @mock.patch('nova.objects.Instance.get_by_uuid') + @mock.patch('nova.objects.InstanceMapping.get_by_instance_uuid') + def test_get_instance_by_uuid_using_api_db_other_cell( + self, mock_get_map, mock_get_inst): + """Tests that _get_instance_by_uuid_using_api_db finds the + instance in another cell different from the currently targeted context. + """ + ctxt = context.get_context() + instance = fake_instance.fake_instance_obj(ctxt) + mock_get_map.return_value = objects.InstanceMapping( + cell_mapping=objects.CellMapping( + uuid=uuids.cell_mapping_uuid, + database_connection= + self.cell_mappings['cell1'].database_connection, + transport_url='none://fake')) + # Mock get_by_uuid to not find the instance in the first call, but + # do find it in the second. Target the instance context as well so + # we can assert that we used a different context for the 2nd call. + + def stub_inst_get_by_uuid(_context, instance_uuid, *args, **kwargs): + if not mock_get_map.called: + # First call, raise InstanceNotFound. + self.assertIs(_context, ctxt) + raise exception.InstanceNotFound(instance_id=instance_uuid) + # Else return the instance with a newly targeted context. + self.assertIsNot(_context, ctxt) + instance._context = _context + return instance + mock_get_inst.side_effect = stub_inst_get_by_uuid + + inst = self.api._get_instance_by_uuid_using_api_db(ctxt, instance.uuid) + # The instance's internal context should still be targeted and not + # the original context. + self.assertIsNot(inst._context, ctxt) + self.assertIsNotNone(inst._context.db_connection) + mock_get_map.assert_called_once_with(ctxt, instance.uuid) + mock_get_inst.assert_has_calls([ + mock.call(ctxt, instance.uuid), + mock.call(inst._context, instance.uuid)]) + + @mock.patch('nova.objects.Instance.get_by_uuid', + side_effect=exception.InstanceNotFound(instance_id=uuids.inst)) + @mock.patch('nova.objects.InstanceMapping.get_by_instance_uuid') + def test_get_instance_by_uuid_using_api_db_other_cell_never_found( + self, mock_get_map, mock_get_inst): + """Tests that _get_instance_by_uuid_using_api_db does not find the + instance in either the current cell or another cell. + """ + ctxt = context.get_context() + instance = fake_instance.fake_instance_obj(ctxt, uuid=uuids.inst) + mock_get_map.return_value = objects.InstanceMapping( + cell_mapping=objects.CellMapping( + uuid=uuids.cell_mapping_uuid, + database_connection= + self.cell_mappings['cell1'].database_connection, + transport_url='none://fake')) + self.assertIsNone( + self.api._get_instance_by_uuid_using_api_db(ctxt, instance.uuid)) + mock_get_map.assert_called_once_with(ctxt, instance.uuid) + mock_get_inst.assert_has_calls([ + mock.call(ctxt, instance.uuid), + mock.call(test.MatchType(context.RequestContext), instance.uuid)]) + + @mock.patch('nova.objects.Instance.get_by_uuid', + side_effect=exception.InstanceNotFound(instance_id=uuids.inst)) + @mock.patch('nova.objects.InstanceMapping.get_by_instance_uuid', + side_effect=exception.InstanceMappingNotFound(uuid=uuids.inst)) + def test_get_instance_by_uuid_using_api_db_other_cell_map_not_found( + self, mock_get_map, mock_get_inst): + """Tests that _get_instance_by_uuid_using_api_db does not find an + instance mapping for the instance. + """ + ctxt = context.get_context() + instance = fake_instance.fake_instance_obj(ctxt, uuid=uuids.inst) + self.assertIsNone( + self.api._get_instance_by_uuid_using_api_db(ctxt, instance.uuid)) + mock_get_inst.assert_called_once_with(ctxt, instance.uuid) + mock_get_map.assert_called_once_with(ctxt, instance.uuid) + @mock.patch('nova.network.neutronv2.api._get_ksa_client', new_callable=mock.NonCallableMock) # asserts not called def test_migrate_instance_start_no_binding_ext(self, get_client_mock):