[libvirt] [PATCH] qemu: Honour <on_reboot/>
Martin Kletzander
mkletzan at redhat.com
Thu Aug 10 14:02:43 UTC 2017
On Thu, Aug 03, 2017 at 09:36:27AM +0200, Michal Privoznik wrote:
>https://bugzilla.redhat.com/show_bug.cgi?id=1476866
>
>For some reason, we completely ignore <on_reboot/> setting for
>domains. The implementation is simply not there. It never was.
>However, things are slightly more complicated. QEMU sends us two
>RESET events on domain reboot. Fortunately, the event contains
>this 'guest' field telling us who initiated the reboot. And since
>we don't want to destroy the domain if the reset is initiated by
>a user, we have to ignore those events. Whatever, just look at
>the code.
>
>Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>---
> src/qemu/qemu_domain.h | 1 +
> src/qemu/qemu_monitor.c | 4 ++--
> src/qemu/qemu_monitor.h | 3 ++-
> src/qemu/qemu_monitor_json.c | 8 +++++++-
> src/qemu/qemu_process.c | 34 ++++++++++++++++++++++++++++++----
> 5 files changed, 42 insertions(+), 8 deletions(-)
>
>diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
>index 4c9050aff..d865e67c7 100644
>--- a/src/qemu/qemu_domain.h
>+++ b/src/qemu/qemu_domain.h
>@@ -233,6 +233,7 @@ struct _qemuDomainObjPrivate {
> bool agentError;
>
> bool gotShutdown;
>+ bool gotReset;
> bool beingDestroyed;
> char *pidfile;
>
>diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
>index 19082d8bf..8f81a2b28 100644
>--- a/src/qemu/qemu_monitor.c
>+++ b/src/qemu/qemu_monitor.c
>@@ -1344,12 +1344,12 @@ qemuMonitorEmitShutdown(qemuMonitorPtr mon, virTristateBool guest)
>
>
> int
>-qemuMonitorEmitReset(qemuMonitorPtr mon)
>+qemuMonitorEmitReset(qemuMonitorPtr mon, virTristateBool guest)
> {
> int ret = -1;
> VIR_DEBUG("mon=%p", mon);
>
>- QEMU_MONITOR_CALLBACK(mon, ret, domainReset, mon->vm);
>+ QEMU_MONITOR_CALLBACK(mon, ret, domainReset, mon->vm, guest);
> return ret;
> }
>
>diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
>index 31f7e97ba..8c33f6783 100644
>--- a/src/qemu/qemu_monitor.h
>+++ b/src/qemu/qemu_monitor.h
>@@ -134,6 +134,7 @@ typedef int (*qemuMonitorDomainShutdownCallback)(qemuMonitorPtr mon,
> void *opaque);
> typedef int (*qemuMonitorDomainResetCallback)(qemuMonitorPtr mon,
> virDomainObjPtr vm,
>+ virTristateBool guest,
> void *opaque);
> typedef int (*qemuMonitorDomainPowerdownCallback)(qemuMonitorPtr mon,
> virDomainObjPtr vm,
>@@ -346,7 +347,7 @@ int qemuMonitorEmitEvent(qemuMonitorPtr mon, const char *event,
> long long seconds, unsigned int micros,
> const char *details);
> int qemuMonitorEmitShutdown(qemuMonitorPtr mon, virTristateBool guest);
>-int qemuMonitorEmitReset(qemuMonitorPtr mon);
>+int qemuMonitorEmitReset(qemuMonitorPtr mon, virTristateBool guest);
> int qemuMonitorEmitPowerdown(qemuMonitorPtr mon);
> int qemuMonitorEmitStop(qemuMonitorPtr mon);
> int qemuMonitorEmitResume(qemuMonitorPtr mon);
>diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
>index b8a68154a..8a1501ced 100644
>--- a/src/qemu/qemu_monitor_json.c
>+++ b/src/qemu/qemu_monitor_json.c
>@@ -536,7 +536,13 @@ static void qemuMonitorJSONHandleShutdown(qemuMonitorPtr mon, virJSONValuePtr da
>
> static void qemuMonitorJSONHandleReset(qemuMonitorPtr mon, virJSONValuePtr data ATTRIBUTE_UNUSED)
> {
>- qemuMonitorEmitReset(mon);
>+ bool guest = false;
>+ virTristateBool guest_initiated = VIR_TRISTATE_BOOL_ABSENT;
>+
>+ if (data && virJSONValueObjectGetBoolean(data, "guest", &guest) == 0)
>+ guest_initiated = guest ? VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO;
>+
>+ qemuMonitorEmitReset(mon, guest_initiated);
> }
>
> static void qemuMonitorJSONHandlePowerdown(qemuMonitorPtr mon, virJSONValuePtr data ATTRIBUTE_UNUSED)
>diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>index 0aecce3b1..889efc7f0 100644
>--- a/src/qemu/qemu_process.c
>+++ b/src/qemu/qemu_process.c
>@@ -478,27 +478,51 @@ qemuProcessFindVolumeQcowPassphrase(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
> static int
> qemuProcessHandleReset(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
> virDomainObjPtr vm,
>+ virTristateBool guest_initiated,
> void *opaque)
> {
> virQEMUDriverPtr driver = opaque;
>- virObjectEventPtr event;
>+ virObjectEventPtr event = NULL;
> qemuDomainObjPrivatePtr priv;
> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>+ bool callOnReboot = false;
>
> virObjectLock(vm);
>
>+ priv = vm->privateData;
>+
>+ /* This is a bit tricky. When a guest does 'reboot' we receive RESET event
>+ * twice, both times it's guest initiated. However, if users call 'virsh
>+ * reset' we still receive two events but the first one is guest_initiated
>+ * = no, the second one is guest_initiated = yes. Therefore, to avoid
>+ * executing onReboot action in the latter case we need this complicated
>+ * construction. */
I think there is a bug in qemu if calling reset gets us one
guest-initiated reset. You are not guaranteed to get two events anyway,
I believe.
Anyway, let's say you're right (for now), I think the following logic is
flawed a bit.
>+ if (guest_initiated == VIR_TRISTATE_BOOL_NO) {
>+ VIR_DEBUG("Ignoring not guest initiated RESET event from domain %s",
>+ vm->def->name);
>+ priv->gotReset = true;
>+ } else if (priv->gotReset && guest_initiated == VIR_TRISTATE_BOOL_YES) {
>+ VIR_DEBUG("Ignoring second RESET event from domain %s",
>+ vm->def->name);
>+ priv->gotReset = false;
>+ } else {
>+ callOnReboot = true;
This will be set either if guest_initiated == VIR_TRISTATE_BOOL_ABSENT
(keep in mind this will always be the case with older QEMUs) or
priv->gotReset == false && guest_initiated == VIR_TRISTATE_BOOL_YES.
If we walk through your examples (reboot => guest_initiated = [yes,
yes], reset => guest_initiated = [no, yes]), then:
reboot:
- first event (guest_initiated = yes) => callOnReboot = true;
- second event (guest_initiated = yes) => callOnReboot = true;
( because priv->gotReset is still false )
reset:
- first event (guest_initiated = no) => priv->gotReset = true;
- second event (guest_initiated = yes) => priv->gotReset = false;
So basically in the first scenario you only set the bool to true and in
the second one nothing is set ...
>+ }
>+
> event = virDomainEventRebootNewFromObj(vm);
>- priv = vm->privateData;
> if (priv->agent)
> qemuAgentNotifyEvent(priv->agent, QEMU_AGENT_EVENT_RESET);
>
> if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0)
> VIR_WARN("Failed to save status on vm %s", vm->def->name);
>
>+ if (callOnReboot &&
>+ guest_initiated == VIR_TRISTATE_BOOL_YES &&
... so either this will be never executed or I missed something.
>+ vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_DESTROY)
>+ qemuProcessShutdownOrReboot(driver, vm);
>+
You should also check VIR_DOMAIN_LIFECYCLE_PRESERVE according to the
documentation:
... The preserve action for an on_reboot event is treated as a destroy ...
> virObjectUnlock(vm);
>-
> qemuDomainEventQueue(driver, event);
>-
Spurious whitespace changes, feel free to keep them if was intended.
> virObjectUnref(cfg);
> return 0;
> }
>@@ -555,6 +579,7 @@ qemuProcessFakeReboot(void *opaque)
> goto endjob;
> }
> priv->gotShutdown = false;
>+ priv->gotReset = false;
> event = virDomainEventLifecycleNewFromObj(vm,
> VIR_DOMAIN_EVENT_RESUMED,
> VIR_DOMAIN_EVENT_RESUMED_UNPAUSED);
>@@ -5320,6 +5345,7 @@ qemuProcessPrepareDomain(virConnectPtr conn,
> priv->monError = false;
> priv->monStart = 0;
> priv->gotShutdown = false;
>+ priv->gotReset = false;
>
> VIR_DEBUG("Updating guest CPU definition");
> if (qemuProcessUpdateGuestCPU(vm->def, priv->qemuCaps, caps, flags) < 0)
>--
>2.13.0
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170810/f799e70b/attachment-0001.sig>
More information about the libvir-list
mailing list