[libvirt] [PATCH] qemu: Rework handling of shutdown event

When QEMU guest finishes its shutdown sequence, qemu stops virtual CPUs and when started with -no-shutdown waits for us to kill it using SGITERM. Since QEMU is flushing its internal buffers, some time may pass before QEMU actually dies. We mistakenly used "paused" state (and events) for this which is quite confusing since users may see a domain going to pause while they expect it to shutdown. Since we already have "shutdown" state with "the domain is being shut down" semantics, we should use it for this state. However, the state didn't have a corresponding event so I created one and called its detail as VIR_DOMAIN_EVENT_SHUTDOWN_FINISHED (guest OS finished its shutdown sequence) with the intent to add VIR_DOMAIN_EVENT_SHUTDOWN_STARTED in the future if we have a sufficiently capable guest agent that can notify us when guest OS starts to shutdown. --- include/libvirt/libvirt.h.in | 10 +++ src/qemu/qemu_process.c | 126 ++++++++++++++++++++++++++++++------------ 2 files changed, 100 insertions(+), 36 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index d01d1bc..8d4a049 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2288,6 +2288,7 @@ typedef enum { VIR_DOMAIN_EVENT_SUSPENDED = 3, VIR_DOMAIN_EVENT_RESUMED = 4, VIR_DOMAIN_EVENT_STOPPED = 5, + VIR_DOMAIN_EVENT_SHUTDOWN = 6, } virDomainEventType; /** @@ -2363,6 +2364,15 @@ typedef enum { /** + * virDomainEventShutdownDetailType: + * + * Details about the 'shutdown' lifecycle event + */ +typedef enum { + VIR_DOMAIN_EVENT_SHUTDOWN_FINISHED = 0, /* Guest finished shutdown sequence */ +} virDomainEventShutdownDetailType; + +/** * virConnectDomainEventCallback: * @conn: virConnect connection * @dom: The domain on which the event occurred diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ab0cb2b..5e8a20a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -431,19 +431,13 @@ cleanup: static void -qemuProcessShutdownOrReboot(virDomainObjPtr vm) +qemuProcessShutdownOrReboot(struct qemud_driver *driver, + virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; - if (priv->gotShutdown) { - VIR_DEBUG("Ignoring repeated SHUTDOWN event from domain %s", - vm->def->name); - return; - } - - priv->gotShutdown = true; if (priv->fakeReboot) { - qemuDomainSetFakeReboot(qemu_driver, vm, false); + qemuDomainSetFakeReboot(driver, vm, false); virDomainObjRef(vm); virThread th; if (virThreadCreate(&th, @@ -464,11 +458,47 @@ static int qemuProcessHandleShutdown(qemuMonitorPtr mon ATTRIBUTE_UNUSED, virDomainObjPtr vm) { + struct qemud_driver *driver = qemu_driver; + qemuDomainObjPrivatePtr priv; + virDomainEventPtr event = NULL; + VIR_DEBUG("vm=%p", vm); virDomainObjLock(vm); - qemuProcessShutdownOrReboot(vm); + + priv = vm->privateData; + if (priv->gotShutdown) { + VIR_DEBUG("Ignoring repeated SHUTDOWN event from domain %s", + vm->def->name); + goto unlock; + } + priv->gotShutdown = true; + + VIR_DEBUG("Transitioned guest %s to shutdown state", + vm->def->name); + virDomainObjSetState(vm, + VIR_DOMAIN_SHUTDOWN, + VIR_DOMAIN_SHUTDOWN_UNKNOWN); + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_SHUTDOWN, + VIR_DOMAIN_EVENT_SHUTDOWN_FINISHED); + + if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) { + VIR_WARN("Unable to save status on vm %s after state change", + vm->def->name); + } + + qemuProcessShutdownOrReboot(driver, vm); + +unlock: virDomainObjUnlock(vm); + + if (event) { + qemuDriverLock(driver); + qemuDomainEventQueue(driver, event); + qemuDriverUnlock(driver); + } + return 0; } @@ -479,22 +509,20 @@ qemuProcessHandleStop(qemuMonitorPtr mon ATTRIBUTE_UNUSED, { struct qemud_driver *driver = qemu_driver; virDomainEventPtr event = NULL; - virDomainPausedReason reason = VIR_DOMAIN_PAUSED_UNKNOWN; virDomainObjLock(vm); if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { qemuDomainObjPrivatePtr priv = vm->privateData; if (priv->gotShutdown) { - VIR_DEBUG("Got STOP event after SHUTDOWN, assuming we are stopping" - " for shutdown"); - reason = VIR_DOMAIN_PAUSED_SHUTTING_DOWN; + VIR_DEBUG("Ignoring STOP event after SHUTDOWN"); + goto unlock; } - VIR_DEBUG("Transitioned guest %s to paused state, reason=%s", - vm->def->name, virDomainPausedReasonTypeToString(reason)); + VIR_DEBUG("Transitioned guest %s to paused state", + vm->def->name); - virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, reason); + virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_UNKNOWN); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_SUSPENDED, VIR_DOMAIN_EVENT_SUSPENDED_PAUSED); @@ -509,12 +537,13 @@ qemuProcessHandleStop(qemuMonitorPtr mon ATTRIBUTE_UNUSED, vm->def->name); } } + +unlock: virDomainObjUnlock(vm); if (event) { qemuDriverLock(driver); - if (event) - qemuDomainEventQueue(driver, event); + qemuDomainEventQueue(driver, event); qemuDriverUnlock(driver); } @@ -2338,7 +2367,10 @@ qemuProcessUpdateState(struct qemud_driver *driver, virDomainObjPtr vm) qemuDomainObjPrivatePtr priv = vm->privateData; virDomainState state; virDomainPausedReason reason; + virDomainState newState = VIR_DOMAIN_NOSTATE; + int newReason; bool running; + char *msg = NULL; int ret; qemuDomainObjEnterMonitorWithDriver(driver, vm); @@ -2351,19 +2383,40 @@ qemuProcessUpdateState(struct qemud_driver *driver, virDomainObjPtr vm) state = virDomainObjGetState(vm, NULL); if (state == VIR_DOMAIN_PAUSED && running) { - VIR_DEBUG("Domain %s was unpaused while its monitor was disconnected;" - " changing state to running", vm->def->name); - virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, - VIR_DOMAIN_RUNNING_UNPAUSED); + newState = VIR_DOMAIN_RUNNING; + newReason = VIR_DOMAIN_RUNNING_UNPAUSED; + msg = strdup("was unpaused"); } else if (state == VIR_DOMAIN_RUNNING && !running) { - VIR_DEBUG("Domain %s was paused (%s) while its monitor was" - " disconnected; changing state to paused", - vm->def->name, virDomainPausedReasonTypeToString(reason)); - virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, reason); + if (reason == VIR_DOMAIN_PAUSED_SHUTTING_DOWN) { + newState = VIR_DOMAIN_SHUTDOWN; + newReason = VIR_DOMAIN_SHUTDOWN_UNKNOWN; + msg = strdup("shutdown"); + } else { + newState = VIR_DOMAIN_PAUSED; + newReason = reason; + virAsprintf(&msg, "was paused (%s)", + virDomainPausedReasonTypeToString(reason)); + } } else if (state == VIR_DOMAIN_SHUTOFF && running) { - VIR_DEBUG("Domain %s finished booting; changing state to running", - vm->def->name); - virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_BOOTED); + newState = VIR_DOMAIN_RUNNING; + newReason = VIR_DOMAIN_RUNNING_BOOTED; + msg = strdup("finished booting"); + } + + if (newState != VIR_DOMAIN_NOSTATE) { + if (!msg) { + virReportOOMError(); + return -1; + } + + VIR_DEBUG("Domain %s %s while its monitor was disconnected;" + " changing state to %s (%s)", + vm->def->name, + msg, + virDomainStateTypeToString(newState), + virDomainStateReasonToString(newState, newReason)); + VIR_FREE(msg); + virDomainObjSetState(vm, newState, newReason); } return 0; @@ -2637,15 +2690,16 @@ qemuProcessReconnect(void *opaque) &priv->qemuCaps) < 0) goto error; - /* In case the domain was paused for shutdown while we were not running, + /* In case the domain shutdown while we were not running, * we need to finish the shutdown process. And we need to do it after * we have qemuCaps filled in. */ - if (state == VIR_DOMAIN_PAUSED - && reason == VIR_DOMAIN_PAUSED_SHUTTING_DOWN) { - VIR_DEBUG("Domain %s shut down while we were not running;" - " finishing shutdown sequence", obj->def->name); - qemuProcessShutdownOrReboot(obj); + if (state == VIR_DOMAIN_SHUTDOWN || + (state == VIR_DOMAIN_PAUSED && + reason == VIR_DOMAIN_PAUSED_SHUTTING_DOWN)) { + VIR_DEBUG("Finishing shutdown sequence for domain %s", + obj->def->name); + qemuProcessShutdownOrReboot(driver, obj); goto endjob; } -- 1.7.8.rc4

