openstack/cinder
Code Issues Proposed changes

Add image_conversion_disable config

Browse Source 
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
This commit is contained in:
ricolin 2022-04-29 00:34:53 +08:00 committed by Rico Lin
parent 405173d477
commit a719525c1c
12 changed files with 250 additions and 47 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
cinder/exception.py
View File

@ -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"

36
cinder/image/image_utils.py
View File

@ -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",

5
cinder/message/message_field.py
View File

@ -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

23
cinder/tests/unit/api/contrib/test_volume_actions.py
View File

@ -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(

75
cinder/tests/unit/test_image_utils.py
View File

@ -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

57
cinder/tests/unit/volume/flows/test_create_volume_flow.py
View File

@ -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.'

22
cinder/tests/unit/volume/test_image.py
View File

@ -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

16
cinder/tests/unit/volume/test_volume_reimage.py
View File

@ -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,

9
cinder/volume/flows/manager/create_volume.py
View File

@ -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

9
cinder/volume/manager.py
View File

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

4
cinder/volume/volume_utils.py
View File

@ -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",

36
releasenotes/notes/allow_disable_image_conversion-ebf33ce9d5edf724.yaml Normal file
View File

@ -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
<https://docs.openstack.org/api-ref/block-storage/v3/#messages-messages>`_,
which can be accessed using the `cinderclient
<https://docs.openstack.org/python-cinderclient/latest/cli/details.html#cinder-message-list>`_
or `openstackclient
<https://docs.openstack.org/python-openstackclient/latest/cli/decoder.html>`_.