Handle instance = None in _local_delete_cleanup

Change I4d3193d8401614311010ed0e055fcb3aaeeebaed added some
additional local delete cleanup to prevent leaking of placement
allocations. The change introduced a regression in our "delete while
booting" handling as the _local_delete_cleanup required a valid
instance object to do its work and in two cases, we could have
instance = None from _lookup_instance if we are racing with a create
request and the conductor has deleted the instance record while we
are in the middle of processing the delete request.

This handles those scenarios by doing two things:

  (1) Changing the _local_delete_cleanup and
      _update_queued_for_deletion methods to take an instance UUID
      instead of a full instance object as they really only need the
      UUID to do their work
  (2) Saving a copy of the instance UUID before doing another instance
      lookup which might return None and passing that UUID to the
      _local_delete_cleanup and _update_queued_for_deletion methods

Closes-Bug: #1914777

Change-Id: I03cf285ad83e09d88cdb702a88dfed53c01610f8
(cherry picked from commit 123f626)

Author: melwitt

nova/compute/api.py

@@ -2179,22 +2179,22 @@ def _delete_while_booting(self, context, instance):
             return True
         return False
 
-    def _local_delete_cleanup(self, context, instance):
+    def _local_delete_cleanup(self, context, instance_uuid):
         # NOTE(aarents) Ensure instance allocation is cleared and instance
         # mapping queued as deleted before _delete() return
         try:
             self.placementclient.delete_allocation_for_instance(
-                context, instance.uuid)
+                context, instance_uuid)
         except exception.AllocationDeleteFailed:
             LOG.info("Allocation delete failed during local delete cleanup.",
-                     instance=instance)
+                     instance_uuid=instance_uuid)
 
         try:
-            self._update_queued_for_deletion(context, instance, True)
+            self._update_queued_for_deletion(context, instance_uuid, True)
         except exception.InstanceMappingNotFound:
             LOG.info("Instance Mapping does not exist while attempting "
                      "local delete cleanup.",
-                     instance=instance)
+                     instance_uuid=instance_uuid)
 
     def _attempt_delete_of_buildrequest(self, context, instance):
         # If there is a BuildRequest then the instance may not have been
@@ -2231,7 +2231,7 @@ def _delete(self, context, instance, delete_type, cb, **instance_attrs):
         if not instance.host and not may_have_ports_or_volumes:
             try:
                 if self._delete_while_booting(context, instance):
-                    self._local_delete_cleanup(context, instance)
+                    self._local_delete_cleanup(context, instance.uuid)
                     return
                 # If instance.host was not set it's possible that the Instance
                 # object here was pulled from a BuildRequest object and is not
@@ -2240,6 +2240,11 @@ def _delete(self, context, instance, delete_type, cb, **instance_attrs):
                 # properly. A lookup is attempted which will either return a
                 # full Instance or None if not found. If not found then it's
                 # acceptable to skip the rest of the delete processing.
+
+                # Save a copy of the instance UUID early, in case
+                # _lookup_instance returns instance = None, to pass to
+                # _local_delete_cleanup if needed.
+                instance_uuid = instance.uuid
                 cell, instance = self._lookup_instance(context, instance.uuid)
                 if cell and instance:
                     try:
@@ -2250,11 +2255,11 @@ def _delete(self, context, instance, delete_type, cb, **instance_attrs):
                     except exception.InstanceNotFound:
                         pass
                     # The instance was deleted or is already gone.
-                    self._local_delete_cleanup(context, instance)
+                    self._local_delete_cleanup(context, instance.uuid)
                     return
                 if not instance:
                     # Instance is already deleted.
-                    self._local_delete_cleanup(context, instance)
+                    self._local_delete_cleanup(context, instance_uuid)
                     return
             except exception.ObjectActionError:
                 # NOTE(melwitt): This means the instance.host changed
@@ -2267,7 +2272,7 @@ def _delete(self, context, instance, delete_type, cb, **instance_attrs):
                 cell, instance = self._lookup_instance(context, instance.uuid)
                 if not instance:
                     # Instance is already deleted
-                    self._local_delete_cleanup(context, instance)
+                    self._local_delete_cleanup(context, instance_uuid)
                     return
 
         bdms = objects.BlockDeviceMappingList.get_by_instance_uuid(
@@ -2311,7 +2316,7 @@ def _delete(self, context, instance, delete_type, cb, **instance_attrs):
                              'field, its vm_state is %(state)s.',
                              {'state': instance.vm_state},
                               instance=instance)
-                    self._local_delete_cleanup(context, instance)
+                    self._local_delete_cleanup(context, instance.uuid)
                     return
                 except exception.ObjectActionError as ex:
                     # The instance's host likely changed under us as
@@ -2496,7 +2501,7 @@ def _local_delete(self, context, instance, bdms, delete_type, cb):
             instance.destroy()
 
     @staticmethod
-    def _update_queued_for_deletion(context, instance, qfd):
+    def _update_queued_for_deletion(context, instance_uuid, 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
@@ -2505,7 +2510,7 @@ def _update_queued_for_deletion(context, instance, qfd):
         # 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)
+                                                          instance_uuid)
         im.queued_for_delete = qfd
         im.save()
 
@@ -2517,7 +2522,7 @@ def _do_delete(self, context, instance, bdms, local=False):
             instance.save()
         else:
             self.compute_rpcapi.terminate_instance(context, instance, bdms)