On 12/02/2011 08:02 AM, Jiri Denemark wrote:
When QEMU guest finishes its shutdown sequence, qemu stops virtual CPUs and when started with -no-shutdown waits for us to kill it using SGITERM. Since QEMU is flushing its internal buffers, some time may pass before QEMU actually dies. We mistakenly used "paused" state (and events) for this which is quite confusing since users may see a domain going to pause while they expect it to shutdown. Since we already have "shutdown" state with "the domain is being shut down" semantics, we should use it for this state.
However, the state didn't have a corresponding event so I created one and called its detail as VIR_DOMAIN_EVENT_SHUTDOWN_FINISHED (guest OS finished its shutdown sequence) with the intent to add VIR_DOMAIN_EVENT_SHUTDOWN_STARTED in the future if we have a sufficiently capable guest agent that can notify us when guest OS starts to shutdown.
Not for this patch, but should we use VIR_DOMAIN_EVENT_SHUTDOWN_STARTED any time we use the virDomainShutdown API to request a graceful shutdown? That is, by actually changing the state of the domain after a call to virDomainShutdown, it will be more apparent whether a user has previously requested shutdown, and if the guest is still running, it means the guest either didn't react to the ACPI interrupt or else is taking a long time to shut down.
+++ b/include/libvirt/libvirt.h.in @@ -2288,6 +2288,7 @@ typedef enum { VIR_DOMAIN_EVENT_SUSPENDED = 3, VIR_DOMAIN_EVENT_RESUMED = 4, VIR_DOMAIN_EVENT_STOPPED = 5, + VIR_DOMAIN_EVENT_SHUTDOWN = 6, } virDomainEventType;
/** @@ -2363,6 +2364,15 @@ typedef enum {
/** + * virDomainEventShutdownDetailType: + * + * Details about the 'shutdown' lifecycle event + */ +typedef enum { + VIR_DOMAIN_EVENT_SHUTDOWN_FINISHED = 0, /* Guest finished shutdown sequence */ +} virDomainEventShutdownDetailType; +
Makes sense. But you're missing a change to examples/domain-events/events-c/event-test.c to reflect the new event.
+ + VIR_DEBUG("Domain %s %s while its monitor was disconnected;" + " changing state to %s (%s)", + vm->def->name, + msg,
I was about to complain that building up an English sentence in pieces like this doesn't translate well; then I realized this is VIR_DEBUG and it isn't translated in the first place :) ACK. Up to you whether to fold the examples/ change in now or as a followon patch, as long as you remember to do it. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Fri, Dec 02, 2011 at 09:22:37 -0700, Eric Blake wrote:
On 12/02/2011 08:02 AM, Jiri Denemark wrote:
When QEMU guest finishes its shutdown sequence, qemu stops virtual CPUs and when started with -no-shutdown waits for us to kill it using SGITERM. Since QEMU is flushing its internal buffers, some time may pass before QEMU actually dies. We mistakenly used "paused" state (and events) for this which is quite confusing since users may see a domain going to pause while they expect it to shutdown. Since we already have "shutdown" state with "the domain is being shut down" semantics, we should use it for this state.
However, the state didn't have a corresponding event so I created one and called its detail as VIR_DOMAIN_EVENT_SHUTDOWN_FINISHED (guest OS finished its shutdown sequence) with the intent to add VIR_DOMAIN_EVENT_SHUTDOWN_STARTED in the future if we have a sufficiently capable guest agent that can notify us when guest OS starts to shutdown.
Not for this patch, but should we use VIR_DOMAIN_EVENT_SHUTDOWN_STARTED any time we use the virDomainShutdown API to request a graceful shutdown? That is, by actually changing the state of the domain after a call to virDomainShutdown, it will be more apparent whether a user has previously requested shutdown
Hmm, in that case I would rather introduce another detail VIR_DOMAIN_EVENT_SHUTDOWN_REQUESTED or something similar so that one can distinguish between a request that any guest can just ignore and a real shutdown. And more importantly, we need to decide what state a domain should be in after virDomainShutdown. IMHO it should still be "running" until we know the guest is really shutting down. Perhaps a new reason for "running" state that would mean the domain is running but a shutdown was requested could make sense.
ACK. Up to you whether to fold the examples/ change in now or as a followon patch, as long as you remember to do it.
Thanks, I sent a separate patch for adding the event to examples/domain-events/ and pushed this patch. Jirka

