[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 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

> 
>> +        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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]