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

[Peter Krempa]

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 at 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>