From 580e8f6a5e8721b0535ea5fb1d414f6e9ede3070 Mon Sep 17 00:00:00 2001 From: Rajesh Tailor Date: Fri, 17 Oct 2014 05:46:41 -0700 Subject: [PATCH] Remove cell api overrides for force-delete The force_delete method of nova/compute/cells_api module is actually not needed, as this method handle objects and is triggered when force-delete api is used for instance deletion in cells environment. It actually causes a race condition with instance deletion as it triggers 2 instance deletion from a compute cell for single force-delete request. Modified delete-api implementation so that force-delete request triggers single instance deletion call in cells environment. Change-Id: I812f27761fc4d4f64563d79f793c484d03aa641f Closes-Bug: #1381425 --- nova/cells/manager.py | 9 ++++--- nova/cells/messaging.py | 12 ++++++---- nova/cells/rpcapi.py | 14 ++++++++--- nova/compute/api.py | 17 +++++++++++-- nova/compute/cells_api.py | 9 +++---- nova/compute/rpcapi.py | 6 ++++- nova/tests/unit/cells/test_cells_manager.py | 6 +++-- nova/tests/unit/cells/test_cells_rpcapi.py | 8 ++++--- nova/tests/unit/compute/test_compute_api.py | 5 ++-- nova/tests/unit/compute/test_compute_cells.py | 24 +++++++++---------- 10 files changed, 70 insertions(+), 40 deletions(-) diff --git a/nova/cells/manager.py b/nova/cells/manager.py index d19f11a4b5e5..4acbd90c0fcb 100644 --- a/nova/cells/manager.py +++ b/nova/cells/manager.py @@ -76,7 +76,7 @@ class CellsManager(manager.Manager): Scheduling requests get passed to the scheduler class. """ - target = oslo_messaging.Target(version='1.35') + target = oslo_messaging.Target(version='1.36') def __init__(self, *args, **kwargs): LOG.warning(_LW('The cells feature of Nova is considered experimental ' @@ -520,9 +520,12 @@ class CellsManager(manager.Manager): """Resume an instance in its cell.""" self.msg_runner.resume_instance(ctxt, instance) - def terminate_instance(self, ctxt, instance): + def terminate_instance(self, ctxt, instance, delete_type='delete'): """Delete an instance in its cell.""" - self.msg_runner.terminate_instance(ctxt, instance) + # NOTE(rajesht): The `delete_type` parameter is passed so that it will + # be routed to destination cell, where instance deletion will happen. + self.msg_runner.terminate_instance(ctxt, instance, + delete_type=delete_type) def soft_delete_instance(self, ctxt, instance): """Soft-delete an instance in its cell.""" diff --git a/nova/cells/messaging.py b/nova/cells/messaging.py index 178c2f9f0688..3c7fe0067697 100644 --- a/nova/cells/messaging.py +++ b/nova/cells/messaging.py @@ -832,7 +832,7 @@ class _TargetedMessageMethods(_BaseMessageMethods): self.msg_runner.instance_destroy_at_top(ctxt, instance) except exception.InstanceInfoCacheNotFound: - if method != 'delete': + if method not in ('delete', 'force_delete'): raise fn = getattr(self.compute_api, method, None) @@ -865,8 +865,8 @@ class _TargetedMessageMethods(_BaseMessageMethods): def get_host_uptime(self, message, host_name): return self.host_api.get_host_uptime(message.ctxt, host_name) - def terminate_instance(self, message, instance): - self._call_compute_api_with_obj(message.ctxt, instance, 'delete') + def terminate_instance(self, message, instance, delete_type='delete'): + self._call_compute_api_with_obj(message.ctxt, instance, delete_type) def soft_delete_instance(self, message, instance): self._call_compute_api_with_obj(message.ctxt, instance, 'soft_delete') @@ -1711,8 +1711,10 @@ class MessageRunner(object): """Resume an instance in its cell.""" self._instance_action(ctxt, instance, 'resume_instance') - def terminate_instance(self, ctxt, instance): - self._instance_action(ctxt, instance, 'terminate_instance') + def terminate_instance(self, ctxt, instance, delete_type='delete'): + extra_kwargs = dict(delete_type=delete_type) + self._instance_action(ctxt, instance, 'terminate_instance', + extra_kwargs=extra_kwargs) def soft_delete_instance(self, ctxt, instance): self._instance_action(ctxt, instance, 'soft_delete_instance') diff --git a/nova/cells/rpcapi.py b/nova/cells/rpcapi.py index 5b5b8a4f4b92..16db040e8949 100644 --- a/nova/cells/rpcapi.py +++ b/nova/cells/rpcapi.py @@ -116,6 +116,7 @@ class CellsAPI(object): * 1.35 - Make instance_update_at_top, instance_destroy_at_top and instance_info_cache_update_at_top use instance objects + * 1.36 - Added 'delete_type' parameter to terminate_instance() ''' VERSION_ALIASES = { @@ -564,15 +565,22 @@ class CellsAPI(object): cctxt = self.client.prepare(version='1.15') cctxt.cast(ctxt, 'resume_instance', instance=instance) - def terminate_instance(self, ctxt, instance, bdms, reservations=None): + def terminate_instance(self, ctxt, instance, bdms, reservations=None, + delete_type='delete'): """Delete an instance in its cell. This method takes a new-world instance object. """ if not CONF.cells.enable: return - cctxt = self.client.prepare(version='1.18') - cctxt.cast(ctxt, 'terminate_instance', instance=instance) + msg_kwargs = {'instance': instance} + if self.client.can_send_version('1.36'): + version = '1.36' + msg_kwargs['delete_type'] = delete_type + else: + version = '1.18' + cctxt = self.client.prepare(version=version) + cctxt.cast(ctxt, 'terminate_instance', **msg_kwargs) def soft_delete_instance(self, ctxt, instance, reservations=None): """Soft-delete an instance in its cell. diff --git a/nova/compute/api.py b/nova/compute/api.py index b752709ab0b4..d765bdcbfeba 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -1778,7 +1778,20 @@ class API(base.Base): instance.save() else: self.compute_rpcapi.terminate_instance(context, instance, bdms, - reservations=reservations) + reservations=reservations, + delete_type='delete') + + def _do_force_delete(self, context, instance, bdms, reservations=None, + local=False): + if local: + instance.vm_state = vm_states.DELETED + instance.task_state = None + instance.terminated_at = timeutils.utcnow() + instance.save() + else: + self.compute_rpcapi.terminate_instance(context, instance, bdms, + reservations=reservations, + delete_type='force_delete') def _do_soft_delete(self, context, instance, bdms, reservations=None, local=False): @@ -1854,7 +1867,7 @@ class API(base.Base): @check_instance_state(must_have_launched=False) def force_delete(self, context, instance): """Force delete an instance in any vm_state/task_state.""" - self._delete(context, instance, 'force_delete', self._do_delete, + self._delete(context, instance, 'force_delete', self._do_force_delete, task_state=task_states.DELETING) def force_stop(self, context, instance, do_cast=True, clean_shutdown=True): diff --git a/nova/compute/cells_api.py b/nova/compute/cells_api.py index 8d0c0a34a416..c4701123eb0a 100644 --- a/nova/compute/cells_api.py +++ b/nova/compute/cells_api.py @@ -206,6 +206,9 @@ class ComputeCellsAPI(compute_api.API): """ pass + def force_delete(self, context, instance): + self._handle_cell_delete(context, instance, 'force_delete') + def soft_delete(self, context, instance): self._handle_cell_delete(context, instance, 'soft_delete') @@ -259,12 +262,6 @@ class ComputeCellsAPI(compute_api.API): super(ComputeCellsAPI, self).restore(context, instance) self._cast_to_cells(context, instance, 'restore') - @check_instance_cell - def force_delete(self, context, instance): - """Force delete a previously deleted (but not reclaimed) instance.""" - super(ComputeCellsAPI, self).force_delete(context, instance) - self._cast_to_cells(context, instance, 'force_delete') - @check_instance_cell def evacuate(self, context, instance, *args, **kwargs): """Evacuate the given instance with the provided attributes.""" diff --git a/nova/compute/rpcapi.py b/nova/compute/rpcapi.py index 325a233f24c1..c624b851e84f 100644 --- a/nova/compute/rpcapi.py +++ b/nova/compute/rpcapi.py @@ -778,7 +778,11 @@ class ComputeAPI(object): version=version) cctxt.cast(ctxt, 'suspend_instance', instance=instance) - def terminate_instance(self, ctxt, instance, bdms, reservations=None): + def terminate_instance(self, ctxt, instance, bdms, reservations=None, + delete_type=None): + # NOTE(rajesht): The `delete_type` parameter is passed because + # the method signature has to match with `terminate_instance()` + # method of cells rpcapi. version = '4.0' cctxt = self.client.prepare(server=_compute_host(None, instance), version=version) diff --git a/nova/tests/unit/cells/test_cells_manager.py b/nova/tests/unit/cells/test_cells_manager.py index d4439d3bdaa8..17c2e6b8af77 100644 --- a/nova/tests/unit/cells/test_cells_manager.py +++ b/nova/tests/unit/cells/test_cells_manager.py @@ -785,10 +785,12 @@ class CellsManagerClassTestCase(test.NoDBTestCase): def test_terminate_instance(self): self.mox.StubOutWithMock(self.msg_runner, 'terminate_instance') - self.msg_runner.terminate_instance(self.ctxt, 'fake-instance') + self.msg_runner.terminate_instance(self.ctxt, 'fake-instance', + delete_type='delete') self.mox.ReplayAll() self.cells_manager.terminate_instance(self.ctxt, - instance='fake-instance') + instance='fake-instance', + delete_type='delete') def test_soft_delete_instance(self): self.mox.StubOutWithMock(self.msg_runner, 'soft_delete_instance') diff --git a/nova/tests/unit/cells/test_cells_rpcapi.py b/nova/tests/unit/cells/test_cells_rpcapi.py index 568d9c9d6cc5..cb503fd893ed 100644 --- a/nova/tests/unit/cells/test_cells_rpcapi.py +++ b/nova/tests/unit/cells/test_cells_rpcapi.py @@ -634,10 +634,12 @@ class CellsAPITestCase(test.NoDBTestCase): call_info = self._stub_rpc_method('cast', None) self.cells_rpcapi.terminate_instance(self.fake_context, - 'fake-instance', []) - expected_args = {'instance': 'fake-instance'} + 'fake-instance', [], + delete_type='delete') + expected_args = {'instance': 'fake-instance', + 'delete_type': 'delete'} self._check_result(call_info, 'terminate_instance', - expected_args, version='1.18') + expected_args, version='1.36') def test_soft_delete_instance(self): call_info = self._stub_rpc_method('cast', None) diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index 1d89dacfeef6..144f61f6a667 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -874,7 +874,8 @@ class _ComputeAPIUnitTestMixIn(object): reservations=cast_reservations) elif delete_type in ['delete', 'force_delete']: rpcapi.terminate_instance(self.context, inst, [], - reservations=cast_reservations) + reservations=cast_reservations, + delete_type=delete_type) if commit_quotas: # Local delete or when we're testing API cell. @@ -985,7 +986,7 @@ class _ComputeAPIUnitTestMixIn(object): rpcapi.terminate_instance( self.context, inst, mox.IsA(objects.BlockDeviceMappingList), - reservations=None) + reservations=None, delete_type='delete') else: compute_utils.notify_about_instance_usage( self.compute_api.notifier, self.context, diff --git a/nova/tests/unit/compute/test_compute_cells.py b/nova/tests/unit/compute/test_compute_cells.py index a4ced5f3667a..0ece1aa66f3d 100644 --- a/nova/tests/unit/compute/test_compute_cells.py +++ b/nova/tests/unit/compute/test_compute_cells.py @@ -138,17 +138,18 @@ class CellsComputeAPITestCase(test_compute.ComputeAPITestCase): def test_error_evacuate(self): self.skipTest("Test is incompatible with cells.") - def test_delete_instance_no_cell(self): + def _test_delete_instance_no_cell(self, method_name): cells_rpcapi = self.compute_api.cells_rpcapi self.mox.StubOutWithMock(cells_rpcapi, 'instance_delete_everywhere') inst = self._create_fake_instance_obj() + delete_type = method_name == 'soft_delete' and 'soft' or 'hard' cells_rpcapi.instance_delete_everywhere(self.context, - inst, 'hard') + inst, delete_type) self.mox.ReplayAll() self.stubs.Set(self.compute_api.network_api, 'deallocate_for_instance', lambda *a, **kw: None) - self.compute_api.delete(self.context, inst) + getattr(self.compute_api, method_name)(self.context, inst) def test_delete_instance_no_cell_constraint_failure_does_not_loop(self): with mock.patch.object(self.compute_api.cells_rpcapi, @@ -241,16 +242,13 @@ class CellsComputeAPITestCase(test_compute.ComputeAPITestCase): _test() def test_soft_delete_instance_no_cell(self): - cells_rpcapi = self.compute_api.cells_rpcapi - self.mox.StubOutWithMock(cells_rpcapi, - 'instance_delete_everywhere') - inst = self._create_fake_instance_obj() - cells_rpcapi.instance_delete_everywhere(self.context, - inst, 'soft') - self.mox.ReplayAll() - self.stubs.Set(self.compute_api.network_api, 'deallocate_for_instance', - lambda *a, **kw: None) - self.compute_api.soft_delete(self.context, inst) + self._test_delete_instance_no_cell('soft_delete') + + def test_delete_instance_no_cell(self): + self._test_delete_instance_no_cell('delete') + + def test_force_delete_instance_no_cell(self): + self._test_delete_instance_no_cell('force_delete') def test_get_migrations(self): filters = {'cell_name': 'ChildCell', 'status': 'confirmed'}