[libvirt] [PATCH] Add RESUME event listener to qemu monitor.
Perform all the appropriate plumbing. When qemu/KVM VMs are paused manually through a monitor not-owned by libvirt, libvirt will think of them as "paused" event after they are resumed and effectively running. With this patch the discrepancy goes away. This is meant to address bug 892791. Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org> --- src/qemu/qemu_monitor.c | 10 ++++++++ src/qemu/qemu_monitor.h | 3 +++ src/qemu/qemu_monitor_json.c | 7 ++++++ src/qemu/qemu_process.c | 56 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 76 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 99642b6..cb5a3e2 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1022,6 +1022,16 @@ int qemuMonitorEmitStop(qemuMonitorPtr mon) } +int qemuMonitorEmitResume(qemuMonitorPtr mon) +{ + int ret = -1; + VIR_DEBUG("mon=%p", mon); + + QEMU_MONITOR_CALLBACK(mon, ret, domainResume, mon->vm); + return ret; +} + + int qemuMonitorEmitRTCChange(qemuMonitorPtr mon, long long offset) { int ret = -1; diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index d7faa90..00ce813 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -97,6 +97,8 @@ struct _qemuMonitorCallbacks { virDomainObjPtr vm); int (*domainStop)(qemuMonitorPtr mon, virDomainObjPtr vm); + int (*domainResume)(qemuMonitorPtr mon, + virDomainObjPtr vm); int (*domainRTCChange)(qemuMonitorPtr mon, virDomainObjPtr vm, long long offset); @@ -187,6 +189,7 @@ int qemuMonitorEmitShutdown(qemuMonitorPtr mon); int qemuMonitorEmitReset(qemuMonitorPtr mon); int qemuMonitorEmitPowerdown(qemuMonitorPtr mon); int qemuMonitorEmitStop(qemuMonitorPtr mon); +int qemuMonitorEmitResume(qemuMonitorPtr mon); int qemuMonitorEmitRTCChange(qemuMonitorPtr mon, long long offset); int qemuMonitorEmitWatchdog(qemuMonitorPtr mon, int action); int qemuMonitorEmitIOError(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 2d2d254..de5f115 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -55,6 +55,7 @@ static void qemuMonitorJSONHandleShutdown(qemuMonitorPtr mon, virJSONValuePtr da static void qemuMonitorJSONHandleReset(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandlePowerdown(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandleStop(qemuMonitorPtr mon, virJSONValuePtr data); +static void qemuMonitorJSONHandleResume(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandleRTCChange(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandleWatchdog(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandleIOError(qemuMonitorPtr mon, virJSONValuePtr data); @@ -87,6 +88,7 @@ static qemuEventHandler eventHandlers[] = { { "DEVICE_TRAY_MOVED", qemuMonitorJSONHandleTrayChange, }, { "POWERDOWN", qemuMonitorJSONHandlePowerdown, }, { "RESET", qemuMonitorJSONHandleReset, }, + { "RESUME", qemuMonitorJSONHandleResume, }, { "RTC_CHANGE", qemuMonitorJSONHandleRTCChange, }, { "SHUTDOWN", qemuMonitorJSONHandleShutdown, }, { "SPICE_CONNECTED", qemuMonitorJSONHandleSPICEConnect, }, @@ -589,6 +591,11 @@ static void qemuMonitorJSONHandleStop(qemuMonitorPtr mon, virJSONValuePtr data A qemuMonitorEmitStop(mon); } +static void qemuMonitorJSONHandleResume(qemuMonitorPtr mon, virJSONValuePtr data ATTRIBUTE_UNUSED) +{ + qemuMonitorEmitResume(mon); +} + static void qemuMonitorJSONHandleRTCChange(qemuMonitorPtr mon, virJSONValuePtr data) { long long offset = 0; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f5fc1b3..938c17e 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -736,6 +736,61 @@ unlock: static int +qemuProcessHandleResume(qemuMonitorPtr mon ATTRIBUTE_UNUSED, + virDomainObjPtr vm) +{ + virQEMUDriverPtr driver = qemu_driver; + virDomainEventPtr event = NULL; + + virDomainObjLock(vm); + if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) { + qemuDomainObjPrivatePtr priv = vm->privateData; + + if (priv->gotShutdown) { + VIR_DEBUG("Ignoring RESUME event after SHUTDOWN"); + goto unlock; + } + + VIR_DEBUG("Transitioned guest %s out of paused into resumed state", + vm->def->name); + + virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, + VIR_DOMAIN_RUNNING_UNPAUSED); + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_RESUMED, + VIR_DOMAIN_EVENT_RESUMED_UNPAUSED); + + VIR_DEBUG("Using lock state '%s' on resume event", NULLSTR(priv->lockState)); + if (virDomainLockProcessResume(driver->lockManager, driver->uri, + vm, priv->lockState) < 0) { + /* Don't free priv->lockState on error, because we need + * to make sure we have state still present if the user + * tries to resume again + */ + goto unlock; + } + VIR_FREE(priv->lockState); + + if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) { + VIR_WARN("Unable to save status on vm %s after state change", + vm->def->name); + } + } + +unlock: + virDomainObjUnlock(vm); + + if (event) { + qemuDriverLock(driver); + qemuDomainEventQueue(driver, event); + qemuDriverUnlock(driver); + } + + return 0; +} + + +static int qemuProcessHandleRTCChange(qemuMonitorPtr mon ATTRIBUTE_UNUSED, virDomainObjPtr vm, long long offset) @@ -1250,6 +1305,7 @@ static qemuMonitorCallbacks monitorCallbacks = { .diskSecretLookup = qemuProcessFindVolumeQcowPassphrase, .domainShutdown = qemuProcessHandleShutdown, .domainStop = qemuProcessHandleStop, + .domainResume = qemuProcessHandleResume, .domainReset = qemuProcessHandleReset, .domainRTCChange = qemuProcessHandleRTCChange, .domainWatchdog = qemuProcessHandleWatchdog, -- 1.7.9.5
On Mon, Jan 07, 2013 at 04:25:01PM -0500, Andres Lagar-Cavilla wrote:
Perform all the appropriate plumbing.
When qemu/KVM VMs are paused manually through a monitor not-owned by libvirt, libvirt will think of them as "paused" event after they are resumed and effectively running. With this patch the discrepancy goes away.
This is meant to address bug 892791.
Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Although we don't support people issuing monitor commands behind libvirt's back, I see no harm in handling the resume event here, as long as we don't end up emitting multiple resume events for a single operation (eg if libvirt triggers the resume it already emits an event indepedently) 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 :|
On Mon, Jan 07, 2013 at 10:44:41PM +0000, Daniel P. Berrange wrote:
On Mon, Jan 07, 2013 at 04:25:01PM -0500, Andres Lagar-Cavilla wrote:
Perform all the appropriate plumbing.
When qemu/KVM VMs are paused manually through a monitor not-owned by libvirt, libvirt will think of them as "paused" event after they are resumed and effectively running. With this patch the discrepancy goes away.
This is meant to address bug 892791.
Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Although we don't support people issuing monitor commands behind libvirt's back, I see no harm in handling the resume event here, as long as we don't end up emitting multiple resume events for a single operation (eg if libvirt triggers the resume it already emits an event indepedently)
+1 Dave
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 :|
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Jan 7, 2013, at 5:44 PM, "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Mon, Jan 07, 2013 at 04:25:01PM -0500, Andres Lagar-Cavilla wrote:
Perform all the appropriate plumbing.
When qemu/KVM VMs are paused manually through a monitor not-owned by libvirt, libvirt will think of them as "paused" event after they are resumed and effectively running. With this patch the discrepancy goes away.
This is meant to address bug 892791.
Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Although we don't support people issuing monitor commands behind libvirt's back, I see no harm in handling the resume event here, as long as we don't end up emitting multiple resume events for a single operation (eg if libvirt triggers the resume it already emits an event independently)
Hi Daniel, In most cases, qemuProcessStartCPUs will be the libvirt API issuing qemu 'cont' commands. Due to the locking rules, we can be confident that it will complete processing while holding the VM lock, and in the process update the state to VIR_DOMAIN_RUNNING. The callback handler for RESUME introduced here will be serialized against the VM lock after StartCPUs, and result in effectively a no-op (because it only does work if the vm state is PAUSED). Create/Load Snapshot call StopCPUs before doing any work, so qemu itself will not attempt to resume the VM, and thus no event will be generated. In strict terms this is unnecessary (although relatively little) work, but here is the rationale in qemu_driver.c: /* savevm monitor command pauses the domain emitting an event which * confuses libvirt since it's not notified when qemu resumes the * domain. Thus we stop and start CPUs ourselves. */ Relevant to the issue at hand. If this patch is merged then the library can get rid of this StopCPUs invocation, but additional book-keeping is necessary. IMHO this is neither pressing nor necessary. Thanks Andres
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 :|
On Tue, Jan 08, 2013 at 01:28:10PM -0500, Andres Lagar-Cavilla wrote:
On Jan 7, 2013, at 5:44 PM, "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Mon, Jan 07, 2013 at 04:25:01PM -0500, Andres Lagar-Cavilla wrote:
Perform all the appropriate plumbing.
When qemu/KVM VMs are paused manually through a monitor not-owned by libvirt, libvirt will think of them as "paused" event after they are resumed and effectively running. With this patch the discrepancy goes away.
This is meant to address bug 892791.
Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Although we don't support people issuing monitor commands behind libvirt's back, I see no harm in handling the resume event here, as long as we don't end up emitting multiple resume events for a single operation (eg if libvirt triggers the resume it already emits an event independently)
Hi Daniel,
In most cases, qemuProcessStartCPUs will be the libvirt API issuing qemu 'cont' commands. Due to the locking rules, we can be confident that it will complete processing while holding the VM lock, and in the process update the state to VIR_DOMAIN_RUNNING. The callback handler for RESUME introduced here will be serialized against the VM lock after StartCPUs, and result in effectively a no-op (because it only does work if the vm state is PAUSED).
Create/Load Snapshot call StopCPUs before doing any work, so qemu itself will not attempt to resume the VM, and thus no event will be generated. In strict terms this is unnecessary (although relatively little) work, but here is the rationale in qemu_driver.c: /* savevm monitor command pauses the domain emitting an event which * confuses libvirt since it's not notified when qemu resumes the * domain. Thus we stop and start CPUs ourselves. */
Relevant to the issue at hand. If this patch is merged then the library can get rid of this StopCPUs invocation, but additional book-keeping is necessary. IMHO this is neither pressing nor necessary.
Actally we can't get rid of it, because events only work in QMP mode. If talking to an old QEMU via HMP we need to deal with it as described in the comment. ACK to your patch anyway. 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 :|
On Jan 8, 2013, at 1:30 PM, "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Tue, Jan 08, 2013 at 01:28:10PM -0500, Andres Lagar-Cavilla wrote:
On Jan 7, 2013, at 5:44 PM, "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Mon, Jan 07, 2013 at 04:25:01PM -0500, Andres Lagar-Cavilla wrote:
Perform all the appropriate plumbing.
When qemu/KVM VMs are paused manually through a monitor not-owned by libvirt, libvirt will think of them as "paused" event after they are resumed and effectively running. With this patch the discrepancy goes away.
This is meant to address bug 892791.
Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Although we don't support people issuing monitor commands behind libvirt's back, I see no harm in handling the resume event here, as long as we don't end up emitting multiple resume events for a single operation (eg if libvirt triggers the resume it already emits an event independently)
Hi Daniel,
In most cases, qemuProcessStartCPUs will be the libvirt API issuing qemu 'cont' commands. Due to the locking rules, we can be confident that it will complete processing while holding the VM lock, and in the process update the state to VIR_DOMAIN_RUNNING. The callback handler for RESUME introduced here will be serialized against the VM lock after StartCPUs, and result in effectively a no-op (because it only does work if the vm state is PAUSED).
Create/Load Snapshot call StopCPUs before doing any work, so qemu itself will not attempt to resume the VM, and thus no event will be generated. In strict terms this is unnecessary (although relatively little) work, but here is the rationale in qemu_driver.c: /* savevm monitor command pauses the domain emitting an event which * confuses libvirt since it's not notified when qemu resumes the * domain. Thus we stop and start CPUs ourselves. */
Relevant to the issue at hand. If this patch is merged then the library can get rid of this StopCPUs invocation, but additional book-keeping is necessary. IMHO this is neither pressing nor necessary.
Actally we can't get rid of it, because events only work in QMP mode. If talking to an old QEMU via HMP we need to deal with it as described in the comment.
ACK to your patch anyway.
Cool, thanks. Not sure about the process around here. More ACKs needed? Andres
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 :|
On 08.01.2013 21:08, Andres Lagar-Cavilla wrote:
On Jan 8, 2013, at 1:30 PM, "Daniel P. Berrange" <berrange@redhat.com> wrote:
<snip/>
ACK to your patch anyway.
Cool, thanks.
Not sure about the process around here. More ACKs needed?
Andres
No, I've pushed the patch now. Michal
On Jan 9, 2013, at 4:26 AM, Michal Privoznik <mprivozn@redhat.com> wrote:
On 08.01.2013 21:08, Andres Lagar-Cavilla wrote:
On Jan 8, 2013, at 1:30 PM, "Daniel P. Berrange" <berrange@redhat.com> wrote:
<snip/>
ACK to your patch anyway.
Cool, thanks.
Not sure about the process around here. More ACKs needed?
Andres
No, I've pushed the patch now.
Thanks much. Regards, Andres
Michal
On 01/07/2013 02:25 PM, Andres Lagar-Cavilla wrote:
Perform all the appropriate plumbing.
When qemu/KVM VMs are paused manually through a monitor not-owned by libvirt, libvirt will think of them as "paused" event after they are resumed and effectively running. With this patch the discrepancy goes away.
This is meant to address bug 892791.
Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org> --- src/qemu/qemu_monitor.c | 10 ++++++++ src/qemu/qemu_monitor.h | 3 +++ src/qemu/qemu_monitor_json.c | 7 ++++++ src/qemu/qemu_process.c | 56 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 76 insertions(+)
Thanks again for insisting on this patch; it turns out that it solved a very real problem with 'virsh block-copy --wait --pivot ...', 'virsh dump --live', and 'virsh create-snapshot-as --live --memspec ...', for upper level applications (like VDSM) that were tracing domain state solely through libvirt events rather than through querying libvirt for its current idea of domain state. See https://bugzilla.redhat.com/show_bug.cgi?id=894085 for details, and my analysis why this patch is necessary even when there is no external monitor and no use of unsupported qemu-monitor-command. It does make me wonder if we ought to audit all callers of qemuDomainStartCPUs/qemuDomainStopCPUs to be more robust if qemu were ever to change to emit events _after_ responding to 'stop' and 'cont' monitor commands, instead of the current (lucky) results that the event always appears on the monitor before the return value of the command. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Jan 15, 2013, at 5:13 PM, Eric Blake <eblake@redhat.com> wrote:
On 01/07/2013 02:25 PM, Andres Lagar-Cavilla wrote:
Perform all the appropriate plumbing.
When qemu/KVM VMs are paused manually through a monitor not-owned by libvirt, libvirt will think of them as "paused" event after they are resumed and effectively running. With this patch the discrepancy goes away.
This is meant to address bug 892791.
Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org> --- src/qemu/qemu_monitor.c | 10 ++++++++ src/qemu/qemu_monitor.h | 3 +++ src/qemu/qemu_monitor_json.c | 7 ++++++ src/qemu/qemu_process.c | 56 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 76 insertions(+)
Thanks again for insisting on this patch; it turns out that it solved a very real problem with 'virsh block-copy --wait --pivot ...', 'virsh dump --live', and 'virsh create-snapshot-as --live --memspec ...', for upper level applications (like VDSM) that were tracing domain state solely through libvirt events rather than through querying libvirt for its current idea of domain state. See https://bugzilla.redhat.com/show_bug.cgi?id=894085 for details, and my analysis why this patch is necessary even when there is no external monitor and no use of unsupported qemu-monitor-command.
Always happy to be of service. Karma points, ka-chin ;)
It does make me wonder if we ought to audit all callers of qemuDomainStartCPUs/qemuDomainStopCPUs to be more robust if qemu were ever to change to emit events _after_ responding to 'stop' and 'cont' monitor commands, instead of the current (lucky) results that the event always appears on the monitor before the return value of the command.
It has been floated around that one should not rely on any event ordering. I understand the qemu maintainer's motivation to not give the impression of stable interfaces where it's not relevant. However, from looking at qemu's code it would be actually harder to emit events out of order than the current state. So I don't think this is a pressing worry. Cheers! Andres
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (6)
-
Andres Lagar-Cavilla -
Andres Lagar-Cavilla -
Daniel P. Berrange -
Dave Allan -
Eric Blake -
Michal Privoznik