Support format info in fs type drivers

This feature adds format info in filesystem type
drivers with the following changes:
1) Store format info in admin_metadata while creating/cloning
volumes
2) Use format info while extending volumes
3) Modify volume format when performing snapshot delete
(blockRebase) operation on attached volume.
4) Return format in connection_info

blueprint add-support-store-volume-format-info

Change-Id: I43036837274a7c8dba612db53b34a6ce2cfb2f07

cinder/api/v3/attachments.py

@@ -23,6 +23,7 @@ from cinder.api.openstack import wsgi
 from cinder.api.schemas import attachments as attachment
 from cinder.api.v3.views import attachments as attachment_views
 from cinder.api import validation
+from cinder import context as cinder_context
 from cinder import exception
 from cinder.i18n import _
 from cinder import objects
@@ -53,6 +54,11 @@ class AttachmentsController(wsgi.Controller):
         """Return data about the given attachment."""
         context = req.environ['cinder.context']
         attachment = objects.VolumeAttachment.get_by_id(context, id)
+        volume = objects.Volume.get_by_id(cinder_context.get_admin_context(),
+                                          attachment.volume_id)
+        if volume.admin_metadata and 'format' in volume.admin_metadata:
+            attachment.connection_info['format'] = (
+                volume.admin_metadata['format'])
         return attachment_views.ViewBuilder.detail(attachment)
 
     @wsgi.Controller.api_version(mv.NEW_ATTACH)

cinder/image/image_utils.py

@@ -406,9 +406,12 @@ def convert_image(source, dest, out_format, out_subformat=None,
                        src_passphrase_file=src_passphrase_file)
 
 
-def resize_image(source, size, run_as_root=False):
+def resize_image(source, size, run_as_root=False, file_format=None):
     """Changes the virtual size of the image."""
-    cmd = ('qemu-img', 'resize', source, '%sG' % size)
+    if file_format:
+        cmd = ('qemu-img', 'resize', '-f', file_format, source, '%sG' % size)
+    else:
+        cmd = ('qemu-img', 'resize', source, '%sG' % size)
     utils.execute(*cmd, run_as_root=run_as_root)
 
 

cinder/tests/unit/test_image_utils.py

@@ -346,24 +346,41 @@ class TestConvertImage(test.TestCase):
                                     ' image conversion.')
 
 
+@ddt.ddt
 class TestResizeImage(test.TestCase):
     @mock.patch('cinder.utils.execute')
-    def test_defaults(self, mock_exec):
+    @ddt.data(None, 'raw', 'qcow2')
+    def test_defaults(self, file_format, mock_exec):
         source = mock.sentinel.source
         size = mock.sentinel.size
-        output = image_utils.resize_image(source, size)
+        output = image_utils.resize_image(source, size,
+                                          file_format=file_format)
         self.assertIsNone(output)
-        mock_exec.assert_called_once_with('qemu-img', 'resize', source,
+        if file_format:
-                                          'sentinel.sizeG', run_as_root=False)
+            mock_exec.assert_called_once_with(
+                'qemu-img', 'resize', '-f', file_format, source,
+                'sentinel.sizeG', run_as_root=False)
+        else:
+            mock_exec.assert_called_once_with('qemu-img', 'resize',
+                                              source, 'sentinel.sizeG',
+                                              run_as_root=False)
 
     @mock.patch('cinder.utils.execute')
-    def test_run_as_root(self, mock_exec):
+    @ddt.data(None, 'raw', 'qcow2')
+    def test_run_as_root(self, file_format, mock_exec):
         source = mock.sentinel.source
         size = mock.sentinel.size
-        output = image_utils.resize_image(source, size, run_as_root=True)
+        output = image_utils.resize_image(source, size, run_as_root=True,
+                                          file_format=file_format)
         self.assertIsNone(output)
