[libvirt] [PATCH] qemu: Honour <on_reboot/>
Martin Kletzander
mkletzan at redhat.com
Fri Aug 18 08:47:45 UTC 2017
On Wed, Aug 16, 2017 at 03:55:19PM +0200, Michal Privoznik wrote:
>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 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.
>
>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
>
I vote for both. Just fire whatever action on any (i.e. the first)
reset event. I don't think there was any logic before the support for
this got "lost". Frankly, I haven't checked the old code.
If you don't want to do any action on virsh reset, just set the gotReset
in the API and reset it when we get the event from the guest with
guest_initiated=yes (or missing).
>>
>>> + 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.
>
ok
>Michal
-------------- 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/20170818/71f6ac3b/attachment-0001.sig>
More information about the libvir-list
mailing list