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. This patch is required to be back-ported in order to back-port the commit8ceaad42ff. Change-Id: I7f84ff23401997c2b92759c306a7bff0589c7f4c (cherry picked from commit0d788525df)
This commit is contained in:
John L. Villalovos
parent
370ce2847d
commit
5c27638d1b
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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):
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue