Migrate LVM driver to privsep

As a part of the community goal [1], we should be replacing
rootwrap in favor of privsep. This change addresses these
changes for the LVM driver. All calls that must run under
sudo are now using privsep.

[1] https://governance.openstack.org/tc/goals/selected/migrate-to-privsep.html

Partially-Implements: bp privsep-migration
Change-Id: Ia76c869f50e2884bcb2ad6e0b150b2dd8aeb2c64

manila/privsep/common.py

@@ -0,0 +1,39 @@
+# Copyright 2021 Red Hat, Inc
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+from manila import exception
+from manila import utils as manila_utils
+from oslo_log import log
+
+LOG = log.getLogger(__name__)
+
+
+def execute_with_retries(action, action_args, max_retries):
+
+    @manila_utils.retry(
+        retry_param=exception.ProcessExecutionError, backoff_rate=2,
+        retries=max_retries)
+    def execute():
+        try:
+            action(*action_args)
+            return True
+        except exception.ProcessExecutionError:
+            LOG.exception("Recovering from a failed execute.")
+            raise
+
+    try:
+        execute()
+    except exception.ProcessExecutionError:
+        LOG.exception("Failed to run command. Tries exhausted.")
+        raise

manila/privsep/filesystem.py

@@ -0,0 +1,36 @@
+# Copyright 2021 Red Hat, Inc
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+"""
+Helpers for filesystem commands
+"""
+
+from oslo_concurrency import processutils
+
+import manila.privsep
+
+
+@manila.privsep.sys_admin_pctxt.entrypoint
+def e2fsck(device_path):
+    return processutils.execute('e2fsck', '-y', '-f', device_path)
+
+
+@manila.privsep.sys_admin_pctxt.entrypoint
+def tune2fs(device_path):
+    return processutils.execute('tune2fs', '-U', 'random', device_path)
+
+
+@manila.privsep.sys_admin_pctxt.entrypoint
+def make_filesystem(ext_version, device_name):
+    return processutils.execute(f'mkfs.{ext_version}', device_name)

manila/privsep/lvm.py

@@ -0,0 +1,78 @@
+# Copyright 2021 Red Hat, Inc
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+"""
+Helpers for lvm related routines
+"""
+
+from oslo_concurrency import processutils
+
+import manila.privsep
+
+
+@manila.privsep.sys_admin_pctxt.entrypoint
+def lvremove(vg_name, lv_name):
+    processutils.execute('lvremove', '-f', f'{vg_name}/{lv_name}')
+
+
+@manila.privsep.sys_admin_pctxt.entrypoint
+def lvcreate(lv_size, lv_name, vg_name, mirrors=0, region_size=0):
+    extra_params = []
+
+    if mirrors:
+        extra_params += ['-m', mirrors, '--nosync']
+    if region_size:
+        extra_params += ['-R', region_size]
+
+    processutils.execute(
+        'lvcreate', '-Wy', '--yes', '-L', f'{lv_size}G', '-n', lv_name,
+        vg_name, *extra_params)
+
+
+@manila.privsep.sys_admin_pctxt.entrypoint
+def lv_snapshot_create(snapshot_size, snap_name, orig_lv_name):
+    size_str = '%sG' % snapshot_size
+    processutils.execute(
+        'lvcreate', '-L', size_str, '--name', snap_name,
+        '--snapshot', orig_lv_name)
+
+
+@manila.privsep.sys_admin_pctxt.entrypoint
+def get_vgs(vg_name):
+    out, err = processutils.execute(
+        'vgs', vg_name, '--rows', '--units', 'g',)
+    return out, err
+
+
+@manila.privsep.sys_admin_pctxt.entrypoint
+def list_vgs_get_name():
+    out, err = processutils.execute('vgs', '--noheadings', '-o', 'name')
+    return out, err
+
+
+@manila.privsep.sys_admin_pctxt.entrypoint
+def lvconvert(vg_name, snapshot_name):
+    processutils.execute(
+        'lvconvert', '--merge', f'{vg_name}/{snapshot_name}')
+
+
+@manila.privsep.sys_admin_pctxt.entrypoint
+def lvrename(vg_name, lv_name, new_name):
+    processutils.execute(
+        'lvrename', vg_name, lv_name, new_name)
+
+
+@manila.privsep.sys_admin_pctxt.entrypoint
+def lvextend(lv_name, new_size):
+    processutils.execute('lvextend', '-L', '%sG' % new_size, '-r', lv_name)

manila/privsep/os.py

@@ -0,0 +1,72 @@
+# Copyright 2021 Red Hat, Inc
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+"""
+Helpers for os basic commands
+"""
+
+from oslo_concurrency import processutils
+
+from manila import exception
+
+import manila.privsep
+
+
+@manila.privsep.sys_admin_pctxt.entrypoint
+def rmdir(dir_path):
+    processutils.execute('rmdir', dir_path)
+
+
+@manila.privsep.sys_admin_pctxt.entrypoint
+def is_data_definition_direct_io_supported(src_str, dest_str):
+    try:
+        processutils.execute(
+            'dd', 'count=0', f'if={src_str}', f'of={dest_str}',
+            'iflag=direct', 'oflag=direct')
+        is_direct_io_supported = True
+    except exception.ProcessExecutionError:
+        is_direct_io_supported = False
+
+    return is_direct_io_supported
+
+
+@manila.privsep.sys_admin_pctxt.entrypoint
+def data_definition(src_str, dest_str, size_in_g, use_direct_io=False):
+    extra_flags = []
+    if use_direct_io:
+        extra_flags += ['iflag=direct', 'oflag=direct']
+    processutils.execute(
+        'dd', 'if=%s' % src_str, 'of=%s' % dest_str, 'count=%d' % size_in_g,
+        'bs=1M', *extra_flags)
+
+
+@manila.privsep.sys_admin_pctxt.entrypoint
+def umount(mount_path):
+    processutils.execute('umount', '-f', mount_path)
+
+
+@manila.privsep.sys_admin_pctxt.entrypoint
+def mount(device_name, mount_path):
+    processutils.execute('mount', device_name, mount_path)
+
+
+@manila.privsep.sys_admin_pctxt.entrypoint
+def list_mounts():
+    out, err = processutils.execute('mount', '-l')
+    return out, err
+
+
+@manila.privsep.sys_admin_pctxt.entrypoint
+def chmod(permission_level_str, mount_path):
+    processutils.execute('chmod', permission_level_str, mount_path)

