Merge "Remove nsenter dependency"

devstack/plugin.sh

@@ -592,12 +592,6 @@ function install_manila {
                 sudo apt-get install -y build-essential
                 sudo apt-get install -y ubuntu-zfs
                 sudo modprobe zfs
-
-                # TODO(vponomaryov): remove following line when we have this
-                # in 'requirements.txt' file.
-                # Package 'nsenter' is expected to be installed on host with
-                # ZFS, if it is remote for manila-share service host.
-                sudo pip install nsenter
             else
                 echo "Manila Devstack plugin does not support installation "\
                     "of ZFS packages for non-'Ubuntu-trusty' distros. "\

etc/manila/rootwrap.d/share.filters

@@ -146,9 +146,6 @@ zpool: CommandFilter, zpool, root
 # manila/share/drivers/zfsonlinux/utils.py
 zfs: CommandFilter, zfs, root
 
-# manila/share/drivers/zfsonlinux/driver.py
-nsenter: CommandFilter, nsenter, root
-
 # LXD driver commands
 # manila/share/drivers/lxd.py
 lxc: CommandFilter, lxc, root

manila/share/drivers/zfsonlinux/driver.py

@@ -27,7 +27,7 @@ from oslo_utils import timeutils
 
 from manila.common import constants
 from manila import exception
-from manila.i18n import _, _LW
+from manila.i18n import _, _LI, _LW
 from manila.share import driver
 from manila.share.drivers.zfsonlinux import utils as zfs_utils
 from manila.share import utils as share_utils
@@ -186,46 +186,29 @@ class ZFSonLinuxShareDriver(zfs_utils.ExecuteMixin, driver.ShareDriver):
                     msg=_("Could not destroy '%s' dataset, "
                           "because it had opened files.") % name)
 
-            try:
-                self.zfs('destroy', '-f', name)
-                return
-            except exception.ProcessExecutionError as e:
-                LOG.debug("Failed to run command, got error: %s\n"
-                          "Assuming other namespace-based services hold "
-                          "ZFS mounts.", e)
-
-            # NOTE(vponomaryov): perform workaround for Neutron bug #1546723
-            # We should release ZFS mount from all namespaces. It should not be
-            # there at all.
-            get_pids_cmd = (
-                "(echo $(grep -s %s /proc/*/mounts) ) 2>&1 " % mountpoint)
-            try:
-                raw_pids, err = self.execute('bash', '-c', get_pids_cmd)
-            except exception.ProcessExecutionError as e:
-                LOG.warning(
-                    _LW("Failed to get list of PIDs that hold ZFS dataset "
-                        "mountpoint. Got following error: %s"), e)
-            else:
-                pids = [s.split('/')[0] for s in raw_pids.split('/proc/') if s]
-                LOG.debug(
-                    "List of pids that hold ZFS mount '%(mnt)s': %(list)s", {
-                        'mnt': mountpoint, 'list': ' '.join(pids)})
-                for pid in pids:
-                    try:
-                        self.execute(
-                            'sudo', 'nsenter', '--mnt', '--target=%s' % pid,
-                            '/bin/umount', mountpoint)
-                    except exception.ProcessExecutionError as e:
-                        LOG.warning(
-                            _LW("Failed to run command with release of "
-                                "ZFS dataset mount, got error: %s"), e)
-
-            # NOTE(vponomaryov): sleep some time after unmount operations.
-            time.sleep(1)
-
         # NOTE(vponomaryov): Now, when no file usages and mounts of dataset
         # exist, destroy dataset.
-        self.zfs_with_retry('destroy', '-f', name)
+        try:
+            self.zfs('destroy', '-f', name)
+            return
+        except exception.ProcessExecutionError:
+            LOG.info(_LI("Failed to destroy ZFS dataset, retrying one time"))
+
+        # NOTE(bswartz): There appears to be a bug in ZFS when creating and
+        # destroying datasets concurrently where the filesystem remains mounted
+        # even though ZFS thinks it's unmounted. The most reliable workaround
+        # I've found is to force the unmount, then retry the destroy, with
+        # short pauses around the unmount.
+        time.sleep(1)
+        try:
+            self.execute('sudo', 'umount', mountpoint)
+        except exception.ProcessExecutionError:
+            # Ignore failed umount, it's normal
+            pass
+        time.sleep(1)
+
+        # This time the destroy is expected to succeed.
+        self.zfs('destroy', '-f', name)
 
     def _setup_helpers(self):
         """Setups share helper for ZFS backend."""
@@ -268,6 +251,11 @@ class ZFSonLinuxShareDriver(zfs_utils.ExecuteMixin, driver.ShareDriver):
                 reason=_("No zpools specified for usage: "
                          "%s") % self.zpool_list)
 
