Jim Fehlig wrote:

The libxl driver was ignoring the <on_*> domain event configuration, causing e.g. a domain to be rebooted even when on_reboot is set to destroy.

This patch honors the <on_*> configuration in the shutdown event handler.

Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 83 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 57 insertions(+), 26 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index c7c7b4f..721577d 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -372,38 +372,69 @@ libxlDomainShutdownThread(void *opaque)

virObjectLock(vm);

- switch (xl_reason) { - case LIBXL_SHUTDOWN_REASON_POWEROFF: - case LIBXL_SHUTDOWN_REASON_CRASH: - if (xl_reason == LIBXL_SHUTDOWN_REASON_CRASH) { - dom_event = virDomainEventLifecycleNewFromObj(vm, + if (xl_reason == LIBXL_SHUTDOWN_REASON_POWEROFF) { + dom_event = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_STOPPED, + VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN); + switch (vm->def->onPoweroff) { + case VIR_DOMAIN_LIFECYCLE_DESTROY: + reason = VIR_DOMAIN_SHUTOFF_SHUTDOWN; + goto destroy; + case VIR_DOMAIN_LIFECYCLE_RESTART: + case VIR_DOMAIN_LIFECYCLE_RESTART_RENAME: + goto restart; + case VIR_DOMAIN_LIFECYCLE_PRESERVE: + goto cleanup; + } + } else if (xl_reason == LIBXL_SHUTDOWN_REASON_CRASH) { + dom_event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_CRASHED); - reason = VIR_DOMAIN_SHUTOFF_CRASHED; - } else { - dom_event = virDomainEventLifecycleNewFromObj(vm, + switch (vm->def->onCrash) { + case VIR_DOMAIN_LIFECYCLE_CRASH_DESTROY: + reason = VIR_DOMAIN_SHUTOFF_CRASHED; + goto destroy; + case VIR_DOMAIN_LIFECYCLE_CRASH_RESTART: + case VIR_DOMAIN_LIFECYCLE_CRASH_RESTART_RENAME: + goto restart; + case VIR_DOMAIN_LIFECYCLE_CRASH_PRESERVE: + goto cleanup; + } + } else if (xl_reason == LIBXL_SHUTDOWN_REASON_REBOOT) { + dom_event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN); - reason = VIR_DOMAIN_SHUTOFF_SHUTDOWN; - } - libxl_domain_destroy(ctx, vm->def->id, NULL); - if (libxlVmCleanupJob(driver, vm, reason)) { - if (!vm->persistent) { - virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } - } - break; - case LIBXL_SHUTDOWN_REASON_REBOOT: - libxl_domain_destroy(ctx, vm->def->id, NULL); - libxlVmCleanupJob(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN); - libxlVmStart(driver, vm, 0, -1); - break; - default: - VIR_INFO("Unhandled shutdown_reason %d", xl_reason); - break; + switch (vm->def->onReboot) { + case VIR_DOMAIN_LIFECYCLE_DESTROY: + reason = VIR_DOMAIN_SHUTOFF_SHUTDOWN; + goto destroy; + case VIR_DOMAIN_LIFECYCLE_RESTART: + case VIR_DOMAIN_LIFECYCLE_RESTART_RENAME: + goto restart; + case VIR_DOMAIN_LIFECYCLE_PRESERVE: + goto cleanup; + } + } else { + VIR_INFO("Unhandled shutdown_reason %d", xl_reason); + goto cleanup; }

+destroy: + libxl_domain_destroy(ctx, vm->def->id, NULL); + if (libxlVmCleanupJob(driver, vm, reason)) { + if (!vm->persistent) { + virDomainObjListRemove(driver->domains, vm); + vm = NULL; + } + } + goto cleanup; + +restart: + libxl_domain_destroy(ctx, vm->def->id, NULL); + libxlVmCleanup(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN);

Opps, botched rebasing of my own branches. This should be libxlVmCleanupJob. I've fixed it in my local branch. Regards, Jim

