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

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

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):

releasenotes/notes/no-last-error-overwrite-b90aac3303eb992e.yaml

@@ -0,0 +1,4 @@
+---
+fixes:
+  - |
+    Fixes empty ``last_error`` field on cleaning failures.