Re: [PATCH] qemu: implement QEMU NBD source reconnect delay attribute

[Jonathon Jongsma <jjongsma@xxxxxxxxxx>]

On 1/27/23 8:40 AM, Christian Nautze wrote:
> Hi!
>
> Is it possible to get a review on this  patch I send before Christmas? :)
> Thank you!
>
> Christian
>
> _____________________________________________________
>
>    Currently it's only possible to set this parameter during domain
>      creation via QEMU commandline passthrough feature.
>      With the new delay attribute it's also possible to set this
>      parameter if you want to attach a new NBD disk
>      using "virsh attach-device domain device.xml" e.g.:
>
>        <disk type='network' device='disk'>
>          <driver name='qemu' type='raw'/>
>          <source protocol='nbd' name='foo'>
>            <host name='example.org  <http://example.org>' port='6000'/>
>            <reconnect enabled='yes' delay='10'/>

In my opinion, this doesn't seem like the ideal syntax for specifying 
this parameter. For example, it allows you to specify something like

 <reconnect enabled='no' ... />

which looks like it would disable any attempt by qemu to reconnect to 
the nbd export. But in your code, this just results in not passing any 
value for 'reconnect-delay' to qemu. And I'm pretty sure qemu will still 
attempt to reconnect to the nbd server in this case, but you will lose 
all of the requests made between the disconnection and a successful 
reconnection.

Jonathon


