From 3123d1edc82bbeebecadcbbf47875787c162c03b Mon Sep 17 00:00:00 2001 From: "Bharat Kumar Kobagana (BharatK)" Date: Wed, 18 Mar 2015 19:16:39 +0530 Subject: [PATCH] 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 --- cinder/tests/unit/test_glusterfs.py | 78 ++++++----------------------- cinder/volume/drivers/glusterfs.py | 24 ++++----- cinder/volume/drivers/remotefs.py | 16 ------ 3 files changed, 27 insertions(+), 91 deletions(-) diff --git a/cinder/tests/unit/test_glusterfs.py b/cinder/tests/unit/test_glusterfs.py index b85da1250..0863c88e5 100644 --- 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 325c7d221..5bdc77dbd 100644 --- 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 30e9feddf..ca30ece8a 100644 --- 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() -- 2.45.2