From 1dd3dea3cd710cb31b00eed1a8ecefec07bae1ce Mon Sep 17 00:00:00 2001 From: Lucian Petrut Date: Fri, 9 Jun 2017 14:02:56 +0300 Subject: [PATCH] SMBFS: enhance volume cloning This change enhances the volume clone operation by simply copying the image file if the cloned volume doesn't have any snapshots. This way, we don't have to convert a chain of images. Also, this means that we may benefit from copy offloading. This improvement especially helps image caching, which relies on efficient volume cloning. This is basically implemented in the base remotefs module, any similar driver leveraging it may enable it by switching a flag, assuming that it implements the required methods. Note that the copied image needs to be extended according to the new volume size. We cannot simply call the 'extend_volume' method as most of the RemoteFS based drivers will apply a lock to it, in which case we'd get into a deadlock. For this reason, we follow the common practice among those drivers, involving a helper that won't get a lock decorator. Change-Id: I96ac2f019d522ebbeb6c28fc4c11b41d9ba8fdc0 --- .../unit/volume/drivers/test_remotefs.py | 101 ++++++++++++++++-- cinder/tests/unit/windows/test_smbfs.py | 6 ++ cinder/volume/drivers/remotefs.py | 77 ++++++++++--- cinder/volume/drivers/windows/smbfs.py | 14 +-- 4 files changed, 168 insertions(+), 30 deletions(-) diff --git a/cinder/tests/unit/volume/drivers/test_remotefs.py b/cinder/tests/unit/volume/drivers/test_remotefs.py index 3f276305a3d..c36e8a70d67 100644 --- a/cinder/tests/unit/volume/drivers/test_remotefs.py +++ b/cinder/tests/unit/volume/drivers/test_remotefs.py @@ -460,8 +460,65 @@ class RemoteFsSnapDriverTestCase(test.TestCase): basedir=basedir, valid_backing_file=False) + @mock.patch.object(remotefs.RemoteFSSnapDriver, '_local_volume_dir') @mock.patch.object(remotefs.RemoteFSSnapDriver, - '_validate_state') + 'get_active_image_from_info') + def test_local_path_active_image(self, mock_get_active_img, + mock_local_vol_dir): + fake_vol_dir = 'fake_vol_dir' + fake_active_img = 'fake_active_img_fname' + + mock_get_active_img.return_value = fake_active_img + mock_local_vol_dir.return_value = fake_vol_dir + + active_img_path = self._driver._local_path_active_image( + mock.sentinel.volume) + exp_act_img_path = os.path.join(fake_vol_dir, fake_active_img) + + self.assertEqual(exp_act_img_path, active_img_path) + mock_get_active_img.assert_called_once_with(mock.sentinel.volume) + mock_local_vol_dir.assert_called_once_with(mock.sentinel.volume) + + @ddt.data({}, + {'provider_location': None}, + {'active_fpath': 'last_snap_img', + 'expect_snaps': True}) + @ddt.unpack + @mock.patch.object(remotefs.RemoteFSSnapDriver, + '_local_path_active_image') + @mock.patch.object(remotefs.RemoteFSSnapDriver, + 'local_path') + def test_snapshots_exist(self, mock_local_path, + mock_local_path_active_img, + provider_location='fake_share', + active_fpath='base_img_path', + base_vol_path='base_img_path', + expect_snaps=False): + self._fake_volume.provider_location = provider_location + + mock_local_path.return_value = base_vol_path + mock_local_path_active_img.return_value = active_fpath + + snaps_exist = self._driver._snapshots_exist(self._fake_volume) + + self.assertEqual(expect_snaps, snaps_exist) + + if provider_location: + mock_local_path.assert_called_once_with(self._fake_volume) + mock_local_path_active_img.assert_called_once_with( + self._fake_volume) + else: + self.assertFalse(mock_local_path.called) + + @ddt.data({}, + {'snapshots_exist': True}, + {'force_temp_snap': True}) + @ddt.unpack + @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, @@ -469,7 +526,13 @@ class RemoteFsSnapDriverTestCase(test.TestCase): def test_create_cloned_volume(self, mock_copy_volume_from_snapshot, mock_delete_snapshot, mock_create_snapshot, - mock_validate_state): + mock_validate_state, + mock_extend_volme, + mock_copy_volume_image, + mock_snapshots_exist, + mock_local_path, + snapshots_exist=False, + force_temp_snap=False): drv = self._driver volume = fake_volume.fake_volume_obj(self.context) @@ -479,6 +542,9 @@ class RemoteFsSnapDriverTestCase(test.TestCase): id=src_vref_id, name='volume-%s' % src_vref_id) + mock_snapshots_exist.return_value = snapshots_exist + drv._always_use_temp_snap_when_cloning = force_temp_snap + vol_attrs = ['provider_location', 'size', 'id', 'name', 'status', 'volume_type', 'metadata'] Volume = collections.namedtuple('Volume', vol_attrs) @@ -509,10 +575,33 @@ class RemoteFsSnapDriverTestCase(test.TestCase): src_vref.status, exp_acceptable_states, obj_description='source volume') - mock_create_snapshot.assert_called_once_with(snap_ref) - mock_copy_volume_from_snapshot.assert_called_once_with( - snap_ref, volume_ref, volume['size']) - self.assertTrue(mock_delete_snapshot.called) + + if snapshots_exist or force_temp_snap: + mock_create_snapshot.assert_called_once_with(snap_ref) + mock_copy_volume_from_snapshot.assert_called_once_with( + snap_ref, volume_ref, volume['size']) + self.assertTrue(mock_delete_snapshot.called) + else: + 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_volme.assert_called_once_with(volume_ref, + volume.size) + + @mock.patch('shutil.copyfile') + @mock.patch.object(remotefs.RemoteFSSnapDriver, '_set_rw_permissions') + def test_copy_volume_image(self, mock_set_perm, mock_copyfile): + self._driver._copy_volume_image(mock.sentinel.src, mock.sentinel.dest) + + mock_copyfile.assert_called_once_with(mock.sentinel.src, + mock.sentinel.dest) + mock_set_perm.assert_called_once_with(mock.sentinel.dest) def test_create_regular_file(self): self._driver._create_regular_file('/path', 1) diff --git a/cinder/tests/unit/windows/test_smbfs.py b/cinder/tests/unit/windows/test_smbfs.py index 4a49e92a285..c192d112e37 100644 --- a/cinder/tests/unit/windows/test_smbfs.py +++ b/cinder/tests/unit/windows/test_smbfs.py @@ -709,6 +709,12 @@ class WindowsSmbFsTestCase(test.TestCase): drv._vhdutils.reconnect_parent_vhd.assert_called_once_with( self._FAKE_SNAPSHOT_PATH, self._FAKE_VOLUME_PATH) + def test_copy_volume_image(self): + self._smbfs_driver._copy_volume_image(mock.sentinel.src, + mock.sentinel.dest) + self._smbfs_driver._pathutils.copy.assert_called_once_with( + mock.sentinel.src, mock.sentinel.dest) + def test_get_pool_name_from_share(self): self._smbfs_driver._pool_mappings = { mock.sentinel.share: mock.sentinel.pool} diff --git a/cinder/volume/drivers/remotefs.py b/cinder/volume/drivers/remotefs.py index d771f50b464..0052c16ce5f 100644 --- a/cinder/volume/drivers/remotefs.py +++ b/cinder/volume/drivers/remotefs.py @@ -20,6 +20,7 @@ import inspect import json import os import re +import shutil import tempfile import time @@ -671,6 +672,11 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): """ _VALID_IMAGE_EXTENSIONS = [] + # The following flag may be overriden by the concrete drivers in order + # to avoid using temporary volume snapshots when creating volume clones, + # when possible. + + _always_use_temp_snap_when_cloning = True def __init__(self, *args, **kwargs): self._remotefsclient = None @@ -955,6 +961,22 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): return snap_info['active'] + def _local_path_active_image(self, volume): + active_fname = self.get_active_image_from_info(volume) + vol_dir = self._local_volume_dir(volume) + + active_fpath = os.path.join(vol_dir, active_fname) + return active_fpath + + def _snapshots_exist(self, volume): + if not volume.provider_location: + return False + + active_fpath = self._local_path_active_image(volume) + base_vol_path = self.local_path(volume) + + return not utils.paths_normcase_equal(active_fpath, base_vol_path) + def _create_cloned_volume(self, volume, src_vref): LOG.info('Cloning volume %(src)s to volume %(dst)s', {'src': src_vref.id, @@ -972,10 +994,6 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): 'volume_type', 'metadata'] Volume = collections.namedtuple('Volume', vol_attrs) - snap_attrs = ['volume_name', 'volume_size', 'name', - 'volume_id', 'id', 'volume'] - Snapshot = collections.namedtuple('Snapshot', snap_attrs) - volume_info = Volume(provider_location=src_vref.provider_location, size=src_vref.size, id=volume.id, @@ -984,24 +1002,38 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): volume_type=src_vref.volume_type, metadata=src_vref.metadata) - temp_snapshot = Snapshot(volume_name=volume_name, - volume_size=src_vref.size, - name='clone-snap-%s' % src_vref.id, - volume_id=src_vref.id, - id='tmp-snap-%s' % src_vref.id, - volume=src_vref) + if (self._always_use_temp_snap_when_cloning or + self._snapshots_exist(src_vref)): + snap_attrs = ['volume_name', 'volume_size', 'name', + 'volume_id', 'id', 'volume'] + Snapshot = collections.namedtuple('Snapshot', snap_attrs) - self._create_snapshot(temp_snapshot) - try: - self._copy_volume_from_snapshot(temp_snapshot, - volume_info, - volume.size) + temp_snapshot = Snapshot(volume_name=volume_name, + volume_size=src_vref.size, + name='clone-snap-%s' % src_vref.id, + volume_id=src_vref.id, + id='tmp-snap-%s' % src_vref.id, + volume=src_vref) - finally: - self._delete_snapshot(temp_snapshot) + self._create_snapshot(temp_snapshot) + try: + self._copy_volume_from_snapshot(temp_snapshot, + volume_info, + volume.size) + + finally: + self._delete_snapshot(temp_snapshot) + else: + self._copy_volume_image(self.local_path(src_vref), + self.local_path(volume_info)) + self._extend_volume(volume_info, volume.size) return {'provider_location': src_vref.provider_location} + def _copy_volume_image(self, src_path, dest_path): + shutil.copyfile(src_path, dest_path) + self._set_rw_permissions(dest_path) + def _delete_stale_snapshot(self, snapshot): info_path = self._local_path_volume_info(snapshot.volume) snap_info = self._read_info_file(info_path) @@ -1544,6 +1576,9 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): {'id': snapshot.id} raise exception.RemoteFSException(msg) + def _extend_volume(self, volume, size_gb): + raise NotImplementedError() + class RemoteFSSnapDriver(RemoteFSSnapDriverBase): @locked_volume_id_operation @@ -1575,6 +1610,10 @@ class RemoteFSSnapDriver(RemoteFSSnapDriverBase): return self._copy_volume_to_image(context, volume, image_service, image_meta) + @locked_volume_id_operation + def extend_volume(self, volume, size_gb): + return self._extend_volume(volume, size_gb) + class RemoteFSSnapDriverDistributed(RemoteFSSnapDriverBase): @coordination.synchronized('{self.driver_prefix}-{snapshot.volume.id}') @@ -1606,6 +1645,10 @@ class RemoteFSSnapDriverDistributed(RemoteFSSnapDriverBase): return self._copy_volume_to_image(context, volume, image_service, image_meta) + @coordination.synchronized('{self.driver_prefix}-{volume.id}') + def extend_volume(self, volume, size_gb): + return self._extend_volume(volume, size_gb) + class RemoteFSPoolMixin(object): """Drivers inheriting this will report each share as a pool.""" diff --git a/cinder/volume/drivers/windows/smbfs.py b/cinder/volume/drivers/windows/smbfs.py index 097dae5e2e0..7bdcb0d2fc1 100644 --- a/cinder/volume/drivers/windows/smbfs.py +++ b/cinder/volume/drivers/windows/smbfs.py @@ -105,6 +105,8 @@ class WindowsSmbfsDriver(remotefs_drv.RemoteFSPoolMixin, _SUPPORTED_IMAGE_FORMATS = [_DISK_FORMAT_VHD, _DISK_FORMAT_VHDX] _VALID_IMAGE_EXTENSIONS = _SUPPORTED_IMAGE_FORMATS + _always_use_temp_snap_when_cloning = False + def __init__(self, *args, **kwargs): self._remotefsclient = None super(WindowsSmbfsDriver, self).__init__(*args, **kwargs) @@ -396,14 +398,9 @@ class WindowsSmbfsDriver(remotefs_drv.RemoteFSPoolMixin, self._vhdutils.create_differencing_vhd(new_snap_path, backing_file_full_path) - @coordination.synchronized('{self.driver_prefix}-{volume.id}') - def extend_volume(self, volume, size_gb): - LOG.info('Extending volume %s.', volume.id) - - self._check_extend_volume_support(volume, size_gb) - self._extend_volume(volume, size_gb) - def _extend_volume(self, volume, size_gb): + self._check_extend_volume_support(volume, size_gb) + volume_path = self.local_path(volume) LOG.info('Resizing file %(volume_path)s to %(size_gb)sGB.', @@ -542,6 +539,9 @@ class WindowsSmbfsDriver(remotefs_drv.RemoteFSPoolMixin, self._vhdutils.resize_vhd(volume_path, volume_size * units.Gi, is_file_max_size=False) + def _copy_volume_image(self, src_path, dest_path): + self._pathutils.copy(src_path, dest_path) + def _get_share_name(self, share): return share.replace('/', '\\').lstrip('\\').split('\\', 1)[1]