Merge "Revert task_state on failed instance actions"

2 files changed, 108 insertions, 10 deletions

diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index 4ed07ac1f..fe2040d1b 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -188,6 +188,30 @@ def checks_instance_lock(function):
     return decorated_function
 
 
+def reverts_task_state(function):
+    """Decorator to revert task_state on failure"""
+
+    @functools.wraps(function)
+    def decorated_function(self, context, *args, **kwargs):
+        instance = kwargs.get('instance', None)
+        if instance:
+            instance_uuid = instance['uuid']
+        else:
+            instance_uuid = kwargs['instance_uuid']
+
+        try:
+            function(self, context, *args, **kwargs)
+        except Exception:
+            with excutils.save_and_reraise_exception():
+                try:
+                    self._instance_update(context, instance_uuid,
+                                          task_state=None)
+                except Exception:
+                    pass
+
+    return decorated_function
+
+
 def wrap_instance_fault(function):
     """Wraps a method to catch exceptions related to instances.
 
@@ -800,6 +824,7 @@ class ComputeManager(manager.SchedulerDependentManager):
         return {'block_device_mapping': block_device_mapping}
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @wrap_instance_fault
     def run_instance(self, context, request_spec=None,
                      filter_properties=None, requested_networks=None,
@@ -893,6 +918,7 @@ class ComputeManager(manager.SchedulerDependentManager):
                 system_metadata=system_meta)
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def terminate_instance(self, context, instance=None, instance_uuid=None):
@@ -917,6 +943,7 @@ class ComputeManager(manager.SchedulerDependentManager):
         do_terminate_instance(instance)
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def stop_instance(self, context, instance=None, instance_uuid=None):
@@ -928,6 +955,7 @@ class ComputeManager(manager.SchedulerDependentManager):
                                 final_state=vm_states.STOPPED)
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def start_instance(self, context, instance=None, instance_uuid=None):
@@ -938,6 +966,7 @@ class ComputeManager(manager.SchedulerDependentManager):
                                instance_uuid=instance_uuid)
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def power_off_instance(self, context, instance=None, instance_uuid=None,
@@ -956,6 +985,7 @@ class ComputeManager(manager.SchedulerDependentManager):
         self._notify_about_instance_usage(context, instance, "power_off.end")
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def power_on_instance(self, context, instance=None, instance_uuid=None):
@@ -973,6 +1003,7 @@ class ComputeManager(manager.SchedulerDependentManager):
         self._notify_about_instance_usage(context, instance, "power_on.end")
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def rebuild_instance(self, context, orig_image_ref,
@@ -1061,6 +1092,7 @@ class ComputeManager(manager.SchedulerDependentManager):
                     extra_usage_info=extra_usage_info)
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def reboot_instance(self, context, instance=None, instance_uuid=None,
@@ -1106,6 +1138,7 @@ class ComputeManager(manager.SchedulerDependentManager):
         self._notify_about_instance_usage(context, instance, "reboot.end")
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @wrap_instance_fault
     def snapshot_instance(self, context, image_id,
                           image_type='snapshot', backup_type=None,
@@ -1145,11 +1178,7 @@ class ComputeManager(manager.SchedulerDependentManager):
         self._notify_about_instance_usage(
                 context, instance, "snapshot.start")
 
-        try:
-            self.driver.snapshot(context, instance, image_id)
-        finally:
-            self._instance_update(context, instance['uuid'],
-                                  task_state=None)
+        self.driver.snapshot(context, instance, image_id)
 
         if image_type == 'snapshot' and rotation:
             raise exception.ImageRotationNotAllowed()
@@ -1213,6 +1242,7 @@ class ComputeManager(manager.SchedulerDependentManager):
                 image_service.delete(context, image_id)
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def set_admin_password(self, context, instance=None, instance_uuid=None,
@@ -1281,6 +1311,7 @@ class ComputeManager(manager.SchedulerDependentManager):
                     continue
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def inject_file(self, context, path, file_contents, instance_uuid=None,
@@ -1301,6 +1332,7 @@ class ComputeManager(manager.SchedulerDependentManager):
         self.driver.inject_file(instance, path, file_contents)
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def rescue_instance(self, context, instance=None, instance_uuid=None,
@@ -1334,6 +1366,7 @@ class ComputeManager(manager.SchedulerDependentManager):
                               power_state=current_power_state)
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def unrescue_instance(self, context, instance=None, instance_uuid=None):
@@ -1358,6 +1391,7 @@ class ComputeManager(manager.SchedulerDependentManager):
                               power_state=current_power_state)
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def change_instance_metadata(self, context, diff, instance=None,
@@ -1400,6 +1434,7 @@ class ComputeManager(manager.SchedulerDependentManager):
             self._quota_commit(context, reservations)
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def revert_resize(self, context, migration_id, instance=None,
@@ -1428,6 +1463,7 @@ class ComputeManager(manager.SchedulerDependentManager):
                     reservations)
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def finish_revert_resize(self, context, migration_id, instance_uuid=None,
@@ -1489,6 +1525,7 @@ class ComputeManager(manager.SchedulerDependentManager):
             QUOTAS.rollback(context, reservations)
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def prep_resize(self, context, image, instance=None, instance_uuid=None,
@@ -1548,6 +1585,7 @@ class ComputeManager(manager.SchedulerDependentManager):
                 extra_usage_info=extra_usage_info)
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def resize_instance(self, context, migration_id, image, instance=None,
@@ -1643,6 +1681,7 @@ class ComputeManager(manager.SchedulerDependentManager):
             network_info=network_info)
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def finish_resize(self, context, migration_id, disk_info, image,
@@ -1670,6 +1709,7 @@ class ComputeManager(manager.SchedulerDependentManager):
                 self._set_instance_error_state(context, instance['uuid'])
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def add_fixed_ip_to_instance(self, context, network_id, instance=None,
@@ -1694,6 +1734,7 @@ class ComputeManager(manager.SchedulerDependentManager):
             context, instance, "create_ip.end", network_info=network_info)
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def remove_fixed_ip_from_instance(self, context, address, instance=None,
@@ -1719,6 +1760,7 @@ class ComputeManager(manager.SchedulerDependentManager):
             context, instance, "delete_ip.end", network_info=network_info)
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def pause_instance(self, context, instance=None, instance_uuid=None):
@@ -1738,6 +1780,7 @@ class ComputeManager(manager.SchedulerDependentManager):
                               task_state=None)
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def unpause_instance(self, context, instance=None, instance_uuid=None):
@@ -1790,6 +1833,7 @@ class ComputeManager(manager.SchedulerDependentManager):
             return self.driver.get_diagnostics(instance)
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def suspend_instance(self, context, instance=None, instance_uuid=None):
@@ -1812,6 +1856,7 @@ class ComputeManager(manager.SchedulerDependentManager):
         self._notify_about_instance_usage(context, instance, 'suspend')
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def resume_instance(self, context, instance=None, instance_uuid=None):
@@ -1872,6 +1917,7 @@ class ComputeManager(manager.SchedulerDependentManager):
                   instance=instance)
         return instance['locked']
 
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def reset_network(self, context, instance=None, instance_uuid=None):
@@ -1977,6 +2023,7 @@ class ComputeManager(manager.SchedulerDependentManager):
         return connection_info
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def attach_volume(self, context, volume_id, mountpoint, instance_uuid=None,
@@ -2046,6 +2093,7 @@ class ComputeManager(manager.SchedulerDependentManager):
                                   mp)
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def detach_volume(self, context, volume_id, instance_uuid=None,
diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py
index 41a365d49..510e7713c 100644
--- a/nova/tests/compute/test_compute.py
+++ b/nova/tests/compute/test_compute.py
@@ -344,10 +344,10 @@ class ComputeTestCase(BaseTestCase):
                           self.context, instance_uuid=instance_uuid)
         #check state is failed even after the periodic poll
         self._assert_state({'vm_state': vm_states.ERROR,
-                            'task_state': task_states.BLOCK_DEVICE_MAPPING})
+                            'task_state': None})
         self.compute.periodic_tasks(context.get_admin_context())
         self._assert_state({'vm_state': vm_states.ERROR,
-                            'task_state': task_states.BLOCK_DEVICE_MAPPING})
+                            'task_state': None})
 
     def test_run_instance_spawn_fail(self):
         """ spawn failure test.
