Fix missing instance.delete notification

The I8742071b55f018f864f5a382de20075a5b444a79 introduced cases when an
instance object is destroyed without the instance.delete notification
being emitted.

This patch adds the necessary notification to restore legacy
behaviour.

Closes-Bug: #1665263
Change-Id: I55ce90ca34c927c5dcd340fa5bdb0607a4ad4971
(cherry picked from commit f9ac9531fb3259fe83642dce92d0d1901d0f067e)

diff --git a/nova/compute/api.py b/nova/compute/api.py
index 3a665d3198a7a6fa1621fa1fe6142de403a23d3d..fee89d42500c023fb9f5b413977f4af9f562f4d6 100644 (file)
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -1740,7 +1740,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:
@@ -1787,7 +1789,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 0f7692b85679626fe285a11cb8c29980678ceffa..48158d1ab790409a3912c9fcf2ad0523c3743b95 100644 (file)
--- 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
@@ -697,3 +698,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 3b1ba23d0510acbe82b4d07c7d304f884d66f02f..4a33e8faee1feb6859787faf678cb719b2e779f4 100644 (file)
--- a/nova/conductor/manager.py
+++ b/nova/conductor/manager.py
@@ -935,7 +935,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.
@@ -960,13 +960,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
@@ -985,18 +987,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 b3cec9f60720ceabdc8b65e118b6389f0b6d3214..daffcf199fac363ff2b3477614241a3b114e2770 100644 (file)
--- a/nova/tests/unit/compute/test_compute.py
+++ b/nova/tests/unit/compute/test_compute.py
@@ -7762,6 +7762,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 7e27b5e2aac0b118cbd6e36f0fe85fa3b77689fe..c2d2ee0437b6c4cb84c22208c70cd1b75debacd0 100644 (file)
--- 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 10d601b1f4d7b6db4d4b9ab557f6f6d5e19a0f72..c692c12940ecec3c9ef4bb3179121e50053d0959 100644 (file)
--- 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 1a4e3258167a7f1492c9ab7e062bfc5f3cb519f2..485e681ccd331e59856e97468e3a35bceaa29a16 100644 (file)
--- 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
 
@@ -1538,6 +1539,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')
@@ -1545,8 +1547,69 @@ 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',
+                                             '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)
+
+        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',
@@ -1555,6 +1618,10 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase):
         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')

diff --git a/nova/tests/unit/utils.py b/nova/tests/unit/utils.py
index 44bbd8bdce2724ff7e555a9024ec32f9c54b9e5f..1d5da29c5cbf7afd90533bfdcf89c0d898c88251 100644 (file)
--- 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={})])