[libvirt] [PATCH 1/2] libxl: add missing cleanup on error path in libxlDomainPMWakeup
Since domain was suspended before and on failed wakeup is destroyed, send an event. Also, add missing libxlDomainCleanup. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/libxl/libxl_driver.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 5aa68a7c43..eb719345e8 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1527,6 +1527,9 @@ libxlDomainPMWakeup(virDomainPtr dom, unsigned int flags) libxlDomainDestroyInternal(driver, vm); vm->def->id = -1; virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_FAILED); + event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, + VIR_DOMAIN_EVENT_STOPPED_FAILED); + libxlDomainCleanup(driver, vm); endjob: libxlDomainObjEndJob(driver, vm); -- 2.17.2
If domain is killed with `xl destroy`, libvirt will not notice it and still report the domain as running. Also trying to destroy the domain through libvirt will fail. The only way to recover from such a situation is to restart libvirt daemon. The problem is that even though libxl report LIBXL_EVENT_TYPE_DOMAIN_DEATH, libvirt ignore it as all the domain cleanup is done in a function actually destroying the domain. If destroy is done outside of libvirt, there is no place where it would be handled. Fix this by doing domain cleanup in LIBXL_EVENT_TYPE_DOMAIN_DEATH too. To avoid doing it twice, add a ignoreDeathEvent flag libxlDomainObjPrivate, set when the domain death is triggered by libvirt itself. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/libxl/libxl_domain.c | 71 ++++++++++++++++++++++++++++++++++++++-- src/libxl/libxl_domain.h | 3 ++ 2 files changed, 71 insertions(+), 3 deletions(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 5fe3f44fbe..6d1e15b14c 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -609,6 +609,54 @@ libxlDomainShutdownThread(void *opaque) virObjectUnref(cfg); } +static void +libxlDomainDeathThread(void *opaque) +{ + struct libxlShutdownThreadInfo *shutdown_info = opaque; + virDomainObjPtr vm = NULL; + libxl_event *ev = shutdown_info->event; + libxlDriverPrivatePtr driver = shutdown_info->driver; + virObjectEventPtr dom_event = NULL; + libxlDriverConfigPtr cfg; + libxlDomainObjPrivatePtr priv; + + cfg = libxlDriverConfigGet(driver); + + vm = virDomainObjListFindByID(driver->domains, ev->domid); + if (!vm) { + /* vm->def->id already cleared, means the death was handled by the + * driver already */ + goto cleanup; + } + + priv = vm->privateData; + + if (priv->ignoreDeathEvent) { + priv->ignoreDeathEvent = false; + goto cleanup; + } + + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup; + + virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_DESTROYED); + dom_event = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_STOPPED, + VIR_DOMAIN_EVENT_STOPPED_DESTROYED); + libxlDomainCleanup(driver, vm); + if (!vm->persistent) + virDomainObjListRemove(driver->domains, vm); + libxlDomainObjEndJob(driver, vm); + + cleanup: + virDomainObjEndAPI(&vm); + virObjectEventStateQueue(driver->domainEventState, dom_event); + libxl_event_free(cfg->ctx, ev); + VIR_FREE(shutdown_info); + virObjectUnref(cfg); +} + + /* * Handle previously registered domain event notification from libxenlight. */ @@ -619,8 +667,10 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event) struct libxlShutdownThreadInfo *shutdown_info = NULL; virThread thread; libxlDriverConfigPtr cfg; + int ret = -1; - if (event->type != LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN) { + if (event->type != LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN && + event->type != LIBXL_EVENT_TYPE_DOMAIN_DEATH) { VIR_INFO("Unhandled event type %d", event->type); goto error; } @@ -634,8 +684,14 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event) shutdown_info->driver = driver; shutdown_info->event = (libxl_event *)event; - if (virThreadCreate(&thread, false, libxlDomainShutdownThread, - shutdown_info) < 0) { + if (event->type == LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN) + ret = virThreadCreate(&thread, false, libxlDomainShutdownThread, + shutdown_info); + else if (event->type == LIBXL_EVENT_TYPE_DOMAIN_DEATH) + ret = virThreadCreate(&thread, false, libxlDomainDeathThread, + shutdown_info); + + if (ret < 0) { /* * Not much we can do on error here except log it. */ @@ -751,14 +807,21 @@ libxlDomainDestroyInternal(libxlDriverPrivatePtr driver, virDomainObjPtr vm) { libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); + libxlDomainObjPrivatePtr priv = vm->privateData; int ret = -1; + /* Ignore next LIBXL_EVENT_TYPE_DOMAIN_DEATH as the caller will handle + * domain death appropriately already (having more info, like the reason). + */ + priv->ignoreDeathEvent = true; /* Unlock virDomainObj during destroy, which can take considerable * time on large memory domains. */ virObjectUnlock(vm); ret = libxl_domain_destroy(cfg->ctx, vm->def->id, NULL); virObjectLock(vm); + if (ret) + priv->ignoreDeathEvent = false; virObjectUnref(cfg); return ret; @@ -811,6 +874,8 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, priv->deathW = NULL; } + priv->ignoreDeathEvent = false; + if (virAtomicIntDecAndTest(&driver->nactive) && driver->inhibitCallback) driver->inhibitCallback(false, driver->inhibitOpaque); diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h index e193881450..993fd18f30 100644 --- a/src/libxl/libxl_domain.h +++ b/src/libxl/libxl_domain.h @@ -65,6 +65,9 @@ struct _libxlDomainObjPrivate { /* console */ virChrdevsPtr devs; libxl_evgen_domain_death *deathW; + /* the upcoming LIBXL_EVENT_TYPE_DOMAIN_DEATH is caused by libvirt and + * should not be handled separately */ + bool ignoreDeathEvent; virThreadPtr migrationDstReceiveThr; unsigned short migrationPort; char *lockState; -- 2.17.2
On 12/7/18 7:46 PM, Marek Marczykowski-Górecki wrote:
If domain is killed with `xl destroy`, libvirt will not notice it and still report the domain as running. Also trying to destroy the domain through libvirt will fail. The only way to recover from such a situation is to restart libvirt daemon. The problem is that even though libxl report LIBXL_EVENT_TYPE_DOMAIN_DEATH, libvirt ignore it as all the domain cleanup is done in a function actually destroying the domain. If destroy is done outside of libvirt, there is no place where it would be handled.
Fix this by doing domain cleanup in LIBXL_EVENT_TYPE_DOMAIN_DEATH too. To avoid doing it twice, add a ignoreDeathEvent flag libxlDomainObjPrivate, set when the domain death is triggered by libvirt itself.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/libxl/libxl_domain.c | 71 ++++++++++++++++++++++++++++++++++++++-- src/libxl/libxl_domain.h | 3 ++ 2 files changed, 71 insertions(+), 3 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 5fe3f44fbe..6d1e15b14c 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -609,6 +609,54 @@ libxlDomainShutdownThread(void *opaque) virObjectUnref(cfg); }
+static void +libxlDomainDeathThread(void *opaque) +{ + struct libxlShutdownThreadInfo *shutdown_info = opaque; + virDomainObjPtr vm = NULL; + libxl_event *ev = shutdown_info->event; + libxlDriverPrivatePtr driver = shutdown_info->driver; + virObjectEventPtr dom_event = NULL; + libxlDriverConfigPtr cfg; + libxlDomainObjPrivatePtr priv; + + cfg = libxlDriverConfigGet(driver); + + vm = virDomainObjListFindByID(driver->domains, ev->domid); + if (!vm) { + /* vm->def->id already cleared, means the death was handled by the + * driver already */ + goto cleanup; + } + + priv = vm->privateData; + + if (priv->ignoreDeathEvent) { + priv->ignoreDeathEvent = false; + goto cleanup; + } + + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup; + + virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_DESTROYED); + dom_event = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_STOPPED, + VIR_DOMAIN_EVENT_STOPPED_DESTROYED); + libxlDomainCleanup(driver, vm); + if (!vm->persistent) + virDomainObjListRemove(driver->domains, vm); + libxlDomainObjEndJob(driver, vm); + + cleanup: + virDomainObjEndAPI(&vm); + virObjectEventStateQueue(driver->domainEventState, dom_event); + libxl_event_free(cfg->ctx, ev); + VIR_FREE(shutdown_info); + virObjectUnref(cfg); +}
Thanks for adding death handling in a new function. libxlDomainShutdownThread() is getting a bit unwieldy.
+ + /* * Handle previously registered domain event notification from libxenlight. */ @@ -619,8 +667,10 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event) struct libxlShutdownThreadInfo *shutdown_info = NULL; virThread thread; libxlDriverConfigPtr cfg; + int ret = -1;
- if (event->type != LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN) { + if (event->type != LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN && + event->type != LIBXL_EVENT_TYPE_DOMAIN_DEATH) { VIR_INFO("Unhandled event type %d", event->type); goto error; } @@ -634,8 +684,14 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event)
shutdown_info->driver = driver; shutdown_info->event = (libxl_event *)event; - if (virThreadCreate(&thread, false, libxlDomainShutdownThread, - shutdown_info) < 0) { + if (event->type == LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN) + ret = virThreadCreate(&thread, false, libxlDomainShutdownThread, + shutdown_info); + else if (event->type == LIBXL_EVENT_TYPE_DOMAIN_DEATH) + ret = virThreadCreate(&thread, false, libxlDomainDeathThread, + shutdown_info); + + if (ret < 0) { /* * Not much we can do on error here except log it. */ @@ -751,14 +807,21 @@ libxlDomainDestroyInternal(libxlDriverPrivatePtr driver, virDomainObjPtr vm) { libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); + libxlDomainObjPrivatePtr priv = vm->privateData; int ret = -1;
+ /* Ignore next LIBXL_EVENT_TYPE_DOMAIN_DEATH as the caller will handle + * domain death appropriately already (having more info, like the reason). + */ + priv->ignoreDeathEvent = true; /* Unlock virDomainObj during destroy, which can take considerable * time on large memory domains. */ virObjectUnlock(vm); ret = libxl_domain_destroy(cfg->ctx, vm->def->id, NULL); virObjectLock(vm); + if (ret) + priv->ignoreDeathEvent = false;
virObjectUnref(cfg); return ret; @@ -811,6 +874,8 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, priv->deathW = NULL; }
+ priv->ignoreDeathEvent = false; + if (virAtomicIntDecAndTest(&driver->nactive) && driver->inhibitCallback) driver->inhibitCallback(false, driver->inhibitOpaque);
diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h index e193881450..993fd18f30 100644 --- a/src/libxl/libxl_domain.h +++ b/src/libxl/libxl_domain.h @@ -65,6 +65,9 @@ struct _libxlDomainObjPrivate { /* console */ virChrdevsPtr devs; libxl_evgen_domain_death *deathW; + /* the upcoming LIBXL_EVENT_TYPE_DOMAIN_DEATH is caused by libvirt and + * should not be handled separately */
I tweaked this comment a tiny bit by prefixing the sentence with "Flag to indicate ".
+ bool ignoreDeathEvent; virThreadPtr migrationDstReceiveThr; unsigned short migrationPort; char *lockState;
Reviewed-by: Jim Fehlig <jfehlig@suse.com> I've pushed both patches. Thanks! Regards, Jim
On 12/7/18 7:45 PM, Marek Marczykowski-Górecki wrote:
Since domain was suspended before and on failed wakeup is destroyed, send an event. Also, add missing libxlDomainCleanup.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/libxl/libxl_driver.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 5aa68a7c43..eb719345e8 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1527,6 +1527,9 @@ libxlDomainPMWakeup(virDomainPtr dom, unsigned int flags) libxlDomainDestroyInternal(driver, vm); vm->def->id = -1; virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_FAILED); + event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, + VIR_DOMAIN_EVENT_STOPPED_FAILED); + libxlDomainCleanup(driver, vm);
endjob: libxlDomainObjEndJob(driver, vm);
Reviewed-by: Jim Fehlig <jfehlig@suse.com> Regards, Jim
participants (2)
-
Jim Fehlig -
Marek Marczykowski-Górecki