From 8f0964a893b98faa6f8fc751a67601c6b58366ed Mon Sep 17 00:00:00 2001
From: Peter Penchev <openstack-dev@storpool.com>
Date: Thu, 9 Jun 2022 01:43:28 +0300
Subject: [PATCH 1/8] Send the correct location URI to the Glance v2 API

When uploading a volume to an image, send the new-style
cinder://<store-id>/<volume-id> URL to the Glance API if
image_service:store_id is present in the volume type extra specs.

Closes-Bug: #1978020
Co-Authored-By: Rajat Dhasmana <rajatdhasmana@gmail.com>
Change-Id: I815706f691a7d1e5a0c54eb15222417008ef1f34
---
 cinder/tests/unit/volume/test_image.py        |   2 +-
 .../tests/unit/volume/test_volume_manager.py  | 160 ++++++++++++++++++
 cinder/volume/manager.py                      |   4 +-
 ...20-glance-upload-uri-8fbc70c442ac620c.yaml |  11 ++
 4 files changed, 175 insertions(+), 2 deletions(-)
 create mode 100644 releasenotes/notes/bug-1978020-glance-upload-uri-8fbc70c442ac620c.yaml

diff --git a/cinder/tests/unit/volume/test_image.py b/cinder/tests/unit/volume/test_image.py
index ebc7904c4..2b1b9c392 100644
--- a/cinder/tests/unit/volume/test_image.py
+++ b/cinder/tests/unit/volume/test_image.py
@@ -366,7 +366,7 @@ class CopyVolumeToImageTestCase(base.BaseVolumeTestCase):
     def test_copy_volume_to_image_with_image_volume(self):
         image = self._test_copy_volume_to_image_with_image_volume()
         self.assertTrue(image['locations'][0]['url'].startswith('cinder://'))
-        image_volume_id = image['locations'][0]['url'][9:]
+        image_volume_id = image['locations'][0]['url'].split('/')[-1]
         # The image volume does NOT include the snapshot_id, and include the
         # source_volid which is the uploaded-volume id.
         vol_ref = db.volume_get(self.context, image_volume_id)
diff --git a/cinder/tests/unit/volume/test_volume_manager.py b/cinder/tests/unit/volume/test_volume_manager.py
index 3b751d2cd..547a48fdd 100644
--- a/cinder/tests/unit/volume/test_volume_manager.py
+++ b/cinder/tests/unit/volume/test_volume_manager.py
@@ -287,3 +287,163 @@ class VolumeManagerTestCase(base.BaseVolumeTestCase):
         manager._parse_connection_options(ctxt, vol, conn_info)
         self.assertIn('cacheable', conn_info['data'])
         self.assertIs(conn_info['data']['cacheable'], False)
