Fix quota issues with instance deletes.

In order to keep quotas in sync as much as possible, only commit quota
changes for delete when:

1) An instance's vm_state is updated to be SOFT_DELETED.
2) The DB record is marked as deleted (and the instance's vm_state is
   not SOFT_DELETED)

If a host is down and we delete the instance in the API, this means
quotas are committed within the API.  Otherwise, quotas are committed
on the manager side.

Fixes bug 1098380

Also needed for proper testing: Fixed compute cells tests so that pseudo
child cells use NoopQuotaDriver.  This uncovered inconsistencies in the
NoopQuotaDriver wrt the DBQuotaDriver.  Those issues were fixed as well.

Change-Id: Ib72de1a457f0c5056d55a5c7dd4d8d7c69708996

4 files changed, 118 insertions, 37 deletions

diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py
index e94d8b788..ec35f2c5a 100644
--- a/nova/tests/compute/test_compute.py
+++ b/nova/tests/compute/test_compute.py
@@ -1828,7 +1828,8 @@ class ComputeTestCase(BaseTestCase):
         """
         instance = self._create_fake_instance()
 
-        def fake_delete_instance(context, instance, bdms):
+        def fake_delete_instance(context, instance, bdms,
+                                 reservations=None):
             raise exception.InstanceTerminationFailure(reason='')
 
         self.stubs.Set(self.compute, '_delete_instance',
@@ -1990,6 +1991,59 @@ class ComputeTestCase(BaseTestCase):
         self.mox.ReplayAll()
         return reservations
 
+    def test_quotas_succesful_delete(self):
+        instance = jsonutils.to_primitive(self._create_fake_instance())
+        resvs = self._ensure_quota_reservations_committed()
+        self.compute.terminate_instance(self.context, instance,
+                                        bdms=None, reservations=resvs)
+
+    def test_quotas_failed_delete(self):
+        instance = jsonutils.to_primitive(self._create_fake_instance())
+
+        def fake_shutdown_instance(*args, **kwargs):
+            raise test.TestingException()
+
+        self.stubs.Set(self.compute, '_shutdown_instance',
+                       fake_shutdown_instance)
+
+        resvs = self._ensure_quota_reservations_rolledback()
+        self.assertRaises(test.TestingException,
+                          self.compute.terminate_instance,
+                          self.context, instance,
+                          bdms=None, reservations=resvs)
+
+    def test_quotas_succesful_soft_delete(self):
+        instance = jsonutils.to_primitive(self._create_fake_instance(
+            params=dict(task_state=task_states.SOFT_DELETING)))
+        resvs = self._ensure_quota_reservations_committed()
+        self.compute.soft_delete_instance(self.context, instance,
+                                          reservations=resvs)
+
+    def test_quotas_failed_soft_delete(self):
+        instance = jsonutils.to_primitive(self._create_fake_instance(
+            params=dict(task_state=task_states.SOFT_DELETING)))
+
+        def fake_soft_delete(*args, **kwargs):
+            raise test.TestingException()
+
+        self.stubs.Set(self.compute.driver, 'soft_delete',
+                       fake_soft_delete)
+
+        resvs = self._ensure_quota_reservations_rolledback()
+        self.assertRaises(test.TestingException,
+                          self.compute.soft_delete_instance,
+                          self.context, instance,
+                          reservations=resvs)
+
+    def test_quotas_destroy_of_soft_deleted_instance(self):
+        instance = jsonutils.to_primitive(self._create_fake_instance(
+            params=dict(vm_state=vm_states.SOFT_DELETED)))
+        # Termination should be successful, but quota reservations
+        # rolled back because the instance was in SOFT_DELETED state.
+        resvs = self._ensure_quota_reservations_rolledback()
+        self.compute.terminate_instance(self.context, instance,
+                                        bdms=None, reservations=resvs)
+
     def test_finish_resize(self):
         # Contrived test to ensure finish_resize doesn't raise anything.
 
@@ -4302,33 +4356,6 @@ class ComputeAPITestCase(BaseTestCase):
         self.assertEqual(instance['task_state'], None)
         self.assertTrue(instance['deleted'])
 
-    def test_repeated_delete_quota(self):
-        in_use = {'instances': 1}
-
-        def fake_reserve(context, expire=None, project_id=None, **deltas):
-            return dict(deltas.iteritems())
-
-        self.stubs.Set(QUOTAS, 'reserve', fake_reserve)
-
-        def fake_commit(context, deltas, project_id=None):
-            for k, v in deltas.iteritems():
-                in_use[k] = in_use.get(k, 0) + v
-
-        self.stubs.Set(QUOTAS, 'commit', fake_commit)
-
-        instance, instance_uuid = self._run_instance(params={
-                'host': CONF.host})
-
-        self.compute_api.delete(self.context, instance)
-        self.compute_api.delete(self.context, instance)
-
-        instance = db.instance_get_by_uuid(self.context, instance_uuid)
-        self.assertEqual(instance['task_state'], task_states.DELETING)
-
-        self.assertEquals(in_use['instances'], 0)
-
-        db.instance_destroy(self.context, instance['uuid'])
-
     def test_delete_fast_if_host_not_set(self):
         instance = self._create_fake_instance({'host': None})
         self.compute_api.delete(self.context, instance)
@@ -4363,9 +4390,8 @@ class ComputeAPITestCase(BaseTestCase):
         instance, instance_uuid = self._run_instance(params={
                 'host': CONF.host})
 
+        # Make sure this is not called on the API side.
         self.mox.StubOutWithMock(nova.quota.QUOTAS, 'commit')
-        nova.quota.QUOTAS.commit(mox.IgnoreArg(), mox.IgnoreArg(),
-                                 project_id=mox.IgnoreArg())
         self.mox.ReplayAll()
 
         self.compute_api.soft_delete(self.context, instance)
@@ -4521,9 +4547,6 @@ class ComputeAPITestCase(BaseTestCase):
         # Ensure quotas are committed
         self.mox.StubOutWithMock(nova.quota.QUOTAS, 'commit')
         nova.quota.QUOTAS.commit(mox.IgnoreArg(), mox.IgnoreArg())
-        if self.__class__.__name__ == 'CellsComputeAPITestCase':
-            # Called a 2nd time (for the child cell) when testing cells
-            nova.quota.QUOTAS.commit(mox.IgnoreArg(), mox.IgnoreArg())
         self.mox.ReplayAll()
 
         self.compute_api.restore(self.context, instance)
diff --git a/nova/tests/compute/test_compute_cells.py b/nova/tests/compute/test_compute_cells.py
index 8ba35e033..190d75a9d 100644
--- a/nova/tests/compute/test_compute_cells.py
+++ b/nova/tests/compute/test_compute_cells.py
@@ -18,10 +18,12 @@ Tests For Compute w/ Cells
 """
 import functools
 