manila/share/drivers/lvm.py

@@ -23,6 +23,7 @@ import math
 import os
 import re
 
+from oslo_concurrency import processutils
 from oslo_config import cfg
 from oslo_log import log
 from oslo_utils import importutils
@@ -30,6 +31,10 @@ from oslo_utils import timeutils
 
 from manila import exception
 from manila.i18n import _
+from manila.privsep import common as privsep_common
+from manila.privsep import filesystem as privsep_filesystem
+from manila.privsep import lvm as privsep_lvm
+from manila.privsep import os as privsep_os
 from manila.share import driver
 from manila.share.drivers import generic
 from manila.share import utils as share_utils
@@ -67,8 +72,11 @@ CONF.register_opts(generic.share_opts)
 class LVMMixin(driver.ExecuteMixin):
     def check_for_setup_error(self):
         """Returns an error if prerequisites aren't met."""
-        out, err = self._execute('vgs', '--noheadings', '-o', 'name',
+        try:
-                                 run_as_root=True)
+            out, err = privsep_lvm.list_vgs_get_name()
+        except processutils.ProcessExecutionError:
+            msg = _("Failed to get LVM volume group names.")
+            raise exception.ShareBackendException(msg=msg)
         volume_groups = out.split()
         if self.configuration.lvm_share_volume_group not in volume_groups:
             msg = (_("Share volume group %s doesn't exist.")
@@ -81,32 +89,46 @@ class LVMMixin(driver.ExecuteMixin):
 
     def _allocate_container(self, share):
         sizestr = '%sG' % share['size']
-        cmd = ['lvcreate', '-Wy', '--yes', '-L', sizestr, '-n', share['name'],
+        mirrors = 0
-               self.configuration.lvm_share_volume_group]
+        region_size = 0
         if self.configuration.lvm_share_mirrors:
-            cmd += ['-m', self.configuration.lvm_share_mirrors, '--nosync']
+            mirrors = self.configuration.lvm_share_mirrors
             terras = int(sizestr[:-1]) / 1024.0
             if terras >= 1.5:
                 rsize = int(2 ** math.ceil(math.log(terras) / math.log(2)))
                 # NOTE(vish): Next power of two for region size. See:
                 #             http://red.ht/U2BPOD
-                cmd += ['-R', str(rsize)]
+                region_size = str(rsize)
-
+        action_args = [
-        self._try_execute(*cmd, run_as_root=True)
+            share['size'],
+            share['name'],
+            self.configuration.lvm_share_volume_group,
+            mirrors,
+            region_size
+        ]
+        privsep_common.execute_with_retries(
+            privsep_lvm.lvcreate, action_args,
+            self.configuration.num_shell_tries)
         device_name = self._get_local_path(share)
-        self._execute('mkfs.%s' % self.configuration.share_volume_fstype,
+        try:
-                      device_name, run_as_root=True)
+            privsep_filesystem.make_filesystem(
+                self.configuration.share_volume_fstype, device_name)
+        except processutils.ProcessExecutionError:
+            raise
 
     def _extend_container(self, share, device_name, size):
-        cmd = ['lvextend', '-L', '%sG' % size, '-r', device_name]
+        privsep_common.execute_with_retries(
-        self._try_execute(*cmd, run_as_root=True)
+            privsep_lvm.lvextend, [device_name, size],
+            self.configuration.num_shell_tries)
 
     def _deallocate_container(self, share_name):
         """Deletes a logical volume for share."""
         try:
-            self._try_execute('lvremove', '-f', "%s/%s" %
+            action_args = [
-                              (self.configuration.lvm_share_volume_group,
+                self.configuration.lvm_share_volume_group, share_name]
-                               share_name), run_as_root=True)
+            privsep_common.execute_with_retries(
+                privsep_lvm.lvremove, action_args,
+                self.configuration.num_shell_tries)
         except exception.ProcessExecutionError as exc:
             err_pattern = re.compile(".*failed to find.*|.*not found.*",
                                      re.IGNORECASE)
@@ -119,10 +141,11 @@ class LVMMixin(driver.ExecuteMixin):
         """Creates a snapshot."""
         orig_lv_name = "%s/%s" % (self.configuration.lvm_share_volume_group,
                                   snapshot['share_name'])
-        self._try_execute(
+        action_args = [
-            'lvcreate', '-L',
+            snapshot['share']['size'], snapshot['name'], orig_lv_name]
-            '%sG' % snapshot['share']['size'], '--name', snapshot['name'],
+        privsep_common.execute_with_retries(
-            '--snapshot', orig_lv_name, run_as_root=True)
+            privsep_lvm.lv_snapshot_create, action_args,
+            self.configuration.num_shell_tries)
 
         self._set_random_uuid_to_device(snapshot)
 
@@ -135,10 +158,12 @@ class LVMMixin(driver.ExecuteMixin):
         # a recently checked filesystem.
         # See: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=857336
         device_path = self._get_local_path(share_or_snapshot)
-        self._execute('e2fsck', '-y', '-f', device_path, run_as_root=True)
+        try:
-        self._execute(
+            privsep_filesystem.e2fsck(device_path)
-            'tune2fs', '-U', 'random', device_path, run_as_root=True,
+            privsep_filesystem.tune2fs(device_path)
-        )
+        except processutils.ProcessExecutionError:
+            msg = _("Failed to check or modify filesystems.")
+            raise exception.ShareBackendException(msg=msg)
 
     def create_snapshot(self, context, snapshot, share_server=None):
         self._create_snapshot(context, snapshot)
@@ -224,10 +249,12 @@ class LVMShareDriver(LVMMixin, driver.ShareDriver):
         super(LVMShareDriver, self)._update_share_stats(data)
 
     def get_share_server_pools(self, share_server=None):
-        out, err = self._execute('vgs',
+        try:
-                                 self.configuration.lvm_share_volume_group,
+            out, err = privsep_lvm.get_vgs(
-                                 '--rows', '--units', 'g',
+                self.configuration.lvm_share_volume_group)
-                                 run_as_root=True)
+        except processutils.ProcessExecutionError:
+            msg = _("Failed to list LVM Volume Groups.")
+            raise exception.ShareBackendException(msg=msg)
         total_size = re.findall(r"VSize\s[0-9.]+g", out)[0][6:-1]
         free_size = re.findall(r"VFree\s[0-9.]+g", out)[0][6:-1]
         return [{
@@ -279,7 +306,7 @@ class LVMShareDriver(LVMMixin, driver.ShareDriver):
                          retries=retries)
             def _unmount_device_with_retry():
                 try:
-                    self._execute('umount', '-f', mount_path, run_as_root=True)
+                    privsep_os.umount(mount_path)
                 except exception.ProcessExecutionError as exc:
                     if 'is busy' in exc.stderr.lower():
                         raise exception.ShareBusyException(
@@ -294,7 +321,11 @@ class LVMShareDriver(LVMMixin, driver.ShareDriver):
 
             _unmount_device_with_retry()
             # remove dir
-            self._execute('rmdir', mount_path, run_as_root=True)
+            try:
+                privsep_os.rmdir(mount_path)
+            except exception.ProcessExecutionError:
+                msg = _("Failed to remove the directory.")
+                raise exception.ShareBackendException(msg=msg)
 
     def ensure_shares(self, context, shares):
         updates = {}
@@ -362,12 +393,10 @@ class LVMShareDriver(LVMMixin, driver.ShareDriver):
         mount_path = self._get_mount_path(share_or_snapshot)
         self._execute('mkdir', '-p', mount_path)
         try:
-            self._execute('mount', device_name, mount_path,
+            privsep_os.mount(device_name, mount_path)
-                          run_as_root=True, check_exit_code=True)
+            privsep_os.chmod('777', mount_path)
-            self._execute('chmod', '777', mount_path,
-                          run_as_root=True, check_exit_code=True)
         except exception.ProcessExecutionError:
-            out, err = self._execute('mount', '-l', run_as_root=True)
+            out, err = privsep_os.list_mounts()
             if device_name in out:
                 LOG.warning("%s is already mounted", device_name)
             else:
@@ -381,19 +410,18 @@ class LVMShareDriver(LVMMixin, driver.ShareDriver):
 
     def _copy_volume(self, srcstr, deststr, size_in_g):
         # Use O_DIRECT to avoid thrashing the system buffer cache
-        extra_flags = ['iflag=direct', 'oflag=direct']
-
         # Check whether O_DIRECT is supported
-        try:
+        use_direct_io = (
-            self._execute('dd', 'count=0', 'if=%s' % srcstr, 'of=%s' % deststr,
+            privsep_os.is_data_definition_direct_io_supported(srcstr, deststr))
-                          *extra_flags, run_as_root=True)
-        except exception.ProcessExecutionError:
-            extra_flags = []
 
         # Perform the copy
-        self._execute('dd', 'if=%s' % srcstr, 'of=%s' % deststr,
+        try:
-                      'count=%d' % (size_in_g * 1024), 'bs=1M',
+            privsep_os.data_definition(
-                      *extra_flags, run_as_root=True)
+                srcstr, deststr, (size_in_g * 1024),
+                use_direct_io=use_direct_io)
+        except exception.ProcessExecutionError:
+            msg = _("Failed while copying from the snapshot to the share.")
+            raise exception.ShareBackendException(msg=msg)
 
     def extend_share(self, share, new_size, share_server=None):
         device_name = self._get_local_path(share)
@@ -412,9 +440,12 @@ class LVMShareDriver(LVMMixin, driver.ShareDriver):
         # Unmount the share filesystem
         self._unmount_device(share)
         # Merge the snapshot LV back into the share, reverting it
-        snap_lv_name = "%s/%s" % (self.configuration.lvm_share_volume_group,
+        try:
+            privsep_lvm.lvconvert(self.configuration.lvm_share_volume_group,
                                   snapshot['name'])
-        self._execute('lvconvert', '--merge', snap_lv_name, run_as_root=True)
+        except exception.ProcessExecutionError:
+            msg = _('Failed to revert the share to the given snapshot.')
+            raise exception.ShareBackendException(msg=msg)
 
         # Now recreate the snapshot that was destroyed by the merge
         self._create_snapshot(context, snapshot)

manila/tests/share/drivers/test_lvm.py

@@ -25,6 +25,10 @@ from oslo_utils import timeutils
 from manila.common import constants as const
 from manila import context
 from manila import exception
+from manila.privsep import common as privsep_common
+from manila.privsep import filesystem
+from manila.privsep import lvm as privsep_lvm
+from manila.privsep import os as os_routines
 from manila.share import configuration
 from manila.share.drivers import lvm
 from manila import test
@@ -134,29 +138,28 @@ class LVMShareDriverTestCase(test.TestCase):
         ])
 
     def test_check_for_setup_error(self):
-        def exec_runner(*ignore_args, **ignore_kwargs):
+        out, err = '\n   fake1\n   fakevg\n   fake2\n', ''
-            return '\n   fake1\n   fakevg\n   fake2\n', ''
+        self.mock_object(privsep_lvm, 'list_vgs_get_name',
+                         mock.Mock(return_value=(out, err)))
 
-        expected_exec = ['vgs --noheadings -o name']
-        fake_utils.fake_execute_set_repliers([(expected_exec[0], exec_runner)])
         self._driver.check_for_setup_error()
-        self.assertEqual(expected_exec, fake_utils.fake_execute_get_log())
+
+        privsep_lvm.list_vgs_get_name.assert_called_once()
 
     def test_check_for_setup_error_no_vg(self):
-        def exec_runner(*ignore_args, **ignore_kwargs):
+        out = '\n   fake0\n   fake1\n   fake2\n'
-            return '\n   fake0\n   fake1\n   fake2\n', ''
+        err = ''
-
+        self.mock_object(privsep_lvm, 'list_vgs_get_name',
-        fake_utils.fake_execute_set_repliers([('vgs --noheadings -o name',
+                         mock.Mock(return_value=(out, err)))
-                                               exec_runner)])
         self.assertRaises(exception.InvalidParameterValue,
                           self._driver.check_for_setup_error)
 
     def test_check_for_setup_error_no_export_ips(self):
-        def exec_runner(*ignore_args, **ignore_kwargs):
+        out = '\n   fake1\n   fakevg\n   fake2\n'
-            return '\n   fake1\n   fakevg\n   fake2\n', ''
+        err = ''
+        self.mock_object(privsep_lvm, 'list_vgs_get_name',
+                         mock.Mock(return_value=(out, err)))
 
-        fake_utils.fake_execute_set_repliers([('vgs --noheadings -o name',
-                                               exec_runner)])
         CONF.set_default('lvm_share_export_ips', None)
         self.assertRaises(exception.InvalidParameterValue,
                           self._driver.check_for_setup_error)
@@ -176,17 +179,22 @@ class LVMShareDriverTestCase(test.TestCase):
     def test_create_share(self):
         CONF.set_default('lvm_share_mirrors', 0)
         self._driver._mount_device = mock.Mock()
+        lv_create_mock = privsep_lvm.lvcreate = mock.Mock()
+        lv_create_args = [
+            self.share['size'], self.share['name'],
+            CONF.lvm_share_volume_group, 0, 0]
+        self.mock_object(privsep_common, 'execute_with_retries')
+        self.mock_object(filesystem, 'make_filesystem')
 
         ret = self._driver.create_share(self._context, self.share,
                                         self.share_server)
 
         self._driver._mount_device.assert_called_with(
             self.share, '/dev/mapper/fakevg-fakename')
-        expected_exec = [
+        privsep_common.execute_with_retries.assert_called_once_with(
-            'lvcreate -Wy --yes -L 1G -n fakename fakevg',
+            lv_create_mock, lv_create_args, CONF.num_shell_tries)
-            'mkfs.ext4 /dev/mapper/fakevg-fakename',
+        filesystem.make_filesystem.assert_called_once_with(
-        ]
+            'ext4', '/dev/mapper/fakevg-fakename')
-        self.assertEqual(expected_exec, fake_utils.fake_execute_get_log())
         self.assertEqual(self._helper_nfs.create_exports.return_value, ret)
 
     def test_create_share_from_snapshot(self):
@@ -199,6 +207,21 @@ class LVMShareDriverTestCase(test.TestCase):
         mount_share = '/dev/mapper/fakevg-fakename'
         mount_snapshot = '/dev/mapper/fakevg-fakename'
         self._helper_nfs.create_export.return_value = 'fakelocation'
+        lv_create_mock = privsep_lvm.lvcreate = mock.Mock()
+        lv_create_args = [
+            self.share['size'], self.share['name'],
+            CONF.lvm_share_volume_group, 0, 0]
+
+        self.mock_object(privsep_common, 'execute_with_retries')
+        self.mock_object(os_routines, 'is_data_definition_direct_io_supported',
+                         mock.Mock(return_value=True))
+        self.mock_object(os_routines, 'data_definition')
+        self.mock_object(os_routines, 'mount')
+        self.mock_object(os_routines, 'chmod')
+        self.mock_object(filesystem, 'make_filesystem')
+        self.mock_object(filesystem, 'e2fsck')
+        self.mock_object(filesystem, 'tune2fs')
+
         self._driver.create_share_from_snapshot(self._context,
                                                 self.share,
                                                 snapshot_instance,
@@ -206,44 +229,59 @@ class LVMShareDriverTestCase(test.TestCase):
 
         self._driver._mount_device.assert_called_with(self.share,
                                                       mount_snapshot)
-        expected_exec = [
+        privsep_common.execute_with_retries.assert_called_once_with(
-            'lvcreate -Wy --yes -L 1G -n fakename fakevg',
+            lv_create_mock, lv_create_args, 3)
-            'mkfs.ext4 /dev/mapper/fakevg-fakename',
+        filesystem.make_filesystem.assert_called_once_with(
-            'e2fsck -y -f %s' % mount_share,
+            'ext4', '/dev/mapper/fakevg-fakename')
-            'tune2fs -U random %s' % mount_share,
+        filesystem.e2fsck.assert_called_once_with(
-            ("dd count=0 if=%s of=%s iflag=direct oflag=direct" %
+            mount_share)
-             (mount_snapshot, mount_share)),
+        filesystem.tune2fs.assert_called_once_with(
-            ("dd if=%s of=%s count=1024 bs=1M iflag=direct oflag=direct" %
+            mount_share)
-             (mount_snapshot, mount_share)),
+        (os_routines.is_data_definition_direct_io_supported
-        ]
+            .assert_called_once_with(
-        self.assertEqual(expected_exec, fake_utils.fake_execute_get_log())
+                mount_snapshot, mount_share))
+        os_routines.data_definition.assert_called_once_with(
+            mount_snapshot, mount_share, (self.share['size'] * 1024),
+            use_direct_io=True)
 
     def test_create_share_mirrors(self):
         share = fake_share(size='2048')
         CONF.set_default('lvm_share_mirrors', 2)
+        lv_create_mock = privsep_lvm.lvcreate = mock.Mock()
+        lv_create_args = [
+            '2048', self.share['name'],
+            CONF.lvm_share_volume_group, 2, '2']
+
         self._driver._mount_device = mock.Mock()
+        self.mock_object(privsep_common, 'execute_with_retries')
+        self.mock_object(filesystem, 'make_filesystem')
 
         ret = self._driver.create_share(self._context, share,
                                         self.share_server)
 
         self._driver._mount_device.assert_called_with(
             share, '/dev/mapper/fakevg-fakename')
-        expected_exec = [
+        privsep_common.execute_with_retries.assert_called_once_with(
-            'lvcreate -Wy --yes -L 2048G -n fakename fakevg -m 2 --nosync'
+            lv_create_mock, lv_create_args, 3)
-            ' -R 2', 'mkfs.ext4 /dev/mapper/fakevg-fakename']
+        filesystem.make_filesystem.assert_called_once_with(
-        self.assertEqual(expected_exec, fake_utils.fake_execute_get_log())
+            'ext4', '/dev/mapper/fakevg-fakename')
         self.assertEqual(self._helper_nfs.create_exports.return_value, ret)
 
     def test_deallocate_container(self):
-        expected_exec = ['lvremove -f fakevg/fakename']
+        mock_lvremove = privsep_lvm.lvremove = mock.Mock()
+        self.mock_object(privsep_common, 'execute_with_retries')
+
         self._driver._deallocate_container(self.share['name'])
-        self.assertEqual(expected_exec, fake_utils.fake_execute_get_log())
+
+        privsep_common.execute_with_retries.assert_called_once_with(
+            mock_lvremove, [CONF.lvm_share_volume_group, self.share['name']], 3
+        )
 
     def test_deallocate_container_error(self):
         def _fake_exec(*args, **kwargs):
             raise exception.ProcessExecutionError(stderr="error")
 
-        self.mock_object(self._driver, '_try_execute', _fake_exec)
+        self.mock_object(privsep_common, 'execute_with_retries', _fake_exec)
         self.assertRaises(exception.ProcessExecutionError,
                           self._driver._deallocate_container,
                           self.share['name'])
@@ -255,7 +293,7 @@ class LVMShareDriverTestCase(test.TestCase):
         def _fake_exec(*args, **kwargs):
             raise exception.ProcessExecutionError(stderr=error_msg)
 
-        self.mock_object(self._driver, '_try_execute', _fake_exec)
+        self.mock_object(privsep_common, 'execute_with_retries', _fake_exec)
         self._driver._deallocate_container(self.share['name'])
 
     @mock.patch.object(lvm.LVMShareDriver, '_update_share_stats', mock.Mock())
@@ -272,74 +310,94 @@ class LVMShareDriverTestCase(test.TestCase):
         self._driver._update_share_stats.assert_called_once_with()
 
     def test__unmount_device_not_mounted(self):
-        def exec_runner(*ignore_args, **ignore_kwargs):
-            umount_msg = (
-                "umount: /opt/stack/data/manila/mnt/share-fake-share: not "
-                "mounted.\n"
-            )
-            raise exception.ProcessExecutionError(stderr=umount_msg)
-        self._os.path.exists.return_value = True
         mount_path = self._get_mount_path(self.share)
-        expected_exec = "umount -f %s" % (mount_path)
+        error_msg = (
-        fake_utils.fake_execute_set_repliers([(expected_exec, exec_runner)])
+            "umount: /opt/stack/data/manila/mnt/share-fake-share: not "
+            "mounted.\n")
+        umount_exception = exception.ProcessExecutionError(stderr=error_msg)
+        self.mock_object(
+            os_routines, 'umount', mock.Mock(side_effect=umount_exception))
+        self.mock_object(os_routines, 'rmdir')
+        self._os.path.exists.return_value = True
 
         self._driver._unmount_device(self.share, raise_if_missing=False)
 
         self._os.path.exists.assert_called_with(mount_path)
+        os_routines.umount.assert_called_once_with(mount_path)
 
     def test__unmount_device_is_busy_error(self):
-        def exec_runner(*ignore_args, **ignore_kwargs):
+        error_msg = 'device is busy'
-            raise exception.ProcessExecutionError(stderr='device is busy')
+        umount_exception = exception.ProcessExecutionError(stderr=error_msg)
+        self.mock_object(
+            os_routines, 'umount', mock.Mock(side_effect=umount_exception))
         self._os.path.exists.return_value = True
         mount_path = self._get_mount_path(self.share)
-        expected_exec = [
-            "umount -f %s" % (mount_path),
-        ]
-        fake_utils.fake_execute_set_repliers([(expected_exec[0], exec_runner)])
 
         self.assertRaises(exception.ShareBusyException,
                           self._driver._unmount_device,
                           self.share)
-        self.assertEqual(expected_exec, fake_utils.fake_execute_get_log())
+        os_routines.umount.assert_called_once_with(mount_path)
 
     def test__unmount_device_error(self):
-        def exec_runner(*ignore_args, **ignore_kwargs):
+        error_msg = 'fake error'
-            raise exception.ProcessExecutionError(stderr='fake error')
         mount_path = self._get_mount_path(self.share)
+        umount_exception = exception.ProcessExecutionError(stderr=error_msg)
+        self.mock_object(
+            os_routines, 'umount', mock.Mock(side_effect=umount_exception))
         self._os.path.exists.return_value = True
-        cmd = "umount -f %s" % (mount_path)
+
-        fake_utils.fake_execute_set_repliers([(cmd, exec_runner)])
         self.assertRaises(processutils.ProcessExecutionError,
                           self._driver._unmount_device,
                           self.share)
         self._os.path.exists.assert_called_with(mount_path)
+        os_routines.umount.assert_called_once_with(mount_path)
 
     def test__unmount_device_rmdir_error(self):
-        def exec_runner(*ignore_args, **ignore_kwargs):
+        error_msg = 'fake error'
-            raise exception.ProcessExecutionError(stderr='fake error')
         mount_path = self._get_mount_path(self.share)
+        umount_exception = exception.ProcessExecutionError(stderr=error_msg)
+        self.mock_object(os_routines, 'umount')
+        self.mock_object(os_routines, 'rmdir',
+                         mock.Mock(side_effect=umount_exception))
         self._os.path.exists.return_value = True
-        cmd = "rmdir %s" % (mount_path)
+
-        fake_utils.fake_execute_set_repliers([(cmd, exec_runner)])
+        self.assertRaises(exception.ShareBackendException,
-        self.assertRaises(processutils.ProcessExecutionError,
                           self._driver._unmount_device,
                           self.share)
         self._os.path.exists.assert_called_with(mount_path)
+        os_routines.umount.assert_called_once_with(mount_path)
+        os_routines.rmdir.assert_called_once_with(mount_path)
 
     def test_create_snapshot(self):
+        mock_lv_create = privsep_lvm.lvcreate = mock.Mock()
+        orig_lv_name = "%s/%s" % (CONF.lvm_share_volume_group,
+                                  self.snapshot['share_name'])
+        device_path = '/dev/mapper/fakevg-%s' % self.snapshot['name']
+        lv_create_args = [
+            self.snapshot['share']['size'], self.snapshot['share']['name'],
+            orig_lv_name]
+
+        self.mock_object(privsep_common, 'execute_with_retries')
+        self.mock_object(filesystem, 'e2fsck')
+        self.mock_object(filesystem, 'tune2fs')
+        self.mock_object(os_routines, 'mount')
+        self.mock_object(os_routines, 'chmod')
+
         self._driver.create_snapshot(self._context, self.snapshot,
                                      self.share_server)
         mount_path = self._get_mount_path(self.snapshot)
         expected_exec = [
-            ("lvcreate -L 1G --name fakesnapshotname --snapshot "
-             "%s/fakename" % (CONF.lvm_share_volume_group,)),
-            "e2fsck -y -f /dev/mapper/fakevg-%s" % self.snapshot['name'],
-            "tune2fs -U random /dev/mapper/fakevg-%s" % self.snapshot['name'],
             "mkdir -p " + mount_path,
-            "mount /dev/mapper/fakevg-fakesnapshotname " + mount_path,
-            "chmod 777 " + mount_path,
         ]
         self.assertEqual(expected_exec, fake_utils.fake_execute_get_log())
+        privsep_common.execute_with_retries(
+            mock_lv_create, lv_create_args, CONF.num_shell_tries)
+        filesystem.e2fsck.assert_called_once_with(device_path)
+        filesystem.tune2fs.assert_called_once_with(device_path)
+        os_routines.mount.assert_called_once_with(
+            "/dev/mapper/fakevg-fakesnapshotname", mount_path)
+        os_routines.chmod.assert_called_once_with(
+            '777', mount_path)
 
     def test_ensure_share(self):
         device_name = '/dev/mapper/fakevg-fakename'
@@ -360,20 +418,32 @@ class LVMShareDriverTestCase(test.TestCase):
 
     def test_delete_snapshot(self):
         mount_path = self._get_mount_path(self.snapshot)
-        expected_exec = [
+        self.mock_object(os_routines, 'umount')
-            'umount -f %s' % mount_path,
+        self.mock_object(os_routines, 'rmdir')
-            'rmdir %s' % mount_path,
+        self.mock_object(privsep_common, 'execute_with_retries')
-            'lvremove -f fakevg/fakesnapshotname',
+        self.mock_object(self._driver, '_deallocate_container')
-        ]
+
         self._driver.delete_snapshot(self._context, self.snapshot,
                                      self.share_server)
-        self.assertEqual(expected_exec, fake_utils.fake_execute_get_log())
+
+        os_routines.umount.assert_called_once_with(mount_path)
+        os_routines.rmdir.assert_called_once_with(mount_path)
+        self._driver._deallocate_container.assert_called_once_with(
+            self.snapshot['name'])
 
     def test_delete_share_invalid_share(self):
+        self.mock_object(self._driver, '_unmount_device')
+        self.mock_object(self._driver, '_deallocate_container')
         self._driver._get_helper = mock.Mock(
             side_effect=exception.InvalidShare(reason='fake'))
+
         self._driver.delete_share(self._context, self.share, self.share_server)
 
+        self._driver._unmount_device.assert_called_once_with(
+            self.share, raise_if_missing=False, retry_busy_device=True)
+        self._driver._deallocate_container.assert_called_once_with(
+            self.share['name'])
+
     def test_delete_share_process_execution_error(self):
         self.mock_object(
             self._helper_nfs,
@@ -421,13 +491,18 @@ class LVMShareDriverTestCase(test.TestCase):
 
     def test_mount_device(self):
         mount_path = self._get_mount_path(self.share)
-        ret = self._driver._mount_device(self.share, 'fakedevice')
+        self.mock_object(os_routines, 'mount')
+        self.mock_object(os_routines, 'chmod')
         expected_exec = [
             "mkdir -p %s" % (mount_path,),
-            "mount fakedevice %s" % (mount_path,),
-            "chmod 777 %s" % (mount_path,),
         ]
+        device_name = 'fakedevice'
+
+        ret = self._driver._mount_device(self.share, device_name)
+
         self.assertEqual(expected_exec, fake_utils.fake_execute_get_log())
+        os_routines.mount.assert_called_once_with(device_name, mount_path)
+        os_routines.chmod.assert_called_once_with('777', mount_path)
         self.assertEqual(mount_path, ret)
 
     def test_mount_device_already(self):
@@ -438,19 +513,22 @@ class LVMShareDriverTestCase(test.TestCase):
                 return 'fakedevice', ''
 
         self.mock_object(self._driver, '_execute', exec_runner)
+        self.mock_object(os_routines, 'mount')
+        self.mock_object(os_routines, 'chmod')
         mount_path = self._get_mount_path(self.share)
 
         ret = self._driver._mount_device(self.share, 'fakedevice')
         self.assertEqual(mount_path, ret)
 
     def test_mount_device_error(self):
-        def exec_runner(*args, **kwargs):
+        self.mock_object(self._driver, '_execute')
-            if 'mount' in args and '-l' not in args:
+        self.mock_object(
-                raise exception.ProcessExecutionError()
+            os_routines, 'mount',
-            else:
+            mock.Mock(side_effect=exception.ProcessExecutionError))
-                return 'fake', ''
+        self.mock_object(
+            os_routines, 'list_mounts',
+            mock.Mock(return_value=('fake', '')))
 
-        self.mock_object(self._driver, '_execute', exec_runner)
         self.assertRaises(exception.ProcessExecutionError,
                           self._driver._mount_device, self.share, 'fakedevice')
 
@@ -477,8 +555,9 @@ class LVMShareDriverTestCase(test.TestCase):
         ] if retry_busy_device else [None, None]
         mount_path = self._get_mount_path(self.share)
         self._os.path.exists.return_value = True
-        self.mock_object(self._driver, '_execute', mock.Mock(
+        self.mock_object(os_routines, 'umount', mock.Mock(
             side_effect=execute_sideeffects))
+        self.mock_object(os_routines, 'rmdir')
 
         self._driver._unmount_device(self.share,
                                      retry_busy_device=retry_busy_device)
@@ -486,11 +565,9 @@ class LVMShareDriverTestCase(test.TestCase):
         num_of_times_umount_is_called = 3 if retry_busy_device else 1
 
         self._os.path.exists.assert_called_with(mount_path)
-        self._driver._execute.assert_has_calls([
+        os_routines.umount.assert_has_calls([
-            mock.call('umount', '-f', mount_path, run_as_root=True),
+            mock.call(mount_path)] * num_of_times_umount_is_called)
-        ] * num_of_times_umount_is_called + [
+        os_routines.rmdir.assert_called_once_with(mount_path)
-            mock.call('rmdir', mount_path, run_as_root=True)
-        ])
 
     def test_extend_share(self):
         local_path = self._driver._get_local_path(self.share)
@@ -515,15 +592,13 @@ class LVMShareDriverTestCase(test.TestCase):
             'fake_command', run_as_root=True, check_exit_code=True)
 
     def test_extend_container(self):
-        self.mock_object(self._driver, '_try_execute')
+        mock_lvextend = privsep_lvm.lvextend = mock.Mock()
+
+        self.mock_object(privsep_common, 'execute_with_retries')
         self._driver._extend_container(self.share, 'device_name', 3)
-        self._driver._try_execute.assert_called_once_with(
+
-            'lvextend',
+        privsep_common.execute_with_retries.assert_called_once_with(
-            '-L',
+            mock_lvextend, ['device_name', 3], CONF.num_shell_tries)
-            '3G',
-            '-r',
-            'device_name',
-            run_as_root=True)
 
     def test_get_share_server_pools(self):
         expected_result = [{
@@ -533,30 +608,32 @@ class LVMShareDriverTestCase(test.TestCase):
             'reserved_percentage': 0,
             'reserved_snapshot_percentage': 0,
         }, ]
+        out, err = "VSize 33g VFree 22g", None
         self.mock_object(
-            self._driver,
+            privsep_lvm, 'get_vgs', mock.Mock(return_value=(out, err)))
-            '_execute',
-            mock.Mock(return_value=("VSize 33g VFree 22g", None)))
 
         self.assertEqual(expected_result,
                          self._driver.get_share_server_pools())
-        self._driver._execute.assert_called_once_with(
-            'vgs', 'fakevg', '--rows', '--units', 'g', run_as_root=True)
 
-    def test_copy_volume_error(self):
+    @ddt.data(True, False)
-        def _fake_exec(*args, **kwargs):
+    def test_copy_volume_error(self, use_direct_io):
-            if 'count=0' in args:
+        src_str = 'src'
-                raise exception.ProcessExecutionError()
+        dest_str = 'dest'
+        self.mock_object(
+            os_routines, 'is_data_definition_direct_io_supported',
+            mock.Mock(return_value=use_direct_io))
+        self.mock_object(
+            os_routines, 'data_definition',
+            mock.Mock(side_effect=exception.ProcessExecutionError))
 
-        self.mock_object(self._driver, '_execute',
+        self.assertRaises(
-                         mock.Mock(side_effect=_fake_exec))
+            exception.ShareBackendException,
-        self._driver._copy_volume('src', 'dest', 1)
+            self._driver._copy_volume, src_str, dest_str, 1)
-        self._driver._execute.assert_any_call('dd', 'count=0', 'if=src',
+        (os_routines.is_data_definition_direct_io_supported
-                                              'of=dest', 'iflag=direct',
+            .assert_called_once_with(
-                                              'oflag=direct', run_as_root=True)
+                src_str, dest_str))
-        self._driver._execute.assert_any_call('dd', 'if=src', 'of=dest',
+        os_routines.data_definition.assert_called_once_with(
-                                              'count=1024', 'bs=1M',
+            src_str, dest_str, (1 * 1024), use_direct_io=use_direct_io)
-                                              run_as_root=True)
 
     @ddt.data((['1.1.1.1'], 4), (['1001::1001'], 6))
     @ddt.unpack
@@ -576,37 +653,38 @@ class LVMShareDriverTestCase(test.TestCase):
         self.assertEqual(version == 6, self._driver._stats['ipv6_support'])
 
     def test_revert_to_snapshot(self):
-        mock_update_access = self.mock_object(self._helper_nfs,
+        share_local_path = '/dev/mapper/fakevg-fakename'
-                                              'update_access')
+        snapshot_local_path = '/dev/mapper/fakevg-fakesnapshotname'
+        mock_update_access = self.mock_object(
+            self._helper_nfs, 'update_access')
+        mock__unmount_device = self.mock_object(
+            self._driver, '_unmount_device')
+        mock_lvconvert = self.mock_object(privsep_lvm, 'lvconvert')
+        mock_create_snapshot = self.mock_object(
+            self._driver, '_create_snapshot')
+        mock_mount_device = self.mock_object(
+            self._driver, '_mount_device')
+        mock_get_local_path = self.mock_object(
+            self._driver, '_get_local_path',
+            mock.Mock(side_effect=[share_local_path, snapshot_local_path]))
+        snapshot_parent_share = self.snapshot['share']
+
         self._driver.revert_to_snapshot(self._context, self.snapshot,
                                         [], [], self.share_server)
-        snap_lv = "%s/fakesnapshotname" % (CONF.lvm_share_volume_group)
-        share_lv = "%s/fakename" % (CONF.lvm_share_volume_group)
-        share_mount_path = self._get_mount_path(self.snapshot['share'])
-        snapshot_mount_path = self._get_mount_path(self.snapshot)
-        expected_exec = [
-            ('umount -f %s' % snapshot_mount_path),
-            ("rmdir %s" % snapshot_mount_path),
-            ("umount -f %s" % share_mount_path),
-            ("rmdir %s" % share_mount_path),
-            ("lvconvert --merge %s" % snap_lv),
-            ("lvcreate -L 1G --name fakesnapshotname --snapshot %s" %
-                share_lv),
-            ("e2fsck -y -f /dev/mapper/%s-fakesnapshotname" %
-                CONF.lvm_share_volume_group),
-            ("tune2fs -U random /dev/mapper/%s-fakesnapshotname" %
-                CONF.lvm_share_volume_group),
-            ("mkdir -p %s" % share_mount_path),
-            ("mount /dev/mapper/%s-fakename %s" %
-                (CONF.lvm_share_volume_group, share_mount_path)),
-            ("chmod 777 %s" % share_mount_path),
-            ("mkdir -p %s" % snapshot_mount_path),
-            ("mount /dev/mapper/fakevg-fakesnapshotname "
-             "%s" % snapshot_mount_path),
-            ("chmod 777 %s" % snapshot_mount_path),
-        ]
-        self.assertEqual(expected_exec, fake_utils.fake_execute_get_log())
         self.assertEqual(4, mock_update_access.call_count)
+        mock__unmount_device.assert_has_calls(
+            [mock.call(self.snapshot), mock.call(self.snapshot['share'])])
+        mock_lvconvert.assert_called_once_with(
+            CONF.lvm_share_volume_group, self.snapshot['name'])
+        mock_create_snapshot.assert_called_once_with(
+            self._context, self.snapshot)
+        mock_mount_device.assert_has_calls(
+            [mock.call(snapshot_parent_share, share_local_path),
+             mock.call(self.snapshot, snapshot_local_path)]
+        )
+        mock_get_local_path.assert_has_calls(
+            [mock.call(snapshot_parent_share),
+             mock.call(self.snapshot)])
 
     def test_snapshot_update_access(self):
         access_rules = [{