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
(cherry picked from commit 46cb2fdfe2
)
This commit is contained in:
parent
acd2daa9dc
commit
cdaa800784
@ -8251,29 +8251,60 @@ class ComputeManager(manager.Manager):
|
|||||||
@contextlib.contextmanager
|
@contextlib.contextmanager
|
||||||
def _error_out_instance_on_exception(self, context, instance,
|
def _error_out_instance_on_exception(self, context, instance,
|
||||||
instance_state=vm_states.ACTIVE):
|
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
|
instance_uuid = instance.uuid
|
||||||
try:
|
try:
|
||||||
yield
|
yield
|
||||||
except NotImplementedError as error:
|
except (NotImplementedError, exception.InstanceFaultRollback) as error:
|
||||||
with excutils.save_and_reraise_exception():
|
# Use reraise=False to determine if we want to raise the original
|
||||||
LOG.info("Setting instance back to %(state)s after: "
|
# exception or something else.
|
||||||
"%(error)s",
|
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},
|
{'state': instance_state, 'error': error},
|
||||||
instance_uuid=instance_uuid)
|
instance_uuid=instance_uuid)
|
||||||
self._instance_update(context, instance,
|
self._instance_update(context, instance,
|
||||||
vm_state=instance_state,
|
vm_state=instance_state,
|
||||||
task_state=None)
|
task_state=None)
|
||||||
except exception.InstanceFaultRollback as error:
|
if isinstance(error, exception.InstanceFaultRollback):
|
||||||
LOG.info("Setting instance back to ACTIVE after: %s",
|
# Raise the wrapped exception.
|
||||||
error, instance_uuid=instance_uuid)
|
raise error.inner_exception
|
||||||
self._instance_update(context, instance,
|
# Else re-raise the NotImplementedError.
|
||||||
vm_state=vm_states.ACTIVE,
|
ctxt.reraise = True
|
||||||
task_state=None)
|
|
||||||
raise error.inner_exception
|
|
||||||
except Exception:
|
except Exception:
|
||||||
LOG.exception('Setting instance vm_state to ERROR',
|
LOG.exception('Setting instance vm_state to ERROR',
|
||||||
instance_uuid=instance_uuid)
|
instance_uuid=instance_uuid)
|
||||||
with excutils.save_and_reraise_exception():
|
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)
|
self._set_instance_obj_error_state(context, instance)
|
||||||
|
|
||||||
@wrap_exception()
|
@wrap_exception()
|
||||||
|
@ -4058,15 +4058,16 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
|
|||||||
instance = fake_instance.fake_instance_obj(self.context)
|
instance = fake_instance.fake_instance_obj(self.context)
|
||||||
|
|
||||||
def do_test():
|
def do_test():
|
||||||
with self.compute._error_out_instance_on_exception(self.context,
|
with self.compute._error_out_instance_on_exception(
|
||||||
instance):
|
self.context, instance, instance_state=vm_states.STOPPED):
|
||||||
raise exception.InstanceFaultRollback(
|
raise exception.InstanceFaultRollback(
|
||||||
inner_exception=test.TestingException('test'))
|
inner_exception=test.TestingException('test'))
|
||||||
|
|
||||||
self.assertRaises(test.TestingException, do_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(
|
inst_update_mock.assert_called_once_with(
|
||||||
self.context, instance,
|
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.'
|
@mock.patch('nova.compute.manager.ComputeManager.'
|
||||||
'_set_instance_obj_error_state')
|
'_set_instance_obj_error_state')
|
||||||
|
Loading…
Reference in New Issue
Block a user