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

Re: [libvirt] [PATCH 01/20] snapshot: new XML for external system checkpoint



On 2012年10月23日 23:12, Peter Krempa wrote:
From: Eric Blake<eblake redhat com>

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 ecaafe9..45d55b5 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 16c844d..6f77026 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -94,6 +94,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);
@@ -182,6 +183,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) {
@@ -208,15 +212,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;
+        }

Okay, coding style improvements.

      }

      def->description = virXPathString("string(./description)", ctxt);
@@ -247,6 +247,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
@@ -274,11 +276,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&&

I think def->memory checking is redundant here. Not big deal though.

+        def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) {
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("memory state cannot be saved with offline snapshot"));

"VM state" is used both in the commit message and docs. Better
to keep consistent, I prefer "memory state" more, as it's more clear.
"VM state" could include disk state too. Correct me if I'm not right.

Others looks just very sanity, ACK.

Regards,
Osier


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