openstack/nova
Code Issues Proposed changes

Clean up test_state_revert

Browse Source 
While the test is simply to ensure that the state is reverted, no
matter what the failure, it did so a bit bluntly. The use of Exception
on both the raising side and the catching side ended up masking a
variety of other unintended failures.

stop_instance and start_instance had bugs (fixed in parent patch)
which ended up raising a KeyError exception. This still ended up
resetting the state correctly because only the nested call to the
decorator did not use the keyword argument for instance.

rebuild_instance, revert_resize, prep_resize and resize_instance all
required multiple extra arguments that weren't passed. This raised
a TypeError exception.

revert_resize and resize_instance also required an elevated context
or nova.db.api.migration_get() would raise a exception.AdminRequired
exception.

Even with these unintended problems, the test still performed it's
intended purpose, to test that task state is reverted. However,
the use of Exception makes auditing the test harder.

Change-Id: I5150957375ab3e57e7971f5e15daba531bb58803
This commit is contained in:
Johannes Erdfelt
2012-09-19 21:07:53 +00:00
parent 51cc814e43
commit 66c34275f2
2 changed files with 38 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
nova/compute/manager.py
View File

@@ -1409,6 +1409,7 @@ class ComputeManager(manager.SchedulerDependentManager):
source machine.
"""
context = context.elevated()
migration_ref = self.db.migration_get(context, migration_id)
with self._error_out_instance_on_exception(context, instance['uuid'],
reservations):
@@ -1543,6 +1544,7 @@ class ComputeManager(manager.SchedulerDependentManager):
def resize_instance(self, context, instance,
migration_id, image, reservations=None):
"""Starts the migration of a running instance to another host."""
context = context.elevated()
migration_ref = self.db.migration_get(context, migration_id)
with self._error_out_instance_on_exception(context, instance['uuid'],
reservations):

59
nova/tests/compute/test_compute.py
View File

@@ -1279,7 +1279,10 @@ class ComputeTestCase(BaseTestCase):
instance=jsonutils.to_primitive(instance))
def _test_state_revert(self, operation, pre_task_state,
post_task_state):
post_task_state=None, kwargs=None):
if kwargs is None:
kwargs = {}
instance = self._create_fake_instance()
self.compute.run_instance(self.context, instance=instance)
@@ -1292,17 +1295,18 @@ class ComputeTestCase(BaseTestCase):
orig_notify = self.compute._notify_about_instance_usage
def _get_an_exception(*args, **kwargs):
raise Exception("This fails every single time!")
raise test.TestingException()
self.stubs.Set(self.context, 'elevated', _get_an_exception)
self.stubs.Set(self.compute,
'_notify_about_instance_usage', _get_an_exception)
func = getattr(self.compute, operation)
raised = False
try:
ret_val = getattr(self.compute, operation)(self.context,
instance=instance)
except Exception:
ret_val = func(self.context, instance=instance, **kwargs)
except test.TestingException:
raised = True
finally:
# self.context.elevated() is called in tearDown()
@@ -1320,28 +1324,37 @@ class ComputeTestCase(BaseTestCase):
def test_state_revert(self):
"""ensure that task_state is reverted after a failed operation"""
actions = [
("reboot_instance", task_states.REBOOTING, None),
("stop_instance", task_states.STOPPING, None),
("start_instance", task_states.STARTING, None),
("reboot_instance", task_states.REBOOTING),
("stop_instance", task_states.STOPPING),
("start_instance", task_states.STARTING),
("terminate_instance", task_states.DELETING,
task_states.DELETING),
("power_off_instance", task_states.POWERING_OFF, None),
("power_on_instance", task_states.POWERING_ON, None),
("rebuild_instance", task_states.REBUILDING, None),
("set_admin_password", task_states.UPDATING_PASSWORD, None),
("rescue_instance", task_states.RESCUING, None),
("unrescue_instance", task_states.UNRESCUING, None),
("revert_resize", task_states.RESIZE_REVERTING, None),
("prep_resize", task_states.RESIZE_PREP, None),
("resize_instance", task_states.RESIZE_PREP, None),
("pause_instance", task_states.PAUSING, None),
("unpause_instance", task_states.UNPAUSING, None),
("suspend_instance", task_states.SUSPENDING, None),
("resume_instance", task_states.RESUMING, None),
("power_off_instance", task_states.POWERING_OFF),
("power_on_instance", task_states.POWERING_ON),
("rebuild_instance", task_states.REBUILDING, None,
{'orig_image_ref': None,
'image_ref': None,
'injected_files': [],
'new_pass': ''}),
("set_admin_password", task_states.UPDATING_PASSWORD),
("rescue_instance", task_states.RESCUING),
("unrescue_instance", task_states.UNRESCUING),
("revert_resize", task_states.RESIZE_REVERTING, None,
{'migration_id': None}),
("prep_resize", task_states.RESIZE_PREP, None,
{'image': {},
'instance_type': {}}),
("resize_instance", task_states.RESIZE_PREP, None,
{'migration_id': None,
'image': {}}),
("pause_instance", task_states.PAUSING),
("unpause_instance", task_states.UNPAUSING),
("suspend_instance", task_states.SUSPENDING),
("resume_instance", task_states.RESUMING),
]
for operation, pre_state, post_state in actions:
self._test_state_revert(operation, pre_state, post_state)
for operation in actions:
self._test_state_revert(*operation)
def _ensure_quota_reservations_committed(self):
"""Mock up commit of quota reservations"""