Merge "Reduce complexity of node_power_action() function"

ironic/conductor/utils.py

@@ -100,35 +100,32 @@ def node_wait_for_power_state(task, new_state, timeout=None):
         raise exception.PowerStateFailure(pstate=new_state)
 
 
-@task_manager.require_exclusive_lock
+def _calculate_target_state(new_state):
-def node_power_action(task, new_state, timeout=None):
-    """Change power state or reset for a node.
-
-    Perform the requested power action if the transition is required.
-
-    :param task: a TaskManager instance containing the node to act on.
-    :param new_state: Any power state from ironic.common.states.
-    :param timeout: timeout (in seconds) positive integer (> 0) for any
-      power state. ``None`` indicates to use default timeout.
-    :raises: InvalidParameterValue when the wrong state is specified
-             or the wrong driver info is specified.
-    :raises: StorageError when a failure occurs updating the node's
-             storage interface upon setting power on.
-    :raises: other exceptions by the node's power driver if something
-             wrong occurred during the power action.
-
-    """
-    notify_utils.emit_power_set_notification(
-        task, fields.NotificationLevel.INFO, fields.NotificationStatus.START,
-        new_state)
-    node = task.node
-
     if new_state in (states.POWER_ON, states.REBOOT, states.SOFT_REBOOT):
         target_state = states.POWER_ON
     elif new_state in (states.POWER_OFF, states.SOFT_POWER_OFF):
         target_state = states.POWER_OFF
     else:
         target_state = None
+    return target_state
+
+
+def _can_skip_state_change(task, new_state):
+    """Check if we can ignore the power state change request for the node.
+
+    Check if we should ignore the requested power state change. This can occur
+    if the requested power state is already the same as our current state. This
+    only works for power on and power off state changes. More complex power
+    state changes, like reboot, are not skipped.
+
+    :param task: a TaskManager instance containing the node to act on.
+    :param new_state: The requested power state to change to. This can be any
+                      power state from ironic.common.states.
+    :returns: True if should ignore the requested power state change. False
+              otherwise
+    """
+
+    node = task.node
 
     def _not_going_to_change():
         # Neither the ironic service nor the hardware has erred. The
@@ -168,16 +165,45 @@ def node_power_action(task, new_state, timeout=None):
     if curr_state == states.POWER_ON:
         if new_state == states.POWER_ON:
             _not_going_to_change()
-            return
+            return True
     elif curr_state == states.POWER_OFF:
         if new_state in (states.POWER_OFF, states.SOFT_POWER_OFF):
             _not_going_to_change()
-            return
+            return True
     else:
         # if curr_state == states.ERROR:
         # be optimistic and continue action
         LOG.warning("Driver returns ERROR power state for node %s.",
                     node.uuid)
+    return False
+
+
+@task_manager.require_exclusive_lock
+def node_power_action(task, new_state, timeout=None):
+    """Change power state or reset for a node.
+
+    Perform the requested power action if the transition is required.
+
+    :param task: a TaskManager instance containing the node to act on.
+    :param new_state: Any power state from ironic.common.states.
+    :param timeout: timeout (in seconds) positive integer (> 0) for any
+      power state. ``None`` indicates to use default timeout.
+    :raises: InvalidParameterValue when the wrong state is specified
+             or the wrong driver info is specified.
+    :raises: StorageError when a failure occurs updating the node's
+             storage interface upon setting power on.
+    :raises: other exceptions by the node's power driver if something
+             wrong occurred during the power action.
+
+    """
+    notify_utils.emit_power_set_notification(
+        task, fields.NotificationLevel.INFO, fields.NotificationStatus.START,
+        new_state)
+    node = task.node
+
+    if _can_skip_state_change(task, new_state):
+        return
+    target_state = _calculate_target_state(new_state)
 
     # Set the target_power_state and clear any last_error, if we're
     # starting a new operation. This will expose to other processes

ironic/tests/unit/conductor/test_utils.py

@@ -558,6 +558,145 @@ class NodePowerActionTestCase(db_base.DbTestCase):
             self.assertIsNone(node['target_power_state'])
             self.assertIsNone(node['last_error'])
 
