[libvirt] [PATCH 1/2] snapshot: add REVERT_FORCE to API
Laine Stump
laine at laine.org
Tue Oct 4 19:24:17 UTC 2011
On 09/30/2011 02:52 PM, Eric Blake wrote:
> 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;
Are you not adding the FORCE flag the first time to allow for better
interoperability with older libvirtd?
That's the only question I had. It all looks fine - ACK.
> + 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>}]
>
More information about the libvir-list
mailing list