[libvirt] [RFC PATCH 1/n] snapshot: add revert-and-create branching of external snapshots
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) 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. 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. * 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 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. 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. </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>. </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'> + <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(). + * * 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")); + 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 -- 1.7.11.7
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
On 11/13/2012 08:26 AM, Peter Krempa wrote:
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
Hmm, if the guest is offline and you are branching away from an external snapshot, the disk image already contains the old state. But you are right that for an online guest, it might be worth having a way to save a checkpoint just before the revert - and this is true whether the point you are going to was an internal or an external snapshot (although for first implementation, I will only allow branching from an external snapshot). In fact, this is true even if we don't add the atomic revert-and-create operation - even a plain revert can abandon runtime state that might be nice to remember. My original idea was that specifying <branch> means that <memory> is ignored on the incoming <domainsnapshot> XML; but maybe I should instead state that the <memory> controls what to do with the branch-point being abandoned: snapshot=no says discard the runtime state (what revert has always done, even without the create operation merged in), and 'snapshot=internal name=...' or 'snapshot=external file=...' says to create a new checkpoint before abandoning running state (so the revert-and-create branch operation would actually be creating two snapshots at once). Or we could just say that this is the responsibility of management apps to avoid abandoning running state - anyone wanting to return to a running domain is responsible for first pausing and saving that domain as an independent operation, and then only use the revert-and-create from an offline or paused state. In a way, it would be easier to insist that branching only ever be done from an offline state; but transient domains throw a wrench in that idea (there is never an offline state for transient domains). Another thought - maybe snapshots should remember if the domain has ever been unpaused after the snapshot was first taken. That is, a snapshot starts life clean as long as the domain remains offline or paused, but is marked dirty the first time the guest is allowed to run (or if any changes are made to the domain definition, such as a hotplug); knowing if a snapshot is dirty would determine whether an additional snapshot is even needed before leaving that branch in a revert operation. Maybe virDomainRevertToSnapshot() needs a way to do a create-and-revert mode of operation; and then virDomainSnapshotCreateXML needs a create-revert-create operation. Perhaps it is time to introduce a new API to give better control over a snapshot being created before the revert, vs. the snapshot being created after the revert; although that would not be something that can be backported to older releases (sometimes there are interesting design trade-offs for what can be done without changing the .so entry points). I'll think about that some more as I continue writing patches. But for now, since our existing revert has always discarded runtime state (and left it to management apps to do their own snapshot prior to revert if they don't want to lose anything), keeping that as the status quo doesn't hurt.
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
Now done in a separate thread.
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.
For an online guest, if we allow the create-before-revert, then the solution is to revert to the checkpoint taken before we did the revert. But for an offline guest, we shouldn't need a magic snapshot at the end of a branch; so I still think we need a new mode to revert that says to revert to the changes beyond the external snapshot.
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).
Or use hard-links instead of copies, and let the file system do the reference counting for us :) But if we do that, _and_ we do the idea of create-revert-create for marking the old point prior to creating the new branch, then the user has to be able to specify the external memory file name of both snapshots. Lots of ideas; thankfully, I don't see a problem with adding features incrementally.
+++ 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)
Well, the phrase about 'create a new snapshot at the same time as the revert' applies to this patch, but the phrase about 'request to delete all invalidated snapshots' indeed goes better with yours. I'm okay if the documentation is slightly inaccurate as our patches converge, as long as the final documentation in 1.0.1 matches what we have together. So it may be best to just separate this into its own patch, to be applied after both our code approaches have been merged.
+++ b/docs/schemas/domainsnapshot.rng @@ -85,6 +85,11 @@ </element> </element> </optional> + <optional> + <element name='parent'>
Doesn't match the docs. s/parent/branch/
Oops. Proof that I need to add a test case for RNG validation.
+ * 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.
Actually, if we add a create-revert-create ability, then _LIVE would apply to the first create (the branch we are leaving). But it definitely doesn't work for the branch we are creating.
@@ -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.
Indeed, also a candidate for a final doc cleanup patch after both of our improvements are in.
@@ -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.
Copy and paste, so I'll also fix the point it copied from.
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.
In fact, the names we generate for --disk-only are generated by libvirt (snapshot_conf.c:virDomainSnapshotAlignDisks), not virsh; so if we come up with a reasonable approach for auto-naming an external memory snapshot, it should be done in the same place. But yes, an ability to autoname external snapshots would be a nice independent patch. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/14/12 01:47, Eric Blake wrote:
On 11/13/2012 08:26 AM, Peter Krempa wrote:
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
Hmm, if the guest is offline and you are branching away from an external snapshot, the disk image already contains the old state. But you are right that for an online guest, it might be worth having a way to save a checkpoint just before the revert - and this is true whether the point you are going to was an internal or an external snapshot (although for first implementation, I will only allow branching from an external snapshot). In fact, this is true even if we don't add the atomic revert-and-create operation - even a plain revert can abandon runtime state that might be nice to remember.
My original idea was that specifying <branch> means that <memory> is ignored on the incoming <domainsnapshot> XML; but maybe I should instead state that the <memory> controls what to do with the branch-point being abandoned: snapshot=no says discard the runtime state (what revert has always done, even without the create operation merged in), and 'snapshot=internal name=...' or 'snapshot=external file=...' says to create a new checkpoint before abandoning running state (so the revert-and-create branch operation would actually be creating two snapshots at once).
That looks like a reasonable option.
Or we could just say that this is the responsibility of management apps to avoid abandoning running state - anyone wanting to return to a running domain is responsible for first pausing and saving that domain as an independent operation, and then only use the revert-and-create from an offline or paused state. In a way, it would be easier to insist that branching only ever be done from an offline state; but transient domains throw a wrench in that idea (there is never an offline state for transient domains).
In that case we will need to hack that up in virsh anyways. But I think it makes a bit more sense to have that functionality in the management app as trying to do it in a way that could be limited in some aspect.
Another thought - maybe snapshots should remember if the domain has ever been unpaused after the snapshot was first taken. That is, a snapshot starts life clean as long as the domain remains offline or paused, but is marked dirty the first time the guest is allowed to run (or if any changes are made to the domain definition, such as a hotplug); knowing if a snapshot is dirty would determine whether an additional snapshot is even needed before leaving that branch in a revert operation.
Maybe virDomainRevertToSnapshot() needs a way to do a create-and-revert mode of operation; and then virDomainSnapshotCreateXML needs a create-revert-create operation. Perhaps it is time to introduce a new API to give better control over a snapshot being created before the revert, vs. the snapshot being created after the revert; although that would not be something that can be backported to older releases (sometimes there are interesting design trade-offs for what can be done without changing the .so entry points).
I'll think about that some more as I continue writing patches. But for now, since our existing revert has always discarded runtime state (and left it to management apps to do their own snapshot prior to revert if they don't want to lose anything), keeping that as the status quo doesn't hurt.
Ah yeah. I agree on this one at this point. I'm just making those points for what I'd like to see from a users perspective.
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
Now done in a separate thread.
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.
For an online guest, if we allow the create-before-revert, then the solution is to revert to the checkpoint taken before we did the revert.
Or as you said: Leave this stuff on the mgmt app. That seems a bit more reasonable for me now. (But it'd be great to add that as a single operation in virsh)
But for an offline guest, we shouldn't need a magic snapshot at the end of a branch; so I still think we need a new mode to revert that says to revert to the changes beyond the external snapshot.
In case of an offline guest, we will need to remember it was offline when we were leaving the branch (in case the previous snapshot was active/online) and from the other side we'd need to remember that the guest was started and the running state was discarded (and was started from an offline snapshot). Otherwise we might mess stuff up.
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).
Or use hard-links instead of copies, and let the file system do the reference counting for us :) But if we do that, _and_ we do the idea of create-revert-create for marking the old point prior to creating the new branch, then the user has to be able to specify the external memory file name of both snapshots.
Well but the specified path might be on other filesystem. Users are sometimes doing crazy things :) Hm that would make the snapshot XMLs even more complex. I think that at the start we should go for the basic functionality and leave the burden of doing this on the mgmt app.
Lots of ideas; thankfully, I don't see a problem with adding features incrementally.
+++ 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)
Well, the phrase about 'create a new snapshot at the same time as the revert' applies to this patch, but the phrase about 'request to delete all invalidated snapshots' indeed goes better with yours. I'm okay if the documentation is slightly inaccurate as our patches converge, as long as the final documentation in 1.0.1 matches what we have together. So it may be best to just separate this into its own patch, to be applied after both our code approaches have been merged.
Hm, yeah, that would also make sense, if we make it before the release so we don't end up with incomplete docs.
+++ b/docs/schemas/domainsnapshot.rng @@ -85,6 +85,11 @@ </element> </element> </optional> + <optional> + <element name='parent'>
Doesn't match the docs. s/parent/branch/
Oops. Proof that I need to add a test case for RNG validation.
+ * 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.
Actually, if we add a create-revert-create ability, then _LIVE would apply to the first create (the branch we are leaving). But it definitely doesn't work for the branch we are creating.
@@ -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.
Indeed, also a candidate for a final doc cleanup patch after both of our improvements are in.
@@ -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.
Copy and paste, so I'll also fix the point it copied from.
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.
In fact, the names we generate for --disk-only are generated by libvirt (snapshot_conf.c:virDomainSnapshotAlignDisks), not virsh; so if we come up with a reasonable approach for auto-naming an external memory snapshot, it should be done in the same place. But yes, an ability to autoname external snapshots would be a nice independent patch.
I was thinking about that quite a lot, but didn't figure any good enough option for the memory. We might save it where the disks are stored or create a special folder for that. But I didn't like any of them that much.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 11/09/2012 12:47 PM, 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)
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.
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.
Sorry, I don't quite understand well the revert-and-create operation for a new branch. For example there is a snapshot chain like baseimage->snap1->snap2->snap3 If we plan to go back to snap1, it will become as follows, is it right? baseimage-->snap1-->snap2-->snap3 \ snap1.1(new branch) If snap1.1 is a new snapshot taken of snap1, is it a live external disk snapshot or offline external disk snapshot? And if user wants to revert back to snap1, and finally he got snap1.1(), it doesn't look good. I am wondering whether we can ensure the snap1.1 is totally a replicate of snap1. Guannan
On 11/14/12 10:29, Guannan Ren wrote:
On 11/09/2012 12:47 PM, 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)
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.
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.
Sorry, I don't quite understand well the revert-and-create operation for a new branch. For example there is a snapshot chain like baseimage->snap1->snap2->snap3
If we plan to go back to snap1, it will become as follows, is it right? baseimage-->snap1-->snap2-->snap3 \ snap1.1(new branch)
If snap1.1 is a new snapshot taken of snap1, is it a live external disk snapshot or offline external disk snapshot?
That depends on what snap1 was originally. snap1.1 at the point of creation snap1 and snap1.1 will be identical, but they will eventually diverge when the machine is booted up in either of the branches.
And if user wants to revert back to snap1, and finally he got snap1.1(), it doesn't look good. I am wondering whether we can ensure the snap1.1 is totally a replicate of snap1.
It can't be a total duplicate of that one. The disk images have to be different otherwise this whole exercise wouldn't make sense.
Guannan
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 11/14/2012 07:06 PM, Peter Krempa wrote:
On 11/14/12 10:29, Guannan Ren wrote:
On 11/09/2012 12:47 PM, 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)
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.
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.
Sorry, I don't quite understand well the revert-and-create operation for a new branch. For example there is a snapshot chain like baseimage->snap1->snap2->snap3
If we plan to go back to snap1, it will become as follows, is it right? baseimage-->snap1-->snap2-->snap3 \ snap1.1(new branch)
If snap1.1 is a new snapshot taken of snap1, is it a live external disk snapshot or offline external disk snapshot?
That depends on what snap1 was originally. snap1.1 at the point of creation snap1 and snap1.1 will be identical, but they will eventually diverge when the machine is booted up in either of the branches.
Get it, but how to make the snapshot of snap1.1 that will be identical with snap1 if snap1 is live external disk snapshot originally. If we boot up the machine using snap1, then do the snapshot, the snap1.1 will be not totally identical as least I think.
On 11/14/2012 04:33 AM, Guannan Ren wrote:
For example there is a snapshot chain like baseimage->snap1->snap2->snap3
If we plan to go back to snap1, it will become as follows, is it right? baseimage-->snap1-->snap2-->snap3 \ snap1.1(new branch)
If snap1.1 is a new snapshot taken of snap1, is it a live external disk snapshot or offline external disk snapshot?
Right now, I'm only proposing the ability to branch from an external snapshot (whether disk-only or external checkpoint doesn't matter). So you are indeed correct that if I have used snapshot commands to create external snapshots leading to the chain: base <- snap1 <- snap2 and then request a revert-and-create of snap1.1 with snap1 as the branch point, then I will have: base <- snap1 <- snap2 \- snap1.1 with snap1.qcow2 and snap1.1.qcow2 BOTH having base as their backing file, but with the two files diverging in contents based on what the guest has executed in the meantime. Then, I'm also proposing a new operation for plain revert, which says to revert to the external snapshot PLUS its subsequent disk changes, which will let me pivot between the two branches by altering which qcow2 file we load when booting the guest.
That depends on what snap1 was originally. snap1.1 at the point of creation snap1 and snap1.1 will be identical, but they will eventually diverge when the machine is booted up in either of the branches.
Get it, but how to make the snapshot of snap1.1 that will be identical with snap1 if snap1 is live external disk snapshot originally. If we boot up the machine using snap1, then do the snapshot, the snap1.1 will be not totally identical as least I think.
We are not booting up the guest before creating the new snapshot. The new branched snapshot shares the same BASE files as the branch point. The parent of the branch snapshot is the same parent of the original snapshot we reverted to - not the reverted snapshot itself. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Guannan Ren -
Peter Krempa