[libvirt] [RFC PATCH 1/n] snapshot: add revert-and-create branching of external snapshots
Peter Krempa
pkrempa at redhat.com
Tue Nov 13 15:26:15 UTC 2012
On 11/09/12 05:47, Eric Blake wrote:
> Right now, libvirt refuses to revert to the state at the time
> of an external snapshot, because doing so would reset things so
> that the next time the domain boots, we are using the backing
> file; but modifying the backing file invalidates all qcow2
> files that are based on top of it. There are three possibilities
> for lifting this restriction:
> 1. delete all snapshot metadata and qcow2 files that are
> invalidated by the revert (losing changes since the snapshot)
> 2. perform a block commit (such as with qemu-img commit) to
> merge the qcow2 file back into the backing file (keeping the
> changes since the snapshot)
> 3. create NEW qcow2 files that wrap the same base file as
> the original snapshot (keeping the changes since the original
> snapshot)
We will need to think of a mechanism how to return to a branch of the
snapshot tree that was previously left by this revert-and-branch
operation. Otherwise you will need either to:
1) create a new snapshot on the previous branch prior leaving it
2) if your machine was running when you were leaving your branch, it
will need to be reverted to the previous snapshot state
3) we will need to add a ability to add a automagic temporary snapshot
that will not create new disk overlays but will store the memory image
to allow reverting to the end of the branch
>
> This patch documents the API for option 3, by adding a new flag
> to virDomainSnapshotCreateXML (the only snapshot-related function
> that takes XML, which is required to pass in new file names
> during the branch), and wires up virsh to expose it. Later
> patches will enhance virDomainRevertToSnapshot to cover options
> 1 and 2.
For option 1 I've sent patches to the list yesterday. It will need some
tuning but it works already.
>
> API wise, an application wishing to do the revert-and-create
> operation must add a <branch>name</branch> element to their
> <domainsnapshot> XML, and pass VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH
> in the flags argument. In virsh, snapshot-create gains a new
> boolean flag --branch that must match the XML passed in, and
> snapshot-create-as gains a new --branch=name argument along
> with a --current boolean for creating such XML.
Makes sense.
>
> * include/libvirt/libvirt.h.in
> (VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH): New flag.
> * src/libvirt.c (virDomainSnapshotCreateXML): Document it, and
> enforce some mutual exclusion.
> (virDomainRevertToSnapshot): Mention how to revert to an
> external snapshot without having to delete snapshots.
> * docs/formatsnapshot.html.in: Document the new <branch> element.
> * docs/schemas/domainsnapshot.rng: Allow new element.
> * tools/virsh-snapshot.c (cmdSnapshotCreate): Add --branch option.
> (cmdSnapshotCreateAs): Likewise, also add --current.
> * tools/virsh.pod (snapshot-create, snapshot-create-as): Document
> new usage.
> ---
>
> Sending this patch now, to make sure I'm on the right track. I
> have the following plans for the next few patches:
>
> 1.
> Enhance virDomainSnapshotListFlags to add two new filter groups:
> VIR_DOMAIN_SNAPSHOT_LIST_OFFLINE
> VIR_DOMAIN_SNAPSHOT_LIST_ONLINE
> VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY
>
> VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL
> VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL
> (and possibly VIR_DOMAIN_SNAPSHOT_LIST_MIXED if we change our stance
> and allow mixing internal and external in one snapshot)
>
> 2.
> Add a flag to virDomainSnapshotParseFlags in src/conf/snapshot_conf.h;
> when present, the new element is parsed, and appropriate elements of
> the in-memory snapshot representation are copied in from the existing
> external snapshot.
>
> 3.
> Implement the new flag in qemu_driver.c for creation. Note that
> branching works for both disk-only (whether offline or online) and
> checkpoints - the new snapshot will share the same memory (none or
> external) as the original, and all that really changes is the
> creation (or reuse) of new backing files.
>
> 4.
> Think about ramifications on revert - right now, with no options,
> revert means to go back to the state where the snapshot was created;
> but now that we allow the creation of branches, and since the branches
> have external disk state which persists even when not on the branch, we
> need a new revert flag that says to revert to an external snapshots
> and its subsequent changes
Ah yeah, this would be applicable. This would work if you shut down your
guest prior to leaving the branch (or can justify loosing data.) And if
you want to return to an existing state, you still can create another
snapshot. Although it would be cool to have it automatic.
>
> 5.
> Think about ramifications on delete - for example, if two branches
> of external snapshots share a common memory state file, and we delete
> only one of the two branches, the memory state file must not be
> deleted.
Or we could use the name provided in the new snapshot XML to store the
same memory image (although copying it might not be that cool, it would
save a ton of logic to wire that up).
>
> docs/formatsnapshot.html.in | 23 +++++++++++++++++++----
> docs/schemas/domainsnapshot.rng | 5 +++++
> include/libvirt/libvirt.h.in | 3 +++
> src/libvirt.c | 35 ++++++++++++++++++++++++++++++-----
> tools/virsh-snapshot.c | 31 ++++++++++++++++++++++++++-----
> tools/virsh.pod | 16 ++++++++++++++--
> 6 files changed, 97 insertions(+), 16 deletions(-)
>
> diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in
> index 8fcc04c..5018f41 100644
> --- a/docs/formatsnapshot.html.in
> +++ b/docs/formatsnapshot.html.in
> @@ -87,7 +87,12 @@
> sets that snapshot as current, and the prior current snapshot is
> the parent of the new snapshot. Branches in the hierarchy can
> be formed by reverting to a snapshot with a child, then creating
> - another snapshot.
> + another snapshot. In the case of external snapshots, modifying
> + the backing files would invalidate all external files that
> + depend on the backing file, so the action of reverting to a
> + snapshot must be accompanied by either a request to delete all
> + invalidated snapshots, or to create a new snapshot at the same
> + time as the revert.
Hm, this comment should probably be in my patch adding the revert
functionality for external snapshots. (With a few changes stating for
example that --force invalidates the image chain and doesn't care)
> </p>
> <p>
> The top-level <code>domainsnapshot</code> element may contain
> @@ -188,9 +193,19 @@
> </dd>
> <dt><code>parent</code></dt>
> <dd>The parent of this snapshot. If present, this element
> - contains exactly one child element, name. This specifies the
> - name of the parent snapshot of this snapshot, and is used to
> - represent trees of snapshots. Readonly.
> + contains exactly one child element, <code>name</code>. This
> + specifies the name of the parent snapshot of this snapshot,
> + and is used to represent trees of snapshots. Readonly.
> + </dd>
> + <dt><code>branch</code></dt>
> + <dd>The name of an existing external snapshot that forms the
> + branch point for this snapshot. Required when creating a
> + snapshot with
> + the <code>VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH</code> flag, and
> + not present on output. When creating a branch snapshot, the
> + same set of <code>disks</code> must be external,
> + and <code>memory</code> is copied from the branch
> + point. <span class="since">since 1.0.1</span>.
Sounds reasonable. Although introduces hassle for us for keeping track
of the memory image.
> </dd>
> <dt><code>domain</code></dt>
> <dd>The domain that this snapshot was taken against. Older
> diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng
> index 45d55b5..6721e8f 100644
> --- a/docs/schemas/domainsnapshot.rng
> +++ b/docs/schemas/domainsnapshot.rng
> @@ -85,6 +85,11 @@
> </element>
> </element>
> </optional>
> + <optional>
> + <element name='parent'>
Doesn't match the docs. s/parent/branch/
> + <text/>
> + </element>
> + </optional>
> </interleave>
> </element>
> </define>
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index bf584a0..53d21ce 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -3775,6 +3775,9 @@ typedef enum {
> VIR_DOMAIN_SNAPSHOT_CREATE_LIVE = (1 << 8), /* create the snapshot
> while the guest is
> running */
> + VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH = (1 << 9), /* specify an existing
> + external snapshot as
> + the branching point */
> } virDomainSnapshotCreateFlags;
>
> /* Take a snapshot of the current VM state */
> diff --git a/src/libvirt.c b/src/libvirt.c
> index bcb8233..e88effc 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -17747,9 +17747,7 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot)
> * normally fails if snapshot metadata still remains on the source
> * machine). When redefining snapshot metadata, the current snapshot
> * will not be altered unless the VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT
> - * flag is also present. It is an error to request the
> - * VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT flag without
> - * VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE. On some hypervisors,
> + * flag is also present. On some hypervisors,
> * redefining an existing snapshot can be used to alter host-specific
> * portions of the domain XML to be used during revert (such as
> * backing filenames associated with disk devices), but must not alter
> @@ -17788,6 +17786,15 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot)
> * is not present, an error is thrown. Moreover, this flag requires
> * VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY to be passed as well.
> *
> + * If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH, then @xmlDesc
> + * must describe an existing external snapshot as well as actions to
> + * create a new snapshot from the point where the existing snapshot was
> + * created rather than the current state of the guest. In this mode,
> + * the new snapshot branch is created but not activated unless the
> + * VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT flag is also present. It is an
> + * error to request VIR_DOMAIN_SNAPSHOT_CREATE_LIVE or
> + * VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE in this mode.
> + *
> * By default, if the snapshot involves external files, and any of the
> * destination files already exist as a non-empty regular file, the
> * snapshot is rejected to avoid losing contents of those files.
> @@ -17844,9 +17851,11 @@ virDomainSnapshotCreateXML(virDomainPtr domain,
> }
>
> if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT) &&
> - !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) {
> + !(flags & (VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE |
> + VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH))) {
> virReportInvalidArg(flags,
> - _("use of 'current' flag in %s requires 'redefine' flag"),
> + _("use of 'current' flag in %s requires 'redefine' "
> + "or 'branch' flag"),
> __FUNCTION__);
> goto error;
> }
> @@ -17864,6 +17873,14 @@ virDomainSnapshotCreateXML(virDomainPtr domain,
> __FUNCTION__);
> goto error;
> }
> + if (!!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) +
> + !!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_LIVE) +
> + !!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH) > 1) {
> + virReportInvalidArg(flags,
> + _("'redefine', 'live', and 'branch' flags in %s are mutually exclusive"),
> + __FUNCTION__);
> + goto error;
> + }
>
> if (conn->driver->domainSnapshotCreateXML) {
> virDomainSnapshotPtr ret;
> @@ -18673,6 +18690,14 @@ error:
> * inactive snapshots with a @flags request to start the domain after
> * the revert.
> *
> + * Reverting to a snapshot with external state and then running the
> + * domain would invalidate all external files that depend on the backing
> + * file. Therefore, this function will fail unless any child external
> + * snapshots of the revert point first been deleted or merged. It is
> + * also possible to effectively revert and create a new snapshot branch
> + * in one operation, without invalidating snapshots, by using the
> + * VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH flag of virDomainSnapshotCreateXML().
> + *
This also probably should go into the revert series.
> * Returns 0 if the creation is successful, -1 on error.
> */
> int
> diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
> index 4281109..3de9878 100644
> --- a/tools/virsh-snapshot.c
> +++ b/tools/virsh-snapshot.c
> @@ -120,7 +120,8 @@ static const vshCmdOptDef opts_snapshot_create[] = {
> {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
> {"xmlfile", VSH_OT_DATA, 0, N_("domain snapshot XML")},
> {"redefine", VSH_OT_BOOL, 0, N_("redefine metadata for existing snapshot")},
> - {"current", VSH_OT_BOOL, 0, N_("with redefine, set current snapshot")},
> + {"current", VSH_OT_BOOL, 0,
> + N_("with redefine or branch, set current snapshot")},
> {"no-metadata", VSH_OT_BOOL, 0, N_("take snapshot but create no metadata")},
> {"halt", VSH_OT_BOOL, 0, N_("halt domain after snapshot is created")},
> {"disk-only", VSH_OT_BOOL, 0, N_("capture disk state but not vm state")},
> @@ -128,6 +129,7 @@ static const vshCmdOptDef opts_snapshot_create[] = {
> {"quiesce", VSH_OT_BOOL, 0, N_("quiesce guest's file systems")},
> {"atomic", VSH_OT_BOOL, 0, N_("require atomic operation")},
> {"live", VSH_OT_BOOL, 0, N_("take a live snapshot")},
> + {"branch", VSH_OT_BOOL, 0, N_("create a branch snapshot")},
> {NULL, 0, 0, NULL}
> };
>
> @@ -158,6 +160,8 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd)
> flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC;
> if (vshCommandOptBool(cmd, "live"))
> flags |= VIR_DOMAIN_SNAPSHOT_CREATE_LIVE;
> + if (vshCommandOptBool(cmd, "branch"))
> + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH;
>
> dom = vshCommandOptDomain(ctl, cmd, NULL);
> if (dom == NULL)
> @@ -306,6 +310,10 @@ static const vshCmdOptDef opts_snapshot_create_as[] = {
> {"quiesce", VSH_OT_BOOL, 0, N_("quiesce guest's file systems")},
> {"atomic", VSH_OT_BOOL, 0, N_("require atomic operation")},
> {"live", VSH_OT_BOOL, 0, N_("take a live snapshot")},
> + {"branch", VSH_OT_DATA, VSH_OFLAG_REQ_OPT,
> + N_("name of external snapshot to use as a branch point")},
> + {"current", VSH_OT_BOOL, 0,
> + N_("with branch, make the new snapshot current")},
> {"memspec", VSH_OT_DATA, VSH_OFLAG_REQ_OPT,
> N_("memory attributes: [file=]name[,snapshot=type]")},
> {"diskspec", VSH_OT_ARGV, 0,
> @@ -322,6 +330,7 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd)
> const char *name = NULL;
> const char *desc = NULL;
> const char *memspec = NULL;
> + const char *branch = NULL;
> virBuffer buf = VIR_BUFFER_INITIALIZER;
> unsigned int flags = 0;
> const vshCmdOpt *opt = NULL;
> @@ -340,6 +349,19 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd)
> flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC;
> if (vshCommandOptBool(cmd, "live"))
> flags |= VIR_DOMAIN_SNAPSHOT_CREATE_LIVE;
> + if (vshCommandOptBool(cmd, "branch")) {
> + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH;
> + if (vshCommandOptString(cmd, "branch", &branch) < 0) {
> + vshError(ctl, _("argument must not be empty"));
This error message isn't very helpful when viewed out of context. Add
the parameter name to make it clear please.
> + goto cleanup;
> + }
> + if (vshCommandOptBool(cmd, "memspec")) {
> + vshError(ctl, _("--branch and --memspec are mutually exclusive"));
> + goto cleanup;
> + }
> + }
> + if (vshCommandOptBool(cmd, "current"))
> + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT;
>
> dom = vshCommandOptDomain(ctl, cmd, NULL);
> if (dom == NULL)
> @@ -352,10 +374,9 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd)
> }
>
> virBufferAddLit(&buf, "<domainsnapshot>\n");
> - if (name)
> - virBufferEscapeString(&buf, " <name>%s</name>\n", name);
> - if (desc)
> - virBufferEscapeString(&buf, " <description>%s</description>\n", desc);
> + virBufferEscapeString(&buf, " <name>%s</name>\n", name);
> + virBufferEscapeString(&buf, " <description>%s</description>\n", desc);
> + virBufferEscapeString(&buf, " <branch>%s</branch>\n", branch);
>
> if (vshCommandOptString(cmd, "memspec", &memspec) < 0 ||
> vshParseSnapshotMemspec(ctl, &buf, memspec) < 0) {
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 0808d72..a79ed50 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -2606,7 +2606,7 @@ used to represent properties of snapshots.
>
> =item B<snapshot-create> I<domain> [I<xmlfile>] {[I<--redefine> [I<--current>]]
> | [I<--no-metadata>] [I<--halt>] [I<--disk-only>] [I<--reuse-external>]
> -[I<--quiesce>] [I<--atomic>] [I<--live>]}
> +[I<--quiesce>] [I<--atomic>] [I<--live>] [I<--branch>]}
>
> Create a snapshot for domain I<domain> with the properties specified in
> I<xmlfile>. Normally, the only properties settable for a domain snapshot
> @@ -2663,6 +2663,12 @@ If I<--live> is specified, libvirt takes the snapshot while the guest is
> running. This increases the size of the memory image of the external
> checkpoint. This is currently supported only for external checkpoints.
>
> +If I<--branch> is specified, then I<xmlfile> must contain a B<branch>
> +element naming an existing external snapshot which the new snapshot
> +will branch from. In this usage, the new snapshot must also be
> +external, and will not be made current unless I<--current> is also
> +requested.
> +
> Existence of snapshot metadata will prevent attempts to B<undefine>
> a persistent domain. However, for transient domains, snapshot
> metadata is silently lost when the domain quits running (whether
> @@ -2671,7 +2677,8 @@ by command such as B<destroy> or by internal guest action).
> =item B<snapshot-create-as> I<domain> {[I<--print-xml>]
> | [I<--no-metadata>] [I<--halt>] [I<--reuse-external>]} [I<name>]
> [I<description>] [I<--disk-only> [I<--quiesce>]] [I<--atomic>]
> -[[I<--live>] [I<--memspec> B<memspec>]] [I<--diskspec>] B<diskspec>]...
> +[[I<--live>] [I<--memspec> B<memspec>]] [I<--branch> B<branch> [I<--current>]]
> +[I<--diskspec>] B<diskspec>]...
>
> Create a snapshot for domain I<domain> with the given <name> and
> <description>; if either value is omitted, libvirt will choose a
> @@ -2687,6 +2694,11 @@ by a B<memspec> of the form B<[file=]name[,snapshot=type]>, where
> type can be B<none>, B<internal>, or B<external>. To include a literal
> comma in B<file=name>, escape it with a second comma.
>
> +The I<--branch> option can be used to create an external snapshot that
> +branches from an existing snapshot named B<branch>. In this usage, the
> +new snapshot is not made current unless I<--current> is also present.
> +Specifying a branch point is incompatible with using I<--memspec>.
> +
> The I<--diskspec> option can be used to control how I<--disk-only> and
> external checkpoints create external files. This option can occur
> multiple times, according to the number of <disk> elements in the domain
>
Looks good. There are a few details we should sort out before doing this
but I think this is useful and the right way.
I was also thinking of enhancing virsh with a ability to generate memory
image names from the snapshot names when the user only provides the
prefix. This saves a lot of hassle when doing multiple snapshots in a
row with the "[up-arrow][enter]" approach.
Peter
More information about the libvir-list
mailing list