Merge "Revert image state to queued if conversion fails" into stable/2024.1

Author: Zuul

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