diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 4de52119e016..244ae5b7f339 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -663,7 +663,14 @@ class ComputeManager(manager.Manager): return evacuations = {mig.instance_uuid: mig for mig in evacuations} - local_instances = self._get_instances_on_driver(context) + # The instances might be deleted in which case we need to avoid + # InstanceNotFound being raised from lazy-loading fields on the + # instances while cleaning up this host. + read_deleted_context = context.elevated(read_deleted='yes') + # TODO(mriedem): We could optimize by pre-loading the joined fields + # we know we'll use, like info_cache and flavor. We can also replace + # this with a generic solution: https://review.openstack.org/575190/ + local_instances = self._get_instances_on_driver(read_deleted_context) evacuated = [inst for inst in local_instances if inst.uuid in evacuations] @@ -708,8 +715,12 @@ class ComputeManager(manager.Manager): continue cn_uuid = compute_nodes[migration.source_node] - if not scheduler_utils.remove_allocation_from_compute( - context, instance, cn_uuid, self.reportclient): + # If the instance was deleted in the interim, assume its + # allocations were properly cleaned up (either by its hosting + # compute service or the API). + if (not instance.deleted and + not scheduler_utils.remove_allocation_from_compute( + context, instance, cn_uuid, self.reportclient)): LOG.error("Failed to clean allocation of evacuated instance " "on the source node %s", cn_uuid, instance=instance) diff --git a/nova/tests/functional/regressions/test_bug_1794996.py b/nova/tests/functional/regressions/test_bug_1794996.py index b35e7bfd5d75..8ca09439c7cc 100644 --- a/nova/tests/functional/regressions/test_bug_1794996.py +++ b/nova/tests/functional/regressions/test_bug_1794996.py @@ -12,7 +12,6 @@ from oslo_log import log as logging -from nova import exception from nova.tests.functional import test_servers LOG = logging.getLogger(__name__) @@ -128,7 +127,8 @@ class TestEvacuateDeleteServerRestartOriginalCompute( instance. Before the bug is fixed, the original compute fails to start because lazy-loading the instance.flavor on the deleted instance, which is needed to cleanup allocations from the source host, raises - InstanceNotFound. + InstanceNotFound. After the bug is fixed, the original source host + compute service starts up. """ source_hostname = self.compute1.host dest_hostname = self.compute2.host @@ -176,8 +176,4 @@ class TestEvacuateDeleteServerRestartOriginalCompute( self._delete_and_check_allocations(server) # restart the source compute - # FIXME(mriedem): This is bug 1794996 where we try to lazy-load the - # instance.flavor from the deleted instance which causes the compute - # startup to fail. - self.assertRaises(exception.InstanceNotFound, - self.restart_compute_service, self.compute1) + self.restart_compute_service(self.compute1) diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 80a05d9a119b..058177a39ae7 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -7415,6 +7415,7 @@ class ComputeTestCase(BaseTestCase, instance = self._create_fake_instance_obj({'host': 'someotherhost'}) self.compute._instance_update(self.context, instance, vcpus=4) + @mock.patch('nova.context.RequestContext.elevated') @mock.patch.object(compute_manager.ComputeManager, '_get_instances_on_driver') @mock.patch.object(compute_manager.ComputeManager, @@ -7426,8 +7427,10 @@ class ComputeTestCase(BaseTestCase, @mock.patch('nova.objects.Migration.save') def test_destroy_evacuated_instance_on_shared_storage(self, mock_save, mock_get_filter, mock_destroy, mock_is_inst, mock_get_blk, - mock_get_inst): + mock_get_inst, mock_elevated): fake_context = context.get_admin_context() + read_deleted_context = fake_context.elevated(read_only='yes') + mock_elevated.return_value = read_deleted_context # instances in central db instances = [ @@ -7464,7 +7467,7 @@ class ComputeTestCase(BaseTestCase, 'pre-migrating', 'done'], 'migration_type': 'evacuation'}) - mock_get_inst.assert_called_once_with(fake_context) + mock_get_inst.assert_called_once_with(read_deleted_context) mock_get_nw.assert_called_once_with(fake_context, evacuated_instance) mock_get_blk.assert_called_once_with(fake_context, evacuated_instance) mock_is_inst.assert_called_once_with(fake_context, evacuated_instance) @@ -7472,6 +7475,7 @@ class ComputeTestCase(BaseTestCase, 'fake_network_info', 'fake_bdi', False) + @mock.patch('nova.context.RequestContext.elevated') @mock.patch.object(compute_manager.ComputeManager, '_get_instances_on_driver') @mock.patch.object(compute_manager.ComputeManager, @@ -7487,8 +7491,10 @@ class ComputeTestCase(BaseTestCase, @mock.patch('nova.objects.Migration.save') def test_destroy_evacuated_instance_with_disks(self, mock_save, mock_get_filter, mock_destroy, mock_check_clean, mock_check, - mock_check_local, mock_get_blk, mock_get_drv): + mock_check_local, mock_get_blk, mock_get_drv, mock_elevated): fake_context = context.get_admin_context() + read_deleted_context = fake_context.elevated(read_deleted='yes') + mock_elevated.return_value = read_deleted_context # instances in central db instances = [ @@ -7519,7 +7525,7 @@ class ComputeTestCase(BaseTestCase, return_value='fake_network_info') as mock_get_nw: self.compute._destroy_evacuated_instances(fake_context) - mock_get_drv.assert_called_once_with(fake_context) + mock_get_drv.assert_called_once_with(read_deleted_context) mock_get_nw.assert_called_once_with(fake_context, evacuated_instance) mock_get_blk.assert_called_once_with(fake_context, evacuated_instance) mock_check_local.assert_called_once_with(fake_context, @@ -7533,6 +7539,7 @@ class ComputeTestCase(BaseTestCase, 'fake_network_info', 'fake-bdi', True) + @mock.patch('nova.context.RequestContext.elevated') @mock.patch.object(compute_manager.ComputeManager, '_get_instances_on_driver') @mock.patch.object(compute_manager.ComputeManager, @@ -7547,8 +7554,10 @@ class ComputeTestCase(BaseTestCase, @mock.patch('nova.objects.Migration.save') def test_destroy_evacuated_instance_not_implemented(self, mock_save, mock_get_filter, mock_destroy, mock_check_clean, mock_check, - mock_check_local, mock_get_blk, mock_get_inst): + mock_check_local, mock_get_blk, mock_get_inst, mock_elevated): fake_context = context.get_admin_context() + read_deleted_context = fake_context.elevated(read_deleted='yes') + mock_elevated.return_value = read_deleted_context # instances in central db instances = [ @@ -7578,7 +7587,7 @@ class ComputeTestCase(BaseTestCase, return_value='fake_network_info') as mock_get_nw: self.compute._destroy_evacuated_instances(fake_context) - mock_get_inst.assert_called_once_with(fake_context) + mock_get_inst.assert_called_once_with(read_deleted_context) mock_get_nw.assert_called_once_with(fake_context, evacuated_instance) mock_get_blk.assert_called_once_with(fake_context, evacuated_instance) mock_check_local.assert_called_once_with(fake_context, diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index 0e4093968620..c557b0c3d9d2 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -705,6 +705,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): self.compute.init_virt_events() self.assertFalse(mock_register.called) + @mock.patch('nova.context.RequestContext.elevated') @mock.patch('nova.objects.ComputeNode.get_by_host_and_nodename') @mock.patch('nova.scheduler.utils.resources_from_flavor') @mock.patch.object(manager.ComputeManager, '_get_instances_on_driver') @@ -719,10 +720,12 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): def test_init_host_with_evacuated_instance(self, mock_save, mock_mig_get, mock_temp_mut, mock_init_host, mock_destroy, mock_host_get, mock_admin_ctxt, mock_init_virt, mock_get_inst, mock_resources, - mock_get_node): + mock_get_node, mock_elevated): our_host = self.compute.host not_our_host = 'not-' + our_host + read_deleted_context = self.context.elevated(read_deleted='yes') + mock_elevated.return_value = read_deleted_context deleted_instance = fake_instance.fake_instance_obj( self.context, host=not_our_host, uuid=uuids.deleted_instance) migration = objects.Migration(instance_uuid=deleted_instance.uuid) @@ -758,7 +761,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): expected_attrs=['info_cache', 'metadata']) mock_init_virt.assert_called_once_with() mock_temp_mut.assert_called_once_with(self.context, read_deleted='yes') - mock_get_inst.assert_called_once_with(self.context) + mock_get_inst.assert_called_once_with(read_deleted_context) mock_get_net.assert_called_once_with(self.context, deleted_instance) # ensure driver.destroy is called so that driver may @@ -3618,6 +3621,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): instance_2.host = 'not-' + our_host instance_2.user_id = uuids.user_id instance_2.project_id = uuids.project_id + instance_2.deleted = False # Only instance 2 has a migration record migration = objects.Migration(instance_uuid=instance_2.uuid) @@ -3675,6 +3679,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): instance_1.host = 'not-' + our_host instance_1.user_id = uuids.user_id instance_1.project_id = uuids.project_id + instance_1.deleted = False instance_2 = objects.Instance(self.context, flavor=flavor) instance_2.uuid = uuids.instance_2 instance_2.task_state = None @@ -3682,6 +3687,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): instance_2.host = 'not-' + our_host instance_2.user_id = uuids.user_id instance_2.project_id = uuids.project_id + instance_2.deleted = False migration_1 = objects.Migration(instance_uuid=instance_1.uuid) # Consider the migration successful but the node was deleted while the