+        # Make pool mounts shared so that cloned namespaces receive unmounts
+        # and don't prevent us from unmounting datasets
+        for zpool in self.configuration.zfs_zpool_list:
+            self.execute('sudo', 'mount', '--make-rshared', ('/%s' % zpool))
+
         if self.configuration.zfs_use_ssh:
             # Check workability of SSH executor
             self.ssh_executor('whoami')

manila/share/drivers/zfsonlinux/utils.py

@@ -124,10 +124,6 @@ class ExecuteMixin(driver.ExecuteMixin):
         """ZFS shell commands executor."""
         return self.execute('sudo', 'zfs', *cmd, **kwargs)
 
-    def zfs_with_retry(self, *cmd, **kwargs):
-        """ZFS shell commands executor with retries."""
-        return self.execute_with_retry('sudo', 'zfs', *cmd, **kwargs)
-
 
 @six.add_metaclass(abc.ABCMeta)
 class NASHelperBase(object):

manila/tests/share/drivers/zfsonlinux/test_driver.py

@@ -121,8 +121,7 @@ class ZFSonLinuxShareDriverTestCase(test.TestCase):
         self.assertTrue(hasattr(self.driver, '_helpers'))
         self.assertEqual(self.driver._helpers, {})
         for attr_name in ('execute', 'execute_with_retry', 'parse_zfs_answer',
-                          'get_zpool_option', 'get_zfs_option', 'zfs',
+                          'get_zpool_option', 'get_zfs_option', 'zfs'):
-                          'zfs_with_retry'):
             self.assertTrue(hasattr(self.driver, attr_name))
 
     def test_init_error_with_duplicated_zpools(self):
@@ -177,9 +176,9 @@ class ZFSonLinuxShareDriverTestCase(test.TestCase):
 
         self.driver._setup_helpers.assert_called_once_with()
         if use_ssh:
-            self.driver.ssh_executor.assert_called_once_with('whoami')
+            self.assertEqual(4, self.driver.ssh_executor.call_count)
         else:
-            self.assertEqual(0, self.driver.ssh_executor.call_count)
+            self.assertEqual(3, self.driver.ssh_executor.call_count)
 
     @ddt.data(
         ('foo', '127.0.0.1'),
@@ -860,13 +859,13 @@ class ZFSonLinuxShareDriverTestCase(test.TestCase):
 
     def test__delete_dataset_or_snapshot_with_retry_snapshot(self):
         self.mock_object(self.driver, 'get_zfs_option')
-        self.mock_object(self.driver, 'zfs_with_retry')
+        self.mock_object(self.driver, 'zfs')
 
         self.driver._delete_dataset_or_snapshot_with_retry('foo@bar')
 
         self.driver.get_zfs_option.assert_called_once_with(
             'foo@bar', 'mountpoint')
-        self.driver.zfs_with_retry.assert_called_once_with(
+        self.driver.zfs.assert_called_once_with(
             'destroy', '-f', 'foo@bar')
 
     def test__delete_dataset_or_snapshot_with_retry_of(self):
@@ -917,92 +916,29 @@ class ZFSonLinuxShareDriverTestCase(test.TestCase):
         zfs_driver.time.sleep.assert_called_once_with(2)
         self.driver.zfs.assert_called_once_with('destroy', '-f', dataset_name)
 
-    def test__delete_dataset_or_snapshot_with_retry_mount_holders(self):
+    def test__delete_dataset_or_snapshot_with_retry_busy(self):
-        mnt = 'fake_mnt'
+        self.mock_object(self.driver, 'get_zfs_option')
         self.mock_object(
-            self.driver, 'get_zfs_option', mock.Mock(return_value=mnt))
+            self.driver, 'execute', mock.Mock(
-        self.mock_object(self.driver, 'zfs_with_retry')
+                side_effect=exception.ProcessExecutionError(
+                    'FAKE lsof returns not found')))
         self.mock_object(
-            self.driver, 'zfs',
+            self.driver, 'zfs', mock.Mock(side_effect=[
-            mock.Mock(side_effect=exception.ProcessExecutionError('FAKE')))
+                exception.ProcessExecutionError(
-        pids_raw = '/proc/1234/foo /proc/5678/bar'
+                    'cannot destroy FAKE: dataset is busy\n'),
-        self.mock_object(
+                None, None]))
-            self.driver, 'execute', mock.Mock(side_effect=[
-                ('a', 'b'),
-                exception.ProcessExecutionError('Fake lsof empty'),
-                (pids_raw, 'c'),
-                exception.ProcessExecutionError('Fake PID not found error'),
-                ('d', 'e'),
-            ]))
         self.mock_object(zfs_driver.time, 'sleep')
