From 3f7f01bb0584289c933a0fae3f915afec41093c8 Mon Sep 17 00:00:00 2001 From: Abhishek Kekane Date: Mon, 8 Jul 2024 09:49:55 +0000 Subject: [PATCH 1/2] Revert image state to queued if conversion fails Made changes to revert image state to `queued` and deleting image data from staging area if image conversion fails. If image is importing to multiple stores at a time then resetting the image properties `os_glance_importing_to_stores` and `os_glance_failed_imports` to reflect the actual result of the operation. Closes-Bug: 2072483 Change-Id: I373dde3a07332184c43d9605bad7a59c70241a71 (cherry picked from commit ea131dd1442861cb5884f99b6bb9e47e397605ce) (cherry picked from commit 633e85c9c07f85de36677137ea15bc3a7f4feb04) (cherry picked from commit 3b3141baa38a8dfbf37e7633cf9bd62898a545da) --- .../async_/flows/plugins/image_conversion.py | 37 ++++++++-- .../flows/plugins/test_image_conversion.py | 73 +++++++++++++++++-- 2 files changed, 94 insertions(+), 16 deletions(-) diff --git a/glance/async_/flows/plugins/image_conversion.py b/glance/async_/flows/plugins/image_conversion.py index 6f5199c82d..fedbec117a 100644 --- a/glance/async_/flows/plugins/image_conversion.py +++ b/glance/async_/flows/plugins/image_conversion.py @@ -63,13 +63,16 @@ class _ConvertImage(task.Task): default_provides = 'file_path' - def __init__(self, context, task_id, task_type, action_wrapper): + def __init__(self, context, task_id, task_type, action_wrapper, + stores): self.context = context self.task_id = task_id self.task_type = task_type self.action_wrapper = action_wrapper + self.stores = stores self.image_id = action_wrapper.image_id self.dest_path = "" + self.src_path = "" self.python = CONF.wsgi.python_interpreter super(_ConvertImage, self).__init__( name='%s-Convert_Image-%s' % (task_type, task_id)) @@ -83,8 +86,8 @@ def _execute(self, action, file_path, **kwargs): target_format = CONF.image_conversion.output_format # TODO(jokke): Once we support other schemas we need to take them into # account and handle the paths here. - src_path = file_path.split('file://')[-1] - dest_path = "%(path)s.%(target)s" % {'path': src_path, + self.src_path = file_path.split('file://')[-1] + dest_path = "%(path)s.%(target)s" % {'path': self.src_path, 'target': target_format} self.dest_path = dest_path @@ -104,7 +107,7 @@ def _execute(self, action, file_path, **kwargs): # qemu-img on it. # See https://bugs.launchpad.net/nova/+bug/2059809 for details. try: - inspector = inspector_cls.from_file(src_path) + inspector = inspector_cls.from_file(self.src_path) if not inspector.safety_check(): LOG.error('Image failed %s safety check; aborting conversion', source_format) @@ -123,7 +126,7 @@ def _execute(self, action, file_path, **kwargs): stdout, stderr = putils.trycmd("qemu-img", "info", "-f", source_format, "--output=json", - src_path, + self.src_path, prlimit=utils.QEMU_IMG_PROC_LIMITS, python_exec=self.python, log_errors=putils.LOG_ALL_ERRORS,) @@ -186,7 +189,7 @@ def _execute(self, action, file_path, **kwargs): stdout, stderr = putils.trycmd('qemu-img', 'convert', '-f', source_format, '-O', target_format, - src_path, dest_path, + self.src_path, dest_path, log_errors=putils.LOG_ALL_ERRORS) except OSError as exc: with excutils.save_and_reraise_exception(): @@ -204,7 +207,7 @@ def _execute(self, action, file_path, **kwargs): LOG.info(_LI('Updated image %s size=%i disk_format=%s'), self.image_id, new_size, target_format) - os.remove(src_path) + os.remove(self.src_path) return "file://%s" % dest_path @@ -217,6 +220,23 @@ def revert(self, result=None, **kwargs): if os.path.exists(self.dest_path): os.remove(self.dest_path) + # NOTE(abhishekk): If we failed to convert the image, then none + # of the _ImportToStore() tasks could have run, so we need + # to move all stores out of "importing" to "failed". + with self.action_wrapper as action: + action.set_image_attribute(status='queued') + if self.stores: + action.remove_importing_stores(self.stores) + action.add_failed_stores(self.stores) + + if self.src_path: + try: + os.remove(self.src_path) + except FileNotFoundError: + # NOTE(abhishekk): We must have raced with something + # else, so this is not a problem + pass + def get_flow(**kwargs): """Return task flow for no-op. @@ -232,7 +252,8 @@ def get_flow(**kwargs): task_id = kwargs.get('task_id') task_type = kwargs.get('task_type') action_wrapper = kwargs.get('action_wrapper') + stores = kwargs.get('backend', []) return lf.Flow(task_type).add( - _ConvertImage(context, task_id, task_type, action_wrapper) + _ConvertImage(context, task_id, task_type, action_wrapper, stores) ) diff --git a/glance/tests/unit/async_/flows/plugins/test_image_conversion.py b/glance/tests/unit/async_/flows/plugins/test_image_conversion.py index 1942dcc43f..28a60a4334 100644 --- a/glance/tests/unit/async_/flows/plugins/test_image_conversion.py +++ b/glance/tests/unit/async_/flows/plugins/test_image_conversion.py @@ -59,6 +59,7 @@ def setUp(self): self.context = mock.MagicMock() self.img_repo = mock.MagicMock() self.task_repo = mock.MagicMock() + self.stores = mock.MagicMock() self.image_id = UUID1 self.gateway = gateway.Gateway() @@ -87,7 +88,10 @@ def setUp(self): task_input=task_input) self.image.extra_properties = { - 'os_glance_import_task': self.task.task_id} + 'os_glance_import_task': self.task.task_id, + 'os_glance_importing_to_stores': mock.MagicMock(), + 'os_glance_failed_import': "" + } self.wrapper = import_flow.ImportActionWrapper(self.img_repo, self.image_id, self.task.task_id) @@ -105,7 +109,8 @@ def test_image_convert_success(self, mock_os_remove, mock_os_stat): image_convert = image_conversion._ConvertImage(self.context, self.task.task_id, self.task_type, - self.wrapper) + self.wrapper, + self.stores) self.task_repo.get.return_value = self.task image = mock.MagicMock(image_id=self.image_id, virtual_size=None, @@ -137,7 +142,8 @@ def _setup_image_convert_info_fail(self, disk_format='qcow2'): image_convert = image_conversion._ConvertImage(self.context, self.task.task_id, self.task_type, - self.wrapper) + self.wrapper, + self.stores) self.task_repo.get.return_value = self.task image = mock.MagicMock(image_id=self.image_id, virtual_size=None, @@ -354,15 +360,41 @@ def test_image_convert_same_format_does_nothing(self): image = self.img_repo.get.return_value self.assertEqual(123, image.virtual_size) - @mock.patch.object(os, 'remove') - def test_image_convert_revert_success(self, mock_os_remove): + def _set_image_conversion(self, mock_os_remove, stores=[]): mock_os_remove.return_value = None + wrapper = mock.MagicMock() image_convert = image_conversion._ConvertImage(self.context, self.task.task_id, self.task_type, - self.wrapper) - + wrapper, + stores) + action = wrapper.__enter__.return_value self.task_repo.get.return_value = self.task + return action, image_convert + + @mock.patch.object(os, 'remove') + def test_image_convert_revert_success_multiple_stores( + self, mock_os_remove): + action, image_convert = self._set_image_conversion( + mock_os_remove, stores=self.stores) + + with mock.patch.object(processutils, 'execute') as exc_mock: + exc_mock.return_value = ("", None) + with mock.patch.object(os.path, 'exists') as os_exists_mock: + os_exists_mock.return_value = True + image_convert.revert(result=mock.MagicMock()) + self.assertEqual(1, mock_os_remove.call_count) + action.set_image_attribute.assert_called_once_with( + status='queued') + action.remove_importing_stores.assert_called_once_with( + self.stores) + action.add_failed_stores.assert_called_once_with( + self.stores) + + @mock.patch.object(os, 'remove') + def test_image_convert_revert_success_single_store( + self, mock_os_remove): + action, image_convert = self._set_image_conversion(mock_os_remove) with mock.patch.object(processutils, 'execute') as exc_mock: exc_mock.return_value = ("", None) @@ -370,6 +402,30 @@ def test_image_convert_revert_success(self, mock_os_remove): os_exists_mock.return_value = True image_convert.revert(result=mock.MagicMock()) self.assertEqual(1, mock_os_remove.call_count) + self.assertEqual(0, action.remove_importing_stores.call_count) + self.assertEqual(0, action.add_failed_store.call_count) + action.set_image_attribute.assert_called_once_with( + status='queued') + + @mock.patch.object(os, 'remove') + def test_image_convert_revert_success_src_file_exists( + self, mock_os_remove): + action, image_convert = self._set_image_conversion( + mock_os_remove, stores=self.stores) + image_convert.src_path = mock.MagicMock() + + with mock.patch.object(processutils, 'execute') as exc_mock: + exc_mock.return_value = ("", None) + with mock.patch.object(os.path, 'exists') as os_exists_mock: + os_exists_mock.return_value = True + image_convert.revert(result=mock.MagicMock()) + action.set_image_attribute.assert_called_once_with( + status='queued') + action.remove_importing_stores.assert_called_once_with( + self.stores) + action.add_failed_stores.assert_called_once_with( + self.stores) + self.assertEqual(2, mock_os_remove.call_count) def test_image_convert_interpreter_configured(self): # By default, wsgi.python_interpreter is None; if it is @@ -380,5 +436,6 @@ def test_image_convert_interpreter_configured(self): convert = image_conversion._ConvertImage(self.context, self.task.task_id, self.task_type, - self.wrapper) + self.wrapper, + self.stores) self.assertEqual(fake_interpreter, convert.python) From 6a17e4e2bae0f3661ffed08b2d4360dfc8b3841d Mon Sep 17 00:00:00 2001 From: Rajat Dhasmana Date: Thu, 25 Jul 2024 21:16:27 +0530 Subject: [PATCH 2/2] Fix LP#2054575 This patch contains a squash of two commits: COMMIT 1: Make location URL compatible with cinder backend While adding location to an image, cinder sends location url as `cinder://volume_id` for single as well as multistore which is incompatible with glance multistore and store throws InvalidLoctation error. Modifying the location url to be compatible with multistore as `cinder://store_id/volume_id` to avoid Invalid Location error. Related-Bug: #2054575 Change-Id: I5f791c58ae857f6c553276dd9808799c3db3aa4f COMMIT 2: Fix: optimized upload volume in Cinder store When Glance is configured to use Cinder store and we upload volume to Glance in the optimized path, it fails with InvalidLocation error. This happens because Cinder is not aware about the store in which we will create the image and supplies the old format URL i.e. cinder:// whereas Glance expects new location format i.e. cinder:///. Glance has code to update the format from old location format to new location format but it isn't triggered in case of old location APIs. This patch adds the context to the update store ID request which calls the Cinder store to provide the updated location, hence fixing the optimized path for upload volume to image. Closes-Bug: #2054575 Change-Id: Idd1cb8982b40b85a17821596f76dfa10207f6381 The commits are squashed together to make backport easier. Change-Id: I9ecdfe08b63c00446dc3e24195e3b8e59b82f55c (cherry picked from commit 53175262f0f8df303176f4e9cd44e36d442c6cea) (cherry picked from commit 8540ffc6ee34976285209e9f3c7c728bf649e0ff) --- glance/api/v2/images.py | 6 ++-- glance/common/store_utils.py | 6 +++- glance/tests/unit/common/test_utils.py | 34 +++++++++++++++---- ...tible-with-locations-1f3e938ff7e11c7d.yaml | 9 +++++ 4 files changed, 45 insertions(+), 10 deletions(-) create mode 100644 releasenotes/notes/make-cinder-url-compatible-with-locations-1f3e938ff7e11c7d.yaml diff --git a/glance/api/v2/images.py b/glance/api/v2/images.py index 95cfe7b9c3..bf74f281b5 100644 --- a/glance/api/v2/images.py +++ b/glance/api/v2/images.py @@ -674,7 +674,7 @@ def _do_add(self, req, image, api_pol, change): json_schema_version = change.get('json_schema_version', 10) if path_root == 'locations': api_pol.update_locations() - self._do_add_locations(image, path[1], value) + self._do_add_locations(image, path[1], value, req.context) else: api_pol.update_property(path_root, value) if ((hasattr(image, path_root) or @@ -1043,7 +1043,7 @@ def _do_replace_locations(self, image, value): raise webob.exc.HTTPBadRequest( explanation=encodeutils.exception_to_unicode(ve)) - def _do_add_locations(self, image, path_pos, value): + def _do_add_locations(self, image, path_pos, value, context): if CONF.show_multiple_locations == False: msg = _("It's not allowed to add locations if locations are " "invisible.") @@ -1059,7 +1059,7 @@ def _do_add_locations(self, image, path_pos, value): updated_location = value if CONF.enabled_backends: updated_location = store_utils.get_updated_store_location( - [value])[0] + [value], context=context)[0] pos = self._get_locations_op_pos(path_pos, len(image.locations), True) diff --git a/glance/common/store_utils.py b/glance/common/store_utils.py index 552f661f58..e3a94d2784 100644 --- a/glance/common/store_utils.py +++ b/glance/common/store_utils.py @@ -238,8 +238,12 @@ def _update_cinder_location_and_store_id(context, loc): "due to unknown issues."), uri) -def get_updated_store_location(locations): +def get_updated_store_location(locations, context=None): for loc in locations: + if loc['url'].startswith("cinder://") and context: + _update_cinder_location_and_store_id(context, loc) + continue + store_id = _get_store_id_from_uri(loc['url']) if store_id: loc['metadata']['store'] = store_id diff --git a/glance/tests/unit/common/test_utils.py b/glance/tests/unit/common/test_utils.py index a07b2709d7..8069ef5742 100644 --- a/glance/tests/unit/common/test_utils.py +++ b/glance/tests/unit/common/test_utils.py @@ -102,7 +102,8 @@ class TestCinderStoreUtils(base.MultiStoreClearingUnitTest): new_callable=mock.PropertyMock) def _test_update_cinder_store_in_location(self, mock_url_prefix, mock_associate_store, - is_valid=True): + is_valid=True, + modify_source_url=False): volume_id = 'db457a25-8f16-4b2c-a644-eae8d17fe224' store_id = 'fast-cinder' expected = 'fast-cinder' @@ -117,7 +118,11 @@ def _test_update_cinder_store_in_location(self, mock_url_prefix, }] mock_url_prefix.return_value = 'cinder://%s' % store_id image.locations = locations - store_utils.update_store_in_locations(context, image, image_repo) + if modify_source_url: + updated_location = store_utils.get_updated_store_location( + locations, context=context) + else: + store_utils.update_store_in_locations(context, image, image_repo) if is_valid: # This is the case where we found an image that has an @@ -129,10 +134,18 @@ def _test_update_cinder_store_in_location(self, mock_url_prefix, # format i.e. this is the case when store is valid and location # url, metadata are updated and image_repo.save is called expected_url = mock_url_prefix.return_value + '/' + volume_id - self.assertEqual(expected_url, image.locations[0].get('url')) - self.assertEqual(expected, image.locations[0]['metadata'].get( - 'store')) - self.assertEqual(1, image_repo.save.call_count) + if modify_source_url: + # This is the case where location url is modified to be + # compatible with multistore as below, + # `cinder://store_id/volume_id` to avoid InvalidLocation error + self.assertEqual(expected_url, updated_location[0].get('url')) + self.assertEqual(expected, + updated_location[0]['metadata'].get('store')) + else: + self.assertEqual(expected_url, image.locations[0].get('url')) + self.assertEqual(expected, image.locations[0]['metadata'].get( + 'store')) + self.assertEqual(1, image_repo.save.call_count) else: # Here, we've got an image backed by a volume which does # not have a corresponding store specifying the volume_type. @@ -151,6 +164,15 @@ def test_update_cinder_store_location_valid_type(self): def test_update_cinder_store_location_invalid_type(self): self._test_update_cinder_store_in_location(is_valid=False) + def test_get_updated_cinder_store_location(self): + """ + Test if location url is modified to be compatible + with multistore. + """ + + self._test_update_cinder_store_in_location( + modify_source_url=True) + class TestUtils(test_utils.BaseTestCase): """Test routines in glance.utils""" diff --git a/releasenotes/notes/make-cinder-url-compatible-with-locations-1f3e938ff7e11c7d.yaml b/releasenotes/notes/make-cinder-url-compatible-with-locations-1f3e938ff7e11c7d.yaml new file mode 100644 index 0000000000..8c9df095cd --- /dev/null +++ b/releasenotes/notes/make-cinder-url-compatible-with-locations-1f3e938ff7e11c7d.yaml @@ -0,0 +1,9 @@ +--- +fixes: + - | + `Bug #2054575 `_: + Fixed the issue when cinder uploads a volume to glance in the + optimized path and glance rejects the request with invalid location. + Now we convert the old location format sent by cinder into the new + location format supported by multi store, hence allowing volumes to + be uploaded in an optimized way.