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
This commit is contained in:
@@ -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():
|
||||
|
@@ -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):
|
||||
|
Reference in New Issue
Block a user