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 9baee0f1d2..37b8b25981 100644
--- a/ironic/tests/unit/drivers/modules/test_ipmitool.py
+++ b/ironic/tests/unit/drivers/modules/test_ipmitool.py
@@ -1680,6 +1680,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
@@ -1695,11 +1697,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
@@ -1714,10 +1739,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
@@ -1733,10 +1760,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
@@ -1754,10 +1783,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
@@ -1774,7 +1805,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.