Thank you, I see your point. We could just ignore the 'enabled' attribute from the reconnect element when it's used in the NBD source context:
<source protocol='nbd' name='foo'> <host name='example.org <http://example.org>' port='6000'/> <reconnect delay='10'/> </source> Another solution would be do add a totally new XML element. What would be your preferred solution? Thanks again Christian On Fri, 27 Jan 2023 at 23:42, Jonathon Jongsma <jjong...@redhat.com> wrote: > 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 > > > >