Make redfish power interface wait for the power state change

Currently it's not consistent with other power interfaces.

Change-Id: I82dd74278121857f5bd810878961aa6fb50e53ff
Related-Bug: #1526477

ironic/drivers/modules/redfish/power.py

@@ -20,6 +20,7 @@ from ironic.common import exception
 from ironic.common.i18n import _
 from ironic.common import states
 from ironic.conductor import task_manager
+from ironic.conductor import utils as cond_utils
 from ironic.drivers import base
 from ironic.drivers.modules.redfish import utils as redfish_utils
 
@@ -43,6 +44,12 @@ if sushy:
         states.SOFT_POWER_OFF: sushy.RESET_GRACEFUL_SHUTDOWN
     }
 
+TARGET_STATE_MAP = {
+    states.REBOOT: states.POWER_ON,
+    states.SOFT_REBOOT: states.POWER_ON,
+    states.SOFT_POWER_OFF: states.POWER_OFF,
+}
+
 
 class RedfishPower(base.PowerInterface):
 
@@ -93,7 +100,7 @@ class RedfishPower(base.PowerInterface):
 
         :param task: a TaskManager instance containing the node to act on.
         :param power_state: Any power state from :mod:`ironic.common.states`.
-        :param timeout: Not used by this driver.
+        :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
@@ -108,12 +115,16 @@ class RedfishPower(base.PowerInterface):
             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.
 
         :param task: a TaskManager instance containing the node to act on.
-        :param timeout: Not used by this driver.
+        :param timeout: Time to wait for the node to become powered on.
         :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
@@ -133,6 +144,9 @@ class RedfishPower(base.PowerInterface):
             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

@@ -35,6 +35,7 @@ class MockedSushyError(Exception):
     pass
 
 
+@mock.patch('eventlet.greenthread.sleep', lambda _t: None)
 class RedfishPowerTestCase(db_base.DbTestCase):
 
     def setUp(self):
@@ -97,16 +98,58 @@ class RedfishPowerTestCase(db_base.DbTestCase):
                 (states.SOFT_POWER_OFF, sushy.RESET_GRACEFUL_SHUTDOWN)
             ]
 
-            fake_system = mock_get_system.return_value
             for target, expected in expected_values:
+                if target in (states.POWER_OFF, states.SOFT_POWER_OFF):
+                    final = sushy.SYSTEM_POWER_STATE_OFF
+                    transient = sushy.SYSTEM_POWER_STATE_ON
+                else:
+                    final = sushy.SYSTEM_POWER_STATE_ON
+                    transient = sushy.SYSTEM_POWER_STATE_OFF
+
+                system_result = [
+                    mock.Mock(power_state=transient)
+                ] * 3 + [mock.Mock(power_state=final)]
+                mock_get_system.side_effect = system_result
+
                 task.driver.power.set_power_state(task, target)
 
                 # Asserts
-                fake_system.reset_system.assert_called_once_with(expected)
+                system_result[0].reset_system.assert_called_once_with(expected)
-                mock_get_system.assert_called_once_with(task.node)
+                mock_get_system.assert_called_with(task.node)
+                self.assertEqual(4, mock_get_system.call_count)
+
+                # Reset mocks
+                mock_get_system.reset_mock()
+
+    @mock.patch.object(redfish_utils, 'get_system', autospec=True)
+    def test_set_power_state_not_reached(self, mock_get_system):
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=False) as task:
+            self.config(power_state_change_timeout=2, group='conductor')
+            expected_values = [
+                (states.POWER_ON, sushy.RESET_ON),
+                (states.POWER_OFF, sushy.RESET_FORCE_OFF),
+                (states.REBOOT, sushy.RESET_FORCE_RESTART),
+                (states.SOFT_REBOOT, sushy.RESET_GRACEFUL_RESTART),
+                (states.SOFT_POWER_OFF, sushy.RESET_GRACEFUL_SHUTDOWN)
+            ]
+
+            for target, expected in expected_values:
+                fake_system = mock_get_system.return_value
+                if target in (states.POWER_OFF, states.SOFT_POWER_OFF):
+                    fake_system.power_state = sushy.SYSTEM_POWER_STATE_ON
+                else:
+                    fake_system.power_state = sushy.SYSTEM_POWER_STATE_OFF
+
+                self.assertRaises(exception.PowerStateFailure,
+                                  task.driver.power.set_power_state,
+                                  task, target)
+
+                # Asserts
+                fake_system.reset_system.assert_called_once_with(expected)
+                mock_get_system.assert_called_with(task.node)
 
                 # Reset mocks
-                fake_system.reset_system.reset_mock()
                 mock_get_system.reset_mock()
 
     @mock.patch('ironic.drivers.modules.redfish.power.sushy')
@@ -127,7 +170,6 @@ class RedfishPowerTestCase(db_base.DbTestCase):
 
     @mock.patch.object(redfish_utils, 'get_system', autospec=True)
     def test_reboot(self, mock_get_system):
-        fake_system = mock_get_system.return_value
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=False) as task:
             expected_values = [
@@ -136,17 +178,40 @@ class RedfishPowerTestCase(db_base.DbTestCase):
             ]
 
             for current, expected in expected_values:
-                fake_system.power_state = current
+                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
-                fake_system.reset_system.assert_called_once_with(expected)
+                system_result[0].reset_system.assert_called_once_with(expected)
-                mock_get_system.assert_called_once_with(task.node)
+                mock_get_system.assert_called_with(task.node)
+                self.assertEqual(3, mock_get_system.call_count)
 
                 # Reset mocks
-                fake_system.reset_system.reset_mock()
                 mock_get_system.reset_mock()
 
+    @mock.patch.object(redfish_utils, 'get_system', autospec=True)
+    def test_reboot_not_reached(self, mock_get_system):
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=False) as task:
+            fake_system = mock_get_system.return_value
+            fake_system.power_state = sushy.SYSTEM_POWER_STATE_OFF
+
+            self.assertRaises(exception.PowerStateFailure,
+                              task.driver.power.reboot, task)
+
+            # Asserts
+            fake_system.reset_system.assert_called_once_with(sushy.RESET_ON)
+            mock_get_system.assert_called_with(task.node)
+
     @mock.patch('ironic.drivers.modules.redfish.power.sushy')
     @mock.patch.object(redfish_utils, 'get_system', autospec=True)
     def test_reboot_fail(self, mock_get_system, mock_sushy):

releasenotes/notes/redfish-power-wait-c5d95432a3a714c4.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    The ``redfish`` power interface now waits for a power action to apply.
+    Previously it used to return immediately, which was not consistent with
+    how other power interfaces work.