From ace17482185f0870d6e26cd0ef8cafb3fd17610b Mon Sep 17 00:00:00 2001 From: Rajat Dhasmana Date: Tue, 3 Nov 2020 08:45:22 -0500 Subject: [PATCH] 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 | 6 ++ cinder/image/image_utils.py | 7 +- cinder/tests/unit/test_image_utils.py | 33 +++++-- .../drivers/netapp/dataontap/test_nfs_base.py | 43 ++++++--- cinder/tests/unit/volume/drivers/test_nfs.py | 24 +++-- .../unit/volume/drivers/test_remotefs.py | 91 +++++++++++++++++++ .../drivers/netapp/dataontap/nfs_base.py | 20 ++-- cinder/volume/drivers/nfs.py | 13 ++- cinder/volume/drivers/remotefs.py | 17 ++++ ...e-volume-format-info-1e17e029a9a9e578.yaml | 11 +++ 10 files changed, 227 insertions(+), 38 deletions(-) create mode 100644 releasenotes/notes/store-volume-format-info-1e17e029a9a9e578.yaml diff --git a/cinder/api/v3/attachments.py b/cinder/api/v3/attachments.py index 420f3101832..fa01cb5467f 100644 --- a/cinder/api/v3/attachments.py +++ b/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) diff --git a/cinder/image/image_utils.py b/cinder/image/image_utils.py index 673bf1d36ec..0ca99c4bc3d 100644 --- a/cinder/image/image_utils.py +++ b/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) diff --git a/cinder/tests/unit/test_image_utils.py b/cinder/tests/unit/test_image_utils.py index 2cee7bf71b4..dc6add96714 100644 --- a/cinder/tests/unit/test_image_utils.py +++ b/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, - 'sentinel.sizeG', run_as_root=False) + if file_format: + 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, - 'sentinel.sizeG', run_as_root=True) + if file_format: + 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): diff --git a/cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_base.py b/cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_base.py index 1fb74b4c2dc..1c7217bbd2c 100644 --- a/cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_base.py +++ b/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_get_volume_extra_specs.assert_called_once_with(fake.VOLUME) + mock_resize_image_file.assert_called_once_with(path, new_size, + 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_get_volume_extra_specs.assert_called_once_with(fake.VOLUME) + mock_resize_image_file.assert_called_once_with(path, new_size, + 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) diff --git a/cinder/tests/unit/volume/drivers/test_nfs.py b/cinder/tests/unit/volume/drivers/test_nfs.py index 6825003e94d..51514bc6b97 100644 --- a/cinder/tests/unit/volume/drivers/test_nfs.py +++ b/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 diff --git a/cinder/tests/unit/volume/drivers/test_remotefs.py b/cinder/tests/unit/volume/drivers/test_remotefs.py index 44961dca60f..8e5e2ac7a9e 100644 --- a/cinder/tests/unit/volume/drivers/test_remotefs.py +++ b/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}) diff --git a/cinder/volume/drivers/netapp/dataontap/nfs_base.py b/cinder/volume/drivers/netapp/dataontap/nfs_base.py index 79614e4528a..927f8ebee36 100644 --- a/cinder/volume/drivers/netapp/dataontap/nfs_base.py +++ b/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) - if self._is_file_size_equal(path, new_size): - return - else: + run_as_root=self._execute_as_root, + file_format=file_format) + if file_format == 'qcow2' and not self._is_file_size_equal( + 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.") % diff --git a/cinder/volume/drivers/nfs.py b/cinder/volume/drivers/nfs.py index 3808a7e4743..5174774e376 100644 --- a/cinder/volume/drivers/nfs.py +++ b/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) - if not self._is_file_size_equal(path, new_size): + run_as_root=self._execute_as_root, + 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.') diff --git a/cinder/volume/drivers/remotefs.py b/cinder/volume/drivers/remotefs.py index 0c66b03dff6..397f95c0b70 100644 --- a/cinder/volume/drivers/remotefs.py +++ b/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) diff --git a/releasenotes/notes/store-volume-format-info-1e17e029a9a9e578.yaml b/releasenotes/notes/store-volume-format-info-1e17e029a9a9e578.yaml new file mode 100644 index 00000000000..3db4216e981 --- /dev/null +++ b/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 `_. \ No newline at end of file