10:22 a.m.
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
10:22 a.m.
New subject: [libvirt PATCH 01/11] qemu_snapshot: revert: always error out if VM XML is missing
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(a)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
8:11 a.m.
New subject: [libvirt PATCH 01/11] qemu_snapshot: revert: always error out if VM XML is missing
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(a)redhat.com>
---
src/qemu/qemu_snapshot.c | 138 +++++++++++++++++++--------------------
1 file changed, 66 insertions(+), 72 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
10:22 a.m.
New subject: [libvirt PATCH 02/11] qemu_snapshot: revert: always restart QEMU process for running VM
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(a)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
8:17 a.m.
New subject: [libvirt PATCH 02/11] qemu_snapshot: revert: always restart QEMU process for running VM
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(a)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(a)redhat.com>
5:32 a.m.
New subject: [libvirt PATCH 02/11] qemu_snapshot: revert: always restart QEMU process for running VM
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(a)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(a)redhat.com>
5:54 a.m.
New subject: [libvirt PATCH 02/11] qemu_snapshot: revert: always restart QEMU process for running VM
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(a)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.
10:22 a.m.
New subject: [libvirt PATCH 03/11] qemu_snapshot: revert: drop unused loadvm code
Now that we always restart QEMU process the loadvm code is unused and
can be dropped.
Signed-off-by: Pavel Hrdina <phrdina(a)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
8:30 a.m.
New subject: [libvirt PATCH 03/11] qemu_snapshot: revert: drop unused loadvm code
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(a)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.
8:33 a.m.
New subject: [libvirt PATCH 03/11] qemu_snapshot: revert: drop unused loadvm code
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(a)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(a)redhat.com>
6:49 a.m.
New subject: [libvirt PATCH 03/11] qemu_snapshot: revert: drop unused loadvm code
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(a)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.
10:22 a.m.
New subject: [libvirt PATCH 04/11] qemu_snapshot: revert: fix emitting events
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(a)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
9:04 a.m.
New subject: [libvirt PATCH 04/11] qemu_snapshot: revert: fix emitting events
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(a)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(a)redhat.com>
10:22 a.m.
New subject: [libvirt PATCH 05/11] qemu_snapshot: revert: drop error that QEMU process must be restarted
This will always happen so there is no need to error out and require
usage of FORCE flag.
Signed-off-by: Pavel Hrdina <phrdina(a)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
9:05 a.m.
New subject: [libvirt PATCH 05/11] qemu_snapshot: revert: drop error that QEMU process must be restarted
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(a)redhat.com>
---
src/qemu/qemu_snapshot.c | 9 ---------
1 file changed, 9 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
10:22 a.m.
New subject: [libvirt PATCH 06/11] test: snapshot revert: always error out if VM XML is missing
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(a)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
9:14 a.m.
New subject: [libvirt PATCH 06/11] test: snapshot revert: always error out if VM XML is missing
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(a)redhat.com>
---
src/test/test_driver.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
10:22 a.m.
New subject: [libvirt PATCH 07/11] test: snapshot revert: always emulate VM process stop
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(a)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
9:15 a.m.
New subject: [libvirt PATCH 07/11] test: snapshot revert: always emulate VM process stop
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(a)redhat.com>
---
src/test/test_driver.c | 29 +++++++----------------------
1 file changed, 7 insertions(+), 22 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
9:23 a.m.
New subject: [libvirt PATCH 07/11] test: snapshot revert: always emulate VM process stop
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(a)redhat.com>
> ---
> src/test/test_driver.c | 29 +++++++----------------------
> 1 file changed, 7 insertions(+), 22 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
This breaks the 'virsh-snapshot' testsuite. (I suggest you configure
your build env with -Dexpensive_tests=enabled.)
2:20 a.m.
New subject: [libvirt PATCH 06.5/12] test: snapshot revert: properly emulate starting CPUs
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(a)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
3:24 a.m.
New subject: [libvirt PATCH 06.5/12] test: snapshot revert: properly emulate starting CPUs
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(a)redhat.com>
---
src/test/test_driver.c | 3 +++
tests/virsh-snapshot | 14 +++++++-------
2 files changed, 10 insertions(+), 7 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
10:22 a.m.
New subject: [libvirt PATCH 08/11] test: snapshot revert: drop unused code
Now that we always emulate VM process stop we can drop the unused code
and simply the logic.
Signed-off-by: Pavel Hrdina <phrdina(a)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
9:16 a.m.
New subject: [libvirt PATCH 08/11] test: snapshot revert: drop unused code
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(a)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(a)redhat.com>
10:22 a.m.
New subject: [libvirt PATCH 09/11] test: snapshot revert: fix emitting events
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(a)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
9:26 a.m.
New subject: [libvirt PATCH 09/11] test: snapshot revert: fix emitting events
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(a)redhat.com>
---
src/test/test_driver.c | 42 +++++++++---------------------------------
1 file changed, 9 insertions(+), 33 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
10:22 a.m.
New subject: [libvirt PATCH 10/11] test: snapshot revert: drop error the VM must be restarted
This will always happen so there is no need to error out and require
usage of FORCE flag.
Signed-off-by: Pavel Hrdina <phrdina(a)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
9:17 a.m.
New subject: [libvirt PATCH 10/11] test: snapshot revert: drop error the VM must be restarted
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(a)redhat.com>
---
src/test/test_driver.c | 12 ------------
1 file changed, 12 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
10:22 a.m.
New subject: [libvirt PATCH 11/11] domain_snapshot: update virDomainRevertToSnapshot description
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(a)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
9:14 a.m.
New subject: [libvirt PATCH 11/11] domain_snapshot: update virDomainRevertToSnapshot description
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(a)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(a)redhat.com>
1095
days inactive
1103
days old
29 comments
2 participants
participants (2)
-
Pavel Hrdina -
Peter Krempa