+ libxlVmStart(driver, vm, 0, -1); + +cleanup: if (vm) virObjectUnlock(vm); if (dom_event)

Michal Privoznik wrote:

On 21.02.2014 00:02, Jim Fehlig wrote:

...

The shutdown handler may restart a domain when handling a reboot event or when <on_*> is set to 'restart'. Restarting consists of calling libxlVmCleanup followed by libxlVmStart. libxlVmStart will emit a VIR_DOMAIN_EVENT_STARTED event, but the SHUTDOWN event is not emitted until exiting the shutdown handler, after the STARTED event. Queue the event immediately after creation to avoid emitting it after the start event.

Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 721577d..e600de7 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -376,6 +376,8 @@ libxlDomainShutdownThread(void *opaque) dom_event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,

VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN); + if (dom_event) + libxlDomainEventQueue(driver, dom_event); switch (vm->def->onPoweroff) { case VIR_DOMAIN_LIFECYCLE_DESTROY: reason = VIR_DOMAIN_SHUTOFF_SHUTDOWN; @@ -390,6 +392,8 @@ libxlDomainShutdownThread(void *opaque) dom_event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,

VIR_DOMAIN_EVENT_STOPPED_CRASHED); + if (dom_event) + libxlDomainEventQueue(driver, dom_event); switch (vm->def->onCrash) { case VIR_DOMAIN_LIFECYCLE_CRASH_DESTROY: reason = VIR_DOMAIN_SHUTOFF_CRASHED; @@ -404,6 +408,8 @@ libxlDomainShutdownThread(void *opaque) dom_event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,

VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN); + if (dom_event) + libxlDomainEventQueue(driver, dom_event); switch (vm->def->onReboot) { case VIR_DOMAIN_LIFECYCLE_DESTROY: reason = VIR_DOMAIN_SHUTOFF_SHUTDOWN; @@ -437,8 +443,6 @@ restart: cleanup: if (vm) virObjectUnlock(vm); - if (dom_event) - libxlDomainEventQueue(driver, dom_event); libxl_event_free(ctx, ev); VIR_FREE(shutdown_info); }

Wouldn't it be better to enqueue events at the beginning of 'destroy' and 'restart' labels? I'm thinking about something among these lines:

Yes, agreed. As discussed on IRC, better to enqueue the event at the labels since it is one less call to libxlDomainEventQueue, and improves the code to handle any future <on_foo> action. Regards, Jim

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 0b9bf7d..c009407 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -425,6 +425,10 @@ libxlDomainShutdownThread(void *opaque) }

