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 <digvijay.ukirde@in.ibm.com>

Change-Id: I770756e0a36c5b61386878164b651fadf9730b7f
Closes-Bug: #1650043
Closes-Bug: #1650044
Closes-Bug: #1650045

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.'
+        if self._has_client_access(local_path):
-        out = self._execute_mmnfs_command(('list', '-n', local_path), err_msg)
-
-        out = re.search(re.escape(local_path), out)
-
-        if out is not None:
             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.'
+        has_exports = self._has_client_access(local_path)
-        out = self._execute_mmnfs_command(('list', '-n', local_path), err_msg)
 
         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 not has_exports:
-
-        if out is None:
             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.'
+        has_export = self._has_client_access(local_path, access['access_to'])
-        out = self._execute_mmnfs_command(('list', '-n', local_path), err_msg)
 
-        out = re.search(re.escape(access['access_to']), out)
+        if has_export:
-
-        if out is not None:
             err_msg = ('Failed to remove access for share %s.'
                        % share['name'])
             self._execute_mmnfs_command(('change', local_path,

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):
+    @ddt.data((None, True),
-        mock_out = "Path Delegations Clients\n\
+              ('44.3.2.11', True),
-                    ------------------------\n\
+              ('44.3.2.1', False),
-                    /gpfs0/share-fakeid none *"
+              ('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])
+            return_value=(mock_out, ''))
-
-        mock_search_out = "/gpfs0/share-fakeid"
-        self.mock_object(re, 'search', mock.Mock(return_value=mock_search_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',
+        self._ces_helper._execute.assert_called_once_with(
-                                                  '-n', local_path)
+            'mmnfs', 'export', 'list', '-n', local_path, '-Y')
-        self._ces_helper._execute.assert_any_call('mmnfs', 'export', 'remove',
+
-                                                  local_path)
+    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',
+        self._ces_helper._execute.assert_has_calls([
-                                                  '-n', local_path)
+            mock.call('mmnfs', 'export', 'list', '-n', local_path, '-Y'),
-        self._ces_helper._execute.assert_any_call('mmnfs', 'export', 'add',
+            mock.call('mmnfs', 'export', 'add', local_path, '-c',
-                                                  local_path, '-c',
+                      access['access_to'] + '(' + export_opts + ')')])
-                                                  access['access_to']
-                                                  + '(' + export_opts + ')')
 
-    def test_ces_allow_access_existing_export(self):
+    def test_ces_allow_access_existing_exports(self):
-        mock_out = "Path Delegations Clients\n\
+        mock_out = self.fake_ces_exports
-                    ------------------------\n\
-                    /gpfs0/share-fakeid none *"
         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',
+        self._ces_helper._execute.assert_has_calls([
-                                                  '-n', local_path)
+            mock.call('mmnfs', 'export', 'list', '-n', local_path, '-Y'),
-        self._ces_helper._execute.assert_any_call('mmnfs', 'export', 'change',
+            mock.call('mmnfs', 'export', 'change', local_path, '--nfsadd',
-                                                  local_path, '--nfsadd',
+                      access['access_to'] + '(' + export_opts + ')')])
-                                                  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\
+        mock_out = self.fake_ces_exports
-                    ------------------------\n\
-                    /gpfs0/share-fakeid none *"
         self._ces_helper._execute = mock.Mock(
-            return_value=[mock_out, 0])
+            return_value=(mock_out, ''))
-
-        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.deny_access(local_path, self.share, access)
 
-        self._ces_helper._execute.assert_any_call('mmnfs', 'export', 'list',
+        self._ces_helper._execute.assert_has_calls([
-                                                  '-n', local_path)
+            mock.call('mmnfs', 'export', 'list', '-n', local_path, '-Y'),
-        self._ces_helper._execute.assert_any_call('mmnfs', 'export', 'change',
+            mock.call('mmnfs', 'export', 'change', local_path, '--nfsremove',
-                                                  local_path, '--nfsremove',
+                      access['access_to'])])
-                                                  access['access_to'])
 
     def test_ces_deny_access_exception(self):
         access = self.access

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.