Merge "Add image_conversion_disable config"

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"

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

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

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(

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

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

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

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,

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

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)

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

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
+    <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>`_.