[libvirt PATCH] 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 as an option for making testing easier by Andrea Bolognani libvirt.spec.in | 10 +--------- src/qemu/libvirtd_qemu.aug | 3 +++ src/qemu/qemu.conf.in | 10 ++++++++++ src/qemu/qemu_conf.c | 14 ++++++++++++++ src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_domain.c | 3 +++ tests/qemuxml2argvtest.c | 15 +++++++++------ 7 files changed, 42 insertions(+), 15 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index f50c451e73..e2ba245ade 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -174,16 +174,8 @@ %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. -# -# 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} - %if 0%{?fedora} >= 40 - %define with_nbdkit 0%{!?_without_nbdkit:1} - %endif + %define with_nbdkit 0%{!?_without_nbdkit:1} %endif %ifarch %{arches_dmidecode} 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/qemu.conf.in b/src/qemu/qemu.conf.in index 6897e0f760..1017edefa5 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 access the remote server directly. +# +# Possible values are 0 or 1. Disabled by default. +# +# storage_use_nbdkit = 1 diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 513b5ebb1e..b5c0ca10b4 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1065,6 +1065,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 +1147,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 4fda68a4ce..3c64fcc7eb 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 08, 2023 at 03:39:22PM -0600, Jonathon Jongsma wrote:
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 as an option for making testing easier by Andrea Bolognani
This is what the Suggested-by tag exists for ;) Anyway, this looks reasonable. In addition to making life easier for those testing the SELinux (and AppArmor!) policy changes during this transitional period, making it possible for the local admin to opt out of nbdkit usage sounds like it could be useful in at least some scenarios. 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.
+++ b/libvirt.spec.in -# 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. -# -# 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} - %if 0%{?fedora} >= 40 - %define with_nbdkit 0%{!?_without_nbdkit:1} - %endif + %define with_nbdkit 0%{!?_without_nbdkit:1} %endif
Once the updated SELinux policy hits Fedora 40 (if it hasn't already) we're going to want nbdkit support to be enabled by default (not just available) there, no? So I think we need something along the lines of %if %{with_qemu} %define with_nbdkit 0%{!?_without_nbdkit:1} %if 0%{?fedora} >= 40 # and later RHEL 9 too %define with_nbdkit_enabled 0%{!?_without_nbdkit_enabled:1} %endif %endif
+++ b/src/qemu/qemu.conf.in +# 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 access the remote server directly. +# +# Possible values are 0 or 1. Disabled by default. +# +# storage_use_nbdkit = 1
... and then to reflect the actual compile-time default here. There's precedent for that, see @QEMU_USER@ and @QEMU_GROUP@. Also s/qemu/QEMU/g in the comment :)
+++ b/src/qemu/qemu_conf.h @@ -230,6 +230,8 @@ struct _virQEMUDriverConfig { + bool storageUseNbdkit;
This works fine as long as the default is false, but in order to make it possible to change it via a compile-time knob virQEMUDriverConfigNew() will need to be updated too. -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Jonathon Jongsma