openstack/ironic
Code Issues Proposed changes

Don't raise exception from set_failed_state()

Browse Source 
deploy_utils.set_failed_state() shouldn't raise any exceptions because
it is used to handle a failed deployment, and we want the node's
last_error to be set.

Instead, in the very rare (if ever) case that the call to
task.process_event() fails, we'll log an exception and continue.

Change-Id: I8f02577de5a5bce34608882f61b5b26f5b998fad
Closes-Bug: #1461670
This commit is contained in:
Ruby Loo 2015-06-03 21:52:14 +00:00
parent f7ada6181c
commit d13ec0b392
2 changed files with 47 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
ironic/drivers/modules/deploy_utils.py
View File

@ -826,11 +826,16 @@ def set_failed_state(task, msg):
:param task: a TaskManager instance containing the node to act on.
:param msg: the message to set in last_error of the node.
:raises: InvalidState if the event is not allowed by the associated
state machine.
"""
task.process_event('fail')
node = task.node
try:
task.process_event('fail')
except exception.InvalidState:
msg2 = (_LE('Internal error. Node %(node)s in provision state '
'"%(state)s" could not transition to a failed state.')
% {'node': node.uuid, 'state': node.provision_state})
LOG.exception(msg2)
try:
manager_utils.node_power_action(task, states.POWER_OFF)
except Exception:
@ -839,11 +844,11 @@ def set_failed_state(task, msg):
'should be removed from Ironic or put in maintenance '
'mode until the problem is resolved.') % node.uuid)
LOG.exception(msg2)
finally:
# NOTE(deva): node_power_action() erases node.last_error
# so we need to set it again here.
node.last_error = msg
node.save()
# NOTE(deva): node_power_action() erases node.last_error
# so we need to set it here.
node.last_error = msg
node.save()
def get_single_nic_with_vif_port_id(task):

34
ironic/tests/drivers/test_deploy_utils.py
View File

@ -1033,6 +1033,40 @@ class OtherFunctionTestCase(db_base.DbTestCase):
self.assertRaises(exception.InvalidParameterValue,
utils.parse_root_device_hints, self.node)
@mock.patch.object(utils, 'LOG', autospec=True)
@mock.patch.object(manager_utils, 'node_power_action', autospec=True)
@mock.patch.object(task_manager.TaskManager, 'process_event',
autospec=True)
def _test_set_failed_state(self, mock_event, mock_power, mock_log,
event_value=None, power_value=None,
log_calls=None):
err_msg = 'some failure'
mock_event.side_effect = event_value
mock_power.side_effect = power_value
with task_manager.acquire(self.context, self.node.uuid,
shared=False) as task:
utils.set_failed_state(task, err_msg)
mock_event.assert_called_once_with(task, 'fail')
mock_power.assert_called_once_with(task, states.POWER_OFF)
self.assertEqual(err_msg, task.node.last_error)
if log_calls:
mock_log.exception.assert_has_calls(log_calls)
else:
self.assertFalse(mock_log.called)
def test_set_failed_state(self):
exc_state = exception.InvalidState('invalid state')
exc_param = exception.InvalidParameterValue('invalid parameter')
mock_call = mock.call(mock.ANY)
self._test_set_failed_state()
self._test_set_failed_state(event_value=exc_state,
log_calls=[mock_call])
self._test_set_failed_state(power_value=exc_param,
log_calls=[mock_call])
self._test_set_failed_state(event_value=exc_state,
power_value=exc_param,
log_calls=[mock_call, mock_call])
@mock.patch.object(disk_partitioner.DiskPartitioner, 'commit', lambda _: None)
class WorkOnDiskTestCase(tests_base.TestCase):