diff --git a/nova/compute/manager.py b/nova/compute/manager.py index beec08f96b38..4527795338c7 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,9 +715,13 @@ class ComputeManager(manager.Manager): continue cn_uuid = compute_nodes[migration.source_node] - if not self.reportclient.\ - remove_provider_tree_from_instance_allocation( - context, instance.uuid, cn_uuid): + # 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 self.reportclient. + remove_provider_tree_from_instance_allocation( + context, instance.uuid, cn_uuid)): 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 499a9f30d3bb..2287fef61516 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 integrated_helpers 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 6c3a6e2a8893..e5d086716151 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -7465,6 +7465,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('nova.scheduler.client.report.SchedulerReportClient.' 'remove_provider_tree_from_instance_allocation') @mock.patch.object(compute_manager.ComputeManager, @@ -7478,8 +7479,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_remove_allocs): + mock_get_inst, mock_remove_allocs, 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 = [ @@ -7516,7 +7519,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) @@ -7527,6 +7530,7 @@ class ComputeTestCase(BaseTestCase, fake_context, evacuated_instance.uuid, self.rt.compute_nodes[NODENAME].uuid) + @mock.patch('nova.context.RequestContext.elevated') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' 'remove_provider_tree_from_instance_allocation') @mock.patch.object(compute_manager.ComputeManager, @@ -7544,8 +7548,11 @@ 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_remove_allocs): + mock_check_local, mock_get_blk, mock_get_drv, mock_remove_allocs, + 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 = [ @@ -7576,7 +7583,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, @@ -7593,6 +7600,7 @@ class ComputeTestCase(BaseTestCase, fake_context, evacuated_instance.uuid, self.rt.compute_nodes[NODENAME].uuid) + @mock.patch('nova.context.RequestContext.elevated') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' 'remove_provider_tree_from_instance_allocation') @mock.patch.object(compute_manager.ComputeManager, @@ -7609,8 +7617,11 @@ 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_remove_allocs): + mock_check_local, mock_get_blk, mock_get_inst, mock_remove_allocs, + 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 = [ @@ -7640,7 +7651,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 c1b8f4154a11..8bdc0f16ec40 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -848,6 +848,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') @@ -862,10 +863,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) @@ -899,7 +902,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 @@ -3807,6 +3810,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) @@ -3863,6 +3867,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 @@ -3870,6 +3875,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