-        mock_exec.assert_called_once_with('qemu-img', 'resize', source,
+        if file_format:
-                                          'sentinel.sizeG', run_as_root=True)
+            mock_exec.assert_called_once_with(
+                'qemu-img', 'resize', '-f', file_format, source,
+                'sentinel.sizeG', run_as_root=True)
+        else:
+            mock_exec.assert_called_once_with('qemu-img', 'resize',
+                                              source, 'sentinel.sizeG',
+                                              run_as_root=True)
 
 
 class TestFetch(test.TestCase):

cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_base.py

@@ -561,10 +561,16 @@ class NetAppNfsDriverTestCase(test.TestCase):
 
         self.assertIn("nfs://host/path/image-id", locations)
 
-    def test_extend_volume(self):
+    @ddt.data(None, 'raw', 'qcow2')
+    @mock.patch('cinder.objects.volume.Volume.get_by_id')
+    def test_extend_volume(self, file_format, mock_get):
 
+        volume = fake_volume.fake_volume_obj(self.ctxt)
+        if file_format:
+            volume.admin_metadata = {'format': file_format}
+        mock_get.return_value = volume
         new_size = 100
-        volume_copy = copy.copy(fake.VOLUME)
+        volume_copy = copy.copy(volume)
         volume_copy['size'] = new_size
 
         path = '%s/%s' % (fake.NFS_SHARE, fake.NFS_VOLUME['name'])
@@ -578,18 +584,22 @@ class NetAppNfsDriverTestCase(test.TestCase):
         mock_do_qos_for_volume = self.mock_object(self.driver,
                                                   '_do_qos_for_volume')
 
-        self.driver.extend_volume(fake.VOLUME, new_size)
+        self.driver.extend_volume(volume, new_size)
 
-        mock_resize_image_file.assert_called_once_with(path, new_size)
+        mock_resize_image_file.assert_called_once_with(path, new_size,
-        mock_get_volume_extra_specs.assert_called_once_with(fake.VOLUME)
+                                                       file_format=file_format)
+        mock_get_volume_extra_specs.assert_called_once_with(volume)
         mock_do_qos_for_volume.assert_called_once_with(volume_copy,
                                                        fake.EXTRA_SPECS,
                                                        cleanup=False)
 
-    def test_extend_volume_resize_error(self):
+    @mock.patch('cinder.objects.volume.Volume.get_by_id')
+    def test_extend_volume_resize_error(self, mock_get):
 
+        volume = fake_volume.fake_volume_obj(self.ctxt)
+        mock_get.return_value = volume
         new_size = 100
-        volume_copy = copy.copy(fake.VOLUME)
+        volume_copy = copy.copy(volume)
         volume_copy['size'] = new_size
 
         path = '%s/%s' % (fake.NFS_SHARE, fake.NFS_VOLUME['name'])
@@ -606,17 +616,21 @@ class NetAppNfsDriverTestCase(test.TestCase):
 
         self.assertRaises(exception.VolumeBackendAPIException,
                           self.driver.extend_volume,
-                          fake.VOLUME,
+                          volume,
                           new_size)
 
-        mock_resize_image_file.assert_called_once_with(path, new_size)
+        mock_resize_image_file.assert_called_once_with(path, new_size,
+                                                       file_format=None)
         self.assertFalse(mock_get_volume_extra_specs.called)
         self.assertFalse(mock_do_qos_for_volume.called)
 
-    def test_extend_volume_qos_error(self):
+    @mock.patch('cinder.objects.volume.Volume.get_by_id')
+    def test_extend_volume_qos_error(self, mock_get):
 
+        volume = fake_volume.fake_volume_obj(self.ctxt)
+        mock_get.return_value = volume
         new_size = 100
-        volume_copy = copy.copy(fake.VOLUME)
+        volume_copy = copy.copy(volume)
         volume_copy['size'] = new_size
 
         path = '%s/%s' % (fake.NFS_SHARE, fake.NFS_VOLUME['name'])
@@ -634,11 +648,12 @@ class NetAppNfsDriverTestCase(test.TestCase):
 
         self.assertRaises(exception.VolumeBackendAPIException,
                           self.driver.extend_volume,
-                          fake.VOLUME,
+                          volume,
                           new_size)
 
