diff options
| author | Dmitry Tantsur <divius.inside@gmail.com> | 2018-02-21 15:58:05 +0100 |
|---|---|---|
| committer | Dmitry Tantsur <divius.inside@gmail.com> | 2018-03-27 12:20:20 +0000 |
| commit | 26f1e784daa5210f0ca13b2aa49c8462a25bdf32 (patch) | |
| tree | d8b602f615d6cc149bf494ee66b1be485e5ce65c | |
| parent | ef089271479ea65fc3200cd9e2bbe69a3d004e07 (diff) | |
| download | ironic-26f1e784daa5210f0ca13b2aa49c8462a25bdf32.tar.gz | |
Prevent overwriting of last_error on cleaning failures
This changes moves the call to tear_down_cleaning to before we set
the last_error and maintenance_reason fields. Thus we avoid
overwriting last_error by e.g. power actions.
Related-Bug: #1588901
Change-Id: Ia448431a2922ea6f7adc27065dbcab1ba8358daa
(cherry picked from commit b93e5b05c43bd1ce23c7ffa85ee0ef1e8aa582ea)
| -rw-r--r-- | ironic/conductor/utils.py | 17 | ||||
| -rw-r--r-- | ironic/tests/unit/conductor/test_utils.py | 12 | ||||
| -rw-r--r-- | releasenotes/notes/no-last-error-overwrite-b90aac3303eb992e.yaml | 4 |
3 files changed, 24 insertions, 9 deletions
diff --git a/ironic/conductor/utils.py b/ironic/conductor/utils.py index 057d4dcd5..94075a4e6 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 f1d96618c..c35e1c31a 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 000000000..29c55ff69 --- /dev/null +++ b/releasenotes/notes/no-last-error-overwrite-b90aac3303eb992e.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - | + Fixes empty ``last_error`` field on cleaning failures. |