From be76d9e404c0f8a9728bccb7b54dffae091d0700 Mon Sep 17 00:00:00 2001 From: Csaba Henk Date: Tue, 29 Sep 2015 09:49:19 +0200 Subject: [PATCH] glusterfs vol layout: start volume cloned from snapshot When handling create_share_from_snapshot with glusterfs volume layout, we do a snapshot clone gluster operation that gives us a new volume (which will be used to back the new share). 'snapshot clone' does not start the resultant volume, we have explicitly start it from Manila. So far the volume layout code did not bother about it, rather the 'vol start' was called from glusterfs-native driver. That however broke all other volume layout based configs (ie. glusterfs driver with vol layout). Fix this now by doing the 'vol start' call in the vol layout code. Change-Id: I63c13ce468a3227f09e381814f55e8c914fbef95 Closes-Bug: #1499347 (cherry picked from commit 4e4c8759a25fa18384b45f4150000aacfaed035a) --- .../share/drivers/glusterfs/layout_volume.py | 11 +++ manila/share/drivers/glusterfs_native.py | 11 ++- .../drivers/glusterfs/test_layout_volume.py | 77 ++++++++++++++++--- .../share/drivers/test_glusterfs_native.py | 19 +++-- 4 files changed, 93 insertions(+), 25 deletions(-) diff --git a/manila/share/drivers/glusterfs/layout_volume.py b/manila/share/drivers/glusterfs/layout_volume.py index ec494f2ec1..0f0e7f6f9a 100644 --- a/manila/share/drivers/glusterfs/layout_volume.py +++ b/manila/share/drivers/glusterfs/layout_volume.py @@ -485,6 +485,17 @@ class GlusterfsVolumeMappedLayout(layout.GlusterfsShareLayoutBase): export = self.driver._setup_via_manager( {'share': share, 'manager': gmgr}, {'share': snapshot['share_instance'], 'manager': old_gmgr}) + + try: + gmgr.gluster_call( + 'volume', 'start', gmgr.volume) + except exception.ProcessExecutionError as exc: + msg = (_("Error starting gluster volume. " + "Volume: %(volname)s, Error: %(error)s") % + {'volname': gmgr.volume, 'error': exc.stderr}) + LOG.error(msg) + raise exception.GlusterfsException(msg) + self.gluster_used_vols.add(gmgr.qualified) self.private_storage.update(share['id'], {'volume': gmgr.qualified}) return export diff --git a/manila/share/drivers/glusterfs_native.py b/manila/share/drivers/glusterfs_native.py index bf1148358b..a0a6ebf476 100644 --- a/manila/share/drivers/glusterfs_native.py +++ b/manila/share/drivers/glusterfs_native.py @@ -126,12 +126,6 @@ class GlusterfsNativeShareDriver(driver.ExecuteMixin, ): gluster_actions.append(('set', option, value)) - if not gluster_mgr_parent: - # TODO(deepakcs) Remove this once ssl options can be - # set dynamically. - gluster_actions.append(('stop', '--mode=script')) - gluster_actions.append(('start',)) - for action in gluster_actions: try: gluster_mgr.gluster_call( @@ -145,6 +139,11 @@ class GlusterfsNativeShareDriver(driver.ExecuteMixin, LOG.error(msg) raise exception.GlusterfsException(msg) + if not gluster_mgr_parent: + # TODO(deepakcs) Remove this once ssl options can be + # set dynamically. + common._restart_gluster_vol(gluster_mgr) + return gluster_mgr.export @utils.synchronized("glusterfs_native_access", external=False) diff --git a/manila/tests/share/drivers/glusterfs/test_layout_volume.py b/manila/tests/share/drivers/glusterfs/test_layout_volume.py index f83b30764f..c63156077d 100644 --- a/manila/tests/share/drivers/glusterfs/test_layout_volume.py +++ b/manila/tests/share/drivers/glusterfs/test_layout_volume.py @@ -715,7 +715,7 @@ class GlusterfsVolumeMappedLayoutTestCase(test.TestCase): self.mock_object(old_gmgr, 'gluster_call', mock.Mock(side_effect=[('', ''), ('', '')])) self.mock_object(new_gmgr, 'gluster_call', - mock.Mock(side_effect=[('', ''), ('', '')])) + mock.Mock(side_effect=[('', '')])) self.mock_object(new_gmgr, 'get_gluster_vol_option', mock.Mock()) new_gmgr.get_gluster_vol_option.return_value = ( @@ -738,6 +738,8 @@ class GlusterfsVolumeMappedLayoutTestCase(test.TestCase): 'force', '--mode=script'), ('snapshot', 'clone', volume, 'fake_snap_id_xyz')) old_gmgr.gluster_call.assert_has_calls([mock.call(*a) for a in args]) + args = (('volume', 'start', volume),) + new_gmgr.gluster_call.assert_has_calls([mock.call(*a) for a in args]) self._layout._share_manager.assert_called_once_with( snapshot['share_instance']) self._layout._glustermanager.assert_called_once_with( @@ -752,7 +754,69 @@ class GlusterfsVolumeMappedLayoutTestCase(test.TestCase): self._layout.gluster_used_vols) self.assertEqual('host1:/gv1', ret) - def test_create_share_from_snapshot_error_old_gmr_gluster_calls(self): + @ddt.data({'trouble': exception.ProcessExecutionError, + '_exception': exception.GlusterfsException}, + {'trouble': RuntimeError, '_exception': RuntimeError}) + @ddt.unpack + def test_create_share_from_snapshot_error_new_gmr_gluster_calls( + self, trouble, _exception): + glusterfs_target = 'root@host1:/gv1' + glusterfs_server = 'root@host1' + share = new_share() + volume = ''.join(['manila-', share['id']]) + new_vol_addr = ':/'.join([glusterfs_server, volume]) + gmgr = common.GlusterManager + old_gmgr = gmgr(glusterfs_target, self._execute, None, None) + new_gmgr = gmgr(new_vol_addr, self._execute, None, None) + self._layout.gluster_used_vols = set([glusterfs_target]) + self._layout.glusterfs_versions = {glusterfs_server: ('3', '7')} + self.mock_object(old_gmgr, 'gluster_call', + mock.Mock(side_effect=[('', ''), ('', '')])) + self.mock_object(new_gmgr, 'gluster_call', + mock.Mock(side_effect=trouble)) + self.mock_object(new_gmgr, 'get_gluster_vol_option', + mock.Mock()) + new_gmgr.get_gluster_vol_option.return_value = ( + 'glusterfs-server-1,client') + self.mock_object(self._layout, '_find_actual_backend_snapshot_name', + mock.Mock(return_value='fake_snap_id_xyz')) + self.mock_object(self._layout, '_share_manager', + mock.Mock(return_value=old_gmgr)) + self.mock_object(self._layout, '_glustermanager', + mock.Mock(return_value=new_gmgr)) + self.mock_object(self.fake_driver, '_setup_via_manager', + mock.Mock(return_value='host1:/gv1')) + + snapshot = { + 'id': 'fake_snap_id', + 'share_instance': new_share(export_location=glusterfs_target) + } + self.assertRaises(_exception, + self._layout.create_share_from_snapshot, + self._context, share, snapshot) + + (self._layout._find_actual_backend_snapshot_name. + assert_called_once_with(old_gmgr, snapshot)) + args = (('snapshot', 'activate', 'fake_snap_id_xyz', + 'force', '--mode=script'), + ('snapshot', 'clone', volume, 'fake_snap_id_xyz')) + old_gmgr.gluster_call.assert_has_calls([mock.call(*a) for a in args]) + args = (('volume', 'start', volume),) + new_gmgr.gluster_call.assert_has_calls([mock.call(*a) for a in args]) + self._layout._share_manager.assert_called_once_with( + snapshot['share_instance']) + self._layout._glustermanager.assert_called_once_with( + gmgr.parse(new_vol_addr)) + self._layout.driver._setup_via_manager.assert_called_once_with( + {'manager': new_gmgr, 'share': share}, + {'manager': old_gmgr, 'share': snapshot['share_instance']}) + + @ddt.data({'trouble': exception.ProcessExecutionError, + '_exception': exception.GlusterfsException}, + {'trouble': RuntimeError, '_exception': RuntimeError}) + @ddt.unpack + def test_create_share_from_snapshot_error_old_gmr_gluster_calls( + self, trouble, _exception): glusterfs_target = 'root@host1:/gv1' glusterfs_server = 'root@host1' share = new_share() @@ -765,9 +829,7 @@ class GlusterfsVolumeMappedLayoutTestCase(test.TestCase): self._layout.glusterfs_versions = {glusterfs_server: ('3', '7')} self.mock_object( old_gmgr, 'gluster_call', - mock.Mock(side_effect=[('', ''), exception.ProcessExecutionError])) - self.mock_object(new_gmgr, 'gluster_call', - mock.Mock(side_effect=[('', ''), ('', '')])) + mock.Mock(side_effect=[('', ''), trouble])) self.mock_object(new_gmgr, 'get_gluster_vol_option', mock.Mock()) new_gmgr.get_gluster_vol_option.return_value = ( @@ -783,7 +845,7 @@ class GlusterfsVolumeMappedLayoutTestCase(test.TestCase): 'id': 'fake_snap_id', 'share_instance': new_share(export_location=glusterfs_target) } - self.assertRaises(exception.GlusterfsException, + self.assertRaises(_exception, self._layout.create_share_from_snapshot, self._context, share, snapshot) @@ -815,9 +877,6 @@ class GlusterfsVolumeMappedLayoutTestCase(test.TestCase): self.mock_object( old_gmgr, 'gluster_call', mock.Mock(side_effect=[('', ''), ('', '')])) - self.mock_object( - new_gmgr, 'gluster_call', - mock.Mock(side_effect=[('', ''), exception.ProcessExecutionError])) self.mock_object(new_gmgr, 'get_gluster_vol_option', mock.Mock()) new_gmgr.get_gluster_vol_option.return_value = ( diff --git a/manila/tests/share/drivers/test_glusterfs_native.py b/manila/tests/share/drivers/test_glusterfs_native.py index f70b8e4fb2..74b62fd309 100644 --- a/manila/tests/share/drivers/test_glusterfs_native.py +++ b/manila/tests/share/drivers/test_glusterfs_native.py @@ -143,8 +143,7 @@ class GlusterfsNativeShareDriverTestCase(test.TestCase): 'glusterfs-server-name,manila-host.com'), ('volume', 'set', 'fakevol', 'nfs.export-volumes', 'off'), ('volume', 'set', 'fakevol', 'client.ssl', 'on'), - ('volume', 'set', 'fakevol', 'server.ssl', 'on'), - ('volume', 'start', 'fakevol')) + ('volume', 'set', 'fakevol', 'server.ssl', 'on')) gmgr.gluster_call.assert_has_calls([mock.call(*a) for a in args]) self.assertEqual(ret, gmgr.export) @@ -187,7 +186,7 @@ class GlusterfsNativeShareDriverTestCase(test.TestCase): self.assertTrue(self._driver.snapshots_are_supported) def test_allow_access_via_manager(self): - common._restart_gluster_vol = mock.Mock() + self.mock_object(common, '_restart_gluster_vol', mock.Mock()) access = {'access_type': 'cert', 'access_to': 'client.example.com'} gmgr1 = common.GlusterManager(self.glusterfs_target1, self._execute, None, None) @@ -205,7 +204,7 @@ class GlusterfsNativeShareDriverTestCase(test.TestCase): common._restart_gluster_vol.assert_called_once_with(gmgr1) def test_allow_access_via_manager_with_share_having_access(self): - common._restart_gluster_vol = mock.Mock() + self.mock_object(common, '_restart_gluster_vol', mock.Mock()) access = {'access_type': 'cert', 'access_to': 'client.example.com'} gmgr1 = common.GlusterManager(self.glusterfs_target1, self._execute, None, None) @@ -222,7 +221,7 @@ class GlusterfsNativeShareDriverTestCase(test.TestCase): self.assertFalse(common._restart_gluster_vol.called) def test_allow_access_via_manager_invalid_access_type(self): - common._restart_gluster_vol = mock.Mock() + self.mock_object(common, '_restart_gluster_vol', mock.Mock()) access = {'access_type': 'invalid', 'access_to': 'client.example.com'} expected_exec = [] @@ -242,7 +241,7 @@ class GlusterfsNativeShareDriverTestCase(test.TestCase): if (args == test_args): raise exception.ProcessExecutionError() - common._restart_gluster_vol = mock.Mock() + self.mock_object(common, '_restart_gluster_vol', mock.Mock()) gmgr1 = common.GlusterManager(self.glusterfs_target1, self._execute, None, None) self.mock_object(gmgr1, 'get_gluster_vol_option', @@ -260,7 +259,7 @@ class GlusterfsNativeShareDriverTestCase(test.TestCase): self.assertFalse(common._restart_gluster_vol.called) def test_deny_access_via_manager(self): - common._restart_gluster_vol = mock.Mock() + self.mock_object(common, '_restart_gluster_vol', mock.Mock()) access = {'access_type': 'cert', 'access_to': 'client.example.com'} gmgr1 = common.GlusterManager(self.glusterfs_target1, self._execute, None, None) @@ -279,7 +278,7 @@ class GlusterfsNativeShareDriverTestCase(test.TestCase): common._restart_gluster_vol.assert_called_once_with(gmgr1) def test_deny_access_via_manager_with_share_having_no_access(self): - common._restart_gluster_vol = mock.Mock() + self.mock_object(common, '_restart_gluster_vol', mock.Mock()) access = {'access_type': 'cert', 'access_to': 'client.example.com'} gmgr1 = common.GlusterManager(self.glusterfs_target1, self._execute, None, None) @@ -295,7 +294,7 @@ class GlusterfsNativeShareDriverTestCase(test.TestCase): self.assertFalse(common._restart_gluster_vol.called) def test_deny_access_via_manager_invalid_access_type(self): - common._restart_gluster_vol = mock.Mock() + self.mock_object(common, '_restart_gluster_vol', mock.Mock()) access = {'access_type': 'invalid', 'access_to': 'NotApplicable'} self.assertRaises(exception.InvalidShareAccess, @@ -313,7 +312,7 @@ class GlusterfsNativeShareDriverTestCase(test.TestCase): if (args == test_args): raise exception.ProcessExecutionError() - common._restart_gluster_vol = mock.Mock() + self.mock_object(common, '_restart_gluster_vol', mock.Mock()) gmgr1 = common.GlusterManager(self.glusterfs_target1, self._execute, None, None) self.mock_object(