@@ -362,10 +362,10 @@ class ComputeTestCase(BaseTestCase):
                           self.context, instance_uuid=instance_uuid)
         #check state is failed even after the periodic poll
         self._assert_state({'vm_state': vm_states.ERROR,
-                            'task_state': task_states.SPAWNING})
+                            'task_state': None})
         self.compute.periodic_tasks(context.get_admin_context())
         self._assert_state({'vm_state': vm_states.ERROR,
-                            'task_state': task_states.SPAWNING})
+                            'task_state': None})
 
     def test_can_terminate_on_error_state(self):
         """Make sure that the instance can be terminated in ERROR state"""
@@ -713,7 +713,7 @@ class ComputeTestCase(BaseTestCase):
         exc = exception.NotAuthorized(_('Internal error'))
         self._do_test_set_admin_password_driver_error(exc,
                                                 vm_states.ERROR,
-                                                task_states.UPDATING_PASSWORD)
+                                                None)
 
     def test_set_admin_password_driver_not_implemented(self):
         """
@@ -1127,6 +1127,56 @@ class ComputeTestCase(BaseTestCase):
         self.compute.terminate_instance(self.context,
                 instance=jsonutils.to_primitive(instance))
 
+    def _test_state_revert(self, operation, pre_task_state):
+        instance = self._create_fake_instance()
+        self.compute.run_instance(self.context, instance=instance)
+
+        # The API would have set task_state, so do that here to test
+        # that the state gets reverted on failure
+        db.instance_update(self.context, instance['uuid'],
+                           {"task_state": pre_task_state})
+
+        raised = False
+        try:
+            ret_val = getattr(self.compute, operation)(self.context,
+                                                       instance=instance)
+        except Exception:
+            raised = True
+        self.assertTrue(raised)
+
+        # Fetch the instance's task_state and make sure it went to None
+        instance = db.instance_get_by_uuid(self.context, instance['uuid'])
+        self.assertEqual(instance["task_state"], None)
+
+    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),
+            ]
+
+        def _get_an_exception(*args, **kwargs):
+            raise Exception("This fails every single time!")
+
+        self.stubs.Set(self.compute, "_get_lock", _get_an_exception)
+        for operation, pre_state in actions:
+            self._test_state_revert(operation, pre_state)
+
     def _ensure_quota_reservations_committed(self):
         """Mock up commit of quota reservations"""
         reservations = list('fake_res')