-        mock_resize_image_file.assert_called_once_with(path, new_size)
+        mock_resize_image_file.assert_called_once_with(path, new_size,
-        mock_get_volume_extra_specs.assert_called_once_with(fake.VOLUME)
+                                                       file_format=None)
+        mock_get_volume_extra_specs.assert_called_once_with(volume)
         mock_do_qos_for_volume.assert_called_once_with(volume_copy,
                                                        fake.EXTRA_SPECS,
                                                        cleanup=False)

cinder/tests/unit/volume/drivers/test_nfs.py

@@ -774,7 +774,8 @@ class NfsDriverTestCase(test.TestCase):
 
             self.assertEqual(ret, 0.14)
 
-    def test_create_sparsed_volume(self):
+    @mock.patch('cinder.objects.volume.Volume.save')
+    def test_create_sparsed_volume(self, mock_save):
         self._set_driver()
         drv = self._driver
         volume = self._simple_volume()
@@ -791,7 +792,8 @@ class NfsDriverTestCase(test.TestCase):
                                                                  mock.ANY)
                 mock_set_rw_permissions.assert_called_once_with(mock.ANY)
 
-    def test_create_nonsparsed_volume(self):
+    @mock.patch('cinder.objects.volume.Volume.save')
+    def test_create_nonsparsed_volume(self, mock_save):
         self._set_driver()
         drv = self._driver
         self.configuration.nfs_sparsed_volumes = False
@@ -1040,7 +1042,9 @@ class NfsDriverTestCase(test.TestCase):
                                                        total_allocated,
                                                        requested_volume_size))
 
-    def test_extend_volume(self):
+    @ddt.data(None, 'raw', 'qcow2')
+    @mock.patch('cinder.objects.volume.Volume.get_by_id')
+    def test_extend_volume(self, file_format, mock_get):
         """Extend a volume by 1."""
         self._set_driver()
         drv = self._driver
@@ -1049,6 +1053,9 @@ class NfsDriverTestCase(test.TestCase):
             id='80ee16b6-75d2-4d54-9539-ffc1b4b0fb10',
             size=1,
             provider_location='nfs_share')
+        if file_format:
+            volume.admin_metadata = {'format': file_format}
+        mock_get.return_value = volume
         path = 'path'
         newSize = volume['size'] + 1
 
@@ -1061,7 +1068,8 @@ class NfsDriverTestCase(test.TestCase):
                         drv.extend_volume(volume, newSize)
 
                         resize.assert_called_once_with(path, newSize,
-                                                       run_as_root=True)
+                                                       run_as_root=True,
+                                                       file_format=file_format)
 
     def test_extend_volume_attached_fail(self):
         """Extend a volume by 1."""
@@ -1085,7 +1093,8 @@ class NfsDriverTestCase(test.TestCase):
                         self.assertRaises(exception.ExtendVolumeError,
                                           drv.extend_volume, volume, newSize)
 
-    def test_extend_volume_failure(self):
+    @mock.patch('cinder.objects.volume.Volume.get_by_id')
+    def test_extend_volume_failure(self, mock_get):
         """Error during extend operation."""
         self._set_driver()
         drv = self._driver
@@ -1094,6 +1103,8 @@ class NfsDriverTestCase(test.TestCase):
             id='80ee16b6-75d2-4d54-9539-ffc1b4b0fb10',
             size=1,
             provider_location='nfs_share')
+        volume.admin_metadata = {'format': 'qcow2'}
+        mock_get.return_value = volume
 
         with mock.patch.object(image_utils, 'resize_image'):
             with mock.patch.object(drv, 'local_path', return_value='path'):
@@ -1359,8 +1370,9 @@ class NfsDriverTestCase(test.TestCase):
               [NFS_CONFIG3, QEMU_IMG_INFO_OUT3, 'available'],
               [NFS_CONFIG4, QEMU_IMG_INFO_OUT4, 'backing-up'])
     @ddt.unpack