>          </source>
>          <target dev='vdb' bus='virtio'/>
>        </disk>
>
> Signed-off-by: Christian Nautze <christian.nautze at exoscale.ch  <https://listman.redhat.com/mailman/listinfo/libvir-list>>
> ---
>   docs/formatdomain.rst                         | 10 ++++++--
>   src/conf/domain_conf.c                        | 19 +++++++++++++++
>   src/conf/schemas/domaincommon.rng             |  3 +++
>   src/conf/schemas/storagecommon.rng            |  5 ++++
>   src/conf/storage_source_conf.c                |  1 +
>   src/conf/storage_source_conf.h                |  4 ++++
>   src/qemu/qemu_block.c                         |  4 +++-
>   src/qemu/qemu_domain.c                        |  9 ++++++++
>   .../disk-network-nbd.x86_64-latest.args       | 23 +++++++++++--------
>   tests/qemuxml2argvdata/disk-network-nbd.xml   |  8 +++++++
>   tests/qemuxml2xmloutdata/disk-network-nbd.xml |  9 ++++++++
>   11 files changed, 82 insertions(+), 13 deletions(-)
>
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index d7fffc6e0b..3fbeba644a 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -2938,13 +2938,19 @@ paravirtualized driver is specified via the ``disk`` element.
>         are intended to be default, then the entire element may be omitted.
>      ``reconnect``
>         For disk type ``vhostuser`` configures reconnect timeout if the connection
> -      is lost. It has two mandatory attributes:
> +      is lost. This is set with the two mandatory attributes ``enabled`` and
> +      ``timeout``. For disk type ``network`` and protocol ``nbd`` the QEMU NBD
> +      reconnect delay can be set via the mandatory attributes ``enabled``
> +      and ``delay``:
>   
>         ``enabled``
>            If the reconnect feature is enabled, accepts ``yes`` and ``no``
>         ``timeout``
>            The amount of seconds after which hypervisor tries to reconnect.
> -
> +      ``delay``
> +         The amount of seconds during which all requests are paused and will be rerun
> +         after a successful reconnect. After that time, any delayed requests and all
> +         future requests before a successful reconnect will immediately fail.
>   
>      For a "file" or "volume" disk type which represents a cdrom or floppy (the
>      ``device`` attribute), it is possible to define policy what to do with the
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6c088ff295..909c78ef28 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -7024,6 +7024,22 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
>           src->tlsFromConfig = !!value;
>       }
>   
> +    if (src->protocol == VIR_STORAGE_NET_PROTOCOL_NBD) {
> +        xmlNodePtr cur;
> +        if ((cur = virXPathNode("./reconnect", ctxt))) {
> +            virTristateBool enabled;
> +            if (virXMLPropTristateBool(cur, "enabled", VIR_XML_PROP_NONE,
> +                                        &enabled) < 0)
> +                return -1;
> +
> +            if (enabled == VIR_TRISTATE_BOOL_YES) {
> +                if (virXMLPropUInt(cur, "delay", 10, VIR_XML_PROP_REQUIRED,
> +                                    &src->reconnectDelay) < 0)
> +                    return -1;
> +            }
> +        }
> +    }
> +
>       /* for historical reasons we store the volume and image name in one XML
>        * element although it complicates thing when attempting to access them. */
>       if (src->path &&
> @@ -21729,6 +21745,9 @@ virDomainDiskSourceFormatNetwork(virBuffer *attrBuf,
>           virBufferAddLit(childBuf, "/>\n");
>       }
>   
> +    if (src->reconnectDelay) {
> +        virBufferAsprintf(childBuf, "<reconnect enabled='yes' delay='%u'/>\n", src->reconnectDelay);
> +    }
>   
>       virBufferEscapeString(childBuf, "<snapshot name='%s'/>\n", src->snapshot);
>       virBufferEscapeString(childBuf, "<config file='%s'/>\n", src->configFile);
> diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng
> index c588a48fd2..72587416fe 100644
> --- a/src/conf/schemas/domaincommon.rng
> +++ b/src/conf/schemas/domaincommon.rng
> @@ -2184,6 +2184,9 @@
>           </optional>
>           <ref name="diskSourceCommon"/>
>           <ref name="diskSourceNetworkHost"/>
> +        <optional>
> +          <ref name="reconnect"/>
> +        </optional>
>           <optional>
>             <ref name="encryption"/>
>           </optional>
> diff --git a/src/conf/schemas/storagecommon.rng b/src/conf/schemas/storagecommon.rng
> index 76714c9aad..2842376e78 100644
> --- a/src/conf/schemas/storagecommon.rng
> +++ b/src/conf/schemas/storagecommon.rng
> @@ -61,6 +61,11 @@
>             <ref name="unsignedInt"/>
>           </attribute>
>         </optional>
> +      <optional>
> +        <attribute name="delay">
> +          <ref name="unsignedInt"/>
> +        </attribute>
> +      </optional>
>       </element>
>     </define>
>   
> diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c
> index 2b4cf5e241..edfd17c77c 100644
> --- a/src/conf/storage_source_conf.c
> +++ b/src/conf/storage_source_conf.c
> @@ -810,6 +810,7 @@ virStorageSourceCopy(const virStorageSource *src,
>       def->sslverify = src->sslverify;
>       def->readahead = src->readahead;
>       def->timeout = src->timeout;
> +    def->reconnectDelay = src->reconnectDelay;
>       def->metadataCacheMaxSize = src->metadataCacheMaxSize;
>   
>       /* storage driver metadata are not copied */
> diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h
> index f2440cec6a..b60a4b3346 100644
> --- a/src/conf/storage_source_conf.h
> +++ b/src/conf/storage_source_conf.h
> @@ -290,6 +290,10 @@ struct _virStorageSource {
>       unsigned long long readahead; /* size of the readahead buffer in bytes */
>       unsigned long long timeout; /* connection timeout in seconds */
>   
> +    /* NBD QEMU reconnect-delay option,
> +     * 0 as default value */
> +    unsigned int reconnectDelay;
> +
>       virStorageSourceNVMeDef *nvme; /* type == VIR_STORAGE_TYPE_NVME */
>   
>       virDomainChrSourceDef *vhostuser; /* type == VIR_STORAGE_TYPE_VHOST_USER */
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index 8a6f601b29..6913c380a0 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -530,6 +530,7 @@ qemuBlockStorageSourceGetNBDProps(virStorageSource *src,
>                                 "S:export", src->path,
>                                 "S:tls-creds", tlsAlias,
>                                 "S:tls-hostname", tlsHostname,
> +                              "p:reconnect-delay", src->reconnectDelay,
>                                 NULL) < 0)
>           return NULL;
>   
> @@ -1817,7 +1818,8 @@ qemuBlockGetBackingStoreString(virStorageSource *src,
>               src->ncookies == 0 &&
>               src->sslverify == VIR_TRISTATE_BOOL_ABSENT &&
>               src->timeout == 0 &&
> -            src->readahead == 0) {
> +            src->readahead == 0 &&
> +            src->reconnectDelay == 0) {
>   
>               switch ((virStorageNetProtocol) src->protocol) {
>               case VIR_STORAGE_NET_PROTOCOL_NBD:
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 5c05032ce3..840d857f78 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4927,6 +4927,15 @@ qemuDomainValidateStorageSource(virStorageSource *src,
>           }
>       }
>   
> +    if (src->reconnectDelay > 0) {
> +        if (actualType != VIR_STORAGE_TYPE_NETWORK ||
> +            src->protocol != VIR_STORAGE_NET_PROTOCOL_NBD) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("reconnect delay is supported only with NBD protocol"));
> +            return -1;
> +        }
> +    }
> +
>       if (src->query &&
>           (actualType != VIR_STORAGE_TYPE_NETWORK ||
>            (src->protocol != VIR_STORAGE_NET_PROTOCOL_HTTPS &&
> diff --git a/tests/qemuxml2argvdata/disk-network-nbd.x86_64-latest.args b/tests/qemuxml2argvdata/disk-network-nbd.x86_64-latest.args
> index 21e619af3e..e8d13b0bd4 100644
> --- a/tests/qemuxml2argvdata/disk-network-nbd.x86_64-latest.args
> +++ b/tests/qemuxml2argvdata/disk-network-nbd.x86_64-latest.args
> @@ -28,21 +28,24 @@ XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
>   -no-acpi \
>   -boot strict=on \
>   -device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \
> --blockdev '{"driver":"nbd","server":{"type":"inet","host":"example.org  <http://example.org>","port":"6000"},"node-name":"libvirt-5-storage","auto-read-only":true,"discard":"unmap"}' \
> +-blockdev '{"driver":"nbd","server":{"type":"inet","host":"example.org  <http://example.org>","port":"6000"},"node-name":"libvirt-6-storage","auto-read-only":true,"discard":"unmap"}' \
> +-blockdev '{"node-name":"libvirt-6-format","read-only":false,"driver":"raw","file":"libvirt-6-storage"}' \
> +-device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x2","drive":"libvirt-6-format","id":"virtio-disk0","bootindex":1}' \
> +-blockdev '{"driver":"nbd","server":{"type":"inet","host":"example.org  <http://example.org>","port":"6000"},"export":"bar","node-name":"libvirt-5-storage","auto-read-only":true,"discard":"unmap"}' \
>   -blockdev '{"node-name":"libvirt-5-format","read-only":false,"driver":"raw","file":"libvirt-5-storage"}' \
> --device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x2","drive":"libvirt-5-format","id":"virtio-disk0","bootindex":1}' \
> --blockdev '{"driver":"nbd","server":{"type":"inet","host":"example.org  <http://example.org>","port":"6000"},"export":"bar","node-name":"libvirt-4-storage","auto-read-only":true,"discard":"unmap"}' \
> +-device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x3","drive":"libvirt-5-format","id":"virtio-disk1"}' \
> +-blockdev '{"driver":"nbd","server":{"type":"inet","host":"::1","port":"6000"},"node-name":"libvirt-4-storage","auto-read-only":true,"discard":"unmap"}' \
>   -blockdev '{"node-name":"libvirt-4-format","read-only":false,"driver":"raw","file":"libvirt-4-storage"}' \
> --device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x3","drive":"libvirt-4-format","id":"virtio-disk1"}' \
> --blockdev '{"driver":"nbd","server":{"type":"inet","host":"::1","port":"6000"},"node-name":"libvirt-3-storage","auto-read-only":true,"discard":"unmap"}' \
> +-device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x4","drive":"libvirt-4-format","id":"virtio-disk2"}' \
> +-blockdev '{"driver":"nbd","server":{"type":"inet","host":"::1","port":"6000"},"export":"bar","node-name":"libvirt-3-storage","auto-read-only":true,"discard":"unmap"}' \
>   -blockdev '{"node-name":"libvirt-3-format","read-only":false,"driver":"raw","file":"libvirt-3-storage"}' \
> --device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x4","drive":"libvirt-3-format","id":"virtio-disk2"}' \
> --blockdev '{"driver":"nbd","server":{"type":"inet","host":"::1","port":"6000"},"export":"bar","node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \
> +-device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x5","drive":"libvirt-3-format","id":"virtio-disk3"}' \
> +-blockdev '{"driver":"nbd","server":{"type":"unix","path":"/var/run/nbdsock"},"export":"bar","node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \
>   -blockdev '{"node-name":"libvirt-2-format","read-only":false,"driver":"raw","file":"libvirt-2-storage"}' \
> --device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x5","drive":"libvirt-2-format","id":"virtio-disk3"}' \
> --blockdev '{"driver":"nbd","server":{"type":"unix","path":"/var/run/nbdsock"},"export":"bar","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \
> +-device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x6","drive":"libvirt-2-format","id":"virtio-disk4"}' \
> +-blockdev '{"driver":"nbd","server":{"type":"inet","host":"example.org  <http://example.org>","port":"6000"},"export":"foo","reconnect-delay":10,"node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \
>   -blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}' \
> --device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x6","drive":"libvirt-1-format","id":"virtio-disk4"}' \
> +-device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x7","drive":"libvirt-1-format","id":"virtio-disk5"}' \
>   -audiodev '{"id":"audio1","driver":"none"}' \
>   -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
>   -msg timestamp=on
> diff --git a/tests/qemuxml2argvdata/disk-network-nbd.xml b/tests/qemuxml2argvdata/disk-network-nbd.xml
> index 8ac6cc3b7b..243ffeb1ed 100644
> --- a/tests/qemuxml2argvdata/disk-network-nbd.xml
> +++ b/tests/qemuxml2argvdata/disk-network-nbd.xml
> @@ -49,6 +49,14 @@
>         </source>
>         <target dev='vde' bus='virtio'/>
>       </disk>
> +    <disk type='network' device='disk'>
> +      <driver name='qemu' type='raw'/>
> +      <source protocol='nbd' name='foo'>
> +        <host name='example.org  <http://example.org>' port='6000'/>
> +        <reconnect enabled='yes' delay='10'/>
> +      </source>
> +      <target dev='vdf' bus='virtio'/>
> +    </disk>
>       <controller type='usb' index='0'/>
>       <controller type='ide' index='0'/>
>       <controller type='pci' index='0' model='pci-root'/>
> diff --git a/tests/qemuxml2xmloutdata/disk-network-nbd.xml b/tests/qemuxml2xmloutdata/disk-network-nbd.xml
> index f8dcca4bab..ed0c760d5b 100644
> --- a/tests/qemuxml2xmloutdata/disk-network-nbd.xml
> +++ b/tests/qemuxml2xmloutdata/disk-network-nbd.xml
> @@ -54,6 +54,15 @@
>         <target dev='vde' bus='virtio'/>
>         <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/>
>       </disk>
> +    <disk type='network' device='disk'>
> +      <driver name='qemu' type='raw'/>
> +      <source protocol='nbd' name='foo'>
> +        <host name='example.org  <http://example.org>' port='6000'/>
> +        <reconnect enabled='yes' delay='10'/>
> +      </source>
> +      <target dev='vdf' bus='virtio'/>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/>
> +    </disk>
>       <controller type='usb' index='0'>
>         <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
>       </controller>
> --
> 2.34.1