<div dir="ltr"><div>Thank you, I see your point. We could just ignore the 'enabled' attribute from the reconnect element</div><div>when it's used in the NBD source context:</div><div><br></div><div><span class="gmail-im"><source protocol='nbd' name='foo'></span><br></div><div> <host name='<a href="http://example.org" rel="noreferrer" target="_blank">example.org</a> <<a href="http://example.org" rel="noreferrer" target="_blank">http://example.org</a>>' port='6000'/><span class="gmail-im"><br> <reconnect delay='10'/><br>
</source></span></div><div><br></div><div>Another solution would be do add a totally new XML element.<br></div><div>What would be your preferred solution?<br></div><div><br></div><div><span id="gmail-hs_cos_wrapper_post_body" class="gmail-hs_cos_wrapper gmail-hs_cos_wrapper_meta_field gmail-hs_cos_wrapper_type_rich_text">Thanks again<br></span></div><div>Christian<br></div><div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 27 Jan 2023 at 23:42, Jonathon Jongsma <<a href="mailto:jjongsma@redhat.com">jjongsma@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 1/27/23 8:40 AM, Christian Nautze wrote:<br>
> Hi!<br>
> <br>
> Is it possible to get a review on this patch I send before Christmas? :)<br>
> Thank you!<br>
> <br>
> Christian<br>
> <br>
> _____________________________________________________<br>
> <br>
> Currently it's only possible to set this parameter during domain<br>
> creation via QEMU commandline passthrough feature.<br>
> With the new delay attribute it's also possible to set this<br>
> parameter if you want to attach a new NBD disk<br>
> using "virsh attach-device domain device.xml" e.g.:<br>
> <br>
> <disk type='network' device='disk'><br>
> <driver name='qemu' type='raw'/><br>
> <source protocol='nbd' name='foo'><br>
> <host name='<a href="http://example.org" rel="noreferrer" target="_blank">example.org</a> <<a href="http://example.org" rel="noreferrer" target="_blank">http://example.org</a>>' port='6000'/><br>
> <reconnect enabled='yes' delay='10'/><br>
<br>
In my opinion, this doesn't seem like the ideal syntax for specifying <br>
this parameter. For example, it allows you to specify something like<br>
<br>
<reconnect enabled='no' ... /><br>
<br>
which looks like it would disable any attempt by qemu to reconnect to <br>
the nbd export. But in your code, this just results in not passing any <br>
value for 'reconnect-delay' to qemu. And I'm pretty sure qemu will still <br>
attempt to reconnect to the nbd server in this case, but you will lose <br>
all of the requests made between the disconnection and a successful <br>
reconnection.<br>
<br>
Jonathon<br>
<br>
<br>
> </source><br>
> <target dev='vdb' bus='virtio'/><br>
> </disk><br>
> <br>
> Signed-off-by: Christian Nautze <christian.nautze at <a href="http://exoscale.ch" rel="noreferrer" target="_blank">exoscale.ch</a> <<a href="https://listman.redhat.com/mailman/listinfo/libvir-list" rel="noreferrer" target="_blank">https://listman.redhat.com/mailman/listinfo/libvir-list</a>>><br>
> ---<br>
> docs/formatdomain.rst | 10 ++++++--<br>
> src/conf/domain_conf.c | 19 +++++++++++++++<br>
> src/conf/schemas/domaincommon.rng | 3 +++<br>
> src/conf/schemas/storagecommon.rng | 5 ++++<br>
> src/conf/storage_source_conf.c | 1 +<br>
> src/conf/storage_source_conf.h | 4 ++++<br>
> src/qemu/qemu_block.c | 4 +++-<br>
> src/qemu/qemu_domain.c | 9 ++++++++<br>
> .../disk-network-nbd.x86_64-latest.args | 23 +++++++++++--------<br>
> tests/qemuxml2argvdata/disk-network-nbd.xml | 8 +++++++<br>
> tests/qemuxml2xmloutdata/disk-network-nbd.xml | 9 ++++++++<br>
> 11 files changed, 82 insertions(+), 13 deletions(-)<br>
> <br>
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst<br>
> index d7fffc6e0b..3fbeba644a 100644<br>
> --- a/docs/formatdomain.rst<br>
> +++ b/docs/formatdomain.rst<br>
> @@ -2938,13 +2938,19 @@ paravirtualized driver is specified via the ``disk`` element.<br>
> are intended to be default, then the entire element may be omitted.<br>
> ``reconnect``<br>
> For disk type ``vhostuser`` configures reconnect timeout if the connection<br>
> - is lost. It has two mandatory attributes:<br>
> + is lost. This is set with the two mandatory attributes ``enabled`` and<br>
> + ``timeout``. For disk type ``network`` and protocol ``nbd`` the QEMU NBD<br>
> + reconnect delay can be set via the mandatory attributes ``enabled``<br>
> + and ``delay``:<br>
> <br>
> ``enabled``<br>
> If the reconnect feature is enabled, accepts ``yes`` and ``no``<br>
> ``timeout``<br>
> The amount of seconds after which hypervisor tries to reconnect.<br>
> -<br>
> + ``delay``<br>
> + The amount of seconds during which all requests are paused and will be rerun<br>
> + after a successful reconnect. After that time, any delayed requests and all<br>
> + future requests before a successful reconnect will immediately fail.<br>
> <br>
> For a "file" or "volume" disk type which represents a cdrom or floppy (the<br>
> ``device`` attribute), it is possible to define policy what to do with the<br>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c<br>
> index 6c088ff295..909c78ef28 100644<br>
> --- a/src/conf/domain_conf.c<br>
> +++ b/src/conf/domain_conf.c<br>
> @@ -7024,6 +7024,22 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,<br>
> src->tlsFromConfig = !!value;<br>
> }<br>
> <br>
> + if (src->protocol == VIR_STORAGE_NET_PROTOCOL_NBD) {<br>
> + xmlNodePtr cur;<br>
> + if ((cur = virXPathNode("./reconnect", ctxt))) {<br>
> + virTristateBool enabled;<br>
> + if (virXMLPropTristateBool(cur, "enabled", VIR_XML_PROP_NONE,<br>
> + &enabled) < 0)<br>
> + return -1;<br>
> +<br>
> + if (enabled == VIR_TRISTATE_BOOL_YES) {<br>
> + if (virXMLPropUInt(cur, "delay", 10, VIR_XML_PROP_REQUIRED,<br>
> + &src->reconnectDelay) < 0)<br>
> + return -1;<br>
> + }<br>
> + }<br>
> + }<br>
> +<br>
> /* for historical reasons we store the volume and image name in one XML<br>
> * element although it complicates thing when attempting to access them. */<br>
> if (src->path &&<br>
> @@ -21729,6 +21745,9 @@ virDomainDiskSourceFormatNetwork(virBuffer *attrBuf,<br>
> virBufferAddLit(childBuf, "/>\n");<br>
> }<br>
> <br>
> + if (src->reconnectDelay) {<br>
> + virBufferAsprintf(childBuf, "<reconnect enabled='yes' delay='%u'/>\n", src->reconnectDelay);<br>
> + }<br>
> <br>
> virBufferEscapeString(childBuf, "<snapshot name='%s'/>\n", src->snapshot);<br>
> virBufferEscapeString(childBuf, "<config file='%s'/>\n", src->configFile);<br>
> diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng<br>
> index c588a48fd2..72587416fe 100644<br>
> --- a/src/conf/schemas/domaincommon.rng<br>
> +++ b/src/conf/schemas/domaincommon.rng<br>
> @@ -2184,6 +2184,9 @@<br>
> </optional><br>
> <ref name="diskSourceCommon"/><br>
> <ref name="diskSourceNetworkHost"/><br>
> + <optional><br>
> + <ref name="reconnect"/><br>
> + </optional><br>
> <optional><br>
> <ref name="encryption"/><br>
> </optional><br>
> diff --git a/src/conf/schemas/storagecommon.rng b/src/conf/schemas/storagecommon.rng<br>
> index 76714c9aad..2842376e78 100644<br>
> --- a/src/conf/schemas/storagecommon.rng<br>
> +++ b/src/conf/schemas/storagecommon.rng<br>
> @@ -61,6 +61,11 @@<br>
> <ref name="unsignedInt"/><br>
> </attribute><br>
> </optional><br>
> + <optional><br>
> + <attribute name="delay"><br>
> + <ref name="unsignedInt"/><br>
> + </attribute><br>
> + </optional><br>
> </element><br>
> </define><br>
> <br>
> diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c<br>
> index 2b4cf5e241..edfd17c77c 100644<br>
> --- a/src/conf/storage_source_conf.c<br>
> +++ b/src/conf/storage_source_conf.c<br>
> @@ -810,6 +810,7 @@ virStorageSourceCopy(const virStorageSource *src,<br>
> def->sslverify = src->sslverify;<br>
> def->readahead = src->readahead;<br>
> def->timeout = src->timeout;<br>
> + def->reconnectDelay = src->reconnectDelay;<br>
> def->metadataCacheMaxSize = src->metadataCacheMaxSize;<br>
> <br>
> /* storage driver metadata are not copied */<br>
> diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h<br>
> index f2440cec6a..b60a4b3346 100644<br>
> --- a/src/conf/storage_source_conf.h<br>
> +++ b/src/conf/storage_source_conf.h<br>
> @@ -290,6 +290,10 @@ struct _virStorageSource {<br>
> unsigned long long readahead; /* size of the readahead buffer in bytes */<br>
> unsigned long long timeout; /* connection timeout in seconds */<br>
> <br>
> + /* NBD QEMU reconnect-delay option,<br>
> + * 0 as default value */<br>
> + unsigned int reconnectDelay;<br>
> +<br>
> virStorageSourceNVMeDef *nvme; /* type == VIR_STORAGE_TYPE_NVME */<br>
> <br>
> virDomainChrSourceDef *vhostuser; /* type == VIR_STORAGE_TYPE_VHOST_USER */<br>
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c<br>
> index 8a6f601b29..6913c380a0 100644<br>
> --- a/src/qemu/qemu_block.c<br>
> +++ b/src/qemu/qemu_block.c<br>
> @@ -530,6 +530,7 @@ qemuBlockStorageSourceGetNBDProps(virStorageSource *src,<br>
> "S:export", src->path,<br>
> "S:tls-creds", tlsAlias,<br>
> "S:tls-hostname", tlsHostname,<br>
> + "p:reconnect-delay", src->reconnectDelay,<br>
> NULL) < 0)<br>
> return NULL;<br>
> <br>
> @@ -1817,7 +1818,8 @@ qemuBlockGetBackingStoreString(virStorageSource *src,<br>
> src->ncookies == 0 &&<br>
> src->sslverify == VIR_TRISTATE_BOOL_ABSENT &&<br>
> src->timeout == 0 &&<br>
> - src->readahead == 0) {<br>
> + src->readahead == 0 &&<br>
> + src->reconnectDelay == 0) {<br>
> <br>
> switch ((virStorageNetProtocol) src->protocol) {<br>
> case VIR_STORAGE_NET_PROTOCOL_NBD:<br>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c<br>
> index 5c05032ce3..840d857f78 100644<br>
> --- a/src/qemu/qemu_domain.c<br>
> +++ b/src/qemu/qemu_domain.c<br>
> @@ -4927,6 +4927,15 @@ qemuDomainValidateStorageSource(virStorageSource *src,<br>
> }<br>
> }<br>
> <br>
> + if (src->reconnectDelay > 0) {<br>
> + if (actualType != VIR_STORAGE_TYPE_NETWORK ||<br>
> + src->protocol != VIR_STORAGE_NET_PROTOCOL_NBD) {<br>
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",<br>
> + _("reconnect delay is supported only with NBD protocol"));<br>
> + return -1;<br>
> + }<br>
> + }<br>
> +<br>
> if (src->query &&<br>
> (actualType != VIR_STORAGE_TYPE_NETWORK ||<br>
> (src->protocol != VIR_STORAGE_NET_PROTOCOL_HTTPS &&<br>
> diff --git a/tests/qemuxml2argvdata/disk-network-nbd.x86_64-latest.args b/tests/qemuxml2argvdata/disk-network-nbd.x86_64-latest.args<br>
> index 21e619af3e..e8d13b0bd4 100644<br>
> --- a/tests/qemuxml2argvdata/disk-network-nbd.x86_64-latest.args<br>
> +++ b/tests/qemuxml2argvdata/disk-network-nbd.x86_64-latest.args<br>
> @@ -28,21 +28,24 @@ XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \<br>
> -no-acpi \<br>
> -boot strict=on \<br>
> -device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \<br>
> --blockdev '{"driver":"nbd","server":{"type":"inet","host":"<a href="http://example.org" rel="noreferrer" target="_blank">example.org</a> <<a href="http://example.org" rel="noreferrer" target="_blank">http://example.org</a>>","port":"6000"},"node-name":"libvirt-5-storage","auto-read-only":true,"discard":"unmap"}' \<br>
> +-blockdev '{"driver":"nbd","server":{"type":"inet","host":"<a href="http://example.org" rel="noreferrer" target="_blank">example.org</a> <<a href="http://example.org" rel="noreferrer" target="_blank">http://example.org</a>>","port":"6000"},"node-name":"libvirt-6-storage","auto-read-only":true,"discard":"unmap"}' \<br>
> +-blockdev '{"node-name":"libvirt-6-format","read-only":false,"driver":"raw","file":"libvirt-6-storage"}' \<br>
> +-device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x2","drive":"libvirt-6-format","id":"virtio-disk0","bootindex":1}' \<br>
> +-blockdev '{"driver":"nbd","server":{"type":"inet","host":"<a href="http://example.org" rel="noreferrer" target="_blank">example.org</a> <<a href="http://example.org" rel="noreferrer" target="_blank">http://example.org</a>>","port":"6000"},"export":"bar","node-name":"libvirt-5-storage","auto-read-only":true,"discard":"unmap"}' \<br>
> -blockdev '{"node-name":"libvirt-5-format","read-only":false,"driver":"raw","file":"libvirt-5-storage"}' \<br>
> --device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x2","drive":"libvirt-5-format","id":"virtio-disk0","bootindex":1}' \<br>
> --blockdev '{"driver":"nbd","server":{"type":"inet","host":"<a href="http://example.org" rel="noreferrer" target="_blank">example.org</a> <<a href="http://example.org" rel="noreferrer" target="_blank">http://example.org</a>>","port":"6000"},"export":"bar","node-name":"libvirt-4-storage","auto-read-only":true,"discard":"unmap"}' \<br>
> +-device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x3","drive":"libvirt-5-format","id":"virtio-disk1"}' \<br>
> +-blockdev '{"driver":"nbd","server":{"type":"inet","host":"::1","port":"6000"},"node-name":"libvirt-4-storage","auto-read-only":true,"discard":"unmap"}' \<br>
> -blockdev '{"node-name":"libvirt-4-format","read-only":false,"driver":"raw","file":"libvirt-4-storage"}' \<br>
> --device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x3","drive":"libvirt-4-format","id":"virtio-disk1"}' \<br>
> --blockdev '{"driver":"nbd","server":{"type":"inet","host":"::1","port":"6000"},"node-name":"libvirt-3-storage","auto-read-only":true,"discard":"unmap"}' \<br>
> +-device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x4","drive":"libvirt-4-format","id":"virtio-disk2"}' \<br>
> +-blockdev '{"driver":"nbd","server":{"type":"inet","host":"::1","port":"6000"},"export":"bar","node-name":"libvirt-3-storage","auto-read-only":true,"discard":"unmap"}' \<br>
> -blockdev '{"node-name":"libvirt-3-format","read-only":false,"driver":"raw","file":"libvirt-3-storage"}' \<br>
> --device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x4","drive":"libvirt-3-format","id":"virtio-disk2"}' \<br>
> --blockdev '{"driver":"nbd","server":{"type":"inet","host":"::1","port":"6000"},"export":"bar","node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \<br>
> +-device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x5","drive":"libvirt-3-format","id":"virtio-disk3"}' \<br>
> +-blockdev '{"driver":"nbd","server":{"type":"unix","path":"/var/run/nbdsock"},"export":"bar","node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \<br>
> -blockdev '{"node-name":"libvirt-2-format","read-only":false,"driver":"raw","file":"libvirt-2-storage"}' \<br>
> --device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x5","drive":"libvirt-2-format","id":"virtio-disk3"}' \<br>
> --blockdev '{"driver":"nbd","server":{"type":"unix","path":"/var/run/nbdsock"},"export":"bar","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \<br>
> +-device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x6","drive":"libvirt-2-format","id":"virtio-disk4"}' \<br>
> +-blockdev '{"driver":"nbd","server":{"type":"inet","host":"<a href="http://example.org" rel="noreferrer" target="_blank">example.org</a> <<a href="http://example.org" rel="noreferrer" target="_blank">http://example.org</a>>","port":"6000"},"export":"foo","reconnect-delay":10,"node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \<br>
> -blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}' \<br>
> --device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x6","drive":"libvirt-1-format","id":"virtio-disk4"}' \<br>
> +-device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x7","drive":"libvirt-1-format","id":"virtio-disk5"}' \<br>
> -audiodev '{"id":"audio1","driver":"none"}' \<br>
> -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \<br>
> -msg timestamp=on<br>
> diff --git a/tests/qemuxml2argvdata/disk-network-nbd.xml b/tests/qemuxml2argvdata/disk-network-nbd.xml<br>
> index 8ac6cc3b7b..243ffeb1ed 100644<br>
> --- a/tests/qemuxml2argvdata/disk-network-nbd.xml<br>
> +++ b/tests/qemuxml2argvdata/disk-network-nbd.xml<br>
> @@ -49,6 +49,14 @@<br>
> </source><br>
> <target dev='vde' bus='virtio'/><br>
> </disk><br>
> + <disk type='network' device='disk'><br>
> + <driver name='qemu' type='raw'/><br>
> + <source protocol='nbd' name='foo'><br>
> + <host name='<a href="http://example.org" rel="noreferrer" target="_blank">example.org</a> <<a href="http://example.org" rel="noreferrer" target="_blank">http://example.org</a>>' port='6000'/><br>
> + <reconnect enabled='yes' delay='10'/><br>
> + </source><br>
> + <target dev='vdf' bus='virtio'/><br>
> + </disk><br>
> <controller type='usb' index='0'/><br>
> <controller type='ide' index='0'/><br>
> <controller type='pci' index='0' model='pci-root'/><br>
> diff --git a/tests/qemuxml2xmloutdata/disk-network-nbd.xml b/tests/qemuxml2xmloutdata/disk-network-nbd.xml<br>
> index f8dcca4bab..ed0c760d5b 100644<br>
> --- a/tests/qemuxml2xmloutdata/disk-network-nbd.xml<br>
> +++ b/tests/qemuxml2xmloutdata/disk-network-nbd.xml<br>
> @@ -54,6 +54,15 @@<br>
> <target dev='vde' bus='virtio'/><br>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/><br>
> </disk><br>
> + <disk type='network' device='disk'><br>
> + <driver name='qemu' type='raw'/><br>
> + <source protocol='nbd' name='foo'><br>
> + <host name='<a href="http://example.org" rel="noreferrer" target="_blank">example.org</a> <<a href="http://example.org" rel="noreferrer" target="_blank">http://example.org</a>>' port='6000'/><br>
> + <reconnect enabled='yes' delay='10'/><br>
> + </source><br>
> + <target dev='vdf' bus='virtio'/><br>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/><br>
> + </disk><br>
> <controller type='usb' index='0'><br>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/><br>
> </controller><br>
> -- <br>
> 2.34.1<br>
> <br>
<br>
</blockquote></div>