From a719525c1c5aa0d08af881dc1ee72f56cb5b7deb Mon Sep 17 00:00:00 2001 From: ricolin Date: Fri, 29 Apr 2022 00:34:53 +0800 Subject: [PATCH] Add image_conversion_disable config Add new config option `image_conversion_disable`, when it's set to `True`, image disk_format and volume format must be the same format. Otherwise will raise ImageUnacceptable exception. `image_conversion_disable` config is bool option, default to `False`. The idea behind this was that in certain high scale environments, it is possible that a cloud allows both qcow2 and raw image uploads. However, uploading a qcow2 image and creating a large number of volumes can cause a tremendous amount of conversions that will kill cinder-volume. It may be undesirable to have this, so a cloud operator can opt to disallow conversions and enforce that the user uploads the correct image type if they want to have volumes (aka raw in rbd case). Closes-Bug: #1970115 Change-Id: Ic481d68639d9460d1fd14225bc17a0d8287d5fd9 --- cinder/exception.py | 5 ++ cinder/image/image_utils.py | 36 +++++++++ cinder/message/message_field.py | 5 ++ .../unit/api/contrib/test_volume_actions.py | 23 ++++++ cinder/tests/unit/test_image_utils.py | 75 ++++++++----------- .../volume/flows/test_create_volume_flow.py | 57 ++++++++++++++ cinder/tests/unit/volume/test_image.py | 22 ++++++ .../tests/unit/volume/test_volume_reimage.py | 16 ++++ cinder/volume/flows/manager/create_volume.py | 9 +++ cinder/volume/manager.py | 9 ++- cinder/volume/volume_utils.py | 4 +- ...ble_image_conversion-ebf33ce9d5edf724.yaml | 36 +++++++++ 12 files changed, 250 insertions(+), 47 deletions(-) create mode 100644 releasenotes/notes/allow_disable_image_conversion-ebf33ce9d5edf724.yaml diff --git a/cinder/exception.py b/cinder/exception.py index fc7f0ce6e92..fa78239185f 100644 --- a/cinder/exception.py +++ b/cinder/exception.py @@ -1069,6 +1069,11 @@ class ImageCompressionNotAllowed(CinderException): "is compressed") +class ImageConversionNotAllowed(CinderException): + message = _("Image Conversion disallowed for image %(image_id)s: " + "%(reason)s") + + class CinderAcceleratorError(CinderException): message = _("Cinder accelerator %(accelerator)s encountered an error " "while compressing/decompressing image.\n" diff --git a/cinder/image/image_utils.py b/cinder/image/image_utils.py index b2ae795b02e..198499d8f44 100644 --- a/cinder/image/image_utils.py +++ b/cinder/image/image_utils.py @@ -73,6 +73,13 @@ image_opts = [ cfg.IntOpt('image_conversion_address_space_limit', default=1, help='Address space limit in gigabytes to convert the image'), + cfg.BoolOpt('image_conversion_disable', + default=False, + help='Disallow image conversion when creating a volume from ' + 'an image and when uploading a volume as an image. Image ' + 'conversion consumes a large amount of system resources and ' + 'can cause performance problems on the cinder-volume node. ' + 'When set True, this option disables image conversion.'), ] CONF = cfg.CONF @@ -685,6 +692,28 @@ def fetch_to_raw(context: context.RequestContext, size=size, run_as_root=run_as_root) +def check_image_conversion_disable(disk_format, volume_format, image_id, + upload=False): + if CONF.image_conversion_disable and disk_format != volume_format: + if upload: + msg = _("Image conversion is disabled. The image disk_format " + "you have requested is '%(disk_format)s', but your " + "volume can only be uploaded in the format " + "'%(volume_format)s'.") + else: + msg = _("Image conversion is disabled. The volume type you have " + "requested requires that the image it is being created " + "from be in '%(volume_format)s' format, but the image " + "you are using has the disk_format property " + "'%(disk_format)s'. You must use an image with the " + "disk_format property '%(volume_format)s' to create a " + "volume of this type.") + raise exception.ImageConversionNotAllowed( + reason=msg % {'disk_format': disk_format, + 'volume_format': volume_format}, + image_id=image_id) + + def fetch_to_volume_format(context: context.RequestContext, image_service: glance.GlanceImageService, image_id: str, @@ -699,6 +728,9 @@ def fetch_to_volume_format(context: context.RequestContext, qemu_img = True image_meta = image_service.show(context, image_id) + check_image_conversion_disable( + image_meta['disk_format'], volume_format, image_id, upload=False) + allow_image_compression = CONF.allow_compression_on_image_upload if image_meta and (image_meta.get('container_format') == 'compressed'): if allow_image_compression is False: @@ -810,6 +842,10 @@ def upload_volume(context: context.RequestContext, # NOTE: You probably want to use volume_utils.upload_volume(), # not this function. image_id = image_meta['id'] + + check_image_conversion_disable( + image_meta['disk_format'], volume_format, image_id, upload=True) + if image_meta.get('container_format') != 'compressed': if (image_meta['disk_format'] == volume_format): LOG.debug("%s was %s, no need to convert to %s", diff --git a/cinder/message/message_field.py b/cinder/message/message_field.py index f6c1657f2fc..ba145c23be9 100644 --- a/cinder/message/message_field.py +++ b/cinder/message/message_field.py @@ -130,6 +130,10 @@ class Detail(object): REIMAGE_VOLUME_FAILED = ( '028', _("Compute service failed to reimage volume.")) + IMAGE_FORMAT_UNACCEPTABLE = ( + '029', + _("The image disk format must be the same as the volume format for " + "the volume type you are requesting.")) ALL = (UNKNOWN_ERROR, DRIVER_NOT_INITIALIZED, @@ -159,6 +163,7 @@ class Detail(object): BACKUP_RESTORE_ERROR, VOLUME_INVALID_STATE, REIMAGE_VOLUME_FAILED, + IMAGE_FORMAT_UNACCEPTABLE, ) # Exception and detail mappings diff --git a/cinder/tests/unit/api/contrib/test_volume_actions.py b/cinder/tests/unit/api/contrib/test_volume_actions.py index 5f685ebed80..90a933eabda 100644 --- a/cinder/tests/unit/api/contrib/test_volume_actions.py +++ b/cinder/tests/unit/api/contrib/test_volume_actions.py @@ -885,6 +885,29 @@ class VolumeImageActionsTest(test.TestCase): 'image_name': 'image_name'}} self.assertDictEqual(expected, res_dict) + @mock.patch.object(volume_api.API, 'get', fake_volume_get_obj) + @mock.patch.object(volume_api.API, "copy_volume_to_image") + def test_copy_volume_to_image_when_image_conversion_not_allowed( + self, + mock_copy_vol_to_img): + """Make sure exception is converted properly.""" + + mock_copy_vol_to_img.side_effect = exception.ImageConversionNotAllowed + id = fake.VOLUME_ID + img = {"container_format": 'ova', + "disk_format": 'vhdx', + "image_name": 'image_name', + "force": True} + body = {"os-volume_upload_image": img} + req = fakes.HTTPRequest.blank('/v3/%s/volumes/%s/action' % + (fake.PROJECT_ID, id)) + + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller._volume_upload_image, + req, + id, + body=body) + @mock.patch.object(volume_api.API, 'get', fake_volume_get_obj) @mock.patch.object(volume_api.API, "copy_volume_to_image") def test_check_image_metadata_copy_encrypted_volume_to_image( diff --git a/cinder/tests/unit/test_image_utils.py b/cinder/tests/unit/test_image_utils.py index 0226881de7d..452b644c4a3 100644 --- a/cinder/tests/unit/test_image_utils.py +++ b/cinder/tests/unit/test_image_utils.py @@ -1085,6 +1085,7 @@ class FakeImageService(object): 'status': 'active'} +@ddt.ddt(testNameFormat=ddt.TestNameFormat.INDEX_ONLY) class TestFetchToVolumeFormat(test.TestCase): @mock.patch('cinder.image.image_utils.check_available_space') @mock.patch('cinder.image.image_utils.convert_image') @@ -1187,6 +1188,32 @@ class TestFetchToVolumeFormat(test.TestCase): mock_check_size.assert_called_once_with(data.virtual_size, size, image_id) + @ddt.data(('raw', 'qcow2', False), + ('raw', 'raw', False), + ('raw', 'raw', True)) + def test_check_image_conversion(self, conversion_opts): + image_disk_format, volume_format, image_conversion_disable = \ + conversion_opts + self.flags(image_conversion_disable=image_conversion_disable) + self.assertIsNone(image_utils.check_image_conversion_disable( + image_disk_format, volume_format, fake.IMAGE_ID)) + + @ddt.data((True, 'volume can only be uploaded in the format'), + (False, 'must use an image with the disk_format property')) + def test_check_image_conversion_disable(self, info): + # NOTE: the error message is different depending on direction, + # where True means upload + direction, message_fragment = info + self.flags(image_conversion_disable=True) + exc = self.assertRaises(exception.ImageConversionNotAllowed, + image_utils.check_image_conversion_disable, + 'foo', 'bar', fake.IMAGE_ID, + upload=direction) + if direction: + self.assertIn(message_fragment, str(exc)) + else: + self.assertIn(message_fragment, str(exc)) + @mock.patch('cinder.image.image_utils.check_virtual_size') @mock.patch('cinder.image.image_utils.check_available_space') @mock.patch('cinder.image.image_utils.convert_image') @@ -1439,51 +1466,6 @@ class TestFetchToVolumeFormat(test.TestCase): self.assertFalse(mock_copy.called) self.assertFalse(mock_convert.called) - @mock.patch('cinder.image.image_utils.convert_image') - @mock.patch('cinder.image.image_utils.volume_utils.copy_volume') - @mock.patch( - 'cinder.image.image_utils.replace_xenserver_image_with_coalesced_vhd') - @mock.patch('cinder.image.image_utils.is_xenserver_format', - return_value=False) - @mock.patch('cinder.image.image_utils.fetch') - @mock.patch('cinder.image.image_utils.qemu_img_info', - side_effect=processutils.ProcessExecutionError) - @mock.patch('cinder.image.image_utils.temporary_file') - def test_no_qemu_img_no_metadata(self, mock_temp, mock_info, - mock_fetch, mock_is_xen, mock_repl_xen, - mock_copy, mock_convert): - ctxt = mock.sentinel.context - image_service = mock.Mock(temp_images=None) - image_id = mock.sentinel.image_id - dest = mock.sentinel.dest - volume_format = mock.sentinel.volume_format - blocksize = mock.sentinel.blocksize - ctxt.user_id = user_id = mock.sentinel.user_id - project_id = mock.sentinel.project_id - size = 4321 - run_as_root = mock.sentinel.run_as_root - - tmp = mock_temp.return_value.__enter__.return_value - image_service.show.return_value = None - - self.assertRaises( - exception.ImageUnacceptable, - image_utils.fetch_to_volume_format, - ctxt, image_service, image_id, dest, volume_format, blocksize, - user_id=user_id, project_id=project_id, size=size, - run_as_root=run_as_root) - - image_service.show.assert_called_once_with(ctxt, image_id) - mock_temp.assert_called_once_with(prefix='image_download_%s_' % - image_id) - mock_info.assert_called_once_with(tmp, - force_share=False, - run_as_root=run_as_root) - self.assertFalse(mock_fetch.called) - self.assertFalse(mock_repl_xen.called) - self.assertFalse(mock_copy.called) - self.assertFalse(mock_convert.called) - @mock.patch('cinder.image.image_utils.check_virtual_size') @mock.patch('cinder.image.image_utils.convert_image') @mock.patch('cinder.image.image_utils.volume_utils.copy_volume') @@ -1513,6 +1495,7 @@ class TestFetchToVolumeFormat(test.TestCase): data.backing_file = None data.virtual_size = int(1234.5 * units.Gi) tmp = mock_temp.return_value.__enter__.return_value + image_service.show.return_value = {'disk_format': 'raw'} mock_check_size.side_effect = exception.ImageUnacceptable( image_id='fake_image_id', reason='test') @@ -1564,6 +1547,7 @@ class TestFetchToVolumeFormat(test.TestCase): data.backing_file = None data.virtual_size = 1234 tmp = mock_temp.return_value.__enter__.return_value + image_service.show.return_value = {'disk_format': 'raw'} self.assertRaises( exception.ImageUnacceptable, @@ -1606,6 +1590,7 @@ class TestFetchToVolumeFormat(test.TestCase): project_id = mock.sentinel.project_id size = 4321 run_as_root = mock.sentinel.run_as_root + image_service.show.return_value = {'disk_format': 'raw'} data = mock_info.return_value data.file_format = volume_format diff --git a/cinder/tests/unit/volume/flows/test_create_volume_flow.py b/cinder/tests/unit/volume/flows/test_create_volume_flow.py index 7482a69ee14..2a7b625bc95 100644 --- a/cinder/tests/unit/volume/flows/test_create_volume_flow.py +++ b/cinder/tests/unit/volume/flows/test_create_volume_flow.py @@ -2330,6 +2330,63 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase): self.assertFalse(self.mock_cache.ensure_space.called) self.assertFalse(self.mock_cache.create_cache_entry.called) + @mock.patch('cinder.image.image_utils.check_available_space') + @mock.patch('cinder.image.image_utils.qemu_img_info') + @mock.patch('cinder.message.api.API.create') + @mock.patch('cinder.image.image_utils.verify_glance_image_signature') + def test_create_from_image_cache_unacceptable_image_message( + self, mock_verify, mock_message_create, mock_qemu_info, + mock_check_space, + mock_get_internal_context, + mock_create_from_img_dl, mock_create_from_src, + mock_handle_bootable, mock_fetch_img): + image_info = imageutils.QemuImgInfo() + image_info.virtual_size = '1073741824' + mock_qemu_info.return_value = image_info + self.mock_driver.clone_image.return_value = (None, False) + self.mock_cache.get_entry.return_value = None + volume = fake_volume.fake_volume_obj(self.ctxt, size=1, + host='foo@bar#pool') + image_volume = fake_volume.fake_db_volume(size=2) + self.mock_db.volume_create.return_value = image_volume + image_id = fakes.IMAGE_ID + mock_create_from_img_dl.side_effect = ( + exception.ImageConversionNotAllowed(image_id=image_id, reason='')) + self.flags(verify_glance_signatures='disabled') + + image_location = 'someImageLocationStr' + image_meta = mock.MagicMock() + + manager = create_volume_manager.CreateVolumeFromSpecTask( + self.mock_volume_manager, + self.mock_db, + self.mock_driver, + image_volume_cache=self.mock_cache + ) + + self.assertRaises( + exception.ImageConversionNotAllowed, + manager._create_from_image_cache_or_download, + self.ctxt, + volume, + image_location, + image_id, + image_meta, + self.mock_image_service + ) + + mock_message_create.assert_called_once_with( + self.ctxt, message_field.Action.COPY_IMAGE_TO_VOLUME, + resource_uuid=volume.id, + detail=message_field.Detail.IMAGE_FORMAT_UNACCEPTABLE) + + # The volume size should NOT be changed when in this case + self.assertFalse(self.mock_db.volume_update.called) + + # Make sure we didn't try and create a cache entry + self.assertFalse(self.mock_cache.ensure_space.called) + self.assertFalse(self.mock_cache.create_cache_entry.called) + @ddt.data(None, {'volume_id': fakes.VOLUME_ID}) @mock.patch('cinder.volume.flows.manager.create_volume.' 'CreateVolumeFromSpecTask.' diff --git a/cinder/tests/unit/volume/test_image.py b/cinder/tests/unit/volume/test_image.py index 4d33b096315..ebc7904c40a 100644 --- a/cinder/tests/unit/volume/test_image.py +++ b/cinder/tests/unit/volume/test_image.py @@ -104,6 +104,28 @@ class CopyVolumeToImageTestCase(base.BaseVolumeTestCase): volume = db.volume_get(self.context, self.volume_id) self.assertEqual('available', volume['status']) + def test_copy_volume_to_image_with_conversion_disabled(self): + self.flags(image_conversion_disable=True) + + self.volume_attrs['instance_uuid'] = None + volume_type_id = db.volume_type_create( + self.context, {'name': 'test', 'extra_specs': { + 'image_service:store_id': 'fake_store' + }}).get('id') + self.volume_attrs['volume_type_id'] = volume_type_id + db.volume_create(self.context, self.volume_attrs) + image_meta = { + 'id': self.image_id, + 'container_format': 'ova', + 'disk_format': 'vhdx' + } + + self.assertRaises(exception.ImageConversionNotAllowed, + self.volume.copy_volume_to_image, + self.context, + self.volume_id, + image_meta) + def test_copy_volume_to_image_over_image_quota(self): # creating volume testdata self.volume_attrs['instance_uuid'] = None diff --git a/cinder/tests/unit/volume/test_volume_reimage.py b/cinder/tests/unit/volume/test_volume_reimage.py index 6902cd0e77c..e03c59bd612 100644 --- a/cinder/tests/unit/volume/test_volume_reimage.py +++ b/cinder/tests/unit/volume/test_volume_reimage.py @@ -17,6 +17,7 @@ import ddt from oslo_concurrency import processutils from cinder import exception +from cinder.message import message_field from cinder.tests.unit import fake_constants from cinder.tests.unit.image import fake as fake_image from cinder.tests.unit import utils as tests_utils @@ -65,6 +66,21 @@ class VolumeReimageTestCase(base.BaseVolumeTestCase): self.assertRaises(exception.ImageUnacceptable, self.volume.reimage, self.context, volume, self.image_meta) + mock_cp_img.side_effect = exception.ImageConversionNotAllowed( + image_id=self.image_meta['id'], reason='') + + with mock.patch.object( + self.volume.message_api, 'create' + ) as mock_msg_create: + self.assertRaises( + exception.ImageConversionNotAllowed, self.volume.reimage, + self.context, volume, self.image_meta) + mock_msg_create.assert_called_with( + self.context, + message_field.Action.REIMAGE_VOLUME, + resource_uuid=volume.id, + detail=message_field.Detail.IMAGE_FORMAT_UNACCEPTABLE) + mock_cp_img.side_effect = exception.ImageTooBig( image_id=self.image_meta['id'], reason='') self.assertRaises(exception.ImageTooBig, self.volume.reimage, diff --git a/cinder/volume/flows/manager/create_volume.py b/cinder/volume/flows/manager/create_volume.py index 3f251289055..4c18d7d8470 100644 --- a/cinder/volume/flows/manager/create_volume.py +++ b/cinder/volume/flows/manager/create_volume.py @@ -108,6 +108,7 @@ class OnFailureRescheduleTask(flow_utils.CinderTask): exception.VolumeNotFound, exception.SnapshotNotFound, exception.VolumeTypeNotFound, + exception.ImageConversionNotAllowed, exception.ImageUnacceptable, exception.ImageTooBig, exception.InvalidSignatureImage, @@ -1016,6 +1017,14 @@ class CreateVolumeFromSpecTask(flow_utils.CinderTask): detail= message_field.Detail.SIGNATURE_VERIFICATION_FAILED, exception=err) + except exception.ImageConversionNotAllowed: + with excutils.save_and_reraise_exception(): + self.message.create( + context, + message_field.Action.COPY_IMAGE_TO_VOLUME, + resource_uuid=volume.id, + detail= + message_field.Detail.IMAGE_FORMAT_UNACCEPTABLE) if should_create_cache_entry: # Update the newly created volume db entry before we clone it diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index 8792b4d80c7..d4b90b450b0 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -5344,7 +5344,7 @@ class VolumeManager(manager.CleanableManager, LOG.debug("Re-imaged %(image_id)s" " to volume %(volume_id)s successfully.", {'image_id': image_id, 'volume_id': volume.id}) - except Exception: + except Exception as err: with excutils.save_and_reraise_exception(): LOG.error('Failed to re-image volume %(volume_id)s with ' 'image %(image_id)s.', @@ -5352,3 +5352,10 @@ class VolumeManager(manager.CleanableManager, volume.previous_status = volume.status volume.status = 'error' volume.save() + if isinstance(err, exception.ImageConversionNotAllowed): + self.message_api.create( + context, + message_field.Action.REIMAGE_VOLUME, + resource_uuid=volume.id, + detail= + message_field.Detail.IMAGE_FORMAT_UNACCEPTABLE) diff --git a/cinder/volume/volume_utils.py b/cinder/volume/volume_utils.py index 01c138206ae..b58640ca6f6 100644 --- a/cinder/volume/volume_utils.py +++ b/cinder/volume/volume_utils.py @@ -1204,7 +1204,9 @@ def copy_image_to_volume(driver, "%(volume_id)s", {'volume_id': volume.id, 'image_id': image_id}) raise exception.ImageCopyFailure(reason=ex.stderr) - except (exception.ImageUnacceptable, exception.ImageTooBig): + except (exception.ImageUnacceptable, + exception.ImageTooBig, + exception.ImageConversionNotAllowed): with excutils.save_and_reraise_exception(): LOG.exception("Failed to copy image %(image_id)s to volume: " "%(volume_id)s", diff --git a/releasenotes/notes/allow_disable_image_conversion-ebf33ce9d5edf724.yaml b/releasenotes/notes/allow_disable_image_conversion-ebf33ce9d5edf724.yaml new file mode 100644 index 00000000000..b498e31311b --- /dev/null +++ b/releasenotes/notes/allow_disable_image_conversion-ebf33ce9d5edf724.yaml @@ -0,0 +1,36 @@ +--- +features: + - | + Added a new configuration option ``image_conversion_disable`` to disallow + conversion between image disk format and volume format when doing + certain operations. This can prevent performance problems on a + cinder-volume node due to the large amount of system resources + consumed during image conversion. The default value is ``False``, + which corresponds to Cinder's current behavior to always attempt + image conversion. + + This option affects three Block Storage API calls: + + * Upload volume to image: + ``POST /v3/volumes/{volume_id}/action`` with the + ``os-volume_upload_image`` action. This call will result in a + 400 (Bad Request) response when an image ``disk_format`` + that would require conversion is requested. + * Create a volume: + ``POST /v3/volumes`` with an ``imageRef`` attribute in the request + body. This will result in a 202 (Accepted) response, but if the + image's ``disk_format`` would require conversion to be written to + the volume, the volume will go to ``error`` status. + * Reimage a volume: + ``POST /v3/volumes/{volume_id}/action`` with the ``os-reimage`` + action. This call will result in a 202 (Accepted) response, but + if the image's ``disk_format`` would require conversion to be written + to the volume, the volume will go to ``error`` status. + + In the latter two cases, an end user can determine what happened + by using the `Messages API + `_, + which can be accessed using the `cinderclient + `_ + or `openstackclient + `_.