+from nova.compute import api as compute_api
 from nova.compute import cells_api as compute_cells_api
 from nova import db
 from nova.openstack.common import jsonutils
 from nova.openstack.common import log as logging
+from nova import quota
 from nova.tests.compute import test_compute
 
 
@@ -40,7 +42,16 @@ def stub_call_to_cells(context, instance, method, *args, **kwargs):
                 dict(vm_state=instance['vm_state'],
                      task_state=instance['task_state']))
 
-    return fn(context, instance, *args, **kwargs)
+    # Use NoopQuotaDriver in child cells.
+    saved_quotas = quota.QUOTAS
+    quota.QUOTAS = quota.QuotaEngine(
+            quota_driver_class=quota.NoopQuotaDriver())
+    compute_api.QUOTAS = quota.QUOTAS
+    try:
+        return fn(context, instance, *args, **kwargs)
+    finally:
+        quota.QUOTAS = saved_quotas
+        compute_api.QUOTAS = saved_quotas
 
 
 def stub_cast_to_cells(context, instance, method, *args, **kwargs):
@@ -52,7 +63,17 @@ def stub_cast_to_cells(context, instance, method, *args, **kwargs):
         db.instance_update(context, instance['uuid'],
                 dict(vm_state=instance['vm_state'],
                      task_state=instance['task_state']))
