snapshot: add REVERT_FORCE to API

Although reverting to a snapshot is a form of data loss, this is
normally expected.  However, there are two cases where additional
surprises (failure to run the reverted state, or a break in
connectivity to the domain) can come into play.  Requiring extra
acknowledgment in these cases will make it less likely that
someone can get into an unrecoverable state due to a default revert.

Also create a new error code, so users can distinguish when forcing
would make a difference, rather than having to blindly request force.

* include/libvirt/libvirt.h.in (VIR_DOMAIN_SNAPSHOT_REVERT_FORCE):
New flag.
* src/libvirt.c (virDomainRevertToSnapshot): Document it.
* include/libvirt/virterror.h (VIR_ERR_SNAPSHOT_REVERT_RISKY): New
error value.
* src/util/virterror.c (virErrorMsg): Implement it.
* tools/virsh.c (cmdDomainSnapshotRevert): Add --force to virsh.
* tools/virsh.pod (snapshot-revert): Document it.

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index bd7a0f7925b4f1ab5816bd6fc021890eb21e1cf8..07617be489f30576b0e60bec261f3501ba868f97 100644 (file)
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -2723,6 +2723,7 @@ virDomainSnapshotPtr virDomainSnapshotGetParent(virDomainSnapshotPtr snapshot,
 typedef enum {
     VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING = 1 << 0, /* Run after revert */
     VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED  = 1 << 1, /* Pause after revert */
+    VIR_DOMAIN_SNAPSHOT_REVERT_FORCE   = 1 << 2, /* Allow risky reverts */
 } virDomainSnapshotRevertFlags;
 
 /* Revert the domain to a point-in-time snapshot.  The

diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
index 0aae6229d1d9c9b3d8fad7c85881e9ddf4edc4a7..0c98014b1e5a023ac1e5875fa030f80d0d6fdbb8 100644 (file)
--- a/include/libvirt/virterror.h
+++ b/include/libvirt/virterror.h
@@ -237,6 +237,8 @@ typedef enum {
                                            the given driver */
     VIR_ERR_STORAGE_PROBE_FAILED = 75,  /* storage pool proble failed */
     VIR_ERR_STORAGE_POOL_BUILT = 76,    /* storage pool already built */
+    VIR_ERR_SNAPSHOT_REVERT_RISKY = 77, /* force was not requested for a
+                                           risky domain snapshot revert */
 } virErrorNumber;
 
 /**

diff --git a/src/libvirt.c b/src/libvirt.c
index 9080b2f7bf722bb2a8033e0df16a90356b331ea3..182e031f9ce43f257578bbdf434886228b4f9e95 100644 (file)
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -16262,6 +16262,28 @@ error:
  * into an inactive state, so transient domains require the use of one
  * of these two flags.
  *
+ * Reverting to any snapshot discards all configuration changes made since
+ * the last snapshot.  Additionally, reverting to a snapshot from a running
+ * domain is a form of data loss, since it discards whatever is in the
+ * guest's RAM at the time.  Since the very nature of keeping snapshots
+ * implies the intent to roll back state, no additional confirmation is
+ * normally required for these lossy effects.
+ *
+ * However, there are two particular situations where reverting will
+ * be refused by default, and where @flags must include
+ * VIR_DOMAIN_SNAPSHOT_REVERT_FORCE to acknowledge the risks.  1) Any
+ * attempt to revert to a snapshot that lacks the metadata to perform
+ * ABI compatibility checks (generally the case for snapshots that
+ * lack a full <domain> when listed by virDomainSnapshotGetXMLDesc(),
+ * such as those created prior to libvirt 0.9.5).  2) Any attempt to
+ * revert a running domain to an active state that requires starting a
+ * new hypervisor instance rather than reusing the existing hypervisor
+ * (since this would terminate all connections to the domain, such as
+ * such as VNC or Spice graphics) - this condition arises from active
+ * snapshots that are provably ABI incomaptible, as well as from
+ * inactive snapshots with a @flags request to start the domain after
+ * the revert.
+ *
  * Returns 0 if the creation is successful, -1 on error.
  */
 int

diff --git a/src/util/virterror.c b/src/util/virterror.c
index 26c4981b067c9681c77821ffff7a6c4e0731f242..5006fa27ede74d1ac140cda9141f327855ef4a5f 100644 (file)
--- a/src/util/virterror.c
+++ b/src/util/virterror.c
@@ -1204,6 +1204,12 @@ virErrorMsg(virErrorNumber error, const char *info)
             else
                 errmsg = _("argument unsupported: %s");
             break;