+
+    @mock.patch('cinder.message.api.API.create')
+    @mock.patch('cinder.volume.volume_utils.require_driver_initialized')
+    @mock.patch('cinder.volume.manager.VolumeManager._clone_image_volume')
+    @mock.patch('cinder.db.volume_metadata_update')
+    def test_clone_image_no_volume(self,
+                                   fake_update,
+                                   fake_clone,
+                                   fake_msg_create,
+                                   fake_init):
+        """Make sure nothing happens if no volume was created."""
+        manager = vol_manager.VolumeManager()
+
+        ctx = mock.sentinel.context
+        volume = fake_volume.fake_volume_obj(ctx)
+        image_service = mock.MagicMock(spec=[])
+
+        fake_clone.return_value = None
+
+        image_meta = {'disk_format': 'raw', 'container_format': 'ova'}
+        manager._clone_image_volume_and_add_location(ctx, volume,
+                                                     image_service, image_meta)
+        fake_clone.assert_not_called()
+        fake_update.assert_not_called()
+
+        image_meta = {'disk_format': 'qcow2', 'container_format': 'bare'}
+        manager._clone_image_volume_and_add_location(ctx, volume,
+                                                     image_service, image_meta)
+        fake_clone.assert_not_called()
+        fake_update.assert_not_called()
+
+        image_meta = {'disk_format': 'raw', 'container_format': 'bare'}
+        manager._clone_image_volume_and_add_location(ctx, volume,
+                                                     image_service, image_meta)
+        fake_clone.assert_called_once_with(ctx, volume, image_meta)
+        fake_update.assert_not_called()
+
+    @mock.patch('cinder.message.api.API.create')
+    @mock.patch('cinder.objects.VolumeType.get_by_id')
+    @mock.patch('cinder.volume.volume_utils.require_driver_initialized')
+    @mock.patch('cinder.volume.manager.VolumeManager._clone_image_volume')
+    @mock.patch('cinder.db.volume_metadata_update')
+    def test_clone_image_no_store_id(self,
+                                     fake_update,
+                                     fake_clone,
+                                     fake_msg_create,
+                                     fake_volume_type_get,
+                                     fake_init):
+        """Send a cinder://<volume-id> URL if no store ID in extra specs."""
+        manager = vol_manager.VolumeManager()
+
+        project_id = fake.PROJECT_ID
+
+        ctx = mock.MagicMock()
+        ctx.elevated.return_value = ctx
+        ctx.project_id = project_id
+
+        vol_type = fake_volume.fake_volume_type_obj(
+            ctx,
+            id=fake.VOLUME_TYPE_ID,
+            name=fake.VOLUME_TYPE_NAME,
+            extra_specs={'volume_type_backend': 'unknown'})
+        fake_volume_type_get.return_value = vol_type
+
+        volume = fake_volume.fake_volume_obj(ctx,
+                                             id=fake.VOLUME_ID,
+                                             volume_type_id=vol_type.id)
+
+        image_volume_id = fake.VOLUME2_ID
+        image_volume = fake_volume.fake_volume_obj(ctx, id=image_volume_id)
+        url = 'cinder://%(vol)s' % {'vol': image_volume_id}
+
+        image_service = mock.MagicMock(spec=['add_location'])
+        image_meta_id = fake.IMAGE_ID
+        image_meta = {
+            'id': image_meta_id,
+            'disk_format': 'raw',
+            'container_format': 'bare',
+        }
+        image_volume_meta = {
+            'image_owner': project_id,
+            'glance_image_id': image_meta_id,
+        }
+
+        fake_clone.return_value = image_volume
+
+        manager._clone_image_volume_and_add_location(ctx, volume,
+                                                     image_service, image_meta)
+        fake_clone.assert_called_once_with(ctx, volume, image_meta)
+        fake_update.assert_called_with(ctx, image_volume_id,
+                                       image_volume_meta, False)
+        image_service.add_location.assert_called_once_with(ctx, image_meta_id,
+                                                           url, {})
+
+    @mock.patch('cinder.message.api.API.create')
+    @mock.patch('cinder.objects.VolumeType.get_by_id')
+    @mock.patch('cinder.volume.volume_utils.require_driver_initialized')
+    @mock.patch('cinder.volume.manager.VolumeManager._clone_image_volume')
+    @mock.patch('cinder.db.volume_metadata_update')
+    def test_clone_image_with_store_id(self,
+                                       fake_update,
+                                       fake_clone,
+                                       fake_msg_create,
+                                       fake_volume_type_get,
+                                       fake_init):
+        """Send a cinder://<store-id>/<volume-id> URL."""
+        manager = vol_manager.VolumeManager()
+
+        project_id = fake.PROJECT_ID
+
+        ctx = mock.MagicMock()
+        ctx.elevated.return_value = ctx
+        ctx.project_id = project_id
+
+        store_id = 'muninn'
+        vol_type = fake_volume.fake_volume_type_obj(
+            ctx,
+            id=fake.VOLUME_TYPE_ID,
+            name=fake.VOLUME_TYPE_NAME,
+            extra_specs={
+                'volume_type_backend': 'unknown',
+                'image_service:store_id': store_id,
+            })
+        fake_volume_type_get.return_value = vol_type
+
+        volume = fake_volume.fake_volume_obj(ctx,
+                                             id=fake.VOLUME_ID,
+                                             volume_type_id=vol_type.id)
+
+        image_volume_id = '42'
+        image_volume = mock.MagicMock(spec=['id'])
+        image_volume.id = image_volume_id
+        url = 'cinder://%(store)s/%(vol)s' % {
+            'store': store_id,
+            'vol': image_volume_id,
+        }
+
+        image_service = mock.MagicMock(spec=['add_location'])
+        image_meta_id = fake.IMAGE_ID
+        image_meta = {
+            'id': image_meta_id,
+            'disk_format': 'raw',
+            'container_format': 'bare',
+        }
+        image_volume_meta = {
+            'image_owner': project_id,
+            'glance_image_id': image_meta_id,
+        }
+
+        fake_clone.return_value = image_volume
+
+        manager._clone_image_volume_and_add_location(ctx, volume,
+                                                     image_service, image_meta)
+        fake_clone.assert_called_once_with(ctx, volume, image_meta)
+        fake_update.assert_called_with(ctx, image_volume_id,
+                                       image_volume_meta, False)
+        image_service.add_location.assert_called_once_with(ctx,
+                                                           image_meta_id,
+                                                           url,
+                                                           {'store': store_id})
diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py
index dbb73d894..804a1c6cb 100644
--- a/cinder/volume/manager.py
+++ b/cinder/volume/manager.py
@@ -1683,7 +1683,6 @@ class VolumeManager(manager.CleanableManager,
                                        image_volume_meta,
                                        False)
 
-        uri = 'cinder://%s' % image_volume.id
         image_registered = None
 
         # retrieve store information from extra-specs
@@ -1692,6 +1691,9 @@ class VolumeManager(manager.CleanableManager,
 
         if store_id:
             location_metadata['store'] = store_id
+            uri = 'cinder://%s/%s' % (store_id, image_volume.id)
+        else:
+            uri = 'cinder://%s' % image_volume.id
 
         try:
             image_registered = image_service.add_location(
diff --git a/releasenotes/notes/bug-1978020-glance-upload-uri-8fbc70c442ac620c.yaml b/releasenotes/notes/bug-1978020-glance-upload-uri-8fbc70c442ac620c.yaml
new file mode 100644
index 000000000..04a909d3d
--- /dev/null
+++ b/releasenotes/notes/bug-1978020-glance-upload-uri-8fbc70c442ac620c.yaml
@@ -0,0 +1,11 @@
+---
+fixes:
+  - |
+    `Bug #1978020 <https://bugs.launchpad.net/cinder/+bug/1978020>`_: Fixed
+    uploading a volume to a Cinder-backed Glance image; if a store name is set
+    in the volume type's extra specs, it must also be sent to Glance as part of
+    the new image location URI.
+    Please note that while the `image_service:store_id` extra spec is
+    validated when it is set for the volume type, it is not validated later;
+    it is the operator's responsibility to make sure that the Glance store is
+    not renamed or removed or that the volume types are updated accordingly.
-- 
2.35.1