Merge pull request #44 from stackhpc/upstream/2023.1-2024-08-19

Synchronise 2023.1 with upstream

Author: markgoddard

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/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)
     )

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/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,22 +360,72 @@ 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)
             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)
+                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)

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.