redfish: split reboot into power off followed by power on

The current implementation may result in a false positive when
the reboot request is ignored completely or when a node stays
powered on some time after the request is received.

Also make error messages a bit more useful by mentioning the desired
power state and avoiding redundant "Redfish".

Task: 39679
Story: 2007527
Change-Id: I35984e315948fbbcf216c40b38397953fb9dc699

ironic/drivers/modules/redfish/power.py

@@ -51,6 +51,23 @@ TARGET_STATE_MAP = {
 }
 
 
+def _set_power_state(task, system, power_state, timeout=None):
+    """An internal helper to set a power state on the system.
+
+    :param task: a TaskManager instance containing the node to act on.
+    :param system: a Redfish System object.
+    :param power_state: Any power state from :mod:`ironic.common.states`.
+    :param timeout: Time to wait for the node to reach the requested state.
+    :raises: MissingParameterValue if a required parameter is missing.
+    :raises: RedfishConnectionError when it fails to connect to Redfish
+    :raises: RedfishError on an error from the Sushy library
+    """
+    system.reset_system(SET_POWER_STATE_MAP.get(power_state))
+    target_state = TARGET_STATE_MAP.get(power_state, power_state)
+    cond_utils.node_wait_for_power_state(task, target_state,
+                                         timeout=timeout)
+
+
 class RedfishPower(base.PowerInterface):
 
     def __init__(self):
@@ -107,18 +124,15 @@ class RedfishPower(base.PowerInterface):
         """
         system = redfish_utils.get_system(task.node)
         try:
-            system.reset_system(SET_POWER_STATE_MAP.get(power_state))
+            _set_power_state(task, system, power_state, timeout=timeout)
         except sushy.exceptions.SushyError as e:
-            error_msg = (_('Redfish set power state failed for node '
+            error_msg = (_('Setting power state to %(state)s failed for node '
                            '%(node)s. Error: %(error)s') %
-                         {'node': task.node.uuid, 'error': e})
+                         {'node': task.node.uuid, 'state': power_state,
+                          'error': e})
             LOG.error(error_msg)
             raise exception.RedfishError(error=error_msg)
 
-        target_state = TARGET_STATE_MAP.get(power_state, power_state)
-        cond_utils.node_wait_for_power_state(task, target_state,
-                                             timeout=timeout)
-
     @task_manager.require_exclusive_lock
     def reboot(self, task, timeout=None):
         """Perform a hard reboot of the task's node.
@@ -134,19 +148,19 @@ class RedfishPower(base.PowerInterface):
 
         try:
             if current_power_state == states.POWER_ON:
-                system.reset_system(SET_POWER_STATE_MAP.get(states.REBOOT))
+                next_state = states.POWER_OFF
-            else:
+                _set_power_state(task, system, next_state, timeout=timeout)
-                system.reset_system(SET_POWER_STATE_MAP.get(states.POWER_ON))
+
+            next_state = states.POWER_ON
+            _set_power_state(task, system, next_state, timeout=timeout)
         except sushy.exceptions.SushyError as e:
-            error_msg = (_('Redfish reboot failed for node %(node)s. '
+            error_msg = (_('Reboot failed for node %(node)s when setting '
-                           'Error: %(error)s') % {'node': task.node.uuid,
+                           'power state to %(state)s. Error: %(error)s') %
-                                                  'error': e})
+                         {'node': task.node.uuid, 'state': next_state,
+                          'error': e})
             LOG.error(error_msg)
             raise exception.RedfishError(error=error_msg)
 