+    @mock.patch('cinder.objects.volume.Volume.save')
     def test_create_volume_from_snapshot(self, nfs_conf, qemu_img_info,
-                                         snap_status):
+                                         snap_status, mock_save):
         self._set_driver(extra_confs=nfs_conf)
         drv = self._driver
 

cinder/tests/unit/volume/drivers/test_remotefs.py

@@ -767,6 +767,97 @@ class RemoteFsSnapDriverTestCase(test.TestCase):
                 [mock.call(src_vref), mock.call(volume_ref)])
             mock_extend_volume.assert_called_once_with(volume_ref, volume.size)
 
+    @ddt.data(None, 'raw', 'qcow2')
+    @mock.patch.object(sys.modules['cinder.objects'], "Snapshot")
+    @mock.patch.object(remotefs.RemoteFSSnapDriver, 'local_path')
+    @mock.patch.object(remotefs.RemoteFSSnapDriver, '_snapshots_exist')
+    @mock.patch.object(remotefs.RemoteFSSnapDriver, '_copy_volume_image')
+    @mock.patch.object(remotefs.RemoteFSSnapDriver, '_extend_volume')
+    @mock.patch.object(remotefs.RemoteFSSnapDriver, '_validate_state')
+    @mock.patch.object(remotefs.RemoteFSSnapDriver, '_create_snapshot')
+    @mock.patch.object(remotefs.RemoteFSSnapDriver, '_delete_snapshot')
+    @mock.patch.object(remotefs.RemoteFSSnapDriver,
+                       '_copy_volume_from_snapshot')
+    def test_create_cloned_volume_with_format(
+            self, file_format, mock_copy_volume_from_snapshot,
+            mock_delete_snapshot, mock_create_snapshot,
+            mock_validate_state, mock_extend_volume,
+            mock_copy_volume_image, mock_snapshots_exist,
+            mock_local_path, mock_obj_snap):
+        drv = self._driver
+
+        # prepare test
+        volume = fake_volume.fake_volume_obj(self.context)
+        src_vref_id = '375e32b2-804a-49f2-b282-85d1d5a5b9e1'
+        src_vref = fake_volume.fake_volume_obj(
+            self.context,
+            id=src_vref_id,
+            name='volume-%s' % src_vref_id,
+            obj_context=self.context)
+        src_vref.context = self.context
+        if file_format:
+            src_vref.admin_metadata = {'format': file_format}
+
+        mock_snapshots_exist.return_value = False
+        drv._always_use_temp_snap_when_cloning = False
+
+        vol_attrs = ['provider_location', 'size', 'id', 'name', 'status',
+                     'volume_type', 'metadata', 'obj_context']
+        Volume = collections.namedtuple('Volume', vol_attrs)
+
+        volume_ref = Volume(id=volume.id,
+                            metadata=volume.metadata,
+                            name=volume.name,
+                            provider_location=volume.provider_location,
+                            status=volume.status,
+                            size=volume.size,
+                            volume_type=volume.volume_type,
+                            obj_context=self.context,)
+
+        snap_args_creation = {
+            'volume_id': src_vref.id,
+            'user_id': None,
+            'project_id': None,
+            'status': fields.SnapshotStatus.CREATING,
+            'progress': '0%',
+            'volume_size': src_vref.size,
+            'display_name': 'tmp-snap-%s' % volume.id,
+            'display_description': None,
+            'volume_type_id': src_vref.volume_type_id,
+            'encryption_key_id': None,
+        }
+        snap_args_deletion = snap_args_creation.copy()
+        snap_args_deletion["status"] = fields.SnapshotStatus.DELETED
+        snap_args_deletion["deleted"] = True
+
+        mock_obj_snap.return_value = mock.Mock()
+        mock_obj_snap.return_value.create = mock.Mock()
+        # end of prepare test
+
+        # run test
+        drv.create_cloned_volume(volume, src_vref)
+
+        # evaluate test
+        exp_acceptable_states = ['available', 'backing-up', 'downloading']
+        mock_validate_state.assert_called_once_with(
+            src_vref.status,
+            exp_acceptable_states,
+            obj_description='source volume')
+
+        self.assertFalse(mock_create_snapshot.called)
+
+        mock_snapshots_exist.assert_called_once_with(src_vref)
+
+        mock_copy_volume_image.assert_called_once_with(
+            mock_local_path.return_value,
+            mock_local_path.return_value)
+        mock_local_path.assert_has_calls(
+            [mock.call(src_vref), mock.call(volume_ref)])
+        mock_extend_volume.assert_called_once_with(volume_ref, volume.size)
+        if file_format:
+            self.assertEqual(file_format,
+                             volume.admin_metadata['format'])
+
     @mock.patch('tempfile.NamedTemporaryFile')
     @mock.patch('cinder.volume.volume_utils.check_encryption_provider',
                 return_value={'encryption_key_id': fake.ENCRYPTION_KEY_ID})

