Fix quota updating during soft delete and restore

Fixes bug 1075716

Quotas were not properly updated when soft deletes were enabled. This
patch fixes quotas for soft deletes and restores.

Change-Id: I77fd3ff76caa9eba3e2180c1abcfb390ea7857d6

3 files changed, 104 insertions, 67 deletions

diff --git a/nova/api/openstack/compute/contrib/deferred_delete.py b/nova/api/openstack/compute/contrib/deferred_delete.py
index 8eaea04bb..ea7ac00f9 100644
--- a/nova/api/openstack/compute/contrib/deferred_delete.py
+++ b/nova/api/openstack/compute/contrib/deferred_delete.py
@@ -42,6 +42,9 @@ class DeferredDeleteController(wsgi.Controller):
         instance = self.compute_api.get(context, id)
         try:
             self.compute_api.restore(context, instance)
+        except exception.QuotaError as error:
+            raise exc.HTTPRequestEntityTooLarge(explanation=unicode(error),
+                                                headers={'Retry-After': 0})
         except exception.InstanceInvalidState as state_error:
             common.raise_http_conflict_for_instance_invalid_state(state_error,
                     'restore')
diff --git a/nova/compute/api.py b/nova/compute/api.py
index ba4f2b4ff..121de6a47 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -823,50 +823,29 @@ class API(base.Base):
 
         return dict(old_ref.iteritems()), dict(instance_ref.iteritems())
 
-    @wrap_check_policy
-    @check_instance_lock
-    @check_instance_state(vm_state=None, task_state=None)
-    def soft_delete(self, context, instance):
-        """Terminate an instance."""
-        LOG.debug(_('Going to try to soft delete instance'),
-                  instance=instance)
-
+    def _delete(self, context, instance, cb, **instance_attrs):
         if instance['disable_terminate']:
+            LOG.info(_('instance termination disabled'),
+                     instance=instance)
             return
 
-        # NOTE(jerdfelt): The compute daemon handles reclaiming instances
-        # that are in soft delete. If there is no host assigned, there is
-        # no daemon to reclaim, so delete it immediately.
-        if instance['host']:
-            instance = self.update(context, instance,
-                                   task_state=task_states.SOFT_DELETING,
-                                   expected_task_state=None,
-                                   deleted_at=timeutils.utcnow())
-
-            self.compute_rpcapi.soft_delete_instance(context, instance)
-        else:
-            LOG.warning(_('No host for instance, deleting immediately'),
-                        instance=instance)
-            try:
-                self.db.instance_destroy(context, instance['uuid'])
-            except exception.InstanceNotFound:
-                # NOTE(comstud): Race condition.  Instance already gone.
-                pass
-
-    def _delete(self, context, instance):
         host = instance['host']
+        bdms = self.db.block_device_mapping_get_all_by_instance(
+                    context, instance['uuid'])
         reservations = None
         try:
-
-            #Note(maoy): no expected_task_state needs to be set
+            # NOTE(maoy): no expected_task_state needs to be set
+            attrs = {'progress': 0}
+            attrs.update(instance_attrs)
             old, updated = self._update(context,
                                         instance,
-                                        task_state=task_states.DELETING,
-                                        progress=0)
+                                        **attrs)
 
             # Avoid double-counting the quota usage reduction
             # where delete is already in progress
-            if old['task_state'] != task_states.DELETING:
+            if (old['vm_state'] != vm_states.SOFT_DELETED and
+                old['task_state'] not in (task_states.DELETING,
+                                          task_states.SOFT_DELETING)):
                 reservations = QUOTAS.reserve(context,
                                               instances=-1,
                                               cores=-instance['vcpus'],
@@ -876,12 +855,11 @@ class API(base.Base):
                 # Just update database, nothing else we can do
                 constraint = self.db.constraint(host=self.db.equal_any(host))
                 try:
-                    result = self.db.instance_destroy(context,
-                                                      instance['uuid'],
-                                                      constraint)
+                    self.db.instance_destroy(context, instance['uuid'],
+                                             constraint)
                     if reservations:
                         QUOTAS.commit(context, reservations)
-                    return result
+                    return
                 except exception.ConstraintNotMet:
                     # Refresh to get new host information
                     instance = self.get(context, instance['uuid'])
@@ -916,9 +894,7 @@ class API(base.Base):
                             reservations=downsize_reservations)
 
             is_up = False
