Commit usage decrement after destroying instance

This fixes a regression in Ocata where we were always
decrementing quota usage during instance delete even
if we failed to delete the instance. Now the reservation
is properly committed after the instance is destroyed.

The related functional test is updated to show this working
correctly now.

Change-Id: I5200e72c195e12b5a069cbae3f209492ed569fb4
Closes-Bug: #1678326

nova/compute/api.py

@@ -1781,8 +1781,6 @@ class API(base.Base):
                                                instance.task_state,
                                                project_id, user_id)
             try:
-                quotas.commit()
-
                 # NOTE(alaski): Though the conductor halts the build process it
                 # does not currently delete the instance record. This is
                 # because in the near future the instance record will not be
@@ -1793,7 +1791,8 @@ class API(base.Base):
                 # Look up the instance because the current instance object was
                 # stashed on the buildrequest and therefore not complete enough
                 # to run .destroy().
-                cell, instance = self._lookup_instance(context, instance.uuid)
+                instance_uuid = instance.uuid
+                cell, instance = self._lookup_instance(context, instance_uuid)
                 if instance is not None:
                     # If instance is None it has already been deleted.
                     if cell:
@@ -1803,6 +1802,11 @@ class API(base.Base):
                                 instance.destroy()
                     else:
                         instance.destroy()
+                    quotas.commit()
+                else:
+                    # The instance is already deleted so rollback the quota
+                    # usage decrement reservation in the not found block below.
+                    raise exception.InstanceNotFound(instance_id=instance_uuid)
             except exception.InstanceNotFound:
                 quotas.rollback()
 
@@ -1875,7 +1879,6 @@ class API(base.Base):
                         quotas = self._create_reservations(
                             context, instance, instance.task_state,
                             project_id, user_id, flavor=quota_flavor)
-                        quotas.commit()
 
                     try:
                         # Now destroy the instance from the cell it lives in.
@@ -1885,12 +1888,14 @@ class API(base.Base):
                             with compute_utils.notify_about_instance_delete(
                                     self.notifier, context, instance):
                                 instance.destroy()
-                            return
+                        # Now commit the quota reservation to decrement usage.
+                        with nova_context.target_cell(context, None):
+                            quotas.commit()
                     except exception.InstanceNotFound:
                         with nova_context.target_cell(context, None):
                             quotas.rollback()
-                            # Instance is already deleted.
-                            return
+                    # The instance was deleted or is already gone.
+                    return
                 if not instance:
                     # Instance is already deleted.
                     return

nova/tests/functional/regressions/test_bug_1678326.py

@@ -62,9 +62,7 @@ class TestDeleteFromCell0CheckQuotaRollback(
                          current_usage['absolute']['totalInstancesUsed'])
 
         # Now we stub out instance.destroy to fail with InstanceNotFound
-        # which triggers the quotas.rollback call, which until the bug is
-        # fixed actually does nothing because we have already committed the
-        # reservation before we actually tried to destroy the instance.
+        # which triggers the quotas.rollback call.
         original_instance_destroy = objects.Instance.destroy
 
         def fake_instance_destroy(*args, **kwargs):
@@ -79,8 +77,7 @@ class TestDeleteFromCell0CheckQuotaRollback(
         self.stub_out('nova.objects.instance.Instance.destroy',
                       original_instance_destroy)
 
-        # Now check the quota again. Until the bug is fixed, quota usage will
-        # have been decremented even though we failed to delete the instance.
+        # Now check the quota again. We should be back to the pre-delete usage.
         ending_usage = self.api.get_limits()
-        self.assertEqual(current_usage['absolute']['totalInstancesUsed'] - 1,
+        self.assertEqual(current_usage['absolute']['totalInstancesUsed'],
                          ending_usage['absolute']['totalInstancesUsed'])

nova/tests/unit/compute/test_compute_api.py

@@ -1464,7 +1464,8 @@ class _ComputeAPIUnitTestMixIn(object):
             self.assertTrue(
                 self.compute_api._delete_while_booting(self.context,
                                                        inst))
-            self.assertTrue(quota_mock.commit.called)
+            self.assertFalse(quota_mock.commit.called)
+            quota_mock.rollback.assert_called_once_with()
 
         test()
 
@@ -1484,7 +1485,7 @@ class _ComputeAPIUnitTestMixIn(object):
             self.assertTrue(
                 self.compute_api._delete_while_booting(self.context,
                                                        inst))
-            self.assertTrue(quota_mock.commit.called)
+            self.assertFalse(quota_mock.commit.called)
             self.assertTrue(quota_mock.rollback.called)
 
         test()
@@ -1600,7 +1601,6 @@ class _ComputeAPIUnitTestMixIn(object):
                 self.context, instance, instance.task_state,
                 self.context.project_id, instance.user_id,
                 flavor=instance.flavor)
-            quota_mock.commit.assert_called_once_with()
             notify_mock.assert_called_once_with(
                 self.compute_api.notifier, self.context, instance)
             destroy_mock.assert_called_once_with()