From 46cb2fdfe2df333d870aca8e6afc521172b8e061 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Fri, 25 Jan 2019 08:58:36 -0500 Subject: [PATCH] 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 | 53 ++++++++++++++++----- nova/tests/unit/compute/test_compute_mgr.py | 7 +-- 2 files changed, 46 insertions(+), 14 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index faa2f3857788..d546cb85329c 100644 --- a/nova/compute/manager.py +++ b/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() diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index a9ca211b8cda..24d91877554f 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/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')