[libvirt] [PATCH v3] qemu: Allow domain reboot after core dump
This patch introduces possibility to reboot domain after core dump finishes. The new flag VIR_DUMP_REBOOT was added to virDomainCoreDumpFlags. The new functionality is accessible via virsh too: virsh dump --reboot <domain> --- diff to v2: -issue reset command instead of qemuDomainReboot include/libvirt/libvirt.h.in | 1 + src/qemu/qemu_driver.c | 54 +++++++++++++++++++++++++++++++++++++++-- tools/virsh.c | 3 ++ 3 files changed, 55 insertions(+), 3 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 39155a6..8c41f5a 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_REBOOT = (1 << 3), /* reboot domain after dump finishes */ } virDomainCoreDumpFlags; /* Domain migration flags. */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0d0bea2..3e05e14 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2860,6 +2860,47 @@ getCompressionType(struct qemud_driver *driver) return compress; } +static int +doSystemReset(struct qemud_driver *driver, + virDomainObjPtr *vm) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv; + + /* Okay, this should never happen, but doesn't hurt to check. */ + if (!driver || !vm || !*vm) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid argument supplied")); + goto cleanup; + } + + priv = (*vm)->privateData; + + if (qemuDomainObjBeginJob(driver, *vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (!virDomainObjIsActive(*vm)) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + goto endjob; + } + + qemuDomainObjEnterMonitorWithDriver(driver, *vm); + if (qemuMonitorSystemReset(priv->mon) < 0) { + qemuDomainObjExitMonitorWithDriver(driver, *vm); + goto endjob; + } + qemuDomainObjExitMonitorWithDriver(driver, *vm); + + ret = 0; + +endjob: + if (qemuDomainObjEndJob(driver, *vm) == 0) + *vm = NULL; +cleanup: + return ret; +} + static int qemudDomainCoreDump(virDomainPtr dom, const char *path, unsigned int flags) @@ -2870,7 +2911,8 @@ static int qemudDomainCoreDump(virDomainPtr dom, 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_REBOOT, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -2949,11 +2991,17 @@ endjob: } cleanup: - if (vm) - virDomainObjUnlock(vm); if (event) qemuDomainEventQueue(driver, event); + + if ((ret == 0) && (flags & VIR_DUMP_REBOOT) && vm) { + ret = doSystemReset(driver, &vm); + } + + if (vm) + virDomainObjUnlock(vm); qemuDriverUnlock(driver); + return ret; } diff --git a/tools/virsh.c b/tools/virsh.c index e5ea9d7..bdff33c 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")}, + {"reboot", VSH_OT_BOOL, 0, N_("reboot 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, "reboot")) + flags |= VIR_DUMP_REBOOT; if (virDomainCoreDump(dom, to, flags) < 0) { vshError(ctl, _("Failed to core dump domain %s to %s"), name, to); -- 1.7.3.4
On 09/23/2011 03:47 AM, Michal Privoznik wrote:
This patch introduces possibility to reboot domain after core dump finishes. The new flag VIR_DUMP_REBOOT was added to virDomainCoreDumpFlags. The new functionality is accessible via virsh too: virsh dump --reboot<domain> --- diff to v2: -issue reset command instead of qemuDomainReboot
include/libvirt/libvirt.h.in | 1 + src/qemu/qemu_driver.c | 54 +++++++++++++++++++++++++++++++++++++++-- tools/virsh.c | 3 ++ 3 files changed, 55 insertions(+), 3 deletions(-)
Missing documentation of the new flag in src/libvirt.c and tools/virsh.pod. Also, is the new flag compatible with VIR_DUMP_CRASH or VIR_DUMP_LIVE? For example, we already declare _CRASH and _LIVE as mutually exclusive, and off-hand, it seems like REBOOT is exclusive with both of these as well. Also, I'd split this patch into two pieces - one for documenting the new API (include/, src/libvirt.c, tools/), and the other for the qemu implementation of the new API (since most of my technical concerns are over the qemu implementation). Independent of your patch, I also just realized that virDomainSave[Flags], virDomainRestore[Flags], virDomainSaveImageGetXMLDesc, virDomainSaveImageDefineXML, and virDomainCoreDump all have the same design issue: they are not very nice for remote management. Each of these functions convert a relative path name into absolute name on the client side, before passing the string to the remote side, even though the actual file to be used is on the remote side. This is not always guaranteed to work, and also leaves things stuck with the file being on the remote side (no way to dump the core across the network back to the client, like there is with virDomainScreenshot). At some point, we may want to introduce new API that performs these types of operations on a stream, where the client can then manage the stream. And/or we may want to introduce a way to specify the "file" to manipulate as a virStorageVolPtr, or even a string relative to a virStoragePoolPtr, for more precise control of where the file ends up without having to first "absolutize" the file argument in the client.
+++ b/src/qemu/qemu_driver.c @@ -2860,6 +2860,47 @@ getCompressionType(struct qemud_driver *driver) return compress; }
+static int +doSystemReset(struct qemud_driver *driver, + virDomainObjPtr *vm) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv; + + /* Okay, this should never happen, but doesn't hurt to check. */ + if (!driver || !vm || !*vm) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid argument supplied")); + goto cleanup; + } + + priv = (*vm)->privateData; + + if (qemuDomainObjBeginJob(driver, *vm, QEMU_JOB_MODIFY)< 0) + goto cleanup; + + if (!virDomainObjIsActive(*vm)) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + goto endjob; + }
Just wondering if this is a case where it would actually make sense to start up a new qemu process, instead of just report that the guest unexpectedly quit.
+ + qemuDomainObjEnterMonitorWithDriver(driver, *vm); + if (qemuMonitorSystemReset(priv->mon)< 0) { + qemuDomainObjExitMonitorWithDriver(driver, *vm);
This is the key point of this new function. But I can't help but wonder...
+ goto endjob; + } + qemuDomainObjExitMonitorWithDriver(driver, *vm); + + ret = 0; + +endjob: + if (qemuDomainObjEndJob(driver, *vm) == 0) + *vm = NULL; +cleanup: + return ret; +} + static int qemudDomainCoreDump(virDomainPtr dom, const char *path, unsigned int flags) @@ -2870,7 +2911,8 @@ static int qemudDomainCoreDump(virDomainPtr dom, 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_REBOOT, -1);
qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -2949,11 +2991,17 @@ endjob: }
cleanup: - if (vm) - virDomainObjUnlock(vm); if (event) qemuDomainEventQueue(driver, event); + + if ((ret == 0)&& (flags& VIR_DUMP_REBOOT)&& vm) { + ret = doSystemReset(driver,&vm);
This ends up starting a second job. Wouldn't it be better to reuse the first job, by checking for VIR_DUMP_REBOOT in the endjob label alongside the check for 'resume && paused', and make the call to qemuMonitorSystemReset instead of qemuProcessStartCPUs at that point, so that the cleanup label remains untouched? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Sep 23, 2011 at 10:23:15AM -0600, Eric Blake wrote: [...]
Independent of your patch, I also just realized that virDomainSave[Flags], virDomainRestore[Flags], virDomainSaveImageGetXMLDesc, virDomainSaveImageDefineXML, and virDomainCoreDump all have the same design issue: they are not very nice for remote management. Each of these functions convert a relative path name into absolute name on the client side, before passing the string to the remote side, even though the actual file to be used is on the remote side. This is not always guaranteed to work, and also leaves things stuck with the file being on the remote side (no way to dump the core across the network back to the client, like there is with virDomainScreenshot).
yes, known annoying issue with those entry point nearly since day 1 of libvirt.
At some point, we may want to introduce new API that performs these types of operations on a stream, where the client can then manage the stream. And/or we may want to introduce a way to specify the "file" to manipulate as a virStorageVolPtr, or even a string relative to a virStoragePoolPtr, for more precise control of where the file ends up without having to first "absolutize" the file argument in the client.
Right we now have enough support to provide better APIs with streams, the 'only' problem is that basically the operation should only succeed if the whole stream was fetched correctly, because the last thing we want is a virDomainSaveStream suceeeding but where the data were not completely and reliably transmitted to the user, the data loss would be too huge, we can't stop the domain in such a failure. That's a bit tricky ... The alternate way which would have my preference is virDomainManagedSave... entry points where libvirt has the authority on the storage and can make sure everything is available, and then a specific function to stream back a managed save through a stream. Then we also have the existing entry point to delete a managed save so we have only 1 remote streaming function to really implement and get right, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Tue, Sep 27, 2011 at 03:52:38PM +0800, Daniel Veillard wrote:
On Fri, Sep 23, 2011 at 10:23:15AM -0600, Eric Blake wrote: [...]
Independent of your patch, I also just realized that virDomainSave[Flags], virDomainRestore[Flags], virDomainSaveImageGetXMLDesc, virDomainSaveImageDefineXML, and virDomainCoreDump all have the same design issue: they are not very nice for remote management. Each of these functions convert a relative path name into absolute name on the client side, before passing the string to the remote side, even though the actual file to be used is on the remote side. This is not always guaranteed to work, and also leaves things stuck with the file being on the remote side (no way to dump the core across the network back to the client, like there is with virDomainScreenshot).
yes, known annoying issue with those entry point nearly since day 1 of libvirt.
At some point, we may want to introduce new API that performs these types of operations on a stream, where the client can then manage the stream. And/or we may want to introduce a way to specify the "file" to manipulate as a virStorageVolPtr, or even a string relative to a virStoragePoolPtr, for more precise control of where the file ends up without having to first "absolutize" the file argument in the client.
Right we now have enough support to provide better APIs with streams, the 'only' problem is that basically the operation should only succeed if the whole stream was fetched correctly, because the last thing we want is a virDomainSaveStream suceeeding but where the data were not completely and reliably transmitted to the user, the data loss would be too huge, we can't stop the domain in such a failure. That's a bit tricky ... The alternate way which would have my preference is virDomainManagedSave... entry points where libvirt has the authority on the storage and can make sure everything is available, and then a specific function to stream back a managed save through a stream. Then we also have the existing entry point to delete a managed save so we have only 1 remote streaming function to really implement and get right,
Arguably, even with the regular virDomainSave method, you actually *do* want the file to be saved on the remote server. eg consider virt-manager running on your laptop, managing a bunch of virt servers. You don't want the VM save images to be on your laptop, you want them on the remote server. Core dump is the one where it is likely that you do want to get the core file onto the client machine. An impl using the streams API would suit this perfectly. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Fri, Sep 23, 2011 at 11:47:24AM +0200, Michal Privoznik wrote:
This patch introduces possibility to reboot domain after core dump finishes. The new flag VIR_DUMP_REBOOT was added to virDomainCoreDumpFlags. The new functionality is accessible via virsh too: virsh dump --reboot <domain> --- diff to v2: -issue reset command instead of qemuDomainReboot
We should name the option --reset rather than --reboot then.
include/libvirt/libvirt.h.in | 1 + src/qemu/qemu_driver.c | 54 +++++++++++++++++++++++++++++++++++++++-- tools/virsh.c | 3 ++ 3 files changed, 55 insertions(+), 3 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 39155a6..8c41f5a 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_REBOOT = (1 << 3), /* reboot domain after dump finishes */ } virDomainCoreDumpFlags;
/* Domain migration flags. */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0d0bea2..3e05e14 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2860,6 +2860,47 @@ getCompressionType(struct qemud_driver *driver) return compress; }
+static int +doSystemReset(struct qemud_driver *driver, + virDomainObjPtr *vm) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv; + + /* Okay, this should never happen, but doesn't hurt to check. */ + if (!driver || !vm || !*vm) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid argument supplied")); + goto cleanup; + } + + priv = (*vm)->privateData; + + if (qemuDomainObjBeginJob(driver, *vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (!virDomainObjIsActive(*vm)) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + goto endjob; + } + + qemuDomainObjEnterMonitorWithDriver(driver, *vm); + if (qemuMonitorSystemReset(priv->mon) < 0) { + qemuDomainObjExitMonitorWithDriver(driver, *vm); + goto endjob; + } + qemuDomainObjExitMonitorWithDriver(driver, *vm); + + ret = 0; + +endjob: + if (qemuDomainObjEndJob(driver, *vm) == 0) + *vm = NULL; +cleanup: + return ret; +} + static int qemudDomainCoreDump(virDomainPtr dom, const char *path, unsigned int flags) @@ -2870,7 +2911,8 @@ static int qemudDomainCoreDump(virDomainPtr dom, 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_REBOOT, -1);
qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -2949,11 +2991,17 @@ endjob: }
cleanup: - if (vm) - virDomainObjUnlock(vm); if (event) qemuDomainEventQueue(driver, event); + + if ((ret == 0) && (flags & VIR_DUMP_REBOOT) && vm) { + ret = doSystemReset(driver, &vm); + } + + if (vm) + virDomainObjUnlock(vm); qemuDriverUnlock(driver); + return ret; }
diff --git a/tools/virsh.c b/tools/virsh.c index e5ea9d7..bdff33c 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")}, + {"reboot", VSH_OT_BOOL, 0, N_("reboot 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, "reboot")) + flags |= VIR_DUMP_REBOOT;
if (virDomainCoreDump(dom, to, flags) < 0) { vshError(ctl, _("Failed to core dump domain %s to %s"), name, to); -- 1.7.3.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (5)
-
Daniel P. Berrange -
Daniel Veillard -
Dave Allan -
Eric Blake -
Michal Privoznik