From 2d5e84390e1a9960abc6cd2969ee7021c62c4c33 Mon Sep 17 00:00:00 2001 From: Dan Smith Date: Wed, 12 Aug 2020 09:22:46 -0700 Subject: [PATCH] Fix import failure status reporting when all_stores_must_succeed=True As described in the referenced bug, if a store fails with all_stores_must_succeed=True, we never add that store to the failed list, nor remove it from the pending-import list, leaving a polling client to wait forever. This fixes that by making the revert handler of the import task do that if we are the one that failed. Closes-Bug: #1891352 Change-Id: I3571960bbfb4f8f0a716937b541f5b1594cd0e16 (cherry picked from commit 737dfca83c8d347ef22eab7afca4057ce5f03f20) --- glance/async_/flows/api_image_import.py | 5 +++ glance/tests/functional/v2/test_images.py | 11 ++----- .../async_/flows/test_api_image_import.py | 32 +++++++++++++++++++ 3 files changed, 39 insertions(+), 9 deletions(-) diff --git a/glance/async_/flows/api_image_import.py b/glance/async_/flows/api_image_import.py index c7f9926413..a846d0aed9 100644 --- a/glance/async_/flows/api_image_import.py +++ b/glance/async_/flows/api_image_import.py @@ -24,6 +24,7 @@ from oslo_utils import encodeutils from oslo_utils import timeutils from oslo_utils import units import six +import taskflow from taskflow.patterns import linear_flow as lf from taskflow import retry from taskflow import task @@ -513,6 +514,10 @@ class _ImportToStore(task.Task): """ with self.action_wrapper as action: action.remove_location_for_store(self.backend) + action.remove_importing_stores([self.backend]) + if isinstance(result, taskflow.types.failure.Failure): + # We are the store that failed, so add us to the failed list + action.add_failed_stores([self.backend]) class _VerifyImageState(task.Task): diff --git a/glance/tests/functional/v2/test_images.py b/glance/tests/functional/v2/test_images.py index aac41057e8..17bb2cac87 100644 --- a/glance/tests/functional/v2/test_images.py +++ b/glance/tests/functional/v2/test_images.py @@ -5606,15 +5606,8 @@ class TestImagesMultipleBackend(functional.MultipleBackendFunctionalTest): self.assertNotIn('file3', jsonutils.loads(response.text)['stores']) fail_key = 'os_glance_failed_import' pend_key = 'os_glance_importing_to_stores' - # NOTE(danms): This is bug #1891352, where a failed import when - # all_stores_must_succeed=True will leave the failed store in the - # importing list forever and never put it into the failed list. - # When the bug is fixed, this is what should happen: - # self.assertEqual('file3', jsonutils.loads(response.text)[fail_key]) - # self.assertEqual('', jsonutils.loads(response.text)[pend_key]) - # but until it is fixed, this asserts the *broken* behavior: - self.assertEqual('', jsonutils.loads(response.text)[fail_key]) - self.assertEqual('file3', jsonutils.loads(response.text)[pend_key]) + self.assertEqual('file3', jsonutils.loads(response.text)[fail_key]) + self.assertEqual('', jsonutils.loads(response.text)[pend_key]) # Copy newly created image to file2 and file3 stores and # all_stores_must_succeed set to false. 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 79280fecca..8049b6bc5e 100644 --- a/glance/tests/unit/async_/flows/test_api_image_import.py +++ b/glance/tests/unit/async_/flows/test_api_image_import.py @@ -13,10 +13,12 @@ # License for the specific language governing permissions and limitations # under the License. +import sys from unittest import mock from oslo_config import cfg from oslo_utils import units +import taskflow import glance.async_.flows.api_image_import as import_flow from glance.common import exception @@ -277,6 +279,36 @@ class TestImportToStoreTask(test_utils.BaseTestCase): self.assertEqual( image.extra_properties['os_glance_importing_to_stores'], "store2") + def test_revert_updates_status_keys(self): + img_repo = mock.MagicMock() + task_repo = mock.MagicMock() + wrapper = import_flow.ImportActionWrapper(img_repo, IMAGE_ID1) + image_import = import_flow._ImportToStore(TASK_ID1, TASK_TYPE, + task_repo, wrapper, + "http://url", + "store1", True, + True) + extra_properties = {"os_glance_importing_to_stores": "store1,store2"} + image = self.img_factory.new_image(image_id=UUID1, + extra_properties=extra_properties) + img_repo.get.return_value = image + + fail_key = 'os_glance_failed_import' + pend_key = 'os_glance_importing_to_stores' + + image_import.revert(None) + self.assertEqual('store2', image.extra_properties[pend_key]) + + try: + raise Exception('foo') + except Exception: + fake_exc_info = sys.exc_info() + + extra_properties = {"os_glance_importing_to_stores": "store1,store2"} + image_import.revert(taskflow.types.failure.Failure(fake_exc_info)) + self.assertEqual('store2', image.extra_properties[pend_key]) + self.assertEqual('store1', image.extra_properties[fail_key]) + @mock.patch("glance.async_.flows.api_image_import.image_import") def test_raises_when_all_stores_must_succeed(self, mock_import): img_repo = mock.MagicMock()