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.

Conflicts:
      nova/compute/api.py
      nova/tests/unit/compute/test_compute_api.py

NOTE(mriedem): The conflict is due to not having change
edf51119fa59ff8a3337abb9107a06fa33d3c68f in stable/ocata.

Change-Id: I5200e72c195e12b5a069cbae3f209492ed569fb4
Closes-Bug: #1678326
(cherry picked from commit 5c1af55cbe526dea72308767df8709064ffae6a8)

diff --git a/nova/compute/api.py b/nova/compute/api.py
index 5abe581e2954a9bd0a8f5b1e44aa7461fa0648a3..210e652f59b4272f5befee65157298cf5b3240c9 100644 (file)
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -1723,8 +1723,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
@@ -1745,6 +1743,7 @@ class API(base.Base):
                                 instance.destroy()
                     else:
                         instance.destroy()
+                    quotas.commit()
             except exception.InstanceNotFound:
                 quotas.rollback()
 
@@ -1805,17 +1804,17 @@ class API(base.Base):
                                                        instance.task_state,
                                                        project_id, user_id,
                                                        flavor=quota_flavor)
-                    quotas.commit()
                     try:
                         with nova_context.target_cell(context, cell):
                             with compute_utils.notify_about_instance_delete(
                                     self.notifier, context, instance):
                                 instance.destroy()
-                            return
+                        # Now commit the quota reservation to decrement usage.
+                        quotas.commit()
                     except exception.InstanceNotFound:
                         quotas.rollback()
-                        # Instance is already deleted.
-                        return
+                    # The instance was deleted or is already gone.
+                    return
                 if not instance:
                     # Instance is already deleted.
                     return

diff --git a/nova/tests/functional/regressions/test_bug_1678326.py b/nova/tests/functional/regressions/test_bug_1678326.py
index e1fc33fd6888a0bdc17bca3063bb0e7bbef0fcfd..1afb6edfc7cfce7b191f093f15ddc2a0e0056bbb 100644 (file)
--- a/nova/tests/functional/regressions/test_bug_1678326.py
+++ b/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'])

diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py
index 2ff2a7f953233c47cda8c2743d7be5eab592f372..49fa1513a90ef6669f11590c795a1d5f9981f21b 100644 (file)
--- a/nova/tests/unit/compute/test_compute_api.py
+++ b/nova/tests/unit/compute/test_compute_api.py
@@ -1459,7 +1459,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)
 
         test()
 
@@ -1479,7 +1479,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()
@@ -1593,7 +1593,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()
             expected_target_cell_calls = [
                 # Get the instance.flavor.
                 mock.call(self.context, cell0),