From 8ceaad42ff363581efd2f43737456d479ea980d7 Mon Sep 17 00:00:00 2001 From: "John L. Villalovos" Date: Mon, 25 Sep 2017 15:29:06 -0700 Subject: [PATCH] ipmitool: reboot: Don't power off node if already off Commit ee5d4942a1c33736ffe05ec01619142be400c2f4 changed the existing behavior so that if an ipmitool command fails when attempting to set the power state it causes a failure. The problem with that approach is that on some systems if the system is already in the desired power state, an error will be generated when ipmitool tries to change it to the desired power state. Now when doing a reboot command we check beforehand to see if the node is already off, if so then don't attempt to power off the node again. Also optimize ironic/conductor/utils.py node_power_action() so that it only checks a node's power status if it might perform an action based on the node's power status. Change-Id: If838aae871753ebfbdf359e0bbe3afcc54c4b559 Closes-Bug: #1718794 --- ironic/conductor/utils.py | 6 +++ ironic/drivers/modules/ipmitool.py | 6 ++- ironic/tests/unit/conductor/test_utils.py | 13 +++--- .../unit/drivers/modules/test_ipmitool.py | 41 ++++++++++++++++--- ...power-off-if-already-1452256167d40009.yaml | 9 ++++ 5 files changed, 64 insertions(+), 11 deletions(-) create mode 100644 releasenotes/notes/reboot-do-not-power-off-if-already-1452256167d40009.yaml diff --git a/ironic/conductor/utils.py b/ironic/conductor/utils.py index 4316dcdb71..2667862e10 100644 --- a/ironic/conductor/utils.py +++ b/ironic/conductor/utils.py @@ -124,6 +124,12 @@ def _can_skip_state_change(task, new_state): :returns: True if should ignore the requested power state change. False otherwise """ + # We only ignore certain state changes. So if the desired new_state is not + # one of them, then we can return early and not do an un-needed + # get_power_state() call + if new_state not in (states.POWER_ON, states.POWER_OFF, + states.SOFT_POWER_OFF): + return False node = task.node diff --git a/ironic/drivers/modules/ipmitool.py b/ironic/drivers/modules/ipmitool.py index 18c2e281bd..b6c505c3c9 100644 --- a/ironic/drivers/modules/ipmitool.py +++ b/ironic/drivers/modules/ipmitool.py @@ -833,7 +833,11 @@ class IPMIPower(base.PowerInterface): """ driver_info = _parse_driver_info(task.node) - _power_off(task, driver_info, timeout=timeout) + # NOTE(jlvillal): Some BMCs will error if setting power state to off if + # the node is already turned off. + current_status = _power_status(driver_info) + if current_status != states.POWER_OFF: + _power_off(task, driver_info, timeout=timeout) driver_utils.ensure_next_boot_device(task, driver_info) _power_on(task, driver_info, timeout=timeout) diff --git a/ironic/tests/unit/conductor/test_utils.py b/ironic/tests/unit/conductor/test_utils.py index 9496eeca54..7c3649ef13 100644 --- a/ironic/tests/unit/conductor/test_utils.py +++ b/ironic/tests/unit/conductor/test_utils.py @@ -179,7 +179,10 @@ class NodePowerActionTestCase(db_base.DbTestCase): task = task_manager.TaskManager(self.context, node.uuid) with mock.patch.object(self.driver.power, 'reboot') as reboot_mock: - conductor_utils.node_power_action(task, states.REBOOT) + with mock.patch.object(self.driver.power, + 'get_power_state') as get_power_mock: + conductor_utils.node_power_action(task, states.REBOOT) + self.assertFalse(get_power_mock.called) node.refresh() reboot_mock.assert_called_once_with(mock.ANY) @@ -205,7 +208,7 @@ class NodePowerActionTestCase(db_base.DbTestCase): "INVALID_POWER_STATE") node.refresh() - get_power_mock.assert_called_once_with(mock.ANY) + self.assertFalse(get_power_mock.called) self.assertEqual(states.POWER_ON, node['power_state']) self.assertIsNone(node['target_power_state']) self.assertIsNotNone(node['last_error']) @@ -240,7 +243,7 @@ class NodePowerActionTestCase(db_base.DbTestCase): "INVALID_POWER_STATE") node.refresh() - get_power_mock.assert_called_once_with(mock.ANY) + self.assertFalse(get_power_mock.called) self.assertEqual(states.POWER_ON, node.power_state) self.assertIsNone(node.target_power_state) self.assertIsNotNone(node.last_error) @@ -720,7 +723,7 @@ class NodeSoftPowerActionTestCase(db_base.DbTestCase): conductor_utils.node_power_action(task, states.SOFT_REBOOT) node.refresh() - get_power_mock.assert_called_once_with(mock.ANY) + self.assertFalse(get_power_mock.called) self.assertEqual(states.POWER_ON, node['power_state']) self.assertIsNone(node['target_power_state']) self.assertIsNone(node['last_error']) @@ -741,7 +744,7 @@ class NodeSoftPowerActionTestCase(db_base.DbTestCase): timeout=2) node.refresh() - get_power_mock.assert_called_once_with(mock.ANY) + self.assertFalse(get_power_mock.called) self.assertEqual(states.POWER_ON, node['power_state']) self.assertIsNone(node['target_power_state']) self.assertIsNone(node['last_error']) diff --git a/ironic/tests/unit/drivers/modules/test_ipmitool.py b/ironic/tests/unit/drivers/modules/test_ipmitool.py index d6d85268bb..82119a7596 100644 --- a/ironic/tests/unit/drivers/modules/test_ipmitool.py +++ b/ironic/tests/unit/drivers/modules/test_ipmitool.py @@ -1684,6 +1684,8 @@ class IPMIToolDriverTestCase(db_base.DbTestCase): @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) + @mock.patch.object(ipmi, '_power_status', + lambda driver_info: states.POWER_ON) def test_reboot_ok(self, mock_on, mock_off, mock_next_boot): manager = mock.MagicMock() # NOTE(rloo): if autospec is True, then manager.mock_calls is empty @@ -1699,11 +1701,34 @@ class IPMIToolDriverTestCase(db_base.DbTestCase): self.driver.power.reboot(task) mock_next_boot.assert_called_once_with(task, self.info) - self.assertEqual(manager.mock_calls, expected) + self.assertEqual(expected, manager.mock_calls) @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) + @mock.patch.object(ipmi, '_power_status', + lambda driver_info: states.POWER_OFF) + def test_reboot_already_off(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') + + with task_manager.acquire(self.context, + self.node.uuid) as task: + expected = [mock.call.power_on(task, self.info, timeout=None)] + self.driver.power.reboot(task) + mock_next_boot.assert_called_once_with(task, self.info) + + self.assertEqual(expected, manager.mock_calls) + + @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) + @mock.patch.object(ipmi, '_power_status', + lambda driver_info: states.POWER_ON) 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 @@ -1718,10 +1743,12 @@ class IPMIToolDriverTestCase(db_base.DbTestCase): self.driver.power.reboot(task, timeout=2) mock_next_boot.assert_called_once_with(task, self.info) - self.assertEqual(manager.mock_calls, expected) + self.assertEqual(expected, manager.mock_calls) @mock.patch.object(ipmi, '_power_off', spec_set=types.FunctionType) @mock.patch.object(ipmi, '_power_on', spec_set=types.FunctionType) + @mock.patch.object(ipmi, '_power_status', + lambda driver_info: states.POWER_ON) def test_reboot_fail_power_off(self, mock_on, mock_off): manager = mock.MagicMock() # NOTE(rloo): if autospec is True, then manager.mock_calls is empty @@ -1737,10 +1764,12 @@ class IPMIToolDriverTestCase(db_base.DbTestCase): self.driver.power.reboot, task) - self.assertEqual(manager.mock_calls, expected) + self.assertEqual(expected, manager.mock_calls) @mock.patch.object(ipmi, '_power_off', spec_set=types.FunctionType) @mock.patch.object(ipmi, '_power_on', spec_set=types.FunctionType) + @mock.patch.object(ipmi, '_power_status', + lambda driver_info: states.POWER_ON) def test_reboot_fail_power_on(self, mock_on, mock_off): manager = mock.MagicMock() # NOTE(rloo): if autospec is True, then manager.mock_calls is empty @@ -1758,10 +1787,12 @@ class IPMIToolDriverTestCase(db_base.DbTestCase): self.driver.power.reboot, task) - self.assertEqual(manager.mock_calls, expected) + self.assertEqual(expected, manager.mock_calls) @mock.patch.object(ipmi, '_power_off', spec_set=types.FunctionType) @mock.patch.object(ipmi, '_power_on', spec_set=types.FunctionType) + @mock.patch.object(ipmi, '_power_status', + lambda driver_info: states.POWER_ON) 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 @@ -1778,7 +1809,7 @@ class IPMIToolDriverTestCase(db_base.DbTestCase): self.driver.power.reboot, task, timeout=2) - self.assertEqual(manager.mock_calls, expected) + self.assertEqual(expected, manager.mock_calls) @mock.patch.object(ipmi, '_parse_driver_info', autospec=True) def test_vendor_passthru_validate__parse_driver_info_fail(self, info_mock): diff --git a/releasenotes/notes/reboot-do-not-power-off-if-already-1452256167d40009.yaml b/releasenotes/notes/reboot-do-not-power-off-if-already-1452256167d40009.yaml new file mode 100644 index 0000000000..2b6f60d45d --- /dev/null +++ b/releasenotes/notes/reboot-do-not-power-off-if-already-1452256167d40009.yaml @@ -0,0 +1,9 @@ +--- +fixes: + - | + Fixes a problem when using ipmitool and rebooting a node which would cause + a deploy to fail. Now when rebooting a node we check if the node is already + powered off, if it is we don't attempt to power off the node. This is + because some BMCs will error if the node is already powered off and an + ipmitool request is made to power it off. See + https://bugs.launchpad.net/ironic/+bug/1718794 for details.