GPFS KNFS: Fix deny access to succeed when possible

Use looser error code checking and add a verify
after deny access so that the call will succeed
whenever it can be confirmed that the access no
longer exists.

This fixes a variety of situations where previously
a return code of 1 while attempting to remove access
would cause the manila access rule to get stuck while
the actual client access export was removed or never
existed.

Change-Id: Ie058a6185e3f5d91fb1cf232301eb0ac6ddcea7e
Closes-Bug: #1651587

manila/share/drivers/helpers.py

@@ -210,7 +210,7 @@ class NFSHelper(NASHelperBase):
                 access_rules, ('ip',),
                 (const.ACCESS_LEVEL_RO, const.ACCESS_LEVEL_RW))
 
-            hosts = self._get_host_list(out, local_path)
+            hosts = self.get_host_list(out, local_path)
             for host in hosts:
                 self._ssh_exec(server, ['sudo', 'exportfs', '-u',
                                         ':'.join((host, local_path))])
@@ -279,7 +279,8 @@ class NFSHelper(NASHelperBase):
             if add_rules:
                 self._sync_nfs_temp_and_perm_files(server)
 
-    def _get_host_list(self, output, local_path):
+    @staticmethod
+    def get_host_list(output, local_path):
         entries = []
         output = output.replace('\n\t\t', ' ')
         lines = output.split('\n')

manila/share/drivers/ibm/gpfs.py

@@ -46,6 +46,7 @@ from manila.common import constants
 from manila import exception
 from manila.i18n import _
 from manila.share import driver
+from manila.share.drivers.helpers import NFSHelper
 from manila.share import share_types
 from manila import utils
 
@@ -678,7 +679,10 @@ class KNFSHelper(NASHelperBase):
             LOG.error(msg)
             raise exception.GPFSException(msg)
 
-    def _publish_access(self, *cmd):
+    def _publish_access(self, *cmd, **kwargs):
+        check_exit_code = kwargs.get('check_exit_code', True)
+
+        outs = []
         localserver_iplist = socket.gethostbyname_ex(socket.gethostname())[2]
         for server in self.configuration.gpfs_nfs_server_list:
             if server in localserver_iplist:
@@ -690,11 +694,40 @@ class KNFSHelper(NASHelperBase):
                 run_command = ['ssh', remote_login] + list(cmd)
                 run_local = False
             try:
-                utils.execute(*run_command,
-                              run_as_root=run_local,
-                              check_exit_code=True)
+                out = utils.execute(*run_command,
+                                    run_as_root=run_local,
+                                    check_exit_code=check_exit_code)
             except exception.ProcessExecutionError:
                 raise
+            outs.append(out)
+        return outs
+
+    def _verify_denied_access(self, local_path, share, ip):
+        try:
+            cmd = ['exportfs']
+            outs = self._publish_access(*cmd)
+        except exception.ProcessExecutionError:
+            msg = _('Failed to verify denied access for '
+                    'share %s.') % share['name']
+            LOG.exception(msg)
+            raise exception.GPFSException(msg)
+
+        for stdout, stderr in outs:
+            if stderr and stderr.strip():
+                msg = ('Log/ignore stderr during _validate_denied_access for '
+                       'share %(sharename)s. Return code OK. '
+                       'Stderr: %(stderr)s' % {'sharename': share['name'],
+                                               'stderr': stderr})
+                LOG.debug(msg)
+
+            gpfs_ips = NFSHelper.get_host_list(stdout, local_path)
+            if ip in gpfs_ips:
+                msg = (_('Failed to deny access for share %(sharename)s. '
+                         'IP %(ip)s still has access.') %
+                       {'sharename': share['name'],
+                        'ip': ip})
+                LOG.error(msg)
+                raise exception.GPFSException(msg)
 
     def remove_export(self, local_path, share):
         """Remove export."""
@@ -741,17 +774,23 @@ class KNFSHelper(NASHelperBase):
 
     def deny_access(self, local_path, share, access, force=False):
         """Remove access for one or more vm instances."""
-        cmd = ['exportfs', '-u', ':'.join([access['access_to'], local_path])]
+        ip = access['access_to']
+        cmd = ['exportfs', '-u', ':'.join([ip, local_path])]
         try:
-            self._publish_access(*cmd)
-        except exception.ProcessExecutionError as e:
-            msg = (_('Failed to deny access for share %(sharename)s. '
-                     'Error: %(excmsg)s.') %
-                   {'sharename': share['name'],
-                    'excmsg': e})
-            LOG.error(msg)
+            # Can get exit code 0 for success or 1 for already gone (also
+            # potentially get 1 due to exportfs bug). So allow
+            # _publish_access to continue with [0, 1] and then verify after
+            # it is done.
+            self._publish_access(*cmd, check_exit_code=[0, 1])
+        except exception.ProcessExecutionError:
+            msg = _('Failed to deny access for share %s.') % share['name']
+            LOG.exception(msg)
             raise exception.GPFSException(msg)
 
+        # Error code (0 or 1) makes deny IP success indeterminate.
+        # So, verify that the IP access was completely removed.
+        self._verify_denied_access(local_path, share, ip)
+
 
 class CESHelper(NASHelperBase):
     """Wrapper for NFS by Spectrum Scale CES"""

manila/tests/share/drivers/ibm/test_gpfs.py

@@ -789,25 +789,95 @@ mmcesnfslsexport:nfsexports:HEADER:version:reserved:reserved:Path:Delegations:Cl
         self._knfs_helper._execute.assert_called_once_with('exportfs',
                                                            run_as_root=True)
 
