diff --git a/ironic/conductor/utils.py b/ironic/conductor/utils.py index 057d4dcd5c..94075a4e6f 100644 --- a/ironic/conductor/utils.py +++ b/ironic/conductor/utils.py @@ -346,6 +346,16 @@ def cleaning_error_handler(task, msg, tear_down_cleaning=True, set_fail_state=True): """Put a failed node in CLEANFAIL and maintenance.""" node = task.node + + if tear_down_cleaning: + try: + task.driver.deploy.tear_down_cleaning(task) + except Exception as e: + msg2 = ('Failed to tear down cleaning on node %(uuid)s, ' + 'reason: %(err)s' % {'err': e, 'uuid': node.uuid}) + LOG.exception(msg2) + msg = _('%s. Also failed to tear down cleaning.') % msg + if node.provision_state in ( states.CLEANING, states.CLEANWAIT, @@ -364,13 +374,6 @@ def cleaning_error_handler(task, msg, tear_down_cleaning=True, node.maintenance = True node.maintenance_reason = msg node.save() - if tear_down_cleaning: - try: - task.driver.deploy.tear_down_cleaning(task) - except Exception as e: - msg = ('Failed to tear down cleaning on node %(uuid)s, ' - 'reason: %(err)s' % {'err': e, 'uuid': node.uuid}) - LOG.exception(msg) if set_fail_state: target_state = states.MANAGEABLE if manual_clean else None diff --git a/ironic/tests/unit/conductor/test_utils.py b/ironic/tests/unit/conductor/test_utils.py index f1d96618c6..c35e1c31ab 100644 --- a/ironic/tests/unit/conductor/test_utils.py +++ b/ironic/tests/unit/conductor/test_utils.py @@ -1229,10 +1229,18 @@ class ErrorHandlersTestCase(tests_base.TestCase): @mock.patch.object(conductor_utils, 'LOG') def test_cleaning_error_handler_tear_down_error(self, log_mock): + def _side_effect(task): + # simulate overwriting last error by another operation (e.g. power) + task.node.last_error = None + raise Exception('bar') + driver = self.task.driver.deploy - driver.tear_down_cleaning.side_effect = Exception('bar') - conductor_utils.cleaning_error_handler(self.task, 'foo') + msg = 'foo' + driver.tear_down_cleaning.side_effect = _side_effect + conductor_utils.cleaning_error_handler(self.task, msg) self.assertTrue(log_mock.exception.called) + self.assertIn(msg, self.node.last_error) + self.assertIn(msg, self.node.maintenance_reason) @mock.patch.object(conductor_utils, 'LOG') def test_spawn_cleaning_error_handler_no_worker(self, log_mock): diff --git a/releasenotes/notes/no-last-error-overwrite-b90aac3303eb992e.yaml b/releasenotes/notes/no-last-error-overwrite-b90aac3303eb992e.yaml new file mode 100644 index 0000000000..29c55ff69d --- /dev/null +++ b/releasenotes/notes/no-last-error-overwrite-b90aac3303eb992e.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - | + Fixes empty ``last_error`` field on cleaning failures.