From 66c34275f2defe94e68669fe4df1c4ea1830f720 Mon Sep 17 00:00:00 2001 From: Johannes Erdfelt Date: Wed, 19 Sep 2012 21:07:53 +0000 Subject: Clean up test_state_revert 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 --- nova/compute/manager.py | 2 ++ nova/tests/compute/test_compute.py | 59 +++++++++++++++++++++++--------------- 2 files changed, 38 insertions(+), 23 deletions(-) (limited to 'nova') diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 6088ba5e6..f8d07d92b 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -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): diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py index 14edaa7a0..d09ae3b32 100644 --- a/nova/tests/compute/test_compute.py +++ b/nova/tests/compute/test_compute.py @@ -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""" -- cgit