[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