Re: [libvirt] [PATCH 2/2] snapshot: enforce REVERT_FORCE on qemu
On 10/04/2011 04:02 PM, Eric Blake wrote: * src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Check for risky situations, and allow force to get past them. ACK. Before pushing this, I'm running some sanity tests. So far, this test sequence (adjusted to the fixed code) shows where force helps with older snapshots (I'll send separate email for showing how force helps active ABI-incompatible snapshots): Test 3: $ virsh define dom # domain with qcow2 disk $ virsh snapshot-create-as dom snap $ virsh edit dom # and add a second disk $ virsh start dom $ virsh snapshot-revert dom snap # revert succeeds and domain is offline $ virsh dumpxml dom # dom is back to one disk $ virsh snapshot-revert dom snap --running # revert succeeds, and dom boots $ virsh shutdown dom # and wait for it to work $ virsh edit dom # and add a second disk $ virsh start dom $ virt-manager # and open a window on the guest $ virsh snapshot-revert dom snap --running error: revert requires force: must respawn qemu to start inactive snapshot # Error was expected $ virsh snapshot-revert dom snap --running --force # revert succeeds, and virt-manager display bounces $ virsh dumpxml dom # dom is back to one disk And now that I've covered all three cases where I used the new VIR_ERR_SNAPSHOT_REVERT_RISKY error code in qemu_driver.c, I'm comfortable enough with my testing to go ahead and push all three ACK'd patches. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] snapshot: enforce REVERT_FORCE on qemu
On 10/04/2011 04:02 PM, Eric Blake wrote:
* src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Check for
risky situations, and allow force to get past them.
ACK.
Before pushing this, I'm running some sanity tests. So far, this test
sequence (adjusted to the fixed code) shows where force helps with older
snapshots (I'll send separate email for showing how force helps active
ABI-incompatible snapshots):
Test 2:
$ virsh define dom # domain with qcow2 disk
$ virsh start dom
$ virsh snapshot-create-as dom snap
$ virsh shutdown dom # and wait for it to work
$ virsh edit dom # and add a second disk
$ virsh start dom
$ virt-manager # and open a window on the guest
$ virsh snapshot-revert dom snap
error: unsupported configuration: Target domain disk count 1 does not
match source 2
# Error was expected(*), cannot do live revert across ABI break
$ virsh snapshot-revert dom snap --force
# --force let things work, and virt-manager display bounced due to
# need to start a new qemu process
$ virsh dumpxml dom # confirm only one disk present
$ virsh snapshot-revert dom snap
# revert happens without complaint and without display bounce
(*)That was the message shown before squashing in this patch; after
squashing in this patch, the message properly mentions "force":
error: revert requires force: Target domain disk count 1 does not match
source 2
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index 55398f6..2e6f3e4 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -9771,12 +9771,16 @@ static int
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
/* Transitions 5, 6, 8, 9 */
/* Check for ABI compatibility. */
if (config && !virDomainDefCheckABIStability(vm->def,
config)) {
+virErrorPtr err = virGetLastError();
+
if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) {
-/* Alter existing error to give correct category. */
-virErrorPtr err = virGetLastError();
-err->code = VIR_ERR_SNAPSHOT_REVERT_RISKY;
+/* Re-spawn error using correct category. */
+if (err->code == VIR_ERR_CONFIG_UNSUPPORTED)
+qemuReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY,
"%s",
+err->str2);
goto endjob;
}
+virResetError(err);
qemuProcessStop(driver, vm, 0,
VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT);
virDomainAuditStop(vm, "from-snapshot");
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] snapshot: enforce REVERT_FORCE on qemu
On 10/04/2011 04:02 PM, Eric Blake wrote: Before pushing this, I'm running some sanity tests. So far, this test sequence (adjusted to the fixed code) shows where force helps with older snapshots (I'll send separate email for showing how force helps active ABI-incompatible snapshots): Test 1: $ virsh snapshot-create-as dom snap # offline domain with one qcow2 disk $ virsh edit dom # add a second qcow2 disk $ virsh snapshot-revert dom snap # offline revert doesn't need force $ virsh dumpxml dom # sure enough, second disk is gone Whoops, this part of the test didn't quite work out, either. I need to revert to the snapshot prior to determining the list of disks to iterate over, so that we avoid calling qemu-img snapshot -a on the disk image that was not part of the snapshot. Likewise, snapshot-delete should call qemu-img snapshot -d only on the disk images involved in the snapshot. And here's what I have to squash in for test 1 to succeed as planned. Rather than squash in the fixed qemu-img iteration unreviewed into this ACK'd patch, I'll submit it as a separate patch. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] snapshot: enforce REVERT_FORCE on qemu
On 10/04/2011 01:26 PM, Laine Stump wrote:
On 09/30/2011 02:52 PM, Eric Blake wrote:
Implements the documentation for snapshot revert vs. force.
Part of the patch tightens existing behavior (previously, reverting
to an old snapshot without was blindly attempted, now it
requires force), while part of it relaxes behavior (previously, it
was not possible to revert an active domain to an ABI-incompatible
active snapshot, now force allows this transition).
* src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Check for
risky situations, and allow force to get past them.
ACK.
Before pushing this, I'm running some sanity tests. So far, this test
sequence (adjusted to the fixed code) shows where force helps with older
snapshots (I'll send separate email for showing how force helps active
ABI-incompatible snapshots):
Test 1:
$ virsh snapshot-create-as dom snap # offline domain with one qcow2 disk
$ virsh edit dom # add a second qcow2 disk
$ virsh snapshot-revert dom snap # offline revert doesn't need force
$ virsh dumpxml dom # sure enough, second disk is gone
$ virsh edit dom # add a second qcow2 disk
$ virsh snapshot-dumpxml dom snap > snap.xml
$ virsh snapshot-delete --metadata dom snap
$ sed -i '\|| d' snap.xml
$ virsh snapshot-create --redefine fedora_15-32 snap.xml
# the delete --metadata/--redefine is necessary, so that libvirt
# won't reuse from the prior definition
$ virsh snapshot-revert dom snap # simulates reverting to 0.9.4 snapshot
error: revert requires force: snapshot 'snap' lacks domain 'dom'
rollback info
$ virsh snapshot-revert dom snap --force
error: internal error Child process (/usr/bin/qemu-img snapshot -a snap
/path/to/second.qcow2) status unexpected: exit status 1
# See why we shouldn't have allowed blind revert? :)
$ virsh edit dom # remove that second disk
$ virsh snapshot-revert dom snap --force
# now that we match expected state, the revert works as desired
And here's what I have to squash in for test 1 to succeed as planned.
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index 1a171cf..07c4fd4 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -9649,7 +9649,8 @@ static int
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
virDomainDefPtr config = NULL;
virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
- VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED, -1);
+ VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED |
+ VIR_DOMAIN_SNAPSHOT_REVERT_FORCE, -1);
/* We have the following transitions, which create the following
events:
* 1. inactive -> inactive: none
@@ -9701,8 +9702,8 @@ static int
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) {
if (!snap->def->dom) {
qemuReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY,
-_("snapshot lacks domain '%s' rollback
details"),
-snap->def->name);
+_("snapshot '%s' lacks domain '%s' rollback
info"),
+snap->def->name, vm->def->name);
goto cleanup;
}
if (virDomainObjIsActive(vm) &&
diff --git i/src/qemu/qemu_domain.c w/src/qemu/qemu_domain.c
index 65f721a..dda53f3 100644
--- i/src/qemu/qemu_domain.c
+++ w/src/qemu/qemu_domain.c
@@ -1441,7 +1441,7 @@ qemuDomainSnapshotForEachQcow2(struct qemud_driver
*driver,
if (virRun(qemuimgarg, NULL) < 0) {
if (try_all) {
VIR_WARN("skipping snapshot action on %s",
- vm->def->disks[i]->info.alias);
+ vm->def->disks[i]->dst);
skipped = true;
continue;
}
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] snapshot: enforce REVERT_FORCE on qemu
On 09/30/2011 02:52 PM, Eric Blake wrote:
Implements the documentation for snapshot revert vs. force.
Part of the patch tightens existing behavior (previously, reverting
to an old snapshot without was blindly attempted, now it
requires force), while part of it relaxes behavior (previously, it
was not possible to revert an active domain to an ABI-incompatible
active snapshot, now force allows this transition).
* src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Check for
risky situations, and allow force to get past them.
---
src/qemu/qemu_driver.c | 47 +--
1 files changed, 37 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5110102..efd60a7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9753,7 +9753,8 @@ static int
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
* 7. paused -> inactive: EVENT_STOPPED
* 8. paused -> running: EVENT_RESUMED
* 9. paused -> paused: none
- * Also, several transitions occur even if we fail partway through.
+ * Also, several transitions occur even if we fail partway through,
+ * and use of FORCE can cause multiple transitions.
*/
qemuDriverLock(driver);
@@ -9789,6 +9790,24 @@ static int
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
"yet"));
goto cleanup;
}
+if (!(flags& VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) {
+if (!snap->def->dom) {
+qemuReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY,
+_("snapshot lacks domain '%s' rollback details"),
+snap->def->name);
+goto cleanup;
+}
+if (virDomainObjIsActive(vm)&&
+!(snap->def->state == VIR_DOMAIN_RUNNING
+ || snap->def->state == VIR_DOMAIN_PAUSED)&&
+(flags& (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
+ VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) {
+qemuReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY,
+_("must respawn qemu to start inactive snapshot"));
+goto cleanup;
+}
+}
+
if (vm->current_snapshot) {
vm->current_snapshot->def->current = false;
@@ -9818,11 +9837,6 @@ static int
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
VIR_FREE(xml);
if (!config)
goto cleanup;
-} else {
-/* XXX Fail if VIR_DOMAIN_REVERT_FORCE is not set, rather than
- * blindly hoping for the best. */
-VIR_WARN("snapshot is lacking rollback information for domain '%s'",
- snap->def->name);
}
if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY)< 0)
@@ -9843,10 +9857,22 @@ static int
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
/* Transitions 5, 6, 8, 9 */
/* Check for ABI compatibility. */
if (config&& !virDomainDefCheckABIStability(vm->def, config)) {
-/* XXX Add VIR_DOMAIN_REVERT_FORCE to permit killing
- * and restarting a new qemu, since loadvm monitor
- * command won't work. */
-goto endjob;
+if (!(flags& VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) {
+/* Alter existing error to give correct category. */
+virErrorPtr err = virGetLastError();
+err->code = VIR_ERR_SNAPSHOT_REVERT_RISKY;
+goto endjob;
+}
+qemuProcessStop(driver, vm, 0,
+VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT);
+virDomainAuditStop(vm, "from-snapshot");
+detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT;
+event = virDomainEventNewFromObj(vm,
+ VIR_DOMAIN_EVENT_STOPPED,
+ detail);
+if (event)
+qemuDomainEventQueue(driver, event);
+goto load;
}
priv = vm->privateData;
@@ -9882,6 +9908,7 @@ static int
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
virDomainObjAssignDef(vm, config, false);
} else {
/* Transitions 2, 3 */
+load:
was_stopped = true;
if (config)
virDomainObjAssignDef(vm, config, false);
ACK.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
