[libvirt] [PATCH 2/2] libxl: handle external domain destroy

[Jim Fehlig]

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 at 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 at suse.com>

I've pushed both patches. Thanks!

Regards,
Jim