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

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



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

Attachment: signature.asc
Description: Digital signature


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