diff --git a/nova/compute/api.py b/nova/compute/api.py index fdfeaa77482d..e57907ee5942 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -1761,7 +1761,9 @@ class API(base.Base): # If instance is None it has already been deleted. if cell: with nova_context.target_cell(context, cell): - instance.destroy() + with compute_utils.notify_about_instance_delete( + self.notifier, context, instance): + instance.destroy() else: instance.destroy() except exception.InstanceNotFound: @@ -1808,7 +1810,9 @@ class API(base.Base): cell, instance = self._lookup_instance(context, instance.uuid) if cell and instance: with nova_context.target_cell(context, cell): - instance.destroy() + with compute_utils.notify_about_instance_delete( + self.notifier, context, instance): + instance.destroy() return if not instance: # Instance is already deleted. diff --git a/nova/compute/utils.py b/nova/compute/utils.py index 750d3a26be4e..def51d1fa4d2 100644 --- a/nova/compute/utils.py +++ b/nova/compute/utils.py @@ -14,6 +14,7 @@ """Compute-related Utilities and helpers.""" +import contextlib import functools import inspect import itertools @@ -700,3 +701,18 @@ class UnlimitedSemaphore(object): @property def balance(self): return 0 + + +@contextlib.contextmanager +def notify_about_instance_delete(notifier, context, instance): + # Pre-load system_metadata because if this context is around an + # instance.destroy(), lazy-loading it later would result in an + # InstanceNotFound error. + system_metadata = instance.system_metadata + try: + notify_about_instance_usage(notifier, context, instance, + "delete.start") + yield + finally: + notify_about_instance_usage(notifier, context, instance, "delete.end", + system_metadata=system_metadata) diff --git a/nova/conductor/manager.py b/nova/conductor/manager.py index 291be9d5ed50..2386ac45106a 100644 --- a/nova/conductor/manager.py +++ b/nova/conductor/manager.py @@ -970,7 +970,7 @@ class ComputeTaskManager(base.Base): inst_mapping.save() if not self._delete_build_request( - build_request, instance, cell, instance_bdms): + context, build_request, instance, cell, instance_bdms): # The build request was deleted before/during scheduling so # the instance is gone and we don't have anything to build for # this one. @@ -995,13 +995,15 @@ class ComputeTaskManager(base.Base): host=host['host'], node=host['nodename'], limits=host['limits']) - def _delete_build_request(self, build_request, instance, cell, + def _delete_build_request(self, context, build_request, instance, cell, instance_bdms): """Delete a build request after creating the instance in the cell. This method handles cleaning up the instance in case the build request is already deleted by the time we try to delete it. + :param context: the context of the request being handled + :type context: nova.context.RequestContext' :param build_request: the build request to delete :type build_request: nova.objects.BuildRequest :param instance: the instance created from the build_request @@ -1020,18 +1022,20 @@ class ComputeTaskManager(base.Base): # processed, and the build should halt here. Clean up the # bdm and instance record. with obj_target_cell(instance, cell): - try: - instance.destroy() - except exception.InstanceNotFound: - pass - except exception.ObjectActionError: - # NOTE(melwitt): Instance became scheduled during - # the destroy, "host changed". Refresh and re-destroy. + with compute_utils.notify_about_instance_delete( + self.notifier, context, instance): try: - instance.refresh() instance.destroy() except exception.InstanceNotFound: pass + except exception.ObjectActionError: + # NOTE(melwitt): Instance became scheduled during + # the destroy, "host changed". Refresh and re-destroy. + try: + instance.refresh() + instance.destroy() + except exception.InstanceNotFound: + pass for bdm in instance_bdms: with obj_target_cell(bdm, cell): try: diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 79aa946fce7b..034111bced6c 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -7823,6 +7823,100 @@ class ComputeTestCase(BaseTestCase): block_device_mapping=[]) self.assertEqual('Preserve this', instance.fault.message) + @mock.patch('nova.objects.Instance.destroy') + @mock.patch('nova.context.target_cell') + @mock.patch('nova.objects.InstanceMapping.get_by_instance_uuid') + @mock.patch('nova.objects.Service.get_minimum_version') + @mock.patch('nova.compute.utils.notify_about_instance_usage') + @mock.patch('nova.objects.BuildRequest.get_by_instance_uuid') + def test_delete_while_booting_instance_not_in_cell_db_cellsv2( + self, br_get_by_instance, notify, minimum_server_version, + im_get_by_instance, target_cell, instance_destroy): + + minimum_server_version.return_value = 15 + im_get_by_instance.return_value = mock.Mock() + + instance = self._create_fake_instance_obj() + instance.host = None + instance.save() + + self.compute_api._delete_instance(self.context, instance) + + instance_destroy.assert_called_once_with() + + # the instance is updated during the delete so we only match by uuid + test_utils.assert_instance_delete_notification_by_uuid( + notify, instance.uuid, self.compute_api.notifier, self.context) + + @mock.patch('nova.objects.Instance.destroy') + @mock.patch('nova.objects.Service.get_minimum_version') + @mock.patch('nova.compute.utils.notify_about_instance_usage') + @mock.patch('nova.objects.BuildRequest.get_by_instance_uuid') + def test_delete_while_booting_instance_not_in_cell_db_cellsv1( + self, br_get_by_instance, notify, minimum_server_version, + instance_destroy): + + minimum_server_version.return_value = 14 + + instance = self._create_fake_instance_obj() + instance.host = None + instance.save() + + self.compute_api._delete_instance(self.context, instance) + + test_utils.assert_instance_delete_notification_by_uuid( + notify, instance.uuid, self.compute_api.notifier, self.context) + + @mock.patch('nova.objects.Instance.destroy') + @mock.patch('nova.objects.InstanceMapping.get_by_instance_uuid') + @mock.patch('nova.compute.utils.notify_about_instance_usage') + @mock.patch('nova.objects.BuildRequest.get_by_instance_uuid') + def test_delete_while_booting_instance_not_scheduled_cellv1( + self, br_get_by_instance, notify, im_get_by_instance, + instance_destroy): + + instance = self._create_fake_instance_obj() + instance.host = None + instance.save() + + # This means compute api looks for an instance to destroy + br_get_by_instance.side_effect = exception.BuildRequestNotFound( + uuid=instance.uuid) + + # no mapping means cellv1 + im_get_by_instance.return_value = None + + self.compute_api._delete_instance(self.context, instance) + + test_utils.assert_instance_delete_notification_by_uuid( + notify, instance.uuid, self.compute_api.notifier, self.context) + + @mock.patch('nova.objects.Instance.destroy') + @mock.patch('nova.context.target_cell') + @mock.patch('nova.objects.InstanceMapping.get_by_instance_uuid') + @mock.patch('nova.compute.utils.notify_about_instance_usage') + @mock.patch('nova.objects.BuildRequest.get_by_instance_uuid') + def test_delete_while_booting_instance_not_scheduled_cellv2( + self, br_get_by_instance, notify, im_get_by_instance, target_cell, + instance_destroy): + + instance = self._create_fake_instance_obj() + instance.host = None + instance.save() + + # This means compute api looks for an instance to destroy + br_get_by_instance.side_effect = exception.BuildRequestNotFound( + uuid=instance.uuid) + + # having a mapping means cellsv2 + im_get_by_instance.return_value = mock.Mock() + + self.compute_api._delete_instance(self.context, instance) + + instance_destroy.assert_called_once_with() + test_utils.assert_instance_delete_notification_by_uuid( + notify, instance.uuid, self.compute_api.notifier, self.context) + class ComputeAPITestCase(BaseTestCase): def setUp(self): diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index 75041fa5a9dc..900a4e62b5c2 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -1209,8 +1209,7 @@ class _ComputeAPIUnitTestMixIn(object): constraint='constraint').AndReturn(fake_inst) compute_utils.notify_about_instance_usage( self.compute_api.notifier, self.context, - inst, 'delete.end', - system_metadata=inst.system_metadata) + inst, 'delete.end', system_metadata={}) self.mox.ReplayAll() diff --git a/nova/tests/unit/compute/test_compute_utils.py b/nova/tests/unit/compute/test_compute_utils.py index 10d601b1f4d7..c692c12940ec 100644 --- a/nova/tests/unit/compute/test_compute_utils.py +++ b/nova/tests/unit/compute/test_compute_utils.py @@ -835,6 +835,23 @@ class ComputeUtilsTestCase(test.NoDBTestCase): self.assertEqual([], addresses) mock_ifaddresses.assert_called_once_with(iface) + @mock.patch('nova.compute.utils.notify_about_instance_usage') + @mock.patch('nova.objects.Instance.destroy') + def test_notify_about_instance_delete(self, mock_instance_destroy, + mock_notify_usage): + instance = fake_instance.fake_instance_obj( + self.context, expected_attrs=('system_metadata',)) + with compute_utils.notify_about_instance_delete( + mock.sentinel.notifier, self.context, instance): + instance.destroy() + expected_notify_calls = [ + mock.call(mock.sentinel.notifier, self.context, instance, + 'delete.start'), + mock.call(mock.sentinel.notifier, self.context, instance, + 'delete.end', system_metadata=instance.system_metadata) + ] + mock_notify_usage.assert_has_calls(expected_notify_calls) + class ComputeUtilsQuotaDeltaTestCase(test.TestCase): def setUp(self): diff --git a/nova/tests/unit/conductor/test_conductor.py b/nova/tests/unit/conductor/test_conductor.py index 964c936e6733..bd3229334381 100644 --- a/nova/tests/unit/conductor/test_conductor.py +++ b/nova/tests/unit/conductor/test_conductor.py @@ -55,6 +55,7 @@ from nova.tests.unit import fake_instance from nova.tests.unit import fake_notifier from nova.tests.unit import fake_request_spec from nova.tests.unit import fake_server_actions +from nova.tests.unit import utils as test_utils from nova.tests import uuidsentinel as uuids from nova import utils @@ -1554,6 +1555,7 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase): self.assertEqual('error', instance.vm_state) self.assertIsNone(None, instance.task_state) + @mock.patch('nova.compute.utils.notify_about_instance_usage') @mock.patch('nova.compute.rpcapi.ComputeAPI.build_and_run_instance') @mock.patch('nova.scheduler.rpcapi.SchedulerAPI.select_destinations') @mock.patch('nova.objects.BuildRequest.destroy') @@ -1561,7 +1563,9 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase): def test_schedule_and_build_delete_during_scheduling(self, bury, br_destroy, select_destinations, - build_and_run): + build_and_run, + notify): + br_destroy.side_effect = exc.BuildRequestNotFound(uuid='foo') self.start_service('compute', host='fake-host') select_destinations.return_value = [{'host': 'fake-host', @@ -1572,6 +1576,69 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase): self.assertFalse(bury.called) self.assertTrue(br_destroy.called) + test_utils.assert_instance_delete_notification_by_uuid( + notify, self.params['build_requests'][0].instance_uuid, + self.conductor.notifier, self.ctxt) + + @mock.patch('nova.objects.Instance.destroy') + @mock.patch('nova.compute.utils.notify_about_instance_usage') + @mock.patch('nova.compute.rpcapi.ComputeAPI.build_and_run_instance') + @mock.patch('nova.scheduler.rpcapi.SchedulerAPI.select_destinations') + @mock.patch('nova.objects.BuildRequest.destroy') + @mock.patch('nova.conductor.manager.ComputeTaskManager._bury_in_cell0') + def test_schedule_and_build_delete_during_scheduling_host_changed( + self, bury, br_destroy, select_destinations, build_and_run, + notify, instance_destroy): + + br_destroy.side_effect = exc.BuildRequestNotFound(uuid='foo') + instance_destroy.side_effect = [ + exc.ObjectActionError(action='destroy', + reason='host changed'), + None, + ] + + self.start_service('compute', host='fake-host') + select_destinations.return_value = [{'host': 'fake-host', + 'nodename': 'nodesarestupid', + 'limits': None}] + self.conductor.schedule_and_build_instances(**self.params) + self.assertFalse(build_and_run.called) + self.assertFalse(bury.called) + self.assertTrue(br_destroy.called) + self.assertEqual(2, instance_destroy.call_count) + test_utils.assert_instance_delete_notification_by_uuid( + notify, self.params['build_requests'][0].instance_uuid, + self.conductor.notifier, self.ctxt) + + @mock.patch('nova.objects.Instance.destroy') + @mock.patch('nova.compute.utils.notify_about_instance_usage') + @mock.patch('nova.compute.rpcapi.ComputeAPI.build_and_run_instance') + @mock.patch('nova.scheduler.rpcapi.SchedulerAPI.select_destinations') + @mock.patch('nova.objects.BuildRequest.destroy') + @mock.patch('nova.conductor.manager.ComputeTaskManager._bury_in_cell0') + def test_schedule_and_build_delete_during_scheduling_instance_not_found( + self, bury, br_destroy, select_destinations, build_and_run, + notify, instance_destroy): + + br_destroy.side_effect = exc.BuildRequestNotFound(uuid='foo') + instance_destroy.side_effect = [ + exc.InstanceNotFound(instance_id='fake'), + None, + ] + + self.start_service('compute', host='fake-host') + select_destinations.return_value = [{'host': 'fake-host', + 'nodename': 'nodesarestupid', + 'limits': None}] + self.conductor.schedule_and_build_instances(**self.params) + self.assertFalse(build_and_run.called) + self.assertFalse(bury.called) + self.assertTrue(br_destroy.called) + self.assertEqual(1, instance_destroy.call_count) + test_utils.assert_instance_delete_notification_by_uuid( + notify, self.params['build_requests'][0].instance_uuid, + self.conductor.notifier, self.ctxt) + @mock.patch('nova.compute.rpcapi.ComputeAPI.build_and_run_instance') @mock.patch('nova.scheduler.rpcapi.SchedulerAPI.select_destinations') @mock.patch('nova.objects.BuildRequest.get_by_instance_uuid') diff --git a/nova/tests/unit/utils.py b/nova/tests/unit/utils.py index 44bbd8bdce27..1d5da29c5cbf 100644 --- a/nova/tests/unit/utils.py +++ b/nova/tests/unit/utils.py @@ -17,6 +17,7 @@ import platform import socket import sys +import mock from six.moves import range from nova.compute import flavors @@ -291,3 +292,31 @@ def obj_called_with(the_mock, *args, **kwargs): def obj_called_once_with(the_mock, *args, **kwargs): return _obj_called_with(the_mock, *args, **kwargs) == 1 + + +class CustomMockCallMatcher(object): + def __init__(self, comparator): + self.comparator = comparator + + def __eq__(self, other): + return self.comparator(other) + + +def assert_instance_delete_notification_by_uuid( + mock_notify, expected_instance_uuid, expected_notifier, + expected_context): + + match_by_instance_uuid = CustomMockCallMatcher( + lambda instance: + instance.uuid == expected_instance_uuid) + + mock_notify.assert_has_calls([ + mock.call(expected_notifier, + expected_context, + match_by_instance_uuid, + 'delete.start'), + mock.call(expected_notifier, + expected_context, + match_by_instance_uuid, + 'delete.end', + system_metadata={})])