snapshot: add atomic create flag

Right now, it is appallingly easy to cause qemu disk snapshots
to alter a domain then fail; for example, by requesting a two-disk
snapshot where the second disk name resides on read-only storage.
In this failure scenario, libvirt reports failure, but modifies
the live domain XML in-place to record that the first disk snapshot
was taken; and places a difficult burden on the management app
to grab the XML and reparse it to see which disks, if any, were
altered by the partial snapshot.

This patch adds a new flag where implementations can request that
the hypervisor make snapshots atomically; either no changes to
XML occur, or all disks were altered as a group.  If you request
the flag, you either get outright failure up front, or you take
advantage of hypervisor abilities to make an atomic snapshot. Of
course, drivers should prefer the atomic means even without the
flag explicitly requested.

There's no way to make snapshots 100% bulletproof - even if the
hypervisor does it perfectly atomic, we could run out of memory
during the followup tasks of updating our in-memory XML, and report
a failure.  However, these sorts of catastrophic failures are rare
and unlikely, and it is still nicer to know that either all
snapshots happened or none of them, as that is an easier state to
recover from.

* include/libvirt/libvirt.h.in
(VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC): New flag.
* src/libvirt.c (virDomainSnapshotCreateXML): Document it.
* tools/virsh.c (cmdSnapshotCreate, cmdSnapshotCreateAs): Expose it.
* tools/virsh.pod (snapshot-create, snapshot-create-as): Document
it.

include/libvirt/libvirt.h.in

@@ -3301,6 +3301,8 @@ typedef enum {
                                                           quiesce all mounted
                                                           file systems within
                                                           the domain */
+    VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC      = (1 << 7), /* atomically avoid
+                                                          partial changes */
 } virDomainSnapshotCreateFlags;
 
 /* Take a snapshot of the current VM state */

src/libvirt.c

@@ -17107,14 +17107,21 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot)
  * @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT, then existing
  * destination files are instead truncated and reused.
  *
- * Returns an (opaque) virDomainSnapshotPtr on success, NULL on failure.
  * Be aware that although libvirt prefers to report errors up front with
- * no other effect, there are certain types of failures where a failure
- * can occur even though the guest configuration was changed (for
- * example, if a disk snapshot request over two disks only fails on the
- * second disk, leaving the first disk altered); so after getting a NULL
- * return, it can be wise to use virDomainGetXMLDesc() to determine if
- * any partial changes occurred.
+ * no other effect, some hypervisors have certain types of failures where
+ * the overall command can easily fail even though the guest configuration
+ * was partially altered (for example, if a disk snapshot request for two
+ * disks fails on the second disk, but the first disk alteration cannot be
+ * rolled back).  If this API call fails, it is therefore normally
+ * necessary to follow up with virDomainGetXMLDesc() and check each disk
+ * to determine if any partial changes occurred.  However, if @flags
+ * contains VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC, then libvirt guarantees
+ * that this command will not alter any disks unless the entire set of
+ * changes can be done atomically, making failure recovery simpler (note
+ * that it is still possible to fail after disks have changed, but only
+ * in the much rarer cases of running out of memory or disk space).
+ *
+ * Returns an (opaque) virDomainSnapshotPtr on success, NULL on failure.
  */
 virDomainSnapshotPtr
 virDomainSnapshotCreateXML(virDomainPtr domain,

tools/virsh.c

@@ -15715,6 +15715,7 @@ static const vshCmdOptDef opts_snapshot_create[] = {
     {"disk-only", VSH_OT_BOOL, 0, N_("capture disk state but not vm state")},
     {"reuse-external", VSH_OT_BOOL, 0, N_("reuse any existing external files")},
     {"quiesce", VSH_OT_BOOL, 0, N_("quiesce guest's file systems")},
+    {"atomic", VSH_OT_BOOL, 0, N_("require atomic operation")},
     {NULL, 0, 0, NULL}
 };
 
@@ -15741,6 +15742,8 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd)
         flags |= VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT;
     if (vshCommandOptBool(cmd, "quiesce"))
         flags |= VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE;
+    if (vshCommandOptBool(cmd, "atomic"))
+        flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC;
 
     if (!vshConnectionUsability(ctl, ctl->conn))
         goto cleanup;
@@ -15851,6 +15854,7 @@ static const vshCmdOptDef opts_snapshot_create_as[] = {
     {"disk-only", VSH_OT_BOOL, 0, N_("capture disk state but not vm state")},
     {"reuse-external", VSH_OT_BOOL, 0, N_("reuse any existing external files")},
     {"quiesce", VSH_OT_BOOL, 0, N_("quiesce guest's file systems")},
+    {"atomic", VSH_OT_BOOL, 0, N_("require atomic operation")},
     {"diskspec", VSH_OT_ARGV, 0,
      N_("disk attributes: disk[,snapshot=type][,driver=type][,file=name]")},
     {NULL, 0, 0, NULL}
@@ -15878,6 +15882,8 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd)
         flags |= VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT;
     if (vshCommandOptBool(cmd, "quiesce"))
         flags |= VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE;
+    if (vshCommandOptBool(cmd, "atomic"))
+        flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC;
 
     if (!vshConnectionUsability(ctl, ctl->conn))
         goto cleanup;

tools/virsh.pod

@@ -2286,7 +2286,7 @@ used to represent properties of snapshots.
 
 =item B<snapshot-create> I<domain> [I<xmlfile>] {[I<--redefine> [I<--current>]]
 | [I<--no-metadata>] [I<--halt>] [I<--disk-only>] [I<--reuse-external>]
-[I<--quiesce>]}
+[I<--quiesce>] [I<--atomic>]}
 
 Create a snapshot for domain I<domain> with the properties specified in
 I<xmlfile>.  Normally, the only properties settable for a domain snapshot
@@ -2333,6 +2333,12 @@ to freeze and unfreeze domain's mounted file systems. However,
 if domain has no guest agent, snapshot creation will fail.
 Currently, this requires I<--disk-only> to be passed as well.
 
+If I<--atomic> is specified, libvirt will guarantee that the snapshot
+either succeeds, or fails with no changes; not all hypervisors support
+this.  If this flag is not specified, then some hypervisors may fail
+after partially performing the action, and B<dumpxml> must be used to
+see whether any partial changes occurred.
+
 Existence of snapshot metadata will prevent attempts to B<undefine>
 a persistent domain.  However, for transient domains, snapshot
 metadata is silently lost when the domain quits running (whether
@@ -2340,7 +2346,7 @@ by command such as B<destroy> or by internal guest action).
 
 =item B<snapshot-create-as> I<domain> {[I<--print-xml>]
 | [I<--no-metadata>] [I<--halt>] [I<--reuse-existing>]} [I<name>]
-[I<description>] [I<--disk-only> [I<--quiesce>]
+[I<description>] [I<--disk-only> [I<--quiesce>] [I<--atomic>]
 [[I<--diskspec>] B<diskspec>]...]
 
 Create a snapshot for domain I<domain> with the given <name> and
@@ -2380,6 +2386,12 @@ treat the snapshot as current, and cannot revert to the snapshot
 unless B<snapshot-create> is later used to teach libvirt about the
 metadata again).  This flag is incompatible with I<--print-xml>.
 
+If I<--atomic> is specified, libvirt will guarantee that the snapshot
+either succeeds, or fails with no changes; not all hypervisors support
+this.  If this flag is not specified, then some hypervisors may fail
+after partially performing the action, and B<dumpxml> must be used to
+see whether any partial changes occurred.
+
 =item B<snapshot-current> I<domain> {[I<--name>] | [I<--security-info>]
 | [I<snapshotname>]}