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

On 08/10/2017 04:02 PM, Martin Kletzander wrote: > 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. I think it's Seabios that issues the second reset. Therefore I don't think it's a bug. But the truth is I completely forgot about UEFI guests. > > 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 ... True, 'reboot' ran from within the guest sets callOnReboot; 'virsh reset' does not. > >> + } >> + >> 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. Therefore, just 'reboot' has an option to fire the action. But since I completely forgot about UEFI, maybe we don't want this logic after all. I mean, how can we safely assume that whatever BIOS guest uses is going to issue the reset event? We can not! So this logic *is* flawed after all but for a different reason. So I guess the only thing we can do here is: a) throw this logic away, b) call whatever action configured

> >> + vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_DESTROY) >> + qemuProcessShutdownOrReboot(driver, vm); >> + > > You should also check VIR_DOMAIN_LIFECYCLE_PRESERVE according to the > documentation: Shoot! You're right. > > ... 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. Yeah, I'd like to keep them in. It's unnecessary to have them in a separate patch since they are trivial and I'm touching the area anyway.

Michal