destroy: + if (dom_event) { + libxlDomainEventQueue(driver, dom_event); + dom_event = NULL; + } libxl_domain_destroy(ctx, vm->def->id, NULL); if (libxlVmCleanupJob(driver, vm, reason)) { if (!vm->persistent) { @@ -435,6 +439,10 @@ destroy: goto cleanup;

restart: + if (dom_event) { + libxlDomainEventQueue(driver, dom_event); + dom_event = NULL; + } libxl_domain_destroy(ctx, vm->def->id, NULL); libxlVmCleanupJob(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN); libxlVmStart(driver, vm, 0, -1);

Michal

On 21.02.2014 00:02, Jim Fehlig wrote:

Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---

Without a libxl config file, I suppose the only thing needed here is the #define in libxl_conf.h. But I have a dusty series adding lockd support to the libxl driver, which I'll be cleaning off soon. That series introduces libxl.conf, and can be expanded to include an auto dump dir entry.

src/libxl/libxl_conf.c | 3 +++ src/libxl/libxl_conf.h | 2 ++ src/libxl/libxl_driver.c | 7 +++++++ 3 files changed, 12 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 4cefadf..13eddd1 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -97,6 +97,7 @@ libxlDriverConfigDispose(void *obj) VIR_FREE(cfg->stateDir); VIR_FREE(cfg->libDir); VIR_FREE(cfg->saveDir); + VIR_FREE(cfg->autoDumpDir); }

static int @@ -1078,6 +1079,8 @@ libxlDriverConfigNew(void) goto error; if (VIR_STRDUP(cfg->saveDir, LIBXL_SAVE_DIR) < 0) goto error; + if (VIR_STRDUP(cfg->autoDumpDir, LIBXL_DUMP_DIR) < 0) + goto error;

if (virAsprintf(&log_file, "%s/libxl-driver.log", cfg->logDir) < 0) goto error; diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index ca7bc7d..f089a92 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -48,6 +48,7 @@ # define LIBXL_LOG_DIR LOCALSTATEDIR "/log/libvirt/libxl" # define LIBXL_LIB_DIR LOCALSTATEDIR "/lib/libvirt/libxl" # define LIBXL_SAVE_DIR LIBXL_LIB_DIR "/save" +# define LIBXL_DUMP_DIR LIBXL_LIB_DIR "/dump" # define LIBXL_BOOTLOADER_PATH BINDIR "/pygrub"

@@ -82,6 +83,7 @@ struct _libxlDriverConfig { char *stateDir; char *libDir; char *saveDir; + char *autoDumpDir; };

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index e600de7..e18fead 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -976,6 +976,13 @@ libxlStateInitialize(bool privileged, virStrerror(errno, ebuf, sizeof(ebuf))); goto error; } + if (virFileMakePath(cfg->autoDumpDir) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to create dump dir '%s': %s"), + cfg->autoDumpDir, + virStrerror(errno, ebuf, sizeof(ebuf))); + goto error; + }

/* read the host sysinfo */ libxl_driver->hostsysinfo = virSysinfoRead();

ACK Michal

On 21.02.2014 00:02, Jim Fehlig wrote:

Add support for coredump-{destroy,restart} actions of <on_crash> event.

Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index e18fead..7640801 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -251,6 +251,50 @@ error: }

/* + * Core dump domain to default dump path. + * + * virDomainObjPtr should be locked on invocation + */ +static int +libxlDomainAutoCoreDump(libxlDriverPrivatePtr driver, + virDomainObjPtr vm) +{ + libxlDomainObjPrivatePtr priv = vm->privateData; + libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); + time_t curtime = time(NULL); + char timestr[100]; + struct tm time_info; + char *dumpfile = NULL; + int ret = -1; + + localtime_r(&curtime, &time_info); + strftime(timestr, sizeof(timestr), "%Y-%m-%d-%H:%M:%S", &time_info); + + if (virAsprintf(&dumpfile, "%s/%s-%s", + cfg->autoDumpDir, + vm->def->name, + timestr) < 0) + goto cleanup; + + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup; + + /* Unlock virDomainObj while dumping core */ + virObjectUnlock(vm); + libxl_domain_core_dump(priv->ctx, vm->def->id, dumpfile, NULL); + virObjectLock(vm); + + ignore_value(libxlDomainObjEndJob(driver, vm)); + ret = 0; + +cleanup: + VIR_FREE(dumpfile); + virObjectUnref(cfg); + + return ret; +} + +/* * Cleanup function for domain that has reached shutoff state. * * virDomainObjPtr should be locked on invocation @@ -403,6 +447,12 @@ libxlDomainShutdownThread(void *opaque) goto restart; case VIR_DOMAIN_LIFECYCLE_CRASH_PRESERVE: goto cleanup; + case VIR_DOMAIN_LIFECYCLE_CRASH_COREDUMP_DESTROY: + libxlDomainAutoCoreDump(driver, vm); + goto destroy; + case VIR_DOMAIN_LIFECYCLE_CRASH_COREDUMP_RESTART: + libxlDomainAutoCoreDump(driver, vm); + goto restart; } } else if (xl_reason == LIBXL_SHUTDOWN_REASON_REBOOT) { dom_event = virDomainEventLifecycleNewFromObj(vm,

ACK Michal