[libvirt] [PATCH 0/3] qemu: don't duplicate suspended events and state changes
This patch series is similar in implementation to [1] but the motivation is simply to avoid suspended event duplication. [1] https://www.redhat.com/archives/libvir-list/2018-September/msg01304.html Nikolay Shirokovskiy (3): qemu: pass stop reason from qemuProcessStopCPUs to stop handler qemu: map suspended state reason to suspended event detail qemu: don't duplicate suspended events and state changes src/qemu/qemu_domain.c | 34 ++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 7 +++++++ src/qemu/qemu_driver.c | 26 +++----------------------- src/qemu/qemu_migration.c | 42 ++++++------------------------------------ src/qemu/qemu_migration.h | 4 ---- src/qemu/qemu_process.c | 37 +++++++++++++++++++++++++------------ 6 files changed, 75 insertions(+), 75 deletions(-) -- 1.8.3.1
We send duplicate suspended lifecycle events on qemu process stop in several places. The reason is stop event handler always send suspended event and we addidionally send same event but with more presise reason after call to qemuProcessStopCPUs. Domain state change is also duplicated. Let's change domain state and send event only in handler. For this purpuse first let's pass state change reason to event handler (event reason is deducible from it). Inspired by similar patch for resume: 5dab984ed "qemu: Pass running reason to RESUME event handler". Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_domain.h | 4 ++++ src/qemu/qemu_process.c | 15 ++++++++++++--- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 80bd4bd..380ea14 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -370,6 +370,10 @@ struct _qemuDomainObjPrivate { /* qemuProcessStartCPUs stores the reason for starting vCPUs here for the * RESUME event handler to use it */ virDomainRunningReason runningReason; + + /* qemuProcessStopCPUs stores the reason for starting vCPUs here for the + * STOP event handler to use it */ + virDomainPausedReason pausedReason; }; # define QEMU_DOMAIN_PRIVATE(vm) \ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e9c7618..27021b9 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -641,14 +641,17 @@ qemuProcessHandleStop(qemuMonitorPtr mon ATTRIBUTE_UNUSED, { virQEMUDriverPtr driver = opaque; virObjectEventPtr event = NULL; - virDomainPausedReason reason = VIR_DOMAIN_PAUSED_UNKNOWN; + virDomainPausedReason reason; virDomainEventSuspendedDetailType detail = VIR_DOMAIN_EVENT_SUSPENDED_PAUSED; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + qemuDomainObjPrivatePtr priv = vm->privateData; virObjectLock(vm); + + reason = priv->pausedReason; + priv->pausedReason = VIR_DOMAIN_PAUSED_UNKNOWN; + if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { - qemuDomainObjPrivatePtr priv = vm->privateData; - if (priv->gotShutdown) { VIR_DEBUG("Ignoring STOP event after SHUTDOWN"); goto unlock; @@ -3132,6 +3135,8 @@ int qemuProcessStopCPUs(virQEMUDriverPtr driver, VIR_FREE(priv->lockState); + priv->pausedReason = reason; + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) goto cleanup; @@ -3154,6 +3159,9 @@ int qemuProcessStopCPUs(virQEMUDriverPtr driver, VIR_DEBUG("Preserving lock state '%s'", NULLSTR(priv->lockState)); cleanup: + if (ret < 0) + priv->pausedReason = VIR_DOMAIN_PAUSED_UNKNOWN; + return ret; } @@ -5996,6 +6004,7 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver, priv->monStart = 0; priv->gotShutdown = false; priv->runningReason = VIR_DOMAIN_RUNNING_UNKNOWN; + priv->pausedReason = VIR_DOMAIN_PAUSED_UNKNOWN; VIR_DEBUG("Updating guest CPU definition"); if (qemuProcessUpdateGuestCPU(vm->def, priv->qemuCaps, caps, flags) < 0) -- 1.8.3.1
$SUBJ: s/pass/Pass On 10/10/18 4:04 AM, Nikolay Shirokovskiy wrote:
We send duplicate suspended lifecycle events on qemu process stop in several places. The reason is stop event handler always send suspended event and we addidionally send same event but with more presise reason after call
*additionally *precise
to qemuProcessStopCPUs. Domain state change is also duplicated.
Let's change domain state and send event only in handler. For this purpuse first let's pass state change reason to event handler (event
*purpose
reason is deducible from it).
Inspired by similar patch for resume: 5dab984ed "qemu: Pass running reason to RESUME event handler".
In any case, I think the above was a bit more appropriate for the cover since it details a few things being altered in the 3rd patch of the series. So, maybe this could change to: Similar to commit 5dab984ed which saves and passes the running reason to the RESUME event handler, during qemuProcessStopCPUs let's save and pass the pause reason in the domain private data so that the STOP event handler can use it.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_domain.h | 4 ++++ src/qemu/qemu_process.c | 15 ++++++++++++--- 2 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 80bd4bd..380ea14 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -370,6 +370,10 @@ struct _qemuDomainObjPrivate { /* qemuProcessStartCPUs stores the reason for starting vCPUs here for the * RESUME event handler to use it */ virDomainRunningReason runningReason; + + /* qemuProcessStopCPUs stores the reason for starting vCPUs here for the
s/starting/pausing/ too much copy-pasta FWIW: Similar to the comment I made to Jirka for his series, I assume this STOP processing would have the similar issue with the event going to the old libvirtd and thus not needing to save/restore via XML state processing. For context see: https://www.redhat.com/archives/libvir-list/2018-September/msg01145.html
+ * STOP event handler to use it */ + virDomainPausedReason pausedReason; };
With a couple of adjustments, Reviewed-by: John Ferlan <jferlan@redhat.com> John I can make the adjustments so that you don't need to send another series - just need your acknowledgment for that.
On 16.10.2018 21:40, John Ferlan wrote:
$SUBJ:
s/pass/Pass
On 10/10/18 4:04 AM, Nikolay Shirokovskiy wrote:
We send duplicate suspended lifecycle events on qemu process stop in several places. The reason is stop event handler always send suspended event and we addidionally send same event but with more presise reason after call
*additionally *precise
to qemuProcessStopCPUs. Domain state change is also duplicated.
Let's change domain state and send event only in handler. For this purpuse first let's pass state change reason to event handler (event
*purpose
reason is deducible from it).
Inspired by similar patch for resume: 5dab984ed "qemu: Pass running reason to RESUME event handler".
In any case, I think the above was a bit more appropriate for the cover since it details a few things being altered in the 3rd patch of the series. So, maybe this could change to:
Similar to commit 5dab984ed which saves and passes the running reason to the RESUME event handler, during qemuProcessStopCPUs let's save and pass the pause reason in the domain private data so that the STOP event handler can use it.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_domain.h | 4 ++++ src/qemu/qemu_process.c | 15 ++++++++++++--- 2 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 80bd4bd..380ea14 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -370,6 +370,10 @@ struct _qemuDomainObjPrivate { /* qemuProcessStartCPUs stores the reason for starting vCPUs here for the * RESUME event handler to use it */ virDomainRunningReason runningReason; + + /* qemuProcessStopCPUs stores the reason for starting vCPUs here for the
s/starting/pausing/
too much copy-pasta
FWIW: Similar to the comment I made to Jirka for his series, I assume this STOP processing would have the similar issue with the event going to the old libvirtd and thus not needing to save/restore via XML state processing. For context see:
https://www.redhat.com/archives/libvir-list/2018-September/msg01145.html
+ * STOP event handler to use it */ + virDomainPausedReason pausedReason; };
With a couple of adjustments,
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
I can make the adjustments so that you don't need to send another series - just need your acknowledgment for that.
I'm ok with you changes Nikolay
Hi, John! Looks like this series is stucked somehow even though there is almost 100% agreement. On 17.10.2018 11:41, Nikolay Shirokovskiy wrote:
On 16.10.2018 21:40, John Ferlan wrote:
$SUBJ:
s/pass/Pass
On 10/10/18 4:04 AM, Nikolay Shirokovskiy wrote:
We send duplicate suspended lifecycle events on qemu process stop in several places. The reason is stop event handler always send suspended event and we addidionally send same event but with more presise reason after call
*additionally *precise
to qemuProcessStopCPUs. Domain state change is also duplicated.
Let's change domain state and send event only in handler. For this purpuse first let's pass state change reason to event handler (event
*purpose
reason is deducible from it).
Inspired by similar patch for resume: 5dab984ed "qemu: Pass running reason to RESUME event handler".
In any case, I think the above was a bit more appropriate for the cover since it details a few things being altered in the 3rd patch of the series. So, maybe this could change to:
Similar to commit 5dab984ed which saves and passes the running reason to the RESUME event handler, during qemuProcessStopCPUs let's save and pass the pause reason in the domain private data so that the STOP event handler can use it.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_domain.h | 4 ++++ src/qemu/qemu_process.c | 15 ++++++++++++--- 2 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 80bd4bd..380ea14 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -370,6 +370,10 @@ struct _qemuDomainObjPrivate { /* qemuProcessStartCPUs stores the reason for starting vCPUs here for the * RESUME event handler to use it */ virDomainRunningReason runningReason; + + /* qemuProcessStopCPUs stores the reason for starting vCPUs here for the
s/starting/pausing/
too much copy-pasta
FWIW: Similar to the comment I made to Jirka for his series, I assume this STOP processing would have the similar issue with the event going to the old libvirtd and thus not needing to save/restore via XML state processing. For context see:
https://www.redhat.com/archives/libvir-list/2018-September/msg01145.html
+ * STOP event handler to use it */ + virDomainPausedReason pausedReason; };
With a couple of adjustments,
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
I can make the adjustments so that you don't need to send another series - just need your acknowledgment for that.
I'm ok with you changes
Nikolay
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 11/2/18 4:21 AM, Nikolay Shirokovskiy wrote:
Hi, John!
Looks like this series is stucked somehow even though there is almost 100% agreement.
Right, but there's been a few events in between that pushed this down the stack of things I was involved with (beyond my own work) - KVM Forum, an acquisition, code freeze, ... Prior to KVM Forum I asked Peter Krempa (via internal IRC) to look at patch 3, but he may have forgotten or pushed it down his todo list. Of course he took an extra week after KVM Forum for holiday - so Thanks for the nudge, it's on my shorter list of things to do! I'm 50/50 on the patch 3 event change. I would like to understand why it was ignored previously. Sometimes there's some very strange and hidden reason which becomes painfully obvious at some point in the future. Then it's a search to figure out why the event just popped up, followed by time spent wondering why we're not filtering the event any more, and why we filtered it in the first place. John
On 17.10.2018 11:41, Nikolay Shirokovskiy wrote:
On 16.10.2018 21:40, John Ferlan wrote:
$SUBJ:
s/pass/Pass
On 10/10/18 4:04 AM, Nikolay Shirokovskiy wrote:
We send duplicate suspended lifecycle events on qemu process stop in several places. The reason is stop event handler always send suspended event and we addidionally send same event but with more presise reason after call
*additionally *precise
to qemuProcessStopCPUs. Domain state change is also duplicated.
Let's change domain state and send event only in handler. For this purpuse first let's pass state change reason to event handler (event
*purpose
reason is deducible from it).
Inspired by similar patch for resume: 5dab984ed "qemu: Pass running reason to RESUME event handler".
In any case, I think the above was a bit more appropriate for the cover since it details a few things being altered in the 3rd patch of the series. So, maybe this could change to:
Similar to commit 5dab984ed which saves and passes the running reason to the RESUME event handler, during qemuProcessStopCPUs let's save and pass the pause reason in the domain private data so that the STOP event handler can use it.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_domain.h | 4 ++++ src/qemu/qemu_process.c | 15 ++++++++++++--- 2 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 80bd4bd..380ea14 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -370,6 +370,10 @@ struct _qemuDomainObjPrivate { /* qemuProcessStartCPUs stores the reason for starting vCPUs here for the * RESUME event handler to use it */ virDomainRunningReason runningReason; + + /* qemuProcessStopCPUs stores the reason for starting vCPUs here for the
s/starting/pausing/
too much copy-pasta
FWIW: Similar to the comment I made to Jirka for his series, I assume this STOP processing would have the similar issue with the event going to the old libvirtd and thus not needing to save/restore via XML state processing. For context see:
https://www.redhat.com/archives/libvir-list/2018-September/msg01145.html
+ * STOP event handler to use it */ + virDomainPausedReason pausedReason; };
With a couple of adjustments,
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
I can make the adjustments so that you don't need to send another series - just need your acknowledgment for that.
I'm ok with you changes
Nikolay
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Map is based on existing cases in code where we send suspended event after changing domain state to paused. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_domain.c | 34 ++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_process.c | 9 ++++++--- 3 files changed, 43 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f00f1b3..557743b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -13558,3 +13558,37 @@ qemuDomainRunningReasonToResumeEvent(virDomainRunningReason reason) return VIR_DOMAIN_EVENT_RESUMED_UNPAUSED; } + + +virDomainEventSuspendedDetailType +qemuDomainPausedReasonToSuspendedEvent(virDomainPausedReason reason) +{ + switch (reason) { + case VIR_DOMAIN_PAUSED_MIGRATION: + return VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED; + + case VIR_DOMAIN_PAUSED_FROM_SNAPSHOT: + return VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT; + + case VIR_DOMAIN_PAUSED_POSTCOPY_FAILED: + return VIR_DOMAIN_EVENT_SUSPENDED_POSTCOPY_FAILED; + + case VIR_DOMAIN_PAUSED_POSTCOPY: + return VIR_DOMAIN_EVENT_SUSPENDED_POSTCOPY; + + case VIR_DOMAIN_PAUSED_UNKNOWN: + case VIR_DOMAIN_PAUSED_USER: + case VIR_DOMAIN_PAUSED_SAVE: + case VIR_DOMAIN_PAUSED_DUMP: + case VIR_DOMAIN_PAUSED_IOERROR: + case VIR_DOMAIN_PAUSED_WATCHDOG: + case VIR_DOMAIN_PAUSED_SHUTTING_DOWN: + case VIR_DOMAIN_PAUSED_SNAPSHOT: + case VIR_DOMAIN_PAUSED_CRASHED: + case VIR_DOMAIN_PAUSED_STARTING_UP: + case VIR_DOMAIN_PAUSED_LAST: + break; + } + + return VIR_DOMAIN_EVENT_SUSPENDED_PAUSED; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 380ea14..ee88266 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1093,4 +1093,7 @@ void qemuDomainStorageIdReset(qemuDomainObjPrivatePtr priv); virDomainEventResumedDetailType qemuDomainRunningReasonToResumeEvent(virDomainRunningReason reason); +virDomainEventSuspendedDetailType +qemuDomainPausedReasonToSuspendedEvent(virDomainPausedReason reason); + #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 27021b9..6858377 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -642,7 +642,7 @@ qemuProcessHandleStop(qemuMonitorPtr mon ATTRIBUTE_UNUSED, virQEMUDriverPtr driver = opaque; virObjectEventPtr event = NULL; virDomainPausedReason reason; - virDomainEventSuspendedDetailType detail = VIR_DOMAIN_EVENT_SUSPENDED_PAUSED; + virDomainEventSuspendedDetailType detail; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData; @@ -668,8 +668,11 @@ qemuProcessHandleStop(qemuMonitorPtr mon ATTRIBUTE_UNUSED, } } - VIR_DEBUG("Transitioned guest %s to paused state, reason %s", - vm->def->name, virDomainPausedReasonTypeToString(reason)); + detail = qemuDomainPausedReasonToSuspendedEvent(reason); + VIR_DEBUG("Transitioned guest %s to paused state, " + "reason %s, event detail %d", + vm->def->name, virDomainPausedReasonTypeToString(reason), + detail); if (priv->job.current) ignore_value(virTimeMillisNow(&priv->job.current->stopped)); -- 1.8.3.1
$SUBJ: s/map/Map On 10/10/18 4:04 AM, Nikolay Shirokovskiy wrote:
Map is based on existing cases in code where we send suspended event after changing domain state to paused.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_domain.c | 34 ++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_process.c | 9 ++++++--- 3 files changed, 43 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f00f1b3..557743b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -13558,3 +13558,37 @@ qemuDomainRunningReasonToResumeEvent(virDomainRunningReason reason)
return VIR_DOMAIN_EVENT_RESUMED_UNPAUSED; } + + +virDomainEventSuspendedDetailType +qemuDomainPausedReasonToSuspendedEvent(virDomainPausedReason reason) +{ + switch (reason) { + case VIR_DOMAIN_PAUSED_MIGRATION: + return VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED; + + case VIR_DOMAIN_PAUSED_FROM_SNAPSHOT: + return VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT; + + case VIR_DOMAIN_PAUSED_POSTCOPY_FAILED: + return VIR_DOMAIN_EVENT_SUSPENDED_POSTCOPY_FAILED; + + case VIR_DOMAIN_PAUSED_POSTCOPY: + return VIR_DOMAIN_EVENT_SUSPENDED_POSTCOPY; + + case VIR_DOMAIN_PAUSED_UNKNOWN: + case VIR_DOMAIN_PAUSED_USER: + case VIR_DOMAIN_PAUSED_SAVE: + case VIR_DOMAIN_PAUSED_DUMP: + case VIR_DOMAIN_PAUSED_IOERROR: + case VIR_DOMAIN_PAUSED_WATCHDOG: + case VIR_DOMAIN_PAUSED_SHUTTING_DOWN: + case VIR_DOMAIN_PAUSED_SNAPSHOT:
This one may cause issues in the next patch...
+ case VIR_DOMAIN_PAUSED_CRASHED: + case VIR_DOMAIN_PAUSED_STARTING_UP: + case VIR_DOMAIN_PAUSED_LAST: + break; + } + + return VIR_DOMAIN_EVENT_SUSPENDED_PAUSED; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 380ea14..ee88266 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1093,4 +1093,7 @@ void qemuDomainStorageIdReset(qemuDomainObjPrivatePtr priv); virDomainEventResumedDetailType qemuDomainRunningReasonToResumeEvent(virDomainRunningReason reason);
+virDomainEventSuspendedDetailType +qemuDomainPausedReasonToSuspendedEvent(virDomainPausedReason reason); + #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 27021b9..6858377 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -642,7 +642,7 @@ qemuProcessHandleStop(qemuMonitorPtr mon ATTRIBUTE_UNUSED, virQEMUDriverPtr driver = opaque; virObjectEventPtr event = NULL; virDomainPausedReason reason; - virDomainEventSuspendedDetailType detail = VIR_DOMAIN_EVENT_SUSPENDED_PAUSED; + virDomainEventSuspendedDetailType detail; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -668,8 +668,11 @@ qemuProcessHandleStop(qemuMonitorPtr mon ATTRIBUTE_UNUSED, } }
- VIR_DEBUG("Transitioned guest %s to paused state, reason %s", - vm->def->name, virDomainPausedReasonTypeToString(reason)); + detail = qemuDomainPausedReasonToSuspendedEvent(reason);
This setting of @detail overrides the @detail set just above... It seems we can remove the @detail setting above in this patch as well as the need for the { } and { } around the setting of @reason in this patch instead of the subsequent one. Also, may as well place the check on the same line since it fits, e.g.: if (priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) reason = VIR_DOMAIN_PAUSED_POSTCOPY; else reason = VIR_DOMAIN_PAUSED_MIGRATION; With that adjustment, Reviewed-by: John Ferlan <jferlan@redhat.com> John Similar to patch 1, I can make the changes for you...
+ VIR_DEBUG("Transitioned guest %s to paused state, " + "reason %s, event detail %d", + vm->def->name, virDomainPausedReasonTypeToString(reason), + detail);
if (priv->job.current) ignore_value(virTimeMillisNow(&priv->job.current->stopped));
On 16.10.2018 21:42, John Ferlan wrote:
$SUBJ:
s/map/Map
On 10/10/18 4:04 AM, Nikolay Shirokovskiy wrote:
Map is based on existing cases in code where we send suspended event after changing domain state to paused.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_domain.c | 34 ++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_process.c | 9 ++++++--- 3 files changed, 43 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f00f1b3..557743b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -13558,3 +13558,37 @@ qemuDomainRunningReasonToResumeEvent(virDomainRunningReason reason)
return VIR_DOMAIN_EVENT_RESUMED_UNPAUSED; } + + +virDomainEventSuspendedDetailType +qemuDomainPausedReasonToSuspendedEvent(virDomainPausedReason reason) +{ + switch (reason) { + case VIR_DOMAIN_PAUSED_MIGRATION: + return VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED; + + case VIR_DOMAIN_PAUSED_FROM_SNAPSHOT: + return VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT; + + case VIR_DOMAIN_PAUSED_POSTCOPY_FAILED: + return VIR_DOMAIN_EVENT_SUSPENDED_POSTCOPY_FAILED; + + case VIR_DOMAIN_PAUSED_POSTCOPY: + return VIR_DOMAIN_EVENT_SUSPENDED_POSTCOPY; + + case VIR_DOMAIN_PAUSED_UNKNOWN: + case VIR_DOMAIN_PAUSED_USER: + case VIR_DOMAIN_PAUSED_SAVE: + case VIR_DOMAIN_PAUSED_DUMP: + case VIR_DOMAIN_PAUSED_IOERROR: + case VIR_DOMAIN_PAUSED_WATCHDOG: + case VIR_DOMAIN_PAUSED_SHUTTING_DOWN: + case VIR_DOMAIN_PAUSED_SNAPSHOT:
This one may cause issues in the next patch...
+ case VIR_DOMAIN_PAUSED_CRASHED: + case VIR_DOMAIN_PAUSED_STARTING_UP: + case VIR_DOMAIN_PAUSED_LAST: + break; + } + + return VIR_DOMAIN_EVENT_SUSPENDED_PAUSED; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 380ea14..ee88266 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1093,4 +1093,7 @@ void qemuDomainStorageIdReset(qemuDomainObjPrivatePtr priv); virDomainEventResumedDetailType qemuDomainRunningReasonToResumeEvent(virDomainRunningReason reason);
+virDomainEventSuspendedDetailType +qemuDomainPausedReasonToSuspendedEvent(virDomainPausedReason reason); + #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 27021b9..6858377 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -642,7 +642,7 @@ qemuProcessHandleStop(qemuMonitorPtr mon ATTRIBUTE_UNUSED, virQEMUDriverPtr driver = opaque; virObjectEventPtr event = NULL; virDomainPausedReason reason; - virDomainEventSuspendedDetailType detail = VIR_DOMAIN_EVENT_SUSPENDED_PAUSED; + virDomainEventSuspendedDetailType detail; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -668,8 +668,11 @@ qemuProcessHandleStop(qemuMonitorPtr mon ATTRIBUTE_UNUSED, } }
- VIR_DEBUG("Transitioned guest %s to paused state, reason %s", - vm->def->name, virDomainPausedReasonTypeToString(reason)); + detail = qemuDomainPausedReasonToSuspendedEvent(reason);
This setting of @detail overrides the @detail set just above...
But overrides to the same value.
It seems we can remove the @detail setting above in this patch as well as the need for the { } and { } around the setting of @reason in this patch instead of the subsequent one. Also, may as well place the check on the same line since it fits, e.g.:
if (priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) reason = VIR_DOMAIN_PAUSED_POSTCOPY; else reason = VIR_DOMAIN_PAUSED_MIGRATION;
With that adjustment,
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
Similar to patch 1, I can make the changes for you...
I'm ok here too, but we can left removing setting details for QEMU_DOMAIN_JOB_STATUS_POSTCOPY as well as this patch correct in this aspect. Nikolay
+ VIR_DEBUG("Transitioned guest %s to paused state, " + "reason %s, event detail %d", + vm->def->name, virDomainPausedReasonTypeToString(reason), + detail);
if (priv->job.current) ignore_value(virTimeMillisNow(&priv->job.current->stopped));
Now when STOP event handler has correct both suspended event reason and paused state reason let's wipe out duplicated event sending and state changed in/after qemuProcessStopCPUs. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 26 +++----------------------- src/qemu/qemu_migration.c | 42 ++++++------------------------------------ src/qemu/qemu_migration.h | 4 ---- src/qemu/qemu_process.c | 13 +++++++------ 4 files changed, 16 insertions(+), 69 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a52e249..7e08768 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1792,10 +1792,8 @@ static int qemuDomainSuspend(virDomainPtr dom) virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; - virObjectEventPtr event = NULL; qemuDomainObjPrivatePtr priv; virDomainPausedReason reason; - int eventDetail; int state; virQEMUDriverConfigPtr cfg = NULL; @@ -1814,16 +1812,12 @@ static int qemuDomainSuspend(virDomainPtr dom) if (virDomainObjCheckActive(vm) < 0) goto endjob; - if (priv->job.asyncJob == QEMU_ASYNC_JOB_MIGRATION_OUT) { + if (priv->job.asyncJob == QEMU_ASYNC_JOB_MIGRATION_OUT) reason = VIR_DOMAIN_PAUSED_MIGRATION; - eventDetail = VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED; - } else if (priv->job.asyncJob == QEMU_ASYNC_JOB_SNAPSHOT) { + else if (priv->job.asyncJob == QEMU_ASYNC_JOB_SNAPSHOT) reason = VIR_DOMAIN_PAUSED_SNAPSHOT; - eventDetail = -1; /* don't create lifecycle events when doing snapshot */ - } else { + else reason = VIR_DOMAIN_PAUSED_USER; - eventDetail = VIR_DOMAIN_EVENT_SUSPENDED_PAUSED; - } state = virDomainObjGetState(vm, NULL); if (state == VIR_DOMAIN_PMSUSPENDED) { @@ -1833,12 +1827,6 @@ static int qemuDomainSuspend(virDomainPtr dom) } else if (state != VIR_DOMAIN_PAUSED) { if (qemuProcessStopCPUs(driver, vm, reason, QEMU_ASYNC_JOB_NONE) < 0) goto endjob; - - if (eventDetail >= 0) { - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_SUSPENDED, - eventDetail); - } } if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) goto endjob; @@ -1850,7 +1838,6 @@ static int qemuDomainSuspend(virDomainPtr dom) cleanup: virDomainObjEndAPI(&vm); - virObjectEventStateQueue(driver->domainEventState, event); virObjectUnref(cfg); return ret; } @@ -16175,13 +16162,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, VIR_DOMAIN_PAUSED_FROM_SNAPSHOT, QEMU_ASYNC_JOB_START) < 0) goto endjob; - /* 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. */ - detail = VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT; - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_SUSPENDED, - detail); if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("guest unexpectedly quit")); diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 6794033..5cfad1c 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1228,29 +1228,6 @@ qemuMigrationSrcIsSafe(virDomainDefPtr def, return true; } -/** qemuMigrationSrcSetOffline - * Pause domain for non-live migration. - */ -int -qemuMigrationSrcSetOffline(virQEMUDriverPtr driver, - virDomainObjPtr vm) -{ - int ret; - VIR_DEBUG("driver=%p vm=%p", driver, vm); - ret = qemuProcessStopCPUs(driver, vm, VIR_DOMAIN_PAUSED_MIGRATION, - QEMU_ASYNC_JOB_MIGRATION_OUT); - if (ret == 0) { - virObjectEventPtr event; - - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_SUSPENDED, - VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED); - virObjectEventStateQueue(driver->domainEventState, event); - } - - return ret; -} - void qemuMigrationAnyPostcopyFailed(virQEMUDriverPtr driver, @@ -1273,19 +1250,10 @@ qemuMigrationAnyPostcopyFailed(virQEMUDriverPtr driver, "leaving the domain paused", vm->def->name); if (state == VIR_DOMAIN_RUNNING) { - virObjectEventPtr event; - if (qemuProcessStopCPUs(driver, vm, VIR_DOMAIN_PAUSED_POSTCOPY_FAILED, - QEMU_ASYNC_JOB_MIGRATION_IN) < 0) { + QEMU_ASYNC_JOB_MIGRATION_IN) < 0) VIR_WARN("Unable to pause guest CPUs for %s", vm->def->name); - return; - } - - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_SUSPENDED, - VIR_DOMAIN_EVENT_SUSPENDED_POSTCOPY_FAILED); - virObjectEventStateQueue(driver->domainEventState, event); } else { virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_POSTCOPY_FAILED); @@ -3493,10 +3461,11 @@ qemuMigrationSrcRun(virQEMUDriverPtr driver, } } - /* Before EnterMonitor, since qemuMigrationSetOffline already does that */ + /* Before EnterMonitor, since already qemuProcessStopCPUs does that */ if (!(flags & VIR_MIGRATE_LIVE) && virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { - if (qemuMigrationSrcSetOffline(driver, vm) < 0) + if (qemuProcessStopCPUs(driver, vm, VIR_DOMAIN_PAUSED_MIGRATION, + QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) goto error; } @@ -3606,7 +3575,8 @@ qemuMigrationSrcRun(virQEMUDriverPtr driver, goto error; } } else if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING && - qemuMigrationSrcSetOffline(driver, vm) < 0) { + qemuProcessStopCPUs(driver, vm, VIR_DOMAIN_PAUSED_MIGRATION, + QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) { goto error; } diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index e12b697..a71cfe6 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -98,10 +98,6 @@ typedef enum { } qemuMigrationJobPhase; VIR_ENUM_DECL(qemuMigrationJobPhase) -int -qemuMigrationSrcSetOffline(virQEMUDriverPtr driver, - virDomainObjPtr vm); - char * qemuMigrationSrcBegin(virConnectPtr conn, virDomainObjPtr vm, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6858377..c5203a1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -659,13 +659,10 @@ qemuProcessHandleStop(qemuMonitorPtr mon ATTRIBUTE_UNUSED, if (priv->job.asyncJob == QEMU_ASYNC_JOB_MIGRATION_OUT) { if (priv->job.current->status == - QEMU_DOMAIN_JOB_STATUS_POSTCOPY) { + QEMU_DOMAIN_JOB_STATUS_POSTCOPY) reason = VIR_DOMAIN_PAUSED_POSTCOPY; - detail = VIR_DOMAIN_EVENT_SUSPENDED_POSTCOPY; - } else { + else reason = VIR_DOMAIN_PAUSED_MIGRATION; - detail = VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED; - } } detail = qemuDomainPausedReasonToSuspendedEvent(reason); @@ -3156,7 +3153,11 @@ int qemuProcessStopCPUs(virQEMUDriverPtr driver, if (priv->job.current) ignore_value(virTimeMillisNow(&priv->job.current->stopped)); - virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, reason); + /* The STOP event handler will change the domain state with the reason + * saved in priv->pausedReason and it will also emit corresponding domain + * lifecycle event. + */ + if (virDomainLockProcessPause(driver->lockManager, vm, &priv->lockState) < 0) VIR_WARN("Unable to release lease on %s", vm->def->name); VIR_DEBUG("Preserving lock state '%s'", NULLSTR(priv->lockState)); -- 1.8.3.1
$SUBJ: s/don't/Don't On 10/10/18 4:04 AM, Nikolay Shirokovskiy wrote:
Now when STOP event handler has correct both suspended event reason and paused state reason let's wipe out duplicated event sending and state changed in/after qemuProcessStopCPUs.
Since the STOP event handler can use the pausedReason as sent to qemuProcessStopCPUs, we no longer need to send duplicate suspended lifecycle events because we know what caused the stop along with extra details. This processing allows us to also remove the duplicated state change from qemuProcessStopCPUs.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 26 +++----------------------- src/qemu/qemu_migration.c | 42 ++++++------------------------------------ src/qemu/qemu_migration.h | 4 ---- src/qemu/qemu_process.c | 13 +++++++------ 4 files changed, 16 insertions(+), 69 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a52e249..7e08768 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1792,10 +1792,8 @@ static int qemuDomainSuspend(virDomainPtr dom) virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; - virObjectEventPtr event = NULL; qemuDomainObjPrivatePtr priv; virDomainPausedReason reason; - int eventDetail; int state; virQEMUDriverConfigPtr cfg = NULL;
@@ -1814,16 +1812,12 @@ static int qemuDomainSuspend(virDomainPtr dom) if (virDomainObjCheckActive(vm) < 0) goto endjob;
- if (priv->job.asyncJob == QEMU_ASYNC_JOB_MIGRATION_OUT) { + if (priv->job.asyncJob == QEMU_ASYNC_JOB_MIGRATION_OUT) reason = VIR_DOMAIN_PAUSED_MIGRATION; - eventDetail = VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED; - } else if (priv->job.asyncJob == QEMU_ASYNC_JOB_SNAPSHOT) { + else if (priv->job.asyncJob == QEMU_ASYNC_JOB_SNAPSHOT) reason = VIR_DOMAIN_PAUSED_SNAPSHOT; - eventDetail = -1; /* don't create lifecycle events when doing snapshot */
So with these changes how do we handle this special case? See commit f569b87f5 when this was introduced. Do we need to adjust qemuProcessHandleStop to not generate the event when "reason == VIR_DOMAIN_PAUSED_SNAPSHOT && priv->job.asyncJob == QEMU_ASYNC_JOB_SNAPSHOT"? The rest is OK - just need your thoughts on this particular case for the R-By. John
- } else { + else reason = VIR_DOMAIN_PAUSED_USER; - eventDetail = VIR_DOMAIN_EVENT_SUSPENDED_PAUSED; - }
[...]
On 16.10.2018 21:46, John Ferlan wrote:
$SUBJ:
s/don't/Don't
On 10/10/18 4:04 AM, Nikolay Shirokovskiy wrote:
Now when STOP event handler has correct both suspended event reason and paused state reason let's wipe out duplicated event sending and state changed in/after qemuProcessStopCPUs.
Since the STOP event handler can use the pausedReason as sent to qemuProcessStopCPUs, we no longer need to send duplicate suspended lifecycle events because we know what caused the stop along with extra details. This processing allows us to also remove the duplicated state change from qemuProcessStopCPUs.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 26 +++----------------------- src/qemu/qemu_migration.c | 42 ++++++------------------------------------ src/qemu/qemu_migration.h | 4 ---- src/qemu/qemu_process.c | 13 +++++++------ 4 files changed, 16 insertions(+), 69 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a52e249..7e08768 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1792,10 +1792,8 @@ static int qemuDomainSuspend(virDomainPtr dom) virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; - virObjectEventPtr event = NULL; qemuDomainObjPrivatePtr priv; virDomainPausedReason reason; - int eventDetail; int state; virQEMUDriverConfigPtr cfg = NULL;
@@ -1814,16 +1812,12 @@ static int qemuDomainSuspend(virDomainPtr dom) if (virDomainObjCheckActive(vm) < 0) goto endjob;
- if (priv->job.asyncJob == QEMU_ASYNC_JOB_MIGRATION_OUT) { + if (priv->job.asyncJob == QEMU_ASYNC_JOB_MIGRATION_OUT) reason = VIR_DOMAIN_PAUSED_MIGRATION; - eventDetail = VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED; - } else if (priv->job.asyncJob == QEMU_ASYNC_JOB_SNAPSHOT) { + else if (priv->job.asyncJob == QEMU_ASYNC_JOB_SNAPSHOT) reason = VIR_DOMAIN_PAUSED_SNAPSHOT; - eventDetail = -1; /* don't create lifecycle events when doing snapshot */
So with these changes how do we handle this special case? See commit f569b87f5 when this was introduced.
Do we need to adjust qemuProcessHandleStop to not generate the event when "reason == VIR_DOMAIN_PAUSED_SNAPSHOT && priv->job.asyncJob == QEMU_ASYNC_JOB_SNAPSHOT"?
Well we had lifecycle event anyway because of stop handler so I decided to keep sending it. However I'm not sure why it was not sent in qemuDomainSuspend originally. For example for migration we do send event. Looks like this event does not hurt anyone if it survives up to now. I'm ok with commit message to if the case. Nikolay
The rest is OK - just need your thoughts on this particular case for the R-By.
John
- } else { + else reason = VIR_DOMAIN_PAUSED_USER; - eventDetail = VIR_DOMAIN_EVENT_SUSPENDED_PAUSED; - }
[...]
On 10/17/18 4:58 AM, Nikolay Shirokovskiy wrote:
On 16.10.2018 21:46, John Ferlan wrote:
$SUBJ:
s/don't/Don't
On 10/10/18 4:04 AM, Nikolay Shirokovskiy wrote:
Now when STOP event handler has correct both suspended event reason and paused state reason let's wipe out duplicated event sending and state changed in/after qemuProcessStopCPUs.
Since the STOP event handler can use the pausedReason as sent to qemuProcessStopCPUs, we no longer need to send duplicate suspended lifecycle events because we know what caused the stop along with extra details. This processing allows us to also remove the duplicated state change from qemuProcessStopCPUs.
Placed pkrempa on the CC line to hopefully get his feedback in order to either move this along or require some rework due to the change possibly generating a lifecycle event now. It's not clear from the referenced commit why no event was desired and I'd prefer Peter's input just in case there's some particular reason that wasn't immediately evident or widely known... Tks - John
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 26 +++----------------------- src/qemu/qemu_migration.c | 42 ++++++------------------------------------ src/qemu/qemu_migration.h | 4 ---- src/qemu/qemu_process.c | 13 +++++++------ 4 files changed, 16 insertions(+), 69 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a52e249..7e08768 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1792,10 +1792,8 @@ static int qemuDomainSuspend(virDomainPtr dom) virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; - virObjectEventPtr event = NULL; qemuDomainObjPrivatePtr priv; virDomainPausedReason reason; - int eventDetail; int state; virQEMUDriverConfigPtr cfg = NULL;
@@ -1814,16 +1812,12 @@ static int qemuDomainSuspend(virDomainPtr dom) if (virDomainObjCheckActive(vm) < 0) goto endjob;
- if (priv->job.asyncJob == QEMU_ASYNC_JOB_MIGRATION_OUT) { + if (priv->job.asyncJob == QEMU_ASYNC_JOB_MIGRATION_OUT) reason = VIR_DOMAIN_PAUSED_MIGRATION; - eventDetail = VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED; - } else if (priv->job.asyncJob == QEMU_ASYNC_JOB_SNAPSHOT) { + else if (priv->job.asyncJob == QEMU_ASYNC_JOB_SNAPSHOT) reason = VIR_DOMAIN_PAUSED_SNAPSHOT; - eventDetail = -1; /* don't create lifecycle events when doing snapshot */
So with these changes how do we handle this special case? See commit f569b87f5 when this was introduced.
Do we need to adjust qemuProcessHandleStop to not generate the event when "reason == VIR_DOMAIN_PAUSED_SNAPSHOT && priv->job.asyncJob == QEMU_ASYNC_JOB_SNAPSHOT"?
Well we had lifecycle event anyway because of stop handler so I decided to keep sending it. However I'm not sure why it was not sent in qemuDomainSuspend originally. For example for migration we do send event. Looks like this event does not hurt anyone if it survives up to now.
I'm ok with commit message to if the case.
Nikolay
The rest is OK - just need your thoughts on this particular case for the R-By.
John
- } else { + else reason = VIR_DOMAIN_PAUSED_USER; - eventDetail = VIR_DOMAIN_EVENT_SUSPENDED_PAUSED; - }
[...]
participants (2)
-
John Ferlan -
Nikolay Shirokovskiy