+        case VIR_ERR_SNAPSHOT_REVERT_RISKY:
+            if (info == NULL)
+                errmsg = _("revert requires force");
+            else
+                errmsg = _("revert requires force: %s");
+            break;
     }
     return (errmsg);
 }

diff --git a/tools/virsh.c b/tools/virsh.c
index 3a17971948d4811f5e6a39cb384830413d143829..48f2b8af7dcba951d8dad774b6d4021ab5a4b22e 100644 (file)
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -13446,6 +13446,7 @@ static const vshCmdOptDef opts_snapshot_revert[] = {
     {"snapshotname", VSH_OT_DATA, VSH_OFLAG_REQ, N_("snapshot name")},
     {"running", VSH_OT_BOOL, 0, N_("after reverting, change state to running")},
     {"paused", VSH_OT_BOOL, 0, N_("after reverting, change state to paused")},
+    {"force", VSH_OT_BOOL, 0, N_("try harder on risky reverts")},
     {NULL, 0, 0, NULL}
 };
 
@@ -13457,11 +13458,19 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd)
     const char *name = NULL;
     virDomainSnapshotPtr snapshot = NULL;
     unsigned int flags = 0;
+    bool force = false;
+    int result;
 
     if (vshCommandOptBool(cmd, "running"))
         flags |= VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING;
     if (vshCommandOptBool(cmd, "paused"))
         flags |= VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED;
+    /* We want virsh snapshot-revert --force to work even when talking
+     * to older servers that did the unsafe revert by default but
+     * reject the flag, so we probe without the flag, and only use it
+     * when the error says it will make a difference.  */
+    if (vshCommandOptBool(cmd, "force"))
+        force = true;
 
     if (!vshConnectionUsability(ctl, ctl->conn))
         goto cleanup;
@@ -13477,7 +13486,15 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd)
     if (snapshot == NULL)
         goto cleanup;
 
-    if (virDomainRevertToSnapshot(snapshot, flags) < 0)
+    result = virDomainRevertToSnapshot(snapshot, flags);
+    if (result < 0 && force &&
+        last_error->code == VIR_ERR_SNAPSHOT_REVERT_RISKY) {
+        flags |= VIR_DOMAIN_SNAPSHOT_REVERT_FORCE;
+        virFreeError(last_error);
+        last_error = NULL;
+        result = virDomainRevertToSnapshot(snapshot, flags);
+    }
+    if (result < 0)
         goto cleanup;
 
     ret = true;

diff --git a/tools/virsh.pod b/tools/virsh.pod
index be81afc47024c1cd2ef912f1e39c68ae1fa54786..1f7c76f3d2ea7778eca820533e219c7930dea700 100644 (file)
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -1995,6 +1995,7 @@ Using I<--security-info> will also include security sensitive information.
 Output the name of the parent snapshot for the given I<snapshot>, if any.
 
 =item B<snapshot-revert> I<domain> I<snapshot> [{I<--running> | I<--paused>}]
+[I<--force>]
 
 Revert the given domain to the snapshot specified by I<snapshot>.  Be aware
 that this is a destructive action; any changes in the domain since the last
@@ -2010,6 +2011,22 @@ I<--running> or I<--paused> flag will perform additional state changes
 transient domains cannot be inactive, it is required to use one of these
 flags when reverting to a disk snapshot of a transient domain.
 
+There are two cases where a snapshot revert involves extra risk, which
+requires the use of I<--force> to proceed.  One is the case of a
+snapshot that lacks full domain information for reverting
+configuration (such as snapshots created prior to libvirt 0.9.5);
+since libvirt cannot prove that the current configuration matches what
+was in use at the time of the snapshot, supplying I<--force> assures
+libvirt that the snapshot is compatible with the current configuration
+(and if it is not, the domain will likely fail to run).  The other is
+the case of reverting from a running domain to an active state where a
+new hypervisor has to be created rather than reusing the existing
+hypervisor, because it implies drawbacks such as breaking any existing
+VNC or Spice connections; this condition happens with an active
+snapshot that uses a provably incompatible configuration, as well as
+with an inactive snapshot that is combined with the I<--start> or
+I<--pause> flag.
+
 =item B<snapshot-delete> I<domain> I<snapshot> [I<--metadata>]
 [{I<--children> | I<--children-only>}]