Merge "ipmitool: reboot: Don't power off node if already off" into stable/pike

5 files changed, 64 insertions, 11 deletions

diff --git a/ironic/conductor/utils.py b/ironic/conductor/utils.py
index 4316dcdb7..2667862e1 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 18c2e281b..b6c505c3c 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 9496eeca5..7c3649ef1 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 d6d85268b..82119a759 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 000000000..2b6f60d45
--- /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.