GlusterFS: Using mount method in RemoteFsClient

This patch modifies the glusterfs driver code to use "mount" method
in RemoteFsClient class.

This patch does the following:
 * Corrects the passing parameters while creating the object of
RemoteFsClient class.
 * Calls the "mount" method in RemoteFsClient to mount GlusterFS
volumes instead of "_do_mount" method in RemoteFSDriver class.
 * Also removes "_do_mount" method RemoteFSDriver class, as it is
not required now.

Closes-Bug: 1433520
Change-Id: I3bfc76fc3228282a741c5265eb06c4974a696b29

diff --git a/cinder/tests/unit/test_glusterfs.py b/cinder/tests/unit/test_glusterfs.py
index b85da12501bf3cf53ed59882771e7dbd5d31c3ee..0863c88e56d1c39732d29c9edd4352f09269f6e5 100644 (file)
--- a/cinder/tests/unit/test_glusterfs.py
+++ b/cinder/tests/unit/test_glusterfs.py
@@ -150,70 +150,24 @@ class GlusterFsDriverTestCase(test.TestCase):
         """_mount_glusterfs common case usage."""
         drv = self._driver
 
-        with mock.patch.object(drv, '_execute') as mock_execute:
-            drv._mount_glusterfs(self.TEST_EXPORT1, self.TEST_MNT_POINT)
-
-            expected = [mock.call('mkdir', '-p', '/mnt/glusterfs'),
-                        mock.call('mount', '-t', 'glusterfs',
-                                  'glusterfs-host1:/export',
-                                  '/mnt/glusterfs', run_as_root=True)]
-            self.assertEqual(expected, mock_execute.mock_calls)
-
-    def test_mount_glusterfs_should_suppress_already_mounted_error(self):
-        """_mount_glusterfs should suppress already mounted error if
-           ensure=True
-        """
-        drv = self._driver
+        with mock.patch.object(brick.remotefs.remotefs.RemoteFsClient,
+                               'mount') as mock_mount:
+            drv._mount_glusterfs(self.TEST_EXPORT1)
 
-        with mock.patch.object(drv, '_execute') as mock_execute:
-            execute_iterable = (None,
-                                putils.ProcessExecutionError(
-                                    stderr='is busy or already mounted'))
-            mock_execute.side_effect = execute_iterable
-            drv._mount_glusterfs(self.TEST_EXPORT1, self.TEST_MNT_POINT,
-                                 ensure=True)
-
-            expected = [mock.call('mkdir', '-p', '/mnt/glusterfs'),
-                        mock.call('mount', '-t', 'glusterfs',
-                                  'glusterfs-host1:/export',
-                                  '/mnt/glusterfs', run_as_root=True)]
-            self.assertEqual(expected, mock_execute.mock_calls)
+            mock_mount.assert_called_once_with(self.TEST_EXPORT1, [])
 
-    def test_mount_glusterfs_should_reraise_already_mounted_error(self):
-        """_mount_glusterfs should not suppress already mounted error
-           if ensure=False
+    def test_mount_glusterfs_should_reraise_exception_on_failure(self):
+        """_mount_glusterfs should reraise exception if mount fails.
         """
         drv = self._driver
 
-        with mock.patch.object(drv, '_execute') as mock_execute:
-            execute_iterable = (None,
-                                putils.ProcessExecutionError(
-                                    stderr='is busy or already mounted'))
-            mock_execute.side_effect = execute_iterable
-
-            self.assertRaises(putils.ProcessExecutionError,
-                              drv._mount_glusterfs, self.TEST_EXPORT1,
-                              self.TEST_MNT_POINT, ensure=False)
-
-            expected = [mock.call('mkdir', '-p', '/mnt/glusterfs'),
-                        mock.call('mount', '-t', 'glusterfs',
-                                  'glusterfs-host1:/export',
-                                  '/mnt/glusterfs', run_as_root=True)]
-            self.assertEqual(expected, mock_execute.mock_calls)
-
-    def test_mount_glusterfs_should_create_mountpoint_if_not_yet(self):
-        """_mount_glusterfs should create mountpoint if it doesn't exist."""
-        drv = self._driver
-
-        with mock.patch.object(drv, '_execute') as mock_execute:
+        with mock.patch.object(brick.remotefs.remotefs.RemoteFsClient,
+                               'mount') as mock_mount:
 
-            drv._mount_glusterfs(self.TEST_EXPORT1, self.TEST_MNT_POINT)
+            mock_mount.side_effect = exception.GlusterfsException()
 
-            expected = [mock.call('mkdir', '-p', '/mnt/glusterfs'),
-                        mock.call('mount', '-t', 'glusterfs',
-                                  'glusterfs-host1:/export',
-                                  '/mnt/glusterfs', run_as_root=True)]
-            self.assertEqual(expected, mock_execute.mock_calls)
+            self.assertRaises(exception.GlusterfsException,
+                              drv._mount_glusterfs, self.TEST_EXPORT1)
 
     def test_get_hash_str(self):
         """_get_hash_str should calculation correct value."""
@@ -395,7 +349,9 @@ class GlusterFsDriverTestCase(test.TestCase):
                 mock.patch.object(utils, 'get_file_mode') as \
                 mock_get_file_mode,\
                 mock.patch.object(tempfile, 'NamedTemporaryFile') as \
-                mock_named_temp:
+                mock_named_temp,\
+                mock.patch.object(brick.remotefs.remotefs.RemoteFsClient,
+                                  'mount') as mock_mount:
             drv._load_shares_config = self._fake_load_shares_config
             mock_named_temp.return_value = self._fake_NamedTemporaryFile
             mock_exists.return_value = True
