[libvirt] [PATCH 0/4] snapshot improvements
There's still a lot to go before virsh snapshot can manage disk snapshots in addition to checkpoints, but I wanted to get the review started on these. Even with these patches, there are some major bugs with snapshots: 1. Snapshots aren't storing the domain XML. If you create a snapshot, then hotplug a device, then revert to the snapshot, then the reversion process should include hot-unplugging that device (qemu won't automatically do it); fixing that requires diffing the xml between now and the reversion point. 2. Restarting libvirtd forgets which snapshot is current. When creating a new snapshot, it is important to remember the current snapshot in order to track the relationships correctly. 3. Deleting a domain that has snapshots doesn't warn. Unlike managed save images, where this represents data loss (you are throwing away the saved RAM state of the guest), it might be possible to permit undefining a domain with snapshots, but that would require a way to define a domain based on a snapshot, so maybe it really is better to prohibit undefining a domain with snapshots. 4. Probably more issues that need thinking about... Eric Blake (4): qemu: minor formatting cleanup qemu: give correct event when reverting to paused snapshot qemu: improve reverting to paused snapshots virsh: concatenate qemu-monitor-command arguments include/libvirt/libvirt.h.in | 3 +- src/qemu/qemu_driver.c | 76 +++++++++++++++++++++++++----------------- tools/virsh.c | 19 ++++++++-- tools/virsh.pod | 6 ++- 4 files changed, 66 insertions(+), 38 deletions(-) -- 1.7.4.4
I noticed some inconsistent use of 'else'. * src/qemu/qemu_driver.c (qemuCPUCompare) (qemuDomainSnapshotCreateXML, qemuDomainRevertToSnapshot) (qemuDomainSnapshotDiscard): Match coding conventions. --- No functional changes. src/qemu/qemu_driver.c | 23 ++++++++++------------- 1 files changed, 10 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ce19be7..3623824 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7990,9 +7990,9 @@ qemuCPUCompare(virConnectPtr conn, if (!driver->caps || !driver->caps->host.cpu) { qemuReportError(VIR_ERR_NO_SUPPORT, "%s", _("cannot get host CPU capabilities")); - } - else + } else { ret = cpuCompareXML(driver->caps->host.cpu, xmlDesc); + } qemuDriverUnlock(driver); @@ -8493,8 +8493,7 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, if (!virDomainObjIsActive(vm)) { if (qemuDomainSnapshotCreateInactive(vm, snap) < 0) goto cleanup; - } - else { + } else { if (qemuDomainSnapshotCreateActive(domain->conn, driver, &vm, snap) < 0) goto cleanup; @@ -8769,15 +8768,15 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, qemuDomainObjExitMonitorWithDriver(driver, vm); if (rc < 0) goto endjob; - } - else { + } else { if (qemuDomainSnapshotSetCurrentActive(vm, driver->snapshotDir) < 0) goto endjob; rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL, false, false, -1, NULL, VIR_VM_OP_CREATE); virDomainAuditStart(vm, "from-snapshot", rc >= 0); - if (qemuDomainSnapshotSetCurrentInactive(vm, driver->snapshotDir) < 0) + if (qemuDomainSnapshotSetCurrentInactive(vm, + driver->snapshotDir) < 0) goto endjob; if (rc < 0) goto endjob; @@ -8801,8 +8800,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); - } - else { + } else { /* qemu is a little funny with running guests and the restoration * of snapshots. If the snapshot was taken online, * then after a "loadvm" monitor command, the VM is set running @@ -8887,8 +8885,7 @@ static int qemuDomainSnapshotDiscard(struct qemud_driver *driver, } } } - } - else { + } else { priv = vm->privateData; qemuDomainObjEnterMonitorWithDriver(driver, vm); /* we continue on even in the face of error */ @@ -8909,9 +8906,9 @@ static int qemuDomainSnapshotDiscard(struct qemud_driver *driver, /* Now we set the new current_snapshot for the domain */ vm->current_snapshot = parentsnap; - } - else + } else { vm->current_snapshot = NULL; + } } if (virAsprintf(&snapFile, "%s/%s/%s.xml", driver->snapshotDir, -- 1.7.4.4
On 08/05/2011 06:00 PM, Eric Blake wrote:
I noticed some inconsistent use of 'else'.
* src/qemu/qemu_driver.c (qemuCPUCompare) (qemuDomainSnapshotCreateXML, qemuDomainRevertToSnapshot) (qemuDomainSnapshotDiscard): Match coding conventions. ---
No functional changes.
I've pushed this under the trivial rule, while still awaiting acks on the rest of the series. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
When reverting a running domain to a paused snapshot, the event that fires should mention that the domain is suspended. * include/libvirt/libvirt.h.in (VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT): New sub-event. * src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Use it. --- include/libvirt/libvirt.h.in | 3 ++- src/qemu/qemu_driver.c | 10 ++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index b1bda31..08832e1 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2009,7 +2009,7 @@ typedef enum { VIR_DOMAIN_EVENT_STARTED_BOOTED = 0, /* Normal startup from boot */ VIR_DOMAIN_EVENT_STARTED_MIGRATED = 1, /* Incoming migration from another host */ VIR_DOMAIN_EVENT_STARTED_RESTORED = 2, /* Restored from a state file */ - VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT = 3, /* Restored from snapshot */ + VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT = 3, /* Restored from running snapshot */ } virDomainEventStartedDetailType; /** @@ -2022,6 +2022,7 @@ typedef enum { VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED = 1, /* Suspended for offline migration */ VIR_DOMAIN_EVENT_SUSPENDED_IOERROR = 2, /* Suspended due to a disk I/O error */ VIR_DOMAIN_EVENT_SUSPENDED_WATCHDOG = 3, /* Suspended due to a watchdog firing */ + VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT = 4, /* Restored from paused snapshot */ } virDomainEventSuspendedDetailType; /** diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3623824..1077843 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8792,14 +8792,16 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, QEMU_ASYNC_JOB_NONE); if (rc < 0) goto endjob; + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT); } else { virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_FROM_SNAPSHOT); + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_STARTED, + VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); } - - event = virDomainEventNewFromObj(vm, - VIR_DOMAIN_EVENT_STARTED, - VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); } else { /* qemu is a little funny with running guests and the restoration * of snapshots. If the snapshot was taken online, -- 1.7.4.4
If you take a checkpoint snapshot of a running domain, then pause qemu, then restore the snapshot, the result should be a running domain, but the code was leaving things paused. Furthermore, if you take a checkpoint of a paused domain, then run, then restore, there was a brief but non-deterministic window of time where the domain was running rather than paused. Fix both of these discrepancies by always pausing before restoring. * src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Always pause before reversion. --- src/qemu/qemu_driver.c | 51 +++++++++++++++++++++++++++++++---------------- 1 files changed, 33 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1077843..2f5af87 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8760,44 +8760,59 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (snap->def->state == VIR_DOMAIN_RUNNING || snap->def->state == VIR_DOMAIN_PAUSED) { - + /* When using the loadvm monitor command, qemu does not know + * whether to pause or run the reverted domain, and just stays + * in the same state as before the monitor command, whether + * that is paused or running. We always pause before loadvm, + * to have finer control. */ if (virDomainObjIsActive(vm)) { priv = vm->privateData; + if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { + if (qemuProcessStopCPUs(driver, vm, + VIR_DOMAIN_PAUSED_FROM_SNAPSHOT, + QEMU_ASYNC_JOB_NONE) < 0) + goto endjob; + /* Create an event now in case the restore fails, so + * that user will be alerted that they are now paused. + * If restore later succeeds to a running state, we + * replace this event with another. */ + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); + } qemuDomainObjEnterMonitorWithDriver(driver, vm); rc = qemuMonitorLoadSnapshot(priv->mon, snap->def->name); qemuDomainObjExitMonitorWithDriver(driver, vm); - if (rc < 0) + if (rc < 0) { + /* XXX resume domain if it was running before the + * failed loadvm attempt? */ goto endjob; + } } else { if (qemuDomainSnapshotSetCurrentActive(vm, driver->snapshotDir) < 0) goto endjob; rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL, - false, false, -1, NULL, VIR_VM_OP_CREATE); + true, false, -1, NULL, VIR_VM_OP_CREATE); virDomainAuditStart(vm, "from-snapshot", rc >= 0); + if (rc < 0) + goto endjob; if (qemuDomainSnapshotSetCurrentInactive(vm, driver->snapshotDir) < 0) goto endjob; - if (rc < 0) - goto endjob; } + /* Touch up domain state. */ if (snap->def->state == VIR_DOMAIN_PAUSED) { - /* qemu unconditionally starts the domain running again after - * loadvm, so let's pause it to keep consistency - * XXX we should have used qemuProcessStart's start_paused instead - */ - rc = qemuProcessStopCPUs(driver, vm, - VIR_DOMAIN_PAUSED_FROM_SNAPSHOT, - QEMU_ASYNC_JOB_NONE); + virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, + VIR_DOMAIN_PAUSED_FROM_SNAPSHOT); + } else { + rc = qemuProcessStartCPUs(driver, vm, snapshot->domain->conn, + VIR_DOMAIN_RUNNING_FROM_SNAPSHOT, + QEMU_ASYNC_JOB_NONE); if (rc < 0) goto endjob; - event = virDomainEventNewFromObj(vm, - VIR_DOMAIN_EVENT_SUSPENDED, - VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT); - } else { - virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, - VIR_DOMAIN_RUNNING_FROM_SNAPSHOT); + virDomainEventFree(event); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); -- 1.7.4.4
Call me lazy, but: virsh qemu-monitor-command dom --hmp info status is nicer than: virsh qemu-monitor-command dom --hmp 'info status' * tools/virsh.c (cmdQemuMonitorCommand): Allow multiple arguments, for convenience. --- This helped me test the previous patch :) tools/virsh.c | 19 +++++++++++++++---- tools/virsh.pod | 6 ++++-- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index b053ed0..ffdb2de 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12381,8 +12381,8 @@ static const vshCmdInfo info_qemu_monitor_command[] = { static const vshCmdOptDef opts_qemu_monitor_command[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, - {"cmd", VSH_OT_DATA, VSH_OFLAG_REQ, N_("command")}, {"hmp", VSH_OT_BOOL, 0, N_("command is in human monitor protocol")}, + {"cmd", VSH_OT_ARGV, VSH_OFLAG_REQ, N_("command")}, {NULL, 0, 0, NULL} }; @@ -12391,9 +12391,12 @@ cmdQemuMonitorCommand(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; bool ret = false; - const char *monitor_cmd = NULL; + char *monitor_cmd = NULL; char *result = NULL; unsigned int flags = 0; + const vshCmdOpt *opt = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + bool pad = false; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -12402,10 +12405,17 @@ cmdQemuMonitorCommand(vshControl *ctl, const vshCmd *cmd) if (dom == NULL) goto cleanup; - if (vshCommandOptString(cmd, "cmd", &monitor_cmd) <= 0) { - vshError(ctl, "%s", _("missing monitor command")); + while ((opt = vshCommandOptArgv(cmd, opt))) { + virBufferAdd(&buf, opt->data, -1); + if (pad) + virBufferAddChar(&buf, ' '); + pad = true; + } + if (virBufferError(&buf)) { + vshPrint(ctl, "%s", _("Failed to collect command")); goto cleanup; } + monitor_cmd = virBufferContentAndReset(&buf); if (vshCommandOptBool(cmd, "hmp")) flags |= VIR_DOMAIN_QEMU_MONITOR_COMMAND_HMP; @@ -12419,6 +12429,7 @@ cmdQemuMonitorCommand(vshControl *ctl, const vshCmd *cmd) cleanup: VIR_FREE(result); + VIR_FREE(monitor_cmd); if (dom) virDomainFree(dom); diff --git a/tools/virsh.pod b/tools/virsh.pod index 01b8fd6..ec778bf 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1683,13 +1683,15 @@ attaching to an externally launched QEMU process. There may be issues with the guest ABI changing upon migration, and hotunplug may not work. -=item B<qemu-monitor-command> I<domain> I<command> [I<--hmp>] +=item B<qemu-monitor-command> I<domain> [I<--hmp>] I<command>... Send an arbitrary monitor command I<command> to domain I<domain> through the qemu monitor. The results of the command will be printed on stdout. If I<--hmp> is passed, the command is considered to be a human monitor command and libvirt will automatically convert it into QMP if needed. In that case -the result will also be converted back from QMP. +the result will also be converted back from QMP. If more than one argument +is provided for I<command>, they are concatenated with a space in between +before passing the single command to the monitor. =back -- 1.7.4.4
On Fri, Aug 05, 2011 at 06:00:08PM -0600, Eric Blake wrote:
Call me lazy, but:
virsh qemu-monitor-command dom --hmp info status
is nicer than:
virsh qemu-monitor-command dom --hmp 'info status'
This does introduce a quoting problem though. eg consider virsh qemu-monitor-command dom --hmp 'foo "Hello World"' vs what you'd now allow: virsh qemu-monitor-command dom --hmp foo "Hello World" but....
@@ -12402,10 +12405,17 @@ cmdQemuMonitorCommand(vshControl *ctl, const vshCmd *cmd) if (dom == NULL) goto cleanup;
- if (vshCommandOptString(cmd, "cmd", &monitor_cmd) <= 0) { - vshError(ctl, "%s", _("missing monitor command")); + while ((opt = vshCommandOptArgv(cmd, opt))) { + virBufferAdd(&buf, opt->data, -1); + if (pad) + virBufferAddChar(&buf, ' '); + pad = true; + }
...this loop will loose the quoting on 'Hello World' I believe. 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 08/08/2011 03:40 AM, Daniel P. Berrange wrote:
On Fri, Aug 05, 2011 at 06:00:08PM -0600, Eric Blake wrote:
Call me lazy, but:
virsh qemu-monitor-command dom --hmp info status
is nicer than:
virsh qemu-monitor-command dom --hmp 'info status'
This does introduce a quoting problem though. eg consider
virsh qemu-monitor-command dom --hmp 'foo "Hello World"'
vs what you'd now allow:
virsh qemu-monitor-command dom --hmp foo "Hello World"
This is no different from how the shell behaves. If you _want_ literal quotes to be part of the command, then you have to double-quote or otherwise escape those quotes. Here's some examples using two spaces to show where spacing is quoted vs. where concatenation occurs, in both shell and virsh. $ echo a "b c" a b c $ echo 'a "b c"' a "b c" $ echo a \"b c\" a "b c" $ virsh echo a 'b c' a b c $ virsh echo 'a "b c"' a "b c" $ virsh echo a \"b c\" a "b c" $ virsh virsh # echo a "b c" a b c virsh # echo 'a "b c"' a "b c" virsh # echo a \"b c\" a "b c" virsh # exit
- if (vshCommandOptString(cmd, "cmd",&monitor_cmd)<= 0) { - vshError(ctl, "%s", _("missing monitor command")); + while ((opt = vshCommandOptArgv(cmd, opt))) { + virBufferAdd(&buf, opt->data, -1); + if (pad) + virBufferAddChar(&buf, ' '); + pad = true; + }
...this loop will loose the quoting on 'Hello World' I believe.
The shell already lost the quoting. Virsh should not be using heuristics to re-add arbitrary quoting, rather, if you need quoting to be passed through to the monitor command, you _have_ to use double-quoting or escapes. Your argument about losing quotes does not change this patch. Argument concatenation is most useful only for words that do not themselves contain quoting or spaces. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Hello Eic, hello list, On Saturday 06 August 2011 02:00:04 Eric Blake wrote:
There's still a lot to go before virsh snapshot can manage disk snapshots in addition to checkpoints, but I wanted to get the review started on these.
Even with these patches, there are some major bugs with snapshots: 1. Snapshots aren't storing the domain XML. If you create a snapshot, then hotplug a device, then revert to the snapshot, then the reversion process should include hot-unplugging that device (qemu won't automatically do it); fixing that requires diffing the xml between now and the reversion point.
I posted a proof-of-concept implementation back in April <http://www.redhat.com/archives/libvir-list/2011-April/msg00564.html>, which put a copy of the domain-xml into the snapshot-xml file, so it could be restored. - The patch is bit-rotting since than, but the implemetation more or less works with our version of libvirt-0.8.3. - The implementation always restarts the kvm process, because virDomainDefCheckABIStability() wasn't available back than which I think can be used to decide, if the current currung kvm process is compatible with the to-be-restored configuration.
2. Restarting libvirtd forgets which snapshot is current. When creating a new snapshot, it is important to remember the current snapshot in order to track the relationships correctly.
That's what I also noticed. Why is the parent important? As long as you use qemus internal snapshots, I think you don't need it. I think is important if you build chained snapshots and than want to delete intermediate snapshots.
4. Probably more issues that need thinking about...
5. Migrating a domain with snapshots looses the snapshots, because the receiving libvirtd doesn't get the snapshot-xml data as well. The data is still there, but you have to move the domain back to the original host and must restart libvirtd, because libvirtd only reads the snapshot-xml-data once on initial start; not on reload or any other event. 6. Reverting to an offline-snapshot currently does not work with qemu-0.14, since "qemu loadvm" doesn't call itself responsible for doing that any longer; see <http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg00650.html> for details. If you (or anybody other) is working on those issues, please keep me (and the list) informed, so we don't do double implementation. Thanks. Sincerely Philipp -- Philipp Hahn Open Source Software Engineer hahn@univention.de Univention GmbH Linux for Your Business fon: +49 421 22 232- 0 Mary-Somerville-Str.1 D-28359 Bremen fax: +49 421 22 232-99 http://www.univention.de/
qemuDomainSnapshotRevertInactive has the same FIXMEs as qemuDomainSnapshotCreateInactive, so algorithmic fixes to properly handle partial loop iterations should be applied later to both functions, but we're not making the situation any worse in this patch. * src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Use qemu-img rather than 'qemu -loadvm' to revert to offline snapshot. (qemuDomainSnapshotRevertInactive): New helper. --- src/qemu/qemu_driver.c | 65 +++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 56 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 16daee2..ac323b6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8726,6 +8726,52 @@ cleanup: return xml; } +/* The domain is expected to be locked and inactive. */ +static int +qemuDomainSnapshotRevertInactive(virDomainObjPtr vm, + virDomainSnapshotObjPtr snap) +{ + const char *qemuimgarg[] = { NULL, "snapshot", "-a", NULL, NULL, NULL }; + int ret = -1; + int i; + + qemuimgarg[0] = qemuFindQemuImgBinary(); + if (qemuimgarg[0] == NULL) { + /* qemuFindQemuImgBinary set the error */ + goto cleanup; + } + + qemuimgarg[3] = snap->def->name; + + for (i = 0; i < vm->def->ndisks; i++) { + /* FIXME: we also need to handle LVM here */ + /* FIXME: if we fail halfway through this loop, we are in an + * inconsistent state. I'm not quite sure what to do about that + */ + if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + if (!vm->def->disks[i]->driverType || + STRNEQ(vm->def->disks[i]->driverType, "qcow2")) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("Disk device '%s' does not support" + " snapshotting"), + vm->def->disks[i]->info.alias); + goto cleanup; + } + + qemuimgarg[4] = vm->def->disks[i]->src; + + if (virRun(qemuimgarg, NULL) < 0) + goto cleanup; + } + } + + ret = 0; + +cleanup: + VIR_FREE(qemuimgarg[0]); + return ret; +} + static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, unsigned int flags) { @@ -8822,14 +8868,9 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); } } else { - /* qemu is a little funny with running guests and the restoration - * of snapshots. If the snapshot was taken online, - * then after a "loadvm" monitor command, the VM is set running - * again. If the snapshot was taken offline, then after a "loadvm" - * monitor command the VM is left paused. Unpausing it leads to - * the memory state *before* the loadvm with the disk *after* the - * loadvm, which obviously is bound to corrupt something. - * Therefore we destroy the domain and set it to "off" in this case. + /* Newer qemu -loadvm refuses to revert to the state of a snapshot + * created by qemu-img snapshot -c. If the domain is running, we + * must take it offline; then do the revert using qemu-img. */ if (virDomainObjIsActive(vm)) { @@ -8839,6 +8880,12 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); if (!vm->persistent) { + /* XXX it should be impossible to create an offline snapshot + * of a transient domain. Once we fix 'undefine' to convert + * a defined domain back to transient, that transition should + * be rejected if any offline snapshots exist. For now, we + * just stop the transient domain and quit, without reverting + * any disk state. */ if (qemuDomainObjEndJob(driver, vm) > 0) virDomainRemoveInactive(&driver->domains, vm); vm = NULL; @@ -8846,7 +8893,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } } - if (qemuDomainSnapshotSetCurrentActive(vm, driver->snapshotDir) < 0) + if (qemuDomainSnapshotRevertInactive(vm, snap) < 0) goto endjob; } -- 1.7.4.4
Hello Eric, you're patch looks very similar to mine, which I created myself yesterday, but hadn't had time to actually send after doing the testing. I'll attach it just FYI. On Wednesday 10 August 2011 01:28:42 Eric Blake wrote:
qemuDomainSnapshotRevertInactive has the same FIXMEs as qemuDomainSnapshotCreateInactive, so algorithmic fixes to properly handle partial loop iterations should be applied later to both functions, but we're not making the situation any worse in this patch.
* src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Use qemu-img rather than 'qemu -loadvm' to revert to offline snapshot. (qemuDomainSnapshotRevertInactive): New helper.
s/offline/inactive/ just for consistency.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c ... @@ -8846,7 +8893,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } }
- if (qemuDomainSnapshotSetCurrentActive(vm, driver->snapshotDir) < 0) + if (qemuDomainSnapshotRevertInactive(vm, snap) < 0)
Now you don't call qemuDomainSnapshotSetCurrent() any more, which marks the snapshot at being current (what ever that is important for). That's why I in my patch also have to change qemuBuildCommandLine() to not add -loadvm, since qemu then will refuse to start.
goto endjob; }
Sincerely Philipp -- Philipp Hahn Open Source Software Engineer hahn@univention.de Univention GmbH Linux for Your Business fon: +49 421 22 232- 0 Mary-Somerville-Str.1 D-28359 Bremen fax: +49 421 22 232-99 http://www.univention.de/
On 08/10/2011 12:24 AM, Philipp Hahn wrote:
Hello Eric,
you're patch looks very similar to mine, which I created myself yesterday, but hadn't had time to actually send after doing the testing. I'll attach it just FYI.
On Wednesday 10 August 2011 01:28:42 Eric Blake wrote:
qemuDomainSnapshotRevertInactive has the same FIXMEs as qemuDomainSnapshotCreateInactive, so algorithmic fixes to properly handle partial loop iterations should be applied later to both functions, but we're not making the situation any worse in this patch.
* src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Use qemu-img rather than 'qemu -loadvm' to revert to offline snapshot. (qemuDomainSnapshotRevertInactive): New helper.
s/offline/inactive/ just for consistency.
Sure.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c ... @@ -8846,7 +8893,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } }
- if (qemuDomainSnapshotSetCurrentActive(vm, driver->snapshotDir)< 0) + if (qemuDomainSnapshotRevertInactive(vm, snap)< 0)
Now you don't call qemuDomainSnapshotSetCurrent() any more, which marks the snapshot at being current (what ever that is important for). That's why I in my patch also have to change qemuBuildCommandLine() to not add -loadvm, since qemu then will refuse to start.
Right now, qemu's _only_ use of <active> (which is set to non-zero by qemuDomainSnapshotSetCurrentActive) is to tell qemuBuildCommandLine to use -loadvm to revert to that point, rather than to do a fresh boot. Your approach of keeping the call means that qemuBuildCommandLine has to special-case to not use -loadvm for inactive snapshots; whereas my approach of no longer marking the snapshot active for inactive snapshots means that the existing qemuBuildCommandLine no longer sees an active snapshot in the first place; both approaches end up omitting the -loadvm call on the next domain boot, but mine does it with less code and without using <active>. Meanwhile, I have plans to repurpose the <active> element. Right now, it is stored as a long, but only ever contains the value 0 or 1 as used by qemu (it is not used by other drivers), and is used by at most one snapshot at a time. But since we _want_ to fix the bug where libvirtd forgets the current snapshot when it is restarted, I propose that active be repurposed slightly, as well as changed to int or even bool, since long is overkill: qemuBuildCommandLine is already being passed current_snapshot or NULL; that should be sufficient to decide whether to add the -loadvm argument without also probing active; so all callers should be updated to pass NULL except for reverting from an active snapshot when qemu is not already running - this means a signature tweak to qemuProcessStart. Once that is done, qemu is no longer using active for its own purposes, so conf/domain_conf can then take over <active> to uniformly identify which snapshot is current as part of the snapshot xml (that is, all but one snapshot will be <active>0</active>, and the current snapshot will be <active>1</active>). Every time the current snapshot changes, this implies updating as many as two files in the snapshot xml directory. <active> is already an internal-only xml element - domain_conf ignores it on user input, and strips it on dumpxml output visible to the user, so it is only present in the internal xml files. The idea is that if there is at least one snapshot, then there generally be a current snapshot; the only time that there is not a current snapshot but where snapshots still exist is when you create multiple trees in the hierarchy by setting the current snapshot to the root of the hierarchy, then delete that snapshot while keeping its children intact (making each child an independent root). More patches coming later today along these lines, once I test it all. Another goal of mine is to introduce 'virsh snapshot-list dom --tree', which shows snapshot names with a visual hierarchy relationship: root child1 grandchild1.1 grandchild1.2 child2 grandchild2.1 grandchild2.2 That would be made much easier if we had an API for virDomainSnapshotGetParentName (see also my wish for virDomainSnapshotGetName in patch 6/4); but it can also be done with xml scraping using existing API. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Hello Eric, thank you for the long description of <active> Am Mittwoch 10 August 2011 14:18:22 schrieb Eric Blake:
Right now, qemu's _only_ use of <active> (which is set to non-zero by qemuDomainSnapshotSetCurrentActive) is to tell qemuBuildCommandLine to use -loadvm to revert to that point, rather than to do a fresh boot. Your approach of keeping the call means that qemuBuildCommandLine has to special-case to not use -loadvm for inactive snapshots; whereas my approach of no longer marking the snapshot active for inactive snapshots means that the existing qemuBuildCommandLine no longer sees an active snapshot in the first place; both approaches end up omitting the -loadvm call on the next domain boot, but mine does it with less code and without using <active>.
Since <active> is persisted in the snapshot/ directory, I didn't want to change that, so I didn't remove that call.
Once that is done, qemu is no longer using active for its own purposes, so conf/domain_conf can then take over <active> to uniformly identify which snapshot is current as part of the snapshot xml (that is, all but one snapshot will be <active>0</active>, and the current snapshot will be <active>1</active>). Every time the current snapshot changes, ...
I find the name "current" highly confusing, because at least for Qemus internal shapshot it is something more like "last snapshot restored": As soon as the VM begins to run, it diverges from the snapshot state and is no longer equal to it, so it is not "current" any more. Only when restoring offline or paused states would I have the chance to realistically see the VM being in the same state as the snapshot. t branch. So if <active> is only used to track the last restored snapshot and to fill in the <parent> data in the <domsinsnapshot> data, your patch looks fine. Sincerely Philipp Hahn -- Philipp Hahn Open Source Software Engineer hahn@univention.de Univention GmbH Linux for Your Business fon: +49 421 22 232- 0 Mary-Somerville-Str.1 D-28359 Bremen fax: +49 421 22 232-99 http://www.univention.de/
Sometimes, full XML is too much; since most snapshot commands operate on a snapshot name, there should be an easy way to get at the current snapshot's name. * tools/virsh.c (cmdSnapshotCurrent): Add an option. * tools/virsh.pod (snapshot-current): Document it. --- tools/virsh.c | 21 ++++++++++++++++++--- tools/virsh.pod | 4 +++- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index bffec1d..1ad45b0 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12034,6 +12034,7 @@ static const vshCmdInfo info_snapshot_current[] = { static const vshCmdOptDef opts_snapshot_current[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"name", VSH_OT_BOOL, 0, N_("list the name, rather than the full xml")}, {NULL, 0, 0, NULL} }; @@ -12044,6 +12045,7 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) bool ret = false; int current; virDomainSnapshotPtr snapshot = NULL; + char *xml = NULL; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -12056,7 +12058,7 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) if (current < 0) goto cleanup; else if (current) { - char *xml; + char *name = NULL; if (!(snapshot = virDomainSnapshotCurrent(dom, 0))) goto cleanup; @@ -12065,13 +12067,26 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) if (!xml) goto cleanup; - vshPrint(ctl, "%s", xml); - VIR_FREE(xml); + if (vshCommandOptBool(cmd, "name")) { + char *tmp; + + name = strstr(xml, "<name>"); + if (!name) + goto cleanup; + name += strlen("<name>"); + tmp = strstr(name, "</name>"); + if (!tmp) + goto cleanup; + *tmp = '\0'; + } + + vshPrint(ctl, "%s", name ? name : xml); } ret = true; cleanup: + VIR_FREE(xml); if (snapshot) virDomainSnapshotFree(snapshot); if (dom) diff --git a/tools/virsh.pod b/tools/virsh.pod index ec778bf..53376d6 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1569,9 +1569,11 @@ Create a snapshot for domain I<domain> with the given <name> and value. If I<--print-xml> is specified, then XML appropriate for I<snapshot-create> is output, rather than actually creating a snapshot. -=item B<snapshot-current> I<domain> +=item B<snapshot-current> I<domain> [I<--name>] Output the snapshot XML for the domain's current snapshot (if any). +If I<--name> is specified, just list the snapshot name instead of the +full xml. =item B<snapshot-list> I<domain> -- 1.7.4.4
On 08/09/2011 09:13 PM, Eric Blake wrote:
Sometimes, full XML is too much; since most snapshot commands operate on a snapshot name, there should be an easy way to get at the current snapshot's name.
* tools/virsh.c (cmdSnapshotCurrent): Add an option. * tools/virsh.pod (snapshot-current): Document it. --- tools/virsh.c | 21 ++++++++++++++++++--- tools/virsh.pod | 4 +++- 2 files changed, 21 insertions(+), 4 deletions(-)
@@ -12065,13 +12067,26 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) if (!xml) goto cleanup;
- vshPrint(ctl, "%s", xml); - VIR_FREE(xml); + if (vshCommandOptBool(cmd, "name")) { + char *tmp; + + name = strstr(xml, "<name>"); + if (!name) + goto cleanup; + name += strlen("<name>"); + tmp = strstr(name, "</name>");
Admittedly, this mishandles any XML escapes, such as '<', occurring in name. A more robust solution involves introducing a new virDomainSnapshotGetName; however, while that is a good idea, it goes counter to my goal of no new API for my first round of disk-only snapshot support. Then again, I'm thinking that even with a new API, virsh should be smart enough to use the new API where it works, and fall back to the xml scraping where it doesn't, so I'm debating whether this needs to switch to full-blown xpath parsing instead of just strstr scraping. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Philipp Hahn