Re: [libvirt] [PATCH 1/4] snapshot: new XML for external system checkpoint

Each <domainsnapshot> can now contain an optional <memory> element that describes how the VM state was handled, similar to disk snapshots. The new element will always appear in output; for back-compat, an input that lacks the element will assume 'no' or 'internal' according to the domain state. Along with this change, it is now possible to pass <disks> in the XML for an offline snapshot; this also needs to be wired up in a future patch, to make it possible to choose internal vs. external on a per-disk basis for each disk in an offline domain. At that point, using the --disk-only flag for an offline domain will be able to work. For some examples below, remember that qemu supports the following snapshot actions: qemu-img: offline external and internal disk savevm: online internal VM and disk migrate: online external VM transaction: online external disk ===== <domainsnapshot> <memory snapshot='no'/> ... </domainsnapshot> implies that there is no VM state saved (mandatory for offline and disk-only snapshots, not possible otherwise); using qemu-img for offline domains and transaction for online. ===== <domainsnapshot> <memory snapshot='internal'/> ... </domainsnapshot> state is saved inside one of the disks (as in qemu's 'savevm' system checkpoint implementation). If needed in the future, we can also add an attribute pointing out _which_ disk saved the internal state; maybe disk='vda'. ===== <domainsnapshot> <memory snapshot='external' file='/path/to/state'/> ... </domainsnapshot> This is not wired up yet, but future patches will allow this to control a combination of 'virsh save /path/to/state' plus disk snapshots from the same point in time. ===== So for 0.10.2, I plan to implement this table of combinations, with '*' designating new code and '+' designating existing code reached through new combinations of xml and/or the existing DISK_ONLY flag:

domain memory disk disk-only | result ----------------------------------------- offline omit omit any | memory=no disk=int, via qemu-img offline no omit any |+memory=no disk=int, via qemu-img offline omit/no no any | invalid combination (nothing to snapshot) offline omit/no int any |+memory=no disk=int, via qemu-img offline omit/no ext any |*memory=no disk=ext, via qemu-img offline int/ext any any | invalid combination (no memory to save) online omit omit off | memory=int disk=int, via savevm online omit omit on | memory=no disk=default, via transaction online omit no/ext off | unsupported for now online omit no on | invalid combination (nothing to snapshot) online omit ext on | memory=no disk=ext, via transaction online omit int off |+memory=int disk=int, via savevm online omit int on | unsupported for now online no omit any |+memory=no disk=default, via transaction online no no any | invalid combination (nothing to snapshot) online no int any | unsupported for now online no ext any |+memory=no disk=ext, via transaction online int/ext any on | invalid combination (disk-only vs. memory) online int omit off |+memory=int disk=int, via savevm online int no/ext off | unsupported for now online int int off |+memory=int disk=int, via savevm online ext omit off |*memory=ext disk=default, via migrate+trans online ext no off |+memory=ext disk=no, via migrate online ext int off | unsupported for now online ext ext off |*memory=ext disk=ext, via migrate+transaction * docs/schemas/domainsnapshot.rng (memory): New RNG element. * docs/formatsnapshot.html.in: Document it. * src/conf/snapshot_conf.h (virDomainSnapshotDef): New fields. * src/conf/domain_conf.c (virDomainSnapshotDefFree) (virDomainSnapshotDefParseString, virDomainSnapshotDefFormat): Manage new fields. * tests/domainsnapshotxml2xmltest.c: New test. * tests/domainsnapshotxml2xmlin/*.xml: Update existing tests. * tests/domainsnapshotxml2xmlout/*.xml: Likewise. --- docs/formatsnapshot.html.in | 11 ++++ docs/schemas/domainsnapshot.rng | 23 ++++++++ src/conf/snapshot_conf.c | 66 +++++++++++++++++----- src/conf/snapshot_conf.h | 4 ++ tests/domainsnapshotxml2xmlin/external_vm.xml | 10 ++++ tests/domainsnapshotxml2xmlin/noparent.xml | 9 +++ tests/domainsnapshotxml2xmlout/all_parameters.xml | 1 + tests/domainsnapshotxml2xmlout/disk_snapshot.xml | 1 + tests/domainsnapshotxml2xmlout/external_vm.xml | 43 ++++++++++++++ tests/domainsnapshotxml2xmlout/full_domain.xml | 1 + tests/domainsnapshotxml2xmlout/metadata.xml | 1 + tests/domainsnapshotxml2xmlout/noparent.xml | 1 + .../noparent_nodescription.xml | 1 + .../noparent_nodescription_noactive.xml | 1 + tests/domainsnapshotxml2xmltest.c | 1 + 15 files changed, 161 insertions(+), 13 deletions(-) create mode 100644 tests/domainsnapshotxml2xmlin/external_vm.xml create mode 100644 tests/domainsnapshotxml2xmlin/noparent.xml create mode 100644 tests/domainsnapshotxml2xmlout/external_vm.xml diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index ec5ebf3..2af32bc 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -106,6 +106,17 @@ description is omitted when initially creating the snapshot, then this field will be empty. </dd> + <dt><code>memory</code></dt> + <dd>On input, this is an optional request for how to handle VM + state. For an offline domain or a disk-only snapshot, + attribute <code>snapshot</code> must be <code>no</code>, since + there is no VM state saved; otherwise, the attribute can + be <code>internal</code> if the VM state is piggy-backed with + other internal disk state, or <code>external</code> along with + a second attribute <code>file</code> giving the absolute path + of the file holding the VM state. <span class="since">Since + 0.10.2</span> + </dd> <dt><code>disks</code></dt> <dd>On input, this is an optional listing of specific instructions for disk snapshots; it is needed when making a diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index 0ef0631..230833d 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -31,6 +31,29 @@ </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'/> + </attribute> + </group> + </choice> + <empty/> + </element> + </optional> + <optional> <element name='disks'> <zeroOrMore> <ref name='disksnapshot'/> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index e13cdd6..7a8a564 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -95,6 +95,7 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def) VIR_FREE(def->name); VIR_FREE(def->description); VIR_FREE(def->parent); + VIR_FREE(def->file); for (i = 0; i < def->ndisks; i++) virDomainSnapshotDiskDefClear(&def->disks[i]); VIR_FREE(def->disks); @@ -174,6 +175,9 @@ virDomainSnapshotDefParseString(const char *xmlStr, int active; char *tmp; int keepBlanksDefault = xmlKeepBlanksDefault(0); + char *memorySnapshot = NULL; + char *memoryFile = NULL; + bool offline = !!(flags & VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE); xml = virXMLParseCtxt(NULL, xmlStr, _("(domain_snapshot)"), &ctxt); if (!xml) { @@ -200,15 +204,11 @@ virDomainSnapshotDefParseString(const char *xmlStr, virReportError(VIR_ERR_XML_ERROR, "%s", _("a redefined snapshot must have a name")); goto cleanup; - } else { - ignore_value(virAsprintf(&def->name, "%lld", - (long long)tv.tv_sec)); } - } - - if (def->name == NULL) { - virReportOOMError(); - goto cleanup; + if (virAsprintf(&def->name, "%lld", (long long)tv.tv_sec) < 0) { + virReportOOMError(); + goto cleanup; + } }

def->description = virXPathString("string(./description)", ctxt); @@ -239,6 +239,8 @@ virDomainSnapshotDefParseString(const char *xmlStr, state); goto cleanup; } + offline = (def->state == VIR_DOMAIN_SHUTOFF || + def->state == VIR_DOMAIN_DISK_SNAPSHOT); /* Older snapshots were created with just <domain>/<uuid>, and * lack domain/@type. In that case, leave dom NULL, and @@ -266,11 +268,42 @@ virDomainSnapshotDefParseString(const char *xmlStr, 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) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown memory snapshot setting '%s'"), + memorySnapshot); + goto cleanup; + } + if (memoryFile && + def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { + virReportError(VIR_ERR_XML_ERROR, + _("memory filename '%s' requires external snapshot"),

+ memoryFile); + goto cleanup; + } + } else if (memoryFile) { + def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + } else if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { + def->memory = (offline ? + VIR_DOMAIN_SNAPSHOT_LOCATION_NONE : + VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL); + } + if (offline && def->memory && + def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("memory state cannot be saved with offline snapshot")); + goto cleanup; + } + def->file = memoryFile; + memoryFile = NULL; + if ((i = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0) goto cleanup; - if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_DISKS || - (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE && - def->state == VIR_DOMAIN_DISK_SNAPSHOT)) { + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_DISKS) { def->ndisks = i; if (def->ndisks && VIR_ALLOC_N(def->disks, def->ndisks) < 0) { virReportOOMError(); @@ -302,6 +335,8 @@ cleanup: VIR_FREE(creation); VIR_FREE(state); VIR_FREE(nodes); + VIR_FREE(memorySnapshot); + VIR_FREE(memoryFile); xmlXPathFreeContext(ctxt); if (ret == NULL) virDomainSnapshotDefFree(def); @@ -523,8 +558,13 @@ char *virDomainSnapshotDefFormat(const char *domain_uuid, } virBufferAsprintf(&buf, " <creationTime>%lld</creationTime>\n", def->creationTime); - /* For now, only output <disks> on disk-snapshot */ - if (def->state == VIR_DOMAIN_DISK_SNAPSHOT) { + if (def->memory) { + virBufferAsprintf(&buf, " <memory snapshot='%s'", + virDomainSnapshotLocationTypeToString(def->memory)); + virBufferEscapeString(&buf, " file='%s'", def->file); + virBufferAddLit(&buf, "/>\n"); + } + if (def->ndisks) { virBufferAddLit(&buf, " <disks>\n"); for (i = 0; i < def->ndisks; i++) { virDomainSnapshotDiskDefPtr disk = &def->disks[i]; diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 135fe7c..5390193 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -66,6 +66,9 @@ struct _virDomainSnapshotDef { long long creationTime; /* in seconds */ int state; /* enum virDomainSnapshotState */ + int memory; /* enum virDomainMemorySnapshot */ + char *file; /* memory state file when snapshot is external */ + size_t ndisks; /* should not exceed dom->ndisks */ virDomainSnapshotDiskDef *disks; @@ -93,6 +96,7 @@ typedef enum { VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE = 1 << 0, VIR_DOMAIN_SNAPSHOT_PARSE_DISKS = 1 << 1, VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL = 1 << 2, + VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE = 1 << 3, } virDomainSnapshotParseFlags; virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr,