From 69e687d38a662a1352e259be93765de0d20d19dc Mon Sep 17 00:00:00 2001 From: Eric Harney Date: Mon, 16 Dec 2013 17:44:02 -0500 Subject: [PATCH] GlusterFS: Fix deadlock in volume clone The create_cloned_volume path could deadlock due to create_cloned_volume and create/delete_snapshot using the same lock for synchronization. Refactor the calls to create/delete snapshot to call the inner method which does not use a lock. Introduced by "06999f6 GlusterFS: Synchronize additional op..." Related-Bug: 1267983 Closes-Bug: 1272092 (cherry picked from commit e9722f09dfd4b58e40afbf285ab9a7738a641dca) Note: Effectively includes most changes from "8772714 Pylint fixes ..." to resolve conflicts. (cherry picked from commit 8772714b4fcb2d05ae7f0bfe5cfd0fa660ab9100) Conflicts: cinder/volume/drivers/glusterfs.py Change-Id: I84ca34b201c10644faa047f1c9274c14bcdd0359 --- cinder/tests/test_glusterfs.py | 16 ++++++++-------- cinder/volume/drivers/glusterfs.py | 19 ++++++++++++++----- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/cinder/tests/test_glusterfs.py b/cinder/tests/test_glusterfs.py index a99b0316b59..fa6dee037bf 100644 --- a/cinder/tests/test_glusterfs.py +++ b/cinder/tests/test_glusterfs.py @@ -641,8 +641,8 @@ class GlusterFsDriverTestCase(test.TestCase): def test_create_cloned_volume(self): (mox, drv) = self._mox, self._driver - mox.StubOutWithMock(drv, 'create_snapshot') - mox.StubOutWithMock(drv, 'delete_snapshot') + mox.StubOutWithMock(drv, '_create_snapshot') + mox.StubOutWithMock(drv, '_delete_snapshot') mox.StubOutWithMock(drv, '_read_info_file') mox.StubOutWithMock(image_utils, 'convert_image') mox.StubOutWithMock(drv, '_copy_volume_from_snapshot') @@ -675,7 +675,7 @@ class GlusterFsDriverTestCase(test.TestCase): 'id': 'tmp-snap-%s' % src_vref['id'], 'volume': src_vref} - drv.create_snapshot(snap_ref) + drv._create_snapshot(snap_ref) snap_info = {'active': volume_file, snap_ref['id']: volume_path + '-clone'} @@ -684,7 +684,7 @@ class GlusterFsDriverTestCase(test.TestCase): drv._copy_volume_from_snapshot(snap_ref, volume_ref, volume['size']) - drv.delete_snapshot(mox_lib.IgnoreArg()) + drv._delete_snapshot(mox_lib.IgnoreArg()) mox.ReplayAll() @@ -1632,9 +1632,9 @@ class GlusterFsDriverTestCase(test.TestCase): volume = self._simple_volume('c1073000-0000-0000-0000-0000000c1073') src_volume = self._simple_volume() - mox.StubOutWithMock(drv, 'create_snapshot') + mox.StubOutWithMock(drv, '_create_snapshot') mox.StubOutWithMock(drv, '_copy_volume_from_snapshot') - mox.StubOutWithMock(drv, 'delete_snapshot') + mox.StubOutWithMock(drv, '_delete_snapshot') snap_ref = {'volume_name': src_volume['name'], 'name': 'clone-snap-%s' % src_volume['id'], @@ -1650,11 +1650,11 @@ class GlusterFsDriverTestCase(test.TestCase): 'provider_location': volume['provider_location'], 'name': 'volume-' + volume['id']} - drv.create_snapshot(snap_ref) + drv._create_snapshot(snap_ref) drv._copy_volume_from_snapshot(snap_ref, volume_ref, src_volume['size']) - drv.delete_snapshot(snap_ref) + drv._delete_snapshot(snap_ref) mox.ReplayAll() diff --git a/cinder/volume/drivers/glusterfs.py b/cinder/volume/drivers/glusterfs.py index e1a110ed6f4..fe5e42bf0a8 100644 --- a/cinder/volume/drivers/glusterfs.py +++ b/cinder/volume/drivers/glusterfs.py @@ -186,14 +186,14 @@ class GlusterfsDriver(nfs.RemoteFsDriver): 'volume_id': src_vref['id'], 'id': 'tmp-snap-%s' % src_vref['id'], 'volume': src_vref} - self.create_snapshot(temp_snapshot) + self._create_snapshot(temp_snapshot) try: self._copy_volume_from_snapshot(temp_snapshot, volume_info, src_vref['size']) finally: - self.delete_snapshot(temp_snapshot) + self._delete_snapshot(temp_snapshot) return {'provider_location': src_vref['provider_location']} @@ -211,6 +211,7 @@ class GlusterfsDriver(nfs.RemoteFsDriver): return {'provider_location': volume['provider_location']} + @utils.synchronized('glusterfs', external=False) def create_volume_from_snapshot(self, volume, snapshot): """Creates a volume from a snapshot. @@ -287,6 +288,11 @@ class GlusterfsDriver(nfs.RemoteFsDriver): @utils.synchronized('glusterfs', external=False) def create_snapshot(self, snapshot): + """Apply locking to the create snapshot operation.""" + + return self._create_snapshot(snapshot) + + def _create_snapshot(self, snapshot): """Create a snapshot. If volume is attached, call to Nova to create snapshot, @@ -455,8 +461,7 @@ class GlusterfsDriver(nfs.RemoteFsDriver): LOG.debug(_('volume id: %s') % snapshot['volume_id']) path_to_disk = self._local_path_volume(snapshot['volume']) - snap_id = snapshot['id'] - self._create_snapshot(snapshot, path_to_disk, snap_id) + self._create_snapshot_offline(snapshot, path_to_disk) def _create_qcow2_snap_file(self, snapshot, backing_filename, new_snap_path): @@ -485,7 +490,7 @@ class GlusterfsDriver(nfs.RemoteFsDriver): new_snap_path] self._execute(*command, run_as_root=True) - def _create_snapshot(self, snapshot, path_to_disk, snap_id): + def _create_snapshot_offline(self, snapshot, path_to_disk): """Create snapshot (offline case).""" # Requires volume status = 'available' @@ -540,6 +545,10 @@ class GlusterfsDriver(nfs.RemoteFsDriver): @utils.synchronized('glusterfs', external=False) def delete_snapshot(self, snapshot): + """Apply locking to the delete snapshot operation.""" + self._delete_snapshot(snapshot) + + def _delete_snapshot(self, snapshot): """Delete a snapshot. If volume status is 'available', delete snapshot here in Cinder