-            bdms = self.db.block_device_mapping_get_all_by_instance(
-                context, instance["uuid"])
-            #Note(jogo): db allows for multiple compute services per host
+            # NOTE(jogo): db allows for multiple compute services per host
             try:
                 services = self.db.service_get_all_compute_by_host(
                         context.elevated(), instance['host'])
@@ -927,8 +903,7 @@ class API(base.Base):
             for service in services:
                 if utils.service_is_up(service):
                     is_up = True
-                    self.compute_rpcapi.terminate_instance(context, instance,
-                                                           bdms)
+                    cb(context, instance, bdms)
                     break
             if not is_up:
                 # If compute node isn't up, just delete from DB
@@ -987,40 +962,69 @@ class API(base.Base):
     @wrap_check_policy
     @check_instance_lock
     @check_instance_state(vm_state=None, task_state=None)
-    def delete(self, context, instance):
+    def soft_delete(self, context, instance):
         """Terminate an instance."""
-        LOG.debug(_("Going to try to terminate instance"), instance=instance)
+        LOG.debug(_('Going to try to soft delete instance'),
+                  instance=instance)
 
-        if instance['disable_terminate']:
-            return
+        def soft_delete(context, instance, bdms):
+            self.compute_rpcapi.soft_delete_instance(context, instance)
+
+        self._delete(context, instance, soft_delete,
+                     task_state=task_states.SOFT_DELETING,
+                     deleted_at=timeutils.utcnow())
 
-        self._delete(context, instance)
+    def _delete_instance(self, context, instance):
+        def terminate(context, instance, bdms):
+            self.compute_rpcapi.terminate_instance(context, instance, bdms)
+
+        self._delete(context, instance, terminate,
+                     task_state=task_states.DELETING)
+
+    @wrap_check_policy
+    @check_instance_lock
+    @check_instance_state(vm_state=None, task_state=None)
+    def delete(self, context, instance):
+        """Terminate an instance."""
+        LOG.debug(_("Going to try to terminate instance"), instance=instance)
+        self._delete_instance(context, instance)
 
     @wrap_check_policy
     @check_instance_lock
     @check_instance_state(vm_state=[vm_states.SOFT_DELETED])
     def restore(self, context, instance):
         """Restore a previously deleted (but not reclaimed) instance."""
-        if instance['host']:
-            instance = self.update(context, instance,
-                        task_state=task_states.RESTORING,
-                        expected_task_state=None,
-                        deleted_at=None)
-            self.compute_rpcapi.restore_instance(context, instance)
-        else:
-            self.update(context,
-                        instance,
-                        vm_state=vm_states.ACTIVE,
-                        task_state=None,
-                        expected_task_state=None,
-                        deleted_at=None)
+        # Reserve quotas
+        instance_type = instance['instance_type']
+        num_instances, quota_reservations = self._check_num_instances_quota(
+                context, instance_type, 1, 1)
+
+        try:
+            if instance['host']:
+                instance = self.update(context, instance,
+                            task_state=task_states.RESTORING,
+                            expected_task_state=None,
+                            deleted_at=None)
+                self.compute_rpcapi.restore_instance(context, instance)
+            else:
+                self.update(context,
+                            instance,
+                            vm_state=vm_states.ACTIVE,
+                            task_state=None,
+                            expected_task_state=None,
+                            deleted_at=None)
+
+            QUOTAS.commit(context, quota_reservations)
+        except Exception:
+            with excutils.save_and_reraise_exception():
+                QUOTAS.rollback(context, quota_reservations)
 
     @wrap_check_policy
     @check_instance_lock
     @check_instance_state(vm_state=[vm_states.SOFT_DELETED])
     def force_delete(self, context, instance):
         """Force delete a previously deleted (but not reclaimed) instance."""
