From e8b13ce1b44313a9604aa9cd8cdbf8bd4d1bda52 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. Change-Id: Ie7558eeae488406a1696f0ee1bf40b644ae6d8cc Partial-Bug: #1889640 --- glance/async_/flows/api_image_import.py | 39 +++++++++++-------- .../async_/flows/test_api_image_import.py | 36 +++++++++++++++++ glance/tests/unit/async_/test_async.py | 2 +- 3 files changed, 60 insertions(+), 17 deletions(-) diff --git a/glance/async_/flows/api_image_import.py b/glance/async_/flows/api_image_import.py index e8a9140aeb..11f344322c 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 ImportActionWrapper(object): """Wrapper for all the image metadata operations we do during an import. @@ -466,29 +472,30 @@ class _ImportToStore(task.Task): action.remove_location_for_store(self.backend) -class _SaveImage(task.Task): +class _VerifyImageState(task.Task): def __init__(self, task_id, task_type, action_wrapper, import_method): self.task_id = task_id self.task_type = task_type self.action_wrapper = action_wrapper 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 """ with self.action_wrapper as action: - if (self.import_method != 'copy-image' - and action.image_status == 'importing'): - # NOTE(flaper87): THIS IS WRONG! - # we should be doing atomic updates to avoid - # race conditions. This happens in other places - # too. - action.set_image_status('active') + if action.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.""" + with self.action_wrapper as action: + if self.import_method != 'copy-image': + action.set_image_status('queued') class _CompleteTask(task.Task): @@ -613,11 +620,11 @@ 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, - action_wrapper, - import_method) - flow.add(save_task) + verify_task = _VerifyImageState(task_id, + task_type, + action_wrapper, + 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 13c0a1de22..452f4f8b39 100644 --- a/glance/tests/unit/async_/flows/test_api_image_import.py +++ b/glance/tests/unit/async_/flows/test_api_image_import.py @@ -213,6 +213,42 @@ class TestImportCopyImageTask(test_utils.BaseTestCase): img_repo.save.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 + wrapper = import_flow.ImportActionWrapper(mock_repo, IMAGE_ID1) + + task = import_flow._VerifyImageState(TASK_ID1, TASK_TYPE, + wrapper, 'anything!') + + task.execute() + + fake_img.status = 'importing' + self.assertRaises(import_flow._NoStoresSucceeded, + task.execute) + + def test_revert_copy_status_unchanged(self): + wrapper = mock.MagicMock() + task = import_flow._VerifyImageState(TASK_ID1, TASK_TYPE, + wrapper, 'copy-image') + task.revert(mock.sentinel.result) + + # If we are doing copy-image, no state update should be made + wrapper.__enter__.return_value.set_image_status.assert_not_called() + + def test_reverts_state_nocopy(self): + wrapper = mock.MagicMock() + task = import_flow._VerifyImageState(TASK_ID1, TASK_TYPE, + wrapper, 'glance-direct') + task.revert(mock.sentinel.result) + + # Except for copy-image, image state should revert to queued + action = wrapper.__enter__.return_value + action.set_image_status.assert_called_once_with('queued') + + class TestImportActionWrapper(test_utils.BaseTestCase): def test_wrapper_success(self): mock_repo = mock.MagicMock() diff --git a/glance/tests/unit/async_/test_async.py b/glance/tests/unit/async_/test_async.py index 5491da7a64..2ec190e8eb 100644 --- a/glance/tests/unit/async_/test_async.py +++ b/glance/tests/unit/async_/test_async.py @@ -80,7 +80,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',