From 6b847ffd945044bc86540884230db10873efc731 Mon Sep 17 00:00:00 2001 From: Lucian Petrut Date: Fri, 7 Oct 2016 14:46:00 +0300 Subject: [PATCH] Windows remotefs: create mountpoints at the expected location In order to support running inside a Scale-Out File Server cluster, when 'mounting' a local share, we're creating a symlink to the local export path without actually creating a SMB mapping. The issue is that instead of creating a symlink using the hash of the share UNC path, we're using the local export path instead. This is undesired behavior, in which case the caller will not be able to find the 'mountpoint'. This change addresses this issue, ensuring that we're using the mountpoint expected by the caller (e.g. the Cinder SMB driver) when mounting SMB shares. Closes-Bug: #1631351 Change-Id: I6bf909da559a745596f7d4c30d12e794b0b156de --- os_brick/remotefs/windows_remotefs.py | 33 +++++++++------- .../tests/remotefs/test_windows_remotefs.py | 38 ++++++++++--------- 2 files changed, 41 insertions(+), 30 deletions(-) diff --git a/os_brick/remotefs/windows_remotefs.py b/os_brick/remotefs/windows_remotefs.py index 1009666c5..4853daf1b 100644 --- a/os_brick/remotefs/windows_remotefs.py +++ b/os_brick/remotefs/windows_remotefs.py @@ -65,39 +65,46 @@ class WindowsRemoteFsClient(remotefs.RemoteFsClient): return local_share_path + def _get_share_norm_path(self, share): + return share.replace('/', '\\') + def get_share_name(self, share): - return share.replace('/', '\\').lstrip('\\').split('\\', 1)[1] + return self._get_share_norm_path(share).lstrip('\\').split('\\', 1)[1] def mount(self, share, flags=None): - share = share.replace('/', '\\') + share_norm_path = self._get_share_norm_path(share) use_local_path = (self._local_path_for_loopback and - self._smbutils.is_local_share(share)) + self._smbutils.is_local_share(share_norm_path)) if use_local_path: LOG.info(_LI("Skipping mounting local share %(share_path)s."), - dict(share_path=share)) + dict(share_path=share_norm_path)) else: mount_options = " ".join( [self._mount_options or '', flags or '']) username, password = self._parse_credentials(mount_options) if not self._smbutils.check_smb_mapping( - share): - self._smbutils.mount_smb_share(share, + share_norm_path): + self._smbutils.mount_smb_share(share_norm_path, username=username, password=password) if self._mount_base: - share_name = self.get_share_name(share) - symlink_dest = (share if not use_local_path - else self.get_local_share_path(share_name)) - self._create_mount_point(symlink_dest) + self._create_mount_point(share, use_local_path) def unmount(self, share): - self._smbutils.unmount_smb_share(share.replace('/', '\\')) + self._smbutils.unmount_smb_share(self._get_share_norm_path(share)) - def _create_mount_point(self, share): + def _create_mount_point(self, share, use_local_path): + # The mount point will contain a hash of the share so we're + # intentionally preserving the original share path as this is + # what the caller will expect. mnt_point = self.get_mount_point(share) + share_norm_path = self._get_share_norm_path(share) + share_name = self.get_share_name(share) + symlink_dest = (share_norm_path if not use_local_path + else self.get_local_share_path(share_name)) if not os.path.isdir(self._mount_base): os.makedirs(self._mount_base) @@ -107,7 +114,7 @@ class WindowsRemoteFsClient(remotefs.RemoteFsClient): raise exception.BrickException(_("Link path already exists " "and it's not a symlink")) else: - self._pathutils.create_sym_link(mnt_point, share) + self._pathutils.create_sym_link(mnt_point, symlink_dest) def _parse_credentials(self, opts_str): if not opts_str: diff --git a/os_brick/tests/remotefs/test_windows_remotefs.py b/os_brick/tests/remotefs/test_windows_remotefs.py index d6ea7bfc7..aa90d2cb0 100644 --- a/os_brick/tests/remotefs/test_windows_remotefs.py +++ b/os_brick/tests/remotefs/test_windows_remotefs.py @@ -65,10 +65,7 @@ class WindowsRemotefsClientTestCase(base.TestCase): @ddt.data(True, False) @mock.patch.object(windows_remotefs.WindowsRemoteFsClient, '_create_mount_point') - @mock.patch.object(windows_remotefs.WindowsRemoteFsClient, - 'get_local_share_path') def test_mount(self, is_local_share, - mock_get_local_share_path, mock_create_mount_point): flags = '-o pass=password' self._remotefs._mount_options = '-o user=username,randomopt' @@ -80,39 +77,37 @@ class WindowsRemotefsClientTestCase(base.TestCase): self._remotefs.mount(self._FAKE_SHARE, flags) if is_local_share: - expected_link_dest = mock_get_local_share_path.return_value - self.assertFalse(self._smbutils.check_smb_mapping.called) self.assertFalse(self._smbutils.mount_smb_share.called) - mock_get_local_share_path.assert_called_once_with( - self._FAKE_SHARE_NAME) else: - expected_link_dest = self._FAKE_SHARE - self._smbutils.check_smb_mapping.assert_called_once_with( self._FAKE_SHARE) self._smbutils.mount_smb_share.assert_called_once_with( self._FAKE_SHARE, username='username', password='password') - self.assertFalse(mock_get_local_share_path.called) - mock_create_mount_point.assert_called_once_with(expected_link_dest) + mock_create_mount_point.assert_called_once_with(self._FAKE_SHARE, + is_local_share) def test_unmount(self): self._remotefs.unmount(self._FAKE_SHARE) self._smbutils.unmount_smb_share.assert_called_once_with( self._FAKE_SHARE) - @ddt.data({}, + @ddt.data({'use_local_path': True}, {'path_exists': True, 'is_symlink': True}, {'path_exists': True}) + @mock.patch.object(windows_remotefs.WindowsRemoteFsClient, + 'get_local_share_path') @mock.patch.object(windows_remotefs.WindowsRemoteFsClient, 'get_mount_point') @mock.patch.object(windows_remotefs, 'os') @ddt.unpack def test_create_mount_point(self, mock_os, mock_get_mount_point, - path_exists=False, is_symlink=False): + mock_get_local_share_path, + path_exists=False, is_symlink=False, + use_local_path=False): mock_os.path.exists.return_value = path_exists mock_os.isdir.return_value = False self._pathutils.is_symlink.return_value = is_symlink @@ -120,17 +115,26 @@ class WindowsRemotefsClientTestCase(base.TestCase): if path_exists and not is_symlink: self.assertRaises(exception.BrickException, self._remotefs._create_mount_point, - mock.sentinel.share) + self._FAKE_SHARE, + use_local_path) else: - self._remotefs._create_mount_point(mock.sentinel.share) + self._remotefs._create_mount_point(self._FAKE_SHARE, + use_local_path) - mock_get_mount_point.assert_called_once_with(mock.sentinel.share) + mock_get_mount_point.assert_called_once_with(self._FAKE_SHARE) mock_os.path.isdir.assert_called_once_with(mock.sentinel.mount_base) + if use_local_path: + mock_get_local_share_path.assert_called_once_with( + self._FAKE_SHARE_NAME) + expected_symlink_target = mock_get_local_share_path.return_value + else: + expected_symlink_target = self._FAKE_SHARE.replace('/', '\\') + if path_exists: self._pathutils.is_symlink.assert_called_once_with( mock_get_mount_point.return_value) else: self._pathutils.create_sym_link.assert_called_once_with( mock_get_mount_point.return_value, - mock.sentinel.share) + expected_symlink_target)