openstack/glance
Code Issues Proposed changes

Merge "Revert image state to queued if conversion fails"

Browse Source
This commit is contained in:
Zuul 2024-08-02 18:19:23 +00:00 committed by Gerrit Code Review
commit 695fcb67c9
2 changed files with 94 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

37
glance/async_/flows/plugins/image_conversion.py
View File

@ -63,13 +63,16 @@ class _ConvertImage(task.Task):
default_provides = 'file_path' 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.context = context
self.task_id = task_id self.task_id = task_id
self.task_type = task_type self.task_type = task_type
self.action_wrapper = action_wrapper self.action_wrapper = action_wrapper
self.stores = stores
self.image_id = action_wrapper.image_id self.image_id = action_wrapper.image_id
self.dest_path = "" self.dest_path = ""
self.src_path = ""
self.python = CONF.wsgi.python_interpreter self.python = CONF.wsgi.python_interpreter
super(_ConvertImage, self).__init__( super(_ConvertImage, self).__init__(
name='%s-Convert_Image-%s' % (task_type, task_id)) name='%s-Convert_Image-%s' % (task_type, task_id))
@ -83,8 +86,8 @@ class _ConvertImage(task.Task):
target_format = CONF.image_conversion.output_format target_format = CONF.image_conversion.output_format
# TODO(jokke): Once we support other schemas we need to take them into # TODO(jokke): Once we support other schemas we need to take them into
# account and handle the paths here. # account and handle the paths here.
src_path = file_path.split('file://')[-1] self.src_path = file_path.split('file://')[-1]
dest_path = "%(path)s.%(target)s" % {'path': src_path, dest_path = "%(path)s.%(target)s" % {'path': self.src_path,
'target': target_format} 'target': target_format}
self.dest_path = dest_path self.dest_path = dest_path
@ -104,7 +107,7 @@ class _ConvertImage(task.Task):
# qemu-img on it. # qemu-img on it.
# See https://bugs.launchpad.net/nova/+bug/2059809 for details. # See https://bugs.launchpad.net/nova/+bug/2059809 for details.
try: try:
inspector = inspector_cls.from_file(src_path) inspector = inspector_cls.from_file(self.src_path)
if not inspector.safety_check(): if not inspector.safety_check():
LOG.error('Image failed %s safety check; aborting conversion', LOG.error('Image failed %s safety check; aborting conversion',
source_format) source_format)
@ -123,7 +126,7 @@ class _ConvertImage(task.Task):
stdout, stderr = putils.trycmd("qemu-img", "info", stdout, stderr = putils.trycmd("qemu-img", "info",
"-f", source_format, "-f", source_format,
"--output=json", "--output=json",
src_path, self.src_path,
prlimit=utils.QEMU_IMG_PROC_LIMITS, prlimit=utils.QEMU_IMG_PROC_LIMITS,
python_exec=self.python, python_exec=self.python,
log_errors=putils.LOG_ALL_ERRORS,) log_errors=putils.LOG_ALL_ERRORS,)
@ -186,7 +189,7 @@ class _ConvertImage(task.Task):
stdout, stderr = putils.trycmd('qemu-img', 'convert', stdout, stderr = putils.trycmd('qemu-img', 'convert',
'-f', source_format, '-f', source_format,
'-O', target_format, '-O', target_format,
src_path, dest_path, self.src_path, dest_path,
log_errors=putils.LOG_ALL_ERRORS) log_errors=putils.LOG_ALL_ERRORS)
except OSError as exc: except OSError as exc:
with excutils.save_and_reraise_exception(): 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'), LOG.info(_LI('Updated image %s size=%i disk_format=%s'),
self.image_id, new_size, target_format) self.image_id, new_size, target_format)
os.remove(src_path) os.remove(self.src_path)
return "file://%s" % dest_path return "file://%s" % dest_path
@ -217,6 +220,23 @@ class _ConvertImage(task.Task):
if os.path.exists(self.dest_path): if os.path.exists(self.dest_path):
os.remove(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): def get_flow(**kwargs):
"""Return task flow for no-op. """Return task flow for no-op.
@ -232,7 +252,8 @@ def get_flow(**kwargs):
task_id = kwargs.get('task_id') task_id = kwargs.get('task_id')
task_type = kwargs.get('task_type') task_type = kwargs.get('task_type')
action_wrapper = kwargs.get('action_wrapper') action_wrapper = kwargs.get('action_wrapper')
stores = kwargs.get('backend', [])
return lf.Flow(task_type).add( return lf.Flow(task_type).add(
_ConvertImage(context, task_id, task_type, action_wrapper) _ConvertImage(context, task_id, task_type, action_wrapper, stores)
) )