-        self._update_queued_for_deletion(context, instance, True)
+        self._update_queued_for_deletion(context, instance.uuid, True)
 
     def _do_soft_delete(self, context, instance, bdms, local=False):
         if local:
@@ -2527,7 +2532,7 @@ def _do_soft_delete(self, context, instance, bdms, local=False):
             instance.save()
         else:
             self.compute_rpcapi.soft_delete_instance(context, instance)
-        self._update_queued_for_deletion(context, instance, True)
+        self._update_queued_for_deletion(context, instance.uuid, True)
 
     # NOTE(maoy): we allow delete to be called no matter what vm_state says.
     @check_instance_lock
@@ -2580,7 +2585,7 @@ def restore(self, context, instance):
             instance.task_state = None
             instance.deleted_at = None
             instance.save(expected_task_state=[None])
-        self._update_queued_for_deletion(context, instance, False)
+        self._update_queued_for_deletion(context, instance.uuid, False)
 
     @check_instance_lock
     @check_instance_state(task_state=None,

nova/tests/functional/regressions/test_bug_1914777.py

@@ -17,7 +17,6 @@
 from nova import objects
 from nova import test
 from nova.tests import fixtures as nova_fixtures
-from nova.tests.functional.api import client
 from nova.tests.functional import integrated_helpers
 from nova.tests.unit import policy_fixture
 
@@ -79,14 +78,8 @@ def test_build_request_and_instance_not_found(self, mock_get_br):
         # while booting" in compute/api fails after a different request has
         # deleted it.
         br_not_found = exception.BuildRequestNotFound(uuid=self.server['id'])
-        mock_get_br.side_effect = [self.br, br_not_found]
-        # FIXME(melwitt): Delete request fails due to the AttributeError.
-        ex = self.assertRaises(
-            client.OpenStackApiException, self._delete_server, self.server)
-        self.assertEqual(500, ex.response.status_code)
-        self.assertIn('AttributeError', str(ex))
-        # FIXME(melwitt): Uncomment when the bug is fixed.
-        # self._delete_server(self.server)
+        mock_get_br.side_effect = [self.br, br_not_found, br_not_found]
+        self._delete_server(self.server)
 
     @mock.patch('nova.objects.build_request.BuildRequest.get_by_instance_uuid')
     @mock.patch('nova.objects.InstanceMapping.get_by_instance_uuid')
@@ -119,15 +112,15 @@ def test_deleting_instance_at_the_same_time(self, mock_get_i, mock_get_im,
             context=self.ctxt, instance_uuid=self.server['id'],
             cell_mapping=self.cell_mappings['cell1'])
         mock_get_im.side_effect = [
-            no_cell_im, has_cell_im, has_cell_im, has_cell_im]
+            no_cell_im, has_cell_im, has_cell_im, has_cell_im, has_cell_im]
         # Simulate that the instance object has been created by the conductor
         # in the create path while the delete request is being processed.
         # First lookups are before the instance has been deleted and the last
         # lookup is after the conductor has deleted the instance. Use the build
         # request to make an instance object for testing.
         i = self.br.get_new_instance(self.ctxt)
         i_not_found = exception.InstanceNotFound(instance_id=self.server['id'])
-        mock_get_i.side_effect = [i, i, i, i_not_found]
+        mock_get_i.side_effect = [i, i, i, i_not_found, i_not_found]
 
         # Simulate that the conductor is running instance_destroy at the same
         # time as we are.
@@ -142,10 +135,4 @@ def fake_instance_destroy(*args, **kwargs):
 
         self.stub_out(
             'nova.objects.instance.Instance.destroy', fake_instance_destroy)
-        # FIXME(melwitt): Delete request fails due to the AttributeError.
-        ex = self.assertRaises(
-            client.OpenStackApiException, self._delete_server, self.server)
-        self.assertEqual(500, ex.response.status_code)
-        self.assertIn('AttributeError', str(ex))
-        # FIXME(melwitt): Uncomment when the bug is fixed.
-        # self._delete_server(self.server)
+        self._delete_server(self.server)

nova/tests/unit/compute/test_api.py

@@ -4211,7 +4211,7 @@ def test_restore_by_admin(self, update_qfd, action_start, instance_save,
                             'cores': 1 + instance.flavor.vcpus,
                             'ram': 512 + instance.flavor.memory_mb},
             project_id=instance.project_id)
-        update_qfd.assert_called_once_with(admin_context, instance, False)
+        update_qfd.assert_called_once_with(admin_context, instance.uuid, False)
 
     @mock.patch('nova.objects.Quotas.get_all_by_project_and_user',
                 new=mock.MagicMock())
@@ -4252,7 +4252,7 @@ def test_restore_by_instance_owner(self, update_qfd, action_start,
                             'cores': 1 + instance.flavor.vcpus,
                             'ram': 512 + instance.flavor.memory_mb},
             project_id=instance.project_id)
-        update_qfd.assert_called_once_with(self.context, instance, False)
+        update_qfd.assert_called_once_with(self.context, instance.uuid, False)
 
     @mock.patch.object(objects.InstanceAction, 'action_start')
     def test_external_instance_event(self, mock_action_start):
@@ -5971,7 +5971,8 @@ def test_update_queued_for_deletion(self, mock_get, mock_save):
         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.compute_api._update_queued_for_deletion(
+            self.context, inst.uuid, True)
         self.assertTrue(im.queued_for_delete)
         mock_get.assert_called_once_with(self.context, inst.uuid)
         mock_save.assert_called_once_with()