openstack
/
nova
Code Issues Proposed changes

Pass exception through TaskBase.rollback 

This modifies the TaskBase.rollback method to pass the
handled exception through so implementations of the
rollback method can use the exception for things like
adding an instance fault, notifications, etc.

Change-Id: Ib31691dca7b1374512fe5e577e46c9e3e223d608
This commit is contained in:
Matt Riedemann 2019-11-02 19:27:18 -04:00
parent 3b678fc276
commit accdc16947
7 changed files with 23 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
nova/conductor/tasks/base.py
View File

@ -22,9 +22,9 @@ def rollback_wrapper(original):
def wrap(self):
try:
return original(self)
except Exception:
except Exception as ex:
with excutils.save_and_reraise_exception():
self.rollback()
self.rollback(ex)
return wrap
@ -48,7 +48,7 @@ class TaskBase(object):
"""
pass
def rollback(self):
def rollback(self, ex):
"""Rollback failed task
Descendants should implement this method to allow task user to
rollback status to state before execute method was call

12
nova/conductor/tasks/cross_cell_migrate.py
View File

@ -190,7 +190,7 @@ class TargetDBSetupTask(base.TaskBase):
return inst, target_cell_migration
def rollback(self):
def rollback(self, ex):
"""Deletes the instance data from the target cell in case of failure"""
if self._target_cell_instance:
# Deleting the instance in the target cell DB should perform a
@ -340,7 +340,7 @@ class PrepResizeAtDestTask(base.TaskBase):
'to host: %s') % destination
raise exception.MigrationPreCheckError(reason=msg)
def rollback(self):
def rollback(self, ex):
# Rollback anything we created.
host = self.host_selection.service_host
# Cleanup any destination host port bindings.
@ -431,7 +431,7 @@ class PrepResizeAtSourceTask(base.TaskBase):
return self._image_id
def rollback(self):
def rollback(self, ex):
# If we created a snapshot image, attempt to delete it.
if self._image_id:
compute_utils.delete_image(
@ -627,7 +627,7 @@ class FinishResizeAtDestTask(base.TaskBase):
# Do the instance.hidden/instance_mapping.cell_mapping swap.
self._update_instance_mapping()
def rollback(self):
def rollback(self, ex):
# The method executed in this task are self-contained for rollbacks.
pass
@ -874,7 +874,7 @@ class CrossCellMigrationTask(base.TaskBase):
self._finish_resize_at_dest(
target_cell_migration, target_cell_mapping, snapshot_id)
def rollback(self):
def rollback(self, ex):
"""Rollback based on how sub-tasks completed
Sub-tasks should rollback appropriately for whatever they do but here
@ -885,6 +885,6 @@ class CrossCellMigrationTask(base.TaskBase):
# Rollback the completed tasks in reverse order.
for task_name in reversed(self._completed_tasks):
try:
self._completed_tasks[task_name].rollback()
self._completed_tasks[task_name].rollback(ex)
except Exception:
LOG.exception('Rollback for task %s failed.', task_name)

2
nova/conductor/tasks/live_migrate.py
View File

@ -140,7 +140,7 @@ class LiveMigrationTask(base.TaskBase):
migration=self.migration,
migrate_data=self.migrate_data)
def rollback(self):
def rollback(self, ex):
# TODO(johngarbutt) need to implement the clean up operation
# but this will make sense only once we pull in the compute
# calls, since this class currently makes no state changes,

2
nova/conductor/tasks/migrate.py
View File

@ -444,7 +444,7 @@ class MigrationTask(base.TaskBase):
raise exception.MaxRetriesExceeded(reason=reason)
return selection
def rollback(self):
def rollback(self, ex):
if self._migration:
self._migration.status = 'error'
self._migration.save()

2
nova/tests/unit/conductor/tasks/test_base.py
View File

@ -42,7 +42,7 @@ class TaskBaseTestCase(test.NoDBTestCase):
self.task.execute()
except Exception:
pass
fake_rollback.assert_called_once_with()
fake_rollback.assert_called_once_with(test.MatchType(Exception))
@mock.patch.object(FakeTask, 'rollback')
def test_wrapper_no_exception(self, fake_rollback):

16
nova/tests/unit/conductor/tasks/test_cross_cell_migrate.py
View File

@ -366,7 +366,7 @@ class TargetDBSetupTaskTestCase(
# Rollback the task and assert the instance and its related data are
# gone from the target cell database. Use a modified context to make
# sure the instance was hard-deleted.
task.rollback()
task.rollback(test.TestingException('error'))
read_deleted_ctxt = self.target_context.elevated(read_deleted='yes')
self.assertRaises(exception.InstanceNotFound,
objects.Instance.get_by_uuid,
@ -426,7 +426,7 @@ class CrossCellMigrationTaskTestCase(test.NoDBTestCase):
mock.sentinel.target_cell_mapping,
mock_prep_resize_at_source.return_value)
# Now rollback the completed sub-tasks
self.task.rollback()
self.task.rollback(test.TestingException('error'))
def test_perform_external_api_checks_ok(self):
"""Tests the happy path scenario where neutron APIs are new enough for
@ -461,14 +461,14 @@ class CrossCellMigrationTaskTestCase(test.NoDBTestCase):
task.rollback.side_effect = test.TestingException('sub-task')
self.task._completed_tasks[str(x)] = task
# Run execute but mock _execute to fail somehow.
with mock.patch.object(self.task, '_execute',
side_effect=test.TestingException('main task')):
error = test.TestingException('main task')
with mock.patch.object(self.task, '_execute', side_effect=error):
# The TestingException from the main task should be raised.
ex = self.assertRaises(test.TestingException, self.task.execute)
self.assertEqual('main task', six.text_type(ex))
# And all three sub-task rollbacks should have been called.
for subtask in self.task._completed_tasks.values():
subtask.rollback.assert_called_once_with()
subtask.rollback.assert_called_once_with(error)
# The 2nd task rollback should have raised and been logged.
mock_log_exception.assert_called_once()
self.assertEqual('1', mock_log_exception.call_args[0][1])
@ -771,7 +771,7 @@ class PrepResizeAtDestTaskTestCase(test.NoDBTestCase):
) as (
delete_port_binding, attachment_delete
):
self.task.rollback()
self.task.rollback(test.TestingException('error'))
# Should have called both delete methods twice in any order.
host = self.task.host_selection.service_host
delete_port_binding.assert_has_calls([
@ -857,11 +857,11 @@ class PrepResizeAtSourceTaskTestCase(test.NoDBTestCase):
def test_rollback(self, delete_image):
"""Tests rollback when there is an image and when there is not."""
# First test when there is no image_id so we do not try to delete it.
self.task.rollback()
self.task.rollback(test.TestingException('error'))
delete_image.assert_not_called()
# Now set an image and we should try to delete it.
self.task._image_id = uuids.image_id
self.task.rollback()
self.task.rollback(test.TestingException('error'))
delete_image.assert_called_once_with(
self.task.context, self.task.instance, self.task.image_api,
self.task._image_id)

6
nova/tests/unit/conductor/test_conductor.py
View File

@ -2666,7 +2666,7 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase):
notify_mock.assert_called_once_with(self.context, inst_obj.uuid,
'migrate_server', updates,
exc_info, fake_spec)
rollback_mock.assert_called_once_with()
rollback_mock.assert_called_once_with(exc_info)
@mock.patch.object(objects.InstanceMapping, 'get_by_instance_uuid')
@mock.patch.object(scheduler_utils, 'setup_instance_group')
@ -2717,7 +2717,7 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase):
notify_mock.assert_called_once_with(self.context, inst_obj.uuid,
'migrate_server', updates,
exc_info, fake_spec)
rollback_mock.assert_called_once_with()
rollback_mock.assert_called_once_with(exc_info)
def test_cold_migrate_no_valid_host_error_msg(self):
inst_obj = objects.Instance(
@ -2858,7 +2858,7 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase):
notify_mock.assert_called_once_with(self.context, inst_obj.uuid,
'migrate_server', updates,
exc_info, fake_spec)
rollback_mock.assert_called_once_with()
rollback_mock.assert_called_once_with(exc_info)
@mock.patch.object(objects.RequestSpec, 'save')
@mock.patch.object(migrate.MigrationTask, 'execute')