73
glance/tests/unit/async_/flows/plugins/test_image_conversion.py
View File

@ -59,6 +59,7 @@ class TestConvertImageTask(test_utils.BaseTestCase):
self.context = mock.MagicMock() self.context = mock.MagicMock()
self.img_repo = mock.MagicMock() self.img_repo = mock.MagicMock()
self.task_repo = mock.MagicMock() self.task_repo = mock.MagicMock()
self.stores = mock.MagicMock()
self.image_id = UUID1 self.image_id = UUID1
self.gateway = gateway.Gateway() self.gateway = gateway.Gateway()
@ -87,7 +88,10 @@ class TestConvertImageTask(test_utils.BaseTestCase):
task_input=task_input) task_input=task_input)
self.image.extra_properties = { 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.wrapper = import_flow.ImportActionWrapper(self.img_repo,
self.image_id, self.image_id,
self.task.task_id) self.task.task_id)
@ -105,7 +109,8 @@ class TestConvertImageTask(test_utils.BaseTestCase):
image_convert = image_conversion._ConvertImage(self.context, image_convert = image_conversion._ConvertImage(self.context,
self.task.task_id, self.task.task_id,
self.task_type, self.task_type,
self.wrapper) self.wrapper,
self.stores)
self.task_repo.get.return_value = self.task self.task_repo.get.return_value = self.task
image = mock.MagicMock(image_id=self.image_id, virtual_size=None, 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, image_convert = image_conversion._ConvertImage(self.context,
self.task.task_id, self.task.task_id,
self.task_type, self.task_type,
self.wrapper) self.wrapper,
self.stores)
self.task_repo.get.return_value = self.task self.task_repo.get.return_value = self.task
image = mock.MagicMock(image_id=self.image_id, virtual_size=None, 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 image = self.img_repo.get.return_value
self.assertEqual(123, image.virtual_size) self.assertEqual(123, image.virtual_size)
@mock.patch.object(os, 'remove') def _set_image_conversion(self, mock_os_remove, stores=[]):
def test_image_convert_revert_success(self, mock_os_remove):
mock_os_remove.return_value = None mock_os_remove.return_value = None
wrapper = mock.MagicMock()
image_convert = image_conversion._ConvertImage(self.context, image_convert = image_conversion._ConvertImage(self.context,
self.task.task_id, self.task.task_id,
self.task_type, self.task_type,
self.wrapper) wrapper,
stores)
action = wrapper.__enter__.return_value
self.task_repo.get.return_value = self.task 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: with mock.patch.object(processutils, 'execute') as exc_mock:
exc_mock.return_value = ("", None) exc_mock.return_value = ("", None)
@ -370,6 +384,48 @@ class TestConvertImageTask(test_utils.BaseTestCase):
os_exists_mock.return_value = True os_exists_mock.return_value = True
image_convert.revert(result=mock.MagicMock()) image_convert.revert(result=mock.MagicMock())
self.assertEqual(1, mock_os_remove.call_count) 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): def test_image_convert_interpreter_configured(self):
# By default, wsgi.python_interpreter is None; if it is # 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, convert = image_conversion._ConvertImage(self.context,
self.task.task_id, self.task.task_id,
self.task_type, self.task_type,
self.wrapper) self.wrapper,
self.stores)
self.assertEqual(fake_interpreter, convert.python) self.assertEqual(fake_interpreter, convert.python)