From 7931f85fe66d4e024d7dd5396dcf38378af85470 Mon Sep 17 00:00:00 2001 From: Silvan Kaiser Date: Wed, 27 Mar 2019 11:47:09 +0100 Subject: [PATCH] Adds systemd detection result caching in Quobyte driver Instead of rechecking for systemd multiple times check only once and cache the result in the Quobyte driver. Furthermore this fixes a range of minor nits like adding comments, generalizing the libvirt privsep umount and clarifying variable names in related tests, as noted in previous change I53f3a062ce419d1142d7dd3103fab565bb105e05 . Last but not least this adds unit tests for newly added functions in privsep.libvirt, using a fixture from change I53b8cca99729bcae6246c79f342f87f55a4ea95c . Related-bug: #1756823 Change-Id: Iba30c49f108af9055de1b1a5c7b1a8406d66cf1a --- nova/privsep/libvirt.py | 13 +-- nova/tests/fixtures.py | 11 +++ nova/tests/unit/privsep/test_libvirt.py | 79 +++++++++++++++++++ .../unit/virt/libvirt/volume/test_quobyte.py | 72 ++++++++--------- nova/virt/libvirt/volume/quobyte.py | 32 ++++++-- 5 files changed, 154 insertions(+), 53 deletions(-) create mode 100644 nova/tests/unit/privsep/test_libvirt.py diff --git a/nova/privsep/libvirt.py b/nova/privsep/libvirt.py index d17fbcf70736..8deecd80b9e1 100644 --- a/nova/privsep/libvirt.py +++ b/nova/privsep/libvirt.py @@ -219,6 +219,7 @@ def create_mdev(physical_device, mdev_type, uuid=None): @nova.privsep.sys_admin_pctxt.entrypoint def systemd_run_qb_mount(qb_vol, mnt_base, cfg_file=None): """Mount QB volume in separate CGROUP""" + # Note(kaisers): Details on why we run without --user at bug #1756823 sysdr_cmd = ['systemd-run', '--scope', 'mount.quobyte', '--disable-xattrs', qb_vol, mnt_base] if cfg_file: @@ -236,13 +237,13 @@ def unprivileged_qb_mount(qb_vol, mnt_base, cfg_file=None): @nova.privsep.sys_admin_pctxt.entrypoint -def qb_umount(mnt_base): - """Unmount QB volume""" - unprivileged_qb_umount(mnt_base) +def umount(mnt_base): + """Unmount volume""" + unprivileged_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] +def unprivileged_umount(mnt_base): + """Unmount volume""" + umnt_cmd = ['umount', mnt_base] return processutils.execute(*umnt_cmd) diff --git a/nova/tests/fixtures.py b/nova/tests/fixtures.py index f16ab3422d7b..297056ae0016 100644 --- a/nova/tests/fixtures.py +++ b/nova/tests/fixtures.py @@ -56,6 +56,7 @@ from nova.network.neutronv2 import constants as neutron_constants from nova import objects from nova.objects import base as obj_base from nova.objects import service as service_obj +from nova import privsep from nova import quota as nova_quota from nova import rpc from nova import service @@ -2039,6 +2040,16 @@ class PrivsepNoHelperFixture(fixtures.Fixture): UnHelperfulClientChannel)) +class PrivsepFixture(fixtures.Fixture): + """Disable real privsep checking so we can test the guts of methods + decorated with sys_admin_pctxt. + """ + def setUp(self): + super(PrivsepFixture, self).setUp() + self.useFixture(fixtures.MockPatchObject( + privsep.sys_admin_pctxt, 'client_mode', False)) + + class NoopQuotaDriverFixture(fixtures.Fixture): """A fixture to run tests using the NoopQuotaDriver. diff --git a/nova/tests/unit/privsep/test_libvirt.py b/nova/tests/unit/privsep/test_libvirt.py new file mode 100644 index 000000000000..3a41692e7db7 --- /dev/null +++ b/nova/tests/unit/privsep/test_libvirt.py @@ -0,0 +1,79 @@ +# Copyright 2019 OpenStack Foundation +# All Rights Reserved. +# +# 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. + +import ddt +import mock + +from nova.privsep import libvirt +from nova import test + + +@ddt.ddt +class PrivsepLibvirtMountTestCase(test.NoDBTestCase): + + QB_BINARY = "mount.quobyte" + QB_FIXED_OPT_1 = "--disable-xattrs" + FAKE_VOLUME = "fake_volume" + FAKE_MOUNT_BASE = "/fake/mount/base" + + def setUp(self): + super(PrivsepLibvirtMountTestCase, self).setUp() + self.useFixture(test.nova_fixtures.PrivsepFixture()) + + @ddt.data(None, "/FAKE/CFG/PATH.cfg") + @mock.patch('oslo_concurrency.processutils.execute') + def test_systemd_run_qb_mount(self, cfg_file, mock_execute): + sysd_bin = "systemd-run" + sysd_opt_1 = "--scope" + + libvirt.systemd_run_qb_mount(self.FAKE_VOLUME, + self.FAKE_MOUNT_BASE, + cfg_file=cfg_file) + + if cfg_file: + mock_execute.assert_called_once_with(sysd_bin, sysd_opt_1, + self.QB_BINARY, + self.QB_FIXED_OPT_1, + self.FAKE_VOLUME, + self.FAKE_MOUNT_BASE, + "-c", + cfg_file) + else: + mock_execute.assert_called_once_with(sysd_bin, sysd_opt_1, + self.QB_BINARY, + self.QB_FIXED_OPT_1, + self.FAKE_VOLUME, + self.FAKE_MOUNT_BASE) + + @ddt.data(None, "/FAKE/CFG/PATH.cfg") + @mock.patch('oslo_concurrency.processutils.execute') + def test_unprivileged_qb_mount(self, cfg_file, mock_execute): + + libvirt.unprivileged_qb_mount(self.FAKE_VOLUME, + self.FAKE_MOUNT_BASE, + cfg_file=cfg_file) + + if cfg_file: + mock_execute.assert_called_once_with(self.QB_BINARY, + self.QB_FIXED_OPT_1, + self.FAKE_VOLUME, + self.FAKE_MOUNT_BASE, + "-c", + cfg_file) + else: + mock_execute.assert_called_once_with(self.QB_BINARY, + self.QB_FIXED_OPT_1, + self.FAKE_VOLUME, + self.FAKE_MOUNT_BASE) diff --git a/nova/tests/unit/virt/libvirt/volume/test_quobyte.py b/nova/tests/unit/virt/libvirt/volume/test_quobyte.py index 1cf2e1fc61c8..a1443f9bbd58 100644 --- a/nova/tests/unit/virt/libvirt/volume/test_quobyte.py +++ b/nova/tests/unit/virt/libvirt/volume/test_quobyte.py @@ -34,6 +34,8 @@ from nova.virt.libvirt.volume import quobyte @ddt.ddt +@mock.patch.object(quobyte, 'found_sysd', False) +@mock.patch.object(quobyte, 'sysd_checked', False) class QuobyteTestCase(test.NoDBTestCase): """Tests the nova.virt.libvirt.volume.quobyte module utilities.""" @@ -73,6 +75,8 @@ class QuobyteTestCase(test.NoDBTestCase): "is-system-running", check_exit_code=[0, 1]) mock_proc.assert_called_once() + self.assertTrue(quobyte.sysd_checked) + self.assertTrue(quobyte.found_sysd) @mock.patch.object(psutil.Process, "name", return_value="NOT_systemd") @mock.patch.object(os.path, "exists", return_value=False) @@ -80,13 +84,15 @@ class QuobyteTestCase(test.NoDBTestCase): self.assertFalse(quobyte.is_systemd()) mock_exists.assert_called_once_with(quobyte.SYSTEMCTL_CHECK_PATH) mock_proc.assert_called_once_with() + self.assertTrue(quobyte.sysd_checked) + self.assertFalse(quobyte.found_sysd) @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"] + mock_execute.return_value = ["FAKE_UNACCEPTABLE_STATE", "fake stderr"] self.assertFalse(quobyte.is_systemd()) @@ -95,52 +101,49 @@ class QuobyteTestCase(test.NoDBTestCase): "is-system-running", check_exit_code=[0, 1]) mock_proc.assert_called_once_with() + self.assertTrue(quobyte.sysd_checked) + self.assertFalse(quobyte.found_sysd) @ddt.data(None, '/some/arbitrary/path') - @mock.patch.object(quobyte, "is_systemd", return_value=False) @mock.patch.object(fileutils, "ensure_tree") @mock.patch.object(libvirt, "unprivileged_qb_mount") - def test_quobyte_mount_volume_not_systemd(self, value, mock_mount, - mock_ensure_tree, mock_is_sysd): + def test_quobyte_mount_volume_not_systemd(self, cfg_file, mock_mount, + mock_ensure_tree): 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, value) + quobyte.mount_volume(quobyte_volume, export_mnt_base, cfg_file) mock_ensure_tree.assert_called_once_with(export_mnt_base) expected_commands = [mock.call(quobyte_volume, export_mnt_base, - cfg_file=value)] + cfg_file=cfg_file)] mock_mount.assert_has_calls(expected_commands) - mock_is_sysd.assert_called_once_with() @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") - def test_quobyte_mount_volume_systemd(self, value, mock_ensure_tree, - mock_exists, mock_privsep_sysdr): + def test_quobyte_mount_volume_systemd(self, cfg_file, mock_ensure_tree, + 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)) + quobyte.found_sysd = True - quobyte.mount_volume(quobyte_volume, export_mnt_base, value) + quobyte.mount_volume(quobyte_volume, export_mnt_base, cfg_file) mock_ensure_tree.assert_called_once_with(export_mnt_base) - mock_exists.assert_called_once_with() mock_privsep_sysdr.assert_called_once_with(quobyte_volume, export_mnt_base, - cfg_file=value) + cfg_file=cfg_file) - @mock.patch.object(quobyte, "is_systemd", return_value=False) @mock.patch.object(fileutils, "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): + def test_quobyte_mount_volume_fails(self, mock_mount, mock_ensure_tree): mnt_base = '/mnt' quobyte_volume = '192.168.1.1/volume-00001' export_mnt_base = os.path.join(mnt_base, @@ -151,12 +154,9 @@ class QuobyteTestCase(test.NoDBTestCase): quobyte_volume, export_mnt_base) mock_ensure_tree.assert_called_once_with(export_mnt_base) - mock_is_sysd.assert_called_once() - @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): + @mock.patch.object(libvirt, 'unprivileged_umount') + def test_quobyte_umount_volume_non_sysd(self, mock_lv_umount): mnt_base = '/mnt' quobyte_volume = '192.168.1.1/volume-00001' export_mnt_base = os.path.join(mnt_base, @@ -165,32 +165,27 @@ class QuobyteTestCase(test.NoDBTestCase): quobyte.umount_volume(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): + @mock.patch.object(libvirt, 'umount') + def test_quobyte_umount_volume_sysd(self, mock_lv_umount): 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.found_sysd = True 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.object(libvirt, 'qb_umount') - def test_quobyte_umount_volume_warns(self, - mock_execute, - mock_debug, - mock_issysd): + @mock.patch.object(libvirt, 'umount') + def test_quobyte_umount_volume_warns(self, mock_execute, mock_debug): 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.found_sysd = True def exec_side_effect(*cmd, **kwargs): exerror = processutils.ProcessExecutionError( @@ -203,29 +198,24 @@ 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.object(libvirt, 'qb_umount', + @mock.patch.object(libvirt, 'umount', side_effect=(processutils.ProcessExecutionError)) - def test_quobyte_umount_volume_fails(self, - mock_execute, - mock_exception, - mock_issysd): + def test_quobyte_umount_volume_fails(self, mock_unmount, mock_exception): 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.found_sysd = True quobyte.umount_volume(export_mnt_base) - mock_execute.assert_called_once_with(export_mnt_base) + mock_unmount.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 41a11c121100..7c7724c5ff48 100644 --- a/nova/virt/libvirt/volume/quobyte.py +++ b/nova/virt/libvirt/volume/quobyte.py @@ -41,17 +41,29 @@ DRIVER_IO = 'native' VALID_SYSD_STATES = ["starting", "running", "degraded"] SYSTEMCTL_CHECK_PATH = "/run/systemd/system" +sysd_checked = False +found_sysd = False + def is_systemd(): """Checks if the host is running systemd""" + global sysd_checked + global found_sysd if psutil.Process(1).name() == "systemd" or os.path.exists( SYSTEMCTL_CHECK_PATH): + # NOTE(kaisers): exit code might be >1 in theory but in practice this + # is hard coded to 1. Due to backwards compatibility and systemd + # CODING_STYLE this is unlikely to change. 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 + if state == sysdout.strip(): + found_sysd = True + sysd_checked = True + return found_sysd + + sysd_checked = True return False @@ -61,7 +73,7 @@ def mount_volume(volume, mnt_base, configfile=None): # Note(kaisers): with systemd this requires a separate CGROUP to # prevent Nova service stop/restarts from killing the mount. - if is_systemd(): + if found_sysd: 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) @@ -76,10 +88,10 @@ def mount_volume(volume, mnt_base, configfile=None): def umount_volume(mnt_base): """Wraps execute calls for unmouting a Quobyte volume""" try: - if is_systemd(): - libvirt.qb_umount(mnt_base) + if found_sysd: + libvirt.umount(mnt_base) else: - libvirt.unprivileged_qb_umount(mnt_base) + libvirt.unprivileged_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) @@ -119,6 +131,9 @@ def validate_volume(mount_path): class LibvirtQuobyteVolumeDriver(fs.LibvirtBaseFileSystemVolumeDriver): """Class implements libvirt part of volume driver for Quobyte.""" + def __init__(self, host): + super(LibvirtQuobyteVolumeDriver, self).__init__(host) + def _get_mount_point_base(self): return CONF.libvirt.quobyte_mount_point_base @@ -139,6 +154,11 @@ class LibvirtQuobyteVolumeDriver(fs.LibvirtBaseFileSystemVolumeDriver): @utils.synchronized('connect_qb_volume') def connect_volume(self, connection_info, instance): """Connect the volume.""" + if is_systemd(): + LOG.debug("systemd detected.") + else: + LOG.debug("No systemd detected.") + data = connection_info['data'] quobyte_volume = self._normalize_export(data['export']) mount_path = self._get_mount_path(connection_info)