Re: [libvirt] [PATCH] qemu: Honour <on_reboot/>

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(a)redhat.com&gt; > --- > 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.