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://<vol-id> whereas Glance expects
new location format i.e. cinder://<store-id>/<vol-id>.

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 5317526)
(cherry picked from commit 8540ffc)

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)

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

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"""

releasenotes/notes/make-cinder-url-compatible-with-locations-1f3e938ff7e11c7d.yaml

@@ -0,0 +1,9 @@
+---
+fixes:
+  - |
+    `Bug #2054575 <https://bugs.launchpad.net/glance/+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.