diff --git a/glance/api/v2/images.py b/glance/api/v2/images.py index 95cfe7b9c3..bf74f281b5 100644 --- a/glance/api/v2/images.py +++ b/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) 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 @@ 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) ) diff --git a/glance/common/store_utils.py b/glance/common/store_utils.py index 552f661f58..e3a94d2784 100644 --- a/glance/common/store_utils.py +++ b/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 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 @@ 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,15 +360,41 @@ 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) @@ -370,6 +402,30 @@ def test_image_convert_revert_success(self, mock_os_remove): 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) diff --git a/glance/tests/unit/common/test_utils.py b/glance/tests/unit/common/test_utils.py index a07b2709d7..8069ef5742 100644 --- a/glance/tests/unit/common/test_utils.py +++ b/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""" diff --git a/releasenotes/notes/make-cinder-url-compatible-with-locations-1f3e938ff7e11c7d.yaml b/releasenotes/notes/make-cinder-url-compatible-with-locations-1f3e938ff7e11c7d.yaml new file mode 100644 index 0000000000..8c9df095cd --- /dev/null +++ b/releasenotes/notes/make-cinder-url-compatible-with-locations-1f3e938ff7e11c7d.yaml @@ -0,0 +1,9 @@ +--- +fixes: + - | + `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.