[libvirt] [PATCH 0/4] Various snapshot fixes for qemu
Peter Krempa (4): qemu: snapshot: Use typecasted switch in qemuDomainSnapshotPrepare() qemu: snapshot: Forbid partial internal snapshots qemu: snapshot: Forbid empty snapshots qemu: snapshot: Fix return value of external checkpoint with no disks src/qemu/qemu_driver.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) -- 1.9.3
Convert the switch to a typecasted value so that the compiler tracks additions for us. --- src/qemu/qemu_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c8a0029..aa07ef6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12635,7 +12635,7 @@ qemuDomainSnapshotPrepare(virConnectPtr conn, virDomainSnapshotDiskDefPtr disk = &def->disks[i]; virDomainDiskDefPtr dom_disk = vm->def->disks[i]; - switch (disk->snapshot) { + switch ((virDomainSnapshotLocation) disk->snapshot) { case VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL: found_internal = true; @@ -12692,7 +12692,7 @@ qemuDomainSnapshotPrepare(virConnectPtr conn, break; case VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT: - default: + case VIR_DOMAIN_SNAPSHOT_LOCATION_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("unexpected code path")); goto cleanup; -- 1.9.3
On 05/20/2014 07:35 AM, Peter Krempa wrote:
Convert the switch to a typecasted value so that the compiler tracks additions for us. --- src/qemu/qemu_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
qemu's savevm command does a snapshot of all non readonly disks of a VM. Libvirt though allowed disabling snapshot for certain disk of a VM. --- src/qemu/qemu_driver.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index aa07ef6..e6b6648 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12621,6 +12621,7 @@ qemuDomainSnapshotPrepare(virConnectPtr conn, bool reuse = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; bool atomic = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) != 0; bool found_internal = false; + bool forbid_internal = false; int external = 0; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -12689,6 +12690,9 @@ qemuDomainSnapshotPrepare(virConnectPtr conn, break; case VIR_DOMAIN_SNAPSHOT_LOCATION_NONE: + /* Remember seeing a disk that has snapshot disabled */ + if (!dom_disk->readonly) + forbid_internal = true; break; case VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT: @@ -12699,12 +12703,13 @@ qemuDomainSnapshotPrepare(virConnectPtr conn, } } - /* internal snapshot requires a disk image to store the memory image to */ - if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL && - !found_internal) { + /* internal snapshot requires a disk image to store the memory image to and + * also disks can't be excluded from a internal snapshot*/ + if ((def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL && !found_internal) || + (found_internal && forbid_internal)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("internal checkpoints require at least " - "one disk to be selected for snapshot")); + _("internal snapshots and checkpoints require all " + "disks to be selected for snapshot")); goto cleanup; } -- 1.9.3
On 05/20/2014 03:36 PM, Peter Krempa wrote:
qemu's savevm command does a snapshot of all non readonly disks of a VM. Libvirt though allowed disabling snapshot for certain disk of a VM. --- src/qemu/qemu_driver.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index aa07ef6..e6b6648 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c
@@ -12699,12 +12703,13 @@ qemuDomainSnapshotPrepare(virConnectPtr conn, } }
- /* internal snapshot requires a disk image to store the memory image to */ - if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL && - !found_internal) { + /* internal snapshot requires a disk image to store the memory image to and + * also disks can't be excluded from a internal snapshot*/
s/a internal snapshot/an internal snapshot /
+ if ((def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL && !found_internal) || + (found_internal && forbid_internal)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("internal checkpoints require at least " - "one disk to be selected for snapshot")); + _("internal snapshots and checkpoints require all " + "disks to be selected for snapshot")); goto cleanup; }
Jan
On 05/20/2014 07:36 AM, Peter Krempa wrote:
qemu's savevm command does a snapshot of all non readonly disks of a VM. Libvirt though allowed disabling snapshot for certain disk of a VM. --- src/qemu/qemu_driver.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
I think this may even be a regression introduced when we added checkpoint snapshots, because I seem to recall trying to prevent just this back when doing external disk snapshots. But I didn't bother to crawl git history to confirm or deny my suspicion.
@@ -12699,12 +12703,13 @@ qemuDomainSnapshotPrepare(virConnectPtr conn, } }
- /* internal snapshot requires a disk image to store the memory image to */ - if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL && - !found_internal) { + /* internal snapshot requires a disk image to store the memory image to and
s/to and/to, and/
+ * also disks can't be excluded from a internal snapshot*/
s/a internal/an internal/
+ if ((def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL && !found_internal) || + (found_internal && forbid_internal)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("internal checkpoints require at least " - "one disk to be selected for snapshot")); + _("internal snapshots and checkpoints require all " + "disks to be selected for snapshot")); goto cleanup; }
ACK with the comment grammar fixes -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
If neither disks nor memory are selected for snapshot we'd record metadata in case of external snapshot and do a disk snapshot in case of external disk snapshot. Forbid this as it doesn't make much sense. --- Notes: There's a slightly usable case where this would be used to backup domain's config. src/qemu/qemu_driver.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e6b6648..6442b3f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12703,6 +12703,13 @@ qemuDomainSnapshotPrepare(virConnectPtr conn, } } + if (!found_internal && !external && + def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("nothing selected for snapshot")); + goto cleanup; + } + /* internal snapshot requires a disk image to store the memory image to and * also disks can't be excluded from a internal snapshot*/ if ((def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL && !found_internal) || -- 1.9.3
On 05/20/2014 03:36 PM, Peter Krempa wrote:
If neither disks nor memory are selected for snapshot we'd record metadata in case of external snapshot and do a disk snapshot in case of external disk snapshot. Forbid this as it doesn't make much sense.
s/external/internal/
---
Notes: There's a slightly usable case where this would be used to backup domain's config.
src/qemu/qemu_driver.c | 7 +++++++ 1 file changed, 7 insertions(+)
Jan
On 05/20/2014 07:36 AM, Peter Krempa wrote:
If neither disks nor memory are selected for snapshot we'd record metadata in case of external snapshot and do a disk snapshot in case of external disk snapshot. Forbid this as it doesn't make much sense. ---
Notes: There's a slightly usable case where this would be used to backup domain's config.
Hmm. Down the road, when we start integrating chain operations (pull, commit) more closely with snapshots, we may hit the case where a blockpull goes through and modifies a snapshot to record that the disk snapshot taken at that point of time was invalidated by the chain operation. Perhaps we could still leave the empty snapshot in the list of snapshots, and if so, then this code should also allow creation of empty snapshots. But I tend to agree that it sounds rather pointless to track empty snapshots, so I'd rather have chain operations delete snapshots that are otherwise rendered empty, and agree with the approach of this patch in forbidding an empty snapshot. Someone wanting to preserve configuration can use virsh dumpxml.
+++ b/src/qemu/qemu_driver.c @@ -12703,6 +12703,13 @@ qemuDomainSnapshotPrepare(virConnectPtr conn, } }
+ if (!found_internal && !external && + def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("nothing selected for snapshot")); + goto cleanup; + }
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
When doing an external checkpoint of a VM with no disk selected we'd return failure but not set error code. This was a result of ret not being set to 0 during walking of the disk array. Rework early failure checking to avoid the problem. Fixes the following symptom (or without --diskspec for diskless VMs) $ virsh snapshot-create-as snapshot-test --memspec /tmp/asdf --diskspec hda,snapshot=no error: An error occurred, but the cause is unknown --- src/qemu/qemu_driver.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6442b3f..460c680 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13010,17 +13010,17 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, { qemuDomainObjPrivatePtr priv = vm->privateData; virJSONValuePtr actions = NULL; - int ret = -1; + int ret = 0; size_t i; bool persist = false; bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virQEMUDriverConfigPtr cfg = NULL; virErrorPtr orig_err = NULL; if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); - goto cleanup; + return -1; } if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION)) { @@ -13030,9 +13030,11 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("live disk snapshot not supported with this " "QEMU binary")); - goto cleanup; + return -1; } + cfg = virQEMUDriverGetConfig(driver); + /* No way to roll back if first disk succeeds but later disks * fail, unless we have transaction support. * Based on earlier qemuDomainSnapshotPrepare, all -- 1.9.3
On 05/20/2014 07:36 AM, Peter Krempa wrote:
When doing an external checkpoint of a VM with no disk selected we'd return failure but not set error code. This was a result of ret not being set to 0 during walking of the disk array.
Rework early failure checking to avoid the problem.
Fixes the following symptom (or without --diskspec for diskless VMs)
$ virsh snapshot-create-as snapshot-test --memspec /tmp/asdf --diskspec hda,snapshot=no error: An error occurred, but the cause is unknown ---
Technically, this is equivalent to a 'virsh save'. Would it be better to avoid an error altogether and allow the operation as a longhand spelling of the same action? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/20/14 16:28, Eric Blake wrote:
On 05/20/2014 07:36 AM, Peter Krempa wrote:
When doing an external checkpoint of a VM with no disk selected we'd return failure but not set error code. This was a result of ret not being set to 0 during walking of the disk array.
Rework early failure checking to avoid the problem.
Fixes the following symptom (or without --diskspec for diskless VMs)
$ virsh snapshot-create-as snapshot-test --memspec /tmp/asdf --diskspec hda,snapshot=no error: An error occurred, but the cause is unknown ---
Technically, this is equivalent to a 'virsh save'. Would it be better to avoid an error altogether and allow the operation as a longhand spelling of the same action?
virsh save kills the VM after it finishes. This allows to preserve the VM running and also allows snapshots of diskless VMs. Peter
On 05/20/2014 08:29 AM, Peter Krempa wrote:
On 05/20/14 16:28, Eric Blake wrote:
On 05/20/2014 07:36 AM, Peter Krempa wrote:
When doing an external checkpoint of a VM with no disk selected we'd return failure but not set error code. This was a result of ret not being set to 0 during walking of the disk array.
Rework early failure checking to avoid the problem.
Fixes the following symptom (or without --diskspec for diskless VMs)
$ virsh snapshot-create-as snapshot-test --memspec /tmp/asdf --diskspec hda,snapshot=no error: An error occurred, but the cause is unknown ---
Technically, this is equivalent to a 'virsh save'. Would it be better to avoid an error altogether and allow the operation as a longhand spelling of the same action?
virsh save kills the VM after it finishes. This allows to preserve the VM running and also allows snapshots of diskless VMs.
Adding an option to 'virsh save' to not kill the guest doesn't make much sense in general, but might make sense for a diskless VM. Meanwhile, don't we already have the option to kill or keep the guest alive after an external memory snapshot? So I'm not quite sure where you are heading with this patch - is the idea that we WANT to allow an external checkpoint with no disks, and this is fixing a bug that gave an unknown error, or is this a case where we want to forbid diskless external checkpoints, and you are just improving the error message? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/20/2014 10:16 AM, Eric Blake wrote:
So I'm not quite sure where you are heading with this patch - is the idea that we WANT to allow an external checkpoint with no disks, and this is fixing a bug that gave an unknown error, or is this a case where we want to forbid diskless external checkpoints, and you are just improving the error message?
More concretely, I _want_ to allow a memory-only external snapshot (particularly of a diskless VM). If this patch is fixing a bogus error message into allowing that case, then please touch up the commit message to make that a bit more clear, and I'm fine with including the patch (my initial read was that this patch is just improving error message quality but still leaving it an error, but on reflection it looks like I mis-read the intent because of confusion about the commit message). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/20/14 18:21, Eric Blake wrote:
On 05/20/2014 10:16 AM, Eric Blake wrote:
So I'm not quite sure where you are heading with this patch - is the idea that we WANT to allow an external checkpoint with no disks, and this is fixing a bug that gave an unknown error, or is this a case where we want to forbid diskless external checkpoints, and you are just improving the error message?
More concretely, I _want_ to allow a memory-only external snapshot
Well, this patch exactly does that. That's why there's no "after" error message. The commit message isn't probably clear enough. How about changing one of the sentences to: Rework early failure checking and set the error code to success before iterating the array of disks so that we return success if no disks are snapshotted.
(particularly of a diskless VM). If this patch is fixing a bogus error message into allowing that case, then please touch up the commit message to make that a bit more clear, and I'm fine with including the patch (my initial read was that this patch is just improving error message quality but still leaving it an error, but on reflection it looks like I mis-read the intent because of confusion about the commit message).
Peter
On 05/20/2014 10:55 AM, Peter Krempa wrote:
How about changing one of the sentences to:
Rework early failure checking and set the error code to success before iterating the array of disks so that we return success if no disks are snapshotted.
Works for me. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/20/14 19:23, Eric Blake wrote:
Rework early failure checking and set the error code to success before
iterating the array of disks so that we return success if no disks are snapshotted.
Thanks; Series pushed with suggested fixes. Peter
On 05/20/2014 10:16 AM, Eric Blake wrote:
Adding an option to 'virsh save' to not kill the guest doesn't make much sense in general,
because memory state without matching disk state can't be used to resume a guest.
but might make sense for a diskless VM. Meanwhile, don't we already have the option to kill or keep the guest alive after an external memory snapshot?
Here, I was thinking of VIR_DOMAIN_SNAPSHOT_CREATE_HALT. Thus, 'virsh save $dom $file' appears to be shorthand for diskless 'virsh snapshot-create-as $dom --halt --memspec $file' -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/20/2014 03:35 PM, Peter Krempa wrote:
Peter Krempa (4): qemu: snapshot: Use typecasted switch in qemuDomainSnapshotPrepare() qemu: snapshot: Forbid partial internal snapshots qemu: snapshot: Forbid empty snapshots qemu: snapshot: Fix return value of external checkpoint with no disks
src/qemu/qemu_driver.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-)
ACK series. Jan
participants (3)
-
Eric Blake -
Ján Tomko -
Peter Krempa