From c5545e861c8ee069a6e8c7b710b8813d8cb77012 Mon Sep 17 00:00:00 2001 From: Mark Sturdevant Date: Wed, 14 Dec 2016 14:49:55 -0800 Subject: [PATCH] GPFS CES: Fix bugs related to access rules not found Several bugs are caused by an error code and exception raised when a path has no NFS exports. Use the machine-readable mmnfs export list command (the -Y option) which does not cause an exception when checking for exports. Co-Authored-By: digvijay2016 Change-Id: I770756e0a36c5b61386878164b651fadf9730b7f Closes-Bug: #1650043 Closes-Bug: #1650044 Closes-Bug: #1650045 (cherry picked from commit 5e7323cd1f53f7f7096bcd32e84c36cc4858097a) --- manila/share/drivers/ibm/gpfs.py | 31 +++-- manila/tests/share/drivers/ibm/test_gpfs.py | 117 +++++++++++------- ...043-gpfs-access-bugs-8c10f26ff1f795f4.yaml | 6 + 3 files changed, 91 insertions(+), 63 deletions(-) create mode 100644 releasenotes/notes/bug-1650043-gpfs-access-bugs-8c10f26ff1f795f4.yaml diff --git a/manila/share/drivers/ibm/gpfs.py b/manila/share/drivers/ibm/gpfs.py index 90c9108cf7..61335fe0e1 100644 --- a/manila/share/drivers/ibm/gpfs.py +++ b/manila/share/drivers/ibm/gpfs.py @@ -769,14 +769,19 @@ class CESHelper(NASHelperBase): raise exception.GPFSException(msg) return out + def _has_client_access(self, local_path, access_to=None): + """Check path for any export or for one with a specific IP address.""" + err_msg = 'Failed to check exports on the system.' + out = self._execute_mmnfs_command(('list', '-n', local_path, '-Y'), + err_msg) + if access_to: + return ':' + access_to + ':' in out + else: + return ':' + local_path + ':' in out + def remove_export(self, local_path, share): """Remove export.""" - err_msg = 'Failed to check exports on the system.' - out = self._execute_mmnfs_command(('list', '-n', local_path), err_msg) - - out = re.search(re.escape(local_path), out) - - if out is not None: + if self._has_client_access(local_path): err_msg = ('Failed to remove export for share %s.' % share['name']) self._execute_mmnfs_command(('remove', local_path), err_msg) @@ -787,16 +792,13 @@ class CESHelper(NASHelperBase): if access['access_type'] != 'ip': raise exception.InvalidShareAccess(reason='Only ip access type ' 'supported.') - err_msg = 'Failed to check exports on the system.' - out = self._execute_mmnfs_command(('list', '-n', local_path), err_msg) + has_exports = self._has_client_access(local_path) options_not_allowed = ['access_type=ro', 'access_type=rw'] export_opts = self.get_export_options(share, access, 'CES', options_not_allowed) - out = re.search(re.escape(local_path), out) - - if out is None: + if not has_exports: cmd = ['add', local_path, '-c', access['access_to'] + '(' + export_opts + ')'] @@ -811,12 +813,9 @@ class CESHelper(NASHelperBase): def deny_access(self, local_path, share, access, force=False): """Deny access to the host.""" - err_msg = 'Failed to check exports on the system.' - out = self._execute_mmnfs_command(('list', '-n', local_path), err_msg) + has_export = self._has_client_access(local_path, access['access_to']) - out = re.search(re.escape(access['access_to']), out) - - if out is not None: + if has_export: err_msg = ('Failed to remove access for share %s.' % share['name']) self._execute_mmnfs_command(('change', local_path, diff --git a/manila/tests/share/drivers/ibm/test_gpfs.py b/manila/tests/share/drivers/ibm/test_gpfs.py index 373c33c0c9..52feb92752 100644 --- a/manila/tests/share/drivers/ibm/test_gpfs.py +++ b/manila/tests/share/drivers/ibm/test_gpfs.py @@ -56,6 +56,20 @@ class GPFSShareDriverTestCase(test.TestCase): self.fakefspath = "/gpfs0" self.fakesharepath = "/gpfs0/share-fakeid" self.fakesnapshotpath = "/gpfs0/.snapshots/snapshot-fakesnapshotid" + + self.fake_ces_exports = """ +mmcesnfslsexport:nfsexports:HEADER:version:reserved:reserved:Path:Delegations:Clients:Access_Type:Protocols:Transports:Squash:Anonymous_uid:Anonymous_gid:SecType:PrivilegedPort:DefaultDelegations:Manage_Gids:NFS_Commit: +mmcesnfslsexport:nfsexports:0:1:::/gpfs0/share-fakeid:none:44.3.2.11:RW:3,4:TCP:ROOT_SQUASH:-2:-2:SYS:FALSE:none:FALSE:FALSE: +mmcesnfslsexport:nfsexports:0:1:::/gpfs0/share-fakeid:none:1:2:3:4:5:6:7:8:RW:3,4:TCP:ROOT_SQUASH:-2:-2:SYS:FALSE:none:FALSE:FALSE: +mmcesnfslsexport:nfsexports:0:1:::/gpfs0/share-fakeid:none:10.0.0.1:RW:3,4:TCP:ROOT_SQUASH:-2:-2:SYS:FALSE:none:FALSE:FALSE: + + """ + self.fake_ces_exports_not_found = """ + +mmcesnfslsexport:nfsexports:HEADER:version:reserved:reserved:Path:Delegations:Clients:Access_Type:Protocols:Transports:Squash:Anonymous_uid:Anonymous_gid:SecType:PrivilegedPort:DefaultDelegations:Manage_Gids:NFS_Commit: + + """ + self.mock_object(gpfs.os.path, 'exists', mock.Mock(return_value=True)) self._driver._helpers = { 'KNFS': self._helper_fake @@ -860,24 +874,49 @@ class GPFSShareDriverTestCase(test.TestCase): self._ces_helper.get_export_options, share, access, 'CES', options_not_allowed) - def test_ces_remove_export(self): - mock_out = "Path Delegations Clients\n\ - ------------------------\n\ - /gpfs0/share-fakeid none *" + @ddt.data((None, True), + ('44.3.2.11', True), + ('44.3.2.1', False), + ('4.3.2.1', False), + ('4.3.2.11', False), + ('1.2.3.4', False), + ('1:2:3:4:5:6:7:8', True)) + @ddt.unpack + def test_ces__has_client_access(self, ip, has_access): + mock_out = self.fake_ces_exports self._ces_helper._execute = mock.Mock( - return_value=[mock_out, 0]) - - mock_search_out = "/gpfs0/share-fakeid" - self.mock_object(re, 'search', mock.Mock(return_value=mock_search_out)) + return_value=(mock_out, '')) local_path = self.fakesharepath + self.assertEqual(has_access, + self._ces_helper._has_client_access(local_path, ip)) + self._ces_helper._execute.assert_called_once_with( + 'mmnfs', 'export', 'list', '-n', local_path, '-Y') + + def test_ces_remove_export_no_exports(self): + mock_out = self.fake_ces_exports_not_found + self._ces_helper._execute = mock.Mock( + return_value=(mock_out, '')) + + local_path = self.fakesharepath self._ces_helper.remove_export(local_path, self.share) - self._ces_helper._execute.assert_any_call('mmnfs', 'export', 'list', - '-n', local_path) - self._ces_helper._execute.assert_any_call('mmnfs', 'export', 'remove', - local_path) + self._ces_helper._execute.assert_called_once_with( + 'mmnfs', 'export', 'list', '-n', local_path, '-Y') + + def test_ces_remove_export_existing_exports(self): + mock_out = self.fake_ces_exports + self._ces_helper._execute = mock.Mock( + return_value=(mock_out, '')) + + local_path = self.fakesharepath + self._ces_helper.remove_export(local_path, self.share) + + self._ces_helper._execute.assert_has_calls([ + mock.call('mmnfs', 'export', 'list', '-n', local_path, '-Y'), + mock.call('mmnfs', 'export', 'remove', local_path), + ]) def test_ces_remove_export_exception(self): local_path = self.fakesharepath @@ -888,52 +927,42 @@ class GPFSShareDriverTestCase(test.TestCase): local_path, self.share) def test_ces_allow_access(self): - mock_out = "Path Delegations Clients\n\ - ------------------------" + mock_out = self.fake_ces_exports_not_found self._ces_helper._execute = mock.Mock( - return_value=[mock_out, 0]) + return_value=(mock_out, '')) export_opts = "access_type=rw" self._ces_helper.get_export_options = mock.Mock( return_value=export_opts) - self.mock_object(re, 'search', mock.Mock(return_value=None)) access = self.access local_path = self.fakesharepath self._ces_helper.allow_access(local_path, self.share, access) - self._ces_helper._execute.assert_any_call('mmnfs', 'export', 'list', - '-n', local_path) - self._ces_helper._execute.assert_any_call('mmnfs', 'export', 'add', - local_path, '-c', - access['access_to'] - + '(' + export_opts + ')') + self._ces_helper._execute.assert_has_calls([ + mock.call('mmnfs', 'export', 'list', '-n', local_path, '-Y'), + mock.call('mmnfs', 'export', 'add', local_path, '-c', + access['access_to'] + '(' + export_opts + ')')]) - def test_ces_allow_access_existing_export(self): - mock_out = "Path Delegations Clients\n\ - ------------------------\n\ - /gpfs0/share-fakeid none *" + def test_ces_allow_access_existing_exports(self): + mock_out = self.fake_ces_exports self._ces_helper._execute = mock.Mock( - return_value=[mock_out, 0]) + return_value=(mock_out, '')) export_opts = "access_type=rw" self._ces_helper.get_export_options = mock.Mock( return_value=export_opts) - mock_search_out = "/gpfs0/share-fakeid" - self.mock_object(re, 'search', mock.Mock(return_value=mock_search_out)) access = self.access local_path = self.fakesharepath self._ces_helper.allow_access(local_path, self.share, access) - self._ces_helper._execute.assert_any_call('mmnfs', 'export', 'list', - '-n', local_path) - self._ces_helper._execute.assert_any_call('mmnfs', 'export', 'change', - local_path, '--nfsadd', - access['access_to'] - + '(' + export_opts + ')') + self._ces_helper._execute.assert_has_calls([ + mock.call('mmnfs', 'export', 'list', '-n', local_path, '-Y'), + mock.call('mmnfs', 'export', 'change', local_path, '--nfsadd', + access['access_to'] + '(' + export_opts + ')')]) def test_ces_allow_access_invalid_access_type(self): access = fake_share.fake_access(access_type='test') @@ -952,25 +981,19 @@ class GPFSShareDriverTestCase(test.TestCase): self.share, access) def test_ces_deny_access(self): - mock_out = "Path Delegations Clients\n\ - ------------------------\n\ - /gpfs0/share-fakeid none *" + mock_out = self.fake_ces_exports self._ces_helper._execute = mock.Mock( - return_value=[mock_out, 0]) - - mock_search_out = "/gpfs0/share-fakeid" - self.mock_object(re, 'search', mock.Mock(return_value=mock_search_out)) + return_value=(mock_out, '')) access = self.access local_path = self.fakesharepath self._ces_helper.deny_access(local_path, self.share, access) - self._ces_helper._execute.assert_any_call('mmnfs', 'export', 'list', - '-n', local_path) - self._ces_helper._execute.assert_any_call('mmnfs', 'export', 'change', - local_path, '--nfsremove', - access['access_to']) + self._ces_helper._execute.assert_has_calls([ + mock.call('mmnfs', 'export', 'list', '-n', local_path, '-Y'), + mock.call('mmnfs', 'export', 'change', local_path, '--nfsremove', + access['access_to'])]) def test_ces_deny_access_exception(self): access = self.access diff --git a/releasenotes/notes/bug-1650043-gpfs-access-bugs-8c10f26ff1f795f4.yaml b/releasenotes/notes/bug-1650043-gpfs-access-bugs-8c10f26ff1f795f4.yaml new file mode 100644 index 0000000000..ac0e81faf3 --- /dev/null +++ b/releasenotes/notes/bug-1650043-gpfs-access-bugs-8c10f26ff1f795f4.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - Fixed GPFS CES to allow adding a first access rule to a share. + - Fixed GPFS CES to allow deleting a share with no access rules. + - Fixed GPFS CES to allow deletion of a failed access rule + when there are no successful access rules.