Re: [PATCH] qemu: implement QEMU NBD source reconnect delay attribute
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' port='6000'/> <reconnect enabled='yes' delay='10'/> </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","port":"6000"},"node-name":"libvirt-5-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"driver":"nbd","server":{"type":"inet","host":"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","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","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","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' 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' 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
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
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 <jjongsma@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 <
--- 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
https://listman.redhat.com/mailman/listinfo/libvir-list>> 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
participants (2)
-
Christian Nautze -
Jonathon Jongsma