From 3c797404a520db95c12eaf4b83518ae5cb42d4e5 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 29 Sep 2011 17:52:06 -0600 Subject: [PATCH] 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. --- include/libvirt/libvirt.h.in | 1 + include/libvirt/virterror.h | 2 ++ src/libvirt.c | 22 ++++++++++++++++++++++ src/util/virterror.c | 6 ++++++ tools/virsh.c | 19 ++++++++++++++++++- tools/virsh.pod | 17 +++++++++++++++++ 6 files changed, 66 insertions(+), 1 deletion(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index bd7a0f7925..07617be489 100644 --- 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 0aae6229d1..0c98014b1e 100644 --- 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 9080b2f7bf..182e031f9c 100644 --- 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 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 26c4981b06..5006fa27ed 100644 --- 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 3a17971948..48f2b8af7d 100644 --- 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 be81afc470..1f7c76f3d2 100644 --- 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, if any. =item B I I [{I<--running> | I<--paused>}] +[I<--force>] Revert the given domain to the snapshot specified by I. 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 I I [I<--metadata>] [{I<--children> | I<--children-only>}]