On Thu, Oct 20, 2022 at 16:59:04 -0500, Jonathon Jongsma wrote:
> For virStorageSource objects that contain an nbdkitProcess, start that
> nbdkit process to serve that network drive and then pass the nbdkit
> socket to qemu rather than sending the network url to qemu directly.
>
> Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> ---
> src/qemu/qemu_block.c | 162 +++++++++++-------
> src/qemu/qemu_domain.c | 21 ++-
> src/qemu/qemu_extdevice.c | 48 ++++++
> src/qemu/qemu_nbdkit.c | 63 +++++++
> src/qemu/qemu_nbdkit.h | 13 ++
> ...sk-cdrom-network-nbdkit.x86_64-latest.args | 42 +++++
> .../disk-cdrom-network-nbdkit.xml | 1 +
> ...isk-network-http-nbdkit.x86_64-latest.args | 45 +++++
> .../disk-network-http-nbdkit.xml | 1 +
> ...rce-curl-nbdkit-backing.x86_64-latest.args | 38 ++++
> ...isk-network-source-curl-nbdkit-backing.xml | 45 +++++
> ...work-source-curl-nbdkit.x86_64-latest.args | 50 ++++++
> .../disk-network-source-curl-nbdkit.xml | 1 +
> ...isk-network-source-curl.x86_64-latest.args | 53 ++++++
> .../disk-network-source-curl.xml | 71 ++++++++
> ...disk-network-ssh-nbdkit.x86_64-latest.args | 36 ++++
> .../disk-network-ssh-nbdkit.xml | 1 +
> tests/qemuxml2argvtest.c | 6 +
> 18 files changed, 627 insertions(+), 70 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/disk-cdrom-network-nbdkit.x86_64-latest.args
> create mode 120000 tests/qemuxml2argvdata/disk-cdrom-network-nbdkit.xml
> create mode 100644 tests/qemuxml2argvdata/disk-network-http-nbdkit.x86_64-latest.args
> create mode 120000 tests/qemuxml2argvdata/disk-network-http-nbdkit.xml
> create mode 100644 tests/qemuxml2argvdata/disk-network-source-curl-nbdkit-backing.x86_64-latest.args
> create mode 100644 tests/qemuxml2argvdata/disk-network-source-curl-nbdkit-backing.xml
> create mode 100644 tests/qemuxml2argvdata/disk-network-source-curl-nbdkit.x86_64-latest.args
> create mode 120000 tests/qemuxml2argvdata/disk-network-source-curl-nbdkit.xml
> create mode 100644 tests/qemuxml2argvdata/disk-network-source-curl.x86_64-latest.args
> create mode 100644 tests/qemuxml2argvdata/disk-network-source-curl.xml
> create mode 100644 tests/qemuxml2argvdata/disk-network-ssh-nbdkit.x86_64-latest.args
> create mode 120000 tests/qemuxml2argvdata/disk-network-ssh-nbdkit.xml
Note that since this commit is most likely misplaced since it enables
nbdkit use before the implementation of passing secrets is done, thus at
this point the feature will be knowingly broken.
You'll need to move it further back after adding the passing of secrets.
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index b82e3311e1..224d468799 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -439,6 +439,32 @@ qemuBlockStorageSourceGetCURLProps(virStorageSource *src,
> }
>
>
> +static virJSONValue *
> +qemuBlockStorageSourceGetNbdkitProps(virStorageSource *src)
> +{
> + qemuDomainStorageSourcePrivate *srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
> + virJSONValue *ret = NULL;
> + g_autoptr(virJSONValue) serverprops = NULL;
> + virStorageNetHostDef host = { .transport = VIR_STORAGE_NET_HOST_TRANS_UNIX };
> +
> + /* srcPriv->nbdkitProcess will already be initialized if we can use nbdkit
> + * to proxy this storage source */
> + if (!(srcPriv && srcPriv->nbdkitProcess))
> + return NULL;
> +
> + host.socket = srcPriv->nbdkitProcess->socketfile;
> + serverprops = qemuBlockStorageSourceBuildJSONSocketAddress(&host);
> +
> + if (!serverprops)
> + return NULL;
> +
> + if (virJSONValueObjectAdd(&ret, "a:server", &serverprops, NULL) < 0)
> + return NULL;
> +
> + return ret;
> +}
> +
> +
> static virJSONValue *
> qemuBlockStorageSourceGetISCSIProps(virStorageSource *src,
> bool onlytarget)
> @@ -851,69 +877,75 @@ qemuBlockStorageSourceGetBackendProps(virStorageSource *src,
I'll try decomposing this function a bit so that special cases such as
this one can be added easily.
In the future we'll need a special case for QSD too.
I'm also starting to think that we'll need a per-storage source
configuration of the used backend, but I'll have to consider to what
granularity we might want to go.
> return NULL;
>
> case VIR_STORAGE_TYPE_NETWORK:
[...]
> @@ -2196,6 +2228,7 @@ qemuBlockStorageSourceCreateGetStorageProps(virStorageSource *src,
> g_autoptr(virJSONValue) location = NULL;
> const char *driver = NULL;
> const char *filename = NULL;
> + qemuDomainStorageSourcePrivate *srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
>
> switch (actualType) {
> case VIR_STORAGE_TYPE_FILE:
> @@ -2224,6 +2257,13 @@ qemuBlockStorageSourceCreateGetStorageProps(virStorageSource *src,
> break;
>
> case VIR_STORAGE_NET_PROTOCOL_SSH:
> + if (srcPriv->nbdkitProcess) {
> + /* disk creation not yet supported with nbdkit, and even if it
> + * was supported, it would not be done with blockdev-create
> + * props */
> + return 0;
Normally we wouldn't be able to do this, but given how extremely clumsy
and not really documented it is to set up SSH disks I think we might be
okay breaking this unused functionality.
Although at this point you should probably propose a separate patch to
disallow this even without nbdkit to prevent any form of regressions in
case ssh gets to become useful.
> + }
> +
> driver = "ssh";
> if (!(location = qemuBlockStorageSourceGetSshProps(src)))
> return -1;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 2ae87392cb..a3fafdd9e3 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -10918,21 +10918,24 @@ qemuDomainPrepareStorageSourceBlockdevNodename(virDomainDiskDef *disk,
> if (qemuDomainSecretStorageSourcePrepareEncryption(priv, src,
> src->nodeformat) < 0)
> return -1;
> - if (qemuDomainSecretStorageSourcePrepareAuth(priv, src,
> - src->nodestorage) < 0)
> - return -1;
>
> if (qemuDomainPrepareStorageSourcePR(src, priv, src->nodestorage) < 0)
> return -1;
>
> - if (qemuDomainPrepareStorageSourceTLS(src, cfg, src->nodestorage,
> - priv) < 0)
> - return -1;
> + if (!qemuDomainPrepareStorageSourceNbdkit(src, cfg, src->nodestorage, priv)) {
> + /* If we're using nbdkit to serve the storage source, we don't pass
> + * authentication secrets to qemu, but will pass them to nbdkit instead */
> + if (qemuDomainSecretStorageSourcePrepareAuth(priv, src,
> + src->nodestorage) < 0)
> + return -1;
>
> - if (qemuDomainPrepareStorageSourceNFS(src) < 0)
> - return -1;
> + if (qemuDomainPrepareStorageSourceTLS(src, cfg, src->nodestorage,
> + priv) < 0)
> + return -1;
>
> - qemuDomainPrepareStorageSourceNbdkit(src, cfg, src->nodestorage, priv);
> + if (qemuDomainPrepareStorageSourceNFS(src) < 0)
> + return -1;
I'm not sure which pattern you've picked here. The persistent
reservations helper is also tied to the protocol layer node, but is not
included in the condition. On the other hand the setup of the NFS
protocol backend is inlcuded here although it's not handled by nbdkit.
> + }
>
> return 0;
> }
> diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
> index 24a57b0f74..4a3d082f1e 100644
> --- a/src/qemu/qemu_extdevice.c
> +++ b/src/qemu/qemu_extdevice.c
> @@ -219,6 +219,17 @@ qemuExtDevicesStart(virQEMUDriver *driver,
> return -1;
> }
>
> + for (i = 0; i < def->ndisks; i++) {
> + virDomainDiskDef *disk = def->disks[i];
> + if (qemuNbdkitStartStorageSource(driver, vm, disk->src) < 0)
> + return -1;
> + }
> +
> + if (def->os.loader && def->os.loader->nvram) {
> + if (qemuNbdkitStartStorageSource(driver, vm, def->os.loader->nvram) < 0)
> + return -1;
> + }
> +
> return 0;
> }
>
> @@ -263,6 +274,14 @@ qemuExtDevicesStop(virQEMUDriver *driver,
> fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS)
> qemuVirtioFSStop(driver, vm, fs);
> }
> +
> + for (i = 0; i < def->ndisks; i++) {
> + virDomainDiskDef *disk = def->disks[i];
> + qemuNbdkitStopStorageSource(disk->src);
> + }
> +
> + if (def->os.loader && def->os.loader->nvram)
> + qemuNbdkitStopStorageSource(def->os.loader->nvram);
> }
>
>
> @@ -292,6 +311,24 @@ qemuExtDevicesHasDevice(virDomainDef *def)
You also need to add appropriate code into qemuExtDevicesHasDevice,
as othewise qemuSetupCgroupForExtDevices() does nothing and all of the
code you are adding below is pointless up to the point when something
else requires and extdevice.
> +/* recursively setup nbdkit cgroups for backing chain of src */
> +static int qemuExtDevicesSetupCgroupNbdkit(virStorageSource *src,
> + virCgroup *cgroup)
> +{
> + qemuDomainStorageSourcePrivate *priv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
> +
> + if (src->backingStore)
> + if (qemuExtDevicesSetupCgroupNbdkit(src->backingStore, cgroup) < 0)
> + return -1;
> +
> + if (priv && priv->nbdkitProcess &&
> + qemuNbdkitProcessSetupCgroup(priv->nbdkitProcess, cgroup) < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +
> int
> qemuExtDevicesSetupCgroup(virQEMUDriver *driver,
> virDomainObj *vm,
> @@ -325,6 +362,17 @@ qemuExtDevicesSetupCgroup(virQEMUDriver *driver,
> return -1;
> }
>
> + for (i = 0; i < def->ndisks; i++) {
> + virDomainDiskDef *disk = def->disks[i];
> + if (qemuExtDevicesSetupCgroupNbdkit(disk->src, cgroup) < 0)
> + return -1;
> + }
> +
> + if (def->os.loader && def->os.loader->nvram) {
> + if (qemuExtDevicesSetupCgroupNbdkit(def->os.loader->nvram, cgroup) < 0)
> + return -1;
[...]
> diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
> index 5b47a15112..8cb9e0e604 100644
> --- a/src/qemu/qemu_nbdkit.c
> +++ b/src/qemu/qemu_nbdkit.c
> @@ -672,6 +672,61 @@ qemuNbdkitInitStorageSource(qemuNbdkitCaps *caps,
> }
>
>
> +static int
> +qemuNbdkitStartStorageSourceOne(virQEMUDriver *driver,
> + virDomainObj *vm,
> + virStorageSource *src)
> +{
> + qemuDomainStorageSourcePrivate *priv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
> +
> + if (priv && priv->nbdkitProcess &&
> + qemuNbdkitProcessStart(priv->nbdkitProcess, vm, driver) < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +
> +/* recursively start nbdkit for backing chain of src */
> +int
> +qemuNbdkitStartStorageSource(virQEMUDriver *driver,
> + virDomainObj *vm,
> + virStorageSource *src)
> +{
> + virStorageSource *backing;
> +
> + for (backing = src->backingStore; backing != NULL; backing = backing->backingStore)
> + if (qemuNbdkitStartStorageSourceOne(driver, vm, backing) < 0)
> + return -1;
> +
> + return qemuNbdkitStartStorageSourceOne(driver, vm, src);
> +}
> +
> +
> +static void
> +qemuNbdkitStopStorageSourceOne(virStorageSource *src)
> +{
> + qemuDomainStorageSourcePrivate *priv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
> +
> + if (priv && priv->nbdkitProcess &&
> + qemuNbdkitProcessStop(priv->nbdkitProcess) < 0)
> + VIR_WARN("Unable to stop nbdkit for storage source '%s'", src->nodestorage);
> +}
[...]
This patch is also missing appropriate additions to
qemuDomainAttachDiskGeneric and qemuDomainRemoveDiskDevice to support
hotplug and hotunplug of disks backed via nbdkit.
[...]
> diff --git a/tests/qemuxml2argvdata/disk-network-ssh-nbdkit.xml b/tests/qemuxml2argvdata/disk-network-ssh-nbdkit.xml
> new file mode 120000
> index 0000000000..b0589bdfb5
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/disk-network-ssh-nbdkit.xml
> @@ -0,0 +1 @@
> +disk-network-ssh.xml
> \ No newline at end of file
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 0032bafdce..60ec42428c 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -1286,6 +1286,7 @@ 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");
> @@ -1326,6 +1327,9 @@ mymain(void)
> /* qemu-6.0 is the last qemu version supporting sheepdog */
> 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);
Note that you can have multiple disks to test multiple scenarios so
there's no need to add special cases for those where backing files are
used.
Preferrably also create the new test files as symlinks of the originals
so that we have equivalent thesting for both code paths.
> DO_TEST_CAPS_LATEST("disk-network-nfs");
> driver.config->vxhsTLS = 1;
> driver.config->nbdTLSx509secretUUID = g_strdup("6fd3f62d-9fe7-4a4e-a869-7acd6376d8ea");
