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@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.
---
 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>
       <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
@@ -128,13 +128,13 @@
         what file name is created in an external snapshot.  On output,
         this is fully populated to show the state of each disk in the
         snapshot, including any properties that were generated by the
-        hypervisor defaults.  For system checkpoints, this field is
-        ignored on input and omitted on output (a system checkpoint
+        hypervisor defaults.  For full system state snapshots, this field is
+        ignored on input and omitted on output (a full system state snapshot
         implies that all disks participate in the snapshot process,
         and since the current implementation only does internal system
-        checkpoints, there are no extra details to add); a future
+        snapshots, there are no extra details to add); a future
         release may allow the use of <code>disks</code> with a system
-        checkpoint.  This element has a list of <code>disk</code>
+        snapshot.  This element has a list of <code>disk</code>
         sub-elements, describing anywhere from zero to all of the
         disks associated with the domain.  <span class="since">Since
         0.9.5</span>
@@ -206,7 +206,7 @@
       </dd>
       <dt><code>state</code></dt>
       <dd>The state of the domain at the time this snapshot was taken.
-        If the snapshot was created as a system checkpoint, then this
+        If the snapshot was created with full system state, then this
         is the state of the domain at that time; when the domain is
         reverted to this snapshot, the domain's state will default to
         whatever is in this field unless additional flags are passed
diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h
index 0f73f24b2b..e5a893a767 100644
--- a/include/libvirt/libvirt-domain-snapshot.h
+++ b/include/libvirt/libvirt-domain-snapshot.h
@@ -58,7 +58,7 @@ typedef enum {
     VIR_DOMAIN_SNAPSHOT_CREATE_HALT        = (1 << 3), /* Stop running guest
                                                           after snapshot */
     VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY   = (1 << 4), /* disk snapshot, not
-                                                          system checkpoint */
+                                                          full system state */
     VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT   = (1 << 5), /* reuse any existing
                                                           external files */
     VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE     = (1 << 6), /* use guest agent to
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 787c3d0feb..5efbef7e09 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -1307,7 +1307,7 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
             (def->state == VIR_DOMAIN_DISK_SNAPSHOT)) {
             virReportError(VIR_ERR_INVALID_ARG,
                            _("cannot change between disk snapshot and "
-                             "system checkpoint in snapshot %s"),
+                             "full system state in snapshot %s"),
                            def->name);
             goto cleanup;
         }
diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c
index 100326a5e7..71881b2db2 100644
--- a/src/libvirt-domain-snapshot.c
+++ b/src/libvirt-domain-snapshot.c
@@ -105,7 +105,7 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot)
  * contained in xmlDesc.
  *
  * If @flags is 0, the domain can be active, in which case the
- * snapshot will be a system checkpoint (both disk state and runtime
+ * snapshot will be a full system state snapshot (both disk state and runtime
  * VM state such as RAM contents), where reverting to the snapshot is
  * the same as resuming from hibernation (TCP connections may have
  * timed out, but everything else picks up where it left off); or
@@ -149,7 +149,7 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot)
  * is not paused while creating the snapshot. This increases the size
  * of the memory dump file, but reduces downtime of the guest while
  * taking the snapshot. Some hypervisors only support this flag during
- * external checkpoints.
+ * external snapshots.
  *
  * If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY, then the
  * snapshot will be limited to the disks described in @xmlDesc, and no
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7c79c324e6..978c02fab9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2167,7 +2167,7 @@ qemuDomainReset(virDomainPtr dom, unsigned int flags)
 }


-/* Count how many snapshots in a set are external snapshots or checkpoints.  */
+/* Count how many snapshots in a set are external snapshots.  */
 static int
 qemuDomainSnapshotCountExternal(void *payload,
                                 const void *name ATTRIBUTE_UNUSED,
@@ -14688,7 +14688,7 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm,
     if ((def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL && !found_internal) ||
         (found_internal && forbid_internal)) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                       _("internal snapshots and checkpoints require all "
+                       _("internal and full system state snapshots require all "
                          "disks to be selected for snapshot"));
         goto cleanup;
     }
@@ -15161,7 +15161,7 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver,
     if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PMSUSPENDED) {
         pmsuspended = true;
     } else if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
