Merge "Split Compute.errors_out_migration into a separate contextmanager"

2017-08-07 23:37:07 +00:00
parent f742baa4c3 a96d9f4bfe
commit e8fbcad8e9
2 changed files with 98 additions and 50 deletions
@@ -108,37 +108,43 @@ wrap_exception = functools.partial(exception_wrapper.wrap_exception,
                                   binary='nova-compute')


+@contextlib.contextmanager
+def errors_out_migration_ctxt(migration):
+    """Context manager to error out migration on failure."""
+
+    try:
+        yield
+    except Exception as ex:
+        with excutils.save_and_reraise_exception():
+            # NOTE(rajesht): If InstanceNotFound error is thrown from
+            # decorated function, migration status should be set to
+            # 'error', without checking current migration status.
+            if not isinstance(ex, exception.InstanceNotFound):
+                status = migration.status
+                if status not in ['migrating', 'post-migrating']:
+                    return
+
+            migration.status = 'error'
+            try:
+                with migration.obj_as_admin():
+                    migration.save()
+            except Exception:
+                LOG.debug('Error setting migration status for instance %s.',
+                          migration.instance_uuid, exc_info=True)
+
+
@utils.expects_func_args('migration')
 def errors_out_migration(function):
    """Decorator to error out migration on failure."""

    @functools.wraps(function)
    def decorated_function(self, context, *args, **kwargs):
-        try:
+        wrapped_func = safe_utils.get_wrapped_function(function)
+        keyed_args = inspect.getcallargs(wrapped_func, self, context,
+                                         *args, **kwargs)
+        migration = keyed_args['migration']
+        with errors_out_migration_ctxt(migration):
            return function(self, context, *args, **kwargs)
-        except Exception as ex:
-            with excutils.save_and_reraise_exception():
-                wrapped_func = safe_utils.get_wrapped_function(function)
-                keyed_args = inspect.getcallargs(wrapped_func, self, context,
-                                                 *args, **kwargs)
-                migration = keyed_args['migration']
-
-                # NOTE(rajesht): If InstanceNotFound error is thrown from
-                # decorated function, migration status should be set to
-                # 'error', without checking current migration status.
-                if not isinstance(ex, exception.InstanceNotFound):
-                    status = migration.status
-                    if status not in ['migrating', 'post-migrating']:
-                        return
-
-                migration.status = 'error'
-                try:
-                    with migration.obj_as_admin():
-                        migration.save()
-                except Exception:
-                    LOG.debug('Error setting migration status '
-                              'for instance %s.',
-                              migration.instance_uuid, exc_info=True)

    return decorated_function

@@ -18,6 +18,7 @@ import time

 from cinderclient import exceptions as cinder_exception
 from cursive import exception as cursive_exception
+import ddt
 from eventlet import event as eventlet_event
 import mock
 import netaddr
@@ -5331,6 +5332,73 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase):
            self.assertEqual(expected_call, create_error_call)


+@ddt.ddt
+class ComputeManagerErrorsOutMigrationTestCase(test.NoDBTestCase):
+    def setUp(self):
+        super(ComputeManagerErrorsOutMigrationTestCase, self).setUp()
+        self.context = context.RequestContext(fakes.FAKE_USER_ID,
+                                              fakes.FAKE_PROJECT_ID)
+
+        self.instance = fake_instance.fake_instance_obj(self.context)
+
+        self.migration = objects.Migration()
+        self.migration.instance_uuid = self.instance.uuid
+        self.migration.status = 'migrating'
+        self.migration.id = 0
+
+    @mock.patch.object(objects.Migration, 'save')
+    @mock.patch.object(objects.Migration, 'obj_as_admin')
+    def test_decorator(self, mock_save, mock_obj_as_admin):
+        # Tests that errors_out_migration decorator in compute manager sets
+        # migration status to 'error' when an exception is raised from
+        # decorated method
+
+        @manager.errors_out_migration
+        def fake_function(self, context, instance, migration):
+            raise test.TestingException()
+
+        mock_obj_as_admin.return_value = mock.MagicMock()
+
+        self.assertRaises(test.TestingException, fake_function,
+                          self, self.context, self.instance, self.migration)
+        self.assertEqual('error', self.migration.status)
+        mock_save.assert_called_once_with()
+        mock_obj_as_admin.assert_called_once_with()
+
+    @mock.patch.object(objects.Migration, 'save')
+    @mock.patch.object(objects.Migration, 'obj_as_admin')
+    def test_contextmanager(self, mock_save, mock_obj_as_admin):
+        # Tests that errors_out_migration_ctxt context manager in compute
+        # manager sets migration status to 'error' when an exception is raised
+        # from decorated method
+
+        def test_function():
+            with manager.errors_out_migration_ctxt(self.migration):
+                raise test.TestingException()
+
+        mock_obj_as_admin.return_value = mock.MagicMock()
+
+        self.assertRaises(test.TestingException, test_function)
+        self.assertEqual('error', self.migration.status)
+        mock_save.assert_called_once_with()
+        mock_obj_as_admin.assert_called_once_with()
+
+    @ddt.data('completed', 'finished')
+    @mock.patch.object(objects.Migration, 'save')
+    def test_status_exclusion(self, status, mock_save):
+        # Tests that errors_out_migration doesn't error out migration if the
+        # status is anything other than 'migrating' or 'post-migrating'
+        self.migration.status = status
+
+        def test_function():
+            with manager.errors_out_migration_ctxt(self.migration):
+                raise test.TestingException()
+
+        self.assertRaises(test.TestingException, test_function)
+        self.assertEqual(status, self.migration.status)
+        mock_save.assert_not_called()
+
+
 class ComputeManagerMigrationTestCase(test.NoDBTestCase):
    def setUp(self):
        super(ComputeManagerMigrationTestCase, self).setUp()
@@ -5347,32 +5415,6 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase):
        self.useFixture(fixtures.SpawnIsSynchronousFixture())
        self.useFixture(fixtures.EventReporterStub())

-    @mock.patch.object(objects.Migration, 'save')
-    @mock.patch.object(objects.Migration, 'obj_as_admin')
-    def test_errors_out_migration_decorator(self, mock_save,
-                                            mock_obj_as_admin):
-        # Tests that errors_out_migration decorator in compute manager
-        # sets migration status to 'error' when an exception is raised
-        # from decorated method
-        instance = fake_instance.fake_instance_obj(self.context)
-
-        migration = objects.Migration()
-        migration.instance_uuid = instance.uuid
-        migration.status = 'migrating'
-        migration.id = 0
-
-        @manager.errors_out_migration
-        def fake_function(self, context, instance, migration):
-            raise test.TestingException()
-
-        mock_obj_as_admin.return_value = mock.MagicMock()
-
-        self.assertRaises(test.TestingException, fake_function,
-                          self, self.context, instance, migration)
-        self.assertEqual('error', migration.status)
-        mock_save.assert_called_once_with()
-        mock_obj_as_admin.assert_called_once_with()
-
    def test_finish_resize_failure(self):
        with test.nested(
            mock.patch.object(self.compute, '_finish_resize',