[libvirt] [PATCH] qemu: Honour <on_reboot/>
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. */ + 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; + } + 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 && + vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_DESTROY) + qemuProcessShutdownOrReboot(driver, vm); + virObjectUnlock(vm); - qemuDomainEventQueue(driver, event); - virObjectUnref(cfg); return 0; } @@ -555,6 +579,7 @@ qemuProcessFakeReboot(void *opaque) goto endjob; } priv->gotShutdown = false; + priv->gotReset = false; event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_RESUMED, VIR_DOMAIN_EVENT_RESUMED_UNPAUSED); @@ -5320,6 +5345,7 @@ qemuProcessPrepareDomain(virConnectPtr conn, priv->monError = false; priv->monStart = 0; priv->gotShutdown = false; + priv->gotReset = false; VIR_DEBUG("Updating guest CPU definition"); if (qemuProcessUpdateGuestCPU(vm->def, priv->qemuCaps, caps, flags) < 0) -- 2.13.0
Hello, Am 03.08.2017 um 09:36 schrieb Michal Privoznik:
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.
White you are at "QEMU reset": From Xen I remember that on reboot a new qemu-dm (Device Model) is created - if I remember correctly - for both PV and HV. For QEMU the old qemu process is reused and the reset is done by SeaBios inside the VM. If would be cool if there was an option to kill the old qemu process and start a new qemu process (with an updated configuration) on reboot. I sometimes have the situation where the libvirt part is done by one group of admins, while the guest OS and everything within in VM is done by some other group of persons. Currently they always have to coordinate a time, where the internal group does initiate the guest OS shutdown and the libvirt admins then updates the configuration and starts the VM again. It would be nice if I could update the config "just now" and then tell the OS group "just do the reboot when your schedule permits it - you will then get your updates configuration automatically." Or is this already there and I missed it? Philipp
On 08/03/2017 06:38 PM, Philipp Hahn wrote:
Hello,
Am 03.08.2017 um 09:36 schrieb Michal Privoznik:
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.
White you are at "QEMU reset": From Xen I remember that on reboot a new qemu-dm (Device Model) is created - if I remember correctly - for both PV and HV. For QEMU the old qemu process is reused and the reset is done by SeaBios inside the VM. If would be cool if there was an option to kill the old qemu process and start a new qemu process (with an updated configuration) on reboot. I sometimes have the situation where the libvirt part is done by one group of admins, while the guest OS and everything within in VM is done by some other group of persons. Currently they always have to coordinate a time, where the internal group does initiate the guest OS shutdown and the libvirt admins then updates the configuration and starts the VM again. It would be nice if I could update the config "just now" and then tell the OS group "just do the reboot when your schedule permits it - you will then get your updates configuration automatically."
Or is this already there and I missed it?
I think this patch enables exactly that. The VM admins don't start the domains by hand but probably have some SW that starts configured domains whenever not running. E.g. if one domain crashes, the mgmt SW starts it up again. With such SW in place this patch is exactly what's been missing. Alternatively, we can introduce new <on_reboot/> target, say "reinit" that would kill the qemu process and start a new one instead. Michal
Hi Michal, Replying to an old thread: On Fri, Aug 4, 2017 at 9:55 AM, Michal Privoznik <mprivozn@redhat.com> wrote:
I think this patch enables exactly that. The VM admins don't start the domains by hand but probably have some SW that starts configured domains whenever not running. E.g. if one domain crashes, the mgmt SW starts it up again. With such SW in place this patch is exactly what's been missing.
Alternatively, we can introduce new <on_reboot/> target, say "reinit" that would kill the qemu process and start a new one instead.
I could also really use something like that. I have no control about when my customers reboot, but when they do, for a kernel update for instance, that's the perfect moment for the vm to restart with new configuration. Right now I have an external process with an event loop that looks for the reboot event for all domains, combined with <on_reboot>destroy</on_reboot>. That works, but I prefer if libvirt would handle this.
Michal
Kind regards, Ruben Kerkhof
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. 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.
virObjectUnref(cfg); return 0; } @@ -555,6 +579,7 @@ qemuProcessFakeReboot(void *opaque) goto endjob; } priv->gotShutdown = false; + priv->gotReset = false; event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_RESUMED, VIR_DOMAIN_EVENT_RESUMED_UNPAUSED); @@ -5320,6 +5345,7 @@ qemuProcessPrepareDomain(virConnectPtr conn, priv->monError = false; priv->monStart = 0; priv->gotShutdown = false; + priv->gotReset = false;
VIR_DEBUG("Updating guest CPU definition"); if (qemuProcessUpdateGuestCPU(vm->def, priv->qemuCaps, caps, flags) < 0) -- 2.13.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
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
participants (4)
-
Martin Kletzander -
Michal Privoznik -
Philipp Hahn -
Ruben Kerkhof