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