-        self._delete(context, instance)
+        self._delete_instance(context, instance)
 
     @wrap_check_policy
     @check_instance_lock
diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py
index be3eeb601..00f2ceb7a 100644
--- a/nova/tests/compute/test_compute.py
+++ b/nova/tests/compute/test_compute.py
@@ -3229,7 +3229,12 @@ class ComputeAPITestCase(BaseTestCase):
         db.instance_destroy(self.context, instance['uuid'])
 
     def test_delete_soft(self):
-        instance, instance_uuid = self._run_instance()
+        instance, instance_uuid = self._run_instance(params={
+                'host': FLAGS.host})
+
+        self.mox.StubOutWithMock(nova.quota.QUOTAS, 'commit')
+        nova.quota.QUOTAS.commit(mox.IgnoreArg(), mox.IgnoreArg())
+        self.mox.ReplayAll()
 
         self.compute_api.soft_delete(self.context, instance)
 
@@ -3239,10 +3244,11 @@ class ComputeAPITestCase(BaseTestCase):
         db.instance_destroy(self.context, instance['uuid'])
 
     def test_delete_soft_fail(self):
-        instance, instance_uuid = self._run_instance()
-
+        instance, instance_uuid = self._run_instance(params={
+                'host': FLAGS.host})
         instance = db.instance_update(self.context, instance_uuid,
                                       {'disable_terminate': True})
+
         self.compute_api.soft_delete(self.context, instance)
 
         instance = db.instance_get_by_uuid(self.context, instance_uuid)
@@ -3250,6 +3256,27 @@ class ComputeAPITestCase(BaseTestCase):
 
         db.instance_destroy(self.context, instance['uuid'])
 
+    def test_delete_soft_rollback(self):
+        instance, instance_uuid = self._run_instance(params={
+                'host': FLAGS.host})
+
+        self.mox.StubOutWithMock(nova.quota.QUOTAS, 'rollback')
+        nova.quota.QUOTAS.rollback(mox.IgnoreArg(), mox.IgnoreArg())
+        self.mox.ReplayAll()
+
+        def fail(*args, **kwargs):
+            raise test.TestingException()
+        self.stubs.Set(self.compute_api.compute_rpcapi, 'soft_delete_instance',
+                       fail)
+
+        self.assertRaises(test.TestingException, self.compute_api.soft_delete,
+                          self.context, instance)
+
+        instance = db.instance_get_by_uuid(self.context, instance_uuid)
+        self.assertEqual(instance['task_state'], task_states.SOFT_DELETING)
+
+        db.instance_destroy(self.context, instance['uuid'])
+
     def test_force_delete(self):
         """Ensure instance can be deleted after a soft delete"""
         instance = jsonutils.to_primitive(self._create_fake_instance(params={
@@ -3344,9 +3371,8 @@ class ComputeAPITestCase(BaseTestCase):
 
     def test_restore(self):
         """Ensure instance can be restored from a soft delete"""
-        instance = jsonutils.to_primitive(self._create_fake_instance())
-        instance_uuid = instance['uuid']
-        self.compute.run_instance(self.context, instance=instance)
+        instance, instance_uuid = self._run_instance(params={
+                'host': FLAGS.host})
 
         instance = db.instance_get_by_uuid(self.context, instance_uuid)
         self.compute_api.soft_delete(self.context, instance)
@@ -3359,6 +3385,10 @@ class ComputeAPITestCase(BaseTestCase):
                                       {'vm_state': vm_states.SOFT_DELETED,
                                        'task_state': None})
 
+        self.mox.StubOutWithMock(nova.quota.QUOTAS, 'commit')
+        nova.quota.QUOTAS.commit(mox.IgnoreArg(), mox.IgnoreArg())
+        self.mox.ReplayAll()
+
         self.compute_api.restore(self.context, instance)
 
         instance = db.instance_get_by_uuid(self.context, instance_uuid)