[PATCH] qemu: Give the users possibility to overwrite temp files for <transient\> disk
Normally when starting up a VM with a transient disk, if the file libvirt would use as the temp overlay for the original disk image exists libvirt will not touch it and fail startup. This is done in order to avoid any potential removal of user files if they manage to create a colliding filename. In case when the user doesn't want the above behaviour this patch' introduces a 'overwriteTemp' attribute for the '<transient/>' element which allows users to opt into simply removing the file before the next start. Closes: https://gitlab.com/libvirt/libvirt/-/issues/684 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatdomain.rst | 7 +++++++ src/conf/domain_conf.c | 7 +++++++ src/conf/domain_conf.h | 1 + src/conf/schemas/domaincommon.rng | 5 +++++ src/qemu/qemu_snapshot.c | 17 +++++++++++++---- .../disk-transient.x86_64-latest.xml | 2 +- tests/qemuxmlconfdata/disk-transient.xml | 2 +- 7 files changed, 35 insertions(+), 6 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index e6f09a728f..ab2fccdd97 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -3479,6 +3479,13 @@ paravirtualized driver is specified via the ``disk`` element. ``shareBacking`` attribute should be set to ``yes``. Note that hypervisor drivers may need to hotplug such disk and thus it works only with configurations supporting hotplug. :since:`Since 7.4.0` + + Hypervisors may need to store a temporary file containing the data written + by the domain while running, which may be stored in the same location + as the original source of the disk. Note that in some cases the temporary + file can't be cleaned up (e.g. when the domain is not shutdown before the host). + An optional attribute ``overwriteTemp`` set to ``yes`` (:since:`Since 10.10`) + indicates that the hypervisor may overwrite the file rather than fail startup. ``serial`` If present, this specify serial number of virtual hard drive. For example, it may look like ``<serial>WD-WMAP9A966149</serial>``. Not supported for diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6d7dee7956..359591d8f7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8240,6 +8240,11 @@ virDomainDiskDefParseXML(virDomainXMLOption *xmlopt, VIR_XML_PROP_NONE, &def->transientShareBacking) < 0) return NULL; + + if (virXMLPropTristateBool(transientNode, "overwriteTemp", + VIR_XML_PROP_NONE, + &def->transientOverwriteTemp) < 0) + return NULL; } if (virDomainDiskDefIotuneParse(def, ctxt) < 0) @@ -23233,6 +23238,8 @@ virDomainDiskDefFormat(virBuffer *buf, virBufferAddLit(&childBuf, "<transient"); if (def->transientShareBacking == VIR_TRISTATE_BOOL_YES) virBufferAddLit(&childBuf, " shareBacking='yes'"); + if (def->transientOverwriteTemp == VIR_TRISTATE_BOOL_YES) + virBufferAddLit(&childBuf, " overwriteTemp='yes'"); virBufferAddLit(&childBuf, "/>\n"); } virBufferEscapeString(&childBuf, "<serial>%s</serial>\n", def->serial); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a15af4fae3..169c626584 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -569,6 +569,7 @@ struct _virDomainDiskDef { virDomainStartupPolicy startupPolicy; bool transient; virTristateBool transientShareBacking; + virTristateBool transientOverwriteTemp; virDomainDeviceInfo info; virTristateBool rawio; virDomainDeviceSGIO sgio; diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index efb5f00d77..cd87e83410 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -1618,6 +1618,11 @@ <ref name='virYesNo'/> </attribute> </optional> + <optional> + <attribute name="overwriteTemp"> + <ref name='virYesNo'/> + </attribute> + </optional> <empty/> </element> </optional> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 1187ebf276..b8ca045900 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1287,10 +1287,19 @@ qemuSnapshotGetTransientDiskDef(virDomainDiskDef *domdisk, domdisk->src->path, suffix); if (virFileExists(snapdisk->src->path)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("Overlay file '%1$s' for transient disk '%2$s' already exists"), - snapdisk->src->path, domdisk->dst); - return NULL; + if (domdisk->transientOverwriteTemp == VIR_TRISTATE_BOOL_YES) { + if (unlink(snapdisk->src->path) != 0) { + virReportSystemError(errno, + _("Failed to delete overlay file '%1$s' for transient disk '%2$s'"), + snapdisk->src->path, domdisk->dst); + return NULL; + } + } else { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("Overlay file '%1$s' for transient disk '%2$s' already exists"), + snapdisk->src->path, domdisk->dst); + return NULL; + } } return g_steal_pointer(&snapdisk); diff --git a/tests/qemuxmlconfdata/disk-transient.x86_64-latest.xml b/tests/qemuxmlconfdata/disk-transient.x86_64-latest.xml index aab5894447..5f8f15f714 100644 --- a/tests/qemuxmlconfdata/disk-transient.x86_64-latest.xml +++ b/tests/qemuxmlconfdata/disk-transient.x86_64-latest.xml @@ -21,7 +21,7 @@ <driver name='qemu' type='qcow2' cache='none'/> <source file='/tmp/QEMUGuest2.img'/> <target dev='vda' bus='virtio'/> - <transient shareBacking='yes'/> + <transient shareBacking='yes' overwriteTemp='yes'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> </disk> <disk type='file' device='disk'> diff --git a/tests/qemuxmlconfdata/disk-transient.xml b/tests/qemuxmlconfdata/disk-transient.xml index edd65a0da0..722f1f9a92 100644 --- a/tests/qemuxmlconfdata/disk-transient.xml +++ b/tests/qemuxmlconfdata/disk-transient.xml @@ -18,7 +18,7 @@ <driver name='qemu' type='qcow2' cache='none'/> <source file='/tmp/QEMUGuest2.img'/> <target dev='vda' bus='virtio'/> - <transient shareBacking='yes'/> + <transient shareBacking='yes' overwriteTemp='yes'/> </disk> <disk type='file' device='disk'> <driver name='qemu' type='qcow2' cache='none'/> -- 2.47.0
On Thu, Oct 17, 2024 at 05:02:00PM +0200, Peter Krempa wrote:
Normally when starting up a VM with a transient disk, if the file libvirt would use as the temp overlay for the original disk image exists libvirt will not touch it and fail startup. This is done in order to avoid any potential removal of user files if they manage to create a colliding filename.
In case when the user doesn't want the above behaviour this patch' introduces a 'overwriteTemp' attribute for the '<transient/>' element which allows users to opt into simply removing the file before the next start.
Closes: https://gitlab.com/libvirt/libvirt/-/issues/684 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatdomain.rst | 7 +++++++ src/conf/domain_conf.c | 7 +++++++ src/conf/domain_conf.h | 1 + src/conf/schemas/domaincommon.rng | 5 +++++ src/qemu/qemu_snapshot.c | 17 +++++++++++++---- .../disk-transient.x86_64-latest.xml | 2 +- tests/qemuxmlconfdata/disk-transient.xml | 2 +- 7 files changed, 35 insertions(+), 6 deletions(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index e6f09a728f..ab2fccdd97 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -3479,6 +3479,13 @@ paravirtualized driver is specified via the ``disk`` element. ``shareBacking`` attribute should be set to ``yes``. Note that hypervisor drivers may need to hotplug such disk and thus it works only with configurations supporting hotplug. :since:`Since 7.4.0` + + Hypervisors may need to store a temporary file containing the data written + by the domain while running, which may be stored in the same location + as the original source of the disk. Note that in some cases the temporary + file can't be cleaned up (e.g. when the domain is not shutdown before the host). + An optional attribute ``overwriteTemp`` set to ``yes`` (:since:`Since 10.10`) + indicates that the hypervisor may overwrite the file rather than fail startup.
Is this really something we want to express in the guest XML ? The times at which you want to overwrite an existing file look to be tailored to the specific failure scenario hit. Users never expect to see a failure at first. If they don't add this attribute, and hit the failure, they must go back & edit the guest to add the attr, start the guest, and then possibly edit it again to remove the attr. If they add the attr unconditionally before seeing any failure, they'll never have an protection against mistakes. It feels like the same kind of situation that motivated flags VIR_DOMAIN_START_FORCE_BOOT and VIR_DOMAIN_START_RESET_NVRAM. IOW, suggesting we need VIR_DOMAIN_START_REPLACE_TRANSIENT ?
``serial`` If present, this specify serial number of virtual hard drive. For example, it may look like ``<serial>WD-WMAP9A966149</serial>``. Not supported for diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6d7dee7956..359591d8f7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8240,6 +8240,11 @@ virDomainDiskDefParseXML(virDomainXMLOption *xmlopt, VIR_XML_PROP_NONE, &def->transientShareBacking) < 0) return NULL; + + if (virXMLPropTristateBool(transientNode, "overwriteTemp", + VIR_XML_PROP_NONE, + &def->transientOverwriteTemp) < 0) + return NULL; }
if (virDomainDiskDefIotuneParse(def, ctxt) < 0) @@ -23233,6 +23238,8 @@ virDomainDiskDefFormat(virBuffer *buf, virBufferAddLit(&childBuf, "<transient"); if (def->transientShareBacking == VIR_TRISTATE_BOOL_YES) virBufferAddLit(&childBuf, " shareBacking='yes'"); + if (def->transientOverwriteTemp == VIR_TRISTATE_BOOL_YES) + virBufferAddLit(&childBuf, " overwriteTemp='yes'"); virBufferAddLit(&childBuf, "/>\n"); } virBufferEscapeString(&childBuf, "<serial>%s</serial>\n", def->serial); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a15af4fae3..169c626584 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -569,6 +569,7 @@ struct _virDomainDiskDef { virDomainStartupPolicy startupPolicy; bool transient; virTristateBool transientShareBacking; + virTristateBool transientOverwriteTemp; virDomainDeviceInfo info; virTristateBool rawio; virDomainDeviceSGIO sgio; diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index efb5f00d77..cd87e83410 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -1618,6 +1618,11 @@ <ref name='virYesNo'/> </attribute> </optional> + <optional> + <attribute name="overwriteTemp"> + <ref name='virYesNo'/> + </attribute> + </optional> <empty/> </element> </optional> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 1187ebf276..b8ca045900 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1287,10 +1287,19 @@ qemuSnapshotGetTransientDiskDef(virDomainDiskDef *domdisk, domdisk->src->path, suffix);
if (virFileExists(snapdisk->src->path)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("Overlay file '%1$s' for transient disk '%2$s' already exists"), - snapdisk->src->path, domdisk->dst); - return NULL; + if (domdisk->transientOverwriteTemp == VIR_TRISTATE_BOOL_YES) { + if (unlink(snapdisk->src->path) != 0) { + virReportSystemError(errno, + _("Failed to delete overlay file '%1$s' for transient disk '%2$s'"), + snapdisk->src->path, domdisk->dst); + return NULL; + } + } else { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("Overlay file '%1$s' for transient disk '%2$s' already exists"), + snapdisk->src->path, domdisk->dst); + return NULL; + } }
return g_steal_pointer(&snapdisk); diff --git a/tests/qemuxmlconfdata/disk-transient.x86_64-latest.xml b/tests/qemuxmlconfdata/disk-transient.x86_64-latest.xml index aab5894447..5f8f15f714 100644 --- a/tests/qemuxmlconfdata/disk-transient.x86_64-latest.xml +++ b/tests/qemuxmlconfdata/disk-transient.x86_64-latest.xml @@ -21,7 +21,7 @@ <driver name='qemu' type='qcow2' cache='none'/> <source file='/tmp/QEMUGuest2.img'/> <target dev='vda' bus='virtio'/> - <transient shareBacking='yes'/> + <transient shareBacking='yes' overwriteTemp='yes'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> </disk> <disk type='file' device='disk'> diff --git a/tests/qemuxmlconfdata/disk-transient.xml b/tests/qemuxmlconfdata/disk-transient.xml index edd65a0da0..722f1f9a92 100644 --- a/tests/qemuxmlconfdata/disk-transient.xml +++ b/tests/qemuxmlconfdata/disk-transient.xml @@ -18,7 +18,7 @@ <driver name='qemu' type='qcow2' cache='none'/> <source file='/tmp/QEMUGuest2.img'/> <target dev='vda' bus='virtio'/> - <transient shareBacking='yes'/> + <transient shareBacking='yes' overwriteTemp='yes'/> </disk> <disk type='file' device='disk'> <driver name='qemu' type='qcow2' cache='none'/> -- 2.47.0
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Oct 17, 2024 at 16:16:25 +0100, Daniel P. Berrangé wrote:
On Thu, Oct 17, 2024 at 05:02:00PM +0200, Peter Krempa wrote:
Normally when starting up a VM with a transient disk, if the file libvirt would use as the temp overlay for the original disk image exists libvirt will not touch it and fail startup. This is done in order to avoid any potential removal of user files if they manage to create a colliding filename.
In case when the user doesn't want the above behaviour this patch' introduces a 'overwriteTemp' attribute for the '<transient/>' element which allows users to opt into simply removing the file before the next start.
Closes: https://gitlab.com/libvirt/libvirt/-/issues/684 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatdomain.rst | 7 +++++++ src/conf/domain_conf.c | 7 +++++++ src/conf/domain_conf.h | 1 + src/conf/schemas/domaincommon.rng | 5 +++++ src/qemu/qemu_snapshot.c | 17 +++++++++++++---- .../disk-transient.x86_64-latest.xml | 2 +- tests/qemuxmlconfdata/disk-transient.xml | 2 +- 7 files changed, 35 insertions(+), 6 deletions(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index e6f09a728f..ab2fccdd97 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -3479,6 +3479,13 @@ paravirtualized driver is specified via the ``disk`` element. ``shareBacking`` attribute should be set to ``yes``. Note that hypervisor drivers may need to hotplug such disk and thus it works only with configurations supporting hotplug. :since:`Since 7.4.0` + + Hypervisors may need to store a temporary file containing the data written + by the domain while running, which may be stored in the same location + as the original source of the disk. Note that in some cases the temporary + file can't be cleaned up (e.g. when the domain is not shutdown before the host). + An optional attribute ``overwriteTemp`` set to ``yes`` (:since:`Since 10.10`) + indicates that the hypervisor may overwrite the file rather than fail startup.
Is this really something we want to express in the guest XML ?
The times at which you want to overwrite an existing file look to be tailored to the specific failure scenario hit.
Users never expect to see a failure at first.
If they don't add this attribute, and hit the failure, they must go back & edit the guest to add the attr, start the guest, and then possibly edit it again to remove the attr.
If they add the attr unconditionally before seeing any failure, they'll never have an protection against mistakes.
It feels like the same kind of situation that motivated flags VIR_DOMAIN_START_FORCE_BOOT and VIR_DOMAIN_START_RESET_NVRAM.
IOW, suggesting we need VIR_DOMAIN_START_REPLACE_TRANSIENT ?
I thought about this as well. The drawback of this solution is that it's hard to use in scripts as users would have to parse errors. and/or always send this flag. Any other option would require adding another error code and that still wouldn't help virsh users as they don't get to see the error code, just the translated string corresponding to it. And lastly it's harder to implement (wiring up the propagation of the flag). Thinking about it a bit more the semantics of the transient disk allow us to simply delete the file without asking. The only thing I was paranoid about, which is the reason for the check to exist is if users would "accidentaly" have file with such name. But .. that is extremely unlikely. We generate the filename for the temporary overlay as: $(origpath).TRANSIENT-$(vmname) So I guess if we just document the above we should be okay simply delete the file if it exists and create another overlay. FWIW, it'd be ideal to simply unlink the file after qemu starts using it so that it doesn't linger until libvirt cleans that up, but that'd require going through the block job code ensuring that it works properly with the file missing, which is something not worth doing IMO. So I suggest I document the temporary file naming scheme and simply delete it always instead of the new attribute. Does that work for you?
On Fri, Oct 18, 2024 at 10:31:34AM +0200, Peter Krempa wrote:
On Thu, Oct 17, 2024 at 16:16:25 +0100, Daniel P. Berrangé wrote:
On Thu, Oct 17, 2024 at 05:02:00PM +0200, Peter Krempa wrote:
Normally when starting up a VM with a transient disk, if the file libvirt would use as the temp overlay for the original disk image exists libvirt will not touch it and fail startup. This is done in order to avoid any potential removal of user files if they manage to create a colliding filename.
In case when the user doesn't want the above behaviour this patch' introduces a 'overwriteTemp' attribute for the '<transient/>' element which allows users to opt into simply removing the file before the next start.
Closes: https://gitlab.com/libvirt/libvirt/-/issues/684 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatdomain.rst | 7 +++++++ src/conf/domain_conf.c | 7 +++++++ src/conf/domain_conf.h | 1 + src/conf/schemas/domaincommon.rng | 5 +++++ src/qemu/qemu_snapshot.c | 17 +++++++++++++---- .../disk-transient.x86_64-latest.xml | 2 +- tests/qemuxmlconfdata/disk-transient.xml | 2 +- 7 files changed, 35 insertions(+), 6 deletions(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index e6f09a728f..ab2fccdd97 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -3479,6 +3479,13 @@ paravirtualized driver is specified via the ``disk`` element. ``shareBacking`` attribute should be set to ``yes``. Note that hypervisor drivers may need to hotplug such disk and thus it works only with configurations supporting hotplug. :since:`Since 7.4.0` + + Hypervisors may need to store a temporary file containing the data written + by the domain while running, which may be stored in the same location + as the original source of the disk. Note that in some cases the temporary + file can't be cleaned up (e.g. when the domain is not shutdown before the host). + An optional attribute ``overwriteTemp`` set to ``yes`` (:since:`Since 10.10`) + indicates that the hypervisor may overwrite the file rather than fail startup.
Is this really something we want to express in the guest XML ?
The times at which you want to overwrite an existing file look to be tailored to the specific failure scenario hit.
Users never expect to see a failure at first.
If they don't add this attribute, and hit the failure, they must go back & edit the guest to add the attr, start the guest, and then possibly edit it again to remove the attr.
If they add the attr unconditionally before seeing any failure, they'll never have an protection against mistakes.
It feels like the same kind of situation that motivated flags VIR_DOMAIN_START_FORCE_BOOT and VIR_DOMAIN_START_RESET_NVRAM.
IOW, suggesting we need VIR_DOMAIN_START_REPLACE_TRANSIENT ?
I thought about this as well. The drawback of this solution is that it's hard to use in scripts as users would have to parse errors. and/or always send this flag.
Any other option would require adding another error code and that still wouldn't help virsh users as they don't get to see the error code, just the translated string corresponding to it.
And lastly it's harder to implement (wiring up the propagation of the flag).
Thinking about it a bit more the semantics of the transient disk allow us to simply delete the file without asking. The only thing I was paranoid about, which is the reason for the check to exist is if users would "accidentaly" have file with such name. But .. that is extremely unlikely. We generate the filename for the temporary overlay as:
$(origpath).TRANSIENT-$(vmname)
So I guess if we just document the above we should be okay simply delete the file if it exists and create another overlay.
Right, if we have $(vmname) in the template, then the only way we could delete something that is still in use, is if libvirtd somehow forgot that the QEMU exists while it was still running. That shouldn't happen, and if it did, we should fix the bug. In any case transient overlays should be used for inherantly throw-away data, since QEMU can crash at any time and libvirt will delete the overlay then no matter what.
FWIW, it'd be ideal to simply unlink the file after qemu starts using it so that it doesn't linger until libvirt cleans that up, but that'd require going through the block job code ensuring that it works properly with the file missing, which is something not worth doing IMO.
So I suggest I document the temporary file naming scheme and simply delete it always instead of the new attribute. Does that work for you?
Yes. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (2)
-
Daniel P. Berrangé -
Peter Krempa