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

@@ -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

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):