From 1b3fad3a23ad99c581104f4e99a7eb2a69b45fd3 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Fri, 30 Jun 2017 18:02:34 +0100 Subject: [PATCH] Ensure errors_out_migration errors out migration errors_out_migration_ctxt includes a check of the current migration state. If the migration is not in either 'migrating' or 'post-migrating', the decorator will not set it to an error state. This behaviour has been present since the decorator was added in change Ie752e483. The change does not describe why this restriction is present and there is no comment, so we can only guess why it exists. My best guess is that it is a hack to limit the scope of the decorator. The idea might have been that if the migration status has been set to some value representing that is has finished or moved to another host, we should no longer set the migration to an error state if there is a subsequent error which wouldn't directly affect the instance. With changes Id2f352a6 and I9bcc1605 we have removed the necessity for this by defining the scope in which we should set a migration to error to the lexical scope of the context manager. We remove this exclusion as its implementation is obtuse, we have replaced its functionality, and it makes extending migration code harder. Change-Id: Ib6156d8e2500cee441c6c04fbe91b5c8eff488b0 --- nova/compute/manager.py | 10 +--------- nova/tests/unit/compute/test_compute_mgr.py | 17 ----------------- 2 files changed, 1 insertion(+), 26 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 4172b1c53338..0fb3852ede54 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -114,16 +114,8 @@ def errors_out_migration_ctxt(migration): try: yield - except Exception as ex: + except Exception: 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(): diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index 38aeeb5c48f1..467e89009610 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -19,7 +19,6 @@ 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 @@ -5385,7 +5384,6 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase): self.assertEqual(expected_call, create_error_call) -@ddt.ddt class ComputeManagerErrorsOutMigrationTestCase(test.NoDBTestCase): def setUp(self): super(ComputeManagerErrorsOutMigrationTestCase, self).setUp() @@ -5436,21 +5434,6 @@ class ComputeManagerErrorsOutMigrationTestCase(test.NoDBTestCase): 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): class TestResizeError(Exception):