On Fri, Dec 02, 2011 at 09:22:37AM -0700, Eric Blake wrote:
On 12/02/2011 08:02 AM, Jiri Denemark wrote:
When QEMU guest finishes its shutdown sequence, qemu stops virtual CPUs and when started with -no-shutdown waits for us to kill it using SGITERM. Since QEMU is flushing its internal buffers, some time may pass before QEMU actually dies. We mistakenly used "paused" state (and events) for this which is quite confusing since users may see a domain going to pause while they expect it to shutdown. Since we already have "shutdown" state with "the domain is being shut down" semantics, we should use it for this state.
However, the state didn't have a corresponding event so I created one and called its detail as VIR_DOMAIN_EVENT_SHUTDOWN_FINISHED (guest OS finished its shutdown sequence) with the intent to add VIR_DOMAIN_EVENT_SHUTDOWN_STARTED in the future if we have a sufficiently capable guest agent that can notify us when guest OS starts to shutdown.
Not for this patch, but should we use VIR_DOMAIN_EVENT_SHUTDOWN_STARTED any time we use the virDomainShutdown API to request a graceful shutdown? That is, by actually changing the state of the domain after a call to virDomainShutdown, it will be more apparent whether a user has previously requested shutdown, and if the guest is still running, it means the guest either didn't react to the ACPI interrupt or else is taking a long time to shut down.
I don't think we should do this in the lifecycle events. Not least because emitting "VIR_DOMAIN_EVENT_SHUTDOWN_STARTED" would be very misleading because a guest OS is free to ignore the ACPI power event. The lifecycle events should only reflect things that have actually happened to the guest VM lifecycle & I don't really this counts. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Daniel P. Berrange
-
Eric Blake
-
Jiri Denemark