Change InstanceFaultRollback handling in _error_out_instance_on_exception

For some reason, only NotImplementedError in _error_out_instance_on_exception
would use the $instance_state parameter which can be controlled by the
caller of the context manager to determine the rollback vm_state. But in the
case of InstanceFaultRollback, the caller may want to reset the vm_state
back to something other than ACTIVE, like if the instance is actually
STOPPED and something like prep_resize fails (you can resize a STOPPED
instance).

This change makes _error_out_instance_on_exception handle InstanceFaultRollback
like NotImplementedError in that the instance_state parameter is used to reset
the instance.vm_state. It also adds a docstring explaining how this context
manager works along with some notes/questions about ways to improve it.

Change-Id: Ie4f9177f4d54cbc7dbcf58bd107fd5f24c60d8bb
Related-Bug: #1811235

nova/compute/manager.py

@@ -8258,29 +8258,60 @@ class ComputeManager(manager.Manager):
     @contextlib.contextmanager
     def _error_out_instance_on_exception(self, context, instance,
                                          instance_state=vm_states.ACTIVE):
+        """Context manager to set instance.vm_state after some operation raises
+
+        Used to handle NotImplementedError and InstanceFaultRollback errors
+        and reset the instance vm_state and task_state. The vm_state is set
+        to the $instance_state parameter and task_state is set to None.
+        For all other types of exceptions, the vm_state is set to ERROR and
+        the task_state is left unchanged (although most callers will have the
+        @reverts_task_state decorator which will set the task_state to None).
+
+        Re-raises the original exception *except* in the case of
+        InstanceFaultRollback in which case the wrapped `inner_exception` is
+        re-raised.
+
+        :param context: The nova auth request context for the operation.
+        :param instance: The instance to update. The vm_state will be set by
+            this context manager when an exception is raised.
+        :param instance_state: For NotImplementedError and
+            InstanceFaultRollback this is the vm_state to set the instance to
+            when handling one of those types of exceptions. By default the
+            instance will be set to ACTIVE, but the caller should control this
+            in case there have been no changes to the running state of the
+            instance. For example, resizing a stopped server where prep_resize
+            fails early and does not change the power state of the guest should
+            not set the instance status to ACTIVE but remain STOPPED.
+            This parameter is ignored for all other types of exceptions and the
+            instance vm_state is set to ERROR.
+        """
+        # NOTE(mriedem): Why doesn't this method just save off the
+        # original instance.vm_state here rather than use a parameter? Or use
+        # instance_state=None as an override but default to the current
+        # vm_state when rolling back.
         instance_uuid = instance.uuid
         try:
             yield
-        except NotImplementedError as error:
-            with excutils.save_and_reraise_exception():
-                LOG.info("Setting instance back to %(state)s after: "
-                         "%(error)s",
+        except (NotImplementedError, exception.InstanceFaultRollback) as error:
+            # Use reraise=False to determine if we want to raise the original
+            # exception or something else.
+            with excutils.save_and_reraise_exception(reraise=False) as ctxt:
+                LOG.info("Setting instance back to %(state)s after: %(error)s",
                          {'state': instance_state, 'error': error},
                          instance_uuid=instance_uuid)
                 self._instance_update(context, instance,
                                       vm_state=instance_state,
                                       task_state=None)
-        except exception.InstanceFaultRollback as error:
-            LOG.info("Setting instance back to ACTIVE after: %s",
-                     error, instance_uuid=instance_uuid)
-            self._instance_update(context, instance,
-                                  vm_state=vm_states.ACTIVE,
-                                  task_state=None)
-            raise error.inner_exception
+                if isinstance(error, exception.InstanceFaultRollback):
+                    # Raise the wrapped exception.
+                    raise error.inner_exception
+                # Else re-raise the NotImplementedError.
+                ctxt.reraise = True
         except Exception:
             LOG.exception('Setting instance vm_state to ERROR',
                           instance_uuid=instance_uuid)
             with excutils.save_and_reraise_exception():
+                # NOTE(mriedem): Why don't we pass clean_task_state=True here?
                 self._set_instance_obj_error_state(context, instance)
 
     @wrap_exception()

nova/tests/unit/compute/test_compute_mgr.py

@@ -4061,15 +4061,16 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
         instance = fake_instance.fake_instance_obj(self.context)
 
         def do_test():
-            with self.compute._error_out_instance_on_exception(self.context,
-                                                               instance):
+            with self.compute._error_out_instance_on_exception(
+                    self.context, instance, instance_state=vm_states.STOPPED):
                 raise exception.InstanceFaultRollback(
                     inner_exception=test.TestingException('test'))
 
         self.assertRaises(test.TestingException, do_test)
+        # The vm_state should be set to the instance_state parameter.
         inst_update_mock.assert_called_once_with(
             self.context, instance,
-            vm_state=vm_states.ACTIVE, task_state=None)
+            vm_state=vm_states.STOPPED, task_state=None)
 
     @mock.patch('nova.compute.manager.ComputeManager.'
                 '_set_instance_obj_error_state')