-        /* For external checkpoints (those with memory), the guest
+        /* For full system external snapshots (those with memory), the guest
          * must pause (either by libvirt up front, or by qemu after
          * _LIVE converges).  For disk-only snapshots with multiple
          * disks, libvirt must pause externally to get all snapshots
@@ -15398,7 +15398,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
          redefine)) {
         virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
                        _("live snapshot creation is supported only "
-                         "with external checkpoints"));
+                         "with external full system state"));
         goto cleanup;
     }

@@ -15518,12 +15518,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
     } else if (virDomainObjIsActive(vm)) {
         if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY ||
             snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
-            /* external checkpoint or disk snapshot */
+            /* external full system or disk snapshot */
             if (qemuDomainSnapshotCreateActiveExternal(driver,
                                                        vm, snap, flags) < 0)
                 goto endjob;
         } else {
-            /* internal checkpoint */
+            /* internal full system */
             if (qemuDomainSnapshotCreateActiveInternal(driver,
                                                        vm, snap, flags) < 0)
                 goto endjob;
diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index 812fa91333..33e3107045 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -1432,7 +1432,7 @@ static const vshCmdOptDef opts_snapshot_list[] = {
     },
     {.name = "active",
      .type = VSH_OT_BOOL,
-     .help = N_("filter by snapshots taken while active (system checkpoints)")
+     .help = N_("filter by snapshots taken while active (full system snapshots)")
     },
     {.name = "disk-only",
      .type = VSH_OT_BOOL,
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 3f3314a87e..cb0dbfa7dd 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -4468,8 +4468,8 @@ If I<--halt> is specified, the domain will be left in an inactive state
 after the snapshot is created.

 If I<--disk-only> is specified, the snapshot will only include disk
-state rather than the usual system checkpoint with vm state.  Disk
-snapshots are faster than full system checkpoints, but reverting to a
+state rather than the usual full system state snapshot with vm state.  Disk
+snapshots are faster than full system snapshots, but reverting to a
 disk snapshot may require fsck or journal replays, since it is like
 the disk state at the point when the power cord is abruptly pulled;
 and mixing I<--halt> and I<--disk-only> loses any data that was not
@@ -4508,10 +4508,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 (checkpoint) while
+If I<--live> is specified, libvirt takes the snapshot while
 the guest is running. Both disk snapshot and domain memory snapshot are
 taken. This increases the size of the memory image of the external
-checkpoint. This is currently supported only for external checkpoints.
+snapshot. This is currently supported only for full system external snapshots.

 Existence of snapshot metadata will prevent attempts to B<undefine>
 a persistent domain.  However, for transient domains, snapshot
@@ -4531,7 +4531,7 @@ Otherwise, if I<--halt> is specified, the domain will be left in an
 inactive state after the snapshot is created, and if I<--disk-only>
 is specified, the snapshot will not include vm state.

-The I<--memspec> option can be used to control whether a checkpoint
+The I<--memspec> option can be used to control whether a full system snapshot
 is internal or external.  The I<--memspec> flag is mandatory, followed
 by a B<memspec> of the form B<[file=]name[,snapshot=type]>, where
 type can be B<no>, B<internal>, or B<external>.  To include a literal
@@ -4539,7 +4539,7 @@ comma in B<file=name>, escape it with a second comma. I<--memspec> cannot
 be used together with I<--disk-only>.

 The I<--diskspec> option can be used to control how I<--disk-only> and
-external checkpoints create external files.  This option can occur
+external full system snapshots create external files.  This option can occur
 multiple times, according to the number of <disk> elements in the domain
 xml.  Each <diskspec> is in the
 form B<disk[,snapshot=type][,driver=type][,file=name]>.  A I<diskspec>
@@ -4579,7 +4579,7 @@ 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. This is currently supported only for external checkpoints.
+snapshot. This is currently supported only for external full system snapshots.

 =item B<snapshot-current> I<domain> {[I<--name>] | [I<--security-info>]
 | [I<snapshotname>]}
-- 
2.14.4

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

On Wed, Jun 13, 2018 at 7:42 PM Eric Blake <eblake@redhat.com> 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.
>

I want to refer only to the new concept of checkpoint, compared with
snapshot.

I think checkpoint should refer to the current snapshot. When you perform
a backup, you should get the changed blocks in the current snapshot.
When you restore, you want to the restore several complete snapshots,
and one partial snapshot, based on the backups of that snapshot.

Lets try to see an example:

T1
- user create new vm marked for incremental backup
- system create base volume (S1)
- system create new dirty bitmap (B1)

T2
- user create a snapshot
- dirty bitmap in original snapshot deactivated (B1)
- system create new snapshot (S2)
- system starts new dirty bitmap in the new snapshot (B2)

T3
- user create new checkpoint
- system deactivate current dirty bitmap (B2)
- system create new dirty bitmap (B3)
- user backups data in snapshot S2 using dirty bitmap B2
- user backups data in snapshot S1 using dirty bitmap B1

T4
- user create new checkpoint
- system deactivate current dirty bitmap (B3)
- system create new dirty bitmap (B4)
- user backups data in snapshot S2 using dirty bitmap B3

Lets say use want to restore to state as it was in T3

This is the data kept by the backup application:

- snapshots
  - S1
    - checkpoints
      - B1
  - S2
    - checkpoints
      - B2
      - B3

T5
- user start restore to state in time T3
- user create new disk
- user create empty snapshot S1
- user upload snapshot S1 data to stoage
- user create empty snaphost disk S2
- user upload snapshot S1 data to stoage

John, are dirty bitmaps implemented in this way in qemu?

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

On 06/26/2018 10:56 AM, Nir Soffer wrote:
> On Wed, Jun 13, 2018 at 7:42 PM Eric Blake <eblake@redhat.com> 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.
>>
> 
> I want to refer only to the new concept of checkpoint, compared with
> snapshot.
> 
> I think checkpoint should refer to the current snapshot. When you perform
> a backup, you should get the changed blocks in the current snapshot.

That is an incremental backup (copying only the blocks that have changed 
since some previous point in time) - and my design was that such points 
in time are named 'checkpoints', where the most recent checkpoint is the 
current checkpoint.  This is different from a snapshot (which is enough 
state that you can revert back to that point in time directly) - a 
checkpoint only carries enough information to perform an incremental 
backup rather than a rollback to earlier state.

> When you restore, you want to the restore several complete snapshots,
> and one partial snapshot, based on the backups of that snapshot.

I'm worried that overloading the term "snapshot" and/or "checkpoint" can 
make it difficult to see whether we are describing the same data motions.

You are correct that in order to roll a virtual machine back to state 
represented by a series of incremental backups will require 
reconstructing the state present on the machine at the desired point to 
roll back to.  But I'll have to read your example first to see if we're 
on the same page.

> 
> Lets try to see an example:
> 
> T1
> - user create new vm marked for incremental backup
> - system create base volume (S1)
> - system create new dirty bitmap (B1)

Why do you need a dirty bitmap on a brand new system?  By definition, if 
the VM is brand new, every sector that the guest touches will be part of 
the first incremental backup, which is no different than taking a full 
backup of every sector?  But if it makes life easier by following 
consistent patterns, I also don't see a problem with creating a first 
checkpoint at the time an image is first created (my API proposal would 
allow you to create a domain, start it in the paused state, create a 
checkpoint, and then resume the guest so that it can start executing).

> 
> T2
> - user create a snapshot
> - dirty bitmap in original snapshot deactivated (B1)
> - system create new snapshot (S2)
> - system starts new dirty bitmap in the new snapshot (B2)

I'm still worried that interactions between snapshots (where the backing 
chain grows) and bitmaps may present interesting challenges.  But what 
you are describing here is that the act of creating a snapshot (to 
enlarge the backing chain) also has the effect of creating a snapshot (a 
new point in time for tracking incremental changes since the creation of 
the snapshot).  Whether we have to copy B1 into image S2, or whether 
image S2 can get by with just bitmap B2, is an implementation detail.

> 
> T3
> - user create new checkpoint
> - system deactivate current dirty bitmap (B2)
> - system create new dirty bitmap (B3)
> - user backups data in snapshot S2 using dirty bitmap B2
> - user backups data in snapshot S1 using dirty bitmap B1

So here you are performing two incremental backups.  Note: the user can 
already backup S1 without using any new APIs, and without reference to 
bitmap B1 - that's because B1 was started when S1 was created, and 
closed out when S1 was no longer modified - but now that S1 is a 
read-only file in the backing chain, copying S1 is the same as copying 
the clusters covered by bitmap B1.

Also, my current API additions do NOT make it easy to grab just the 
incremental data covered by bitmap B1 at time T3; rather, the time to 
grab the copy of the data covered just by B1 is at time T2 when you 
create bitmap B2 (whether or not you also create file S2).  The API 
additions as I have proposed them only make it easy to grab a full 
backup of all data up to time T3 (no checkpoint as its start), an 
incremental backup of all data since T1 (checkpoint T1 as its start, 
using the merge of B1 and B2 to learn which clusters to grab), or an 
incremental backup of all data since T2 (checkpoint T2 as its start, 
using B2 to learn which clusters to grab).

If you NEED to grab an incremental snapshot whose history is NOT bounded 
by the current moment in time, then we need to rethink the operations we 
are offering via my new API.  On the bright side, since my API for 
virDomainBackupBegin() takes an XML description, we DO have the option 
of enhancing that XML to take a second point in time as the end boundary 
(it already has an optional <incremental> tag as the first point in time 
for the start boundary; or a full backup if that tag is omitted) - if we 
enhance that XML, we'd also have to figure out how to map it to the 
operations that qemu exposes.  (The blockdev-backup command makes it 
easy to grab an incremental backup ending at the current moment in time, 
by using the "sync":"none" option to a temporary scratch file so that 
further guest writes do not corrupt the data to be grabbed from that 
point in time - but it does NOT make it easy to see the state of data 
from an earlier point in time - I'll demonstrate that below).

> 
> T4
> - user create new checkpoint
> - system deactivate current dirty bitmap (B3)
> - system create new dirty bitmap (B4)
> - user backups data in snapshot S2 using dirty bitmap B3

Yes, this is similar to what was done at T3, without the complication of 
trying to grab an incremental backup whose end boundary is not the 
current moment in time.

> 
> Lets say use want to restore to state as it was in T3
> 
> This is the data kept by the backup application:
> 
> - snapshots
>    - S1
>      - checkpoints
>        - B1
>    - S2
>      - checkpoints
>        - B2
>        - B3
> 
> T5
> - user start restore to state in time T3
> - user create new disk
> - user create empty snapshot S1
> - user upload snapshot S1 data to stoage
> - user create empty snaphost disk S2
> - user upload snapshot S1 data to stoage

Presumably, this would be 'user uploads S2 to storage', not S1.  But 
restoring in this manner didn't make any use of your incremental snapshots.

Maybe what I need to do is give a more visual indication of what 
incremental backups store.

At T1, we create S1 and start populating it.  As this was a brand new 
guest, the storage starts empty. Since you mentioned B1, I'll show it 
here, even though I argued it is pointless other than for fewer 
differences from later cases:

S1: |--------|
B1: |--------|
guest sees: |--------|

At T2, the guest has written things, so we now have:

S1: |AAAA----|
B1: |XXXX----|
guest sees: |AAAA----|

where A is the contents of the data the guest has written, and X is an 
indication in the bitmap which sections are dirty.

Also at time T2, we create a snapshot S2, making S1 become a read-only 
picture of the state of the disk at T2; we also started bitmap B2 on S2 
to track what the guest does:

S1: |AAAA----| <- S2: |--------|
B1: |XXXX----|    B2: |--------|

we can copy S1 to S1.bak at any point in time now that S1 is readonly.

S1.bak: |AAAA----|

At T3, the guest has written things, so we now have:

S1: |AAAA----| <- S2: |---BBB--|
B1: |XXXX----|    B2: |---XXX--|
guest sees: |AAABBB--|

so at this point, we freeze B2 and create B3; the new 
virDomainBackupBegin() API will let us also access the following copies 
at this time:

S1: |AAAA----| <- S2: |---BBB--|
B1: |XXXX----|    B2: |---XXX--|
                   B3: |--------|

full3.bak (no checkpoint as starting point): |AAABBB--|
B2.bak (checkpoint B2 as starting point): |---BBB--|

B2.bak by itself does not match anything the guest ever saw, but you can 
string together:

S1.bak <- S2.bak

to reconstruct the state the guest saw at T3.  By T4, the guest has made 
more edits:

S1: |AAAA----| <- S2: |D--BBDD-|
B1: |XXXX----|    B2: |---XXX--|
                   B3: |X----XX-|
guest sees: |DAABBDD-|

and as before, we now create B4, and have the option of several backups 
(usually, you'll only grab the most recent incremental backup, and not 
multiple backups; this is more an exploration of what is possible):

full4.bak (no checkpoint as starting): |DAABBDD-|
S2_3.bak (B2 as starting point, covering merge of B2 and B3): |D--BBDD-|
S3.bak (B3 as starting point): |D----DD-|

Note that both (S1.bak <- S2_3.bak) and (S1.bak <- S2.bak <- S3.bak) 
result in the same reconstructed guest image at time T4.  Also note that 
reading the contents of bitmap B2 in isolation at this time is NOT 
usable (you'd get |---BBD--|, which has mixed the incremental difference 
from T2 to T3 with a subset of the difference from T3 to T4, so it NO 
LONGER REPRESENTS the state of the guest at either T2 or T3, even when 
used as an overlay on top of S1.bak).  Hence, my emphasis that it is 
usually important to create your incremental backup at the same time you 
start your next bitmap, rather than trying to do it after the fact.

Also, you are starting to see the benefits of incremental backups. 
Creating S2_3.bak doesn't necessarily need bitmaps (it results in the 
same image as you would get if you create a temporary overlay [S1 <- S2 
<- tmp], copy off S2, then live merge tmp back into S2), but both 
full4.bak and S2_3.bak had to copy more data than S3.bak.

Later on, if you want to roll back to what the guest saw at T4, you just 
have to restore [S1.bak <- S2.bak <- S3.bak] as your backing chain to 
provide the data the guest saw at that time.


> 
> John, are dirty bitmaps implemented in this way in qemu?

The whole point of the libvirt API proposals is to make it possible to 
create bitmaps in qcow2 images at the point where you are creating 
incremental backups, so that the next incremental backup can be created 
using the previous one as its base.

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

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

On 06/26/2018 08:27 PM, Eric Blake wrote:
>>
>> Lets try to see an example:
>>
>> T1
>> - user create new vm marked for incremental backup
>> - system create base volume (S1)
>> - system create new dirty bitmap (B1)
> 
> Why do you need a dirty bitmap on a brand new system?  By definition, if 
> the VM is brand new, every sector that the guest touches will be part of 
> the first incremental backup, which is no different than taking a full 
> backup of every sector?  But if it makes life easier by following 
> consistent patterns, I also don't see a problem with creating a first 
> checkpoint at the time an image is first created (my API proposal would 
> allow you to create a domain, start it in the paused state, create a 
> checkpoint, and then resume the guest so that it can start executing).
> 
>>
>> T2
>> - user create a snapshot
>> - dirty bitmap in original snapshot deactivated (B1)
>> - system create new snapshot (S2)
>> - system starts new dirty bitmap in the new snapshot (B2)
> 
> I'm still worried that interactions between snapshots (where the backing 
> chain grows) and bitmaps may present interesting challenges.  But what 
> you are describing here is that the act of creating a snapshot (to 
> enlarge the backing chain) also has the effect of creating a snapshot (a 

that should read "also has the effect of creating a checkpoint"

Except that I'm not quite sure how best to handle the interaction 
between snapshots and checkpoints using existing qemu primitives.  Right 
now, I'm leaning back to the idea that if you have an external backing 
file (that is, the act of creating a snapshot expanded the disk chain 
from 'S1' into 'S1 <- S2'), then creating an incremental backup that 
covers just the disk changes since that point in time is the same as a 
"sync":"top" copy of the just-created S2 image (no bitmap is needed to 
track what needs copying) - which works well for qemu writing out the 
backup file.  But since we are talking about allowing third-party 
backups (where we provide an NBD export and the client can query which 
portions are dirty), then using the snapshot as the start point in time 
would indeed require that we either have a bitmap to expose (that is, we 
need to create a bitmap as part of the same transaction as creating the 
external snapshot file), or that we can resynthesize a bitmap based on 
the clusters allocated in S2 at the time we start the backup operation 
(that's an operation that I don't see in qemu right now).  And if we DO 
want to allow external snapshots to automatically behave as checkpoints 
for use by incremental backups, that makes me wonder if I need to 
eventually enhance the existing virDomainSnapshotCreateXML() to also 
accept XML describing a checkpoint to be created simultaneously with the 
snapshot (the way my proposal already allows creating a checkpoint 
simultaneously with virDomainBackupBegin()).

Another point that John and I discussed on IRC is that migrating bitmaps 
still has some design work to figure out. Remember, right now, there are 
basically three modes of operation regarding storage between source and 
endpoint of a migration:
1. Storage is shared. As long as qemu flushes the bitmap before 
inactivating on the source, then activating on the destination can load 
the bitmap, and everything is fine. The migration stream does not have 
to include the bitmaps.
2. Storage is not shared, but the storage is migrated via flags to the 
migrate command (we're trying to move away from this version) - there, 
qemu knows that it has to migrate the bitmaps as part of the migration 
stream.
3. Storage is not shared, and the storage is migrated via NBD (libvirt 
favors using this version for non-shared storage). Libvirt starts 'qemu 
-S' on the destination, pre-creates a destination file large enough to 
match the source, starts an NBD server at the destination, then on the 
source starts a block-mirror operation to the destination. When the 
drive is mirrored, libvirt then kicks off the migration using the same 
command as in style 1; when all state is transferred, the source then 
stops the block-mirror, disconnects the NBD client, the destination then 
stops the NBD server, and the destination can finally start executing. 
But note that in this mode, no bitmaps are migrated. So we need some way 
for libvirt to also migrate bitmap state to the destination (perhaps 
having the NBD server open multiple exports - one for the block itself, 
but another export for each bitmap that needs to be copied).

At this point, I think the pressure is on me to provide a working demo 
of incremental backups working without any external snapshots or 
migration, before we expand into figuring out interactions between features.

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

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

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

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.

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.


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

Also the "flag; in both cases...", I think should be a "flag. Regardless
of the flags value provided, restoration of the snapshot is handled by
the virDomainRevertToSnapshot() function."

But that's just me being "particular". ;-)  There's bigger fish to fry
here other than grammar issues.   There's so many usages of the "; " to
join two sentences in this page - it'd probably take more effort than
desired to go through each one.


> @@ -128,13 +128,13 @@
>          what file name is created in an external snapshot.  On output,
>          this is fully populated to show the state of each disk in the
>          snapshot, including any properties that were generated by the
> -        hypervisor defaults.  For system checkpoints, this field is
> -        ignored on input and omitted on output (a system checkpoint
> +        hypervisor defaults.  For full system state snapshots, this field is
> +        ignored on input and omitted on output (a full system state snapshot
>          implies that all disks participate in the snapshot process,
>          and since the current implementation only does internal system
> -        checkpoints, there are no extra details to add); a future
> +        snapshots, there are no extra details to add); a future
>          release may allow the use of <code>disks</code> with a system
> -        checkpoint.  This element has a list of <code>disk</code>
> +        snapshot.  This element has a list of <code>disk</code>
>          sub-elements, describing anywhere from zero to all of the
>          disks associated with the domain.  <span class="since">Since
>          0.9.5</span>
> @@ -206,7 +206,7 @@
>        </dd>
>        <dt><code>state</code></dt>
>        <dd>The state of the domain at the time this snapshot was taken.
> -        If the snapshot was created as a system checkpoint, then this
> +        If the snapshot was created with full system state, then this
>          is the state of the domain at that time; when the domain is
>          reverted to this snapshot, the domain's state will default to
>          whatever is in this field unless additional flags are passed

Oy - this is so hard to read... Such as what flags?.... leaves me
searching... ahhh... REVERT_RUNNING or REVERT_PAUSED... So as a suggestion:

If a full system snapshot was created, then this is the state of the
domain at that time. When the domain is reverted to this snapshot, then
the domain's state will default to this state unless overridden by
virDomainRevertToSnapshot() flags, such as revert to running or to
paused state.


> diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h
> index 0f73f24b2b..e5a893a767 100644
> --- a/include/libvirt/libvirt-domain-snapshot.h
> +++ b/include/libvirt/libvirt-domain-snapshot.h
> @@ -58,7 +58,7 @@ typedef enum {
>      VIR_DOMAIN_SNAPSHOT_CREATE_HALT        = (1 << 3), /* Stop running guest
>                                                            after snapshot */
>      VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY   = (1 << 4), /* disk snapshot, not
> -                                                          system checkpoint */
> +                                                          full system state */
>      VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT   = (1 << 5), /* reuse any existing
>                                                            external files */
>      VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE     = (1 << 6), /* use guest agent to
> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index 787c3d0feb..5efbef7e09 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -1307,7 +1307,7 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
>              (def->state == VIR_DOMAIN_DISK_SNAPSHOT)) {
>              virReportError(VIR_ERR_INVALID_ARG,
>                             _("cannot change between disk snapshot and "
> -                             "system checkpoint in snapshot %s"),
> +                             "full system state in snapshot %s"),

"cannot change between disk only and full system snapshots"

[honestly, "full system state in snapshot" doesn't read well to me.]

>                             def->name);
>              goto cleanup;
>          }
> diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c
> index 100326a5e7..71881b2db2 100644
> --- a/src/libvirt-domain-snapshot.c
> +++ b/src/libvirt-domain-snapshot.c
> @@ -105,7 +105,7 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot)
>   * contained in xmlDesc.
>   *
>   * If @flags is 0, the domain can be active, in which case the
> - * snapshot will be a system checkpoint (both disk state and runtime
> + * snapshot will be a full system state snapshot (both disk state and runtime

"disk state"?   Should that be disk contents?

>   * VM state such as RAM contents), where reverting to the snapshot is
>   * the same as resuming from hibernation (TCP connections may have
>   * timed out, but everything else picks up where it left off); or
> @@ -149,7 +149,7 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot)
>   * is not paused while creating the snapshot. This increases the size
>   * of the memory dump file, but reduces downtime of the guest while
>   * taking the snapshot. Some hypervisors only support this flag during
> - * external checkpoints.
> + * external snapshots.
>   *
>   * If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY, then the
>   * snapshot will be limited to the disks described in @xmlDesc, and no
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 7c79c324e6..978c02fab9 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -2167,7 +2167,7 @@ qemuDomainReset(virDomainPtr dom, unsigned int flags)
>  }
> 
> 
> -/* Count how many snapshots in a set are external snapshots or checkpoints.  */
> +/* Count how many snapshots in a set are external snapshots.  */
>  static int
>  qemuDomainSnapshotCountExternal(void *payload,
>                                  const void *name ATTRIBUTE_UNUSED,
> @@ -14688,7 +14688,7 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm,
>      if ((def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL && !found_internal) ||
>          (found_internal && forbid_internal)) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                       _("internal snapshots and checkpoints require all "
> +                       _("internal and full system state snapshots require all "
>                           "disks to be selected for snapshot"));
>          goto cleanup;
>      }
> @@ -15161,7 +15161,7 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver,
>      if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PMSUSPENDED) {
>          pmsuspended = true;
>      } else if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
> -        /* For external checkpoints (those with memory), the guest
> +        /* For full system external snapshots (those with memory), the guest
>           * must pause (either by libvirt up front, or by qemu after
>           * _LIVE converges).  For disk-only snapshots with multiple
>           * disks, libvirt must pause externally to get all snapshots
> @@ -15398,7 +15398,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
>           redefine)) {
>          virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
>                         _("live snapshot creation is supported only "
> -                         "with external checkpoints"));
> +                         "with external full system state"));

live snapshot creation is supported only using a full system snapshot

>          goto cleanup;
>      }
> 
> @@ -15518,12 +15518,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
>      } else if (virDomainObjIsActive(vm)) {
>          if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY ||
>              snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
> -            /* external checkpoint or disk snapshot */
> +            /* external full system or disk snapshot */
>              if (qemuDomainSnapshotCreateActiveExternal(driver,
>                                                         vm, snap, flags) < 0)
>                  goto endjob;
>          } else {
> -            /* internal checkpoint */
> +            /* internal full system */
>              if (qemuDomainSnapshotCreateActiveInternal(driver,
>                                                         vm, snap, flags) < 0)
>                  goto endjob;
> diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
> index 812fa91333..33e3107045 100644
> --- a/tools/virsh-snapshot.c
> +++ b/tools/virsh-snapshot.c
> @@ -1432,7 +1432,7 @@ static const vshCmdOptDef opts_snapshot_list[] = {
>      },
>      {.name = "active",
>       .type = VSH_OT_BOOL,
> -     .help = N_("filter by snapshots taken while active (system checkpoints)")
> +     .help = N_("filter by snapshots taken while active (full system snapshots)")
>      },
>      {.name = "disk-only",
>       .type = VSH_OT_BOOL,
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 3f3314a87e..cb0dbfa7dd 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -4468,8 +4468,8 @@ If I<--halt> is specified, the domain will be left in an inactive state
>  after the snapshot is created.
> 
>  If I<--disk-only> is specified, the snapshot will only include disk
> -state rather than the usual system checkpoint with vm state.  Disk
> -snapshots are faster than full system checkpoints, but reverting to a
> +state rather than the usual full system state snapshot with vm state.  Disk

Here, "with vm state" would seem to be redundant.

Also here again, is it really "disk state" or "disk content".

John

> +snapshots are faster than full system snapshots, but reverting to a
>  disk snapshot may require fsck or journal replays, since it is like
>  the disk state at the point when the power cord is abruptly pulled;
>  and mixing I<--halt> and I<--disk-only> loses any data that was not
> @@ -4508,10 +4508,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 (checkpoint) while
> +If I<--live> is specified, libvirt takes the snapshot while
>  the guest is running. Both disk snapshot and domain memory snapshot are
>  taken. This increases the size of the memory image of the external
> -checkpoint. This is currently supported only for external checkpoints.
> +snapshot. This is currently supported only for full system external snapshots.
> 
>  Existence of snapshot metadata will prevent attempts to B<undefine>
>  a persistent domain.  However, for transient domains, snapshot
> @@ -4531,7 +4531,7 @@ Otherwise, if I<--halt> is specified, the domain will be left in an
>  inactive state after the snapshot is created, and if I<--disk-only>
>  is specified, the snapshot will not include vm state.
> 
> -The I<--memspec> option can be used to control whether a checkpoint
> +The I<--memspec> option can be used to control whether a full system snapshot
>  is internal or external.  The I<--memspec> flag is mandatory, followed
>  by a B<memspec> of the form B<[file=]name[,snapshot=type]>, where
>  type can be B<no>, B<internal>, or B<external>.  To include a literal
> @@ -4539,7 +4539,7 @@ comma in B<file=name>, escape it with a second comma. I<--memspec> cannot
>  be used together with I<--disk-only>.
> 
>  The I<--diskspec> option can be used to control how I<--disk-only> and
> -external checkpoints create external files.  This option can occur
> +external full system snapshots create external files.  This option can occur
>  multiple times, according to the number of <disk> elements in the domain
>  xml.  Each <diskspec> is in the
>  form B<disk[,snapshot=type][,driver=type][,file=name]>.  A I<diskspec>
> @@ -4579,7 +4579,7 @@ 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. This is currently supported only for external checkpoints.
> +snapshot. This is currently supported only for external full system snapshots.
> 
>  =item B<snapshot-current> I<domain> {[I<--name>] | [I<--security-info>]
>  | [I<snapshotname>]}
> 

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

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

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

On 06/25/2018 06:27 PM, Eric Blake wrote:
> 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@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.
> 

Understood - since I've made it further now. Domain state is somewhat of
a tangled or interwoven part of the domain XML lifecycle fabric, so I
perhaps process it that way when reading new code.

It seems the design is that XML for domain{checkpoint|backup} will be
similar to domainstatus, but far more restrictive to the specific needs
for each since you're saving the original config in the output.

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

I saw things as more of "{Create|Start|Set|Clear|End|Destroy}" type
operations initially, but I've gone through my viewpoint has changed.
I'm still concerned with over complicating with too many flags which is
where we seem to have gotten ourselves in trouble with snapshot as time
went on and more or less functionality points were desired.

Checkpoint/backup is complex enough without adding levels of features
for consumers that may or may not be used. In the long run, it's where
do you give up control and how much can/should be assumed to be
libvirt's "job".

If a consumer wants to do something particularly tricky, then maybe we
should just hand them the gun with one bullet in the chamber (so to
speak). Provide an API that "locks out" other threads instead of doing
that for them ;-) - someone always thinks they can do it better!

In the long run, we don't necessarily know how all consumers would like
to use this and so far there's been mixed opinions on what should be
done. At some level the operation of starting checkpoints, setting
checkpoints, and allowing/performing backups from a specific checkpoint
are fairly straightforward type operations. It's the additional cruft
and flags to do "fancy" stuff or the desire to worry about every
possible operation option someone could possibly want that I think
causes over complication. Let the up-the-stack app keep track of things
if that's the complicated level they desire. Keep the libvirt side simple.

> 
> 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 think so, but I haven't reached the Backup API's yet. There's the
interaction with NBD that's going to keep things "interesting".
Reminding myself that under the covers, migrations start/stop an NBD
server is I assume the impetus behind [re]using NBD as the 3rd party
integration point. Perhaps something that becomes part of the
documentation description for describing the difference between
pull/push backup models.

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

OK - it's not 100% clear in my mind all the various pieces needed for
this to all work properly. I understand the desire/need for inclusion;
however, we do need to consider that in more recent times "waiting" for
a more functional solution to be "ready" in QEMU before pushing libvirt
changes - especially ones that bake an API...

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

Understood.

John

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