+    def test__calculate_target_state(self):
+        for new_state in (states.POWER_ON, states.REBOOT, states.SOFT_REBOOT):
+            self.assertEqual(
+                states.POWER_ON,
+                conductor_utils._calculate_target_state(new_state))
+        for new_state in (states.POWER_OFF, states.SOFT_POWER_OFF):
+            self.assertEqual(
+                states.POWER_OFF,
+                conductor_utils._calculate_target_state(new_state))
+        self.assertIsNone(conductor_utils._calculate_target_state('bad_state'))
+
+    def test__can_skip_state_change_different_state(self):
+        """Test setting node state to different state.
+
+        Test that we should change state if requested state is different from
+        current state.
+        """
+        node = obj_utils.create_test_node(self.context,
+                                          uuid=uuidutils.generate_uuid(),
+                                          driver='fake',
+                                          last_error='anything but None',
+                                          power_state=states.POWER_ON)
+        task = task_manager.TaskManager(self.context, node.uuid)
+
+        with mock.patch.object(self.driver.power,
+                               'get_power_state') as get_power_mock:
+            get_power_mock.return_value = states.POWER_ON
+
+            result = conductor_utils._can_skip_state_change(
+                task, states.POWER_OFF)
+
+            self.assertFalse(result)
+            get_power_mock.assert_called_once_with(mock.ANY)
+
+    @mock.patch.object(conductor_utils, 'LOG', autospec=True)
+    def test__can_skip_state_change_same_state(self, mock_log):
+        """Test setting node state to its present state.
+
+        Test that we don't try to set the power state if the requested
+        state is the same as the current state.
+        """
+        node = obj_utils.create_test_node(self.context,
+                                          uuid=uuidutils.generate_uuid(),
+                                          driver='fake',
+                                          last_error='anything but None',
+                                          power_state=states.POWER_ON)
+        task = task_manager.TaskManager(self.context, node.uuid)
+
+        with mock.patch.object(self.driver.power,
+                               'get_power_state') as get_power_mock:
+            get_power_mock.return_value = states.POWER_ON
+
+            result = conductor_utils._can_skip_state_change(
+                task, states.POWER_ON)
+
+            self.assertTrue(result)
+            node.refresh()
+            get_power_mock.assert_called_once_with(mock.ANY)
+            self.assertEqual(states.POWER_ON, node['power_state'])
+            self.assertEqual(states.NOSTATE, node['target_power_state'])
+            self.assertIsNone(node['last_error'])
+            mock_log.warning.assert_called_once_with(
+                u"Not going to change node %(node)s power state because "
+                u"current state = requested state = '%(state)s'.",
+                {'state': states.POWER_ON, 'node': node.uuid})
+
+    def test__can_skip_state_change_db_not_in_sync(self):
+        """Test setting node state to its present state if DB is out of sync.
+
+        Under rare conditions (see bug #1403106) database might contain stale
+        information, make sure we fix it.
+        """
+        node = obj_utils.create_test_node(self.context,
+                                          uuid=uuidutils.generate_uuid(),
+                                          driver='fake',
+                                          last_error='anything but None',
+                                          power_state=states.POWER_ON)
+        task = task_manager.TaskManager(self.context, node.uuid)
+
+        with mock.patch.object(self.driver.power,
+                               'get_power_state') as get_power_mock:
+            get_power_mock.return_value = states.POWER_OFF
+
+            result = conductor_utils._can_skip_state_change(
+                task, states.POWER_OFF)
+
+            self.assertTrue(result)
+
+            node.refresh()
+            get_power_mock.assert_called_once_with(mock.ANY)
+            self.assertEqual(states.POWER_OFF, node['power_state'])
+            self.assertEqual(states.NOSTATE, node['target_power_state'])
+            self.assertIsNone(node['last_error'])
+
+    @mock.patch('ironic.objects.node.NodeSetPowerStateNotification')
+    def test__can_skip_state_change_failed_getting_state_notify(
+            self, mock_notif):
+        """Test for notification & exception when can't get power state.
+
+        Test to make sure we generate a notification and also that an exception
+        is raised when we can't get the current power state.
+        """
+        self.config(notification_level='info')
+        self.config(host='my-host')
+        # Required for exception handling
+        mock_notif.__name__ = 'NodeSetPowerStateNotification'
+        node = obj_utils.create_test_node(self.context,
+                                          uuid=uuidutils.generate_uuid(),
+                                          driver='fake',
+                                          power_state=states.POWER_ON)
+        task = task_manager.TaskManager(self.context, node.uuid)
+
+        with mock.patch.object(self.driver.power,
+                               'get_power_state') as get_power_state_mock:
+            get_power_state_mock.side_effect = (
+                exception.InvalidParameterValue('failed getting power state'))
+
+            self.assertRaises(exception.InvalidParameterValue,
+                              conductor_utils._can_skip_state_change,
+                              task,
+                              states.POWER_ON)
+
+            node.refresh()
+            get_power_state_mock.assert_called_once_with(mock.ANY)
+            self.assertEqual(states.POWER_ON, node.power_state)
+            self.assertEqual(states.NOSTATE, node['target_power_state'])
+            self.assertIsNotNone(node.last_error)
+
+            # 1 notification should be sent for the error
+            self.assertEqual(1, mock_notif.call_count)
+            self.assertEqual(1, mock_notif.return_value.emit.call_count)
+
+            notif_args = mock_notif.call_args_list[0][1]
+
+            self.assertNotificationEqual(notif_args,
+                                         'ironic-conductor', CONF.host,
+                                         'baremetal.node.power_set.error',
+                                         obj_fields.NotificationLevel.ERROR)
+
 
 class NodeSoftPowerActionTestCase(db_base.DbTestCase):