[libvirt PATCH 00/11] improve and cleanup internal snapshot revert
This fixes few issues with reverting internal snapshot where the VM was modified and our compatibility check code failed to detect it, for example changing one of the VM disks. It also cleanups the code significantly as a preparation for other cleanups and future external snapshot implementation. Pavel Hrdina (11): qemu_snapshot: revert: always error out if VM XML is missing qemu_snapshot: revert: always restart QEMU process for running VM qemu_snapshot: revert: drop unused loadvm code qemu_snapshot: revert: fix emitting events qemu_snapshot: revert: drop error that QEMU process must be restarted test: snapshot revert: always error out if VM XML is missing test: snapshot revert: always emulate VM process stop test: snapshot revert: drop unused code test: snapshot revert: fix emitting events test: snapshot revert: drop error the VM must be restarted domain_snapshot: update virDomainRevertToSnapshot description src/libvirt-domain-snapshot.c | 5 + src/qemu/qemu_snapshot.c | 244 +++++++++------------------------- src/test/test_driver.c | 121 ++++------------- 3 files changed, 93 insertions(+), 277 deletions(-) -- 2.31.1
The support to revert snapshots was introduced in libvirt 0.8.0 but saving the whole VM XML was implemented later in libvirt 0.9.5. That is more then 10 years ago so we can safely assume that nobody will try reverting to snapshot created by that old libvirt. In the unlikely scenario where someone would actually did it we would simply error out. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 138 +++++++++++++++++++-------------------- 1 file changed, 66 insertions(+), 72 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index d105eead27..3d6ec490ab 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1908,13 +1908,14 @@ qemuSnapshotRevert(virDomainObj *vm, goto endjob; } + if (!snap->def->dom) { + virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, + _("snapshot '%s' lacks domain '%s' rollback info"), + snap->def->name, vm->def->name); + goto endjob; + } + if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) { - if (!snap->def->dom) { - virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, - _("snapshot '%s' lacks domain '%s' rollback info"), - snap->def->name, vm->def->name); - goto endjob; - } if (virDomainObjIsActive(vm) && !(snapdef->state == VIR_DOMAIN_SNAPSHOT_RUNNING || snapdef->state == VIR_DOMAIN_SNAPSHOT_PAUSED) && @@ -1935,16 +1936,14 @@ qemuSnapshotRevert(virDomainObj *vm, } } - if (snap->def->dom) { - config = virDomainDefCopy(snap->def->dom, - driver->xmlopt, priv->qemuCaps, true); - if (!config) - goto endjob; + config = virDomainDefCopy(snap->def->dom, + driver->xmlopt, priv->qemuCaps, true); + if (!config) + goto endjob; - if (STRNEQ(config->name, vm->def->name)) { - VIR_FREE(config->name); - config->name = g_strdup(vm->def->name); - } + if (STRNEQ(config->name, vm->def->name)) { + VIR_FREE(config->name); + config->name = g_strdup(vm->def->name); } if (snap->def->inactiveDom) { @@ -1992,61 +1991,59 @@ qemuSnapshotRevert(virDomainObj *vm, /* Transitions 5, 6, 8, 9 */ /* Check for ABI compatibility. We need to do this check against * the migratable XML or it will always fail otherwise */ - if (config) { - bool compatible; + bool compatible; - /* Replace the CPU in config and put the original one in priv - * once we're done. When we have the updated CPU def in the - * cookie, we don't want to replace the CPU in migratable def - * when doing ABI checks to make sure the current CPU exactly - * matches the one used at the time the snapshot was taken. - */ - if (cookie && cookie->cpu && config->cpu) { - origCPU = config->cpu; - if (!(config->cpu = virCPUDefCopy(cookie->cpu))) - goto endjob; + /* Replace the CPU in config and put the original one in priv + * once we're done. When we have the updated CPU def in the + * cookie, we don't want to replace the CPU in migratable def + * when doing ABI checks to make sure the current CPU exactly + * matches the one used at the time the snapshot was taken. + */ + if (cookie && cookie->cpu && config->cpu) { + origCPU = config->cpu; + if (!(config->cpu = virCPUDefCopy(cookie->cpu))) + goto endjob; - compatible = qemuDomainDefCheckABIStability(driver, - priv->qemuCaps, - vm->def, - config); - } else { - compatible = qemuDomainCheckABIStability(driver, vm, config); - } + compatible = qemuDomainDefCheckABIStability(driver, + priv->qemuCaps, + vm->def, + config); + } else { + compatible = qemuDomainCheckABIStability(driver, vm, config); + } - /* If using VM GenID, there is no way currently to change - * the genid for the running guest, so set an error, - * mark as incompatible, and don't allow change of genid - * if the revert force flag would start the guest again. */ - if (compatible && config->genidRequested) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("domain genid update requires restart")); - compatible = false; - start_flags &= ~VIR_QEMU_PROCESS_START_GEN_VMID; - } + /* If using VM GenID, there is no way currently to change + * the genid for the running guest, so set an error, + * mark as incompatible, and don't allow change of genid + * if the revert force flag would start the guest again. */ + if (compatible && config->genidRequested) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("domain genid update requires restart")); + compatible = false; + start_flags &= ~VIR_QEMU_PROCESS_START_GEN_VMID; + } - if (!compatible) { - virErrorPtr err = virGetLastError(); + if (!compatible) { + virErrorPtr err = virGetLastError(); - if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) { - /* Re-spawn error using correct category. */ - if (err->code == VIR_ERR_CONFIG_UNSUPPORTED) - virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s", - err->str2); - goto endjob; - } - virResetError(err); - qemuProcessStop(driver, vm, - VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, - QEMU_ASYNC_JOB_START, 0); - virDomainAuditStop(vm, "from-snapshot"); - detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT; - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_STOPPED, - detail); - virObjectEventStateQueue(driver->domainEventState, event); - goto load; + if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) { + /* Re-spawn error using correct category. */ + if (err->code == VIR_ERR_CONFIG_UNSUPPORTED) + virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s", + err->str2); + goto endjob; } + virResetError(err); + qemuProcessStop(driver, vm, + VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, + QEMU_ASYNC_JOB_START, 0); + virDomainAuditStop(vm, "from-snapshot"); + detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT; + event = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_STOPPED, + detail); + virObjectEventStateQueue(driver->domainEventState, event); + goto load; } if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { @@ -2073,10 +2070,9 @@ qemuSnapshotRevert(virDomainObj *vm, * failed loadvm attempt? */ goto endjob; } - if (config) { - virCPUDefFree(priv->origCPU); - priv->origCPU = g_steal_pointer(&origCPU); - } + + virCPUDefFree(priv->origCPU); + priv->origCPU = g_steal_pointer(&origCPU); if (cookie && !cookie->slirpHelper) priv->disableSlirp = true; @@ -2097,10 +2093,8 @@ qemuSnapshotRevert(virDomainObj *vm, defined = true; } - if (config) { - virDomainObjAssignDef(vm, config, true, NULL); - config = NULL; - } + virDomainObjAssignDef(vm, config, true, NULL); + config = NULL; /* No cookie means libvirt which saved the domain was too old to * mess up the CPU definitions. -- 2.31.1
On Mon, Nov 15, 2021 at 17:22:44 +0100, Pavel Hrdina wrote:
The support to revert snapshots was introduced in libvirt 0.8.0 but saving the whole VM XML was implemented later in libvirt 0.9.5.
That is more then 10 years ago so we can safely assume that nobody will try reverting to snapshot created by that old libvirt. In the unlikely scenario where someone would actually did it we would simply error out.
In addition we don't even support QEMUs which were around at that time, so with reasonable configuration this will not be a problem because such old libvirt on the other hand wouldn't be able to drive new qemus.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 138 +++++++++++++++++++-------------------- 1 file changed, 66 insertions(+), 72 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Our compatibility check code isn't complete and there are cases where it fails to detect incompatible configuration and the revert fails. In addition future support for external snapshot will always require restarting the QEMU process. To unify the behavior drop the compatibility check code and always restart the QEMU process. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 66 ++++++---------------------------------- 1 file changed, 10 insertions(+), 56 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 3d6ec490ab..661f74146c 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1989,62 +1989,16 @@ qemuSnapshotRevert(virDomainObj *vm, * to have finer control. */ if (virDomainObjIsActive(vm)) { /* Transitions 5, 6, 8, 9 */ - /* Check for ABI compatibility. We need to do this check against - * the migratable XML or it will always fail otherwise */ - bool compatible; - - /* Replace the CPU in config and put the original one in priv - * once we're done. When we have the updated CPU def in the - * cookie, we don't want to replace the CPU in migratable def - * when doing ABI checks to make sure the current CPU exactly - * matches the one used at the time the snapshot was taken. - */ - if (cookie && cookie->cpu && config->cpu) { - origCPU = config->cpu; - if (!(config->cpu = virCPUDefCopy(cookie->cpu))) - goto endjob; - - compatible = qemuDomainDefCheckABIStability(driver, - priv->qemuCaps, - vm->def, - config); - } else { - compatible = qemuDomainCheckABIStability(driver, vm, config); - } - - /* If using VM GenID, there is no way currently to change - * the genid for the running guest, so set an error, - * mark as incompatible, and don't allow change of genid - * if the revert force flag would start the guest again. */ - if (compatible && config->genidRequested) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("domain genid update requires restart")); - compatible = false; - start_flags &= ~VIR_QEMU_PROCESS_START_GEN_VMID; - } - - if (!compatible) { - virErrorPtr err = virGetLastError(); - - if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) { - /* Re-spawn error using correct category. */ - if (err->code == VIR_ERR_CONFIG_UNSUPPORTED) - virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s", - err->str2); - goto endjob; - } - virResetError(err); - qemuProcessStop(driver, vm, - VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, - QEMU_ASYNC_JOB_START, 0); - virDomainAuditStop(vm, "from-snapshot"); - detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT; - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_STOPPED, - detail); - virObjectEventStateQueue(driver->domainEventState, event); - goto load; - } + qemuProcessStop(driver, vm, + VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, + QEMU_ASYNC_JOB_START, 0); + virDomainAuditStop(vm, "from-snapshot"); + detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT; + event = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_STOPPED, + detail); + virObjectEventStateQueue(driver->domainEventState, event); + goto load; if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { /* Transitions 5, 6 */ -- 2.31.1
On Mon, Nov 15, 2021 at 17:22:45 +0100, Pavel Hrdina wrote:
Our compatibility check code isn't complete and there are cases where it fails to detect incompatible configuration and the revert fails. In addition future support for external snapshot will always require restarting the QEMU process.
To unify the behavior drop the compatibility check code and always restart the QEMU process.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 66 ++++++---------------------------------- 1 file changed, 10 insertions(+), 56 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 3d6ec490ab..661f74146c 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1989,62 +1989,16 @@ qemuSnapshotRevert(virDomainObj *vm,
[...]
/* Transitions 5, 6, 8, 9 */ - /* If using VM GenID, there is no way currently to change - * the genid for the running guest, so set an error, - * mark as incompatible, and don't allow change of genid - * if the revert force flag would start the guest again. */ - if (compatible && config->genidRequested) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("domain genid update requires restart")); - compatible = false; - start_flags &= ~VIR_QEMU_PROCESS_START_GEN_VMID;
We still need this bit. If genid is requested we must ensure that the new start of the VM will regenerate it to ensure that the guest can detect the reversion. Apart from that I agree that the "feature" of not restarting qemu for some reversions was more of a gimmick and micro-optimization and getting rid of it for code clarity is worthwhile even when it results in the users losing/needing reconnect the remote viewer connections. Once you ensure that the genid is always regenerated when present: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On Tue, Nov 16, 2021 at 03:17:56PM +0100, Peter Krempa wrote:
On Mon, Nov 15, 2021 at 17:22:45 +0100, Pavel Hrdina wrote:
Our compatibility check code isn't complete and there are cases where it fails to detect incompatible configuration and the revert fails. In addition future support for external snapshot will always require restarting the QEMU process.
To unify the behavior drop the compatibility check code and always restart the QEMU process.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 66 ++++++---------------------------------- 1 file changed, 10 insertions(+), 56 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 3d6ec490ab..661f74146c 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1989,62 +1989,16 @@ qemuSnapshotRevert(virDomainObj *vm,
[...]
/* Transitions 5, 6, 8, 9 */ - /* If using VM GenID, there is no way currently to change - * the genid for the running guest, so set an error, - * mark as incompatible, and don't allow change of genid - * if the revert force flag would start the guest again. */ - if (compatible && config->genidRequested) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("domain genid update requires restart")); - compatible = false; - start_flags &= ~VIR_QEMU_PROCESS_START_GEN_VMID;
We still need this bit. If genid is requested we must ensure that the new start of the VM will regenerate it to ensure that the guest can detect the reversion.
At the beginning of the function qemuSnapshotRevert() there is this line: unsigned int start_flags = VIR_QEMU_PROCESS_START_GEN_VMID; so generating of VMID is enabled by default. This part of code disabled the VMID regeneration if FORCE flag had to be used to revert the snapshot. That change was introduced by commit <e5d7064be02a0105b7c> but it doesn't make sense to me. The commit messages states: If the the snapshot revert involves a forced revert option, then let's not cause startup to change the genid flag in order to signify that we're still running the same/previous guest and not some snapshot reversion. but how we would run the same guest if this code is reverting to different snapshot? Pavel
Apart from that I agree that the "feature" of not restarting qemu for some reversions was more of a gimmick and micro-optimization and getting rid of it for code clarity is worthwhile even when it results in the users losing/needing reconnect the remote viewer connections.
Once you ensure that the genid is always regenerated when present:
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On Mon, Nov 22, 2021 at 12:32:03 +0100, Pavel Hrdina wrote:
On Tue, Nov 16, 2021 at 03:17:56PM +0100, Peter Krempa wrote:
On Mon, Nov 15, 2021 at 17:22:45 +0100, Pavel Hrdina wrote:
Our compatibility check code isn't complete and there are cases where it fails to detect incompatible configuration and the revert fails. In addition future support for external snapshot will always require restarting the QEMU process.
To unify the behavior drop the compatibility check code and always restart the QEMU process.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 66 ++++++---------------------------------- 1 file changed, 10 insertions(+), 56 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 3d6ec490ab..661f74146c 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1989,62 +1989,16 @@ qemuSnapshotRevert(virDomainObj *vm,
[...]
/* Transitions 5, 6, 8, 9 */ - /* If using VM GenID, there is no way currently to change - * the genid for the running guest, so set an error, - * mark as incompatible, and don't allow change of genid - * if the revert force flag would start the guest again. */ - if (compatible && config->genidRequested) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("domain genid update requires restart")); - compatible = false; - start_flags &= ~VIR_QEMU_PROCESS_START_GEN_VMID;
We still need this bit. If genid is requested we must ensure that the new start of the VM will regenerate it to ensure that the guest can detect the reversion.
At the beginning of the function qemuSnapshotRevert() there is this line:
unsigned int start_flags = VIR_QEMU_PROCESS_START_GEN_VMID;
so generating of VMID is enabled by default. This part of code disabled the VMID regeneration if FORCE flag had to be used to revert the snapshot. That change was introduced by commit <e5d7064be02a0105b7c> but it doesn't make sense to me.
The commit messages states:
If the the snapshot revert involves a forced revert option, then let's not cause startup to change the genid flag in order to signify that we're still running the same/previous guest and not some snapshot reversion.
but how we would run the same guest if this code is reverting to different snapshot?
Oh, I didn't actually notice that this is masking out the VIR_QEMU_PROCESS_START_GEN_VMID flag, so yeah it can be removed completely.
Now that we always restart QEMU process the loadvm code is unused and can be dropped. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 96 +++++++++++----------------------------- 1 file changed, 26 insertions(+), 70 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 661f74146c..251a0e5cfa 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1982,11 +1982,6 @@ qemuSnapshotRevert(virDomainObj *vm, start_flags |= VIR_QEMU_PROCESS_START_PAUSED; /* Transitions 2, 3, 5, 6, 8, 9 */ - /* When using the loadvm monitor command, qemu does not know - * whether to pause or run the reverted domain, and just stays - * in the same state as before the monitor command, whether - * that is paused or running. We always pause before loadvm, - * to have finer control. */ if (virDomainObjIsActive(vm)) { /* Transitions 5, 6, 8, 9 */ qemuProcessStop(driver, vm, @@ -1998,78 +1993,39 @@ qemuSnapshotRevert(virDomainObj *vm, VIR_DOMAIN_EVENT_STOPPED, detail); virObjectEventStateQueue(driver->domainEventState, event); - goto load; - - if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { - /* Transitions 5, 6 */ - if (qemuProcessStopCPUs(driver, vm, - VIR_DOMAIN_PAUSED_FROM_SNAPSHOT, - QEMU_ASYNC_JOB_START) < 0) - goto endjob; - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("guest unexpectedly quit")); - goto endjob; - } - } - - if (qemuDomainObjEnterMonitorAsync(driver, vm, - QEMU_ASYNC_JOB_START) < 0) - goto endjob; - rc = qemuMonitorLoadSnapshot(priv->mon, snap->def->name); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto endjob; - if (rc < 0) { - /* XXX resume domain if it was running before the - * failed loadvm attempt? */ - goto endjob; - } - - virCPUDefFree(priv->origCPU); - priv->origCPU = g_steal_pointer(&origCPU); - - if (cookie && !cookie->slirpHelper) - priv->disableSlirp = true; - - if (inactiveConfig) { - virDomainObjAssignDef(vm, inactiveConfig, false, NULL); - inactiveConfig = NULL; - defined = true; - } } else { /* Transitions 2, 3 */ - load: was_stopped = true; + } - if (inactiveConfig) { - virDomainObjAssignDef(vm, inactiveConfig, false, NULL); - inactiveConfig = NULL; - defined = true; - } + if (inactiveConfig) { + virDomainObjAssignDef(vm, inactiveConfig, false, NULL); + inactiveConfig = NULL; + defined = true; + } - virDomainObjAssignDef(vm, config, true, NULL); - config = NULL; + virDomainObjAssignDef(vm, config, true, NULL); + config = NULL; - /* No cookie means libvirt which saved the domain was too old to - * mess up the CPU definitions. - */ - if (cookie && - qemuDomainFixupCPUs(vm, &cookie->cpu) < 0) - goto cleanup; + /* No cookie means libvirt which saved the domain was too old to + * mess up the CPU definitions. + */ + if (cookie && + qemuDomainFixupCPUs(vm, &cookie->cpu) < 0) + goto cleanup; - rc = qemuProcessStart(snapshot->domain->conn, driver, vm, - cookie ? cookie->cpu : NULL, - QEMU_ASYNC_JOB_START, NULL, -1, NULL, snap, - VIR_NETDEV_VPORT_PROFILE_OP_CREATE, - start_flags); - virDomainAuditStart(vm, "from-snapshot", rc >= 0); - detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_STARTED, - detail); - if (rc < 0) - goto endjob; - } + rc = qemuProcessStart(snapshot->domain->conn, driver, vm, + cookie ? cookie->cpu : NULL, + QEMU_ASYNC_JOB_START, NULL, -1, NULL, snap, + VIR_NETDEV_VPORT_PROFILE_OP_CREATE, + start_flags); + virDomainAuditStart(vm, "from-snapshot", rc >= 0); + detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; + event = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_STARTED, + detail); + if (rc < 0) + goto endjob; /* Touch up domain state. */ if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) && -- 2.31.1
On Mon, Nov 15, 2021 at 17:22:46 +0100, Pavel Hrdina wrote:
Now that we always restart QEMU process the loadvm code is unused and can be dropped.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 96 +++++++++++----------------------------- 1 file changed, 26 insertions(+), 70 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 661f74146c..251a0e5cfa 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1982,11 +1982,6 @@ qemuSnapshotRevert(virDomainObj *vm, start_flags |= VIR_QEMU_PROCESS_START_PAUSED;
/* Transitions 2, 3, 5, 6, 8, 9 */ - /* When using the loadvm monitor command, qemu does not know - * whether to pause or run the reverted domain, and just stays - * in the same state as before the monitor command, whether - * that is paused or running. We always pause before loadvm, - * to have finer control. */ if (virDomainObjIsActive(vm)) { /* Transitions 5, 6, 8, 9 */ qemuProcessStop(driver, vm, @@ -1998,78 +1993,39 @@ qemuSnapshotRevert(virDomainObj *vm, VIR_DOMAIN_EVENT_STOPPED, detail); virObjectEventStateQueue(driver->domainEventState, event); - goto load;
So this jumped ...
- - if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { - /* Transitions 5, 6 */ - if (qemuProcessStopCPUs(driver, vm, - VIR_DOMAIN_PAUSED_FROM_SNAPSHOT, - QEMU_ASYNC_JOB_START) < 0) - goto endjob; - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("guest unexpectedly quit")); - goto endjob; - } - } - - if (qemuDomainObjEnterMonitorAsync(driver, vm, - QEMU_ASYNC_JOB_START) < 0) - goto endjob; - rc = qemuMonitorLoadSnapshot(priv->mon, snap->def->name);
(Don't forget do delete the monitor code for 'loadvm' ;))
- if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto endjob; - if (rc < 0) { - /* XXX resume domain if it was running before the - * failed loadvm attempt? */ - goto endjob; - } - - virCPUDefFree(priv->origCPU); - priv->origCPU = g_steal_pointer(&origCPU); - - if (cookie && !cookie->slirpHelper) - priv->disableSlirp = true; - - if (inactiveConfig) { - virDomainObjAssignDef(vm, inactiveConfig, false, NULL); - inactiveConfig = NULL; - defined = true; - } } else { /* Transitions 2, 3 */ - load:
... here.
was_stopped = true;
Which meant that 'was_stopped' was set to true when reverting when the VM was killed, which is not happening after this patch. The commit message is claiming pure dead code removal, thus this must be fixed or justified.
+ }
- if (inactiveConfig) { - virDomainObjAssignDef(vm, inactiveConfig, false, NULL); - inactiveConfig = NULL; - defined = true; - }
The rest looks good.
On Tue, Nov 16, 2021 at 15:30:02 +0100, Peter Krempa wrote:
On Mon, Nov 15, 2021 at 17:22:46 +0100, Pavel Hrdina wrote:
Now that we always restart QEMU process the loadvm code is unused and can be dropped.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 96 +++++++++++----------------------------- 1 file changed, 26 insertions(+), 70 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 661f74146c..251a0e5cfa 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c
[...]
/* Transitions 2, 3 */ - load:
... here.
was_stopped = true;
Which meant that 'was_stopped' was set to true when reverting when the VM was killed, which is not happening after this patch.
The commit message is claiming pure dead code removal, thus this must be fixed or justified.
Looking at the next commit it should be sufficient to add another 'was_stopped = true;' into the other branch and remove them both in the next commit. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On Tue, Nov 16, 2021 at 03:30:02PM +0100, Peter Krempa wrote:
On Mon, Nov 15, 2021 at 17:22:46 +0100, Pavel Hrdina wrote:
Now that we always restart QEMU process the loadvm code is unused and can be dropped.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 96 +++++++++++----------------------------- 1 file changed, 26 insertions(+), 70 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 661f74146c..251a0e5cfa 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1982,11 +1982,6 @@ qemuSnapshotRevert(virDomainObj *vm, start_flags |= VIR_QEMU_PROCESS_START_PAUSED;
/* Transitions 2, 3, 5, 6, 8, 9 */ - /* When using the loadvm monitor command, qemu does not know - * whether to pause or run the reverted domain, and just stays - * in the same state as before the monitor command, whether - * that is paused or running. We always pause before loadvm, - * to have finer control. */ if (virDomainObjIsActive(vm)) { /* Transitions 5, 6, 8, 9 */ qemuProcessStop(driver, vm, @@ -1998,78 +1993,39 @@ qemuSnapshotRevert(virDomainObj *vm, VIR_DOMAIN_EVENT_STOPPED, detail); virObjectEventStateQueue(driver->domainEventState, event); - goto load;
So this jumped ...
- - if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { - /* Transitions 5, 6 */ - if (qemuProcessStopCPUs(driver, vm, - VIR_DOMAIN_PAUSED_FROM_SNAPSHOT, - QEMU_ASYNC_JOB_START) < 0) - goto endjob; - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("guest unexpectedly quit")); - goto endjob; - } - } - - if (qemuDomainObjEnterMonitorAsync(driver, vm, - QEMU_ASYNC_JOB_START) < 0) - goto endjob; - rc = qemuMonitorLoadSnapshot(priv->mon, snap->def->name);
(Don't forget do delete the monitor code for 'loadvm' ;))
- if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto endjob; - if (rc < 0) { - /* XXX resume domain if it was running before the - * failed loadvm attempt? */ - goto endjob; - } - - virCPUDefFree(priv->origCPU); - priv->origCPU = g_steal_pointer(&origCPU); - - if (cookie && !cookie->slirpHelper) - priv->disableSlirp = true; - - if (inactiveConfig) { - virDomainObjAssignDef(vm, inactiveConfig, false, NULL); - inactiveConfig = NULL; - defined = true; - } } else { /* Transitions 2, 3 */ - load:
... here.
was_stopped = true;
Which meant that 'was_stopped' was set to true when reverting when the VM was killed, which is not happening after this patch.
The commit message is claiming pure dead code removal, thus this must be fixed or justified.
True, originally I had the `was_stopped = true;` after the if part, not in the else part and it was correct, no idea what made me to change the code. I'll fix it. Thanks, Pavel
+ }
- if (inactiveConfig) { - virDomainObjAssignDef(vm, inactiveConfig, false, NULL); - inactiveConfig = NULL; - defined = true; - }
The rest looks good.
Now that we always restart the QEMU process events are emitted differently so we need to update the code and the comment as well. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 37 ++++++++++--------------------------- 1 file changed, 10 insertions(+), 27 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 251a0e5cfa..e08959a754 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1852,7 +1852,6 @@ qemuSnapshotRevert(virDomainObj *vm, virDomainDef *config = NULL; virDomainDef *inactiveConfig = NULL; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - bool was_stopped = false; qemuDomainSaveCookie *cookie; virCPUDef *origCPU = NULL; unsigned int start_flags = VIR_QEMU_PROCESS_START_GEN_VMID; @@ -1865,15 +1864,14 @@ qemuSnapshotRevert(virDomainObj *vm, /* We have the following transitions, which create the following events: * 1. inactive -> inactive: none * 2. inactive -> running: EVENT_STARTED - * 3. inactive -> paused: EVENT_STARTED, EVENT_PAUSED + * 3. inactive -> paused: EVENT_STARTED, EVENT_SUSPENDED * 4. running -> inactive: EVENT_STOPPED - * 5. running -> running: none - * 6. running -> paused: EVENT_PAUSED + * 5. running -> running: EVENT_STOPPED, EVENT_STARTED + * 6. running -> paused: EVENT_STOPPED, EVENT_STARTED, EVENT_SUSPENDED * 7. paused -> inactive: EVENT_STOPPED - * 8. paused -> running: EVENT_RESUMED - * 9. paused -> paused: none - * Also, several transitions occur even if we fail partway through, - * and use of FORCE can cause multiple transitions. + * 8. paused -> running: EVENT_STOPPED, EVENT_STARTED + * 9. paused -> paused: EVENT_STOPPED, EVENT_STARTED, EVENT_SUSPENDED + * Also, several transitions occur even if we fail partway through. */ if (qemuDomainHasBlockjob(vm, false)) { @@ -1993,9 +1991,6 @@ qemuSnapshotRevert(virDomainObj *vm, VIR_DOMAIN_EVENT_STOPPED, detail); virObjectEventStateQueue(driver->domainEventState, event); - } else { - /* Transitions 2, 3 */ - was_stopped = true; } if (inactiveConfig) { @@ -2034,13 +2029,10 @@ qemuSnapshotRevert(virDomainObj *vm, /* Transitions 3, 6, 9 */ virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_FROM_SNAPSHOT); - if (was_stopped) { - /* Transition 3, use event as-is and add event2 */ - detail = VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT; - event2 = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_SUSPENDED, - detail); - } /* else transition 6 and 9 use event as-is */ + detail = VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT; + event2 = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + detail); } else { /* Transitions 2, 5, 8 */ if (!virDomainObjIsActive(vm)) { @@ -2053,15 +2045,6 @@ qemuSnapshotRevert(virDomainObj *vm, QEMU_ASYNC_JOB_START); if (rc < 0) goto endjob; - virObjectUnref(event); - event = NULL; - if (was_stopped) { - /* Transition 2 */ - detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_STARTED, - detail); - } } break; -- 2.31.1
On Mon, Nov 15, 2021 at 17:22:47 +0100, Pavel Hrdina wrote:
Now that we always restart the QEMU process events are emitted differently so we need to update the code and the comment as well.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 37 ++++++++++--------------------------- 1 file changed, 10 insertions(+), 27 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 251a0e5cfa..e08959a754 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c
[...]
@@ -1865,15 +1864,14 @@ qemuSnapshotRevert(virDomainObj *vm, /* We have the following transitions, which create the following events: * 1. inactive -> inactive: none * 2. inactive -> running: EVENT_STARTED - * 3. inactive -> paused: EVENT_STARTED, EVENT_PAUSED + * 3. inactive -> paused: EVENT_STARTED, EVENT_SUSPENDED * 4. running -> inactive: EVENT_STOPPED - * 5. running -> running: none - * 6. running -> paused: EVENT_PAUSED + * 5. running -> running: EVENT_STOPPED, EVENT_STARTED + * 6. running -> paused: EVENT_STOPPED, EVENT_STARTED, EVENT_SUSPENDED * 7. paused -> inactive: EVENT_STOPPED - * 8. paused -> running: EVENT_RESUMED - * 9. paused -> paused: none - * Also, several transitions occur even if we fail partway through, - * and use of FORCE can cause multiple transitions. + * 8. paused -> running: EVENT_STOPPED, EVENT_STARTED + * 9. paused -> paused: EVENT_STOPPED, EVENT_STARTED, EVENT_SUSPENDED + * Also, several transitions occur even if we fail partway through. */
[...]
@@ -2053,15 +2045,6 @@ qemuSnapshotRevert(virDomainObj *vm, QEMU_ASYNC_JOB_START); if (rc < 0) goto endjob; - virObjectUnref(event); - event = NULL; - if (was_stopped) { - /* Transition 2 */ - detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_STARTED, - detail); - }
Okay so if I understand correctly, this was a duplicate 'STARTED' event which would overwrite the previously assigned STARTED event. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This will always happen so there is no need to error out and require usage of FORCE flag. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index e08959a754..9e975e0eaa 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1914,15 +1914,6 @@ qemuSnapshotRevert(virDomainObj *vm, } if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) { - if (virDomainObjIsActive(vm) && - !(snapdef->state == VIR_DOMAIN_SNAPSHOT_RUNNING || - snapdef->state == VIR_DOMAIN_SNAPSHOT_PAUSED) && - (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | - VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) { - virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s", - _("must respawn qemu to start inactive snapshot")); - goto endjob; - } if (vm->hasManagedSave && !(snapdef->state == VIR_DOMAIN_SNAPSHOT_RUNNING || snapdef->state == VIR_DOMAIN_SNAPSHOT_PAUSED)) { -- 2.31.1
On Mon, Nov 15, 2021 at 17:22:48 +0100, Pavel Hrdina wrote:
This will always happen so there is no need to error out and require usage of FORCE flag.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 9 --------- 1 file changed, 9 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
We should have this check even if FORCE flag is used because later we unconditionally copy the `snap->def->dom` and error out if there is no copy created. The test driver will always save the VM XML when creating new snapshot. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/test/test_driver.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ea474d55ac..2fa78a15d6 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -9043,13 +9043,14 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto cleanup; } + if (!snap->def->dom) { + virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, + _("snapshot '%s' lacks domain '%s' rollback info"), + snap->def->name, vm->def->name); + goto cleanup; + } + if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) { - if (!snap->def->dom) { - virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, - _("snapshot '%s' lacks domain '%s' rollback info"), - snap->def->name, vm->def->name); - goto cleanup; - } if (virDomainObjIsActive(vm) && !(snapdef->state == VIR_DOMAIN_SNAPSHOT_RUNNING || snapdef->state == VIR_DOMAIN_SNAPSHOT_PAUSED) && -- 2.31.1
On Mon, Nov 15, 2021 at 17:22:49 +0100, Pavel Hrdina wrote:
We should have this check even if FORCE flag is used because later we unconditionally copy the `snap->def->dom` and error out if there is no copy created. The test driver will always save the VM XML when creating new snapshot.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/test/test_driver.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reflect the same change in test driver as in QEMU driver because the compatibility check code isn't perfect. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/test/test_driver.c | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 2fa78a15d6..0da8a2ea0f 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -9077,28 +9077,13 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (virDomainObjIsActive(vm)) { /* Transitions 5, 6, 8, 9 */ - /* Check for ABI compatibility. */ - if (!virDomainDefCheckABIStability(vm->def, config, - privconn->xmlopt)) { - virErrorPtr err = virGetLastError(); - - if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) { - /* Re-spawn error using correct category. */ - if (err->code == VIR_ERR_CONFIG_UNSUPPORTED) - virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s", - err->str2); - goto cleanup; - } - - virResetError(err); - testDomainShutdownState(snapshot->domain, vm, - VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT); - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_STOPPED, - VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); - virObjectEventStateQueue(privconn->eventState, event); - goto load; - } + testDomainShutdownState(snapshot->domain, vm, + VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT); + event = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_STOPPED, + VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); + virObjectEventStateQueue(privconn->eventState, event); + goto load; if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { /* Transitions 5, 6 */ -- 2.31.1
On Mon, Nov 15, 2021 at 17:22:50 +0100, Pavel Hrdina wrote:
Reflect the same change in test driver as in QEMU driver because the compatibility check code isn't perfect.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/test/test_driver.c | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On Tue, Nov 16, 2021 at 16:15:54 +0100, Peter Krempa wrote:
On Mon, Nov 15, 2021 at 17:22:50 +0100, Pavel Hrdina wrote:
Reflect the same change in test driver as in QEMU driver because the compatibility check code isn't perfect.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/test/test_driver.c | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This breaks the 'virsh-snapshot' testsuite. (I suggest you configure your build env with -Dexpensive_tests=enabled.)
When active snapshot is reverted we stop CPUs in order to load the snapshot but we never start the CPUs again. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/test/test_driver.c | 3 +++ tests/virsh-snapshot | 14 +++++++------- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index c17ed9d2a4..985f08ea1f 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -9144,6 +9144,9 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, virObjectUnref(event); event = NULL; + virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, + VIR_DOMAIN_PAUSED_FROM_SNAPSHOT); + if (was_stopped) { /* Transition 2 */ event = virDomainEventLifecycleNewFromObj(vm, diff --git a/tests/virsh-snapshot b/tests/virsh-snapshot index 289de5b2db..4c64bb537b 100755 --- a/tests/virsh-snapshot +++ b/tests/virsh-snapshot @@ -137,25 +137,25 @@ Domain snapshot s1 deleted Name Creation Time State --------------------------------------------- s3 TIMESTAMP running - s7 TIMESTAMP paused + s7 TIMESTAMP running Name Creation Time State --------------------------------------------- s2 TIMESTAMP running - s4 TIMESTAMP paused - s5 TIMESTAMP paused - s8 TIMESTAMP paused + s4 TIMESTAMP running + s5 TIMESTAMP running + s8 TIMESTAMP running Name Creation Time State Parent ------------------------------------------------------ s3 TIMESTAMP running - s6 TIMESTAMP paused s3 - s7 TIMESTAMP paused + s6 TIMESTAMP running s3 + s7 TIMESTAMP running Name Creation Time State --------------------------------------------- s2 TIMESTAMP running - s6 TIMESTAMP paused + s6 TIMESTAMP running s2 s4 -- 2.31.1
On Tue, Nov 23, 2021 at 09:20:20 +0100, Pavel Hrdina wrote:
When active snapshot is reverted we stop CPUs in order to load the snapshot but we never start the CPUs again.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/test/test_driver.c | 3 +++ tests/virsh-snapshot | 14 +++++++------- 2 files changed, 10 insertions(+), 7 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Now that we always emulate VM process stop we can drop the unused code and simply the logic. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/test/test_driver.c | 33 ++++++++------------------------- 1 file changed, 8 insertions(+), 25 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 0da8a2ea0f..4ae68571ef 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -9083,35 +9083,18 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); virObjectEventStateQueue(privconn->eventState, event); - goto load; - - if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { - /* Transitions 5, 6 */ - was_running = true; - virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, - VIR_DOMAIN_PAUSED_FROM_SNAPSHOT); - /* Create an event now in case the restore fails, so - * that user will be alerted that they are now paused. - * If restore later succeeds, we might replace this. */ - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_SUSPENDED, - VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT); - } - virDomainObjAssignDef(vm, config, false, NULL); - } else { - /* Transitions 2, 3 */ - load: was_stopped = true; - virDomainObjAssignDef(vm, config, false, NULL); - if (testDomainStartState(privconn, vm, - VIR_DOMAIN_RUNNING_FROM_SNAPSHOT) < 0) - goto cleanup; - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_STARTED, - VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); } + virDomainObjAssignDef(vm, config, false, NULL); + if (testDomainStartState(privconn, vm, + VIR_DOMAIN_RUNNING_FROM_SNAPSHOT) < 0) + goto cleanup; + event = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_STARTED, + VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); + /* Touch up domain state. */ if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) && (snapdef->state == VIR_DOMAIN_SNAPSHOT_PAUSED || -- 2.31.1
On Mon, Nov 15, 2021 at 17:22:51 +0100, Pavel Hrdina wrote:
Now that we always emulate VM process stop we can drop the unused code and simply the logic.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/test/test_driver.c | 33 ++++++++------------------------- 1 file changed, 8 insertions(+), 25 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 0da8a2ea0f..4ae68571ef 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -9083,35 +9083,18 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); virObjectEventStateQueue(privconn->eventState, event); - goto load; - - if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { - /* Transitions 5, 6 */ - was_running = true; - virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, - VIR_DOMAIN_PAUSED_FROM_SNAPSHOT); - /* Create an event now in case the restore fails, so - * that user will be alerted that they are now paused. - * If restore later succeeds, we might replace this. */ - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_SUSPENDED, - VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT); - } - virDomainObjAssignDef(vm, config, false, NULL); - } else { - /* Transitions 2, 3 */ - load: was_stopped = true;
Same issue as with the patch for the qemu driver. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Now that we always emulate restarting the VM process events are emitted differently so we need to update the code and the comment as well. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/test/test_driver.c | 42 +++++++++--------------------------------- 1 file changed, 9 insertions(+), 33 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 4ae68571ef..333b5e8bfc 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -9014,15 +9014,14 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, /* We have the following transitions, which create the following events: * 1. inactive -> inactive: none * 2. inactive -> running: EVENT_STARTED - * 3. inactive -> paused: EVENT_STARTED, EVENT_PAUSED + * 3. inactive -> paused: EVENT_STARTED, EVENT_SUSPENDED * 4. running -> inactive: EVENT_STOPPED - * 5. running -> running: none - * 6. running -> paused: EVENT_PAUSED + * 5. running -> running: EVENT_STOPPED, EVENT_STARTED + * 6. running -> paused: EVENT_STOPPED, EVENT_STARTED, EVENT_SUSPENDED * 7. paused -> inactive: EVENT_STOPPED - * 8. paused -> running: EVENT_RESUMED - * 9. paused -> paused: none - * Also, several transitions occur even if we fail partway through, - * and use of FORCE can cause multiple transitions. + * 8. paused -> running: EVENT_STOPPED, EVENT_STARTED + * 9. paused -> paused: EVENT_STOPPED, EVENT_STARTED, EVENT_SUSPENDED + * Also, several transitions occur even if we fail partway through. */ if (!(vm = testDomObjFromSnapshot(snapshot))) @@ -9072,8 +9071,6 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (snapdef->state == VIR_DOMAIN_SNAPSHOT_RUNNING || snapdef->state == VIR_DOMAIN_SNAPSHOT_PAUSED) { /* Transitions 2, 3, 5, 6, 8, 9 */ - bool was_running = false; - bool was_stopped = false; if (virDomainObjIsActive(vm)) { /* Transitions 5, 6, 8, 9 */ @@ -9083,8 +9080,6 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); virObjectEventStateQueue(privconn->eventState, event); - } else { - was_stopped = true; } virDomainObjAssignDef(vm, config, false, NULL); @@ -9102,28 +9097,9 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, /* Transitions 3, 6, 9 */ virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_FROM_SNAPSHOT); - if (was_stopped) { - /* Transition 3, use event as-is and add event2 */ - event2 = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_SUSPENDED, - VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT); - } /* else transition 6 and 9 use event as-is */ - } else { - /* Transitions 2, 5, 8 */ - virObjectUnref(event); - event = NULL; - - if (was_stopped) { - /* Transition 2 */ - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_STARTED, - VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); - } else if (was_running) { - /* Transition 8 */ - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_RESUMED, - VIR_DOMAIN_EVENT_RESUMED); - } + event2 = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT); } } else { /* Transitions 1, 4, 7 */ -- 2.31.1
On Mon, Nov 15, 2021 at 17:22:52 +0100, Pavel Hrdina wrote:
Now that we always emulate restarting the VM process events are emitted differently so we need to update the code and the comment as well.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/test/test_driver.c | 42 +++++++++--------------------------------- 1 file changed, 9 insertions(+), 33 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This will always happen so there is no need to error out and require usage of FORCE flag. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/test/test_driver.c | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 333b5e8bfc..2871c8ebac 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -9049,18 +9049,6 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto cleanup; } - if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) { - if (virDomainObjIsActive(vm) && - !(snapdef->state == VIR_DOMAIN_SNAPSHOT_RUNNING || - snapdef->state == VIR_DOMAIN_SNAPSHOT_PAUSED) && - (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | - VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) { - virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s", - _("must respawn guest to start inactive snapshot")); - goto cleanup; - } - } - virDomainSnapshotSetCurrent(vm->snapshots, NULL); config = virDomainDefCopy(snap->def->dom, -- 2.31.1
On Mon, Nov 15, 2021 at 17:22:53 +0100, Pavel Hrdina wrote:
This will always happen so there is no need to error out and require usage of FORCE flag.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/test/test_driver.c | 12 ------------ 1 file changed, 12 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
We've changed the behavior of this API that from now on it will always restart the VM process and we are no longer able to revert to snapshots created by libvirt older then 0.9.5. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt-domain-snapshot.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c index 4a79e95704..99316460aa 100644 --- a/src/libvirt-domain-snapshot.c +++ b/src/libvirt-domain-snapshot.c @@ -960,6 +960,11 @@ virDomainSnapshotHasMetadata(virDomainSnapshotPtr snapshot, * implies the intent to roll back state, no additional confirmation is * normally required for these lossy effects. * + * Since libvirt 7.10.0 the VM process is always restarted so the following + * paragraph is no longer valid. In case that the snapshot metadata lacks + * full VM XML we will error out as it is no longer possible to revert to + * these old snapshots. + * * However, there are two particular situations where reverting will * be refused by default, and where @flags must include * VIR_DOMAIN_SNAPSHOT_REVERT_FORCE to acknowledge the risks. 1) Any -- 2.31.1
On Mon, Nov 15, 2021 at 17:22:54 +0100, Pavel Hrdina wrote:
We've changed the behavior of this API that from now on it will always restart the VM process and we are no longer able to revert to snapshots created by libvirt older then 0.9.5.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt-domain-snapshot.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c index 4a79e95704..99316460aa 100644 --- a/src/libvirt-domain-snapshot.c +++ b/src/libvirt-domain-snapshot.c @@ -960,6 +960,11 @@ virDomainSnapshotHasMetadata(virDomainSnapshotPtr snapshot, * implies the intent to roll back state, no additional confirmation is * normally required for these lossy effects. * + * Since libvirt 7.10.0 the VM process is always restarted so the following + * paragraph is no longer valid. In case that the snapshot metadata lacks + * full VM XML we will error out as it is no longer possible to revert to
"If the snapshot metadata lacks the full VM XML it's no longer possible to revert to such snapshot." Instead of the last sentence. Additionally you should add a similar notice to the virsh man page (separate commit). Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Pavel Hrdina -
Peter Krempa