Re: [libvirt] [PATCH 1/2] 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 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 3c7f278..7403a9a 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2736,6 +2736,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 0aae622..0c98014 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 2b2f6be..c2aabf7 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -16373,6 +16373,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. However, the very nature of keeping snapshots + * implies the intent to roll back state, so no additional confirmation is + * normally required for these 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 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 26c4981..5006fa2 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 655378c..4e79325 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -13571,6 +13571,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} }; @@ -13582,11 +13583,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; @@ -13602,7 +13611,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 7c91d75..dd60a64 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2002,6 +2002,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 @@ -2017,6 +2018,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>}]