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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org