[libvirt] [PATCHv3 0/5] snapshot revert-and-create
v2 was here: https://www.redhat.com/archives/libvir-list/2012-November/msg00818.html One patch from v2 has already been committed. This patch additionally adds a qemu implementation for the new flag, and I have tested creation of offline branches (I still need to test creation of a branch from a disk-only or online external snapshot). I've also started documenting my plans for a new revert FOLLOW flag, which updates to the current state of external files (rather than reverting completely to the point where the snapshot was taken); once that is working, then implementing the combination of createXML(LIVE|BRANCH) will really be creating the branch, then farming out to revert(FOLLOW) to switch over to the new branch. Eric Blake (5): snapshot: add revert-and-create branching of external snapshots snapshot: prepare to parse new XML snapshot: actually compute branch definition from XML snapshot: support revert-and-create branching in qemu snapshot: add another revert API flag docs/formatsnapshot.html.in | 16 ++++- docs/schemas/domainsnapshot.rng | 45 ++++++------ include/libvirt/libvirt.h.in | 6 ++ src/conf/snapshot_conf.c | 118 ++++++++++++++++++++++++++++--- src/conf/snapshot_conf.h | 2 + src/esx/esx_driver.c | 2 +- src/libvirt.c | 41 +++++++++-- src/qemu/qemu_driver.c | 50 +++++++++++-- src/vbox/vbox_tmpl.c | 2 +- tests/domainsnapshotxml2xmlin/branch.xml | 5 ++ tests/domainsnapshotxml2xmltest.c | 2 +- tools/virsh-snapshot.c | 44 +++++++++--- tools/virsh.pod | 25 ++++++- 13 files changed, 300 insertions(+), 58 deletions(-) create mode 100644 tests/domainsnapshotxml2xmlin/branch.xml -- 1.7.11.7
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, but losing the qcow2 files) 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. * 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. * tests/domainsnapshotxml2xmlin/branch.xml: New test file. --- docs/formatsnapshot.html.in | 16 +++++++++--- docs/schemas/domainsnapshot.rng | 45 ++++++++++++++++++-------------- include/libvirt/libvirt.h.in | 3 +++ src/libvirt.c | 27 +++++++++++++++---- tests/domainsnapshotxml2xmlin/branch.xml | 5 ++++ tools/virsh-snapshot.c | 40 ++++++++++++++++++++++------ tools/virsh.pod | 16 ++++++++++-- 7 files changed, 114 insertions(+), 38 deletions(-) create mode 100644 tests/domainsnapshotxml2xmlin/branch.xml diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index 8fcc04c..8350cb0 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -188,9 +188,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. Use of this element must be + paired with the <code>VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH</code> + flag during creation, and it is not present on output. When + creating a branch snapshot, the same set of <code>disks</code> + must be external, and <code>memory</code> must be omitted (it + will be 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..37910d8 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -31,27 +31,32 @@ </element> </optional> <optional> - <element name='memory'> - <choice> - <attribute name='snapshot'> - <choice> - <value>no</value> - <value>internal</value> - </choice> - </attribute> - <group> - <optional> - <attribute name='snapshot'> - <value>external</value> - </attribute> - </optional> - <attribute name='file'> - <ref name='absFilePath'/> + <choice> + <element name='branch'> + <text/> + </element> + <element name='memory'> + <choice> + <attribute name='snapshot'> + <choice> + <value>no</value> + <value>internal</value> + </choice> </attribute> - </group> - </choice> - <empty/> - </element> + <group> + <optional> + <attribute name='snapshot'> + <value>external</value> + </attribute> + </optional> + <attribute name='file'> + <ref name='absFilePath'/> + </attribute> + </group> + </choice> + <empty/> + </element> + </choice> </optional> <optional> <element name='disks'> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 84dcde1..be16629 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3777,6 +3777,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 4af6089..3229133 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -17746,9 +17746,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 @@ -17787,6 +17785,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. @@ -17843,9 +17850,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; } @@ -17863,6 +17872,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; diff --git a/tests/domainsnapshotxml2xmlin/branch.xml b/tests/domainsnapshotxml2xmlin/branch.xml new file mode 100644 index 0000000..1e35dc7 --- /dev/null +++ b/tests/domainsnapshotxml2xmlin/branch.xml @@ -0,0 +1,5 @@ +<domainsnapshot> + <name>my snap name</name> + <description>!@#$%^</description> + <branch>other</branch> +</domainsnapshot> diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 398730c..6232ec2 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -121,7 +121,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")}, @@ -129,6 +130,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} }; @@ -159,6 +161,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) @@ -307,6 +311,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, @@ -323,6 +331,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; @@ -341,22 +350,37 @@ 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, _("branch 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) goto cleanup; - if (vshCommandOptString(cmd, "name", &name) < 0 || - vshCommandOptString(cmd, "description", &desc) < 0) { - vshError(ctl, _("argument must not be empty")); + if (vshCommandOptString(cmd, "name", &name) < 0) { + vshError(ctl, _("name argument must not be empty")); + goto cleanup; + } + if (vshCommandOptString(cmd, "description", &desc) < 0) { + vshError(ctl, _("description argument must not be empty")); goto cleanup; } 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 c4d505f..38c691e 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2594,7 +2594,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 @@ -2651,6 +2651,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 @@ -2659,7 +2665,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 @@ -2675,6 +2682,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
No semantic change, but prepare for a new mode of parsing where a new _BRANCH flag requests that the parse look up the existing snapshot to branch from. * src/conf/snapshot_conf.h (VIR_DOMAIN_SNAPSHOT_PARSE_BRANCH): New flag, unused for now. (virDomainSnapshotDefParseString): Add parameter. * src/conf/snapshot_conf.c (virDomainSnapshotDefParseString): Update signature. * src/esx/esx_driver.c (esxDomainSnapshotCreateXML): Update caller. * src/qemu/qemu_driver.c (qemuDomainSnapshotLoad): Likewise. * src/vbox/vbox_tmpl.c (vboxDomainSnapshotCreateXML): Likewise. * tests/domainsnapshotxml2xmltest.c (testCompareXMLToXMLFiles): Likewise. --- src/conf/snapshot_conf.c | 8 +++++--- src/conf/snapshot_conf.h | 2 ++ src/esx/esx_driver.c | 2 +- src/qemu/qemu_driver.c | 3 ++- src/vbox/vbox_tmpl.c | 2 +- tests/domainsnapshotxml2xmltest.c | 2 +- 6 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 72bdd30..8a3146f 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -162,14 +162,16 @@ cleanup: return ret; } -/* flags is bitwise-or of virDomainSnapshotParseFlags. - * If flags does not include VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE, then - * caps and expectedVirtTypes are ignored. +/* flags is bitwise-or of virDomainSnapshotParseFlags. If flags does + * not include VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE, then 'caps' and + * 'expectedVirtTypes' are ignored. If flags does not include + * VIR_DOMAIN_SNAPSHOT_PARSE_BRANCH, then 'snapshots' is ignored. */ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, virCapsPtr caps, unsigned int expectedVirtTypes, + virDomainSnapshotObjListPtr snapshots ATTRIBUTE_UNUSED, unsigned int flags) { xmlXPathContextPtr ctxt = NULL; diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index b0f8760..c06f3b3 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -97,11 +97,13 @@ typedef enum { VIR_DOMAIN_SNAPSHOT_PARSE_DISKS = 1 << 1, VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL = 1 << 2, VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE = 1 << 3, + VIR_DOMAIN_SNAPSHOT_PARSE_BRANCH = 1 << 4, } virDomainSnapshotParseFlags; virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, virCapsPtr caps, unsigned int expectedVirtTypes, + virDomainSnapshotObjListPtr snapshots, unsigned int flags); void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def); char *virDomainSnapshotDefFormat(const char *domain_uuid, diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 56f31bb..3c869d7 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4267,7 +4267,7 @@ esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, return NULL; } - def = virDomainSnapshotDefParseString(xmlDesc, NULL, 0, 0); + def = virDomainSnapshotDefParseString(xmlDesc, NULL, 0, NULL, 0); if (def == NULL) { return NULL; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d4cafcc..f5bbc52 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -509,7 +509,7 @@ qemuDomainSnapshotLoad(void *payload, def = virDomainSnapshotDefParseString(xmlStr, qemu_driver->caps, QEMU_EXPECTED_VIRT_TYPES, - flags); + NULL, flags); if (def == NULL) { /* Nothing we can do here, skip this one */ VIR_ERROR(_("Failed to parse snapshot XML from file '%s'"), @@ -11511,6 +11511,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, if (!(def = virDomainSnapshotDefParseString(xmlDesc, driver->caps, QEMU_EXPECTED_VIRT_TYPES, + vm->snapshots, parse_flags))) goto cleanup; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index bcffb2f..4fd0505 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -5929,7 +5929,7 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, /* VBox has no snapshot metadata, so this flag is trivial. */ virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL); - if (!(def = virDomainSnapshotDefParseString(xmlDesc, NULL, 0, 0))) + if (!(def = virDomainSnapshotDefParseString(xmlDesc, NULL, 0, NULL, 0))) goto cleanup; if (def->ndisks) { diff --git a/tests/domainsnapshotxml2xmltest.c b/tests/domainsnapshotxml2xmltest.c index 84278d6..2c631c3 100644 --- a/tests/domainsnapshotxml2xmltest.c +++ b/tests/domainsnapshotxml2xmltest.c @@ -35,7 +35,7 @@ testCompareXMLToXMLFiles(const char *inxml, const char *uuid, int internal) flags |= VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL; if (!(def = virDomainSnapshotDefParseString(inXmlData, driver.caps, QEMU_EXPECTED_VIRT_TYPES, - flags))) + NULL, flags))) goto fail; if (!(actual = virDomainSnapshotDefFormat(uuid, def, -- 1.7.11.7
When asked to parse a branch snapshot XML definition, we have to piece together the definition of the new snapshot from parts of the branch point, as well as run some sanity checks that the branches are compatible. This patch is rather restrictive in what it allows; depending on effort and future needs, we may be able to relax some of those restrictions and allow more cases. * src/conf/snapshot_conf.c (virDomainSnapshotDefParseString): Honor new flag. --- src/conf/snapshot_conf.c | 112 ++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 106 insertions(+), 6 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 8a3146f..909a750 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -171,7 +171,7 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, virCapsPtr caps, unsigned int expectedVirtTypes, - virDomainSnapshotObjListPtr snapshots ATTRIBUTE_UNUSED, + virDomainSnapshotObjListPtr snapshots, unsigned int flags) { xmlXPathContextPtr ctxt = NULL; @@ -181,6 +181,7 @@ virDomainSnapshotDefParseString(const char *xmlStr, xmlNodePtr *nodes = NULL; int i; char *creation = NULL, *state = NULL; + char *branch = NULL; struct timeval tv; int active; char *tmp; @@ -188,6 +189,7 @@ virDomainSnapshotDefParseString(const char *xmlStr, char *memorySnapshot = NULL; char *memoryFile = NULL; bool offline = !!(flags & VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE); + virDomainSnapshotObjPtr other = NULL; xml = virXMLParseCtxt(NULL, xmlStr, _("(domain_snapshot)"), &ctxt); if (!xml) { @@ -223,7 +225,84 @@ virDomainSnapshotDefParseString(const char *xmlStr, def->description = virXPathString("string(./description)", ctxt); - if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { + branch = virXPathString("string(./branch)", ctxt); + memorySnapshot = virXPathString("string(./memory/@snapshot)", ctxt); + memoryFile = virXPathString("string(./memory/@file)", ctxt); + + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_BRANCH) { + if ((flags & (VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE | + VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL)) || + !snapshots) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unexpected parse request")); + goto cleanup; + } + + /* In order to form a branch, we must find the existing + * snapshot, and it must be external. */ + if (!branch) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("use of branch flag requires <branch> element")); + goto cleanup; + } + other = virDomainSnapshotFindByName(snapshots, branch); + if (!other) { + virReportError(VIR_ERR_XML_ERROR, _("could not find branch '%s'"), + branch); + goto cleanup; + } + + if (!virDomainSnapshotIsExternal(other)) { + virReportError(VIR_ERR_XML_ERROR, + _("branch '%s' is not an external snapshot"), + branch); + goto cleanup; + } + if (!other->def->dom) { + virReportError(VIR_ERR_XML_ERROR, + _("branch '%s' lacks corresponding domain details"), + branch); + goto cleanup; + } + if (!(def->dom = virDomainDefCopy(caps, other->def->dom, true))) + goto cleanup; + + /* The new definition shares the same <parent>, <state>, and + * <domain> as what it is branching from. */ + def->creationTime = tv.tv_sec; + if (other->def->parent && + !(def->parent = strdup(other->def->parent))) { + virReportOOMError(); + goto cleanup; + } + def->state = other->def->state; + + /* Branches reuse existing memory files (with bookkeeping to + * only delete the file when the last snapshot reference is + * deleted). Thus, it is an error to explicitly request + * memory with a branch. */ + if (memorySnapshot || memoryFile) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("cannot specify <memory> and <branch> together")); + goto cleanup; + } + def->memory = other->def->memory; + if (other->def->file && + !(def->file = strdup(other->def->file))) { + virReportOOMError(); + goto cleanup; + } + + /* Any <disk> in the XML must be consistent with the branch + * point, and any <disk> not in the XML will be populated to + * match the branch; checked below. */ + } else if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { + if (branch) { + virReportError(VIR_ERR_XML_ERROR, + _("branch '%s' incompatible with redefine flag"), + branch); + goto cleanup; + } if (virXPathLongLong("string(./creationTime)", ctxt, &def->creationTime) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -275,11 +354,15 @@ virDomainSnapshotDefParseString(const char *xmlStr, VIR_WARN("parsing older snapshot that lacks domain"); } } else { + if (branch) { + virReportError(VIR_ERR_XML_ERROR, + _("branch '%s' requires use of branch flag"), + branch); + goto cleanup; + } def->creationTime = tv.tv_sec; } - memorySnapshot = virXPathString("string(./memory/@snapshot)", ctxt); - memoryFile = virXPathString("string(./memory/@file)", ctxt); if (memorySnapshot) { def->memory = virDomainSnapshotLocationTypeFromString(memorySnapshot); if (def->memory <= 0) { @@ -314,8 +397,10 @@ virDomainSnapshotDefParseString(const char *xmlStr, _("memory state cannot be saved with offline snapshot")); goto cleanup; } - def->file = memoryFile; - memoryFile = NULL; + if (memoryFile) { + def->file = memoryFile; + memoryFile = NULL; + } if ((i = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0) goto cleanup; @@ -335,6 +420,20 @@ virDomainSnapshotDefParseString(const char *xmlStr, _("unable to handle disk requests in snapshot")); goto cleanup; } + if (other) { + /* For now, we only allow branch snapshots of external snapshots. */ + if (virDomainSnapshotAlignDisks(def, + VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL, + true) < 0) + goto cleanup; + for (i = 0; i < def->ndisks; i++) + if (def->disks[i].snapshot != other->def->disks[i].snapshot) { + virReportError(VIR_ERR_XML_ERROR, + _("mismatch in snapshot mode for disk '%s'"), + def->disks[i].name); + goto cleanup; + } + } if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL) { if (virXPathInt("string(./active)", ctxt, &active) < 0) { @@ -353,6 +452,7 @@ cleanup: VIR_FREE(nodes); VIR_FREE(memorySnapshot); VIR_FREE(memoryFile); + VIR_FREE(branch); xmlXPathFreeContext(ctxt); if (ret == NULL) virDomainSnapshotDefFree(def); -- 1.7.11.7
First cut at allowing snapshot branch creation. For now, the code requires that the snapshot be created but not activated (thus leaving the user on the original branch); this is because activating the new branch will require shared code with revert. Of course, until we allow reverting to a snapshot branch, this functionality feels more like a write-only interface (we can create the snapshot but not use it); but one thing at a time. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): Support new flag. (qemuDomainSnapshotPrepare): Likewise. --- src/qemu/qemu_driver.c | 47 +++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f5bbc52..a17ab62 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10901,7 +10901,8 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, qemuDomainObjPrivatePtr priv = vm->privateData; if (def->state == VIR_DOMAIN_DISK_SNAPSHOT && - reuse && !qemuCapsGet(priv->caps, QEMU_CAPS_TRANSACTION)) { + reuse && !qemuCapsGet(priv->caps, QEMU_CAPS_TRANSACTION) && + !(*flags & VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("reuse is not supported with this QEMU binary")); goto cleanup; @@ -11015,7 +11016,8 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, if (external && !active) *flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; - if (def->state != VIR_DOMAIN_DISK_SNAPSHOT && active) { + if (def->state != VIR_DOMAIN_DISK_SNAPSHOT && active && + !(*flags & VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH)) { if (external == 1 || qemuCapsGet(priv->caps, QEMU_CAPS_TRANSACTION)) { *flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC; @@ -11464,7 +11466,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT | VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE | VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC | - VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, NULL); + VIR_DOMAIN_SNAPSHOT_CREATE_LIVE | + VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH, NULL); if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE) && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY)) { @@ -11473,12 +11476,15 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, return NULL; } - if (((flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) && + if (((flags & (VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | + VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH)) && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT)) || (flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) update_current = false; if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE; + else if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH) + parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_BRANCH; qemuDriverLock(driver); virUUIDFormat(domain->uuid, uuidstr); @@ -11519,7 +11525,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_LIVE && (!virDomainObjIsActive(vm) || def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL || - flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) { + flags & (VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | + VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH))) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("live snapshot creation is supported only " "with external checkpoints")); @@ -11636,6 +11643,22 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, align_match) < 0) goto cleanup; } + } else if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH) { + /* XXX For now, we require that the new branch is not current + * (getting that to work will require sharing code with + * snapshot revert for (re-)starting the domain with correct + * events). */ + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("branch snapshot creation does not yet support " + "the current flag")); + goto cleanup; + } + /* def->dom was already populated, and the disks aligned; but + * we still need to check that we can create the new disk + * wrappers. */ + if (qemuDomainSnapshotPrepare(vm, def, &flags) < 0) + goto cleanup; } else { /* Easiest way to clone inactive portion of vm->def is via * conversion in and back out of xml. */ @@ -11678,7 +11701,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, if (update_current) snap->def->current = true; if (vm->current_snapshot) { - if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) { + if (!(flags & (VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | + VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH))) { snap->def->parent = strdup(vm->current_snapshot->def->name); if (snap->def->parent == NULL) { virReportOOMError(); @@ -11699,6 +11723,17 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, /* XXX Should we validate that the redefined snapshot even * makes sense, such as checking that qemu-img recognizes the * snapshot name in at least one of the domain's disks? */ + } else if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH) { + /* Whether or not the domain is online or the snapshot was + * from a running state, we reuse the offline creation code to + * create the new qcow2 files. */ + bool reuse = !!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT); + + if (qemuDomainSnapshotCreateInactiveExternal(driver, vm, snap, + reuse) < 0) + goto cleanup; + /* XXX Figure out how to support _CURRENT by reusing code from + * revert to swap over to the just-created snapshot. */ } else if (virDomainObjIsActive(vm)) { if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY || snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { -- 1.7.11.7
Now that we can create external snapshot branches, we need to be able to switch between them. Add a new flag, which states that we will merely use the external files as-is (and assume that the user hasn't done any nasty hotplug or hotunplug of disks after that branch was created, since we don't have any way to track the guest ABI at a tip of a branch that we are about to leave). * include/libvirt/libvirt.h.in (VIR_DOMAIN_SNAPSHOT_REVERT_FOLLOW): New flag. * src/libvirt.c (virDomainRevertToSnapshot): Document it. * tools/virsh-snapshot.c (cmdDomainSnapshotRevert): Add --follow flag. * tools/virsh.pod (snapshot-revert): Document it. --- include/libvirt/libvirt.h.in | 3 +++ src/libvirt.c | 14 +++++++++++++- tools/virsh-snapshot.c | 4 ++++ tools/virsh.pod | 9 ++++++++- 4 files changed, 28 insertions(+), 2 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index be16629..0957300 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3897,6 +3897,9 @@ 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 */ + VIR_DOMAIN_SNAPSHOT_REVERT_FOLLOW = 1 << 3, /* Follow subsequent disk + state rather than point of + external snapshot */ } virDomainSnapshotRevertFlags; /* Revert the domain to a point-in-time snapshot. The diff --git a/src/libvirt.c b/src/libvirt.c index 3229133..d2447c3 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -18727,12 +18727,24 @@ error: * into an inactive state, so transient domains require the use of one * of these two flags. * + * In the case of external snapshots, it is also possible to resume from + * the state of the external disks as modified after the snapshot was + * originally taken, by adding VIR_DOMAIN_SNAPSHOT_REVERT_FOLLOW to + * @flags. In particular, this is the only way to switch between two + * external branches as created with VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH + * by virDomainSnapshotCreateXML(). + * * 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. Since the very nature of keeping snapshots * implies the intent to roll back state, no additional confirmation is - * normally required for these lossy effects. + * normally required for these lossy effects. Users are advised to + * pause or shut down a domain and create a final snapshot at the tip of + * that branch before using revert, if it may be desirable to return to + * that branch later on (especially true when the current execution + * descends from an external snapshot, to ensure pending I/O has been + * flushed to disk for later use by VIR_DOMAIN_SNAPSHOT_REVERT_FOLLOW). * * However, there are two particular situations where reverting will * be refused by default, and where @flags must include diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 6232ec2..fed728c 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -1696,6 +1696,8 @@ static const vshCmdOptDef opts_snapshot_revert[] = { {"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")}, + {"follow", VSH_OT_BOOL, 0, + N_("follow disk changes made since an external snapshot")}, {NULL, 0, 0, NULL} }; @@ -1714,6 +1716,8 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING; if (vshCommandOptBool(cmd, "paused")) flags |= VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED; + if (vshCommandOptBool(cmd, "follow")) + flags |= VIR_DOMAIN_SNAPSHOT_REVERT_FOLLOW; /* 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 diff --git a/tools/virsh.pod b/tools/virsh.pod index 38c691e..dc70e68 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2837,7 +2837,7 @@ Output the name of the parent snapshot, if any, for the given I<snapshot>, or for the current snapshot with I<--current>. =item B<snapshot-revert> I<domain> {I<snapshot> | I<--current>} -[{I<--running> | I<--paused>}] [I<--force>] +[{I<--running> | I<--paused>}] [I<--force>] [I<--follow>] Revert the given domain to the snapshot specified by I<snapshot>, or to the current snapshot with I<--current>. Be aware @@ -2870,6 +2870,13 @@ 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. +When reverting to an external snapshot, the I<--follow> flag specifies +to revert to the subsequent state contained in the external disks rather +than the state at the time the snapshot was taken. This works best when +paired with B<snapshot-create> with the I<--branch> flag, and where the +guest is shut down before reverting between branches, so as not to lose +in-flight guest I/O that was not flushed to disk before reverting. + =item B<snapshot-delete> I<domain> {I<snapshot> | I<--current>} [I<--metadata>] [{I<--children> | I<--children-only>}] -- 1.7.11.7
On 11/21/12 01:36, Eric Blake wrote:
v2 was here: https://www.redhat.com/archives/libvir-list/2012-November/msg00818.html
One patch from v2 has already been committed. This patch additionally adds a qemu implementation for the new flag, and I have tested creation of offline branches (I still need to test creation of a branch from a disk-only or online external snapshot). I've also started documenting my plans for a new revert FOLLOW flag, which updates to the current state of external files (rather than reverting completely to the point where the snapshot was taken); once that is working, then implementing the combination of createXML(LIVE|BRANCH) will really be creating the branch, then farming out to revert(FOLLOW) to switch over to the new branch.
Eric Blake (5): snapshot: add revert-and-create branching of external snapshots snapshot: prepare to parse new XML snapshot: actually compute branch definition from XML snapshot: support revert-and-create branching in qemu snapshot: add another revert API flag
Hi Eric, this code has been buried long enough so that it doesn't apply. I'm reviving my snapshot patches and would like to integrate this functionality too. If you happen to have the branch still in you repo, could you please push it somewhere so git can help me while rebasing. Thanks Peter
participants (2)
-
Eric Blake -
Peter Krempa