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

Re: [libvirt] [PATCH 06/20] snapshot: Add flag to enable creating checkpoints in paused state



On 10/31/12 23:12, Eric Blake wrote:
On 10/23/2012 09:12 AM, Peter Krempa wrote:
The default behavior while creating external checkpoints is to let the
guest run while the memory state is caputred. This leads to a larger
save file but minimizes the time needed to take the checkpoint.

This patch adds a flag that causes the guest to be paused before taking
the snapshot.

For this patch, I'm going to review the updated version at
  git fetch git://pipo.sk/pipo/libvirt.git snap-revert


commit 8cf5a508f0ef37308ce7601b1632a2fb0853233f
Author: Peter Krempa <pkrempa redhat com>
Date:   Tue Oct 9 12:11:56 2012 +0200

     snapshot: Add flag to enable creating checkpoints in live state

     The default behavior while creating external checkpoints is to pause the
     guest while the memory state is captured. We want the users to sacrifice
     space saving for creating the memory save image while the guest is live
     to minimize downtime.

     This patch adds a flag that causes the guest not to be paused before
     taking the snapshot.
      *include/libvirt/libvirt.h.in:
         - add new paused reason: VIR_DOMAIN_PAUSED_SNAPSHOT
         - add new flag for takin snapshot: VIR_DOMAIN_SNAPSHOT_CREATE_LIVE

s/takin/taking/

      *tools/virsh-domain-monitor.c:
         - add string representation for VIR_DOMAIN_PAUSED_SNAPSHOT
      *tools/virsh-snapshot.c:
         - add support for VIR_DOMAIN_SNAPSHOT_CREATE_LIVE
      *tools/virsh.pod:
         - add docs for --live option added to use
         VIR_DOMAIN_SNAPSHOT_CREATE_LIVE flag

This misses examples/domain-events/events-c/event-test.c, which also
needs updates.

Hm, the event-test file uses switch() statements that fully cover all possible values and I didn't run into a compile problem with this.

What am I missing here?



diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 2b17cef..d520144 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -179,6 +179,7 @@ typedef enum {
      VIR_DOMAIN_PAUSED_WATCHDOG = 6,     /* paused due to a watchdog event */
      VIR_DOMAIN_PAUSED_FROM_SNAPSHOT = 7, /* paused after restoring from snapshot */
      VIR_DOMAIN_PAUSED_SHUTTING_DOWN = 8, /* paused during shutdown process */
+    VIR_DOMAIN_PAUSED_SNAPSHOT = 9,      /* paused while creating a snaphot */

s/snaphot/snapshot/

At first, I wondered why you weren't reusing _FROM_SNAPSHOT, but after
looking through the series, now I know - you need distinct states to
know across libvirtd restarts whether the pause was temporary (due to
taking the snapshot) or end result (the snapshot itself requested paused
state when being restored).  Makes sense.

+++ b/tools/virsh-snapshot.c
@@ -127,6 +127,7 @@ static const vshCmdOptDef opts_snapshot_create[] = {
      {"reuse-external", VSH_OT_BOOL, 0, N_("reuse any existing external files")},
      {"quiesce", VSH_OT_BOOL, 0, N_("quiesce guest's file systems")},
      {"atomic", VSH_OT_BOOL, 0, N_("require atomic operation")},
+    {"pause", VSH_OT_BOOL, 0, N_("pause guest before taking snapshot")},

Stale - this line should be talking about live.

+++ 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<--quiesce>] [I<--atomic>] [I<--live>]}

  Create a snapshot for domain I<domain> with the properties specified in
  I<xmlfile>.  Normally, the only properties settable for a domain snapshot
@@ -2647,6 +2647,10 @@ this.  If this flag is not specified, then some hypervisors may fail
  after partially performing the action, and B<dumpxml> must be used to
  see whether any partial changes occurred.

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

Should we also mention that it is not supported with internal
checkpoint, and/or mention that it is silently ignored for offline
snapshots?

I think we should. I just added that to the libvirt.c documentation comment so I'll try to state that also in the man page

Patch looks okay once those issues are fixed, but since I reviewed an
unposted later version of your patch straight from your git tree, it
will help to see a v2 series with the fixes incorporated rather than
giving ACK now.

I'll be posting a v2 today (hopefuly).



--
libvir-list mailing list
libvir-list redhat com
https://www.redhat.com/mailman/listinfo/libvir-list



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