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.