From ed6c599083e9ddf8215c013dfe69947e86eeeaf0 Mon Sep 17 00:00:00 2001 From: Naohiro Tamura Date: Tue, 25 Aug 2015 17:39:46 +0900 Subject: [PATCH] Ipmitool power driver for soft reboot and soft power off This patch enhances ipmitool power driver to support SOFT_REBOOT and SOFT_POWER_OFF. Partial-Bug: #1526226 Change-Id: If01721625c22a578b4311b82104cd895139e3a01 --- ironic/drivers/modules/ipmitool.py | 129 ++++++--- .../unit/drivers/modules/test_ipmitool.py | 255 +++++++++++++++++- .../ipmitool-soft-power-d5599dcb69eb0f5e.yaml | 4 + 3 files changed, 349 insertions(+), 39 deletions(-) create mode 100644 releasenotes/notes/ipmitool-soft-power-d5599dcb69eb0f5e.yaml diff --git a/ironic/drivers/modules/ipmitool.py b/ironic/drivers/modules/ipmitool.py index 9388dd5408..e646862774 100644 --- a/ironic/drivers/modules/ipmitool.py +++ b/ironic/drivers/modules/ipmitool.py @@ -471,7 +471,7 @@ def _sleep_time(iter): return iter ** 2 -def _set_and_wait(target_state, driver_info): +def _set_and_wait(power_action, driver_info, timeout=None): """Helper function for DynamicLoopingCall. This method changes the power state and polls the BMC until the desired @@ -483,21 +483,32 @@ def _set_and_wait(target_state, driver_info): if a driver is concerned, the state should be checked prior to calling this method. - :param target_state: desired power state + :param power_action: the action Ironic will perform when changing the + power state of the node. + :param timeout: timeout (in seconds) positive integer (> 0) for any + power state. ``None`` indicates to use default timeout. :param driver_info: the ipmitool parameters for accessing a node. :returns: one of ironic.common.states """ - if target_state == states.POWER_ON: - state_name = "on" - elif target_state == states.POWER_OFF: - state_name = "off" + retry_timeout = timeout or CONF.ipmi.retry_timeout + + if power_action == states.POWER_ON: + cmd_name = "on" + target_state = states.POWER_ON + elif power_action == states.POWER_OFF: + cmd_name = "off" + target_state = states.POWER_OFF + elif power_action == states.SOFT_POWER_OFF: + cmd_name = "soft" + target_state = states.POWER_OFF + retry_timeout = timeout or CONF.conductor.soft_power_off_timeout def _wait(mutable): try: # Only issue power change command once if mutable['iter'] < 0: - _exec_ipmitool(driver_info, "power %s" % state_name) + _exec_ipmitool(driver_info, "power %s" % cmd_name) else: mutable['power'] = _power_status(driver_info) except (exception.PasswordFileFailedToCreate, @@ -505,7 +516,7 @@ def _set_and_wait(target_state, driver_info): exception.IPMIFailure): # Log failures but keep trying LOG.warning(_LW("IPMI power %(state)s failed for node %(node)s."), - {'state': state_name, 'node': driver_info['uuid']}) + {'state': cmd_name, 'node': driver_info['uuid']}) finally: mutable['iter'] += 1 @@ -513,11 +524,11 @@ def _set_and_wait(target_state, driver_info): raise loopingcall.LoopingCallDone() sleep_time = _sleep_time(mutable['iter']) - if (sleep_time + mutable['total_time']) > CONF.ipmi.retry_timeout: + if (sleep_time + mutable['total_time']) > retry_timeout: # Stop if the next loop would exceed maximum retry_timeout LOG.error(_LE('IPMI power %(state)s timed out after ' '%(tries)s retries on node %(node_id)s.'), - {'state': state_name, 'tries': mutable['iter'], + {'state': cmd_name, 'tries': mutable['iter'], 'node_id': driver_info['uuid']}) mutable['power'] = states.ERROR raise loopingcall.LoopingCallDone() @@ -534,26 +545,43 @@ def _set_and_wait(target_state, driver_info): return status['power'] -def _power_on(driver_info): +def _power_on(driver_info, timeout=None): """Turn the power ON for this node. :param driver_info: the ipmitool parameters for accessing a node. + :param timeout: the timeout in seconds (> 0) to wait for the power + action to be completed. ``None`` indicates default timeout". :returns: one of ironic.common.states POWER_ON or ERROR. :raises: IPMIFailure on an error from ipmitool (from _power_status call). """ - return _set_and_wait(states.POWER_ON, driver_info) + return _set_and_wait(states.POWER_ON, driver_info, timeout=timeout) -def _power_off(driver_info): +def _power_off(driver_info, timeout=None): """Turn the power OFF for this node. :param driver_info: the ipmitool parameters for accessing a node. + :param timeout: the timeout in seconds (> 0) to wait for the power + action to be completed. ``None`` indicates default timeout". :returns: one of ironic.common.states POWER_OFF or ERROR. :raises: IPMIFailure on an error from ipmitool (from _power_status call). """ - return _set_and_wait(states.POWER_OFF, driver_info) + return _set_and_wait(states.POWER_OFF, driver_info, timeout=timeout) + + +def _soft_power_off(driver_info, timeout=None): + """Turn the power SOFT OFF for this node. + + :param driver_info: the ipmitool parameters for accessing a node. + :param timeout: the timeout in seconds (> 0) to wait for the power + action to be completed. ``None`` indicates default timeout". + :returns: one of ironic.common.states POWER_OFF or ERROR. + :raises: IPMIFailure on an error from ipmitool (from _power_status call). + + """ + return _set_and_wait(states.SOFT_POWER_OFF, driver_info, timeout=timeout) def _power_status(driver_info): @@ -793,12 +821,17 @@ class IPMIPower(base.PowerInterface): @METRICS.timer('IPMIPower.set_power_state') @task_manager.require_exclusive_lock - def set_power_state(self, task, pstate): - """Turn the power on or off. + def set_power_state(self, task, power_state, timeout=None): + """Turn the power on, off, soft reboot, or soft power off. :param task: a TaskManager instance containing the node to act on. - :param pstate: The desired power state, one of ironic.common.states - POWER_ON, POWER_OFF. + :param power_state: desired power state. + one of ironic.common.states, POWER_ON, POWER_OFF, SOFT_POWER_OFF, + or SOFT_REBOOT. + :param timeout: timeout (in seconds) positive integer (> 0) for any + power state. The timeout is counted once during power off and once + during power on for reboots. ``None`` indicates that the default + timeout will be used. :raises: InvalidParameterValue if an invalid power state was specified. :raises: MissingParameterValue if required ipmi parameters are missing :raises: PowerStateFailure if the power couldn't be set to pstate. @@ -806,41 +839,77 @@ class IPMIPower(base.PowerInterface): """ driver_info = _parse_driver_info(task.node) - if pstate == states.POWER_ON: + if power_state == states.POWER_ON: driver_utils.ensure_next_boot_device(task, driver_info) - state = _power_on(driver_info) - elif pstate == states.POWER_OFF: - state = _power_off(driver_info) + target_state = states.POWER_ON + state = _power_on(driver_info, timeout=timeout) + elif power_state == states.POWER_OFF: + target_state = states.POWER_OFF + state = _power_off(driver_info, timeout=timeout) + elif power_state == states.SOFT_POWER_OFF: + target_state = states.POWER_OFF + state = _soft_power_off(driver_info, timeout=timeout) + elif power_state == states.SOFT_REBOOT: + intermediate_state = _soft_power_off(driver_info, timeout=timeout) + intermediate_target_state = states.POWER_OFF + if intermediate_state != intermediate_target_state: + raise exception.PowerStateFailure( + pstate=(_( + "%(intermediate)s while on %(power_state)s") % + {'intermediate': intermediate_target_state, + 'power_state': power_state})) + driver_utils.ensure_next_boot_device(task, driver_info) + target_state = states.POWER_ON + state = _power_on(driver_info, timeout=timeout) else: raise exception.InvalidParameterValue( _("set_power_state called " - "with invalid power state %s.") % pstate) + "with invalid power state %s.") % power_state) - if state != pstate: - raise exception.PowerStateFailure(pstate=pstate) + if state != target_state: + raise exception.PowerStateFailure(pstate=target_state) @METRICS.timer('IPMIPower.reboot') @task_manager.require_exclusive_lock - def reboot(self, task): + def reboot(self, task, timeout=None): """Cycles the power to the task's node. :param task: a TaskManager instance containing the node to act on. + :param timeout: timeout (in seconds) positive integer (> 0) for any + power state. The timeout is counted once during power off and once + during power on for reboots. ``None`` indicates that the default + timeout will be used. :raises: MissingParameterValue if required ipmi parameters are missing. :raises: InvalidParameterValue if an invalid power state was specified. :raises: PowerStateFailure if the final state of the node is not - POWER_ON or the intermediate state of the node is not POWER_OFF. + POWER_ON or the intermediate state of the node is not POWER_OFF. """ driver_info = _parse_driver_info(task.node) - intermediate_state = _power_off(driver_info) + intermediate_state = _power_off(driver_info, timeout=timeout) if intermediate_state != states.POWER_OFF: - raise exception.PowerStateFailure(pstate=states.POWER_OFF) + raise exception.PowerStateFailure( + pstate=(_( + "%(power_off)s while on %(reboot)s") % + {'power_off': states.POWER_OFF, + 'reboot': states.REBOOT})) driver_utils.ensure_next_boot_device(task, driver_info) - state = _power_on(driver_info) + state = _power_on(driver_info, timeout=timeout) if state != states.POWER_ON: raise exception.PowerStateFailure(pstate=states.POWER_ON) + def get_supported_power_states(self, task): + """Get a list of the supported power states. + + :param task: A TaskManager instance containing the node to act on. + currently not used. + :returns: A list with the supported power states defined + in :mod:`ironic.common.states`. + """ + return [states.POWER_ON, states.POWER_OFF, states.REBOOT, + states.SOFT_REBOOT, states.SOFT_POWER_OFF] + class IPMIManagement(base.ManagementInterface): diff --git a/ironic/tests/unit/drivers/modules/test_ipmitool.py b/ironic/tests/unit/drivers/modules/test_ipmitool.py index 1f311f218e..30f08f30db 100644 --- a/ironic/tests/unit/drivers/modules/test_ipmitool.py +++ b/ironic/tests/unit/drivers/modules/test_ipmitool.py @@ -1257,6 +1257,47 @@ class IPMIToolPrivateMethodTestCase(db_base.DbTestCase): self.assertEqual(mock_exec.call_args_list, expected) self.assertEqual(states.ERROR, state) + @mock.patch.object(ipmi, '_exec_ipmitool', autospec=True) + @mock.patch('eventlet.greenthread.sleep', autospec=True) + def test__soft_power_off(self, sleep_mock, mock_exec, + mock_sleep): + + def side_effect(driver_info, command): + resp_dict = {"power status": ["Chassis Power is off\n", None], + "power soft": [None, None]} + return resp_dict.get(command, ["Bad\n", None]) + + mock_exec.side_effect = side_effect + + expected = [mock.call(self.info, "power soft"), + mock.call(self.info, "power status")] + + state = ipmi._soft_power_off(self.info, timeout=None) + + self.assertEqual(mock_exec.call_args_list, expected) + self.assertEqual(states.POWER_OFF, state) + + @mock.patch.object(ipmi, '_exec_ipmitool', autospec=True) + @mock.patch('eventlet.greenthread.sleep', autospec=True) + def test__soft_power_off_max_retries(self, sleep_mock, mock_exec, + mock_sleep): + + def side_effect(driver_info, command): + resp_dict = {"power status": ["Chassis Power is on\n", None], + "power soft": [None, None]} + return resp_dict.get(command, ["Bad\n", None]) + + mock_exec.side_effect = side_effect + + expected = [mock.call(self.info, "power soft"), + mock.call(self.info, "power status"), + mock.call(self.info, "power status")] + + state = ipmi._soft_power_off(self.info, timeout=2) + + self.assertEqual(mock_exec.call_args_list, expected) + self.assertEqual(states.ERROR, state) + class IPMIToolDriverTestCase(db_base.DbTestCase): @@ -1337,7 +1378,22 @@ class IPMIToolDriverTestCase(db_base.DbTestCase): self.driver.power.set_power_state(task, states.POWER_ON) - mock_on.assert_called_once_with(self.info) + mock_on.assert_called_once_with(self.info, timeout=None) + self.assertFalse(mock_off.called) + + @mock.patch.object(ipmi, '_power_on', autospec=True) + @mock.patch.object(ipmi, '_power_off', autospec=True) + def test_set_power_on_timeout_ok(self, mock_off, mock_on): + self.config(retry_timeout=0, group='ipmi') + + mock_on.return_value = states.POWER_ON + with task_manager.acquire(self.context, + self.node.uuid) as task: + self.driver.power.set_power_state(task, + states.POWER_ON, + timeout=2) + + mock_on.assert_called_once_with(self.info, timeout=2) self.assertFalse(mock_off.called) @mock.patch.object(driver_utils, 'ensure_next_boot_device', autospec=True) @@ -1354,7 +1410,25 @@ class IPMIToolDriverTestCase(db_base.DbTestCase): states.POWER_ON) mock_next_boot.assert_called_once_with(task, self.info) - mock_on.assert_called_once_with(self.info) + mock_on.assert_called_once_with(self.info, timeout=None) + self.assertFalse(mock_off.called) + + @mock.patch.object(driver_utils, 'ensure_next_boot_device', autospec=True) + @mock.patch.object(ipmi, '_power_on', autospec=True) + @mock.patch.object(ipmi, '_power_off', autospec=True) + def test_set_power_on_with_next_boot_timeout(self, mock_off, mock_on, + mock_next_boot): + self.config(retry_timeout=0, group='ipmi') + + mock_on.return_value = states.POWER_ON + with task_manager.acquire(self.context, + self.node.uuid) as task: + self.driver.power.set_power_state(task, + states.POWER_ON, + timeout=2) + mock_next_boot.assert_called_once_with(task, self.info) + + mock_on.assert_called_once_with(self.info, timeout=2) self.assertFalse(mock_off.called) @mock.patch.object(ipmi, '_power_on', autospec=True) @@ -1369,7 +1443,113 @@ class IPMIToolDriverTestCase(db_base.DbTestCase): self.driver.power.set_power_state(task, states.POWER_OFF) - mock_off.assert_called_once_with(self.info) + mock_off.assert_called_once_with(self.info, timeout=None) + self.assertFalse(mock_on.called) + + @mock.patch.object(ipmi, '_power_on', autospec=True) + @mock.patch.object(ipmi, '_power_off', autospec=True) + def test_set_power_off_timeout_ok(self, mock_off, mock_on): + self.config(retry_timeout=0, group='ipmi') + + mock_off.return_value = states.POWER_OFF + + with task_manager.acquire(self.context, + self.node.uuid) as task: + self.driver.power.set_power_state(task, + states.POWER_OFF, + timeout=2) + + mock_off.assert_called_once_with(self.info, timeout=2) + self.assertFalse(mock_on.called) + + @mock.patch.object(ipmi, '_power_on', autospec=True) + @mock.patch.object(ipmi, '_soft_power_off', autospec=True) + def test_set_soft_power_off_ok(self, mock_off, mock_on): + self.config(retry_timeout=0, group='ipmi') + + mock_off.return_value = states.POWER_OFF + + with task_manager.acquire(self.context, + self.node['uuid']) as task: + self.driver.power.set_power_state(task, + states.SOFT_POWER_OFF) + + mock_off.assert_called_once_with(self.info, timeout=None) + self.assertFalse(mock_on.called) + + @mock.patch.object(ipmi, '_power_on', autospec=True) + @mock.patch.object(ipmi, '_soft_power_off', autospec=True) + def test_set_soft_power_off_timeout_ok(self, mock_off, mock_on): + self.config(retry_timeout=0, group='ipmi') + + mock_off.return_value = states.POWER_OFF + + with task_manager.acquire(self.context, + self.node['uuid']) as task: + self.driver.power.set_power_state(task, + states.SOFT_POWER_OFF, + timeout=2) + + mock_off.assert_called_once_with(self.info, timeout=2) + self.assertFalse(mock_on.called) + + @mock.patch.object(driver_utils, 'ensure_next_boot_device', autospec=True) + @mock.patch.object(ipmi, '_power_on', autospec=True) + @mock.patch.object(ipmi, '_soft_power_off', autospec=True) + def test_set_soft_reboot_ok(self, mock_off, mock_on, mock_next_boot): + self.config(retry_timeout=0, group='ipmi') + + mock_off.return_value = states.POWER_OFF + mock_on.return_value = states.POWER_ON + + with task_manager.acquire(self.context, + self.node['uuid']) as task: + self.driver.power.set_power_state(task, + states.SOFT_REBOOT) + mock_next_boot.assert_called_once_with(task, self.info) + + mock_off.assert_called_once_with(self.info, timeout=None) + mock_on.assert_called_once_with(self.info, timeout=None) + + @mock.patch.object(driver_utils, 'ensure_next_boot_device', autospec=True) + @mock.patch.object(ipmi, '_power_on', autospec=True) + @mock.patch.object(ipmi, '_soft_power_off', autospec=True) + def test_set_soft_reboot_timeout_ok(self, mock_off, mock_on, + mock_next_boot): + self.config(retry_timeout=0, group='ipmi') + + mock_off.return_value = states.POWER_OFF + mock_on.return_value = states.POWER_ON + + with task_manager.acquire(self.context, + self.node['uuid']) as task: + self.driver.power.set_power_state(task, + states.SOFT_REBOOT, + timeout=2) + mock_next_boot.assert_called_once_with(task, self.info) + + mock_off.assert_called_once_with(self.info, timeout=2) + mock_on.assert_called_once_with(self.info, timeout=2) + + @mock.patch.object(driver_utils, 'ensure_next_boot_device', autospec=True) + @mock.patch.object(ipmi, '_power_on', autospec=True) + @mock.patch.object(ipmi, '_soft_power_off', autospec=True) + def test_set_soft_reboot_timeout_fail(self, mock_off, mock_on, + mock_next_boot): + self.config(retry_timeout=0, group='ipmi') + + mock_off.return_value = states.POWER_ON + + with task_manager.acquire(self.context, + self.node['uuid']) as task: + self.assertRaises(exception.PowerStateFailure, + self.driver.power.set_power_state, + task, + states.SOFT_REBOOT, + timeout=2) + + mock_off.assert_called_once_with(self.info, timeout=2) + self.assertFalse(mock_next_boot.called) self.assertFalse(mock_on.called) @mock.patch.object(ipmi, '_power_on', autospec=True) @@ -1385,7 +1565,24 @@ class IPMIToolDriverTestCase(db_base.DbTestCase): task, states.POWER_ON) - mock_on.assert_called_once_with(self.info) + mock_on.assert_called_once_with(self.info, timeout=None) + self.assertFalse(mock_off.called) + + @mock.patch.object(ipmi, '_power_on', autospec=True) + @mock.patch.object(ipmi, '_power_off', autospec=True) + def test_set_power_on_timeout_fail(self, mock_off, mock_on): + self.config(retry_timeout=0, group='ipmi') + + mock_on.return_value = states.ERROR + with task_manager.acquire(self.context, + self.node.uuid) as task: + self.assertRaises(exception.PowerStateFailure, + self.driver.power.set_power_state, + task, + states.POWER_ON, + timeout=2) + + mock_on.assert_called_once_with(self.info, timeout=2) self.assertFalse(mock_off.called) def test_set_power_invalid_state(self): @@ -1458,8 +1655,8 @@ class IPMIToolDriverTestCase(db_base.DbTestCase): mock_on.return_value = states.POWER_ON manager.attach_mock(mock_off, 'power_off') manager.attach_mock(mock_on, 'power_on') - expected = [mock.call.power_off(self.info), - mock.call.power_on(self.info)] + expected = [mock.call.power_off(self.info, timeout=None), + mock.call.power_on(self.info, timeout=None)] with task_manager.acquire(self.context, self.node.uuid) as task: @@ -1468,6 +1665,26 @@ class IPMIToolDriverTestCase(db_base.DbTestCase): self.assertEqual(manager.mock_calls, expected) + @mock.patch.object(driver_utils, 'ensure_next_boot_device', autospec=True) + @mock.patch.object(ipmi, '_power_off', spec_set=types.FunctionType) + @mock.patch.object(ipmi, '_power_on', spec_set=types.FunctionType) + def test_reboot_timeout_ok(self, mock_on, mock_off, mock_next_boot): + manager = mock.MagicMock() + # NOTE(rloo): if autospec is True, then manager.mock_calls is empty + mock_off.return_value = states.POWER_OFF + mock_on.return_value = states.POWER_ON + manager.attach_mock(mock_off, 'power_off') + manager.attach_mock(mock_on, 'power_on') + expected = [mock.call.power_off(self.info, timeout=2), + mock.call.power_on(self.info, timeout=2)] + + with task_manager.acquire(self.context, + self.node.uuid) as task: + self.driver.power.reboot(task, timeout=2) + mock_next_boot.assert_called_once_with(task, self.info) + + self.assertEqual(manager.mock_calls, expected) + @mock.patch.object(ipmi, '_power_off', spec_set=types.FunctionType) @mock.patch.object(ipmi, '_power_on', spec_set=types.FunctionType) def test_reboot_fail_power_off(self, mock_on, mock_off): @@ -1476,7 +1693,7 @@ class IPMIToolDriverTestCase(db_base.DbTestCase): mock_off.return_value = states.ERROR manager.attach_mock(mock_off, 'power_off') manager.attach_mock(mock_on, 'power_on') - expected = [mock.call.power_off(self.info)] + expected = [mock.call.power_off(self.info, timeout=None)] with task_manager.acquire(self.context, self.node.uuid) as task: @@ -1495,8 +1712,8 @@ class IPMIToolDriverTestCase(db_base.DbTestCase): mock_on.return_value = states.ERROR manager.attach_mock(mock_off, 'power_off') manager.attach_mock(mock_on, 'power_on') - expected = [mock.call.power_off(self.info), - mock.call.power_on(self.info)] + expected = [mock.call.power_off(self.info, timeout=None), + mock.call.power_on(self.info, timeout=None)] with task_manager.acquire(self.context, self.node.uuid) as task: @@ -1506,6 +1723,26 @@ class IPMIToolDriverTestCase(db_base.DbTestCase): self.assertEqual(manager.mock_calls, expected) + @mock.patch.object(ipmi, '_power_off', spec_set=types.FunctionType) + @mock.patch.object(ipmi, '_power_on', spec_set=types.FunctionType) + def test_reboot_timeout_fail(self, mock_on, mock_off): + manager = mock.MagicMock() + # NOTE(rloo): if autospec is True, then manager.mock_calls is empty + mock_off.return_value = states.POWER_OFF + mock_on.return_value = states.ERROR + manager.attach_mock(mock_off, 'power_off') + manager.attach_mock(mock_on, 'power_on') + expected = [mock.call.power_off(self.info, timeout=2), + mock.call.power_on(self.info, timeout=2)] + + with task_manager.acquire(self.context, + self.node.uuid) as task: + self.assertRaises(exception.PowerStateFailure, + self.driver.power.reboot, + task, timeout=2) + + self.assertEqual(manager.mock_calls, expected) + @mock.patch.object(ipmi, '_parse_driver_info', autospec=True) def test_vendor_passthru_validate__parse_driver_info_fail(self, info_mock): info_mock.side_effect = exception.InvalidParameterValue("bad") diff --git a/releasenotes/notes/ipmitool-soft-power-d5599dcb69eb0f5e.yaml b/releasenotes/notes/ipmitool-soft-power-d5599dcb69eb0f5e.yaml new file mode 100644 index 0000000000..9e1ce306ee --- /dev/null +++ b/releasenotes/notes/ipmitool-soft-power-d5599dcb69eb0f5e.yaml @@ -0,0 +1,4 @@ +--- +features: + - Adds support for ``soft reboot`` and ``soft power off`` to + ipmitool driver.