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

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(a)redhat.com&gt; > 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.

> > 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?

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.

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