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.2On 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.
>
© 2016 - 2024 Red Hat, Inc.