From 6b536b9171e0263acecf0ba6be7d6e17f3dee75f Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Wed, 26 Jul 2017 14:37:12 +0100 Subject: [PATCH] Fix scope of errors_out_migration in finish_resize _finish_resize executes in the context of the errors_out_migration decorator of the finish_resize function which calls it. It sets migration status to 'finished' when the migration completes before sending instance notifications. errors_out_migration_ctxt doesn't set the migration to error status unless it is 'migrating', or 'post-migrating', so an error sending instance notifications will not set the migration to an error state, which is what we want. The interaction with the status check in errors_out_migration_ctxt is not obvious to a maintainer. It is also problematic, and we remove it in change Ib6156d8e. This change reduces the lexical scope of errors_out_migration to the code where we want an error to be set. This is both immediately obvious to a maintainer, and allows us to later remove the status check in errors_out_migration_ctxt. In adding a test for the reduced scope, we also refactor test_finish_resize_failure to reuse its suite of finish_resize mocks. We take the opportunity to remove unnecessary mocks and simplify others, and to remove assertions which don't relate to code correctness or what we're testing. Change-Id: I9bcc1605945ddc35df2288be72e194c050b8ddd9 --- nova/compute/manager.py | 24 +++++----- nova/tests/unit/compute/test_compute_mgr.py | 50 +++++++++++++++------ 2 files changed, 49 insertions(+), 25 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index cc260b8210..ebd9a25e9a 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -3990,18 +3990,11 @@ class ComputeManager(manager.Manager): instance.launched_at = timeutils.utcnow() instance.save(expected_task_state=task_states.RESIZE_FINISH) - self._update_scheduler_instance_info(context, instance) - self._notify_about_instance_usage( - context, instance, "finish_resize.end", - network_info=network_info) - compute_utils.notify_about_instance_action(context, instance, - self.host, action=fields.NotificationAction.RESIZE_FINISH, - phase=fields.NotificationPhase.END) + return network_info @wrap_exception() @reverts_task_state @wrap_instance_event(prefix='compute') - @errors_out_migration @wrap_instance_fault def finish_resize(self, context, disk_info, image, instance, reservations, migration): @@ -4011,10 +4004,19 @@ class ComputeManager(manager.Manager): new host machine. """ - with self._error_out_instance_on_exception(context, instance): + with self._error_out_instance_on_exception(context, instance), \ + errors_out_migration_ctxt(migration): image_meta = objects.ImageMeta.from_dict(image) - self._finish_resize(context, instance, migration, - disk_info, image_meta) + network_info = self._finish_resize(context, instance, migration, + disk_info, image_meta) + + self._update_scheduler_instance_info(context, instance) + self._notify_about_instance_usage( + context, instance, "finish_resize.end", + network_info=network_info) + compute_utils.notify_about_instance_action(context, instance, + self.host, action=fields.NotificationAction.RESIZE_FINISH, + phase=fields.NotificationPhase.END) @wrap_exception() @wrap_instance_fault diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index d2ec76e17f..c37a94f22e 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -5419,29 +5419,51 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase): self.useFixture(fixtures.SpawnIsSynchronousFixture()) self.useFixture(fixtures.EventReporterStub()) - def test_finish_resize_failure(self): + @contextlib.contextmanager + def _mock_finish_resize(self): with test.nested( - mock.patch.object(self.compute, '_finish_resize', - side_effect=exception.ResizeError(reason='')), + mock.patch.object(self.compute, '_finish_resize'), mock.patch.object(db, 'instance_fault_create'), - mock.patch.object(self.compute, '_instance_update'), + mock.patch.object(self.compute, '_update_resource_tracker'), mock.patch.object(self.instance, 'save'), - mock.patch.object(self.migration, 'save'), - mock.patch.object(self.migration, 'obj_as_admin', - return_value=mock.MagicMock()) - ) as (meth, fault_create, instance_update, instance_save, - migration_save, migration_obj_as_admin): + ) as (_finish_resize, fault_create, instance_update, instance_save): fault_create.return_value = ( test_instance_fault.fake_faults['fake-uuid'][0]) + yield _finish_resize + + def test_finish_resize_failure(self): + migration = mock.NonCallableMagicMock() + migration.status = 'post-migrating' + + with self._mock_finish_resize() as _finish_resize: + _finish_resize.side_effect = self.TestResizeError self.assertRaises( - exception.ResizeError, self.compute.finish_resize, + self.TestResizeError, self.compute.finish_resize, context=self.context, disk_info=[], image=self.image, instance=self.instance, reservations=[], - migration=self.migration + migration=migration ) - self.assertEqual("error", self.migration.status) - migration_save.assert_called_once_with() - migration_obj_as_admin.assert_called_once_with() + + # Assert that we set the migration to an error state + self.assertEqual("error", migration.status) + + @mock.patch('nova.compute.manager.ComputeManager.' + '_notify_about_instance_usage') + def test_finish_resize_notify_failure(self, notify): + migration = mock.NonCallableMagicMock() + migration.status = 'post-migrating' + + with self._mock_finish_resize(): + notify.side_effect = self.TestResizeError + self.assertRaises( + self.TestResizeError, self.compute.finish_resize, + context=self.context, disk_info=[], image=self.image, + instance=self.instance, reservations=[], + migration=migration + ) + + # Assert that we did not set the migration to an error state + self.assertEqual('post-migrating', migration.status) @contextlib.contextmanager def _mock_resize_instance(self):