-    fn(context, instance, *args, **kwargs)
+
+    # Use NoopQuotaDriver in child cells.
+    saved_quotas = quota.QUOTAS
+    quota.QUOTAS = quota.QuotaEngine(
+            quota_driver_class=quota.NoopQuotaDriver())
+    compute_api.QUOTAS = quota.QUOTAS
+    try:
+        fn(context, instance, *args, **kwargs)
+    finally:
+        quota.QUOTAS = saved_quotas
+        compute_api.QUOTAS = saved_quotas
 
 
 def deploy_stubs(stubs, api, original_instance=None):
diff --git a/nova/tests/compute/test_rpcapi.py b/nova/tests/compute/test_rpcapi.py
index 6c40a95e2..a089e9dc6 100644
--- a/nova/tests/compute/test_rpcapi.py
+++ b/nova/tests/compute/test_rpcapi.py
@@ -221,7 +221,9 @@ class ComputeRpcAPITestCase(test.TestCase):
 
     def test_soft_delete_instance(self):
         self._test_compute_api('soft_delete_instance', 'cast',
-                instance=self.fake_instance)
+                instance=self.fake_instance,
+                reservations=['uuid1', 'uuid2'],
+                version='2.27')
 
     def test_restore_instance(self):
         self._test_compute_api('restore_instance', 'cast',
@@ -368,7 +370,8 @@ class ComputeRpcAPITestCase(test.TestCase):
     def test_terminate_instance(self):
         self._test_compute_api('terminate_instance', 'cast',
                 instance=self.fake_instance, bdms=[],
-                version='2.4')
+                reservations=['uuid1', 'uuid2'],
+                version='2.27')
 
     def test_unpause_instance(self):
         self._test_compute_api('unpause_instance', 'cast',
diff --git a/nova/tests/conductor/test_conductor.py b/nova/tests/conductor/test_conductor.py
index 72c04e427..00f7faac5 100644
--- a/nova/tests/conductor/test_conductor.py
+++ b/nova/tests/conductor/test_conductor.py
@@ -990,6 +990,40 @@ class ConductorAPITestCase(_BaseTestCase, test.TestCase):
         self.conductor.security_groups_trigger_handler(self.context,
                                                        'event', 'arg')
 
+    def test_quota_commit_with_project_id(self):
+        diff_proj_id = 'diff_fake_proj_id'
+        self.assertNotEqual(self.context.project_id, diff_proj_id)
+        call_info = {}
+
+        def mgr_quota_commit(ctxt, reservations):
+            call_info['resvs'] = reservations
+            call_info['project_id'] = ctxt.project_id
+
+        self.stubs.Set(self.conductor_manager, 'quota_commit',
+                       mgr_quota_commit)
+
+        self.conductor.quota_commit(self.context, 'fake_resvs',
+                                    project_id=diff_proj_id)
+        self.assertEqual(diff_proj_id, call_info['project_id'])
+        self.assertEqual('fake_resvs', call_info['resvs'])
+
+    def test_quota_rollback_with_project_id(self):
+        diff_proj_id = 'diff_fake_proj_id'
+        self.assertNotEqual(self.context.project_id, diff_proj_id)
+        call_info = {}
+
+        def mgr_quota_rollback(ctxt, reservations):
+            call_info['resvs'] = reservations
+            call_info['project_id'] = ctxt.project_id
+
+        self.stubs.Set(self.conductor_manager, 'quota_rollback',
+                       mgr_quota_rollback)
+
+        self.conductor.quota_rollback(self.context, 'fake_resvs',
+                                      project_id=diff_proj_id)
+        self.assertEqual(diff_proj_id, call_info['project_id'])
+        self.assertEqual('fake_resvs', call_info['resvs'])
+
 
 class ConductorLocalAPITestCase(ConductorAPITestCase):
     """Conductor LocalAPI Tests."""