cinder/volume/drivers/netapp/dataontap/nfs_base.py

@@ -36,9 +36,11 @@ from oslo_utils import units
 import six
 from six.moves import urllib
 
+from cinder import context
 from cinder import exception
 from cinder.i18n import _
 from cinder.image import image_utils
+from cinder import objects
 import cinder.privsep.path
 from cinder import utils
 from cinder.volume import driver
@@ -644,7 +646,7 @@ class NetAppNfsDriver(driver.ManageableVD,
         raise exception.InvalidResults(
             _("NFS file could not be discovered."))
 
-    def _resize_image_file(self, path, new_size):
+    def _resize_image_file(self, path, new_size, file_format=None):
         """Resize the image file on share to new size."""
         LOG.debug('Checking file for resize')
         if self._is_file_size_equal(path, new_size):
@@ -652,10 +654,10 @@ class NetAppNfsDriver(driver.ManageableVD,
         else:
             LOG.info('Resizing file to %sG', new_size)
             image_utils.resize_image(path, new_size,
-                                     run_as_root=self._execute_as_root)
+                                     run_as_root=self._execute_as_root,
-            if self._is_file_size_equal(path, new_size):
+                                     file_format=file_format)
-                return
+            if file_format == 'qcow2' and not self._is_file_size_equal(
-            else:
+                    path, new_size):
                 raise exception.InvalidResults(
                     _('Resizing image file failed.'))
 
@@ -798,7 +800,13 @@ class NetAppNfsDriver(driver.ManageableVD,
 
         try:
             path = self.local_path(volume)
-            self._resize_image_file(path, new_size)
+            file_format = None
+            admin_metadata = objects.Volume.get_by_id(
+                context.get_admin_context(), volume.id).admin_metadata
+            if admin_metadata and 'format' in admin_metadata:
+                file_format = admin_metadata['format']
+            self._resize_image_file(
+                path, new_size, file_format=file_format)
         except Exception as err:
             exception_msg = (_("Failed to extend volume "
                                "%(name)s, Error msg: %(msg)s.") %

cinder/volume/drivers/nfs.py

@@ -28,11 +28,13 @@ from oslo_log import log as logging
 from oslo_utils import units
 import six
 
+from cinder import context
 from cinder import coordination
 from cinder import exception
 from cinder.i18n import _
 from cinder.image import image_utils
 from cinder import interface
+from cinder import objects
 from cinder import utils
 from cinder.volume import configuration
 from cinder.volume.drivers import remotefs
@@ -383,9 +385,16 @@ class NfsDriver(remotefs.RemoteFSSnapDriverDistributed):
                                               % (volume.id, new_size))
         path = self.local_path(volume)
         LOG.info('Resizing file to %sG...', new_size)
+        file_format = None
+        admin_metadata = objects.Volume.get_by_id(
+            context.get_admin_context(), volume.id).admin_metadata
+        if admin_metadata and 'format' in admin_metadata:
+            file_format = admin_metadata['format']
         image_utils.resize_image(path, new_size,
-                                 run_as_root=self._execute_as_root)
+                                 run_as_root=self._execute_as_root,
-        if not self._is_file_size_equal(path, new_size):
+                                 file_format=file_format)
+        if file_format == 'qcow2' and not self._is_file_size_equal(
+                path, new_size):
             raise exception.ExtendVolumeError(
                 reason='Resizing image file failed.')
 

cinder/volume/drivers/remotefs.py

@@ -172,6 +172,7 @@ class RemoteFSDriver(driver.BaseVD):
         self._execute_as_root = True
         self._is_voldb_empty_at_startup = kwargs.pop('is_vol_db_empty', None)
         self._supports_encryption = False
+        self.format = 'raw'
 
         if self.configuration:
             self.configuration.append_config_values(nas_opts)
@@ -316,6 +317,7 @@ class RemoteFSDriver(driver.BaseVD):
             # QCOW2 volumes are inherently sparse, so this setting
             # will override the _sparsed_volumes setting.
             self._create_qcow2_file(volume_path, volume_size)
+            self.format = 'qcow2'
         elif getattr(self.configuration,
                      self.driver_prefix + '_sparsed_volumes', False):
             self._create_sparsed_file(volume_path, volume_size)
@@ -323,6 +325,11 @@ class RemoteFSDriver(driver.BaseVD):
             self._create_regular_file(volume_path, volume_size)
 
         self._set_rw_permissions(volume_path)
+        volume.admin_metadata['format'] = self.format
+        # This is done here because when creating a volume from image,
+        # while encountering other volume.save() method fails for non-admins
+        with volume.obj_as_admin():
+            volume.save()
 
     def _ensure_shares_mounted(self):
         """Look for remote shares in the flags and mount them locally."""
@@ -1169,6 +1176,9 @@ class RemoteFSSnapDriverBase(RemoteFSDriver):
                                     self.local_path(volume_info))
             self._extend_volume(volume_info, volume.size)
 
