diff --git a/glance/async_/flows/plugins/image_conversion.py b/glance/async_/flows/plugins/image_conversion.py index 6f5199c82d..fedbec117a 100644 --- a/glance/async_/flows/plugins/image_conversion.py +++ b/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 @@ class _ConvertImage(task.Task): 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 @@ class _ConvertImage(task.Task): # 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 @@ class _ConvertImage(task.Task): 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 @@ class _ConvertImage(task.Task): 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 @@ class _ConvertImage(task.Task): 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 @@ class _ConvertImage(task.Task): 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) ) diff --git a/glance/tests/unit/async_/flows/plugins/test_image_conversion.py b/glance/tests/unit/async_/flows/plugins/test_image_conversion.py index 1942dcc43f..28a60a4334 100644 --- a/glance/tests/unit/async_/flows/plugins/test_image_conversion.py +++ b/glance/tests/unit/async_/flows/plugins/test_image_conversion.py @@ -59,6 +59,7 @@ class TestConvertImageTask(test_utils.BaseTestCase): 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 @@ class TestConvertImageTask(test_utils.BaseTestCase): 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 @@ class TestConvertImageTask(test_utils.BaseTestCase): 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 @@ class TestConvertImageTask(test_utils.BaseTestCase): 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,15 +360,23 @@ class TestConvertImageTask(test_utils.BaseTestCase): 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) @@ -370,6 +384,48 @@ class TestConvertImageTask(test_utils.BaseTestCase): 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 @@ class TestConvertImageTask(test_utils.BaseTestCase): 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)