[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 09/11/12 02:01, Eric Blake wrote:
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:

Hm, things would be a little cleaner without the 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;
+        }
      }

This hunk doesn't seem to be part of this patch. I'd push it separately.


      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"),

This error message sounds a little bit awkward. I'd write something along "memory filename is supported only with external snapshot".

The other question that comes into my mind is what happens if the user requests an external snapshot but specifies no filename. I suppose you plan hypervisor specific behavior.

+                           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,

Otherwise looks good. ACK.

Peter


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]