Merge "Don't raise exception from set_failed_state()"

ironic/drivers/modules/deploy_utils.py

@@ -826,11 +826,16 @@ def set_failed_state(task, msg):
 
     :param task: a TaskManager instance containing the node to act on.
     :param msg: the message to set in last_error of the node.
-    :raises: InvalidState if the event is not allowed by the associated
-             state machine.
     """
-    task.process_event('fail')
     node = task.node
+    try:
+        task.process_event('fail')
+    except exception.InvalidState:
+        msg2 = (_LE('Internal error. Node %(node)s in provision state '
+                    '"%(state)s" could not transition to a failed state.')
+                % {'node': node.uuid, 'state': node.provision_state})
+        LOG.exception(msg2)
+
     try:
         manager_utils.node_power_action(task, states.POWER_OFF)
     except Exception:
@@ -839,11 +844,11 @@ def set_failed_state(task, msg):
                     'should be removed from Ironic or put in maintenance '
                     'mode until the problem is resolved.') % node.uuid)
         LOG.exception(msg2)
-    finally:
+
-        # NOTE(deva): node_power_action() erases node.last_error
+    # NOTE(deva): node_power_action() erases node.last_error
-        #             so we need to set it again here.
+    #             so we need to set it here.
-        node.last_error = msg
+    node.last_error = msg
-        node.save()
+    node.save()
 
 
 def get_single_nic_with_vif_port_id(task):

ironic/tests/drivers/test_deploy_utils.py

@@ -1033,6 +1033,40 @@ class OtherFunctionTestCase(db_base.DbTestCase):
         self.assertRaises(exception.InvalidParameterValue,
                           utils.parse_root_device_hints, self.node)
 
+    @mock.patch.object(utils, 'LOG', autospec=True)
+    @mock.patch.object(manager_utils, 'node_power_action', autospec=True)
+    @mock.patch.object(task_manager.TaskManager, 'process_event',
+                       autospec=True)
+    def _test_set_failed_state(self, mock_event, mock_power, mock_log,
+                               event_value=None, power_value=None,
+                               log_calls=None):
+        err_msg = 'some failure'
+        mock_event.side_effect = event_value
+        mock_power.side_effect = power_value
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=False) as task:
+            utils.set_failed_state(task, err_msg)
+            mock_event.assert_called_once_with(task, 'fail')
+            mock_power.assert_called_once_with(task, states.POWER_OFF)
+            self.assertEqual(err_msg, task.node.last_error)
+            if log_calls:
+                mock_log.exception.assert_has_calls(log_calls)
+            else:
+                self.assertFalse(mock_log.called)
+
+    def test_set_failed_state(self):
+        exc_state = exception.InvalidState('invalid state')
+        exc_param = exception.InvalidParameterValue('invalid parameter')
+        mock_call = mock.call(mock.ANY)
+        self._test_set_failed_state()
+        self._test_set_failed_state(event_value=exc_state,
+                                    log_calls=[mock_call])
+        self._test_set_failed_state(power_value=exc_param,
+                                    log_calls=[mock_call])
+        self._test_set_failed_state(event_value=exc_state,
+                                    power_value=exc_param,
+                                    log_calls=[mock_call, mock_call])
+
 
 @mock.patch.object(disk_partitioner.DiskPartitioner, 'commit', lambda _: None)
 class WorkOnDiskTestCase(tests_base.TestCase):