-        cond_utils.node_wait_for_power_state(task, states.POWER_ON,
-                                             timeout=timeout)
-
     def get_supported_power_states(self, task):
         """Get a list of the supported power states.
 

ironic/tests/unit/drivers/modules/redfish/test_power.py

@@ -160,41 +160,57 @@ class RedfishPowerTestCase(db_base.DbTestCase):
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=False) as task:
             self.assertRaisesRegex(
-                exception.RedfishError, 'Redfish set power state',
+                exception.RedfishError, 'power on failed',
                 task.driver.power.set_power_state, task, states.POWER_ON)
             fake_system.reset_system.assert_called_once_with(
                 sushy.RESET_ON)
             mock_get_system.assert_called_once_with(task.node)
 
     @mock.patch.object(redfish_utils, 'get_system', autospec=True)
-    def test_reboot(self, mock_get_system):
+    def test_reboot_from_power_off(self, mock_get_system):
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=False) as task:
-            expected_values = [
+            system_result = [
-                (sushy.SYSTEM_POWER_STATE_ON, sushy.RESET_FORCE_RESTART),
+                # Initial state
-                (sushy.SYSTEM_POWER_STATE_OFF, sushy.RESET_ON)
+                mock.Mock(power_state=sushy.SYSTEM_POWER_STATE_OFF),
+                # Transient state - still powered off
+                mock.Mock(power_state=sushy.SYSTEM_POWER_STATE_OFF),
+                # Final state - down powering off
+                mock.Mock(power_state=sushy.SYSTEM_POWER_STATE_ON)
             ]
+            mock_get_system.side_effect = system_result
 
-            for current, expected in expected_values:
+            task.driver.power.reboot(task)
-                system_result = [
-                    # Initial state
-                    mock.Mock(power_state=current),
-                    # Transient state - powering off
-                    mock.Mock(power_state=sushy.SYSTEM_POWER_STATE_OFF),
-                    # Final state - down powering off
-                    mock.Mock(power_state=sushy.SYSTEM_POWER_STATE_ON)
-                ]
-                mock_get_system.side_effect = system_result
 
-                task.driver.power.reboot(task)
+            # Asserts
+            system_result[0].reset_system.assert_called_once_with(
+                sushy.RESET_ON)
+            mock_get_system.assert_called_with(task.node)
+            self.assertEqual(3, mock_get_system.call_count)
 
-                # Asserts
+    @mock.patch.object(redfish_utils, 'get_system', autospec=True)
-                system_result[0].reset_system.assert_called_once_with(expected)
+    def test_reboot_from_power_on(self, mock_get_system):
-                mock_get_system.assert_called_with(task.node)
+        with task_manager.acquire(self.context, self.node.uuid,
-                self.assertEqual(3, mock_get_system.call_count)
+                                  shared=False) as task:
+            system_result = [
+                # Initial state
+                mock.Mock(power_state=sushy.SYSTEM_POWER_STATE_ON),
+                # Transient state - powering off
+                mock.Mock(power_state=sushy.SYSTEM_POWER_STATE_OFF),
+                # Final state - down powering off
+                mock.Mock(power_state=sushy.SYSTEM_POWER_STATE_ON)
+            ]
+            mock_get_system.side_effect = system_result
 
-                # Reset mocks
+            task.driver.power.reboot(task)
-                mock_get_system.reset_mock()
+
+            # Asserts
+            system_result[0].reset_system.assert_has_calls([
+                mock.call(sushy.RESET_FORCE_OFF),
+                mock.call(sushy.RESET_ON),
+            ])
+            mock_get_system.assert_called_with(task.node)
+            self.assertEqual(3, mock_get_system.call_count)
 
     @mock.patch.object(redfish_utils, 'get_system', autospec=True)
     def test_reboot_not_reached(self, mock_get_system):
@@ -221,12 +237,41 @@ class RedfishPowerTestCase(db_base.DbTestCase):
                                   shared=False) as task:
             fake_system.power_state = sushy.SYSTEM_POWER_STATE_ON
             self.assertRaisesRegex(
-                exception.RedfishError, 'Redfish reboot failed',
+                exception.RedfishError, 'Reboot failed.*power off',
                 task.driver.power.reboot, task)
             fake_system.reset_system.assert_called_once_with(
-                sushy.RESET_FORCE_RESTART)
+                sushy.RESET_FORCE_OFF)
             mock_get_system.assert_called_once_with(task.node)
 
+    @mock.patch.object(sushy, 'Sushy', autospec=True)
+    @mock.patch.object(redfish_utils, 'get_system', autospec=True)
+    def test_reboot_fail_on_power_on(self, mock_get_system, mock_sushy):
+        system_result = [
+            # Initial state
+            mock.Mock(power_state=sushy.SYSTEM_POWER_STATE_ON),
+            # Transient state - powering off
+            mock.Mock(power_state=sushy.SYSTEM_POWER_STATE_OFF),
+            # Final state - down powering off
+            mock.Mock(power_state=sushy.SYSTEM_POWER_STATE_ON)
+        ]
+        mock_get_system.side_effect = system_result
+        fake_system = system_result[0]
+        fake_system.reset_system.side_effect = [
+            None,
+            sushy.exceptions.SushyError(),
+        ]
+
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=False) as task:
+            self.assertRaisesRegex(
+                exception.RedfishError, 'Reboot failed.*power on',
+                task.driver.power.reboot, task)
+            fake_system.reset_system.assert_has_calls([
+                mock.call(sushy.RESET_FORCE_OFF),
+                mock.call(sushy.RESET_ON),
+            ])
+            mock_get_system.assert_called_with(task.node)
+
     def test_get_supported_power_states(self):
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=True) as task:

releasenotes/notes/redfish-power-87062756bce8b047.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    Rebooting a node with the ``redfish`` power interface is now implemented
+    via a power off request followed by power on to avoid returning success
+    when a node stays powered on after the reboot request.