On Tue, Jun 18, 2019 at 22:47:49 -0500, Eric Blake wrote:
> Introduce a bunch of new virsh commands for managing checkpoints
> in isolation. More commands are needed for performing incremental
> backups, but these commands were easy to implement by modeling
> heavily after virsh-snapshot.c (no need for checkpoint-revert,
> and checkpoint-list was a lot easier since we don't have to cater
> to older libvirt API).
>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
[..]
> diff --git a/tools/virsh-checkpoint.c b/tools/virsh-checkpoint.c
> new file mode 100644
> index 0000000000..41b07e5452
> --- /dev/null
> +++ b/tools/virsh-checkpoint.c
> @@ -0,0 +1,1342 @@
[...]
> +/* Helper for checkpoint-create and checkpoint-create-as */
> +static bool
> +virshCheckpointCreate(vshControl *ctl,
> + virDomainPtr dom,
> + const char *buffer,
> + unsigned int flags,
> + const char *from)
> +{
> + bool ret = false;
> + virDomainCheckpointPtr checkpoint;
> + const char *name = NULL;
> +
> + checkpoint = virDomainCheckpointCreateXML(dom, buffer, flags);
> +
> + if (checkpoint == NULL)
> + goto cleanup;
> +
> + name = virDomainCheckpointGetName(checkpoint);
> + if (!name) {
> + vshError(ctl, "%s", _("Could not get snapshot name"));
leftover 'snapshot'
> + goto cleanup;
> + }
> +
> + if (from)
> + vshPrintExtra(ctl, _("Domain checkpoint %s created from '%s'"),
> + name, from);
> + else
> + vshPrintExtra(ctl, _("Domain checkpoint %s created"), name);
> +
> + ret = true;
> +
> + cleanup:
> + virshDomainCheckpointFree(checkpoint);
> + return ret;
> +}
> +
[...]
> +static bool
> +cmdCheckpointCreateAs(vshControl *ctl,
> + const vshCmd *cmd)
> +{
> + virDomainPtr dom = NULL;
> + bool ret = false;
> + char *buffer = NULL;
> + const char *name = NULL;
> + const char *desc = NULL;
> + virBuffer buf = VIR_BUFFER_INITIALIZER;
> + unsigned int flags = 0;
> + const vshCmdOpt *opt = NULL;
> +
> + if (vshCommandOptBool(cmd, "no-metadata")) {
> + if (vshCommandOptBool(cmd, "print-xml")) {
> + vshError(ctl, "%s",
> + _("--print-xml is incompatible with --no-metadata"));
Why? The XML used to create the checkpoint is the same regardless of the
flag.
> + return false;
> + }
> + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA;
> + }
> + if (vshCommandOptBool(cmd, "quiesce"))
> + flags |= VIR_DOMAIN_CHECKPOINT_CREATE_QUIESCE;
> +
[...]
> +static const vshCmdOptDef opts_checkpoint_edit[] = {
> + VIRSH_COMMON_OPT_DOMAIN_FULL(0),
> + {.name = "checkpointname",
> + .type = VSH_OT_STRING,
> + .help = N_("checkpoint name"),
> + .completer = virshCheckpointNameCompleter,
> + },
> + VIRSH_COMMON_OPT_CURRENT(N_("also set edited checkpoint as current")),
> + {.name = "rename",
> + .type = VSH_OT_BOOL,
> + .help = N_("allow renaming an existing checkpoint")
I'd prefer if these operations aren't really encouraged ... in the docs
at least.
> + },
> + {.name = "clone",
> + .type = VSH_OT_BOOL,
> + .help = N_("allow cloning to new name")
> + },
> + {.name = NULL}
[...]
> +/*
> + * "checkpoint-current" command
> + */
> +static const vshCmdInfo info_checkpoint_current[] = {
> + {.name = "help",
> + .data = N_("Get or set the current checkpoint")
> + },
> + {.name = "desc",
> + .data = N_("Get or set the current checkpoint")
> + },
> + {.name = NULL}
> +};
[...]
> +
> +static bool
> +cmdCheckpointCurrent(vshControl *ctl,
> + const vshCmd *cmd)
What's the point of this API? Getting current checkpoint ... maybe, but
setting?!? especially since the API does seem to use _REDEFINE, which
does not actually change the actual state.
Isn't plain Define/Edit API good enough?
This might give the false sense to the user that it's actually useful.
Also I just remembered I already complained about this in v8 ...
> +{
> + virDomainPtr dom = NULL;
> + bool ret = false;
> + virDomainCheckpointPtr checkpoint = NULL;
> + char *xml = NULL;
> + const char *checkpointname = NULL;
> + unsigned int flags = 0;
> + const char *domname;
> +
> + if (vshCommandOptBool(cmd, "security-info"))
> + flags |= VIR_DOMAIN_CHECKPOINT_XML_SECURE;
> + if (vshCommandOptBool(cmd, "no-domain"))
> + flags |= VIR_DOMAIN_CHECKPOINT_XML_NO_DOMAIN;
> + if (vshCommandOptBool(cmd, "size"))
> + flags |= VIR_DOMAIN_CHECKPOINT_XML_SIZE;
> +
> + VSH_EXCLUSIVE_OPTIONS("name", "checkpointname");
> +
> + if (!(dom = virshCommandOptDomain(ctl, cmd, &domname)))
> + return false;
> +
> + if (vshCommandOptStringReq(ctl, cmd, "checkpointname", &checkpointname) < 0)
> + goto cleanup;
> +
> + if (checkpointname) {
> + virDomainCheckpointPtr checkpoint2 = NULL;
> + flags = (VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE |
> + VIR_DOMAIN_CHECKPOINT_CREATE_CURRENT);
[...]
> +static bool
> +cmdCheckpointList(vshControl *ctl,
> + const vshCmd *cmd)
> +{
[...]
> +
> + if (!tree && !name) {
> + if (parent)
> + vshPrintExtra(ctl, " %-20s %-25s %s",
> + _("Name"), _("Creation Time"), _("Parent"));
> + else
> + vshPrintExtra(ctl, " %-20s %-25s",
> + _("Name"), _("Creation Time"));
> + vshPrintExtra(ctl, "\n"
> + "------------------------------"
> + "--------------\n");
This uses the non-tablefied APIs whereas we already switched most of
these to vshTableNew and friends.
> + }
> +
> + if (tree) {
> + for (i = 0; i < chklist->nchks; i++) {
> + if (!chklist->chks[i].parent &&
> + vshTreePrint(ctl, virshCheckpointListLookup, chklist,
[...]
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 828ae30789..1d6a34de4b 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -3633,6 +3633,10 @@ static const vshCmdOptDef opts_undefine[] = {
> .type = VSH_OT_BOOL,
> .help = N_("remove all domain snapshot metadata (vm must be inactive)")
> },
> + {.name = "checkpoints-metadata",
> + .type = VSH_OT_BOOL,
> + .help = N_("remove all domain checkpoint metadata, if inactive")
Please update to the new format of the message (see above)
> + },
> {.name = "nvram",
> .type = VSH_OT_BOOL,
> .help = N_("remove nvram file, if inactive")
[...]
> @@ -3765,6 +3772,8 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
> }
> }
> }
> + /* Clear flags which older servers might reject, if they would
> + * otherwise have no effect. */
Does not seem relevant to this patch.
> if (!has_managed_save) {
> flags &= ~VIR_DOMAIN_UNDEFINE_MANAGED_SAVE;
> managed_save_safe = true;
[...]
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 2e70b68a66..a5db2507cd 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
[...]
> @@ -4884,6 +4898,216 @@ the data contents from that point in time.
>
> =back
>
> +=head1 CHECKPOINT COMMANDS
> +
> +The following commands manipulate domain checkpoints. Checkpoints serve as
> +a point in time to identify which portions of a guest's disks have changed
> +after that time, making it possible to perform incremental and differential
> +backups. Checkpoints are identified with a unique name. See
> +L<https://libvirt.org/formatcheckpoint.html> for documentation of the XML
> +format used to represent properties of checkpoints.
> +
> +=over 4
> +
> +=item B<checkpoint-create> I<domain> [I<xmlfile>] {[I<--redefine>
> +{[I<--current>] | [I<--redefine-list>]}] | [I<--no-metadata>] [I<--quiesce>]}
> +
> +Create a checkpoint for domain I<domain> with the properties specified
> +in I<xmlfile> describing a <domaincheckpoint> top-level element. If
> +I<xmlfile> is completely omitted, then libvirt will create a
> +checkpoint with a name based on the current time. The new checkpoint
> +will become current, as listed by B<checkpoint-current>.
> +
> +If I<--redefine> is specified, then all XML elements produced by
> +B<checkpoint-dumpxml> are valid; this can be used to migrate
> +checkpoint hierarchy from one machine to another, to recreate
> +hierarchy for the case of a transient domain that goes away and is
> +later recreated with the same name and UUID, or to make slight
> +alterations in the checkpoint metadata (such as host-specific aspects
> +of the domain XML embedded in the checkpoint). When this flag is
> +supplied, the I<xmlfile> argument is mandatory, and the domain's
> +current snapshot will not be altered unless the I<--current> flag is
> +also given. If I<--redefine-list> is specified, I<--redefine> is
> +implied, I<--current> is rejected, and the XML changes from being a
> +single <domaincheckpoint> to instead being a <checkpoints> element
> +describing a list of checkpoints. List form only works if the domain
> +has no currently-defined checkpoint metadata, and can be obtained as a
> +subset of I<dumpxml --checkpoints> output.
> +
> +If I<--no-metadata> is specified, then the checkpoint data is created,
> +but any metadata is immediately discarded (that is, libvirt does not
> +treat the checkpoint as current, and cannot use the checkpoint for an
> +incremental backup unless I<--redefine> is later used to teach libvirt
> +about the metadata again).
No, nope, nein. Don't tell users to do this. Just tell them that it's
pointless and also that drivers may decide not to support it in the end.
> +If I<--quiesce> is specified, libvirt will try to use guest agent
> +to freeze and unfreeze domain's mounted file systems. However,
> +if domain has no guest agent, checkpoint creation will fail.
> +
> +Existence of checkpoint metadata will prevent attempts to B<undefine>
> +a persistent domain. However, for transient domains, checkpoint
> +metadata is silently lost when the domain quits running (whether
> +by command such as B<destroy> or by internal guest action).
> +
> +=item B<checkpoint-create-as> I<domain> {[I<--print-xml>]
> +| [I<--no-metadata>]} [I<name>] [I<description>] [I<--quiesce>]
> +[I<--diskspec>] B<diskspec>]...
> +
> +Create a checkpoint for domain I<domain> with the given <name> and
> +<description>; if either value is omitted, libvirt will choose a
> +value. If I<--print-xml> is specified, then XML appropriate for
> +I<checkpoint-create> is output, rather than actually creating a
> +checkpoint.
> +
> +The I<--diskspec> option can be used to control which guest disks participate in the checkpoint. This option can occur
> +multiple times, according to the number of <disk> elements in the domain
> +xml. Each <diskspec> is in the
Few very long lines
> +form B<disk[,checkpoint=type][,bitmap=name]>. A literal I<--diskspec> must precede each B<diskspec> unless
> +all three of I<domain>, I<name>, and I<description> are also present.
> +For example, a diskspec of "vda,checkpoint=bitmap,bitmap=map1"
> +results in the following XML:
> + <disk name='vda' checkpoint='bitmap' bitmap='map1'/>
> +
> +If I<--quiesce> is specified, libvirt will try to use guest agent
> +to freeze and unfreeze domain's mounted file systems. However,
> +if domain has no guest agent, checkpoint creation will fail.
> +
> +If I<--no-metadata> is specified, then the checkpoint data is created,
> +but any metadata is immediately discarded (that is, libvirt does not
> +treat the checkpoint as current, and cannot use the checkpoint for an
> +incremental backup unless I<--redefine> is later used to teach libvirt
> +about the metadata again).
Same comment as above.
> +
> +=item B<checkpoint-current> I<domain> {[I<--name>] | [I<--security-info>]
> +[I<--no-domain>] [I<--size>] | [I<checkpointname>]}
> +
> +Without I<checkpointname>, this will output the checkpoint XML for the
> +domain's current checkpoint (if any). If I<--name> is specified,
> +output just the current checkpoint name instead of the full xml.
> +Otherwise, using I<--security-info> will also include security
> +sensitive information in the XML, using I<--size> will add XML
> +indicating roughly how much guest data has changed since the
> +checkpoint was created, and using I<--no-domain> will omit the
> +<domain> element from the output for a more compact view.
> +
> +With I<checkpointname>, this is a request to make the existing named
> +checkpoint become the current checkpoint.
> +
> +=item B<checkpoint-edit> I<domain> [I<checkpointname>] [I<--current>]
> +{[I<--rename>] | [I<--clone>]}
> +
> +Edit the XML configuration file for I<checkpointname> of a domain. If
> +both I<checkpointname> and I<--current> are specified, also force the
> +edited checkpoint to become the current snapshot. If
> +I<checkpointname> is omitted, then I<--current> must be supplied, to
> +edit the current checkpoint.
> +
> +This is equivalent to:
> +
> + virsh checkpoint-dumpxml dom name > checkpoint.xml
> + vi checkpoint.xml (or make changes with your other text editor)
> + virsh checkpoint-create dom checkpoint.xml --redefine [--current]
> +
> +except that it does some error checking.
> +
> +The editor used can be supplied by the C<$VISUAL> or C<$EDITOR> environment
> +variables, and defaults to C<vi>.
> +
> +If I<--rename> is specified, then the edits can change the checkpoint
> +name. If I<--clone> is specified, then changing the snapshot name
> +will create a clone of the checkpoint metadata. If neither is
> +specified, then the edits must not change the checkpoint name. Note
> +that changing a checkpoint name must be done with care, since some
> +drivers may require the original checkpoint name for actually
> +accessing changes since a point in time.
Please strongly discourage use of --clone and --rename. Also --redefine
isn't really described.
> +
> +=item B<checkpoint-info> I<domain> {I<checkpoint> | I<--current>}
> +
> +Output basic information about a named <checkpoint>, or the current
> +checkpoint with I<--current>.
> +
> +=item B<checkpoint-list> I<domain> [I<--metadata>] [I<--no-metadata>]
> +[{I<--parent> | I<--roots> | [{I<--tree> | I<--name>}]}] [I<--topological>]
> +[{[I<--from>] B<checkpoint> | I<--current>} [I<--descendants>]]
> +[I<--leaves>] [I<--no-leaves>]
> +
> +List all of the available checkpoints for the given domain, defaulting
> +to show columns for the checkpoint name and creation time.
> +
> +Normally, table form output is sorted by checkpoint name; using
> +I<--topological> instead sorts so that no child is listed before its
> +ancestors (although there may be more than one possible ordering with
> +this property).
> +
> +If I<--parent> is specified, add a column to the output table giving
> +the name of the parent of each checkpoint. If I<--roots> is
> +specified, the list will be filtered to just checkpoints that have no
> +parents. If I<--tree> is specified, the output will be in a tree
> +format, listing just checkpoint names. These three options are
> +mutually exclusive. If I<--name> is specified only the checkpoint name
> +is printed. This option is mutually exclusive with I<--tree>.
> +
> +If I<--from> is provided, filter the list to checkpoints which are
> +children of the given B<checkpoint>; or if I<--current> is provided,
> +start at the current checkpoint. When used in isolation or with
> +I<--parent>, the list is limited to direct children unless
> +I<--descendants> is also present. When used with I<--tree>, the use
> +of I<--descendants> is implied. This option is not compatible with
> +I<--roots>. Note that the starting point of I<--from> or I<--current>
> +is not included in the list unless the I<--tree> option is also
> +present.
> +
> +If I<--leaves> is specified, the list will be filtered to just
> +checkpoints that have no children. Likewise, if I<--no-leaves> is
> +specified, the list will be filtered to just checkpoints with
> +children. (Note that omitting both options does no filtering, while
> +providing both options will either produce the same list or error out
> +depending on whether the server recognizes the flags). Filtering
> +options are not compatible with I<--tree>.
> +
> +If I<--metadata> is specified, the list will be filtered to just
> +checkpoints that involve libvirt metadata, and thus would prevent
> +B<undefine> of a persistent domain, or be lost on B<destroy> of a
> +transient domain. Likewise, if I<--no-metadata> is specified, the
> +list will be filtered to just checkpoints that exist without the need
> +for libvirt metadata.
This does not exist.
> +
> +=item B<checkpoint-dumpxml> I<domain> I<snapshot> [I<--security-info>]
> +[I<--no-domain>] [I<--size>]
> +
> +Output the snapshot XML for the domain's checkpoint named
> +I<checkpoint>. Using I<--security-info> will also include security
> +sensitive information, using I<--size> will add XML indicating roughly
> +how much guest data has changed since the checkpoint was created, and
define "roughly". Upper bound?
> +using I<--no-domain> will omit the <domain> element from the output
> +for a more compact view. Use B<checkpoint-current> to easily access
> +the XML of the current snapshot. To grab the XML for all checkpoints
> +at once, use B<dumpxml --checkpoints>.
> +
> +=item B<checkpoint-parent> I<domain> {I<checkpoint> | I<--current>}
> +
> +Output the name of the parent checkpoint, if any, for the given
> +I<checkpoint>, or for the current checkpoint with I<--current>.
Again this adds bunch of technical debt by copying stuff from snapshots,
but if you pledge to fix it I'm willing to ACK it for now.
Obviously cmdCheckpointCurrent is dangerous and I don't see a point for
the setting part since I already complained, consider it an
pre-requisite for the ACK./
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