@@ -410,11 +366,6 @@ class GlusterFsDriverTestCase(test.TestCase):
                 mock.call('umount',
                           '/mnt/test/8f0473c9ad824b8b6a27264b9cacb005',
                           run_as_root=True),
-                mock.call('mkdir', '-p',
-                          '/mnt/test/8f0473c9ad824b8b6a27264b9cacb005'),
-                mock.call('mount', '-t', 'glusterfs', '127.7.7.7:/gluster1',
-                          '/mnt/test/8f0473c9ad824b8b6a27264b9cacb005',
-                          run_as_root=True),
                 mock.call('chgrp', 888,
                           '/mnt/test/8f0473c9ad824b8b6a27264b9cacb005',
                           run_as_root=True),
@@ -422,6 +373,7 @@ class GlusterFsDriverTestCase(test.TestCase):
                           '/mnt/test/8f0473c9ad824b8b6a27264b9cacb005',
                           run_as_root=True)]
             self.assertEqual(expected, mock_execute.mock_calls)
+            mock_mount.assert_called_once_with('127.7.7.7:/gluster1', [])
 
     def test_find_share_should_throw_error_if_there_is_no_mounted_shares(self):
         """_find_share should throw error if there is no mounted shares."""

diff --git a/cinder/volume/drivers/glusterfs.py b/cinder/volume/drivers/glusterfs.py
index 325c7d2214506e3ad666785dd77efd612e03d4b9..5bdc77dbd4afaf6a81be1204cde1b0aaff7dff74 100644 (file)
--- a/cinder/volume/drivers/glusterfs.py
+++ b/cinder/volume/drivers/glusterfs.py
@@ -71,12 +71,12 @@ class GlusterfsDriver(remotefs_drv.RemoteFSSnapDriver):
         self._remotefsclient = None
         super(GlusterfsDriver, self).__init__(*args, **kwargs)
         self.configuration.append_config_values(volume_opts)
+        root_helper = utils.get_root_helper()
         self.base = getattr(self.configuration,
                             'glusterfs_mount_point_base',
                             CONF.glusterfs_mount_point_base)
         self._remotefsclient = remotefs_brick.RemoteFsClient(
-            'glusterfs',
-            execute,
+            'glusterfs', root_helper, execute,
             glusterfs_mount_point_base=self.base)
 
     def set_execute(self, execute):
@@ -362,7 +362,7 @@ class GlusterfsDriver(remotefs_drv.RemoteFSSnapDriver):
         :param glusterfs_share: string
         """
         mount_path = self._get_mount_point_for_share(glusterfs_share)
-        self._mount_glusterfs(glusterfs_share, mount_path, ensure=True)
+        self._mount_glusterfs(glusterfs_share)
 
         # Ensure we can write to this share
         group_id = os.getegid()
@@ -402,17 +402,17 @@ class GlusterfsDriver(remotefs_drv.RemoteFSSnapDriver):
                 volume_size=volume_size_for)
         return greatest_share
 
-    def _mount_glusterfs(self, glusterfs_share, mount_path, ensure=False):
+    def _mount_glusterfs(self, glusterfs_share):
         """Mount GlusterFS share to mount path."""
-        # TODO(eharney): make this fs-agnostic and factor into remotefs
-        self._execute('mkdir', '-p', mount_path)
-
-        command = ['mount', '-t', 'glusterfs', glusterfs_share,
-                   mount_path]
+        mnt_flags = []
         if self.shares.get(glusterfs_share) is not None:
-            command.extend(self.shares[glusterfs_share].split())
-
-        self._do_mount(command, ensure, glusterfs_share)
+            mnt_flags = self.shares[glusterfs_share].split()
+        try:
+            self._remotefsclient.mount(glusterfs_share, mnt_flags)
+        except processutils.ProcessExecutionError:
+            LOG.error(_LE("Mount failure for %(share)s."),
+                      {'share': glusterfs_share})
+            raise
 
     def backup_volume(self, context, backup, backup_service):
         """Create a new backup from an existing volume.

diff --git a/cinder/volume/drivers/remotefs.py b/cinder/volume/drivers/remotefs.py
index 30e9feddf0b1a149cd486c428a105c7d74570faf..ca30ece8a05c4d70a6659b983de532128152dcd2 100644 (file)
--- a/cinder/volume/drivers/remotefs.py
+++ b/cinder/volume/drivers/remotefs.py
@@ -22,7 +22,6 @@ import re
 import tempfile
 import time
 
-from oslo_concurrency import processutils as putils
 from oslo_config import cfg
 from oslo_log import log as logging
 from oslo_utils import units
@@ -499,21 +498,6 @@ class RemoteFSDriver(driver.VolumeDriver):
         data['QoS_support'] = False
         self._stats = data
 
-    def _do_mount(self, cmd, ensure, share):
-        """Finalize mount command.
-
-        :param cmd: command to do the actual mount
-        :param ensure: boolean to allow remounting a share with a warning
-        :param share: description of the share for error reporting
-        """
-        try:
-            self._execute(*cmd, run_as_root=True)
-        except putils.ProcessExecutionError as exc:
-            if ensure and 'already mounted' in exc.stderr:
-                LOG.warn(_LW("%s is already mounted"), share)
-            else:
-                raise
-
     def _get_capacity_info(self, share):
         raise NotImplementedError()