From 0d788525dfa0a7dee11b005318478a7cee597055 Mon Sep 17 00:00:00 2001 From: "John L. Villalovos" Date: Tue, 26 Sep 2017 13:19:53 -0700 Subject: [PATCH] Reduce complexity of node_power_action() function While attempting to modify the node_power_action() function it caused the complexity to exceed the 'pep8' check limits. This patch reduces the complexity of the node_power_action() function by refactoring it. This will allow future changes to node_power_action() to be allowed. Added unit tests to test the functions that were pulled out. Change-Id: I7f84ff23401997c2b92759c306a7bff0589c7f4c --- ironic/conductor/utils.py | 76 ++++++++---- ironic/tests/unit/conductor/test_utils.py | 139 ++++++++++++++++++++++ 2 files changed, 190 insertions(+), 25 deletions(-) diff --git a/ironic/conductor/utils.py b/ironic/conductor/utils.py index 99dcac93e55..4316dcdb716 100644 --- a/ironic/conductor/utils.py +++ b/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 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 - +def _calculate_target_state(new_state): 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 diff --git a/ironic/tests/unit/conductor/test_utils.py b/ironic/tests/unit/conductor/test_utils.py index 01ff22ad0cc..9496eeca54d 100644 --- a/ironic/tests/unit/conductor/test_utils.py +++ b/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):