From 8562df092f4286afc1e592951c7006eb56a4bf09 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Tue, 5 May 2020 12:13:33 +0200 Subject: [PATCH] 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 | 46 +++++---- .../drivers/modules/redfish/test_power.py | 93 ++++++++++++++----- .../notes/redfish-power-87062756bce8b047.yaml | 6 ++ 3 files changed, 105 insertions(+), 40 deletions(-) create mode 100644 releasenotes/notes/redfish-power-87062756bce8b047.yaml diff --git a/ironic/drivers/modules/redfish/power.py b/ironic/drivers/modules/redfish/power.py index 8f568bfb9e..cb944a4273 100644 --- a/ironic/drivers/modules/redfish/power.py +++ b/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)) - else: - system.reset_system(SET_POWER_STATE_MAP.get(states.POWER_ON)) + next_state = states.POWER_OFF + _set_power_state(task, system, next_state, timeout=timeout) + + 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: %(error)s') % {'node': task.node.uuid, - 'error': e}) + error_msg = (_('Reboot failed for node %(node)s when setting ' + 'power state to %(state)s. Error: %(error)s') % + {'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. diff --git a/ironic/tests/unit/drivers/modules/redfish/test_power.py b/ironic/tests/unit/drivers/modules/redfish/test_power.py index 932d44af89..22470a7270 100644 --- a/ironic/tests/unit/drivers/modules/redfish/test_power.py +++ b/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 = [ - (sushy.SYSTEM_POWER_STATE_ON, sushy.RESET_FORCE_RESTART), - (sushy.SYSTEM_POWER_STATE_OFF, sushy.RESET_ON) + system_result = [ + # Initial state + 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: - 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) - 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 - system_result[0].reset_system.assert_called_once_with(expected) - 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_from_power_on(self, mock_get_system): + with task_manager.acquire(self.context, self.node.uuid, + 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 - mock_get_system.reset_mock() + task.driver.power.reboot(task) + + # 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: diff --git a/releasenotes/notes/redfish-power-87062756bce8b047.yaml b/releasenotes/notes/redfish-power-87062756bce8b047.yaml new file mode 100644 index 0000000000..03f3e2dfb6 --- /dev/null +++ b/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.