Avoid VM task state revert on instance termination

Related to bug 1046236.

Previously, if an exception is raised during the nova-compute node's
instance termination logic *before* the instance is evicted from the DB
(e.g. in the network teardown), then the task_state is reverted back to
None by the reverts_task_state decorator, so the instance continues
to be reported as ACTIVE with no outstanding task.

A subsequent attempt to delete the same instance results in the
quota_usages in_use count being decremented a second time.

This occurs despite I91a70ada as that fix depends on the VM task state
continuing to be DELETING. That logic was undermined in Id4358c50 which
caused the task state to be reverted on the successful or unsuccessful
conclusion of the action on the compute node.

Now, the motivation for Id4358c50 was to avoid the VM getting stuck with
a task state that prevents any further action other than deletion.
However in the case of a deletion already having been requested, this
protection is unnecessary and counter-productive. Hence we remove the
task state reversion from the terminate_instance action.

Change-Id: Ie5701e5c12f6241a203423d29d05df1858406c56

2 files changed, 62 insertions, 24 deletions

diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index 9fc3104d0..3dc8ab23a 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -891,10 +891,14 @@ class ComputeManager(manager.SchedulerDependentManager):
                 system_metadata=system_meta)
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
-    @reverts_task_state
     @wrap_instance_fault
     def terminate_instance(self, context, instance):
-        """Terminate an instance on this host."""
+        """Terminate an instance on this host.  """
+        # Note(eglynn): we do not decorate this action with reverts_task_state
+        # because a failure during termination should leave the task state as
+        # DELETING, as a signal to the API layer that a subsequent deletion
+        # attempt should not result in a further decrement of the quota_usages
+        # in_use count (see bug 1046236).
 
         elevated = context.elevated()
 
diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py
index f47a5b99d..438b433fe 100644
--- a/nova/tests/compute/test_compute.py
+++ b/nova/tests/compute/test_compute.py
@@ -509,6 +509,37 @@ class ComputeTestCase(BaseTestCase):
         LOG.info(_("After terminating instances: %s"), instances)
         self.assertEqual(len(instances), 0)
 
+    def test_terminate_failure_leaves_task_state(self):
+        """Ensure that a failure in terminate_instance does not result
+        in the task state being reverted from DELETING (see LP 1046236).
+        """
+        instance = jsonutils.to_primitive(self._create_fake_instance())
+
+        self.compute.run_instance(self.context, instance=instance)
+
+        instances = db.instance_get_all(context.get_admin_context())
+        LOG.info(_("Running instances: %s"), instances)
+        self.assertEqual(len(instances), 1)
+
+        # Network teardown fails ungracefully
+        self.mox.StubOutWithMock(self.compute, '_get_instance_nw_info')
+        self.compute._get_instance_nw_info(
+                mox.IgnoreArg(),
+                mox.IgnoreArg()).AndRaise(TypeError())
+        self.mox.ReplayAll()
+
+        db.instance_update(self.context, instance['uuid'],
+                           {"task_state": task_states.DELETING})
+        try:
+            self.compute.terminate_instance(self.context, instance=instance)
+        except TypeError:
+            pass
+
+        instances = db.instance_get_all(context.get_admin_context())
+        LOG.info(_("After terminating instances: %s"), instances)
+        self.assertEqual(len(instances), 1)
+        self.assertEqual(instances[0]['task_state'], 'deleting')
+
     def test_run_terminate_timestamps(self):
         """Make sure timestamps are set for launched and destroyed"""
         instance = jsonutils.to_primitive(self._create_fake_instance())
@@ -1240,7 +1271,8 @@ class ComputeTestCase(BaseTestCase):
         self.compute.terminate_instance(self.context,
                 instance=jsonutils.to_primitive(instance))
 
-    def _test_state_revert(self, operation, pre_task_state):
+    def _test_state_revert(self, operation, pre_task_state,
+                           post_task_state):
         instance = self._create_fake_instance()
         self.compute.run_instance(self.context, instance=instance)
 
@@ -1273,34 +1305,36 @@ class ComputeTestCase(BaseTestCase):
 
         self.assertTrue(raised)
 
-        # Fetch the instance's task_state and make sure it went to None
+        # Fetch the instance's task_state and make sure it went to expected
+        # post-state
         instance = db.instance_get_by_uuid(self.context, instance['uuid'])
-        self.assertEqual(instance["task_state"], None)
+        self.assertEqual(instance["task_state"], post_task_state)
 
     def test_state_revert(self):
         """ensure that task_state is reverted after a failed operation"""
         actions = [
-            ("reboot_instance", task_states.REBOOTING),
-            ("stop_instance", task_states.STOPPING),
-            ("start_instance", task_states.STARTING),
-            ("terminate_instance", task_states.DELETING),
-            ("power_off_instance", task_states.POWERING_OFF),
-            ("power_on_instance", task_states.POWERING_ON),
-            ("rebuild_instance", task_states.REBUILDING),
-            ("set_admin_password", task_states.UPDATING_PASSWORD),
-            ("rescue_instance", task_states.RESCUING),
-            ("unrescue_instance", task_states.UNRESCUING),
-            ("revert_resize", task_states.RESIZE_REVERTING),
-            ("prep_resize", task_states.RESIZE_PREP),
-            ("resize_instance", task_states.RESIZE_PREP),
-            ("pause_instance", task_states.PAUSING),
-            ("unpause_instance", task_states.UNPAUSING),
-            ("suspend_instance", task_states.SUSPENDING),
-            ("resume_instance", task_states.RESUMING),
+            ("reboot_instance", task_states.REBOOTING, None),
+            ("stop_instance", task_states.STOPPING, None),
+            ("start_instance", task_states.STARTING, None),
+            ("terminate_instance", task_states.DELETING,
+                                   task_states.DELETING),
+            ("power_off_instance", task_states.POWERING_OFF, None),
+            ("power_on_instance", task_states.POWERING_ON, None),
+            ("rebuild_instance", task_states.REBUILDING, None),
+            ("set_admin_password", task_states.UPDATING_PASSWORD, None),
+            ("rescue_instance", task_states.RESCUING, None),
+            ("unrescue_instance", task_states.UNRESCUING, None),
+            ("revert_resize", task_states.RESIZE_REVERTING, None),
+            ("prep_resize", task_states.RESIZE_PREP, None),
+            ("resize_instance", task_states.RESIZE_PREP, None),
+            ("pause_instance", task_states.PAUSING, None),
+            ("unpause_instance", task_states.UNPAUSING, None),
+            ("suspend_instance", task_states.SUSPENDING, None),
+            ("resume_instance", task_states.RESUMING, None),
             ]
 
-        for operation, pre_state in actions:
-            self._test_state_revert(operation, pre_state)
+        for operation, pre_state, post_state in actions:
+            self._test_state_revert(operation, pre_state, post_state)
 
     def _ensure_quota_reservations_committed(self):
         """Mock up commit of quota reservations"""