[libvirt PATCH v2] qemu: add runtime config option for nbdkit
Currently when we build with nbdkit support, libvirt will always try to use nbdkit to access remote disk sources when it is available. But without an up-to-date selinux policy allowing this, it will fail. Because the required selinux policies are not yet widely available, we have disabled nbdkit support on rpm builds for all distributions before Fedora 40. Unfortunately, this makes it more difficult to test nbdkit support. After someone updates to the necessary selinux policies, they would also need to rebuild libvirt to enable nbdkit support. By introducing a configure option (storage_use_nbdkit), we can build packages with nbdkit support but have it disabled by default. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Suggested-by: Andrea Bolognani <abologna@redhat.com> --- Responding to one part of Andrea's review on v1:
The main concern I have is how this will be handled for upgrades. At some point we want to make nbdkit the default, right? But that would mean changing how existing installations behave. I guess that is fine in this case? Performing the switch transparently was always the plan after all... I still feel a tiny bit uneasy about that though.
Yes, I feel the same tiny bit of unease. I think it will be OK though. For upgrades, we have to consider three basic cases: 1. user doesn't make any configuration changes - by default nbdkit will not be used. When the default changes in the future, nbdkit will start being used after upgrade. 2. user explicitly enables nbdkit - if the user decides to enable nbdkit explicitly via config file, libvirt will start using nbdkit to serve remote disks. In order for this to work, the user would have had to install a custom selinux policy. When the default changes in the future (because the selinux policy is included downstream) libvirt will continue using nbdkit for remote disks after upgrade 3. user explicitly disables nbdkit - there is very little reason for the user to explicitly disable this configure option until there is a widely-available selinux policy that makes it possible to *enable* the option. I can think of two potential reaons it might be done: A) the user wanted to test the nbdkit feature so they explicitly enabled it in the config file. After finding out that it failed due to missing selinux policies, the user explicitly disabled it in the config file rather than just deleting the configuration option. B) the user doesn't want nbdkit to be used even if the default changes in the future. The only slightly problematic option for upgrades is 3A. If a user disabled the option and then upgraded to a libvirt version that defaulted to enabling nbdkit, that new default would not be used. If the downstream distribution then removed the qemu block drivers for curl and ssh, remote disks may no longer work. But since there's very little reason for a user to explicitly disable this configure option while it is disabled by default, I don't see this as a scenario that is likely to happen to anybody aside from a developer or two. libvirt.spec.in | 18 ++++++++++++++---- meson.build | 4 ++++ meson_options.txt | 3 ++- src/qemu/libvirtd_qemu.aug | 3 +++ src/qemu/meson.build | 2 ++ src/qemu/qemu.conf.in | 10 ++++++++++ src/qemu/qemu_conf.c | 15 +++++++++++++++ src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_domain.c | 3 +++ tests/qemuxml2argvtest.c | 15 +++++++++------ 10 files changed, 64 insertions(+), 11 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index f50c451e73..5107b06677 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -96,6 +96,7 @@ %define with_sanlock 0 %define with_numad 0 %define with_nbdkit 0 +%define with_nbdkit_config_default 0 %define with_firewalld_zone 0 %define with_netcf 0 %define with_libssh2 0 @@ -174,15 +175,17 @@ %endif %endif -# We should only enable nbdkit support if the OS ships a SELinux policy that -# allows libvirt to launch it. Right now that's not the case anywhere, but -# things should be fine by the time Fedora 40 is released. +# We want to build with nbdkit support, but should only enable nbdkit by +# default if the OS ships a SELinux policy that allows libvirt to launch it. +# Right now that's not the case anywhere, but things should be fine by the time +# Fedora 40 is released. # # TODO: add RHEL 9 once a minor release that contains the necessary SELinux # bits exists (we only support the most recent minor release) %if %{with_qemu} + %define with_nbdkit 0%{!?_without_nbdkit:1} %if 0%{?fedora} >= 40 - %define with_nbdkit 0%{!?_without_nbdkit:1} + %define with_nbdkit_config_default 0%{!?_without_nbdkit_config_default:1} %endif %endif @@ -1184,6 +1187,12 @@ exit 1 %define arg_nbdkit -Dnbdkit=disabled %endif +%if %{with_nbdkit_config_default} + %define arg_nbdkit_config_default -Dnbdkit_config_default=true +%else + %define arg_nbdkit_config_default -Dnbdkit_config_default=false +%endif + %if %{with_fuse} %define arg_fuse -Dfuse=enabled %else @@ -1298,6 +1307,7 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y' %{_specdir}/libvirt.spec) %{?arg_sanlock} \ -Dlibpcap=enabled \ %{?arg_nbdkit} \ + %{?arg_nbdkit_config_default} \ -Dlibnl=enabled \ -Daudit=enabled \ -Ddtrace=enabled \ diff --git a/meson.build b/meson.build index 62481a008d..ce8ea1300f 100644 --- a/meson.build +++ b/meson.build @@ -1012,6 +1012,10 @@ if not conf.has('WITH_NBDKIT') libnbd_dep = dependency('', required: false) endif +# default value for storage_use_nbdkit config option +use_nbdkit_default = get_option('nbdkit_config_default') +conf.set10('USE_NBDKIT_DEFAULT', use_nbdkit_default) + libnl_version = '3.0' if not get_option('libnl').disabled() and host_machine.system() == 'linux' libnl_dep = dependency('libnl-3.0', version: '>=' + libnl_version, required: get_option('libnl')) diff --git a/meson_options.txt b/meson_options.txt index a0928102bf..af76cc967e 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -104,7 +104,8 @@ option('loader_nvram', type: 'string', value: '', description: 'Pass list of pai option('login_shell', type: 'feature', value: 'auto', description: 'build virt-login-shell') option('nss', type: 'feature', value: 'auto', description: 'enable Name Service Switch plugin for resolving guest IP addresses') option('numad', type: 'feature', value: 'auto', description: 'use numad to manage CPU placement dynamically') -option('nbdkit', type: 'feature', value: 'auto', description: 'use nbdkit to access network disks') +option('nbdkit', type: 'feature', value: 'auto', description: 'Build nbdkit storage backend') +option('nbdkit_config_default', type: 'boolean', value: 'false', description: 'Whether to use nbdkit storage backend for network disks by default (configurable)') option('pm_utils', type: 'feature', value: 'auto', description: 'use pm-utils for power management') option('sysctl_config', type: 'feature', value: 'auto', description: 'Whether to install sysctl configs') option('tls_priority', type: 'string', value: 'NORMAL', description: 'set the default TLS session priority string') diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index ed097ea3d9..43485b43fb 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -147,6 +147,8 @@ module Libvirtd_qemu = let capability_filters_entry = str_array_entry "capability_filters" + let storage_entry = bool_entry "storage_use_nbdkit" + (* Each entry in the config is one of the following ... *) let entry = default_tls_entry | vnc_entry @@ -170,6 +172,7 @@ module Libvirtd_qemu = | nbd_entry | swtpm_entry | capability_filters_entry + | storage_entry | obsolete_entry let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ] diff --git a/src/qemu/meson.build b/src/qemu/meson.build index 2279fef2ca..4c3e1dee78 100644 --- a/src/qemu/meson.build +++ b/src/qemu/meson.build @@ -137,6 +137,7 @@ if conf.has('WITH_QEMU') qemu_user_group_conf = configuration_data({ 'QEMU_USER': qemu_user, 'QEMU_GROUP': qemu_group, + 'USE_NBDKIT_DEFAULT': use_nbdkit_default.to_int(), }) qemu_conf = configure_file( input: 'qemu.conf.in', @@ -147,6 +148,7 @@ if conf.has('WITH_QEMU') qemu_user_group_hack_conf = configuration_data({ 'QEMU_USER': qemu_user, 'QEMU_GROUP': qemu_group, + 'USE_NBDKIT_DEFAULT': use_nbdkit_default.to_int(), # This hack is necessary because the output file is going to be # used as input for another configure_file() call later, which # will take care of substituting @CONFIG@ with useful data diff --git a/src/qemu/qemu.conf.in b/src/qemu/qemu.conf.in index 6897e0f760..d12ff9f641 100644 --- a/src/qemu/qemu.conf.in +++ b/src/qemu/qemu.conf.in @@ -974,3 +974,13 @@ # "full" - both QEMU and its helper processes are placed into separate # scheduling group #sched_core = "none" + +# Using nbdkit to access remote disk sources +# +# If this is set then libvirt will use nbdkit to access remote disk sources +# when available. nbdkit will export an NBD share to QEMU rather than having +# QEMU attempt to access the remote server directly. +# +# Possible values are 0 or 1. Default value is @USE_NBDKIT_DEFAULT@. +# +# storage_use_nbdkit = @USE_NBDKIT_DEFAULT@ diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 513b5ebb1e..fbc32822ec 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -285,6 +285,7 @@ virQEMUDriverConfig *virQEMUDriverConfigNew(bool privileged, return NULL; cfg->deprecationBehavior = g_strdup("none"); + cfg->storageUseNbdkit = USE_NBDKIT_DEFAULT; return g_steal_pointer(&cfg); } @@ -1065,6 +1066,17 @@ virQEMUDriverConfigLoadCapsFiltersEntry(virQEMUDriverConfig *cfg, } +static int +virQEMUDriverConfigLoadStorageEntry(virQEMUDriverConfig *cfg, + virConf *conf) +{ + if (virConfGetValueBool(conf, "storage_use_nbdkit", &cfg->storageUseNbdkit) < 0) + return -1; + + return 0; +} + + int virQEMUDriverConfigLoadFile(virQEMUDriverConfig *cfg, const char *filename, bool privileged) @@ -1136,6 +1148,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfig *cfg, if (virQEMUDriverConfigLoadCapsFiltersEntry(cfg, conf) < 0) return -1; + if (virQEMUDriverConfigLoadStorageEntry(cfg, conf) < 0) + return -1; + return 0; } diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 1a3ba3a0fb..36049b4bfa 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -230,6 +230,8 @@ struct _virQEMUDriverConfig { char *deprecationBehavior; + bool storageUseNbdkit; + virQEMUSchedCore schedCore; }; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ae19ce884b..f8dda6c898 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10333,6 +10333,9 @@ qemuDomainPrepareStorageSourceNbdkit(virStorageSource *src, { g_autoptr(qemuNbdkitCaps) nbdkit = NULL; + if (!cfg->storageUseNbdkit) + return false; + if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) return false; diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 24997c0aaa..e7a364f897 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1115,7 +1115,6 @@ mymain(void) DO_TEST_CAPS_LATEST("disk-cdrom-empty-network-invalid"); DO_TEST_CAPS_LATEST("disk-cdrom-bus-other"); DO_TEST_CAPS_LATEST("disk-cdrom-network"); - DO_TEST_CAPS_LATEST_NBDKIT("disk-cdrom-network-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_CURL); DO_TEST_CAPS_LATEST("disk-cdrom-tray"); DO_TEST_CAPS_LATEST("disk-floppy"); DO_TEST_CAPS_LATEST("disk-floppy-q35"); @@ -1161,8 +1160,6 @@ mymain(void) DO_TEST_CAPS_VER("disk-network-sheepdog", "6.0.0"); DO_TEST_CAPS_LATEST("disk-network-source-auth"); DO_TEST_CAPS_LATEST("disk-network-source-curl"); - DO_TEST_CAPS_LATEST_NBDKIT("disk-network-source-curl-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_CURL); - DO_TEST_CAPS_LATEST_NBDKIT("disk-network-source-curl-nbdkit-backing", QEMU_NBDKIT_CAPS_PLUGIN_CURL); DO_TEST_CAPS_LATEST("disk-network-nfs"); driver.config->vxhsTLS = 1; driver.config->nbdTLSx509secretUUID = g_strdup("6fd3f62d-9fe7-4a4e-a869-7acd6376d8ea"); @@ -1173,13 +1170,10 @@ mymain(void) DO_TEST_CAPS_LATEST("disk-network-tlsx509-nbd-hostname"); DO_TEST_CAPS_VER("disk-network-tlsx509-vxhs", "5.0.0"); DO_TEST_CAPS_LATEST("disk-network-http"); - DO_TEST_CAPS_LATEST_NBDKIT("disk-network-http-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_CURL); VIR_FREE(driver.config->nbdTLSx509secretUUID); VIR_FREE(driver.config->vxhsTLSx509secretUUID); driver.config->vxhsTLS = 0; DO_TEST_CAPS_LATEST("disk-network-ssh"); - DO_TEST_CAPS_LATEST_NBDKIT("disk-network-ssh-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_SSH); - DO_TEST_CAPS_LATEST_NBDKIT("disk-network-ssh-password", QEMU_NBDKIT_CAPS_PLUGIN_SSH); DO_TEST_CAPS_LATEST("disk-no-boot"); DO_TEST_CAPS_LATEST("disk-nvme"); DO_TEST_CAPS_VER("disk-vhostuser-numa", "4.2.0"); @@ -1249,6 +1243,15 @@ mymain(void) DO_TEST_CAPS_LATEST("disk-geometry"); DO_TEST_CAPS_LATEST("disk-blockio"); + driver.config->storageUseNbdkit = 1; + DO_TEST_CAPS_LATEST_NBDKIT("disk-cdrom-network-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_CURL); + DO_TEST_CAPS_LATEST_NBDKIT("disk-network-source-curl-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_CURL); + DO_TEST_CAPS_LATEST_NBDKIT("disk-network-source-curl-nbdkit-backing", QEMU_NBDKIT_CAPS_PLUGIN_CURL); + DO_TEST_CAPS_LATEST_NBDKIT("disk-network-http-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_CURL); + DO_TEST_CAPS_LATEST_NBDKIT("disk-network-ssh-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_SSH); + DO_TEST_CAPS_LATEST_NBDKIT("disk-network-ssh-password", QEMU_NBDKIT_CAPS_PLUGIN_SSH); + driver.config->storageUseNbdkit = 0; + DO_TEST_CAPS_VER("disk-virtio-scsi-reservations", "5.2.0"); DO_TEST_CAPS_LATEST("disk-virtio-scsi-reservations"); -- 2.41.0
On Wed, Nov 15, 2023 at 01:52:48PM -0600, Jonathon Jongsma wrote:
The main concern I have is how this will be handled for upgrades. At some point we want to make nbdkit the default, right? But that would mean changing how existing installations behave. I guess that is fine in this case? Performing the switch transparently was always the plan after all... I still feel a tiny bit uneasy about that though.
Yes, I feel the same tiny bit of unease. I think it will be OK though. For upgrades, we have to consider three basic cases:
1. user doesn't make any configuration changes - by default nbdkit will not be used. When the default changes in the future, nbdkit will start being used after upgrade.
I assume that you've already verified that the switch from not using nbdkit to using it is transparent, right? As in, if the change happens during a libvirt upgrade existing VMs will not be affected and will continue to use the QEMU backend until they're destroyed and started again; similarly, migrating from a host where nbdkit is disabled to one where it is enabled, or viceversa, works fine.
2. user explicitly enables nbdkit - if the user decides to enable nbdkit explicitly via config file, libvirt will start using nbdkit to serve remote disks. In order for this to work, the user would have had to install a custom selinux policy. When the default changes in the future (because the selinux policy is included downstream) libvirt will continue using nbdkit for remote disks after upgrade
3. user explicitly disables nbdkit - there is very little reason for the user to explicitly disable this configure option until there is a widely-available selinux policy that makes it possible to *enable* the option. I can think of two potential reaons it might be done: A) the user wanted to test the nbdkit feature so they explicitly enabled it in the config file. After finding out that it failed due to missing selinux policies, the user explicitly disabled it in the config file rather than just deleting the configuration option. B) the user doesn't want nbdkit to be used even if the default changes in the future.
The only slightly problematic option for upgrades is 3A. If a user disabled the option and then upgraded to a libvirt version that defaulted to enabling nbdkit, that new default would not be used. If the downstream distribution then removed the qemu block drivers for curl and ssh, remote disks may no longer work. But since there's very little reason for a user to explicitly disable this configure option while it is disabled by default, I don't see this as a scenario that is likely to happen to anybody aside from a developer or two.
Agreed.
+++ b/meson.build @@ -1012,6 +1012,10 @@ if not conf.has('WITH_NBDKIT') +# default value for storage_use_nbdkit config option +use_nbdkit_default = get_option('nbdkit_config_default') +conf.set10('USE_NBDKIT_DEFAULT', use_nbdkit_default)
I think this would work better as a feature instead of a boolean. Right now the sane default would probably still be to keep it off, but later we can make things better by e.g. enabling it by default when AppArmor is used, after we've merged the necessary changes into our AppArmor profile of course. Later still, we will be able to just match the status of the nbdkit option unless explicitly told otherwise. We should also reject senseless combinations such as -Dnbdkit=disabled and -Dnbdkit_config_default=enabled, and mention whether or not nbdkit is enabled by default in the meson configuration summary.
+++ b/src/qemu/qemu.conf.in @@ -974,3 +974,13 @@ +# Using nbdkit to access remote disk sources +# +# If this is set then libvirt will use nbdkit to access remote disk sources +# when available. nbdkit will export an NBD share to QEMU rather than having +# QEMU attempt to access the remote server directly. +# +# Possible values are 0 or 1. Default value is @USE_NBDKIT_DEFAULT@.
Since we're planning to make this enabled by default over time, I think it would be nice to inform users, lest they get caught unprepared. Something as simple as Please note that the default might change in future releases. could do.
+++ b/src/qemu/qemu_domain.c @@ -10333,6 +10333,9 @@ qemuDomainPrepareStorageSourceNbdkit(virStorageSource *src, { g_autoptr(qemuNbdkitCaps) nbdkit = NULL;
+ if (!cfg->storageUseNbdkit) + return false;
This does the right thing if nbdkit support is compiled in but disabled at the configuration file level. However, what about the opposite scenario, where someone tries to enable nbdkit in the configuration file but it's not been compiled in? We'd be silently ignoring an explicit admin request. Perhaps I'm overthinking this, and we don't go to such length for existing options controlling the use of features that can be disabled at compile time. Feel free to ignore this bit of feedback if that's the case. -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Jonathon Jongsma