From acf19befe59614d15b89ea4be1217de2d1712f3b Mon Sep 17 00:00:00 2001 From: Alessandro Pilotti Date: Wed, 1 Apr 2015 20:57:14 +0300 Subject: [PATCH] SMBFS: Lock on a per-volume basis This allows operations that do not conflict with each other (i.e. are on different volumes) to run concurrently. The prior locking scheme was too coarse and essentially made the driver single-threaded. This patch moves the implementation of the GlusterFS driver locking scheme to the RemoteFS base driver so that other similar volume drivers can use it. Closes-Bug: #1439352 Change-Id: I37be14e7525406e0ad568f6228c1303998c5235f --- cinder/tests/test_remotefs.py | 59 ++++++++++++++++++- cinder/volume/drivers/glusterfs.py | 92 ++---------------------------- cinder/volume/drivers/remotefs.py | 66 ++++++++++++++++++++- cinder/volume/drivers/smbfs.py | 30 +--------- 4 files changed, 129 insertions(+), 118 deletions(-) diff --git a/cinder/tests/test_remotefs.py b/cinder/tests/test_remotefs.py index 3dc31903a..a17723d2d 100644 --- a/cinder/tests/test_remotefs.py +++ b/cinder/tests/test_remotefs.py @@ -19,14 +19,16 @@ import mock from cinder import exception from cinder import test +from cinder import utils from cinder.volume.drivers import remotefs class RemoteFsSnapDriverTestCase(test.TestCase): _FAKE_CONTEXT = 'fake_context' - _FAKE_VOLUME_NAME = 'volume-4f711859-4928-4cb7-801a-a50c37ceaccc' - _FAKE_VOLUME = {'id': '4f711859-4928-4cb7-801a-a50c37ceaccc', + _FAKE_VOLUME_ID = '4f711859-4928-4cb7-801a-a50c37ceaccc' + _FAKE_VOLUME_NAME = 'volume-%s' % _FAKE_VOLUME_ID + _FAKE_VOLUME = {'id': _FAKE_VOLUME_ID, 'size': 1, 'provider_location': 'fake_share', 'name': _FAKE_VOLUME_NAME, @@ -295,3 +297,56 @@ class RemoteFsSnapDriverTestCase(test.TestCase): self.assertRaises(exception.InvalidVolume, self._driver._create_snapshot, fake_snapshot) + + @mock.patch.object(utils, 'synchronized') + def _locked_volume_operation_test_helper(self, mock_synchronized, func, + expected_exception=False, + *args, **kwargs): + def mock_decorator(*args, **kwargs): + def mock_inner(f): + return f + return mock_inner + + mock_synchronized.side_effect = mock_decorator + expected_lock = '%s-%s' % (self._driver.driver_prefix, + self._FAKE_VOLUME_ID) + + if expected_exception: + self.assertRaises(expected_exception, func, + self._driver, + *args, **kwargs) + else: + ret_val = func(self._driver, *args, **kwargs) + + mock_synchronized.assert_called_with(expected_lock, + external=False) + self.assertEqual(mock.sentinel.ret_val, ret_val) + + def test_locked_volume_id_operation(self): + mock_volume = {'id': self._FAKE_VOLUME_ID} + + @remotefs.locked_volume_id_operation + def synchronized_func(inst, volume): + return mock.sentinel.ret_val + + self._locked_volume_operation_test_helper(func=synchronized_func, + volume=mock_volume) + + def test_locked_volume_id_snapshot_operation(self): + mock_snapshot = {'volume': {'id': self._FAKE_VOLUME_ID}} + + @remotefs.locked_volume_id_operation + def synchronized_func(inst, snapshot): + return mock.sentinel.ret_val + + self._locked_volume_operation_test_helper(func=synchronized_func, + snapshot=mock_snapshot) + + def test_locked_volume_id_operation_exception(self): + @remotefs.locked_volume_id_operation + def synchronized_func(inst): + return mock.sentinel.ret_val + + self._locked_volume_operation_test_helper( + func=synchronized_func, + expected_exception=exception.VolumeBackendAPIException) diff --git a/cinder/volume/drivers/glusterfs.py b/cinder/volume/drivers/glusterfs.py index 01853ac54..325c7d221 100644 --- a/cinder/volume/drivers/glusterfs.py +++ b/cinder/volume/drivers/glusterfs.py @@ -52,48 +52,6 @@ volume_opts = [ CONF = cfg.CONF CONF.register_opts(volume_opts) -lock_tag = 'glusterfs' - - -def locked_volume_id_operation(f, external=False): - """Lock decorator for volume operations. - - Takes a named lock prior to executing the operation. The lock is named - with the id of the volume. This lock can then be used - by other operations to avoid operation conflicts on shared volumes. - - May be applied to methods of signature: - method(, volume, *, **) - """ - - def lvo_inner1(inst, volume, *args, **kwargs): - @utils.synchronized('%s-%s' % (lock_tag, volume['id']), - external=external) - def lvo_inner2(*_args, **_kwargs): - return f(*_args, **_kwargs) - return lvo_inner2(inst, volume, *args, **kwargs) - return lvo_inner1 - - -def locked_volume_id_snapshot_operation(f, external=False): - """Lock decorator for volume operations that use snapshot objects. - - Takes a named lock prior to executing the operation. The lock is named - with the id of the volume. This lock can then be used - by other operations to avoid operation conflicts on shared volumes. - - May be applied to methods of signature: - method(, snapshot, *, **) - """ - - def lso_inner1(inst, snapshot, *args, **kwargs): - @utils.synchronized('%s-%s' % (lock_tag, snapshot['volume']['id']), - external=external) - def lso_inner2(*_args, **_kwargs): - return f(*_args, **_kwargs) - return lso_inner2(inst, snapshot, *args, **kwargs) - return lso_inner1 - class GlusterfsDriver(remotefs_drv.RemoteFSSnapDriver): """Gluster based cinder driver. Creates file on Gluster share for using it @@ -202,12 +160,7 @@ class GlusterfsDriver(remotefs_drv.RemoteFSSnapDriver): hashed) return path - @locked_volume_id_operation - def create_cloned_volume(self, volume, src_vref): - """Creates a clone of the specified volume.""" - self._create_cloned_volume(volume, src_vref) - - @locked_volume_id_operation + @remotefs_drv.locked_volume_id_operation def create_volume(self, volume): """Creates a volume.""" @@ -221,10 +174,6 @@ class GlusterfsDriver(remotefs_drv.RemoteFSSnapDriver): return {'provider_location': volume['provider_location']} - @locked_volume_id_operation - def create_volume_from_snapshot(self, volume, snapshot): - return self._create_volume_from_snapshot(volume, snapshot) - def _copy_volume_from_snapshot(self, snapshot, volume, volume_size): """Copy data from snapshot to destination volume. @@ -265,7 +214,7 @@ class GlusterfsDriver(remotefs_drv.RemoteFSSnapDriver): self._set_rw_permissions_for_all(path_to_new_vol) - @locked_volume_id_operation + @remotefs_drv.locked_volume_id_operation def delete_volume(self, volume): """Deletes a logical volume.""" @@ -291,21 +240,10 @@ class GlusterfsDriver(remotefs_drv.RemoteFSSnapDriver): info_path = self._local_path_volume_info(volume) fileutils.delete_if_exists(info_path) - @locked_volume_id_snapshot_operation - def create_snapshot(self, snapshot): - """Apply locking to the create snapshot operation.""" - - return self._create_snapshot(snapshot) - def _get_matching_backing_file(self, backing_chain, snapshot_file): return next(f for f in backing_chain if f.get('backing-filename', '') == snapshot_file) - @locked_volume_id_snapshot_operation - def delete_snapshot(self, snapshot): - """Apply locking to the delete snapshot operation.""" - self._delete_snapshot(snapshot) - def ensure_export(self, ctx, volume): """Synchronously recreates an export for a logical volume.""" @@ -323,7 +261,7 @@ class GlusterfsDriver(remotefs_drv.RemoteFSSnapDriver): def validate_connector(self, connector): pass - @locked_volume_id_operation + @remotefs_drv.locked_volume_id_operation def initialize_connection(self, volume, connector): """Allow connection to connector and return connection info.""" @@ -355,29 +293,7 @@ class GlusterfsDriver(remotefs_drv.RemoteFSSnapDriver): """Disallow connection from connector.""" pass - def copy_volume_to_image(self, context, volume, image_service, image_meta): - """Copy the volume to the specified image. - - Warning: parameter order is non-standard to assist with locking - decorators. - """ - - return self._copy_volume_to_image_with_lock(volume, - context, - image_service, - image_meta) - - @locked_volume_id_operation - def _copy_volume_to_image_with_lock(self, volume, context, - image_service, image_meta): - """Call private method for this, which handles per-volume locking.""" - - return self._copy_volume_to_image(context, - volume, - image_service, - image_meta) - - @locked_volume_id_operation + @remotefs_drv.locked_volume_id_operation def extend_volume(self, volume, size_gb): volume_path = self.local_path(volume) volume_filename = os.path.basename(volume_path) diff --git a/cinder/volume/drivers/remotefs.py b/cinder/volume/drivers/remotefs.py index c9655feb7..30e9feddf 100644 --- a/cinder/volume/drivers/remotefs.py +++ b/cinder/volume/drivers/remotefs.py @@ -15,6 +15,7 @@ # under the License. import hashlib +import inspect import json import os import re @@ -29,6 +30,7 @@ from oslo_utils import units from cinder import compute from cinder import db from cinder import exception +from cinder import utils from cinder.i18n import _, _LE, _LI, _LW from cinder.image import image_utils from cinder.volume import driver @@ -87,11 +89,43 @@ CONF = cfg.CONF CONF.register_opts(nas_opts) +def locked_volume_id_operation(f, external=False): + """Lock decorator for volume operations. + + Takes a named lock prior to executing the operation. The lock is named + with the id of the volume. This lock can then be used + by other operations to avoid operation conflicts on shared volumes. + + May be applied to methods of signature: + method(, volume, *, **) + """ + + def lvo_inner1(inst, *args, **kwargs): + lock_tag = inst.driver_prefix + call_args = inspect.getcallargs(f, inst, *args, **kwargs) + + if call_args.get('volume'): + volume_id = call_args['volume']['id'] + elif call_args.get('snapshot'): + volume_id = call_args['snapshot']['volume']['id'] + else: + err_msg = _('The decorated method must accept either a volume or ' + 'a snapshot object') + raise exception.VolumeBackendAPIException(data=err_msg) + + @utils.synchronized('%s-%s' % (lock_tag, volume_id), + external=external) + def lvo_inner2(): + return f(inst, *args, **kwargs) + return lvo_inner2() + return lvo_inner1 + + class RemoteFSDriver(driver.VolumeDriver): """Common base for drivers that work like NFS.""" driver_volume_type = None - driver_prefix = None + driver_prefix = 'remotefs' volume_backend_name = None SHARE_FORMAT_REGEX = r'.+:/.+' @@ -1368,3 +1402,33 @@ class RemoteFSSnapDriver(RemoteFSDriver): path_to_delete = os.path.join( self._local_volume_dir(snapshot['volume']), file_to_delete) self._execute('rm', '-f', path_to_delete, run_as_root=True) + + @locked_volume_id_operation + def create_snapshot(self, snapshot): + """Apply locking to the create snapshot operation.""" + + return self._create_snapshot(snapshot) + + @locked_volume_id_operation + def delete_snapshot(self, snapshot): + """Apply locking to the delete snapshot operation.""" + + return self._delete_snapshot(snapshot) + + @locked_volume_id_operation + def create_volume_from_snapshot(self, volume, snapshot): + return self._create_volume_from_snapshot(volume, snapshot) + + @locked_volume_id_operation + def create_cloned_volume(self, volume, src_vref): + """Creates a clone of the specified volume.""" + return self._create_cloned_volume(volume, src_vref) + + @locked_volume_id_operation + def copy_volume_to_image(self, context, volume, image_service, image_meta): + """Copy the volume to the specified image.""" + + return self._copy_volume_to_image(context, + volume, + image_service, + image_meta) diff --git a/cinder/volume/drivers/smbfs.py b/cinder/volume/drivers/smbfs.py index eb4a54dd9..9191b197f 100644 --- a/cinder/volume/drivers/smbfs.py +++ b/cinder/volume/drivers/smbfs.py @@ -105,6 +105,7 @@ class SmbfsDriver(remotefs_drv.RemoteFSSnapDriver): return super(SmbfsDriver, self)._qemu_img_info_base( path, volume_name, self.configuration.smbfs_mount_point_base) + @remotefs_drv.locked_volume_id_operation def initialize_connection(self, volume, connector): """Allow connection to connector and return connection info. @@ -220,7 +221,7 @@ class SmbfsDriver(remotefs_drv.RemoteFSSnapDriver): return volume_format - @utils.synchronized('smbfs', external=False) + @remotefs_drv.locked_volume_id_operation def delete_volume(self, volume): """Deletes a logical volume.""" if not volume['provider_location']: @@ -387,12 +388,6 @@ class SmbfsDriver(remotefs_drv.RemoteFSSnapDriver): return False return True - @utils.synchronized('smbfs', external=False) - def create_snapshot(self, snapshot): - """Apply locking to the create snapshot operation.""" - - return self._create_snapshot(snapshot) - def _create_snapshot_online(self, snapshot, backing_filename, new_snap_path): msg = _("This driver does not support snapshotting in-use volumes.") @@ -415,13 +410,7 @@ class SmbfsDriver(remotefs_drv.RemoteFSSnapDriver): "format: %s") % volume_format raise exception.InvalidVolume(err_msg) - @utils.synchronized('smbfs', external=False) - def delete_snapshot(self, snapshot): - """Apply locking to the delete snapshot operation.""" - - return self._delete_snapshot(snapshot) - - @utils.synchronized('smbfs', external=False) + @remotefs_drv.locked_volume_id_operation def extend_volume(self, volume, size_gb): LOG.info(_LI('Extending volume %s.'), volume['id']) self._extend_volume(volume, size_gb) @@ -473,14 +462,6 @@ class SmbfsDriver(remotefs_drv.RemoteFSSnapDriver): 'extend volume %s to %sG.' % (volume['id'], size_gb)) - @utils.synchronized('smbfs', external=False) - def copy_volume_to_image(self, context, volume, image_service, image_meta): - self._copy_volume_to_image(context, volume, image_service, image_meta) - - @utils.synchronized('smbfs', external=False) - def create_volume_from_snapshot(self, volume, snapshot): - return self._create_volume_from_snapshot(volume, snapshot) - def _copy_volume_from_snapshot(self, snapshot, volume, volume_size): """Copy data from snapshot to destination volume. @@ -548,11 +529,6 @@ class SmbfsDriver(remotefs_drv.RemoteFSSnapDriver): reason=(_("Expected volume size was %d") % volume['size']) + (_(" but size is now %d.") % virt_size)) - @utils.synchronized('smbfs', external=False) - def create_cloned_volume(self, volume, src_vref): - """Creates a clone of the specified volume.""" - return self._create_cloned_volume(volume, src_vref) - def _ensure_share_mounted(self, smbfs_share): mnt_flags = [] if self.shares.get(smbfs_share) is not None: -- 2.45.2