+        if src_vref.admin_metadata and 'format' in src_vref.admin_metadata:
+            volume.admin_metadata['format'] = (
+                src_vref.admin_metadata['format'])
         return {'provider_location': src_vref.provider_location}
 
     def _copy_volume_image(self, src_path, dest_path):
@@ -1725,6 +1735,7 @@ class RemoteFSSnapDriverBase(RemoteFSDriver):
         # active file never changes
         info_path = self._local_path_volume_info(snapshot.volume)
         snap_info = self._read_info_file(info_path)
+        update_format = False
 
         if utils.paths_normcase_equal(info['active_file'],
                                       info['snapshot_file']):
@@ -1746,6 +1757,7 @@ class RemoteFSSnapDriverBase(RemoteFSDriver):
                            'volume_id': snapshot.volume.id}
 
             del(snap_info[snapshot.id])
+            update_format = True
         else:
             # blockCommit snapshot into base
             # info['base'] <= snapshot_file
@@ -1761,6 +1773,11 @@ class RemoteFSSnapDriverBase(RemoteFSDriver):
 
         self._nova_assisted_vol_snap_delete(context, snapshot, delete_info)
 
+        if update_format:
+            snapshot.volume.admin_metadata['format'] = 'qcow2'
+            with snapshot.volume.obj_as_admin():
+                snapshot.volume.save()
+
         # Write info file updated above
         self._write_info_file(info_path, snap_info)
 

releasenotes/notes/store-volume-format-info-1e17e029a9a9e578.yaml

@@ -0,0 +1,11 @@
+---
+features:
+  - |
+    Cinder now stores the format of the backing file (raw or qcow2), for FS
+    backends, in the volume admin metadata and includes the format in the
+    connection_info returned in the Attachments API.
+    Previously cinder tried to introspect the format, and under some
+    circumstances, an incorrect format would be deduced. This will still be the
+    case for legacy volumes. Explicitly storing the format will avoid this issue
+    for newly created volumes.
+    `See spec for more info <https://review.opendev.org/c/openstack/cinder-specs/+/760999>`_.