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
This commit is contained in:
Silvan Kaiser 2019-03-27 11:47:09 +01:00 committed by Eric Fried
parent f130b295bb
commit 7931f85fe6
5 changed files with 154 additions and 53 deletions

View File

@ -219,6 +219,7 @@ def create_mdev(physical_device, mdev_type, uuid=None):
@nova.privsep.sys_admin_pctxt.entrypoint @nova.privsep.sys_admin_pctxt.entrypoint
def systemd_run_qb_mount(qb_vol, mnt_base, cfg_file=None): def systemd_run_qb_mount(qb_vol, mnt_base, cfg_file=None):
"""Mount QB volume in separate CGROUP""" """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', sysdr_cmd = ['systemd-run', '--scope', 'mount.quobyte', '--disable-xattrs',
qb_vol, mnt_base] qb_vol, mnt_base]
if cfg_file: if cfg_file:
@ -236,13 +237,13 @@ def unprivileged_qb_mount(qb_vol, mnt_base, cfg_file=None):
@nova.privsep.sys_admin_pctxt.entrypoint @nova.privsep.sys_admin_pctxt.entrypoint
def qb_umount(mnt_base): def umount(mnt_base):
"""Unmount QB volume""" """Unmount volume"""
unprivileged_qb_umount(mnt_base) unprivileged_umount(mnt_base)
# NOTE(kaisers): this method is deliberately not wrapped in a privsep entryp. # NOTE(kaisers): this method is deliberately not wrapped in a privsep entryp.
def unprivileged_qb_umount(mnt_base): def unprivileged_umount(mnt_base):
"""Unmount QB volume""" """Unmount volume"""
umnt_cmd = ['umount.quobyte', mnt_base] umnt_cmd = ['umount', mnt_base]
return processutils.execute(*umnt_cmd) return processutils.execute(*umnt_cmd)

View File

@ -56,6 +56,7 @@ from nova.network.neutronv2 import constants as neutron_constants
from nova import objects from nova import objects
from nova.objects import base as obj_base from nova.objects import base as obj_base
from nova.objects import service as service_obj from nova.objects import service as service_obj
from nova import privsep
from nova import quota as nova_quota from nova import quota as nova_quota
from nova import rpc from nova import rpc
from nova import service from nova import service
@ -2039,6 +2040,16 @@ class PrivsepNoHelperFixture(fixtures.Fixture):
UnHelperfulClientChannel)) 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): class NoopQuotaDriverFixture(fixtures.Fixture):
"""A fixture to run tests using the NoopQuotaDriver. """A fixture to run tests using the NoopQuotaDriver.

View File

@ -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)

View File

@ -34,6 +34,8 @@ from nova.virt.libvirt.volume import quobyte
@ddt.ddt @ddt.ddt
@mock.patch.object(quobyte, 'found_sysd', False)
@mock.patch.object(quobyte, 'sysd_checked', False)
class QuobyteTestCase(test.NoDBTestCase): class QuobyteTestCase(test.NoDBTestCase):
"""Tests the nova.virt.libvirt.volume.quobyte module utilities.""" """Tests the nova.virt.libvirt.volume.quobyte module utilities."""
@ -73,6 +75,8 @@ class QuobyteTestCase(test.NoDBTestCase):
"is-system-running", "is-system-running",
check_exit_code=[0, 1]) check_exit_code=[0, 1])
mock_proc.assert_called_once() 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(psutil.Process, "name", return_value="NOT_systemd")
@mock.patch.object(os.path, "exists", return_value=False) @mock.patch.object(os.path, "exists", return_value=False)
@ -80,13 +84,15 @@ class QuobyteTestCase(test.NoDBTestCase):
self.assertFalse(quobyte.is_systemd()) self.assertFalse(quobyte.is_systemd())
mock_exists.assert_called_once_with(quobyte.SYSTEMCTL_CHECK_PATH) mock_exists.assert_called_once_with(quobyte.SYSTEMCTL_CHECK_PATH)
mock_proc.assert_called_once_with() 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(psutil.Process, "name", return_value="NOT_systemd")
@mock.patch.object(processutils, "execute") @mock.patch.object(processutils, "execute")
@mock.patch.object(os.path, "exists", return_value=True) @mock.patch.object(os.path, "exists", return_value=True)
def test_quobyte_is_systemd_invalid_state(self, mock_exists, mock_execute, def test_quobyte_is_systemd_invalid_state(self, mock_exists, mock_execute,
mock_proc): 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()) self.assertFalse(quobyte.is_systemd())
@ -95,52 +101,49 @@ class QuobyteTestCase(test.NoDBTestCase):
"is-system-running", "is-system-running",
check_exit_code=[0, 1]) check_exit_code=[0, 1])
mock_proc.assert_called_once_with() mock_proc.assert_called_once_with()
self.assertTrue(quobyte.sysd_checked)
self.assertFalse(quobyte.found_sysd)
@ddt.data(None, '/some/arbitrary/path') @ddt.data(None, '/some/arbitrary/path')
@mock.patch.object(quobyte, "is_systemd", return_value=False)
@mock.patch.object(fileutils, "ensure_tree") @mock.patch.object(fileutils, "ensure_tree")
@mock.patch.object(libvirt, "unprivileged_qb_mount") @mock.patch.object(libvirt, "unprivileged_qb_mount")
def test_quobyte_mount_volume_not_systemd(self, value, mock_mount, def test_quobyte_mount_volume_not_systemd(self, cfg_file, mock_mount,
mock_ensure_tree, mock_is_sysd): mock_ensure_tree):
mnt_base = '/mnt' mnt_base = '/mnt'
quobyte_volume = '192.168.1.1/volume-00001' quobyte_volume = '192.168.1.1/volume-00001'
export_mnt_base = os.path.join(mnt_base, export_mnt_base = os.path.join(mnt_base,
utils.get_hash_str(quobyte_volume)) 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) mock_ensure_tree.assert_called_once_with(export_mnt_base)
expected_commands = [mock.call(quobyte_volume, 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_mount.assert_has_calls(expected_commands)
mock_is_sysd.assert_called_once_with()
@ddt.data(None, '/some/arbitrary/path') @ddt.data(None, '/some/arbitrary/path')
@mock.patch.object(libvirt, 'systemd_run_qb_mount') @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.object(fileutils, "ensure_tree")
def test_quobyte_mount_volume_systemd(self, value, mock_ensure_tree, def test_quobyte_mount_volume_systemd(self, cfg_file, mock_ensure_tree,
mock_exists, mock_privsep_sysdr): mock_privsep_sysdr):
mnt_base = '/mnt' mnt_base = '/mnt'
quobyte_volume = '192.168.1.1/volume-00001' quobyte_volume = '192.168.1.1/volume-00001'
export_mnt_base = os.path.join(mnt_base, export_mnt_base = os.path.join(mnt_base,
utils.get_hash_str(quobyte_volume)) 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_ensure_tree.assert_called_once_with(export_mnt_base)
mock_exists.assert_called_once_with()
mock_privsep_sysdr.assert_called_once_with(quobyte_volume, mock_privsep_sysdr.assert_called_once_with(quobyte_volume,
export_mnt_base, 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(fileutils, "ensure_tree")
@mock.patch.object(libvirt, 'unprivileged_qb_mount', @mock.patch.object(libvirt, 'unprivileged_qb_mount',
side_effect=(processutils. side_effect=(processutils.
ProcessExecutionError)) ProcessExecutionError))
def test_quobyte_mount_volume_fails(self, mock_mount, mock_ensure_tree, def test_quobyte_mount_volume_fails(self, mock_mount, mock_ensure_tree):
mock_is_sysd):
mnt_base = '/mnt' mnt_base = '/mnt'
quobyte_volume = '192.168.1.1/volume-00001' quobyte_volume = '192.168.1.1/volume-00001'
export_mnt_base = os.path.join(mnt_base, export_mnt_base = os.path.join(mnt_base,
@ -151,12 +154,9 @@ class QuobyteTestCase(test.NoDBTestCase):
quobyte_volume, quobyte_volume,
export_mnt_base) export_mnt_base)
mock_ensure_tree.assert_called_once_with(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_umount')
@mock.patch.object(libvirt, 'unprivileged_qb_umount') def test_quobyte_umount_volume_non_sysd(self, mock_lv_umount):
def test_quobyte_umount_volume_non_sysd(self,
mock_lv_umount, mock_is_sysd):
mnt_base = '/mnt' mnt_base = '/mnt'
quobyte_volume = '192.168.1.1/volume-00001' quobyte_volume = '192.168.1.1/volume-00001'
export_mnt_base = os.path.join(mnt_base, export_mnt_base = os.path.join(mnt_base,
@ -165,32 +165,27 @@ class QuobyteTestCase(test.NoDBTestCase):
quobyte.umount_volume(export_mnt_base) quobyte.umount_volume(export_mnt_base)
mock_lv_umount.assert_called_once_with(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, 'umount')
@mock.patch.object(libvirt, 'qb_umount') def test_quobyte_umount_volume_sysd(self, mock_lv_umount):
def test_quobyte_umount_volume_sysd(self, mock_lv_umount, mock_exists):
mnt_base = '/mnt' mnt_base = '/mnt'
quobyte_volume = '192.168.1.1/volume-00001' quobyte_volume = '192.168.1.1/volume-00001'
export_mnt_base = os.path.join(mnt_base, export_mnt_base = os.path.join(mnt_base,
utils.get_hash_str(quobyte_volume)) utils.get_hash_str(quobyte_volume))
quobyte.found_sysd = True
quobyte.umount_volume(export_mnt_base) quobyte.umount_volume(export_mnt_base)
mock_lv_umount.assert_called_once_with(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(quobyte.LOG, "error")
@mock.patch.object(libvirt, 'qb_umount') @mock.patch.object(libvirt, 'umount')
def test_quobyte_umount_volume_warns(self, def test_quobyte_umount_volume_warns(self, mock_execute, mock_debug):
mock_execute,
mock_debug,
mock_issysd):
mnt_base = '/mnt' mnt_base = '/mnt'
quobyte_volume = '192.168.1.1/volume-00001' quobyte_volume = '192.168.1.1/volume-00001'
export_mnt_base = os.path.join(mnt_base, export_mnt_base = os.path.join(mnt_base,
utils.get_hash_str(quobyte_volume)) utils.get_hash_str(quobyte_volume))
quobyte.found_sysd = True
def exec_side_effect(*cmd, **kwargs): def exec_side_effect(*cmd, **kwargs):
exerror = processutils.ProcessExecutionError( exerror = processutils.ProcessExecutionError(
@ -203,29 +198,24 @@ class QuobyteTestCase(test.NoDBTestCase):
(mock_debug. (mock_debug.
assert_called_once_with("The Quobyte volume at %s is still in use.", assert_called_once_with("The Quobyte volume at %s is still in use.",
export_mnt_base)) 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(quobyte.LOG, "exception")
@mock.patch.object(libvirt, 'qb_umount', @mock.patch.object(libvirt, 'umount',
side_effect=(processutils.ProcessExecutionError)) side_effect=(processutils.ProcessExecutionError))
def test_quobyte_umount_volume_fails(self, def test_quobyte_umount_volume_fails(self, mock_unmount, mock_exception):
mock_execute,
mock_exception,
mock_issysd):
mnt_base = '/mnt' mnt_base = '/mnt'
quobyte_volume = '192.168.1.1/volume-00001' quobyte_volume = '192.168.1.1/volume-00001'
export_mnt_base = os.path.join(mnt_base, export_mnt_base = os.path.join(mnt_base,
utils.get_hash_str(quobyte_volume)) utils.get_hash_str(quobyte_volume))
quobyte.found_sysd = True
quobyte.umount_volume(export_mnt_base) 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. (mock_exception.
assert_called_once_with("Couldn't unmount " assert_called_once_with("Couldn't unmount "
"the Quobyte Volume at %s", "the Quobyte Volume at %s",
export_mnt_base)) export_mnt_base))
mock_issysd.assert_called_once_with()
@mock.patch.object(psutil, "disk_partitions") @mock.patch.object(psutil, "disk_partitions")
@mock.patch.object(os, "stat") @mock.patch.object(os, "stat")

View File

@ -41,17 +41,29 @@ DRIVER_IO = 'native'
VALID_SYSD_STATES = ["starting", "running", "degraded"] VALID_SYSD_STATES = ["starting", "running", "degraded"]
SYSTEMCTL_CHECK_PATH = "/run/systemd/system" SYSTEMCTL_CHECK_PATH = "/run/systemd/system"
sysd_checked = False
found_sysd = False
def is_systemd(): def is_systemd():
"""Checks if the host is running systemd""" """Checks if the host is running systemd"""
global sysd_checked
global found_sysd
if psutil.Process(1).name() == "systemd" or os.path.exists( if psutil.Process(1).name() == "systemd" or os.path.exists(
SYSTEMCTL_CHECK_PATH): 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", sysdout, sysderr = processutils.execute("systemctl",
"is-system-running", "is-system-running",
check_exit_code=[0, 1]) check_exit_code=[0, 1])
for state in VALID_SYSD_STATES: for state in VALID_SYSD_STATES:
if state in sysdout: if state == sysdout.strip():
return True found_sysd = True
sysd_checked = True
return found_sysd
sysd_checked = True
return False return False
@ -61,7 +73,7 @@ def mount_volume(volume, mnt_base, configfile=None):
# Note(kaisers): with systemd this requires a separate CGROUP to # Note(kaisers): with systemd this requires a separate CGROUP to
# prevent Nova service stop/restarts from killing the mount. # 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', LOG.debug('Mounting volume %s at mount point %s via systemd-run',
volume, mnt_base) volume, mnt_base)
libvirt.systemd_run_qb_mount(volume, mnt_base, cfg_file=configfile) 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): def umount_volume(mnt_base):
"""Wraps execute calls for unmouting a Quobyte volume""" """Wraps execute calls for unmouting a Quobyte volume"""
try: try:
if is_systemd(): if found_sysd:
libvirt.qb_umount(mnt_base) libvirt.umount(mnt_base)
else: else:
libvirt.unprivileged_qb_umount(mnt_base) libvirt.unprivileged_umount(mnt_base)
except processutils.ProcessExecutionError as exc: except processutils.ProcessExecutionError as exc:
if 'Device or resource busy' in six.text_type(exc): if 'Device or resource busy' in six.text_type(exc):
LOG.error("The Quobyte volume at %s is still in use.", mnt_base) 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 LibvirtQuobyteVolumeDriver(fs.LibvirtBaseFileSystemVolumeDriver):
"""Class implements libvirt part of volume driver for Quobyte.""" """Class implements libvirt part of volume driver for Quobyte."""
def __init__(self, host):
super(LibvirtQuobyteVolumeDriver, self).__init__(host)
def _get_mount_point_base(self): def _get_mount_point_base(self):
return CONF.libvirt.quobyte_mount_point_base return CONF.libvirt.quobyte_mount_point_base
@ -139,6 +154,11 @@ class LibvirtQuobyteVolumeDriver(fs.LibvirtBaseFileSystemVolumeDriver):
@utils.synchronized('connect_qb_volume') @utils.synchronized('connect_qb_volume')
def connect_volume(self, connection_info, instance): def connect_volume(self, connection_info, instance):
"""Connect the volume.""" """Connect the volume."""
if is_systemd():
LOG.debug("systemd detected.")
else:
LOG.debug("No systemd detected.")
data = connection_info['data'] data = connection_info['data']
quobyte_volume = self._normalize_export(data['export']) quobyte_volume = self._normalize_export(data['export'])
mount_path = self._get_mount_path(connection_info) mount_path = self._get_mount_path(connection_info)