From 6a17e4e2bae0f3661ffed08b2d4360dfc8b3841d Mon Sep 17 00:00:00 2001 From: Rajat Dhasmana Date: Thu, 25 Jul 2024 21:16:27 +0530 Subject: [PATCH] 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 @@ class ImagesController(object): 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 @@ class ImagesController(object): 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 @@ class ImagesController(object): 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 @@ class TestCinderStoreUtils(base.MultiStoreClearingUnitTest): }] 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 @@ class TestCinderStoreUtils(base.MultiStoreClearingUnitTest): # 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 @@ class TestCinderStoreUtils(base.MultiStoreClearingUnitTest): 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.