[libvirt PATCH 00/20] introduce external snapshot revert support
This implements virDomainRevertToSnapshot to work with external snapshots. In addition it modifies virDomainSnapshotDelete to work correctly when we revert to non-leaf snapshot. Gitlab repo with the patches: https://gitlab.com/phrdina/libvirt/-/tree/snapshot-revert-external Pavel Hrdina (20): libvirt_private: list virDomainMomentDefPostParse snapshot_conf: export virDomainSnapshotDiskDefClear snapshot_conf: use alternate domain definition in virDomainSnapshotDefAssignExternalNames snapshot_conf: introduce <revertDisks> metadata element qemu_snapshot: introduce qemuSnapshotDomainDefUpdateDisk qemu_snapshot: use virDomainDiskByName while updating domain def qemu_snapshot: introduce qemuSnapshotCreateQcow2Files qemu_snapshot: allow using alternate domain definition when creating qcow2 files qemu_snapshot: introduce external snapshot revert support qemu_snapshot: rename qemuSnapshotDeleteExternalPrepare qemu_snapshot: extract external snapshot delete prepare to function qemu_snapshot: add blockCommit to external snapshot delete prepare data qemu_snapshot: prepare data for non-active leaf external snapshot deletion qemu_snapshot: add support to delete external snapshot without block commit qemu_snapshot: delete: properly update parent snapshot with revert data qemu_snapshot: don't allow creating external snapshot after reverting virdomainmomentobjlist: introduce virDomainMomentIsAncestor qemu_snapshot: check only once if snapshot is external qemu_snapshot: add checks for external snapshot deletion qemu_snapshot: allow snapshot revert for external snapshots src/conf/schemas/domainsnapshot.rng | 7 + src/conf/snapshot_conf.c | 37 +- src/conf/snapshot_conf.h | 8 + src/conf/virdomainmomentobjlist.c | 21 + src/conf/virdomainmomentobjlist.h | 4 + src/libvirt_private.syms | 6 + src/qemu/qemu_snapshot.c | 621 ++++++++++++++++++++-------- 7 files changed, 538 insertions(+), 166 deletions(-) -- 2.39.2
We will need to call this function from qemu_snapshot when introducing external snapshot revert support. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt_private.syms | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6f44788233..245f258df4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -809,6 +809,10 @@ virInterfaceDefParseString; virInterfaceDefParseXML; +# conf/moment_conf.h +virDomainMomentDefPostParse; + + # conf/netdev_bandwidth_conf.h virDomainClearNetBandwidth; virNetDevBandwidthFormat; -- 2.39.2
On Mon, Mar 13, 2023 at 16:42:02 +0100, Pavel Hrdina wrote:
We will need to call this function from qemu_snapshot when introducing external snapshot revert support.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt_private.syms | 4 ++++ 1 file changed, 4 insertions(+)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
We will need to call this function from qemu_snapshot when introducing external snapshot revert support. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/snapshot_conf.c | 2 +- src/conf/snapshot_conf.h | 3 +++ src/libvirt_private.syms | 1 + 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 9bf3c78353..1329292293 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -74,7 +74,7 @@ VIR_ENUM_IMPL(virDomainSnapshotState, ); /* Snapshot Def functions */ -static void +void virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDef *disk) { VIR_FREE(disk->name); diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 96c77ef42b..ad49990a1e 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -59,6 +59,9 @@ struct _virDomainSnapshotDiskDef { virStorageSource *src; }; +void +virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDef *disk); + void virDomainSnapshotDiskDefFree(virDomainSnapshotDiskDef *disk); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 245f258df4..dd752ba55c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1039,6 +1039,7 @@ virDomainSnapshotDefFormat; virDomainSnapshotDefIsExternal; virDomainSnapshotDefNew; virDomainSnapshotDefParseString; +virDomainSnapshotDiskDefClear; virDomainSnapshotDiskDefFree; virDomainSnapshotDiskDefParseXML; virDomainSnapshotFormatConvertXMLFlags; -- 2.39.2
On Mon, Mar 13, 2023 at 16:42:03 +0100, Pavel Hrdina wrote:
We will need to call this function from qemu_snapshot when introducing external snapshot revert support.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/snapshot_conf.c | 2 +- src/conf/snapshot_conf.h | 3 +++ src/libvirt_private.syms | 1 + 3 files changed, 5 insertions(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Commit <ef3f3884a2432958bdd4ea0ce45509d47a91a453> introduced new argument for virDomainSnapshotAlignDisks() that allows passing alternate domain definition in case the snapshot parent.dom is NULL. In case of redefining snapshot it will not hit the part of code that unconditionally uses parent.dom as there will not be need to generate default external file names. It should be still fixed to make it safe. Future external snapshot revert code will use this to generate default file names and in this case it would crash. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/snapshot_conf.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 1329292293..da1c694cb9 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -489,12 +489,14 @@ virDomainSnapshotRedefineValidate(virDomainSnapshotDef *def, /** * virDomainSnapshotDefAssignExternalNames: * @def: snapshot def object + * @domdef: domain def object * * Generate default external file names for snapshot targets. Returns 0 on * success, -1 on error. */ static int -virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDef *def) +virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDef *def, + virDomainDef *domdef) { const char *origpath; char *tmppath; @@ -518,7 +520,7 @@ virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDef *def) return -1; } - if (!(origpath = virDomainDiskGetSource(def->parent.dom->disks[i]))) { + if (!(origpath = virDomainDiskGetSource(domdef->disks[i]))) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("cannot generate external snapshot name " "for disk '%s' without source"), @@ -711,7 +713,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDef *snapdef, } /* Generate default external file names for external snapshot locations */ - if (virDomainSnapshotDefAssignExternalNames(snapdef) < 0) + if (virDomainSnapshotDefAssignExternalNames(snapdef, domdef) < 0) return -1; return 0; -- 2.39.2
On Mon, Mar 13, 2023 at 16:42:04 +0100, Pavel Hrdina wrote:
Commit <ef3f3884a2432958bdd4ea0ce45509d47a91a453> introduced new argument for virDomainSnapshotAlignDisks() that allows passing alternate domain definition in case the snapshot parent.dom is NULL.
In case of redefining snapshot it will not hit the part of code that unconditionally uses parent.dom as there will not be need to generate default external file names.
It should be still fixed to make it safe. Future external snapshot revert code will use this to generate default file names and in this case it would crash.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/snapshot_conf.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This new element will hold the new disk overlay created when reverting to non-leaf snapshot in order to remember the files libvirt created. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/schemas/domainsnapshot.rng | 7 +++++++ src/conf/snapshot_conf.c | 27 +++++++++++++++++++++++++++ src/conf/snapshot_conf.h | 5 +++++ 3 files changed, 39 insertions(+) diff --git a/src/conf/schemas/domainsnapshot.rng b/src/conf/schemas/domainsnapshot.rng index 45f01b96cd..2549c47b22 100644 --- a/src/conf/schemas/domainsnapshot.rng +++ b/src/conf/schemas/domainsnapshot.rng @@ -60,6 +60,13 @@ </zeroOrMore> </element> </optional> + <optional> + <element name="revertDisks"> + <zeroOrMore> + <ref name="disksnapshot"/> + </zeroOrMore> + </element> + </optional> <optional> <element name="active"> <choice> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index da1c694cb9..949104c1fd 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -376,6 +376,22 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, return NULL; } + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { + g_autofree xmlNodePtr *revertDiskNodes = NULL; + + if ((n = virXPathNodeSet("./revertDisks/*", ctxt, &revertDiskNodes)) < 0) + return NULL; + if (n) + def->revertdisks = g_new0(virDomainSnapshotDiskDef, n); + def->nrevertdisks = n; + for (i = 0; i < def->nrevertdisks; i++) { + if (virDomainSnapshotDiskDefParseXML(revertDiskNodes[i], ctxt, + &def->revertdisks[i], + flags, xmlopt) < 0) + return NULL; + } + } + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL) { int active; @@ -843,6 +859,17 @@ virDomainSnapshotDefFormatInternal(virBuffer *buf, virBufferAddLit(buf, "</disks>\n"); } + if (def->nrevertdisks) { + virBufferAddLit(buf, "<revertDisks>\n"); + virBufferAdjustIndent(buf, 2); + for (i = 0; i < def->nrevertdisks; i++) { + if (virDomainSnapshotDiskDefFormat(buf, &def->revertdisks[i], xmlopt) < 0) + return -1; + } + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</revertDisks>\n"); + } + if (def->parent.dom) { if (virDomainDefFormatInternal(def->parent.dom, xmlopt, buf, domainflags) < 0) diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index ad49990a1e..ab76af604a 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -80,6 +80,11 @@ struct _virDomainSnapshotDef { size_t ndisks; /* should not exceed dom->ndisks */ virDomainSnapshotDiskDef *disks; + /* When we revert to non-leaf snapshot we need to + * store the new overlay disks. */ + size_t nrevertdisks; + virDomainSnapshotDiskDef *revertdisks; + virObject *cookie; }; -- 2.39.2
On Mon, Mar 13, 2023 at 16:42:05 +0100, Pavel Hrdina wrote:
This new element will hold the new disk overlay created when reverting to non-leaf snapshot in order to remember the files libvirt created.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/schemas/domainsnapshot.rng | 7 +++++++ src/conf/snapshot_conf.c | 27 +++++++++++++++++++++++++++ src/conf/snapshot_conf.h | 5 +++++ 3 files changed, 39 insertions(+)
[...]
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index da1c694cb9..949104c1fd 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c
[...]
@@ -843,6 +859,17 @@ virDomainSnapshotDefFormatInternal(virBuffer *buf, virBufferAddLit(buf, "</disks>\n"); }
+ if (def->nrevertdisks) {
Always use explicit comparison against 0 for non-pointer, non-booleans.
+ virBufferAddLit(buf, "<revertDisks>\n"); + virBufferAdjustIndent(buf, 2); + for (i = 0; i < def->nrevertdisks; i++) { + if (virDomainSnapshotDiskDefFormat(buf, &def->revertdisks[i], xmlopt) < 0) + return -1; + } + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</revertDisks>\n");
Also preferrably use virXMLFormatElement.
+ } + if (def->parent.dom) { if (virDomainDefFormatInternal(def->parent.dom, xmlopt, buf, domainflags) < 0)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On Mon, Apr 03, 2023 at 03:45:50PM +0200, Peter Krempa wrote:
On Mon, Mar 13, 2023 at 16:42:05 +0100, Pavel Hrdina wrote:
This new element will hold the new disk overlay created when reverting to non-leaf snapshot in order to remember the files libvirt created.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/schemas/domainsnapshot.rng | 7 +++++++ src/conf/snapshot_conf.c | 27 +++++++++++++++++++++++++++ src/conf/snapshot_conf.h | 5 +++++ 3 files changed, 39 insertions(+)
[...]
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index da1c694cb9..949104c1fd 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c
[...]
@@ -843,6 +859,17 @@ virDomainSnapshotDefFormatInternal(virBuffer *buf, virBufferAddLit(buf, "</disks>\n"); }
+ if (def->nrevertdisks) {
Always use explicit comparison against 0 for non-pointer, non-booleans.
+ virBufferAddLit(buf, "<revertDisks>\n"); + virBufferAdjustIndent(buf, 2); + for (i = 0; i < def->nrevertdisks; i++) { + if (virDomainSnapshotDiskDefFormat(buf, &def->revertdisks[i], xmlopt) < 0) + return -1; + } + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</revertDisks>\n");
Also preferrably use virXMLFormatElement.
I've basically copy&pasted what we do for the <disks> element. Will fix it for this patch and post a separate patch fixing the other incorrect comparisons and building xml.
+ } + if (def->parent.dom) { if (virDomainDefFormatInternal(def->parent.dom, xmlopt, buf, domainflags) < 0)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Extract the code that updates disks in domain definition while creating external snapshots. We will use it later in the external snapshot revert code. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 63 ++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 7aa4195f04..924731fdf1 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -142,6 +142,42 @@ qemuSnapshotFSThaw(virDomainObj *vm, } +static int +qemuSnapshotDomainDefUpdateDisk(virDomainDef *domdef, + virDomainSnapshotDef *snapdef, + bool reuse) +{ + size_t i; + + for (i = 0; i < snapdef->ndisks; i++) { + g_autoptr(virStorageSource) newsrc = NULL; + virDomainSnapshotDiskDef *snapdisk = &(snapdef->disks[i]); + virDomainDiskDef *defdisk = domdef->disks[i]; + + if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + continue; + + if (!(newsrc = virStorageSourceCopy(snapdisk->src, false))) + return -1; + + if (virStorageSourceInitChainElement(newsrc, defdisk->src, false) < 0) + return -1; + + if (!reuse && + virStorageSourceHasBacking(defdisk->src)) { + defdisk->src->readonly = true; + newsrc->backingStore = g_steal_pointer(&defdisk->src); + } else { + virObjectUnref(defdisk->src); + } + + defdisk->src = g_steal_pointer(&newsrc); + } + + return 0; +} + + /* The domain is expected to be locked and inactive. */ static int qemuSnapshotCreateInactiveInternal(virQEMUDriver *driver, @@ -216,31 +252,8 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, } /* update disk definitions */ - for (i = 0; i < snapdef->ndisks; i++) { - g_autoptr(virStorageSource) newsrc = NULL; - - snapdisk = &(snapdef->disks[i]); - defdisk = vm->def->disks[i]; - - if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) - continue; - - if (!(newsrc = virStorageSourceCopy(snapdisk->src, false))) - goto cleanup; - - if (virStorageSourceInitChainElement(newsrc, defdisk->src, false) < 0) - goto cleanup; - - if (!reuse && - virStorageSourceHasBacking(defdisk->src)) { - defdisk->src->readonly = true; - newsrc->backingStore = g_steal_pointer(&defdisk->src); - } else { - virObjectUnref(defdisk->src); - } - - defdisk->src = g_steal_pointer(&newsrc); - } + if (qemuSnapshotDomainDefUpdateDisk(vm->def, snapdef, reuse) < 0) + goto cleanup; if (virDomainDefSave(vm->def, driver->xmlopt, cfg->configDir) < 0) goto cleanup; -- 2.39.2
On Mon, Mar 13, 2023 at 16:42:06 +0100, Pavel Hrdina wrote:
Extract the code that updates disks in domain definition while creating external snapshots. We will use it later in the external snapshot revert code.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 63 ++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 25 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
When creating external snapshot this function is called only when the VM is not running so there is only one definition to care about. However, it will be used by external snapshot revert code for active and inactive definition and they may be different if a disk was (un)plugged only for the active or inactive definition. The current code would crash so use virDomainDiskByName() to get the correct disk from the domain definition based on the disk name and make sure it exists. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 924731fdf1..bbef753db6 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -152,11 +152,14 @@ qemuSnapshotDomainDefUpdateDisk(virDomainDef *domdef, for (i = 0; i < snapdef->ndisks; i++) { g_autoptr(virStorageSource) newsrc = NULL; virDomainSnapshotDiskDef *snapdisk = &(snapdef->disks[i]); - virDomainDiskDef *defdisk = domdef->disks[i]; + virDomainDiskDef *defdisk = virDomainDiskByName(domdef, snapdisk->name, false); if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) continue; + if (!defdisk) + continue; + if (!(newsrc = virStorageSourceCopy(snapdisk->src, false))) return -1; -- 2.39.2
On Mon, Mar 13, 2023 at 16:42:07 +0100, Pavel Hrdina wrote:
When creating external snapshot this function is called only when the VM is not running so there is only one definition to care about. However, it will be used by external snapshot revert code for active and inactive definition and they may be different if a disk was (un)plugged only for the active or inactive definition.
The current code would crash so use virDomainDiskByName() to get the correct disk from the domain definition based on the disk name and make sure it exists.
Historically the snapshot code expects that virDomainSnapshotAlignDisks was ran and thus the disk and snapshot definitions are in sync.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Extract creation of qcow2 files for external snapshots to separate function as we will need it for external snapshot revert code. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 67 +++++++++++++++++++++++++--------------- 1 file changed, 42 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index bbef753db6..06ed20c815 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -181,35 +181,21 @@ qemuSnapshotDomainDefUpdateDisk(virDomainDef *domdef, } -/* The domain is expected to be locked and inactive. */ static int -qemuSnapshotCreateInactiveInternal(virQEMUDriver *driver, - virDomainObj *vm, - virDomainMomentObj *snap) -{ - return qemuDomainSnapshotForEachQcow2(driver, vm->def, snap, "-c", false); -} - - -/* The domain is expected to be locked and inactive. */ -static int -qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, - virDomainObj *vm, - virDomainMomentObj *snap, - bool reuse) +qemuSnapshotCreateQcow2Files(virDomainObj *vm, + virDomainSnapshotDef *snapdef, + virBitmap *created, + bool reuse) { size_t i; - virDomainSnapshotDiskDef *snapdisk; - virDomainDiskDef *defdisk; const char *qemuImgPath; - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - int ret = -1; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); - g_autoptr(virBitmap) created = virBitmapNew(snapdef->ndisks); + virQEMUDriver *driver = ((qemuDomainObjPrivate *) vm->privateData)->driver; + virDomainSnapshotDiskDef *snapdisk = NULL; + virDomainDiskDef *defdisk = NULL; if (!(qemuImgPath = qemuFindQemuImgBinary(driver))) - goto cleanup; + return -1; /* If reuse is true, then qemuSnapshotPrepare already * ensured that the new files exist, and it was up to the user to @@ -218,6 +204,7 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, g_autoptr(virCommand) cmd = NULL; snapdisk = &(snapdef->disks[i]); defdisk = vm->def->disks[i]; + if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) continue; @@ -225,7 +212,7 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, snapdisk->src->format = VIR_STORAGE_FILE_QCOW2; if (qemuDomainStorageSourceValidateDepth(defdisk->src, 1, defdisk->dst) < 0) - goto cleanup; + return -1; /* creates cmd line args: qemu-img create -f qcow2 -o */ if (!(cmd = virCommandNewArgList(qemuImgPath, @@ -234,7 +221,7 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, virStorageFileFormatTypeToString(snapdisk->src->format), "-o", NULL))) - goto cleanup; + return -1; /* adds cmd line arg: backing_fmt=format,backing_file=/path/to/backing/file */ virBufferAsprintf(&buf, "backing_fmt=%s,backing_file=", @@ -251,9 +238,39 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, ignore_value(virBitmapSetBit(created, i)); if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + return -1; } + return 0; +} + + +/* The domain is expected to be locked and inactive. */ +static int +qemuSnapshotCreateInactiveInternal(virQEMUDriver *driver, + virDomainObj *vm, + virDomainMomentObj *snap) +{ + return qemuDomainSnapshotForEachQcow2(driver, vm->def, snap, "-c", false); +} + + +/* The domain is expected to be locked and inactive. */ +static int +qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, + virDomainObj *vm, + virDomainMomentObj *snap, + bool reuse) +{ + virDomainSnapshotDiskDef *snapdisk; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + int ret = -1; + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); + g_autoptr(virBitmap) created = virBitmapNew(snapdef->ndisks); + + if (qemuSnapshotCreateQcow2Files(vm, snapdef, created, reuse) < 0) + goto cleanup; + /* update disk definitions */ if (qemuSnapshotDomainDefUpdateDisk(vm->def, snapdef, reuse) < 0) goto cleanup; -- 2.39.2
On Mon, Mar 13, 2023 at 16:42:08 +0100, Pavel Hrdina wrote:
Extract creation of qcow2 files for external snapshots to separate function as we will need it for external snapshot revert code.
Hmm, I don't think I like where this is going. I presume you want to use this code to create the new overlay images. If you want to use this also in cases where the VM was live you might run into scenarios where the qemu-img code will not be able to handle the overlay creation, specifically on networked storage. I think we'll need to create the overlay images as part of the startup of the reverted VM via QMP exactly as we are creating overlays currently for snapshots. Even when today's API will not allow reversion of non-local snapshots (I didn't check further in this series yet) I don't think we should go the way of using qemu-img at all.
On Tue, Apr 04, 2023 at 01:09:41PM +0200, Peter Krempa wrote:
On Mon, Mar 13, 2023 at 16:42:08 +0100, Pavel Hrdina wrote:
Extract creation of qcow2 files for external snapshots to separate function as we will need it for external snapshot revert code.
Hmm, I don't think I like where this is going. I presume you want to use this code to create the new overlay images.
Correct
If you want to use this also in cases where the VM was live you might run into scenarios where the qemu-img code will not be able to handle the overlay creation, specifically on networked storage.
We always kill the qemu process when reverting so I figured that qemu-img should be good enough to create the new overlay because the qemu process is not running at that point. Did not think we allow snapshot creation for qcow2 images on networked storage as it would not work for offline VM where we also use this qemu-img approach. Looking at the code for running VM we are using QMP to create the overlay files and I did not manage to find any check to limit it only for local images. If that's the case we have IMHO really bad inconsistent behavior where we would allow creating new snapshots for non-local images if the VM is running but fail to do so if the VM is offline.
I think we'll need to create the overlay images as part of the startup of the reverted VM via QMP exactly as we are creating overlays currently for snapshots.
Even when today's API will not allow reversion of non-local snapshots (I didn't check further in this series yet) I don't think we should go the way of using qemu-img at all.
There is no check for that as I assumed (probably incorrectly) that we allow creating snapshot only for local images. If that's not the case then yes we need to use the QMP commands to create overlays. Pavel
To create new overlay files when external snapshot revert support is introduced we will be using different domain definition than what is currently used by the domain. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 06ed20c815..9e4b978b1b 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -183,6 +183,7 @@ qemuSnapshotDomainDefUpdateDisk(virDomainDef *domdef, static int qemuSnapshotCreateQcow2Files(virDomainObj *vm, + virDomainDef *def, virDomainSnapshotDef *snapdef, virBitmap *created, bool reuse) @@ -194,6 +195,9 @@ qemuSnapshotCreateQcow2Files(virDomainObj *vm, virDomainSnapshotDiskDef *snapdisk = NULL; virDomainDiskDef *defdisk = NULL; + if (!def) + def = vm->def; + if (!(qemuImgPath = qemuFindQemuImgBinary(driver))) return -1; @@ -203,7 +207,7 @@ qemuSnapshotCreateQcow2Files(virDomainObj *vm, for (i = 0; i < snapdef->ndisks && !reuse; i++) { g_autoptr(virCommand) cmd = NULL; snapdisk = &(snapdef->disks[i]); - defdisk = vm->def->disks[i]; + defdisk = def->disks[i]; if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) continue; @@ -268,7 +272,7 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); g_autoptr(virBitmap) created = virBitmapNew(snapdef->ndisks); - if (qemuSnapshotCreateQcow2Files(vm, snapdef, created, reuse) < 0) + if (qemuSnapshotCreateQcow2Files(vm, NULL, snapdef, created, reuse) < 0) goto cleanup; /* update disk definitions */ -- 2.39.2
When reverting to external snapshot we need to create new overlay qcow2 files from the disk files the VM had when the snapshot was taken. There are some specifics and limitations when reverting to a snapshot: 1) When reverting to last snapshot we need to first create new overlay files before we can safely delete the old overlay files in case the creation fails so we have still recovery option when we error out. These new files will not have the suffix as when the snapshot was created as renaming the original files in order to use the same file names as when the snapshot was created would add unnecessary complexity to the code. 2) When reverting to any snapshot we will always create overlay files for every disk the VM had when the snapshot was done. Otherwise we would have to figure out if there is any other qcow2 image already using any of the VM disks as backing store and that itself might be extremely complex and in some cases impossible. 3) When reverting from any state the current overlay files will be always removed as that VM state is not meant to be saved. It's the same as with internal snapshots. If user want's to keep the current state before reverting they need to create a new snapshot. For now this will only work if the current snapshot is the last. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 143 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 139 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 9e4b978b1b..af4e2ea6aa 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -18,6 +18,8 @@ #include <config.h> +#include <fcntl.h> + #include "qemu_snapshot.h" #include "qemu_monitor.h" @@ -1968,6 +1970,119 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm, } +static int +qemuSnapshotRevertExternal(virDomainObj *vm, + virDomainMomentObj *snap, + virDomainDef *config, + virDomainDef *inactiveConfig, + int *memsnapFD, + char **memsnapPath) +{ + size_t i; + virDomainDef *domdef = NULL; + virDomainSnapshotLocation location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + virDomainMomentObj *curSnap = virDomainSnapshotGetCurrent(vm->snapshots); + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); + virDomainSnapshotDef *curdef = virDomainSnapshotObjGetDef(curSnap); + g_autoptr(virDomainSnapshotDef) tmpsnapdef = NULL; + g_autoptr(virBitmap) created = NULL; + int ret = -1; + + if (config) { + domdef = config; + } else { + domdef = inactiveConfig; + } + + if (!(tmpsnapdef = virDomainSnapshotDefNew())) + return -1; + + if (virDomainMomentDefPostParse(&tmpsnapdef->parent) < 0) + return -1; + + if (virDomainSnapshotAlignDisks(tmpsnapdef, domdef, location, false) < 0) + return -1; + + created = virBitmapNew(tmpsnapdef->ndisks); + + if (qemuSnapshotCreateQcow2Files(vm, domdef, tmpsnapdef, created, false) < 0) + return -1; + + if (memsnapFD && memsnapPath && snapdef->memorysnapshotfile) { + virQEMUDriver *driver = ((qemuDomainObjPrivate *) vm->privateData)->driver; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + + *memsnapPath = snapdef->memorysnapshotfile; + *memsnapFD = qemuDomainOpenFile(cfg, NULL, *memsnapPath, O_RDONLY, NULL); + } + + if (config) { + if (qemuSnapshotDomainDefUpdateDisk(config, tmpsnapdef, false) < 0) + goto cleanup; + } + + if (qemuSnapshotDomainDefUpdateDisk(inactiveConfig, tmpsnapdef, false) < 0) + goto cleanup; + + if (curdef->revertdisks) { + for (i = 0; i < curdef->nrevertdisks; i++) { + virDomainSnapshotDiskDef *snapdisk = &(curdef->revertdisks[i]); + + if (unlink(snapdisk->src->path) < 0) { + VIR_WARN("Failed to remove snapshot image '%s'", + snapdisk->src->path); + } + + virDomainSnapshotDiskDefClear(snapdisk); + } + + g_clear_pointer(&curdef->revertdisks, g_free); + curdef->nrevertdisks = 0; + } else { + for (i = 0; i < curdef->ndisks; i++) { + virDomainSnapshotDiskDef *snapdisk = &(curdef->disks[i]); + + if (unlink(snapdisk->src->path) < 0) { + VIR_WARN("Failed to remove snapshot image '%s'", + snapdisk->src->path); + } + } + } + + if (snap->nchildren != 0) { + snapdef->revertdisks = g_steal_pointer(&tmpsnapdef->disks); + snapdef->nrevertdisks = tmpsnapdef->ndisks; + tmpsnapdef->ndisks = 0; + } else { + for (i = 0; i < snapdef->ndisks; i++) { + virDomainSnapshotDiskDefClear(&snapdef->disks[i]); + } + g_free(snapdef->disks); + snapdef->disks = g_steal_pointer(&tmpsnapdef->disks); + snapdef->ndisks = tmpsnapdef->ndisks; + tmpsnapdef->ndisks = 0; + } + + ret = 0; + + cleanup: + if (ret != 0 && created) { + ssize_t bit = -1; + + while ((bit = virBitmapNextSetBit(created, bit)) >= 0) { + virDomainSnapshotDiskDef *snapdisk = &(tmpsnapdef->disks[bit]); + + if (unlink(snapdisk->src->path) < 0) { + VIR_WARN("Failed to remove snapshot image '%s'", + snapdisk->src->path); + } + } + } + + return ret; +} + + static int qemuSnapshotRevertActive(virDomainObj *vm, virDomainSnapshotPtr snapshot, @@ -1982,6 +2097,9 @@ qemuSnapshotRevertActive(virDomainObj *vm, { virObjectEvent *event = NULL; virObjectEvent *event2 = NULL; + virDomainMomentObj *loadSnap = NULL; + VIR_AUTOCLOSE memsnapFD = -1; + char *memsnapPath = NULL; int detail; bool defined = false; qemuDomainSaveCookie *cookie = (qemuDomainSaveCookie *) snapdef->cookie; @@ -2003,6 +2121,15 @@ qemuSnapshotRevertActive(virDomainObj *vm, virObjectEventStateQueue(driver->domainEventState, event); } + if (virDomainSnapshotIsExternal(snap)) { + if (qemuSnapshotRevertExternal(vm, snap, *config, *inactiveConfig, + &memsnapFD, &memsnapPath) < 0) { + return -1; + } + } else { + loadSnap = snap; + } + if (*inactiveConfig) { virDomainObjAssignDef(vm, inactiveConfig, false, NULL); defined = true; @@ -2019,7 +2146,8 @@ qemuSnapshotRevertActive(virDomainObj *vm, rc = qemuProcessStart(snapshot->domain->conn, driver, vm, cookie ? cookie->cpu : NULL, - VIR_ASYNC_JOB_START, NULL, -1, NULL, snap, + VIR_ASYNC_JOB_START, NULL, memsnapFD, + memsnapPath, loadSnap, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, start_flags); virDomainAuditStart(vm, "from-snapshot", rc >= 0); @@ -2124,9 +2252,16 @@ qemuSnapshotRevertInactive(virDomainObj *vm, virObjectEventStateQueue(driver->domainEventState, event); } - if (qemuSnapshotInternalRevertInactive(driver, vm, snap) < 0) { - qemuDomainRemoveInactive(driver, vm, 0, false); - return -1; + if (virDomainSnapshotIsExternal(snap)) { + if (qemuSnapshotRevertExternal(vm, snap, NULL, *inactiveConfig, + NULL, NULL) < 0) { + return -1; + } + } else { + if (qemuSnapshotInternalRevertInactive(driver, vm, snap) < 0) { + qemuDomainRemoveInactive(driver, vm, 0, false); + return -1; + } } if (*inactiveConfig) { -- 2.39.2
On Mon, Mar 13, 2023 at 16:42:10 +0100, Pavel Hrdina wrote:
When reverting to external snapshot we need to create new overlay qcow2 files from the disk files the VM had when the snapshot was taken.
There are some specifics and limitations when reverting to a snapshot:
1) When reverting to last snapshot we need to first create new overlay files before we can safely delete the old overlay files in case the creation fails so we have still recovery option when we error out.
These new files will not have the suffix as when the snapshot was created as renaming the original files in order to use the same file names as when the snapshot was created would add unnecessary complexity to the code.
2) When reverting to any snapshot we will always create overlay files for every disk the VM had when the snapshot was done. Otherwise we would have to figure out if there is any other qcow2 image already using any of the VM disks as backing store and that itself might be extremely complex and in some cases impossible.
3) When reverting from any state the current overlay files will be always removed as that VM state is not meant to be saved. It's the same as with internal snapshots. If user want's to keep the current state before reverting they need to create a new snapshot. For now this will only work if the current snapshot is the last.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 143 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 139 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 9e4b978b1b..af4e2ea6aa 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -18,6 +18,8 @@
#include <config.h>
+#include <fcntl.h> + #include "qemu_snapshot.h"
#include "qemu_monitor.h" @@ -1968,6 +1970,119 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm, }
+static int +qemuSnapshotRevertExternal(virDomainObj *vm, + virDomainMomentObj *snap, + virDomainDef *config, + virDomainDef *inactiveConfig, + int *memsnapFD, + char **memsnapPath) +{ + size_t i; + virDomainDef *domdef = NULL; + virDomainSnapshotLocation location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + virDomainMomentObj *curSnap = virDomainSnapshotGetCurrent(vm->snapshots); + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); + virDomainSnapshotDef *curdef = virDomainSnapshotObjGetDef(curSnap); + g_autoptr(virDomainSnapshotDef) tmpsnapdef = NULL; + g_autoptr(virBitmap) created = NULL; + int ret = -1; + + if (config) { + domdef = config; + } else { + domdef = inactiveConfig; + } + + if (!(tmpsnapdef = virDomainSnapshotDefNew())) + return -1; + + if (virDomainMomentDefPostParse(&tmpsnapdef->parent) < 0)
Weird at first glance. If we ever add something more to postparse that might be a wrong thing to call here. Add a comment here that it's needed _just_ for the timestamp stuff.
+ return -1; + + if (virDomainSnapshotAlignDisks(tmpsnapdef, domdef, location, false) < 0) + return -1;
So in the end you do align the definition, thus the modification to the function you did should not be needed. You also seem to rely on the fact that this auto-selects all non-readonly disks for snapshot, but note that in case when the definition has VIR_DOMAIN_SNAPSHOT_LOCATION_NO for some disks they will not be selected. Having VIR_DOMAIN_SNAPSHOT_LOCATION_NO though doesn't mean that there isn't a snapshot of that disk as it can be overriden when specifying disks explicitly, and thus that image does have an overlay. Reverting in the way implemented here would thus invalidate the overlay. This contradicts point 2 from the commit message. Also at this point this effectively limits all of this to work on local files only as virDomainSnapshotDefAssignExternalNames works only on local files ...
+ + created = virBitmapNew(tmpsnapdef->ndisks); + + if (qemuSnapshotCreateQcow2Files(vm, domdef, tmpsnapdef, created, false) < 0) + return -1;
... thus this will for this very specific moment work. But since you'll most likely will be adding a proper revert API with XML which should allow reversion also for network disks this is limiting that work.
+ + if (memsnapFD && memsnapPath && snapdef->memorysnapshotfile) { + virQEMUDriver *driver = ((qemuDomainObjPrivate *) vm->privateData)->driver; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + + *memsnapPath = snapdef->memorysnapshotfile; + *memsnapFD = qemuDomainOpenFile(cfg, NULL, *memsnapPath, O_RDONLY, NULL); + } + + if (config) { + if (qemuSnapshotDomainDefUpdateDisk(config, tmpsnapdef, false) < 0) + goto cleanup; + } + + if (qemuSnapshotDomainDefUpdateDisk(inactiveConfig, tmpsnapdef, false) < 0) + goto cleanup; + + if (curdef->revertdisks) { + for (i = 0; i < curdef->nrevertdisks; i++) { + virDomainSnapshotDiskDef *snapdisk = &(curdef->revertdisks[i]); + + if (unlink(snapdisk->src->path) < 0) { + VIR_WARN("Failed to remove snapshot image '%s'", + snapdisk->src->path); + } + + virDomainSnapshotDiskDefClear(snapdisk); + } + + g_clear_pointer(&curdef->revertdisks, g_free); + curdef->nrevertdisks = 0; + } else { + for (i = 0; i < curdef->ndisks; i++) { + virDomainSnapshotDiskDef *snapdisk = &(curdef->disks[i]); + + if (unlink(snapdisk->src->path) < 0) { + VIR_WARN("Failed to remove snapshot image '%s'", + snapdisk->src->path); + } + } + }
Also both branches in this condition should be careful when accessing src->path unconditionally for the future use case of network disks. Additionally the 'else' branch at least can hit cases when src->path is NULL due to the disk being excluded from a snapshot.
+ + if (snap->nchildren != 0) { + snapdef->revertdisks = g_steal_pointer(&tmpsnapdef->disks); + snapdef->nrevertdisks = tmpsnapdef->ndisks; + tmpsnapdef->ndisks = 0; + } else { + for (i = 0; i < snapdef->ndisks; i++) { + virDomainSnapshotDiskDefClear(&snapdef->disks[i]); + } + g_free(snapdef->disks); + snapdef->disks = g_steal_pointer(&tmpsnapdef->disks); + snapdef->ndisks = tmpsnapdef->ndisks; + tmpsnapdef->ndisks = 0; + } + + ret = 0; + + cleanup: + if (ret != 0 && created) { + ssize_t bit = -1; + + while ((bit = virBitmapNextSetBit(created, bit)) >= 0) { + virDomainSnapshotDiskDef *snapdisk = &(tmpsnapdef->disks[bit]); + + if (unlink(snapdisk->src->path) < 0) { + VIR_WARN("Failed to remove snapshot image '%s'", + snapdisk->src->path);
Similarly to above, in certain cases 'path' can be NULL here.
+ } + } + } + + return ret; +}
On Tue, Apr 04, 2023 at 03:03:56PM +0200, Peter Krempa wrote:
On Mon, Mar 13, 2023 at 16:42:10 +0100, Pavel Hrdina wrote:
When reverting to external snapshot we need to create new overlay qcow2 files from the disk files the VM had when the snapshot was taken.
There are some specifics and limitations when reverting to a snapshot:
1) When reverting to last snapshot we need to first create new overlay files before we can safely delete the old overlay files in case the creation fails so we have still recovery option when we error out.
These new files will not have the suffix as when the snapshot was created as renaming the original files in order to use the same file names as when the snapshot was created would add unnecessary complexity to the code.
2) When reverting to any snapshot we will always create overlay files for every disk the VM had when the snapshot was done. Otherwise we would have to figure out if there is any other qcow2 image already using any of the VM disks as backing store and that itself might be extremely complex and in some cases impossible.
3) When reverting from any state the current overlay files will be always removed as that VM state is not meant to be saved. It's the same as with internal snapshots. If user want's to keep the current state before reverting they need to create a new snapshot. For now this will only work if the current snapshot is the last.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 143 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 139 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 9e4b978b1b..af4e2ea6aa 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -18,6 +18,8 @@
#include <config.h>
+#include <fcntl.h> + #include "qemu_snapshot.h"
#include "qemu_monitor.h" @@ -1968,6 +1970,119 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm, }
+static int +qemuSnapshotRevertExternal(virDomainObj *vm, + virDomainMomentObj *snap, + virDomainDef *config, + virDomainDef *inactiveConfig, + int *memsnapFD, + char **memsnapPath) +{ + size_t i; + virDomainDef *domdef = NULL; + virDomainSnapshotLocation location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + virDomainMomentObj *curSnap = virDomainSnapshotGetCurrent(vm->snapshots); + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); + virDomainSnapshotDef *curdef = virDomainSnapshotObjGetDef(curSnap); + g_autoptr(virDomainSnapshotDef) tmpsnapdef = NULL; + g_autoptr(virBitmap) created = NULL; + int ret = -1; + + if (config) { + domdef = config; + } else { + domdef = inactiveConfig; + } + + if (!(tmpsnapdef = virDomainSnapshotDefNew())) + return -1; + + if (virDomainMomentDefPostParse(&tmpsnapdef->parent) < 0)
Weird at first glance. If we ever add something more to postparse that might be a wrong thing to call here. Add a comment here that it's needed _just_ for the timestamp stuff.
I can probably just rename the function to reflect that it just generates creation time and uses it as default name so in the future if we need to add more post parse functionality we would have to create true post parse function calling this renamed helper.
+ return -1; + + if (virDomainSnapshotAlignDisks(tmpsnapdef, domdef, location, false) < 0) + return -1;
So in the end you do align the definition, thus the modification to the function you did should not be needed.
You also seem to rely on the fact that this auto-selects all non-readonly disks for snapshot, but note that in case when the definition has VIR_DOMAIN_SNAPSHOT_LOCATION_NO for some disks they will not be selected. Having VIR_DOMAIN_SNAPSHOT_LOCATION_NO though doesn't mean that there isn't a snapshot of that disk as it can be overriden when specifying disks explicitly, and thus that image does have an overlay. Reverting in the way implemented here would thus invalidate the overlay. This contradicts point 2 from the commit message.
This should not be an issue as the tmpsnapdef is a new empty snapshot definition so no disk should have VIR_DOMAIN_SNAPSHOT_LOCATION_NO. This code should just take existing domain definition and fill in the new empty snapshot definition with all disks the domain is currently using and the domain definition is from the snapshot metadata that we are reverting to. So this part should work correctly.
Also at this point this effectively limits all of this to work on local files only as virDomainSnapshotDefAssignExternalNames works only on local files ...
+ + created = virBitmapNew(tmpsnapdef->ndisks); + + if (qemuSnapshotCreateQcow2Files(vm, domdef, tmpsnapdef, created, false) < 0) + return -1;
... thus this will for this very specific moment work. But since you'll most likely will be adding a proper revert API with XML which should allow reversion also for network disks this is limiting that work.
This should not be an issue and the fact we call virDomainSnapshotDefAssignExternalNames actually helps us to not support reverting to non-local disks as with the existing API it is not possible. This function is also called unconditionally when creating external snapshots so without providing correct XML it will also fail. I need to definitely remove all code that directly uses disk->src->path but we should be able to keep virDomainSnapshotAlignDisks as the same is used when creating snapshot.
+ + if (memsnapFD && memsnapPath && snapdef->memorysnapshotfile) { + virQEMUDriver *driver = ((qemuDomainObjPrivate *) vm->privateData)->driver; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + + *memsnapPath = snapdef->memorysnapshotfile; + *memsnapFD = qemuDomainOpenFile(cfg, NULL, *memsnapPath, O_RDONLY, NULL); + } + + if (config) { + if (qemuSnapshotDomainDefUpdateDisk(config, tmpsnapdef, false) < 0) + goto cleanup; + } + + if (qemuSnapshotDomainDefUpdateDisk(inactiveConfig, tmpsnapdef, false) < 0) + goto cleanup; + + if (curdef->revertdisks) { + for (i = 0; i < curdef->nrevertdisks; i++) { + virDomainSnapshotDiskDef *snapdisk = &(curdef->revertdisks[i]); + + if (unlink(snapdisk->src->path) < 0) { + VIR_WARN("Failed to remove snapshot image '%s'", + snapdisk->src->path); + } + + virDomainSnapshotDiskDefClear(snapdisk); + } + + g_clear_pointer(&curdef->revertdisks, g_free); + curdef->nrevertdisks = 0; + } else { + for (i = 0; i < curdef->ndisks; i++) { + virDomainSnapshotDiskDef *snapdisk = &(curdef->disks[i]); + + if (unlink(snapdisk->src->path) < 0) { + VIR_WARN("Failed to remove snapshot image '%s'", + snapdisk->src->path); + } + } + }
Also both branches in this condition should be careful when accessing src->path unconditionally for the future use case of network disks.
Additionally the 'else' branch at least can hit cases when src->path is NULL due to the disk being excluded from a snapshot.
+ + if (snap->nchildren != 0) { + snapdef->revertdisks = g_steal_pointer(&tmpsnapdef->disks); + snapdef->nrevertdisks = tmpsnapdef->ndisks; + tmpsnapdef->ndisks = 0; + } else { + for (i = 0; i < snapdef->ndisks; i++) { + virDomainSnapshotDiskDefClear(&snapdef->disks[i]); + } + g_free(snapdef->disks); + snapdef->disks = g_steal_pointer(&tmpsnapdef->disks); + snapdef->ndisks = tmpsnapdef->ndisks; + tmpsnapdef->ndisks = 0; + } + + ret = 0; + + cleanup: + if (ret != 0 && created) { + ssize_t bit = -1; + + while ((bit = virBitmapNextSetBit(created, bit)) >= 0) { + virDomainSnapshotDiskDef *snapdisk = &(tmpsnapdef->disks[bit]); + + if (unlink(snapdisk->src->path) < 0) { + VIR_WARN("Failed to remove snapshot image '%s'", + snapdisk->src->path);
Similarly to above, in certain cases 'path' can be NULL here.
+ } + } + } + + return ret; +}
On Tue, Apr 04, 2023 at 19:21:01 +0200, Pavel Hrdina wrote:
On Tue, Apr 04, 2023 at 03:03:56PM +0200, Peter Krempa wrote:
On Mon, Mar 13, 2023 at 16:42:10 +0100, Pavel Hrdina wrote:
When reverting to external snapshot we need to create new overlay qcow2 files from the disk files the VM had when the snapshot was taken.
There are some specifics and limitations when reverting to a snapshot:
1) When reverting to last snapshot we need to first create new overlay files before we can safely delete the old overlay files in case the creation fails so we have still recovery option when we error out.
These new files will not have the suffix as when the snapshot was created as renaming the original files in order to use the same file names as when the snapshot was created would add unnecessary complexity to the code.
2) When reverting to any snapshot we will always create overlay files for every disk the VM had when the snapshot was done. Otherwise we would have to figure out if there is any other qcow2 image already using any of the VM disks as backing store and that itself might be extremely complex and in some cases impossible.
3) When reverting from any state the current overlay files will be always removed as that VM state is not meant to be saved. It's the same as with internal snapshots. If user want's to keep the current state before reverting they need to create a new snapshot. For now this will only work if the current snapshot is the last.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 143 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 139 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 9e4b978b1b..af4e2ea6aa 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -18,6 +18,8 @@
#include <config.h>
+#include <fcntl.h> + #include "qemu_snapshot.h"
#include "qemu_monitor.h" @@ -1968,6 +1970,119 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm, }
+static int +qemuSnapshotRevertExternal(virDomainObj *vm, + virDomainMomentObj *snap, + virDomainDef *config, + virDomainDef *inactiveConfig, + int *memsnapFD, + char **memsnapPath) +{ + size_t i; + virDomainDef *domdef = NULL; + virDomainSnapshotLocation location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + virDomainMomentObj *curSnap = virDomainSnapshotGetCurrent(vm->snapshots); + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); + virDomainSnapshotDef *curdef = virDomainSnapshotObjGetDef(curSnap); + g_autoptr(virDomainSnapshotDef) tmpsnapdef = NULL; + g_autoptr(virBitmap) created = NULL; + int ret = -1; + + if (config) { + domdef = config; + } else { + domdef = inactiveConfig; + } + + if (!(tmpsnapdef = virDomainSnapshotDefNew())) + return -1; + + if (virDomainMomentDefPostParse(&tmpsnapdef->parent) < 0)
Weird at first glance. If we ever add something more to postparse that might be a wrong thing to call here. Add a comment here that it's needed _just_ for the timestamp stuff.
That works for me too.
I can probably just rename the function to reflect that it just generates creation time and uses it as default name so in the future if we need to add more post parse functionality we would have to create true post parse function calling this renamed helper.
+ return -1; + + if (virDomainSnapshotAlignDisks(tmpsnapdef, domdef, location, false) < 0) + return -1;
So in the end you do align the definition, thus the modification to the function you did should not be needed.
You also seem to rely on the fact that this auto-selects all non-readonly disks for snapshot, but note that in case when the definition has VIR_DOMAIN_SNAPSHOT_LOCATION_NO for some disks they will not be selected. Having VIR_DOMAIN_SNAPSHOT_LOCATION_NO though doesn't mean that there isn't a snapshot of that disk as it can be overriden when specifying disks explicitly, and thus that image does have an overlay. Reverting in the way implemented here would thus invalidate the overlay. This contradicts point 2 from the commit message.
This should not be an issue as the tmpsnapdef is a new empty snapshot definition so no disk should have VIR_DOMAIN_SNAPSHOT_LOCATION_NO. This code should just take existing domain definition and fill in the new empty snapshot definition with all disks the domain is currently using and the domain definition is from the snapshot metadata that we are reverting to. So this part should work correctly.
No I'm speaking about the default disk snapshot mode that is requested in the *domain* disk configuration: <disk snapshot='no'> This configuration gets taken in case when the *snapshot* disk configuration has no setting, which is this case.
Also at this point this effectively limits all of this to work on local files only as virDomainSnapshotDefAssignExternalNames works only on local files ...
+ + created = virBitmapNew(tmpsnapdef->ndisks); + + if (qemuSnapshotCreateQcow2Files(vm, domdef, tmpsnapdef, created, false) < 0) + return -1;
... thus this will for this very specific moment work. But since you'll most likely will be adding a proper revert API with XML which should allow reversion also for network disks this is limiting that work.
This should not be an issue and the fact we call virDomainSnapshotDefAssignExternalNames actually helps us to not support reverting to non-local disks as with the existing API it is not possible. This function is also called unconditionally when creating external snapshots so without providing correct XML it will also fail.
This is called only when creating external *inactive* snapshots. For active snapshots we create/format the files using the qemu binary and the 'blockdev-create' QMP command, which is fully featured. Yes external inactive snapshots are not fully featured. I'm mentioning this here because while it works with the old-style API where you can't override the filenames and virDomainSnapshotDefAssignExternalNames doesn't work on network storage if either of them were to change this part would break. As noted I'd prefer to keep the overlay creation done via 'blockdev-create' in all cases where possible.
I need to definitely remove all code that directly uses disk->src->path but we should be able to keep virDomainSnapshotAlignDisks as the same is used when creating snapshot.
The new name reflects that we prepare data for external snapshot deletion and the old name will be used later for different part of code. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index af4e2ea6aa..64730dcb0b 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2474,9 +2474,9 @@ qemuSnapshotFindParentSnapForDisk(virDomainMomentObj *snap, static int -qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, - virDomainMomentObj *snap, - GSList **externalData) +qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, + virDomainMomentObj *snap, + GSList **externalData) { ssize_t i; virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); @@ -3333,7 +3333,7 @@ qemuSnapshotDelete(virDomainObj *vm, g_autoslist(qemuSnapshotDeleteExternalData) tmpData = NULL; /* this also serves as validation whether the snapshot can be deleted */ - if (qemuSnapshotDeleteExternalPrepare(vm, snap, &tmpData) < 0) + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, &tmpData) < 0) goto endjob; if (!virDomainObjIsActive(vm)) { @@ -3348,7 +3348,7 @@ qemuSnapshotDelete(virDomainObj *vm, /* Call the prepare again as some data require that the VM is * running to get everything we need. */ - if (qemuSnapshotDeleteExternalPrepare(vm, snap, &externalData) < 0) + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, &externalData) < 0) goto endjob; } else { qemuDomainJobPrivate *jobPriv = vm->job->privateData; -- 2.39.2
On Mon, Mar 13, 2023 at 16:42:11 +0100, Pavel Hrdina wrote:
The new name reflects that we prepare data for external snapshot deletion and the old name will be used later for different part of code.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This part of code is about to grow to make deletion work when use reverts to non-leaf snapshot. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 89 +++++++++++++++++++++++++--------------- 1 file changed, 55 insertions(+), 34 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 64730dcb0b..d545c984ce 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2557,6 +2557,58 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, } +static int +qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, + virDomainMomentObj *snap, + unsigned int flags, + GSList **externalData, + bool *stop_qemu) +{ + virQEMUDriver *driver = ((qemuDomainObjPrivate *) vm->privateData)->driver; + g_autoslist(qemuSnapshotDeleteExternalData) tmpData = NULL; + + if (!virDomainSnapshotIsExternal(snap)) + return 0; + + if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { + return 0; + } + + /* this also serves as validation whether the snapshot can be deleted */ + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, &tmpData) < 0) + return -1; + + if (!virDomainObjIsActive(vm)) { + if (qemuProcessStart(NULL, driver, vm, NULL, VIR_ASYNC_JOB_SNAPSHOT, + NULL, -1, NULL, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_CREATE, + VIR_QEMU_PROCESS_START_PAUSED) < 0) { + return -1; + } + + *stop_qemu = true; + + /* Call the prepare again as some data require that the VM is + * running to get everything we need. */ + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, externalData) < 0) + return -1; + } else { + qemuDomainJobPrivate *jobPriv = vm->job->privateData; + + *externalData = g_steal_pointer(&tmpData); + + /* If the VM is running we need to indicate that the async snapshot + * job is snapshot delete job. */ + jobPriv->snapshotDelete = true; + + qemuDomainSaveStatus(vm); + } + + return 0; +} + + typedef struct _virQEMUMomentReparent virQEMUMomentReparent; struct _virQEMUMomentReparent { const char *dir; @@ -3327,40 +3379,9 @@ qemuSnapshotDelete(virDomainObj *vm, if (qemuSnapshotDeleteValidate(vm, snap, flags) < 0) goto endjob; - if (virDomainSnapshotIsExternal(snap) && - !(flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | - VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY))) { - g_autoslist(qemuSnapshotDeleteExternalData) tmpData = NULL; - - /* this also serves as validation whether the snapshot can be deleted */ - if (qemuSnapshotDeleteExternalPrepareData(vm, snap, &tmpData) < 0) - goto endjob; - - if (!virDomainObjIsActive(vm)) { - if (qemuProcessStart(NULL, driver, vm, NULL, VIR_ASYNC_JOB_SNAPSHOT, - NULL, -1, NULL, NULL, - VIR_NETDEV_VPORT_PROFILE_OP_CREATE, - VIR_QEMU_PROCESS_START_PAUSED) < 0) { - goto endjob; - } - - stop_qemu = true; - - /* Call the prepare again as some data require that the VM is - * running to get everything we need. */ - if (qemuSnapshotDeleteExternalPrepareData(vm, snap, &externalData) < 0) - goto endjob; - } else { - qemuDomainJobPrivate *jobPriv = vm->job->privateData; - - externalData = g_steal_pointer(&tmpData); - - /* If the VM is running we need to indicate that the async snapshot - * job is snapshot delete job. */ - jobPriv->snapshotDelete = true; - - qemuDomainSaveStatus(vm); - } + if (qemuSnapshotDeleteExternalPrepare(vm, snap, flags, + &externalData, &stop_qemu) < 0) { + goto endjob; } } -- 2.39.2
On Mon, Mar 13, 2023 at 16:42:12 +0100, Pavel Hrdina wrote:
This part of code is about to grow to make deletion work when use reverts to non-leaf snapshot.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 89 +++++++++++++++++++++++++--------------- 1 file changed, 55 insertions(+), 34 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Before external snapshot revert every delete operation did block commit in order to delete a snapshot. But now when user reverts to non-leaf snapshot deleting leaf snapshot will not have any overlay files so we can just simply delete the snapshot images. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 102 ++++++++++++++++++++++++--------------- 1 file changed, 62 insertions(+), 40 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index d545c984ce..6ff18d7a9e 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2476,6 +2476,7 @@ qemuSnapshotFindParentSnapForDisk(virDomainMomentObj *snap, static int qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, virDomainMomentObj *snap, + bool blockCommit, GSList **externalData) { ssize_t i; @@ -2483,7 +2484,6 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, g_autoslist(qemuSnapshotDeleteExternalData) ret = NULL; for (i = 0; i < snapdef->ndisks; i++) { - g_autofree qemuSnapshotDeleteExternalData *data = NULL; virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]); if (snapDisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) @@ -2495,56 +2495,78 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, snapDisk->name); return -1; } + } + + if (!blockCommit) { + if (!snap->parent) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("snapshot '%s' doesn't have a parent snapshot"), + snapdef->parent.name); + return -1; + } + + snapdef = virDomainSnapshotObjGetDef(snap->parent); + } + + for (i = 0; i < snapdef->ndisks; i++) { + virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]); + g_autofree qemuSnapshotDeleteExternalData *data = NULL; + + if (snapDisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + continue; data = g_new0(qemuSnapshotDeleteExternalData, 1); data->snapDisk = snapDisk; - data->domDisk = qemuDomainDiskByName(vm->def, snapDisk->name); - if (!data->domDisk) - return -1; - - data->parentDomDisk = virDomainDiskByTarget(snapdef->parent.dom, - data->snapDisk->name); - if (!data->parentDomDisk) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("failed to find disk '%s' in snapshot VM XML"), - snapDisk->name); - return -1; - } - - if (virDomainObjIsActive(vm)) { - data->diskSrc = virStorageSourceChainLookupBySource(data->domDisk->src, - data->snapDisk->src, - &data->prevDiskSrc); - if (!data->diskSrc) + if (blockCommit) { + data->domDisk = qemuDomainDiskByName(vm->def, snapDisk->name); + if (!data->domDisk) return -1; - if (!virStorageSourceIsSameLocation(data->diskSrc, data->snapDisk->src)) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("VM disk source and snapshot disk source are not the same")); + data->parentDomDisk = virDomainDiskByTarget(snapdef->parent.dom, + data->snapDisk->name); + if (!data->parentDomDisk) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("failed to find disk '%s' in snapshot VM XML"), + snapDisk->name); return -1; } - data->parentDiskSrc = data->diskSrc->backingStore; - if (!virStorageSourceIsBacking(data->parentDiskSrc)) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("failed to find parent disk source in backing chain")); - return -1; - } + if (virDomainObjIsActive(vm)) { + data->diskSrc = virStorageSourceChainLookupBySource(data->domDisk->src, + data->snapDisk->src, + &data->prevDiskSrc); + if (!data->diskSrc) + return -1; - if (!virStorageSourceIsSameLocation(data->parentDiskSrc, data->parentDomDisk->src)) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("snapshot VM disk source and parent disk source are not the same")); - return -1; + if (!virStorageSourceIsSameLocation(data->diskSrc, data->snapDisk->src)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("VM disk source and snapshot disk source are not the same")); + return -1; + } + + data->parentDiskSrc = data->diskSrc->backingStore; + if (!virStorageSourceIsBacking(data->parentDiskSrc)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("failed to find parent disk source in backing chain")); + return -1; + } + + if (!virStorageSourceIsSameLocation(data->parentDiskSrc, + data->parentDomDisk->src)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("snapshot VM disk source and parent disk source are not the same")); + return -1; + } } - } - data->parentSnap = qemuSnapshotFindParentSnapForDisk(snap, data->snapDisk); + data->parentSnap = qemuSnapshotFindParentSnapForDisk(snap, data->snapDisk); - if (data->parentSnap && !virDomainSnapshotIsExternal(data->parentSnap)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("deleting external snapshot that has internal snapshot as parent not supported")); - return -1; + if (data->parentSnap && !virDomainSnapshotIsExternal(data->parentSnap)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("deleting external snapshot that has internal snapshot as parent not supported")); + return -1; + } } ret = g_slist_prepend(ret, g_steal_pointer(&data)); @@ -2576,7 +2598,7 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, } /* this also serves as validation whether the snapshot can be deleted */ - if (qemuSnapshotDeleteExternalPrepareData(vm, snap, &tmpData) < 0) + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, true, &tmpData) < 0) return -1; if (!virDomainObjIsActive(vm)) { @@ -2591,7 +2613,7 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, /* Call the prepare again as some data require that the VM is * running to get everything we need. */ - if (qemuSnapshotDeleteExternalPrepareData(vm, snap, externalData) < 0) + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, true, externalData) < 0) return -1; } else { qemuDomainJobPrivate *jobPriv = vm->job->privateData; -- 2.39.2
On Mon, Mar 13, 2023 at 16:42:13 +0100, Pavel Hrdina wrote:
Before external snapshot revert every delete operation did block commit in order to delete a snapshot. But now when user reverts to non-leaf snapshot deleting leaf snapshot will not have any overlay files so we can just simply delete the snapshot images.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 102 ++++++++++++++++++++++++--------------- 1 file changed, 62 insertions(+), 40 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index d545c984ce..6ff18d7a9e 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2476,6 +2476,7 @@ qemuSnapshotFindParentSnapForDisk(virDomainMomentObj *snap, static int qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, virDomainMomentObj *snap, + bool blockCommit, GSList **externalData)
This function is starting to get into the region of needing documentation after the flag was added. Also I'd call it a bit differently to de-emphasise that block commit is used, but rather that we are merging images. Either way: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
In this case there is no need to run block commit and using qemu process at all. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 52 ++++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 6ff18d7a9e..be2e5c8cc4 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2597,34 +2597,40 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, return 0; } - /* this also serves as validation whether the snapshot can be deleted */ - if (qemuSnapshotDeleteExternalPrepareData(vm, snap, true, &tmpData) < 0) - return -1; - - if (!virDomainObjIsActive(vm)) { - if (qemuProcessStart(NULL, driver, vm, NULL, VIR_ASYNC_JOB_SNAPSHOT, - NULL, -1, NULL, NULL, - VIR_NETDEV_VPORT_PROFILE_OP_CREATE, - VIR_QEMU_PROCESS_START_PAUSED) < 0) { - return -1; - } - - *stop_qemu = true; - - /* Call the prepare again as some data require that the VM is - * running to get everything we need. */ - if (qemuSnapshotDeleteExternalPrepareData(vm, snap, true, externalData) < 0) + if (snap != virDomainSnapshotGetCurrent(vm->snapshots) && + snap->nchildren == 0) { + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, false, externalData) < 0) return -1; } else { - qemuDomainJobPrivate *jobPriv = vm->job->privateData; + /* this also serves as validation whether the snapshot can be deleted */ + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, true, &tmpData) < 0) + return -1; - *externalData = g_steal_pointer(&tmpData); + if (!virDomainObjIsActive(vm)) { + if (qemuProcessStart(NULL, driver, vm, NULL, VIR_ASYNC_JOB_SNAPSHOT, + NULL, -1, NULL, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_CREATE, + VIR_QEMU_PROCESS_START_PAUSED) < 0) { + return -1; + } - /* If the VM is running we need to indicate that the async snapshot - * job is snapshot delete job. */ - jobPriv->snapshotDelete = true; + *stop_qemu = true; - qemuDomainSaveStatus(vm); + /* Call the prepare again as some data require that the VM is + * running to get everything we need. */ + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, true, externalData) < 0) + return -1; + } else { + qemuDomainJobPrivate *jobPriv = vm->job->privateData; + + *externalData = g_steal_pointer(&tmpData); + + /* If the VM is running we need to indicate that the async snapshot + * job is snapshot delete job. */ + jobPriv->snapshotDelete = true; + + qemuDomainSaveStatus(vm); + } } return 0; -- 2.39.2
On Mon, Mar 13, 2023 at 16:42:14 +0100, Pavel Hrdina wrote:
In this case there is no need to run block commit and using qemu process at all.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 52 ++++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 6ff18d7a9e..be2e5c8cc4 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2597,34 +2597,40 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm,
[...] Please explain the logic and implications in a comment above this condition.
+ if (snap != virDomainSnapshotGetCurrent(vm->snapshots) && + snap->nchildren == 0) { + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, false, externalData) < 0) return -1; } else {
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
When block commit is not needed we can just simply unlink the disk files. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 55 +++++++++++++++++++++++++--------------- 1 file changed, 35 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index be2e5c8cc4..dbcdf56758 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2414,6 +2414,7 @@ typedef struct _qemuSnapshotDeleteExternalData { virStorageSource *prevDiskSrc; /* source of disk for which @diskSrc is backing disk */ qemuBlockJobData *job; + bool blockCommit; } qemuSnapshotDeleteExternalData; @@ -2517,8 +2518,9 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, data = g_new0(qemuSnapshotDeleteExternalData, 1); data->snapDisk = snapDisk; + data->blockCommit = blockCommit; - if (blockCommit) { + if (data->blockCommit) { data->domDisk = qemuDomainDiskByName(vm->def, snapDisk->name); if (!data->domDisk) return -1; @@ -2949,31 +2951,41 @@ qemuSnapshotDiscardExternal(virDomainObj *vm, virTristateBool autofinalize = VIR_TRISTATE_BOOL_NO; unsigned int commitFlags = VIR_DOMAIN_BLOCK_COMMIT_DELETE; - if (data->domDisk->src == data->diskSrc) { - commitFlags |= VIR_DOMAIN_BLOCK_COMMIT_ACTIVE; - autofinalize = VIR_TRISTATE_BOOL_YES; + if (data->blockCommit) { + if (data->domDisk->src == data->diskSrc) { + commitFlags |= VIR_DOMAIN_BLOCK_COMMIT_ACTIVE; + autofinalize = VIR_TRISTATE_BOOL_YES; + } + + if (qemuSnapshotSetInvalid(vm, data->parentSnap, data->snapDisk, true) < 0) + goto error; + + data->job = qemuBlockCommit(vm, + data->domDisk, + data->parentDiskSrc, + data->diskSrc, + data->prevDiskSrc, + 0, + VIR_ASYNC_JOB_SNAPSHOT, + autofinalize, + commitFlags); + + if (!data->job) + goto error; + } else { + if (unlink(data->snapDisk->src->path) < 0) { + VIR_WARN("Failed to remove snapshot image '%s'", + data->snapDisk->name); + } } - - if (qemuSnapshotSetInvalid(vm, data->parentSnap, data->snapDisk, true) < 0) - goto error; - - data->job = qemuBlockCommit(vm, - data->domDisk, - data->parentDiskSrc, - data->diskSrc, - data->prevDiskSrc, - 0, - VIR_ASYNC_JOB_SNAPSHOT, - autofinalize, - commitFlags); - - if (!data->job) - goto error; } for (cur = externalData; cur; cur = g_slist_next(cur)) { qemuSnapshotDeleteExternalData *data = cur->data; + if (!data->blockCommit) + continue; + if (qemuSnapshotDeleteBlockJobRunning(vm, data->job) < 0) goto error; @@ -2988,6 +3000,9 @@ qemuSnapshotDiscardExternal(virDomainObj *vm, for (cur = externalData; cur; cur = g_slist_next(cur)) { qemuSnapshotDeleteExternalData *data = cur->data; + if (!data->blockCommit) + continue; + if (data->job->state == QEMU_BLOCKJOB_STATE_READY) { if (qemuBlockPivot(vm, data->job, VIR_ASYNC_JOB_SNAPSHOT, NULL) < 0) goto error; -- 2.39.2
On Mon, Mar 13, 2023 at 16:42:15 +0100, Pavel Hrdina wrote:
When block commit is not needed we can just simply unlink the disk files.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 55 +++++++++++++++++++++++++--------------- 1 file changed, 35 insertions(+), 20 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index be2e5c8cc4..dbcdf56758 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c
[...]
@@ -2949,31 +2951,41 @@ qemuSnapshotDiscardExternal(virDomainObj *vm, virTristateBool autofinalize = VIR_TRISTATE_BOOL_NO; unsigned int commitFlags = VIR_DOMAIN_BLOCK_COMMIT_DELETE;
- if (data->domDisk->src == data->diskSrc) { - commitFlags |= VIR_DOMAIN_BLOCK_COMMIT_ACTIVE; - autofinalize = VIR_TRISTATE_BOOL_YES; + if (data->blockCommit) { + if (data->domDisk->src == data->diskSrc) { + commitFlags |= VIR_DOMAIN_BLOCK_COMMIT_ACTIVE; + autofinalize = VIR_TRISTATE_BOOL_YES; + } + + if (qemuSnapshotSetInvalid(vm, data->parentSnap, data->snapDisk, true) < 0) + goto error; + + data->job = qemuBlockCommit(vm, + data->domDisk, + data->parentDiskSrc, + data->diskSrc, + data->prevDiskSrc, + 0, + VIR_ASYNC_JOB_SNAPSHOT, + autofinalize, + commitFlags); + + if (!data->job) + goto error; + } else { + if (unlink(data->snapDisk->src->path) < 0) {
This can be done only when the 'src' object is "local storage" (virStorageSourceIsLocalStorage)
+ VIR_WARN("Failed to remove snapshot image '%s'", + data->snapDisk->name); + }
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
When deleting external snapshot and parent snapshot is the currently active snapshot as user reverted to it we need to properly update the parent snapshot metadata. After the delete is done the new overlay files will be the currently used files created when snapshot revert was done, replacing the original overlay files. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index dbcdf56758..513bcb5a86 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3063,6 +3063,41 @@ qemuSnapshotDiscardExternal(virDomainObj *vm, } +static int +qemuSnapshotDeleteUpdateParent(virDomainObj *vm, + virDomainMomentObj *parent) +{ + size_t i; + virQEMUDriver *driver = ((qemuDomainObjPrivate *) vm->privateData)->driver; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + virDomainSnapshotDef *parentDef = virDomainSnapshotObjGetDef(parent); + + if (!parentDef) + return 0; + + if (!parentDef->revertdisks) + return 0; + + for (i = 0; i < parentDef->ndisks; i++) { + virDomainSnapshotDiskDefClear(&parentDef->disks[i]); + } + g_free(parentDef->disks); + + parentDef->disks = g_steal_pointer(&parentDef->revertdisks); + parentDef->ndisks = parentDef->nrevertdisks; + parentDef->nrevertdisks = 0; + + if (qemuDomainSnapshotWriteMetadata(vm, + parent, + driver->xmlopt, + cfg->snapshotDir) < 0) { + return -1; + } + + return 0; +} + + static int qemuSnapshotDiscardMetadata(virDomainObj *vm, virDomainMomentObj *snap, @@ -3102,6 +3137,11 @@ qemuSnapshotDiscardMetadata(virDomainObj *vm, virDomainMomentMoveChildren(snap, snap->parent); } + if (update_parent && snap->parent) { + if (qemuSnapshotDeleteUpdateParent(vm, snap->parent) < 0) + ret = -1; + } + snapFile = g_strdup_printf("%s/%s/%s.xml", cfg->snapshotDir, vm->def->name, snap->def->name); -- 2.39.2
On Mon, Mar 13, 2023 at 16:42:16 +0100, Pavel Hrdina wrote:
When deleting external snapshot and parent snapshot is the currently active snapshot as user reverted to it we need to properly update the parent snapshot metadata.
After the delete is done the new overlay files will be the currently used files created when snapshot revert was done, replacing the original overlay files.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index dbcdf56758..513bcb5a86 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3063,6 +3063,41 @@ qemuSnapshotDiscardExternal(virDomainObj *vm, }
+static int +qemuSnapshotDeleteUpdateParent(virDomainObj *vm, + virDomainMomentObj *parent) +{ + size_t i; + virQEMUDriver *driver = ((qemuDomainObjPrivate *) vm->privateData)->driver;
You can use QEMU_DOMAIN_PRIVATE(vm)->driver here to hide the typecast.
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + virDomainSnapshotDef *parentDef = virDomainSnapshotObjGetDef(parent); + + if (!parentDef) + return 0; + + if (!parentDef->revertdisks) + return 0;
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This would allow creating new branch in the external snapshot history which we currently don't support. The issue with branching the external snapshot history is that deleting some snapshot would require calling block-stream instead of block-commit which is currently not implemented. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 513bcb5a86..95297cfe6a 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -665,6 +665,8 @@ qemuSnapshotPrepare(virDomainObj *vm, bool found_internal = false; bool forbid_internal = false; int external = 0; + virDomainMomentObj *curSnap = NULL; + virDomainSnapshotDef *curDef = NULL; for (i = 0; i < def->ndisks; i++) { virDomainSnapshotDiskDef *disk = &def->disks[i]; @@ -801,6 +803,16 @@ qemuSnapshotPrepare(virDomainObj *vm, return -1; } + curSnap = virDomainSnapshotGetCurrent(vm->snapshots); + if (curSnap) + curDef = virDomainSnapshotObjGetDef(curSnap); + + if (curDef && curDef->revertdisks) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("creating external snapshot after reverting to not last snapshot is not supported")); + return -1; + } + /* Alter flags to let later users know what we learned. */ if (external && !active) *flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; -- 2.39.2
On Mon, Mar 13, 2023 at 16:42:17 +0100, Pavel Hrdina wrote:
This would allow creating new branch in the external snapshot history which we currently don't support.
The issue with branching the external snapshot history is that deleting some snapshot would require calling block-stream instead of block-commit which is currently not implemented.
If the problem is with deletion of the snapshot afterwards I strongly suggest detecting and limiting the deletion itself. Creation doesn't seem to be the problem and it feels weird to restrict it.
This new helper will allow us to check if we are able to delete external snapshot after user did revert to non-leaf snapshot. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/virdomainmomentobjlist.c | 21 +++++++++++++++++++++ src/conf/virdomainmomentobjlist.h | 4 ++++ src/libvirt_private.syms | 1 + 3 files changed, 26 insertions(+) diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c index f19ec3319a..0c520fb173 100644 --- a/src/conf/virdomainmomentobjlist.c +++ b/src/conf/virdomainmomentobjlist.c @@ -582,3 +582,24 @@ virDomainMomentFindLeaf(virDomainMomentObjList *list) return moment; return NULL; } + + +bool +virDomainMomentIsAncestor(virDomainMomentObj *moment, + virDomainMomentObj *ancestor) +{ + if (!moment) + return false; + + if (moment == ancestor) + return false; + + do { + moment = moment->parent; + + if (moment == ancestor) + return true; + } while (moment); + + return false; +} diff --git a/src/conf/virdomainmomentobjlist.h b/src/conf/virdomainmomentobjlist.h index d2ab3b46b1..2ea6b181c0 100644 --- a/src/conf/virdomainmomentobjlist.h +++ b/src/conf/virdomainmomentobjlist.h @@ -157,3 +157,7 @@ virDomainMomentCheckCycles(virDomainMomentObjList *list, virDomainMomentObj * virDomainMomentFindLeaf(virDomainMomentObjList *list); + +bool +virDomainMomentIsAncestor(virDomainMomentObj *moment, + virDomainMomentObj *ancestor); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dd752ba55c..8a724ef241 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1222,6 +1222,7 @@ virDomainMomentDropChildren; virDomainMomentDropParent; virDomainMomentForEachChild; virDomainMomentForEachDescendant; +virDomainMomentIsAncestor; virDomainMomentMoveChildren; virDomainMomentObjFree; virDomainMomentObjNew; -- 2.39.2
On Mon, Mar 13, 2023 at 16:42:18 +0100, Pavel Hrdina wrote:
This new helper will allow us to check if we are able to delete external snapshot after user did revert to non-leaf snapshot.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/virdomainmomentobjlist.c | 21 +++++++++++++++++++++ src/conf/virdomainmomentobjlist.h | 4 ++++ src/libvirt_private.syms | 1 + 3 files changed, 26 insertions(+)
diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c index f19ec3319a..0c520fb173 100644 --- a/src/conf/virdomainmomentobjlist.c +++ b/src/conf/virdomainmomentobjlist.c @@ -582,3 +582,24 @@ virDomainMomentFindLeaf(virDomainMomentObjList *list) return moment; return NULL; } + + +bool +virDomainMomentIsAncestor(virDomainMomentObj *moment, + virDomainMomentObj *ancestor) +{ + if (!moment) + return false;
for (moment = moment->parent; moment; moment = moment->parent) { if (moment == ancestor) return true; } return false; This should be sufficient IIUC to replace the condition and the loop, right? Either way: Reviewed-by: Peter Krempa <pkrempa@redhat.com> Consider documenting expectations though.
There will be more external snapshot checks introduced by following patch so group them together. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 95297cfe6a..05386b11fe 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3426,18 +3426,18 @@ qemuSnapshotDeleteValidate(virDomainObj *vm, } } - if (virDomainSnapshotIsExternal(snap) && - qemuDomainHasBlockjob(vm, false)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("cannot delete external snapshots when there is another active block job")); - return -1; - } + if (virDomainSnapshotIsExternal(snap)) { + if (qemuDomainHasBlockjob(vm, false)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("cannot delete external snapshots when there is another active block job")); + return -1; + } - if (virDomainSnapshotIsExternal(snap) && - (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("deletion of external disk snapshots with children not supported")); - return -1; + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("deletion of external disk snapshots with children not supported")); + return -1; + } } return 0; -- 2.39.2
On Mon, Mar 13, 2023 at 16:42:19 +0100, Pavel Hrdina wrote:
There will be more external snapshot checks introduced by following patch so group them together.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
With the introduction of external snapshot revert support we need to error out in some cases when trying to delete some snapshots. If users reverts to non-leaf snapshots and would try to delete it after the revert is done it would not work currently as this operation would require using block-stream which is not implemented for now as in this case the snapshot has two children so the disk files have multiple overlays. Similarly if user reverts to non-leaf snapshot and would try to delete snapshot that is non-leaf but not in currently active snapshot chain we would still need to use block-commit operation. The issue here is that in order to do that we would have to start new qemu process with different domain definition than what is currently used by the domain. If the current domain would be running it would complicate things even more so this operation is not yet supported. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 05386b11fe..c6c7b077dd 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3427,6 +3427,8 @@ qemuSnapshotDeleteValidate(virDomainObj *vm, } if (virDomainSnapshotIsExternal(snap)) { + virDomainMomentObj *current = virDomainSnapshotGetCurrent(vm->snapshots); + if (qemuDomainHasBlockjob(vm, false)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot delete external snapshots when there is another active block job")); @@ -3438,6 +3440,19 @@ qemuSnapshotDeleteValidate(virDomainObj *vm, _("deletion of external disk snapshots with children not supported")); return -1; } + + if (snap == current && snap->nchildren != 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("deletion of active external snapshot that is not a leaf snapshot is not supported")); + return -1; + } + + if (snap != current && snap->nchildren != 0 && + virDomainMomentIsAncestor(snap, current)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("deletion of non-leaf external snapshot that is not in active chain is not supported ")); + return -1; + } } return 0; -- 2.39.2
Now that the support to revert external snapshots is implemented we can drop this check. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index c6c7b077dd..ec5b2e68d6 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1864,12 +1864,6 @@ qemuSnapshotRevertValidate(virDomainObj *vm, return -1; } - if (virDomainSnapshotIsExternal(snap)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("revert to external snapshot not supported yet")); - return -1; - } - if (!snap->def->dom) { virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, _("snapshot '%s' lacks domain '%s' rollback info"), -- 2.39.2
participants (2)
-
Pavel Hrdina -
Peter Krempa