+    def test_knfs__verify_denied_access_pass(self):
+        local_path = self.fakesharepath
+        ip = self.access['access_to']
+        fake_exportfs = ('/shares/share-1\n\t\t1.1.1.1\n'
+                         '/shares/share-2\n\t\t2.2.2.2\n')
+        self._knfs_helper._publish_access = mock.Mock(
+            return_value=[(fake_exportfs, '')])
+
+        self._knfs_helper._verify_denied_access(local_path, self.share, ip)
+
+        self._knfs_helper._publish_access.assert_called_once_with('exportfs')
+
+    def test_knfs__verify_denied_access_fail(self):
+        local_path = self.fakesharepath
+        ip = self.access['access_to']
+        data = {'path': local_path, 'ip': ip}
+        fake_exportfs = ('/shares/share-1\n\t\t1.1.1.1\n'
+                         '%(path)s\n\t\t%(ip)s\n'
+                         '/shares/share-2\n\t\t2.2.2.2\n') % data
+        self._knfs_helper._publish_access = mock.Mock(
+            return_value=[(fake_exportfs, '')])
+
+        self.assertRaises(exception.GPFSException,
+                          self._knfs_helper._verify_denied_access,
+                          local_path,
+                          self.share,
+                          ip)
+
+        self._knfs_helper._publish_access.assert_called_once_with('exportfs')
+
+    def test_knfs__verify_denied_access_exception(self):
+        self._knfs_helper._publish_access = mock.Mock(
+            side_effect=exception.ProcessExecutionError
+        )
+
+        ip = self.access['access_to']
+        local_path = self.fakesharepath
+
+        self.assertRaises(exception.GPFSException,
+                          self._knfs_helper._verify_denied_access,
+                          local_path,
+                          self.share,
+                          ip)
+
+        self._knfs_helper._publish_access.assert_called_once_with('exportfs')
+
+    @ddt.data((None, False),
+              ('', False),
+              (' ', False),
+              ('Some error to log', True))
+    @ddt.unpack
+    def test_knfs__verify_denied_access_stderr(self, stderr, is_logged):
+        """Stderr debug logging should only happen when not empty."""
+        outputs = [('', stderr)]
+        self._knfs_helper._publish_access = mock.Mock(return_value=outputs)
+        gpfs.LOG.debug = mock.Mock()
+
+        self._knfs_helper._verify_denied_access(
+            self.fakesharepath, self.share, self.remote_ip)
+
+        self._knfs_helper._publish_access.assert_called_once_with('exportfs')
+        self.assertEqual(is_logged, gpfs.LOG.debug.called)
+
     def test_knfs_deny_access(self):
-        self._knfs_helper._publish_access = mock.Mock()
+        self._knfs_helper._publish_access = mock.Mock(return_value=[('', '')])
+
         access = self.access
         local_path = self.fakesharepath
         self._knfs_helper.deny_access(local_path, self.share, access)
-        cmd = ['exportfs', '-u', ':'.join([access['access_to'], local_path])]
-        self._knfs_helper._publish_access.assert_called_once_with(*cmd)
+
+        deny = ['exportfs', '-u', ':'.join([access['access_to'], local_path])]
+        self._knfs_helper._publish_access.assert_has_calls([
+            mock.call(*deny, check_exit_code=[0, 1]),
+            mock.call('exportfs')])
 
     def test_knfs_deny_access_exception(self):
         self._knfs_helper._publish_access = mock.Mock(
             side_effect=exception.ProcessExecutionError
         )
+
         access = self.access
         local_path = self.fakesharepath
         cmd = ['exportfs', '-u', ':'.join([access['access_to'], local_path])]
         self.assertRaises(exception.GPFSException,
                           self._knfs_helper.deny_access, local_path,
                           self.share, access)
-        self._knfs_helper._publish_access.assert_called_once_with(*cmd)
+
+        self._knfs_helper._publish_access.assert_called_once_with(
+            *cmd, check_exit_code=[0, 1])
 
     def test_knfs__publish_access(self):
         self.mock_object(utils, 'execute')

manila/tests/share/drivers/test_helpers.py

@@ -180,7 +180,7 @@ class NFSHelperTestCase(test.TestCase):
                          '/shares/share-3\n\t\t30.0.0.4\n'
                          '/shares/share-3\n\t\t30.0.0.7\n')
         expected = ['20.0.0.3', '20.0.0.6']
-        result = self._helper._get_host_list(fake_exportfs, '/shares/share-1')
+        result = self._helper.get_host_list(fake_exportfs, '/shares/share-1')
         self.assertEqual(expected, result)
 
     @ddt.data(const.ACCESS_LEVEL_RW, const.ACCESS_LEVEL_RO)
@@ -192,7 +192,7 @@ class NFSHelperTestCase(test.TestCase):
         access_rules = [test_generic.get_fake_access_rule(
             '1.1.1.1', access_level), ]
         self.mock_object(self._helper, '_sync_nfs_temp_and_perm_files')
-        self.mock_object(self._helper, '_get_host_list',
+        self.mock_object(self._helper, 'get_host_list',
                          mock.Mock(return_value=['1.1.1.1']))
         self._helper.update_access(self.server, self.share_name, access_rules,
                                    [], [])

releasenotes/notes/bug-1651587-deny-access-verify-563ef2f3f6b8c13b.yaml

@@ -0,0 +1,4 @@
+---
+fixes:
+  - Fixed GPFS KNFS deny access so that it will not fail when
+    the access can be verified to not exist.