From d63d72799766801414500934e06db8c88807fe59 Mon Sep 17 00:00:00 2001 From: Erno Kuvaja Date: Thu, 30 Jul 2020 17:00:23 +0100 Subject: [PATCH] Fix active image when all uploads fail This fix is for the case where all stores fails and the end result is active image with no image data. Conflicts: Action wrapper does not exists, utilized image_repo instead. Change-Id: Ie7558eeae488406a1696f0ee1bf40b644ae6d8cc Partial-Bug: #1889640 (cherry picked from commit e8b13ce1b44313a9604aa9cd8cdbf8bd4d1bda52) --- glance/async_/flows/api_image_import.py | 43 +++++++++++-------- .../async_/flows/test_api_image_import.py | 41 ++++++++++++++++++ glance/tests/unit/async_/test_async.py | 2 +- 3 files changed, 67 insertions(+), 19 deletions(-) diff --git a/glance/async_/flows/api_image_import.py b/glance/async_/flows/api_image_import.py index 1458522b06..42614438c2 100644 --- a/glance/async_/flows/api_image_import.py +++ b/glance/async_/flows/api_image_import.py @@ -76,6 +76,12 @@ CONF.register_opts(api_import_opts, group='image_import_opts') # need to duplicate what we have already for example in base_import.py. +class _NoStoresSucceeded(exception.GlanceException): + + def __init__(self, message): + super(_NoStoresSucceeded, self).__init__(message) + + class _DeleteFromFS(task.Task): def __init__(self, task_id, task_type): @@ -324,7 +330,7 @@ class _ImportToStore(task.Task): break -class _SaveImage(task.Task): +class _VerifyImageState(task.Task): def __init__(self, task_id, task_type, image_repo, image_id, import_method): @@ -333,23 +339,24 @@ class _SaveImage(task.Task): self.image_repo = image_repo self.image_id = image_id self.import_method = import_method - super(_SaveImage, self).__init__( - name='%s-SaveImage-%s' % (task_type, task_id)) + super(_VerifyImageState, self).__init__( + name='%s-VerifyImageState-%s' % (task_type, task_id)) def execute(self): - """Transition image status to active + """Verify we have active image :param image_id: Glance Image ID """ new_image = self.image_repo.get(self.image_id) - if (self.import_method != 'copy-image' - and new_image.status == 'importing'): - # NOTE(flaper87): THIS IS WRONG! - # we should be doing atomic updates to avoid - # race conditions. This happens in other places - # too. - new_image.status = 'active' - self.image_repo.save(new_image) + if new_image.status != 'active': + raise _NoStoresSucceeded(_('None of the uploads finished!')) + + def revert(self, result, **kwargs): + """Set back to queued if this wasn't copy-image job.""" + if self.import_method != 'copy-image': + new_image = self.image_repo.get(self.image_id) + new_image.status = 'queued' + self.image_repo.save_image(new_image) class _CompleteTask(task.Task): @@ -470,12 +477,12 @@ def get_flow(**kwargs): delete_task = lf.Flow(task_type).add(_DeleteFromFS(task_id, task_type)) flow.add(delete_task) - save_task = _SaveImage(task_id, - task_type, - image_repo, - image_id, - import_method) - flow.add(save_task) + verify_task = _VerifyImageState(task_id, + task_type, + image_repo, + image_id, + import_method) + flow.add(verify_task) complete_task = _CompleteTask(task_id, task_type, diff --git a/glance/tests/unit/async_/flows/test_api_image_import.py b/glance/tests/unit/async_/flows/test_api_image_import.py index 7dbf1c8c16..b449768cdb 100644 --- a/glance/tests/unit/async_/flows/test_api_image_import.py +++ b/glance/tests/unit/async_/flows/test_api_image_import.py @@ -219,3 +219,44 @@ class TestDeleteFromFS(test_utils.BaseTestCase): task = import_flow._DeleteFromFS(TASK_ID1, TASK_TYPE) task.execute('foo') mock_unlink.assert_not_called() + + +class TestVerifyImageStateTask(test_utils.BaseTestCase): + def test_verify_active_status(self): + fake_img = mock.MagicMock(status='active') + mock_repo = mock.MagicMock() + mock_repo.get.return_value = fake_img + + task = import_flow._VerifyImageState(TASK_ID1, TASK_TYPE, + mock_repo, IMAGE_ID1, + 'anything!') + + task.execute() + + fake_img.status = 'importing' + self.assertRaises(import_flow._NoStoresSucceeded, + task.execute) + + def test_revert_copy_status_unchanged(self): + fake_img = mock.MagicMock(status='active') + mock_repo = mock.MagicMock() + mock_repo.get.return_value = fake_img + task = import_flow._VerifyImageState(TASK_ID1, TASK_TYPE, + mock_repo, IMAGE_ID1, + 'copy-image') + task.revert(mock.sentinel.result) + + # If we are doing copy-image, no state update should be made + mock_repo.save_image.assert_not_called() + + def test_reverts_state_nocopy(self): + fake_img = mock.MagicMock(status='importing') + mock_repo = mock.MagicMock() + mock_repo.get.return_value = fake_img + task = import_flow._VerifyImageState(TASK_ID1, TASK_TYPE, + mock_repo, IMAGE_ID1, + 'glance-direct') + task.revert(mock.sentinel.result) + + # Except for copy-image, image state should revert to queued + mock_repo.save_image.assert_called_once() diff --git a/glance/tests/unit/async_/test_async.py b/glance/tests/unit/async_/test_async.py index b6039d2b2a..cf1a7254c2 100644 --- a/glance/tests/unit/async_/test_async.py +++ b/glance/tests/unit/async_/test_async.py @@ -70,7 +70,7 @@ class TestImportTaskFlow(test_utils.BaseTestCase): self.config(node_staging_uri='file:///tmp/staging') store.create_stores(CONF) self.base_flow = ['ConfigureStaging', 'ImportToStore', - 'DeleteFromFS', 'SaveImage', + 'DeleteFromFS', 'VerifyImageState', 'CompleteTask'] self.import_plugins = ['Convert_Image', 'Decompress_Image',