[libvirt] [PATCH] On Xen, domains may be destroyed without libvirt being involved
This can happen via `xl destroy`, for example. When this happens, libvirt is stuck in an inconsistent state: libvirt believes the domain is still running, but attempts to use libvirt’s APIs to shutdown the domain fail. The only way out of this situation is to restart libvirt. To prevent this from happening, process LIBXL_EVENT_TYPE_DOMAIN_DEATH as well as LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN, but only if libvirt has not already begun to destroy the domain. Signed-off-by: Demi Obenour <demiobenour@gmail.com> --- src/conf/domain_conf.h | 4 ++++ src/libxl/libxl_domain.c | 24 +++++++++++++++++++----- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index b24e6ec3de..d3520bde15 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2620,6 +2620,10 @@ struct _virDomainObj { unsigned int updated : 1; unsigned int removing : 1; + /* Only used by the Xen backend */ + unsigned int being_destroyed_by_libvirt : 1; + unsigned int already_destroyed : 1; + virDomainDefPtr def; /* The current definition */ virDomainDefPtr newDef; /* New definition to activate at shutdown */ diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 5fe3f44fbe..680e5f209f 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -482,9 +482,21 @@ libxlDomainShutdownThread(void *opaque) goto cleanup; } + VIR_INFO("Domain %d died", event->domid); + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) goto cleanup; + if (LIBXL_EVENT_TYPE_DOMAIN_DEATH == ev->type) { + if (vm->being_destroyed_by_libvirt) { + VIR_INFO("VM %d already being destroyed by libvirt", event->domid); + goto cleanup; + } + + VIR_INFO("Marking VM %d as already destroyed", event->domid); + vm->already_destroyed = true; + } + if (xl_reason == LIBXL_SHUTDOWN_REASON_POWEROFF) { virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_SHUTDOWN); @@ -620,7 +632,8 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event) virThread thread; libxlDriverConfigPtr cfg; - if (event->type != LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN) { + if (LIBXL_EVENT_TYPE_DOMAIN_DEATH != event->type && + LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN != event->type) { VIR_INFO("Unhandled event type %d", event->type); goto error; } @@ -629,18 +642,16 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event) * Start a thread to handle shutdown. We don't want to be tying up * libxl's event machinery by doing a potentially lengthy shutdown. */ - if (VIR_ALLOC(shutdown_info) < 0) - goto error; + while (VIR_ALLOC(shutdown_info) < 0) {} shutdown_info->driver = driver; shutdown_info->event = (libxl_event *)event; - if (virThreadCreate(&thread, false, libxlDomainShutdownThread, + while (virThreadCreate(&thread, false, libxlDomainShutdownThread, shutdown_info) < 0) { /* * Not much we can do on error here except log it. */ VIR_ERROR(_("Failed to create thread to handle domain shutdown")); - goto error; } /* @@ -752,6 +763,9 @@ libxlDomainDestroyInternal(libxlDriverPrivatePtr driver, { libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); int ret = -1; + if (vm->already_destroyed) + return -1; + vm->being_destroyed_by_libvirt = true; /* Unlock virDomainObj during destroy, which can take considerable * time on large memory domains. -- 2.17.2
On Sun, Dec 09, 2018 at 13:01:27 -0500, Demi M. Obenour wrote:
This can happen via `xl destroy`, for example. When this happens, libvirt is stuck in an inconsistent state: libvirt believes the domain is still running, but attempts to use libvirt’s APIs to shutdown the domain fail. The only way out of this situation is to restart libvirt.
To prevent this from happening, process LIBXL_EVENT_TYPE_DOMAIN_DEATH as well as LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN, but only if libvirt has not already begun to destroy the domain.
Signed-off-by: Demi Obenour <demiobenour@gmail.com> --- src/conf/domain_conf.h | 4 ++++ src/libxl/libxl_domain.c | 24 +++++++++++++++++++----- 2 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index b24e6ec3de..d3520bde15 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2620,6 +2620,10 @@ struct _virDomainObj { unsigned int updated : 1; unsigned int removing : 1; + /* Only used by the Xen backend */ + unsigned int being_destroyed_by_libvirt : 1; + unsigned int already_destroyed : 1;
Please put this into 'struct _libxlDomainObjPrivate' if it's going to be only used by the libxl driver. Also the 'bool' type should be used.
+ virDomainDefPtr def; /* The current definition */ virDomainDefPtr newDef; /* New definition to activate at shutdown */
On Sun, Dec 09, 2018 at 01:01:27PM -0500, Demi M. Obenour wrote:
This can happen via `xl destroy`, for example. When this happens, libvirt is stuck in an inconsistent state: libvirt believes the domain is still running, but attempts to use libvirt’s APIs to shutdown the domain fail. The only way out of this situation is to restart libvirt.
To prevent this from happening, process LIBXL_EVENT_TYPE_DOMAIN_DEATH as well as LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN, but only if libvirt has not already begun to destroy the domain.
Signed-off-by: Demi Obenour <demiobenour@gmail.com> --- src/conf/domain_conf.h | 4 ++++ src/libxl/libxl_domain.c | 24 +++++++++++++++++++----- 2 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index b24e6ec3de..d3520bde15 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2620,6 +2620,10 @@ struct _virDomainObj { unsigned int updated : 1; unsigned int removing : 1; + /* Only used by the Xen backend */ + unsigned int being_destroyed_by_libvirt : 1; + unsigned int already_destroyed : 1;
This ought to go in the _libxlDomainObjPrivate struct instead
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 5fe3f44fbe..680e5f209f 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -482,9 +482,21 @@ libxlDomainShutdownThread(void *opaque) goto cleanup; } + VIR_INFO("Domain %d died", event->domid); + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) goto cleanup; + if (LIBXL_EVENT_TYPE_DOMAIN_DEATH == ev->type) {
Normal style is to have variable name first, constant second
+ if (vm->being_destroyed_by_libvirt) { + VIR_INFO("VM %d already being destroyed by libvirt", event->domid);
The patch got mangled by your mail client i'm afraid.
+ goto cleanup; + } + + VIR_INFO("Marking VM %d as already destroyed", event->domid); + vm->already_destroyed = true;
Using 'true' for an "unsigned int :1' bitfield is not valid. You would nede to declare the variable as a bool, or use an integer value here.
+ } + if (xl_reason == LIBXL_SHUTDOWN_REASON_POWEROFF) { virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_SHUTDOWN); @@ -620,7 +632,8 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event) virThread thread; libxlDriverConfigPtr cfg; - if (event->type != LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN) { + if (LIBXL_EVENT_TYPE_DOMAIN_DEATH != event->type && + LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN != event->type) {
Again, this is contrary to normal libvirt style, so please put it back to variable name first.
VIR_INFO("Unhandled event type %d", event->type); goto error; } @@ -629,18 +642,16 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event) * Start a thread to handle shutdown. We don't want to be tying up * libxl's event machinery by doing a potentially lengthy shutdown. */ - if (VIR_ALLOC(shutdown_info) < 0) - goto error; + while (VIR_ALLOC(shutdown_info) < 0) {}
Huh ? This is busy-waiting when getting OOM which is definitely not what we want.
shutdown_info->driver = driver; shutdown_info->event = (libxl_event *)event; - if (virThreadCreate(&thread, false, libxlDomainShutdownThread, + while (virThreadCreate(&thread, false, libxlDomainShutdownThread, shutdown_info) < 0) {
Again, busy-waiting when failin to spawn threads.
/* * Not much we can do on error here except log it. */ VIR_ERROR(_("Failed to create thread to handle domain shutdown")); - goto error; } /* @@ -752,6 +763,9 @@ libxlDomainDestroyInternal(libxlDriverPrivatePtr driver, { libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); int ret = -1; + if (vm->already_destroyed) + return -1; + vm->being_destroyed_by_libvirt = true;
Again, using a bool "true" with an unsigned int bitfied.
/* Unlock virDomainObj during destroy, which can take considerable * time on large memory domains.
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 12/9/18 11:01 AM, Demi M. Obenour wrote:
This can happen via `xl destroy`, for example. When this happens, libvirt is stuck in an inconsistent state: libvirt believes the domain is still running, but attempts to use libvirt’s APIs to shutdown the domain fail. The only way out of this situation is to restart libvirt.
Marek asked about this last Friday https://www.redhat.com/archives/libvir-list/2018-December/msg00196.html and then spent the day creating a patch :-) https://www.redhat.com/archives/libvir-list/2018-December/msg00212.html I'll review/test his patches today, but thank you for taking a stab at this problem! Regards, Jim
To prevent this from happening, process LIBXL_EVENT_TYPE_DOMAIN_DEATH as well as LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN, but only if libvirt has not already begun to destroy the domain.
Signed-off-by: Demi Obenour <demiobenour@gmail.com> --- src/conf/domain_conf.h | 4 ++++ src/libxl/libxl_domain.c | 24 +++++++++++++++++++----- 2 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index b24e6ec3de..d3520bde15 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2620,6 +2620,10 @@ struct _virDomainObj { unsigned int updated : 1; unsigned int removing : 1;
+ /* Only used by the Xen backend */ + unsigned int being_destroyed_by_libvirt : 1; + unsigned int already_destroyed : 1; + virDomainDefPtr def; /* The current definition */ virDomainDefPtr newDef; /* New definition to activate at shutdown */
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 5fe3f44fbe..680e5f209f 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -482,9 +482,21 @@ libxlDomainShutdownThread(void *opaque) goto cleanup; }
+ VIR_INFO("Domain %d died", event->domid); + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) goto cleanup;
+ if (LIBXL_EVENT_TYPE_DOMAIN_DEATH == ev->type) { + if (vm->being_destroyed_by_libvirt) { + VIR_INFO("VM %d already being destroyed by libvirt", event->domid); + goto cleanup; + } + + VIR_INFO("Marking VM %d as already destroyed", event->domid); + vm->already_destroyed = true; + } + if (xl_reason == LIBXL_SHUTDOWN_REASON_POWEROFF) { virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_SHUTDOWN); @@ -620,7 +632,8 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event) virThread thread; libxlDriverConfigPtr cfg;
- if (event->type != LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN) { + if (LIBXL_EVENT_TYPE_DOMAIN_DEATH != event->type && + LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN != event->type) { VIR_INFO("Unhandled event type %d", event->type); goto error; } @@ -629,18 +642,16 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event) * Start a thread to handle shutdown. We don't want to be tying up * libxl's event machinery by doing a potentially lengthy shutdown. */ - if (VIR_ALLOC(shutdown_info) < 0) - goto error; + while (VIR_ALLOC(shutdown_info) < 0) {}
shutdown_info->driver = driver; shutdown_info->event = (libxl_event *)event; - if (virThreadCreate(&thread, false, libxlDomainShutdownThread, + while (virThreadCreate(&thread, false, libxlDomainShutdownThread, shutdown_info) < 0) { /* * Not much we can do on error here except log it. */ VIR_ERROR(_("Failed to create thread to handle domain shutdown")); - goto error; }
/* @@ -752,6 +763,9 @@ libxlDomainDestroyInternal(libxlDriverPrivatePtr driver, { libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); int ret = -1; + if (vm->already_destroyed) + return -1; + vm->being_destroyed_by_libvirt = true;
/* Unlock virDomainObj during destroy, which can take considerable * time on large memory domains.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (4)
-
Daniel P. Berrangé -
Demi M. Obenour -
Jim Fehlig -
Peter Krempa