5:54 p.m.
Well, at least I hope so. There's a couple of related bz's in
the last two patches with gobs more details, but the long and
short of it is that when using the force flag, the domain is
Stop'd and re-Start'd without applying any snapshot. For both
issues I've seen, the "error: internal error: unexpected async
job 6" occurs on the Start and thus the CPU's aren't started.
For one bz, when running managedsave the call never returns.
For the other bz, one cannot restart the paused domain.
If there's a better way to do this - I'm sure someone will
let me know.
John Ferlan (5):
qemu: Adjust async job failure message
qemu: Promote start_flags in qemuDomainRevertToSnapshot
qemu: Use start_flags for RUNNING and PAUSED transitions
qemu: Unset the genid start change flag for revert force
qemu: Don't use asyncJob after stop during snapshot revert
src/qemu/qemu_domain.c | 3 ++-
src/qemu/qemu_driver.c | 22 +++++++++++++++-------
2 files changed, 17 insertions(+), 8 deletions(-)
--
2.14.4
5:54 p.m.
New subject: [libvirt] [PATCH 1/5] qemu: Adjust async job failure message
Make it clearer what asyncJob type was passed and what was expected.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_domain.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 8a2d4750a5..939d64542c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6581,7 +6581,8 @@ qemuDomainObjBeginNestedJob(virQEMUDriverPtr driver,
if (asyncJob != priv->job.asyncJob) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected async job %d"), asyncJob);
+ _("unexpected async job %d type expected %d"),
+ asyncJob, priv->job.asyncJob);
return -1;
}
--
2.14.4
5:54 p.m.
New subject: [libvirt] [PATCH 2/5] qemu: Promote start_flags in qemuDomainRevertToSnapshot
Promote the @start_flags to the top of the function, a
subsequent patch needs to use it.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3abbe41895..971f485226 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15990,6 +15990,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
bool was_stopped = false;
qemuDomainSaveCookiePtr cookie;
virCPUDefPtr origCPU = NULL;
+ unsigned int start_flags = VIR_QEMU_PROCESS_START_GEN_VMID;
virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED |
@@ -16307,7 +16308,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) {
/* Flush first event, now do transition 2 or 3 */
bool paused = (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED) != 0;
- unsigned int start_flags = VIR_QEMU_PROCESS_START_GEN_VMID;
start_flags |= paused ? VIR_QEMU_PROCESS_START_PAUSED : 0;
--
2.14.4
5:54 p.m.
New subject: [libvirt] [PATCH 3/5] qemu: Use start_flags for RUNNING and PAUSED transitions
Use and set the @start_flags at the top of the RUNNING and PAUSED
transitions to GEN_VMID | PAUSED.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_driver.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 971f485226..01011906d1 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16102,6 +16102,9 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
switch ((virDomainState) snap->def->state) {
case VIR_DOMAIN_RUNNING:
case VIR_DOMAIN_PAUSED:
+
+ 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
@@ -16221,8 +16224,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
cookie ? cookie->cpu : NULL,
QEMU_ASYNC_JOB_START, NULL, -1, NULL, snap,
VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
- VIR_QEMU_PROCESS_START_PAUSED |
- VIR_QEMU_PROCESS_START_GEN_VMID);
+ start_flags);
virDomainAuditStart(vm, "from-snapshot", rc >= 0);
detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT;
event = virDomainEventLifecycleNewFromObj(vm,
--
2.14.4
5:54 p.m.
New subject: [libvirt] [PATCH 4/5] qemu: Unset the genid start change flag for revert force
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.
https://bugzilla.redhat.com/show_bug.cgi?id=1149445
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_driver.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 01011906d1..f737f4d350 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16136,12 +16136,14 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
}
/* If using VM GenID, there is no way currently to change
- * the genid for the running guest, so set an error and
- * mark as incompatible. */
+ * 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) {
--
2.14.4
8:20 a.m.
New subject: [libvirt] [PATCH 4/5] qemu: Unset the genid start change flag for revert force
On 06/20/2018 12:54 AM, John Ferlan wrote:
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.
https://bugzilla.redhat.com/show_bug.cgi?id=1149445
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_driver.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 01011906d1..f737f4d350 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16136,12 +16136,14 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
}
/* If using VM GenID, there is no way currently to change
- * the genid for the running guest, so set an error and
- * mark as incompatible. */
+ * 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;
Pre-existing, but what is the point of this virReportError() if it does
not cause domain starting?
Michal
9:19 a.m.
New subject: [libvirt] [PATCH 4/5] qemu: Unset the genid start change flag for revert force
On 06/20/2018 09:20 AM, Michal Privoznik wrote:
On 06/20/2018 12:54 AM, John Ferlan wrote:
> 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.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1149445
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/qemu/qemu_driver.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 01011906d1..f737f4d350 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -16136,12 +16136,14 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> }
>
> /* If using VM GenID, there is no way currently to change
> - * the genid for the running guest, so set an error and
> - * mark as incompatible. */
> + * 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;
Pre-existing, but what is the point of this virReportError() if it does
not cause domain starting?
Michal
See a few lines lower:
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);
It's thus "similar to" what virDomainDefCheckABIStabilityFlags would do
(see a few lines above) when generating an error such as "Target domain
disk count 1 does not match source 2" which is seen in bz1591628.
A virsh failure would see :
error: revert requires force: "%s" (message)
which immediately for qe is followed by --force ;-)
Tks for the review...
John
5:54 p.m.
New subject: [libvirt] [PATCH 5/5] qemu: Don't use asyncJob after stop during snapshot revert
Attempting to use the FORCE flag for snapshot-revert was resulting
in failures because qemuProcessStart and qemuProcessStartCPUs were
using QEMU_ASYNC_JOB_START after a qemuProcessStop resulting in an
error when entering the monitor:
error: internal error: unexpected async job 6 type expected 0
So create a local @jobType, initialize to QEMU_ASYNC_JOB_START, and
change to QEMU_ASYNC_JOB_NONE if we end up in the --force path
where the qemuProcessStop is run before a Start and StartCPUs.
https://bugzilla.redhat.com/show_bug.cgi?id=1591628
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_driver.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f737f4d350..71d2bb357c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15991,6 +15991,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
qemuDomainSaveCookiePtr cookie;
virCPUDefPtr origCPU = NULL;
unsigned int start_flags = VIR_QEMU_PROCESS_START_GEN_VMID;
+ qemuDomainAsyncJob jobType = QEMU_ASYNC_JOB_START;
virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED |
@@ -16166,6 +16167,9 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
VIR_DOMAIN_EVENT_STOPPED,
detail);
virObjectEventStateQueue(driver->domainEventState, event);
+ /* Start after stop won't be an async start job, so
+ * reset to none */
+ jobType = QEMU_ASYNC_JOB_NONE;
goto load;
}
}
@@ -16224,7 +16228,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
rc = qemuProcessStart(snapshot->domain->conn, driver, vm,
cookie ? cookie->cpu : NULL,
- QEMU_ASYNC_JOB_START, NULL, -1, NULL, snap,
+ jobType, NULL, -1, NULL, snap,
VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
start_flags);
virDomainAuditStart(vm, "from-snapshot", rc >= 0);
@@ -16259,7 +16263,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
}
rc = qemuProcessStartCPUs(driver, vm,
VIR_DOMAIN_RUNNING_FROM_SNAPSHOT,
- QEMU_ASYNC_JOB_START);
+ jobType);
if (rc < 0)
goto endjob;
virObjectUnref(event);
--
2.14.4
8:20 a.m.
On 06/20/2018 12:54 AM, John Ferlan wrote:
Well, at least I hope so. There's a couple of related bz's
in
the last two patches with gobs more details, but the long and
short of it is that when using the force flag, the domain is
Stop'd and re-Start'd without applying any snapshot. For both
issues I've seen, the "error: internal error: unexpected async
job 6" occurs on the Start and thus the CPU's aren't started.
For one bz, when running managedsave the call never returns.
For the other bz, one cannot restart the paused domain.
If there's a better way to do this - I'm sure someone will
let me know.
John Ferlan (5):
qemu: Adjust async job failure message
qemu: Promote start_flags in qemuDomainRevertToSnapshot
qemu: Use start_flags for RUNNING and PAUSED transitions
qemu: Unset the genid start change flag for revert force
qemu: Don't use asyncJob after stop during snapshot revert
src/qemu/qemu_domain.c | 3 ++-
src/qemu/qemu_driver.c | 22 +++++++++++++++-------
2 files changed, 17 insertions(+), 8 deletions(-)
ACK series.
Michal
2422
days inactive
2423
days old
8 comments
2 participants
participants (2)
-
John Ferlan -
Michal Privoznik