From 2499f616094f63ed0a1a3c201789d14cc61a62c4 Mon Sep 17 00:00:00 2001 From: Surya Seetharaman Date: Tue, 8 May 2018 10:25:25 +0200 Subject: [PATCH] Update queued-for-delete from the ComputeAPI during deletion/restoration This patch updates queued-for-delete column in the instance_mapping table from the ComputeAPI during normal delete, local delete (when the compute service is down or instance is shelved offloaded) and soft-delete operations. Note that as the name of the column suggests, it is only "queued" for deletion and does not guarantee successful deletion of the instance. Hence the value could be stale which is fine, considering its use is only during down cell (desperate) situation. This in no way hampers normal delete operations in nova. This column is also updated from the ComputeAPI during the nova restore operation. Related to blueprint handling-down-cell Change-Id: I3de5c839d212cfab1c69e2c78617c1051b971c38 --- nova/compute/api.py | 18 +++++++++++ nova/tests/unit/compute/test_compute.py | 3 +- nova/tests/unit/compute/test_compute_api.py | 30 +++++++++++++++---- nova/tests/unit/compute/test_compute_cells.py | 8 +++-- 4 files changed, 51 insertions(+), 8 deletions(-) diff --git a/nova/compute/api.py b/nova/compute/api.py index 3cde93928446..b4f37a237293 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -2143,6 +2143,20 @@ class API(base.Base): cb(context, instance, bdms, local=True) instance.destroy() + @staticmethod + def _update_queued_for_deletion(context, instance, qfd): + # NOTE(tssurya): We query the instance_mapping record of this instance + # and update the queued_for_delete flag to True (or False according to + # the state of the instance). This just means that the instance is + # queued for deletion (or is no longer queued for deletion). It does + # not guarantee its successful deletion (or restoration). Hence the + # value could be stale which is fine, considering its use is only + # during down cell (desperate) situation. + im = objects.InstanceMapping.get_by_instance_uuid(context, + instance.uuid) + im.queued_for_delete = qfd + im.save() + def _do_delete(self, context, instance, bdms, local=False): if local: instance.vm_state = vm_states.DELETED @@ -2152,6 +2166,7 @@ class API(base.Base): else: self.compute_rpcapi.terminate_instance(context, instance, bdms, delete_type='delete') + self._update_queued_for_deletion(context, instance, True) def _do_force_delete(self, context, instance, bdms, local=False): if local: @@ -2162,6 +2177,7 @@ class API(base.Base): else: self.compute_rpcapi.terminate_instance(context, instance, bdms, delete_type='force_delete') + self._update_queued_for_deletion(context, instance, True) def _do_soft_delete(self, context, instance, bdms, local=False): if local: @@ -2171,6 +2187,7 @@ class API(base.Base): instance.save() else: self.compute_rpcapi.soft_delete_instance(context, instance) + self._update_queued_for_deletion(context, instance, True) # NOTE(maoy): we allow delete to be called no matter what vm_state says. @check_instance_lock @@ -2225,6 +2242,7 @@ class API(base.Base): instance.task_state = None instance.deleted_at = None instance.save(expected_task_state=[None]) + self._update_queued_for_deletion(context, instance, False) @check_instance_lock @check_instance_state(task_state=None, diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index a45b3f897369..e529f911daa6 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -9813,7 +9813,8 @@ class ComputeAPITestCase(BaseTestCase): i_ref = self._create_fake_instance_obj() self.assertEqual(i_ref['name'], i_ref['uuid']) - def test_add_remove_fixed_ip(self): + @mock.patch('nova.compute.api.API._update_queued_for_deletion') + def test_add_remove_fixed_ip(self, mock_update_queued_for_delete): instance = self._create_fake_instance_obj(params={'host': CONF.host}) self.compute_api.add_fixed_ip(self.context, instance, '1') self.compute_api.remove_fixed_ip(self.context, diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index 9bef37e4067f..db5060afdf13 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -1018,6 +1018,7 @@ class _ComputeAPIUnitTestMixIn(object): @mock.patch.object(objects.Migration, 'get_by_instance_and_status') @mock.patch.object(image_api.API, 'delete') + @mock.patch.object(objects.InstanceMapping, 'save') @mock.patch.object(objects.InstanceMapping, 'get_by_instance_uuid') @mock.patch.object(consoleauth_rpcapi.ConsoleAuthAPI, 'delete_tokens_for_instance') @@ -1037,7 +1038,7 @@ class _ComputeAPIUnitTestMixIn(object): def _test_delete(self, delete_type, mock_save, mock_bdm_get, mock_elevated, mock_get_cn, mock_up, mock_record, mock_inst_update, mock_deallocate, mock_inst_meta, mock_inst_destroy, - mock_notify, mock_del_token, mock_get_inst, + mock_notify, mock_del_token, mock_get_inst, mock_save_im, mock_image_delete, mock_mig_get, **attrs): expected_save_calls = [mock.call()] expected_record_calls = [] @@ -1177,8 +1178,10 @@ class _ComputeAPIUnitTestMixIn(object): mock_deallocate.assert_called_once_with(self.context, inst) mock_inst_destroy.assert_called_once_with( self.context, instance_uuid, constraint=None) - mock_get_inst.assert_called_once_with(self.context, - instance_uuid) + mock_get_inst.assert_called_with(self.context, instance_uuid) + self.assertEqual(2, mock_get_inst.call_count) + self.assertTrue(mock_get_inst.return_value.queued_for_delete) + mock_save_im.assert_called_once_with() if cast: if delete_type == 'soft_delete': @@ -4036,7 +4039,8 @@ class _ComputeAPIUnitTestMixIn(object): @mock.patch('nova.objects.Quotas.limit_check_project_and_user') @mock.patch('nova.objects.Instance.save') @mock.patch('nova.objects.InstanceAction.action_start') - def test_restore_by_admin(self, action_start, instance_save, + @mock.patch('nova.compute.api.API._update_queued_for_deletion') + def test_restore_by_admin(self, update_qfd, action_start, instance_save, quota_check, quota_count): admin_context = context.RequestContext('admin_user', 'admin_project', @@ -4067,12 +4071,15 @@ class _ComputeAPIUnitTestMixIn(object): 'cores': 1 + instance.flavor.vcpus, 'ram': 512 + instance.flavor.memory_mb}, project_id=instance.project_id, user_id=instance.user_id) + update_qfd.assert_called_once_with(admin_context, instance, False) @mock.patch('nova.objects.Quotas.count_as_dict') @mock.patch('nova.objects.Quotas.limit_check_project_and_user') @mock.patch('nova.objects.Instance.save') @mock.patch('nova.objects.InstanceAction.action_start') - def test_restore_by_instance_owner(self, action_start, instance_save, + @mock.patch('nova.compute.api.API._update_queued_for_deletion') + def test_restore_by_instance_owner(self, update_qfd, action_start, + instance_save, quota_check, quota_count): proj_count = {'instances': 1, 'cores': 1, 'ram': 512} user_count = proj_count.copy() @@ -4101,6 +4108,7 @@ class _ComputeAPIUnitTestMixIn(object): 'cores': 1 + instance.flavor.vcpus, 'ram': 512 + instance.flavor.memory_mb}, project_id=instance.project_id, user_id=instance.user_id) + update_qfd.assert_called_once_with(self.context, instance, False) @mock.patch.object(objects.InstanceAction, 'action_start') def test_external_instance_event(self, mock_action_start): @@ -5472,6 +5480,18 @@ class _ComputeAPIUnitTestMixIn(object): self.context, requested_networks, 5) self.assertEqual(4, count) + @mock.patch.object(objects.InstanceMapping, 'save') + @mock.patch.object(objects.InstanceMapping, 'get_by_instance_uuid') + def test_update_queued_for_deletion(self, mock_get, mock_save): + uuid = uuids.inst + inst = objects.Instance(uuid=uuid) + im = objects.InstanceMapping(instance_uuid=uuid) + mock_get.return_value = im + self.compute_api._update_queued_for_deletion(self.context, inst, True) + self.assertTrue(im.queued_for_delete) + mock_get.assert_called_once_with(self.context, inst.uuid) + mock_save.assert_called_once_with() + @mock.patch.object(objects.InstanceMapping, 'get_by_instance_uuid', side_effect=exception.InstanceMappingNotFound(uuid='fake')) @mock.patch.object(objects.BuildRequest, 'get_by_instance_uuid') diff --git a/nova/tests/unit/compute/test_compute_cells.py b/nova/tests/unit/compute/test_compute_cells.py index b8e03ade71a2..0b8111409694 100644 --- a/nova/tests/unit/compute/test_compute_cells.py +++ b/nova/tests/unit/compute/test_compute_cells.py @@ -288,10 +288,12 @@ class CellsComputeAPITestCase(test_compute.ComputeAPITestCase): 'instance_delete_everywhere') @mock.patch.object(compute_api.API, '_lookup_instance', return_value=(None, inst)) - def _test(_mock_lookup_inst, _mock_delete_everywhere): + @mock.patch.object(objects.InstanceMapping, 'get_by_instance_uuid') + def _test(mock_get_im, _mock_lookup_inst, _mock_delete_everywhere): self.assertRaises(exception.ObjectActionError, self.compute_api.delete, self.context, inst) inst.destroy.assert_called_once_with() + mock_get_im.assert_called_once_with(self.context, inst.uuid) _test() @@ -313,12 +315,14 @@ class CellsComputeAPITestCase(test_compute.ComputeAPITestCase): 'instance_delete_everywhere', side_effect=add_cell_name) @mock.patch.object(compute_api.API, '_lookup_instance', return_value=(None, inst)) - def _test(_mock_lookup_inst, mock_delete_everywhere, + @mock.patch.object(objects.InstanceMapping, 'get_by_instance_uuid') + def _test(mock_get_im, _mock_lookup_inst, mock_delete_everywhere, mock_compute_delete): self.compute_api.delete(self.context, inst) inst.destroy.assert_called_once_with() mock_compute_delete.assert_called_once_with(self.context, inst) + mock_get_im.assert_called_once_with(self.context, inst.uuid) _test()