-        self.mock_object(zfs_driver.LOG, 'debug')
+        self.mock_object(zfs_driver.LOG, 'info')
-        self.mock_object(zfs_driver.LOG, 'warning')
-        self.mock_object(
-            zfs_driver.time, 'time', mock.Mock(side_effect=range(1, 70, 2)))
         dataset_name = 'fake/dataset/name'
 
         self.driver._delete_dataset_or_snapshot_with_retry(dataset_name)
 
         self.driver.get_zfs_option.assert_called_once_with(
             dataset_name, 'mountpoint')
-        self.assertEqual(3, zfs_driver.time.time.call_count)
+        self.assertEqual(2, zfs_driver.time.sleep.call_count)
-        self.assertEqual(3, zfs_driver.LOG.debug.call_count)
+        self.assertEqual(2, self.driver.execute.call_count)
-        self.assertEqual(5, self.driver.execute.call_count)
+        self.assertEqual(1, zfs_driver.LOG.info.call_count)
-        self.driver.execute.assert_has_calls([
+        self.assertEqual(2, self.driver.zfs.call_count)
-            mock.call('lsof', '-w', mnt),
-            mock.call('lsof', '-w', mnt),
-            mock.call('bash', '-c',
-                      '(echo $(grep -s %s /proc/*/mounts) ) 2>&1 ' % mnt),
-            mock.call('sudo', 'nsenter', '--mnt', '--target=1234',
-                      '/bin/umount', mnt),
-            mock.call('sudo', 'nsenter', '--mnt', '--target=5678',
-                      '/bin/umount', mnt),
-        ])
-        zfs_driver.time.sleep.assert_has_calls([mock.call(2), mock.call(1)])
-        self.driver.zfs.assert_called_once_with('destroy', '-f', dataset_name)
-        zfs_driver.LOG.warning.assert_called_once_with(mock.ANY, mock.ANY)
-        self.driver.zfs_with_retry.asssert_called_once_with(
-            'destroy', '-f', dataset_name)
-
-    def test__delete_dataset_or_snapshot_with_retry_mount_holders_error(self):
-        mnt = 'fake_mnt'
-        self.mock_object(
-            self.driver, 'get_zfs_option', mock.Mock(return_value=mnt))
-        self.mock_object(self.driver, 'zfs_with_retry')
-        self.mock_object(
-            self.driver, 'zfs',
-            mock.Mock(side_effect=exception.ProcessExecutionError('FAKE')))
-        self.mock_object(
-            self.driver, 'execute', mock.Mock(side_effect=[
-                ('a', 'b'),
-                exception.ProcessExecutionError('Fake lsof empty'),
-                exception.ProcessExecutionError('Failed to get list of PIDs'),
-            ]))
-        self.mock_object(zfs_driver.time, 'sleep')
-        self.mock_object(zfs_driver.LOG, 'debug')
-        self.mock_object(zfs_driver.LOG, 'warning')
-        self.mock_object(
-            zfs_driver.time, 'time', mock.Mock(side_effect=range(1, 70, 2)))
-        dataset_name = 'fake/dataset/name'
-
-        self.driver._delete_dataset_or_snapshot_with_retry(dataset_name)
-
-        self.driver.get_zfs_option.assert_called_once_with(
-            dataset_name, 'mountpoint')
-        self.assertEqual(3, zfs_driver.time.time.call_count)
-        self.assertEqual(2, zfs_driver.LOG.debug.call_count)
-        self.assertEqual(3, self.driver.execute.call_count)
-        self.driver.execute.assert_has_calls([
-            mock.call('lsof', '-w', mnt),
-            mock.call('lsof', '-w', mnt),
-            mock.call('bash', '-c',
-                      '(echo $(grep -s %s /proc/*/mounts) ) 2>&1 ' % mnt),
-        ])
-        zfs_driver.time.sleep.assert_has_calls([mock.call(2), mock.call(1)])
-        self.driver.zfs.assert_called_once_with('destroy', '-f', dataset_name)
-        zfs_driver.LOG.warning.assert_called_once_with(mock.ANY, mock.ANY)
-        self.driver.zfs_with_retry.asssert_called_once_with(
-            'destroy', '-f', dataset_name)
 
     def test_create_replica(self):
         active_replica = {

manila/tests/share/drivers/zfsonlinux/test_utils.py

@@ -213,16 +213,6 @@ foo_res  opt_3           some_value       local"""
         self.driver.execute.asssert_called_once_with(
             'sudo', 'zfs', 'foo', 'bar')
 
-    def test_zfs_with_retrying(self):
-        self.mock_object(self.driver, 'execute')
-        self.mock_object(self.driver, 'execute_with_retry')
-
-        self.driver.zfs_with_retry('foo', 'bar')
-
-        self.assertEqual(0, self.driver.execute.call_count)
-        self.driver.execute_with_retry.asssert_called_once_with(
-            'sudo', 'zfs', 'foo', 'bar')
-
 
 @ddt.ddt
 class NFSviaZFSHelperTestCase(test.TestCase):