[libvirt] [PATCH 1/8] snapshots: Avoid term 'checkpoint' for full system snapshot

[Eric Blake]

On 06/22/2018 04:16 PM, John Ferlan wrote:
> 
> 
> On 06/13/2018 12:42 PM, Eric Blake wrote:
>> Upcoming patches plan to introduce virDomainCheckpointPtr as a new
>> object for use in incremental backups, along with documentation
>> how incremental backups differ from snapshots.  But first, we need
>> to rename any existing mention of a 'system checkpoint' to instead
>> be a 'full system state snapshot', so that we aren't overloading
>> the term checkpoint.
>>
>> Signed-off-by: Eric Blake <eblake at redhat.com>
>>
>> ---
>> Bikeshed suggestions on what to name the new object for use in
>> backups is welcome, if we would rather keep the term 'checkpoint'
>> for a disk+memory snapshot.
>> ---
> 
> "Naming is hard" and opinions can vary greatly - be careful for what you
> ask in case you receive something not wanted ;-).
> 
> I haven't followed the discussions thus far all that closely, but I'll
> give this a go anyway since it's languishing and saying nothing is akin
> to implicitly agreeing everything is fine. Fair warning, I'm not all
> that familiar with snapshot algorithms having largely tried to ignore it
> since others (Eric and Peter) have far more in depth knowledge.
> 
> In any case, another option for the proposed "checkpoint" could be a
> "snapshot reference". One can start or end a reference period and then
> set or clear a reference point.
> 
> What I'm not clear on yet is whether the intention is to have this
> checkpoint (and backup) be integrated in any way to the existing
> snapshot algorithms. I guess part of me thinks that if I take a full
> system snapshot, then any backup/checkpoint data should be included so
> that if/when I go back to that point in time I can start from whence I
> left as it relates to my backup. Kind of a superset and/or integrated
> model rather than something bolted onto the side to resolve a specific need.

That's a tough call.  My current design has incremental backups 
completely separate from the existing checkpoint code for several reasons:
- the snapshot code is already confusing with lots of flags 
(internal/external, disk/memory, etc)
- snapshots can be reverted to (well, in theory - we STILL can't revert 
to an external snapshot cleanly, even though the design supports it)
- incremental backups are not direct revert points

so, rather than bolt something on to the existing design, I went with a 
new concept. As you found later in the series, I then tried to provide a 
good summary page describing the different pieces, and what tradeoffs 
are involved in order to know which approach will work for a given need.

> 
> I suppose a reservation I have about separate virDomainCheckpoint* and
> virDomainBackup* API's is understanding the relationship between the two
> naming spaces. IIUC though a Checkpoint would be reference point in time
> within a Backup period.

A sequence of snapshots are different points in time you can revert to. 
A sequence of checkpoints are different points in time you can use as 
the reference point for starting an incremental backup.

So if we don't like the term 'checkpoint', maybe 
virDomainBlockBackupReference would work.  But it is longer, and would 
make for some mouthful API names.

Also, you commented elsewhere that 'virDomainBackupBegin' misses out on 
the fact that under the hood, it is a block operation (only disk state); 
would 'virDomainBlockBackupBegin' be any better?  There are fewer APIs 
with the term 'Backup' than with 'Checkpoint', if we do want go with 
that particular rename.

> 
> I do have more comments in patch2, but I want to make them coherent
> before posting. Still I wanted to be sure you got at least "some"
> feedback for this and well of course an opinion on checkpoint ;-)
> 
>>   docs/formatsnapshot.html.in               | 14 +++++++-------
>>   include/libvirt/libvirt-domain-snapshot.h |  2 +-
>>   src/conf/snapshot_conf.c                  |  2 +-
>>   src/libvirt-domain-snapshot.c             |  4 ++--
>>   src/qemu/qemu_driver.c                    | 12 ++++++------
>>   tools/virsh-snapshot.c                    |  2 +-
>>   tools/virsh.pod                           | 14 +++++++-------
>>   7 files changed, 25 insertions(+), 25 deletions(-)
>>
>> diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in
>> index fbbecfd242..f2e51df5ab 100644
>> --- a/docs/formatsnapshot.html.in
>> +++ b/docs/formatsnapshot.html.in
>> @@ -33,7 +33,7 @@
>>           resume in a consistent state; but if the disks are modified
>>           externally in the meantime, this is likely to lead to data
>>           corruption.</dd>
>> -      <dt>system checkpoint</dt>
>> +      <dt>full system state</dt>
> 
> Is "state" superfluous in this context?  IOW: Everywhere that "full
> system state" exists, it seems "full system" could be used.
> 
> Other synonyms that came up are complete, entire, integrated, or
> thorough (hah!). But I think "Full System" conveys enough meaning even
> though it could convey more meaning than intended.

Okay, I can live with shortening the replacement to 'full system'. 
Don't know if it will happen in the v2 series that I hope to post later 
tonight, or if it would be done on top (my immediate short-term goal is 
to get a demo of incremental backups working, to show the API is usable; 
although the demo depends on unreleased qemu code so only the API would 
actually go in this month's libvirt release, while the underlying 
src/qemu/* changes can be delayed and polished to be better than the 
demo for the time when qemu 3.0 releases the needed bitmap/NBD features).

> 
> 
>>         <dd>A combination of disk snapshots for all disks as well as VM
>>           memory state, which can be used to resume the guest from where it
>>           left off with symptoms similar to hibernation (that is, TCP
>> @@ -55,7 +55,7 @@
>>         as <code>virDomainSaveImageGetXMLDesc()</code> to work with
>>         those files.
>>       </p>
>> -    <p>System checkpoints are created
>> +    <p>Full system state snapshots are created
>>         by <code>virDomainSnapshotCreateXML()</code> with no flags, and
>>         disk snapshots are created by the same function with
>>         the <code>VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY</code> flag; in
> 
> BTW: Existing and maybe it's just me, but when I read a conjunctive
> sentence with only two parts I don't expect to see ", and" or ", or" -
> it's just "and" or "or" without the comma....

Thanks for the careful grammar/legibility review.  I'll try to fold in 
those suggestions (again, might not make it into v2).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org