From 7231f7dee10fa8f9e6cead026f6a5ae3f5b15ae4 Mon Sep 17 00:00:00 2001 From: Silvan Kaiser Date: Mon, 19 Mar 2018 11:26:22 +0100 Subject: [PATCH] Exec systemd-run without --user flag in Quobyte driver Removes the --user flag from the systemd based mount command in the Quobyte driver. This prevents mount failures due to: - Older systemd releases not supporting the --user flag (e.g. CentOS) - Systemd versions having a bug preventing running the --scope and --user flags together - processutils context not allowing mount to run with this flag (see bug referenced below for details) Furthermore the systemd detection is fixed and all mount and umount commands are moved to libvirt privsep. Closes-Bug: #1756823 Change-Id: I53f3a062ce419d1142d7dd3103fab565bb105e05 --- nova/privsep/libvirt.py | 32 ++++ .../unit/virt/libvirt/volume/test_quobyte.py | 160 ++++++++++-------- nova/virt/libvirt/volume/quobyte.py | 46 +++-- .../bug-1756823-fix-d3a999a258019c54.yaml | 5 + 4 files changed, 160 insertions(+), 83 deletions(-) create mode 100644 releasenotes/notes/bug-1756823-fix-d3a999a258019c54.yaml diff --git a/nova/privsep/libvirt.py b/nova/privsep/libvirt.py index 64d46b90d654..b9d97105c00c 100644 --- a/nova/privsep/libvirt.py +++ b/nova/privsep/libvirt.py @@ -255,3 +255,35 @@ def create_mdev(physical_device, mdev_type, uuid=None): with open(fpath, 'w') as f: f.write(uuid) return uuid + + +@nova.privsep.sys_admin_pctxt.entrypoint +def systemd_run_qb_mount(qb_vol, mnt_base, cfg_file=None): + """Mount QB volume in separate CGROUP""" + sysdr_cmd = ['systemd-run', '--scope', 'mount.quobyte', '--disable-xattrs', + qb_vol, mnt_base] + if cfg_file: + sysdr_cmd.extend(['-c', cfg_file]) + return processutils.execute(*sysdr_cmd) + + +# NOTE(kaisers): this method is deliberately not wrapped in a privsep entryp. +def unprivileged_qb_mount(qb_vol, mnt_base, cfg_file=None): + """Mount QB volume""" + mnt_cmd = ['mount.quobyte', '--disable-xattrs', qb_vol, mnt_base] + if cfg_file: + mnt_cmd.extend(['-c', cfg_file]) + return processutils.execute(*mnt_cmd) + + +@nova.privsep.sys_admin_pctxt.entrypoint +def qb_umount(mnt_base): + """Unmount QB volume""" + unprivileged_qb_umount(mnt_base) + + +# NOTE(kaisers): this method is deliberately not wrapped in a privsep entryp. +def unprivileged_qb_umount(mnt_base): + """Unmount QB volume""" + umnt_cmd = ['umount.quobyte', mnt_base] + return processutils.execute(*umnt_cmd) diff --git a/nova/tests/unit/virt/libvirt/volume/test_quobyte.py b/nova/tests/unit/virt/libvirt/volume/test_quobyte.py index e84b9472c342..1cf2e1fc61c8 100644 --- a/nova/tests/unit/virt/libvirt/volume/test_quobyte.py +++ b/nova/tests/unit/virt/libvirt/volume/test_quobyte.py @@ -17,6 +17,7 @@ import os import traceback +import ddt import mock from oslo_concurrency import processutils from oslo_utils import fileutils @@ -24,6 +25,7 @@ import psutil import six from nova import exception as nova_exception +from nova.privsep import libvirt from nova import test from nova.tests.unit.virt.libvirt.volume import test_volume from nova import utils @@ -31,6 +33,7 @@ from nova.virt.libvirt import utils as libvirt_utils from nova.virt.libvirt.volume import quobyte +@ddt.ddt class QuobyteTestCase(test.NoDBTestCase): """Tests the nova.virt.libvirt.volume.quobyte module utilities.""" @@ -59,86 +62,85 @@ class QuobyteTestCase(test.NoDBTestCase): mypart.mountpoint = self.TEST_MNT_POINT return [mypart] + @ddt.data(u"starting", u"running", u"degraded") + @mock.patch.object(psutil.Process, "name", return_value="systemd") + @mock.patch.object(processutils, "execute") + def test_quobyte_is_systemd_ok(self, value, mock_execute, mock_proc): + mock_execute.return_value = [value, "fake stderr"] + + self.assertTrue(quobyte.is_systemd()) + mock_execute.assert_called_once_with("systemctl", + "is-system-running", + check_exit_code=[0, 1]) + mock_proc.assert_called_once() + + @mock.patch.object(psutil.Process, "name", return_value="NOT_systemd") @mock.patch.object(os.path, "exists", return_value=False) - @mock.patch.object(fileutils, "ensure_tree") - @mock.patch('oslo_concurrency.processutils.execute') - def test_quobyte_mount_volume_not_systemd(self, mock_execute, - mock_ensure_tree, - mock_exists): - mnt_base = '/mnt' - quobyte_volume = '192.168.1.1/volume-00001' - export_mnt_base = os.path.join(mnt_base, - utils.get_hash_str(quobyte_volume)) - - quobyte.mount_volume(quobyte_volume, export_mnt_base) - - mock_ensure_tree.assert_called_once_with(export_mnt_base) - expected_commands = [mock.call('mount.quobyte', - '--disable-xattrs', - quobyte_volume, - export_mnt_base) - ] - mock_execute.assert_has_calls(expected_commands) - mock_exists.assert_called_once_with(" /run/systemd/system") + def test_quobyte_is_systemd_not(self, mock_exists, mock_proc): + self.assertFalse(quobyte.is_systemd()) + mock_exists.assert_called_once_with(quobyte.SYSTEMCTL_CHECK_PATH) + mock_proc.assert_called_once_with() + @mock.patch.object(psutil.Process, "name", return_value="NOT_systemd") + @mock.patch.object(processutils, "execute") @mock.patch.object(os.path, "exists", return_value=True) + def test_quobyte_is_systemd_invalid_state(self, mock_exists, mock_execute, + mock_proc): + mock_execute.return_value = ["FAKE_INACCEPTABLE_STATE", "fake stderr"] + + self.assertFalse(quobyte.is_systemd()) + + mock_exists.assert_called_once_with(quobyte.SYSTEMCTL_CHECK_PATH) + mock_execute.assert_called_once_with("systemctl", + "is-system-running", + check_exit_code=[0, 1]) + mock_proc.assert_called_once_with() + + @ddt.data(None, '/some/arbitrary/path') + @mock.patch.object(quobyte, "is_systemd", return_value=False) @mock.patch.object(fileutils, "ensure_tree") - @mock.patch('oslo_concurrency.processutils.execute') - def test_quobyte_mount_volume_systemd(self, mock_execute, - mock_ensure_tree, - mock_exists): + @mock.patch.object(libvirt, "unprivileged_qb_mount") + def test_quobyte_mount_volume_not_systemd(self, value, mock_mount, + mock_ensure_tree, mock_is_sysd): mnt_base = '/mnt' quobyte_volume = '192.168.1.1/volume-00001' export_mnt_base = os.path.join(mnt_base, utils.get_hash_str(quobyte_volume)) - quobyte.mount_volume(quobyte_volume, export_mnt_base) + quobyte.mount_volume(quobyte_volume, export_mnt_base, value) mock_ensure_tree.assert_called_once_with(export_mnt_base) - expected_commands = [mock.call('systemd-run', - '--scope', - '--user', - 'mount.quobyte', - '--disable-xattrs', - quobyte_volume, - export_mnt_base) - ] - mock_execute.assert_has_calls(expected_commands) - mock_exists.assert_called_once_with(" /run/systemd/system") + expected_commands = [mock.call(quobyte_volume, export_mnt_base, + cfg_file=value)] + mock_mount.assert_has_calls(expected_commands) + mock_is_sysd.assert_called_once_with() - @mock.patch.object(os.path, "exists", return_value=False) + @ddt.data(None, '/some/arbitrary/path') + @mock.patch.object(libvirt, 'systemd_run_qb_mount') + @mock.patch.object(quobyte, "is_systemd", return_value=True) @mock.patch.object(fileutils, "ensure_tree") - @mock.patch('oslo_concurrency.processutils.execute') - def test_quobyte_mount_volume_with_config(self, - mock_execute, - mock_ensure_tree, - mock_exists): + def test_quobyte_mount_volume_systemd(self, value, mock_ensure_tree, + mock_exists, mock_privsep_sysdr): mnt_base = '/mnt' quobyte_volume = '192.168.1.1/volume-00001' export_mnt_base = os.path.join(mnt_base, utils.get_hash_str(quobyte_volume)) - config_file_dummy = "/etc/quobyte/dummy.conf" - quobyte.mount_volume(quobyte_volume, - export_mnt_base, - config_file_dummy) + quobyte.mount_volume(quobyte_volume, export_mnt_base, value) mock_ensure_tree.assert_called_once_with(export_mnt_base) - expected_commands = [mock.call('mount.quobyte', - '--disable-xattrs', - quobyte_volume, - export_mnt_base, - '-c', - config_file_dummy) - ] - mock_execute.assert_has_calls(expected_commands) - mock_exists.assert_called_once_with(" /run/systemd/system") + mock_exists.assert_called_once_with() + mock_privsep_sysdr.assert_called_once_with(quobyte_volume, + export_mnt_base, + cfg_file=value) + @mock.patch.object(quobyte, "is_systemd", return_value=False) @mock.patch.object(fileutils, "ensure_tree") - @mock.patch('oslo_concurrency.processutils.execute', - side_effect=(processutils. - ProcessExecutionError)) - def test_quobyte_mount_volume_fails(self, mock_execute, mock_ensure_tree): + @mock.patch.object(libvirt, 'unprivileged_qb_mount', + side_effect=(processutils. + ProcessExecutionError)) + def test_quobyte_mount_volume_fails(self, mock_mount, mock_ensure_tree, + mock_is_sysd): mnt_base = '/mnt' quobyte_volume = '192.168.1.1/volume-00001' export_mnt_base = os.path.join(mnt_base, @@ -148,9 +150,13 @@ class QuobyteTestCase(test.NoDBTestCase): quobyte.mount_volume, quobyte_volume, export_mnt_base) + mock_ensure_tree.assert_called_once_with(export_mnt_base) + mock_is_sysd.assert_called_once() - @mock.patch('oslo_concurrency.processutils.execute') - def test_quobyte_umount_volume(self, mock_execute): + @mock.patch.object(quobyte, "is_systemd", return_value=False) + @mock.patch.object(libvirt, 'unprivileged_qb_umount') + def test_quobyte_umount_volume_non_sysd(self, + mock_lv_umount, mock_is_sysd): mnt_base = '/mnt' quobyte_volume = '192.168.1.1/volume-00001' export_mnt_base = os.path.join(mnt_base, @@ -158,14 +164,29 @@ class QuobyteTestCase(test.NoDBTestCase): quobyte.umount_volume(export_mnt_base) - mock_execute.assert_called_once_with('umount.quobyte', - export_mnt_base) + mock_lv_umount.assert_called_once_with(export_mnt_base) + mock_is_sysd.assert_called_once() + @mock.patch.object(quobyte, "is_systemd", return_value=True) + @mock.patch.object(libvirt, 'qb_umount') + def test_quobyte_umount_volume_sysd(self, mock_lv_umount, mock_exists): + mnt_base = '/mnt' + quobyte_volume = '192.168.1.1/volume-00001' + export_mnt_base = os.path.join(mnt_base, + utils.get_hash_str(quobyte_volume)) + + quobyte.umount_volume(export_mnt_base) + + mock_lv_umount.assert_called_once_with(export_mnt_base) + mock_exists.assert_called_once_with() + + @mock.patch.object(quobyte, "is_systemd", return_value=True) @mock.patch.object(quobyte.LOG, "error") - @mock.patch('oslo_concurrency.processutils.execute') + @mock.patch.object(libvirt, 'qb_umount') def test_quobyte_umount_volume_warns(self, mock_execute, - mock_debug): + mock_debug, + mock_issysd): mnt_base = '/mnt' quobyte_volume = '192.168.1.1/volume-00001' export_mnt_base = os.path.join(mnt_base, @@ -182,13 +203,16 @@ class QuobyteTestCase(test.NoDBTestCase): (mock_debug. assert_called_once_with("The Quobyte volume at %s is still in use.", export_mnt_base)) + mock_issysd.assert_called_once_with() + @mock.patch.object(quobyte, "is_systemd", return_value=True) @mock.patch.object(quobyte.LOG, "exception") - @mock.patch('oslo_concurrency.processutils.execute', - side_effect=(processutils.ProcessExecutionError)) + @mock.patch.object(libvirt, 'qb_umount', + side_effect=(processutils.ProcessExecutionError)) def test_quobyte_umount_volume_fails(self, mock_execute, - mock_exception): + mock_exception, + mock_issysd): mnt_base = '/mnt' quobyte_volume = '192.168.1.1/volume-00001' export_mnt_base = os.path.join(mnt_base, @@ -196,10 +220,12 @@ class QuobyteTestCase(test.NoDBTestCase): quobyte.umount_volume(export_mnt_base) + mock_execute.assert_called_once_with(export_mnt_base) (mock_exception. assert_called_once_with("Couldn't unmount " "the Quobyte Volume at %s", export_mnt_base)) + mock_issysd.assert_called_once_with() @mock.patch.object(psutil, "disk_partitions") @mock.patch.object(os, "stat") diff --git a/nova/virt/libvirt/volume/quobyte.py b/nova/virt/libvirt/volume/quobyte.py index e0885931cdc3..41a11c121100 100644 --- a/nova/virt/libvirt/volume/quobyte.py +++ b/nova/virt/libvirt/volume/quobyte.py @@ -25,6 +25,7 @@ import six import nova.conf from nova import exception as nova_exception from nova.i18n import _ +from nova.privsep import libvirt from nova import utils from nova.virt.libvirt import utils as libvirt_utils from nova.virt.libvirt.volume import fs @@ -37,35 +38,48 @@ SOURCE_PROTOCOL = 'quobyte' SOURCE_TYPE = 'file' DRIVER_CACHE = 'none' DRIVER_IO = 'native' +VALID_SYSD_STATES = ["starting", "running", "degraded"] +SYSTEMCTL_CHECK_PATH = "/run/systemd/system" + + +def is_systemd(): + """Checks if the host is running systemd""" + if psutil.Process(1).name() == "systemd" or os.path.exists( + SYSTEMCTL_CHECK_PATH): + sysdout, sysderr = processutils.execute("systemctl", + "is-system-running", + check_exit_code=[0, 1]) + for state in VALID_SYSD_STATES: + if state in sysdout: + return True + return False def mount_volume(volume, mnt_base, configfile=None): """Wraps execute calls for mounting a Quobyte volume""" fileutils.ensure_tree(mnt_base) - # NOTE(kaisers): disable xattrs to speed up io as this omits - # additional metadata requests in the backend. xattrs can be - # enabled without issues but will reduce performance. - command = ['mount.quobyte', '--disable-xattrs', volume, mnt_base] - if os.path.exists(" /run/systemd/system"): - # Note(kaisers): with systemd this requires a separate CGROUP to - # prevent Nova service stop/restarts from killing the mount. - command = ['systemd-run', '--scope', '--user', 'mount.quobyte', - '--disable-xattrs', volume, mnt_base] - if configfile: - command.extend(['-c', configfile]) + # Note(kaisers): with systemd this requires a separate CGROUP to + # prevent Nova service stop/restarts from killing the mount. + if is_systemd(): + LOG.debug('Mounting volume %s at mount point %s via systemd-run', + volume, mnt_base) + libvirt.systemd_run_qb_mount(volume, mnt_base, cfg_file=configfile) + else: + LOG.debug('Mounting volume %s at mount point %s via mount.quobyte', + volume, mnt_base, cfg_file=configfile) - LOG.debug('Mounting volume %s at mount point %s ...', - volume, - mnt_base) - processutils.execute(*command) + libvirt.unprivileged_qb_mount(volume, mnt_base, cfg_file=configfile) LOG.info('Mounted volume: %s', volume) def umount_volume(mnt_base): """Wraps execute calls for unmouting a Quobyte volume""" try: - processutils.execute('umount.quobyte', mnt_base) + if is_systemd(): + libvirt.qb_umount(mnt_base) + else: + libvirt.unprivileged_qb_umount(mnt_base) except processutils.ProcessExecutionError as exc: if 'Device or resource busy' in six.text_type(exc): LOG.error("The Quobyte volume at %s is still in use.", mnt_base) diff --git a/releasenotes/notes/bug-1756823-fix-d3a999a258019c54.yaml b/releasenotes/notes/bug-1756823-fix-d3a999a258019c54.yaml new file mode 100644 index 000000000000..f9a24e77e60e --- /dev/null +++ b/releasenotes/notes/bug-1756823-fix-d3a999a258019c54.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Fixes a bug causing mount failures on systemd based systems that are + using the systemd-run based mount with the Nova Quobyte driver.