[libvirt] [PATCH v4 0/2] Allow domain reboot after core dump
This is the 4th (and hopefully last) version of these patches. diff to v3: -split in 2 patches -and some other Eric's suggestions taken in Michal Privoznik (2): virDomainCoreDump: Introduce VIR_DUMP_RESET flag qemu: Implement VIR_DUMP_RESET include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 15 ++++++++++++++- src/qemu/qemu_driver.c | 9 ++++++++- tools/virsh.c | 3 +++ tools/virsh.pod | 4 +++- 5 files changed, 29 insertions(+), 3 deletions(-) -- 1.7.3.4
This flag is intended to allow user to do so called system reset after dump, instead of sending ACPI reboot event. --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 15 ++++++++++++++- tools/virsh.c | 3 +++ tools/virsh.pod | 4 +++- 4 files changed, 21 insertions(+), 2 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 39155a6..fc82e13 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -748,6 +748,7 @@ typedef enum { VIR_DUMP_CRASH = (1 << 0), /* crash after dump */ VIR_DUMP_LIVE = (1 << 1), /* live dump */ VIR_DUMP_BYPASS_CACHE = (1 << 2), /* avoid file system cache pollution */ + VIR_DUMP_RESET = (1 << 3), /* reset domain after dump finishes */ } virDomainCoreDumpFlags; /* Domain migration flags. */ diff --git a/src/libvirt.c b/src/libvirt.c index 8f94b11..f8be566 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -2784,7 +2784,8 @@ error: * a crashed state after the dump completes. If @flags includes * VIR_DUMP_LIVE, then make the core dump while continuing to allow * the guest to run; otherwise, the guest is suspended during the dump. - * The above two flags are mutually exclusive. + * VIR_DUMP_RESET flag forces reset of the quest after dump. + * The above three flags are mutually exclusive. * * Additionally, if @flags includes VIR_DUMP_BYPASS_CACHE, then libvirt * will attempt to bypass the file system cache while creating the file, @@ -2823,6 +2824,18 @@ virDomainCoreDump(virDomainPtr domain, const char *to, unsigned int flags) goto error; } + if ((flags & VIR_DUMP_CRASH) && (flags & VIR_DUMP_RESET)) { + virLibDomainError(VIR_ERR_INVALID_ARG, + _("crash and reset flags are mutually exclusive")); + goto error; + } + + if ((flags & VIR_DUMP_LIVE) && (flags & VIR_DUMP_RESET)) { + virLibDomainError(VIR_ERR_INVALID_ARG, + _("live and reset flags are mutually exclusive")); + goto error; + } + if (conn->driver->domainCoreDump) { int ret; char *absolute_to; diff --git a/tools/virsh.c b/tools/virsh.c index 7b0533d..b860d1a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2899,6 +2899,7 @@ static const vshCmdOptDef opts_dump[] = { {"crash", VSH_OT_BOOL, 0, N_("crash the domain after core dump")}, {"bypass-cache", VSH_OT_BOOL, 0, N_("avoid file system cache when saving")}, + {"reset", VSH_OT_BOOL, 0, N_("reset the domain after core dump")}, {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("where to dump the core")}, {NULL, 0, 0, NULL} @@ -2928,6 +2929,8 @@ cmdDump(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DUMP_CRASH; if (vshCommandOptBool(cmd, "bypass-cache")) flags |= VIR_DUMP_BYPASS_CACHE; + if (vshCommandOptBool(cmd, "reset")) + flags |= VIR_DUMP_RESET; if (virDomainCoreDump(dom, to, flags) < 0) { vshError(ctl, _("Failed to core dump domain %s to %s"), name, to); diff --git a/tools/virsh.pod b/tools/virsh.pod index 43ed1ea..fc735f7 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -629,13 +629,15 @@ Convert the file I<xml> in domain XML format to the native guest configuration format named by I<format>. =item B<dump> I<domain-id> I<corefilepath> [I<--live>] [I<--crash>] -[I<--bypass-cache>] +[I<--bypass-cache>] [I<--reset>] Dumps the core of a domain to a file for analysis. If I<--live> is specified, the domain continues to run until the core dump is complete, rather than pausing up front. If I<--crash> is specified, the domain is halted with a crashed status, rather than merely left in a paused state. +If I<--reset> is specified, the domain is reset after successful dump. +Note, these three switches are mutually exclusive. If I<--bypass-cache> is specified, the save will avoid the file system cache, although this may slow down the operation. -- 1.7.3.4
On 09/26/2011 10:19 AM, Michal Privoznik wrote:
This flag is intended to allow user to do so called system reset after dump, instead of sending ACPI reboot event. --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 15 ++++++++++++++- tools/virsh.c | 3 +++ tools/virsh.pod | 4 +++- 4 files changed, 21 insertions(+), 2 deletions(-)
ACK with one nit:
+++ b/tools/virsh.pod @@ -629,13 +629,15 @@ Convert the file I<xml> in domain XML format to the native guest configuration format named by I<format>.
=item B<dump> I<domain-id> I<corefilepath> [I<--live>] [I<--crash>] -[I<--bypass-cache>] +[I<--bypass-cache>] [I<--reset>]
Let's expose the mutual exclusion in our man page, by using the {a|b} notation like we do on other commands: =item B<dump> I<domain-id> I<corefilepath> [I<--bypass-cache>] {[I<--live>] | [I<--crash>] | [I<--reset>]} -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
This patch extends qemudDomainCoreDump so it supports new VIR_DUMP_RESET flag. If this flag is set, domain is reset on successful dump. However, this is needed to be done after we start CPUs. --- src/qemu/qemu_driver.c | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0d0bea2..6199db5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2866,11 +2866,13 @@ static int qemudDomainCoreDump(virDomainPtr dom, { struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm; + qemuDomainObjPrivatePtr priv; int resume = 0, paused = 0; int ret = -1; virDomainEventPtr event = NULL; - virCheckFlags(VIR_DUMP_LIVE | VIR_DUMP_CRASH | VIR_DUMP_BYPASS_CACHE, -1); + virCheckFlags(VIR_DUMP_LIVE | VIR_DUMP_CRASH | + VIR_DUMP_BYPASS_CACHE | VIR_DUMP_RESET, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -2938,6 +2940,11 @@ endjob: if (virGetLastError() == NULL) qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("resuming after dump failed")); + } else if ((ret == 0) && (flags & VIR_DUMP_RESET)) { + priv = vm->privateData; + qemuDomainObjEnterMonitorWithDriver(driver, vm); + ret = qemuMonitorSystemReset(priv->mon); + qemuDomainObjExitMonitorWithDriver(driver, vm); } } -- 1.7.3.4
On 09/26/2011 10:19 AM, Michal Privoznik wrote:
This patch extends qemudDomainCoreDump so it supports new VIR_DUMP_RESET flag. If this flag is set, domain is reset on successful dump. However, this is needed to be done after we start CPUs. --- src/qemu/qemu_driver.c | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0d0bea2..6199db5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2866,11 +2866,13 @@ static int qemudDomainCoreDump(virDomainPtr dom, { struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm; + qemuDomainObjPrivatePtr priv; int resume = 0, paused = 0; int ret = -1; virDomainEventPtr event = NULL;
- virCheckFlags(VIR_DUMP_LIVE | VIR_DUMP_CRASH | VIR_DUMP_BYPASS_CACHE, -1); + virCheckFlags(VIR_DUMP_LIVE | VIR_DUMP_CRASH | + VIR_DUMP_BYPASS_CACHE | VIR_DUMP_RESET, -1);
This part is okay, but...
qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -2938,6 +2940,11 @@ endjob: if (virGetLastError() == NULL) qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("resuming after dump failed")); + } else if ((ret == 0)&& (flags& VIR_DUMP_RESET)) { + priv = vm->privateData; + qemuDomainObjEnterMonitorWithDriver(driver, vm); + ret = qemuMonitorSystemReset(priv->mon); + qemuDomainObjExitMonitorWithDriver(driver, vm); } }
Hmm. This is inside a hunk that says if (resume && paused && active). But we want reset to work even if we don't need to resume the guest (that is, the guest was already paused before we triggered the core dump api). Also, I don't see why qemuMonitorSystemReset has to wait for CPUs to be running. Shouldn't the logic instead be: else if (((resume && paused) || (flags & VIR_DUMP_RESET)) && virDomainObjIsActive(vm)) { if (flags & VIR_DUMP_RESET) ... qemuMonitorSystemReset if (resume) ... qemuProcessStartCPUs } -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
This patch extends qemudDomainCoreDump so it supports new VIR_DUMP_RESET flag. If this flag is set, domain is reset on successful dump. However, this is needed to be done after we start CPUs. --- src/qemu/qemu_driver.c | 20 +++++++++++++++----- 1 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4ea3236..5fadc1d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2919,11 +2919,13 @@ static int qemudDomainCoreDump(virDomainPtr dom, { struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm; + qemuDomainObjPrivatePtr priv; int resume = 0, paused = 0; int ret = -1; virDomainEventPtr event = NULL; - virCheckFlags(VIR_DUMP_LIVE | VIR_DUMP_CRASH | VIR_DUMP_BYPASS_CACHE, -1); + virCheckFlags(VIR_DUMP_LIVE | VIR_DUMP_CRASH | + VIR_DUMP_BYPASS_CACHE | VIR_DUMP_RESET, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -2984,10 +2986,18 @@ endjob: /* Since the monitor is always attached to a pty for libvirt, it will support synchronous operations so we always get here after the migration is complete. */ - else if (resume && paused && virDomainObjIsActive(vm)) { - if (qemuProcessStartCPUs(driver, vm, dom->conn, - VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_DUMP) < 0) { + else if (((resume && paused) || (flags & VIR_DUMP_RESET)) && + virDomainObjIsActive(vm)) { + if ((ret == 0) && (flags & VIR_DUMP_RESET)) { + priv = vm->privateData; + qemuDomainObjEnterMonitorWithDriver(driver, vm); + ret = qemuMonitorSystemReset(priv->mon); + qemuDomainObjExitMonitorWithDriver(driver, vm); + } + + if (resume && qemuProcessStartCPUs(driver, vm, dom->conn, + VIR_DOMAIN_RUNNING_UNPAUSED, + QEMU_ASYNC_JOB_DUMP) < 0) { if (virGetLastError() == NULL) qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("resuming after dump failed")); -- 1.7.3.4
On 10/11/2011 06:56 AM, Michal Privoznik wrote:
This patch extends qemudDomainCoreDump so it supports new VIR_DUMP_RESET flag. If this flag is set, domain is reset on successful dump. However, this is needed to be done after we start CPUs. --- src/qemu/qemu_driver.c | 20 +++++++++++++++----- 1 files changed, 15 insertions(+), 5 deletions(-)
ACK - you've covered all my review comments. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 12.10.2011 20:23, Eric Blake wrote:
On 10/11/2011 06:56 AM, Michal Privoznik wrote:
This patch extends qemudDomainCoreDump so it supports new VIR_DUMP_RESET flag. If this flag is set, domain is reset on successful dump. However, this is needed to be done after we start CPUs. --- src/qemu/qemu_driver.c | 20 +++++++++++++++----- 1 files changed, 15 insertions(+), 5 deletions(-)
ACK - you've covered all my review comments.
Thanks, pushed.
On 26.09.2011 18:19, Michal Privoznik wrote:
This is the 4th (and hopefully last) version of these patches.
diff to v3: -split in 2 patches -and some other Eric's suggestions taken in
Michal Privoznik (2): virDomainCoreDump: Introduce VIR_DUMP_RESET flag qemu: Implement VIR_DUMP_RESET
include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 15 ++++++++++++++- src/qemu/qemu_driver.c | 9 ++++++++- tools/virsh.c | 3 +++ tools/virsh.pod | 4 +++- 5 files changed, 29 insertions(+), 3 deletions(-)
Ping?
participants (2)
-
Eric Blake -
Michal Privoznik