[libvirt] [PATCH v4 13/20] backup: Implement virsh support for checkpoints

[Eric Blake]

On 2/12/19 1:15 PM, John Ferlan wrote:
> 
> 
> On 2/6/19 2:18 PM, 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 at redhat.com>
>> ---
>>  tools/virsh-checkpoint.h |   29 +
>>  tools/virsh-completer.h  |    4 +
>>  tools/virsh-util.h       |    3 +
>>  tools/virsh.h            |    1 +
>>  po/POTFILES              |    1 +
>>  tools/Makefile.am        |    3 +-
>>  tools/virsh-checkpoint.c | 1329 ++++++++++++++++++++++++++++++++++++++
>>  tools/virsh-completer.c  |   52 +-
>>  tools/virsh-util.c       |   11 +
>>  tools/virsh.c            |    2 +
>>  10 files changed, 1433 insertions(+), 2 deletions(-)
>>  create mode 100644 tools/virsh-checkpoint.h
>>  create mode 100644 tools/virsh-checkpoint.c
> 
> virsh.pod would be useful in order to figure out what everything means.
> 

Gah. I completely forgot to copy-and-paste that portion of the snapshot
code.  Will rectify.

> This is just too much being added here to really give this a proper
> review. This really is a case where too much has been built up. Start
> with the basics and build up from there.
> 
> I have similar things as previously w/r/t 2 blank lines between
> functions and one argument per line per method - all newer norms than
> when -snapshot was created.
> 
> Also may as well use the VIR_AUTOFREE type functions for (char *)
> VIR_FREE's.  And VIR_AUTOPTR(virString) as approriate

Yep, on my list of things to clean up where I spot them, to avoid
regressing to the older state-of-the-art when I first copied this code
from snapshots.


>> +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"));
> 
> Could not find domain checkpoint '%s'
> 
> would be better

Particularly since it's not a snapshot.  (Too much copy-and-paste...)


>> +    /* TODO - worth adding this flag?
>> +    {.name = "quiesce",
>> +     .type = VSH_OT_BOOL,
>> +     .help = N_("quiesce guest's file systems")
>> +    },
>> +    */
> 
> More to do

At least adding it here is easy. Adding it in qemu is harder.


>> +/*
>> + * "checkpoint-edit" command
>> + */
> 
> Oh and this is *really* a scary thing to allow! Especially since IIRC
> the Assign function would just create a new one if a name didn't
> exist... There seems to be a few things that could happen without locks
> that could really mess things up.

Modeled after "snapshot-edit" - but yes, it is a scary-enough command
that splitting it to a separate commit doesn't hurt my feelings (nor
hold up the initial API).


>> +static bool
>> +cmdCheckpointCurrent(vshControl *ctl, const vshCmd *cmd)
>> +{
>> +    virDomainPtr dom = NULL;
>> +    bool ret = false;
>> +    int current;
>> +    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);
>> +
>> +        if (!(checkpoint = virDomainCheckpointLookupByName(dom,
>> +                                                           checkpointname, 0)))
>> +            goto cleanup;
>> +
>> +        xml = virDomainCheckpointGetXMLDesc(checkpoint,
>> +                                            VIR_DOMAIN_CHECKPOINT_XML_SECURE);
>> +        if (!xml)
>> +            goto cleanup;
>> +
>> +        if (!(checkpoint2 = virDomainCheckpointCreateXML(dom, xml, flags)))
>> +            goto cleanup;
> 
> I need a better map.  When creating a checkpoint a couple patches ago,
> the AssignDef would fail if the name already existed. This code seems to
> take existing XML with a name that would already exist.

Modeled after the existing 'snapshot-current', which can be used to
redefine WHICH snapshot is marked current (but oddly enough lacks a way
to mark NO snapshot as current).  It uses the _REDEFINE flag to do an
in-place edit to the virDomainCheckpointObjList on which checkpoint is
marked current (and based on the use of the _CURRENT flag to state which
checkpoint is the new current one).


>> +
>> +    dom = virshCommandOptDomain(ctl, cmd, NULL);
>> +    if (dom == NULL)
>> +        return false;
>> +
>> +    if (virshLookupCheckpoint(ctl, cmd, "checkpointname", true, dom,
>> +                              &checkpoint, &name) < 0)
>> +        goto cleanup;
>> +
>> +    vshPrint(ctl, "%-15s %s\n", _("Name:"), name);
>> +    vshPrint(ctl, "%-15s %s\n", _("Domain:"), virDomainGetName(dom));
> 
> Meaning we could have partial printing if the subequent fails

Yeah. And isn't the new norm to use the new table-printer that formats
columns nicely according to content width?  I'll have to see if snapshot
code has improved here in the meantime.


>> +    /* Children, Descendants.  */
>> +    flags = 0;
>> +    count = virDomainCheckpointListChildren(checkpoint, NULL, flags);
>> +    if (count < 0) {
>> +        if (last_error->code == VIR_ERR_NO_SUPPORT) {
>> +            vshResetLibvirtError();
>> +            ret = true;
>> +        }
>> +        goto cleanup;
>> +    }
>> +    vshPrint(ctl, "%-15s %d\n", _("Children:"), count);
>> +    flags = VIR_DOMAIN_CHECKPOINT_LIST_DESCENDANTS;
>> +    count = virDomainCheckpointListChildren(checkpoint, NULL, flags);
>> +    if (count < 0)
>> +        goto cleanup;
>> +    vshPrint(ctl, "%-15s %d\n", _("Descendants:"), count);
> 
> Is delineating children and desendants necessary?

Knowing that you have 1 child but 3 descendents (and therefore, 2 of the
3 descendents are grandchildren or below) is interesting, if only to
prove that the filtering API flags work :)


>> +    /* When mixing --from and --tree, we also want a copy of from
>> +     * in the list, but with no parent for that one entry.  */
> 
> Still mumbling about too many options.

And that snapshot-list has just as many options.

> 
> That's a lot of different options... I'm confused over multiple
> checkpoints without a parent. I would figure a checkpoint without a
> parent is the root or first checkpoint and children are essentially
> linear. But this tree concept just complicates things so much.

Snapshots can definitely be more than linear. I'm not sure if
checkpoints can easily become more than linear, but I don't want to rule
it out (after all, reverting to prior states can do weird things).

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