[libvirt] [PATCH 6/8] backup: Add support for filtering based on current moment

[Peter Krempa]

On Fri, Jul 05, 2019 at 23:37:33 -0500, Eric Blake wrote:
> Right now, the snapshot API permits at most one current snapshot, and
> includes specific API for getting at that snapshot
> (virDomainHasCurrentSnapshot, virDomainSnapshotCurrent,
> virDomainSnapshotIsCurrent).  However, with upcoming checkpoints, it
> is conceivable that a hypervisor could mark multiple checkpoints as

The same happens to snapshots too. Since you can create an "incomplete"
snapshot by unselecting some disks and then do a complement snapshot of
all the remaining disks then you get to the same situation.

For more fun you can create a snapshot which partially overlaps and then
neither of the "current" snapshots makes sense any more.

> current (the qemu implementation has only one current checkpoint for
> an incremental backup, and computes the set of changes for a
> differential backup by merging a chain of checkpoints - but other
> hypervisors may treat all differential checkpoints as current).  As
> such, it is more desirable to avoid explicit API for getting at the
> one current checkpoint, and instead have the List API include a filter
> for that purpose.  Still, it is easier to implement that filter
> directly in the common virDomainMomentObjList code, since that is the
> only code that tracks the one moment that is current (both for
> existing snapshots, and for how qemu will be using current
> checkpoints).

I don't quite understand why we need the notion of the current
checkpoint anyways. Technically it's just an implementation detal (I
will object to the decisions separately in the patch which implements it
) and all the checkpoints are "current" by getting updates.

It even does not make sense to stop recording the differences at all
since the changed blocks are not kept separately thus if any of the
checkpoints stops being "current" (by stopping updating the bitmap or
chain of bitmaps) it becomes totally worthless since you won't be able
to reconstruct it.

As such I don't understand why we need the notion of the current
checkpoint altogether. It didn't clarify for me from all the
backs-and-forths when I was commenting about it, but you seem to think
it's necessary, but I can't seem to understand why. As long as the
justification is other than 'it's for symmetry with snapshots' (which is
a bogus justification) I'm willing to go with it, but as such since I
don't really undestand why this is necessary I won't be able to maintain
such code.

> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/conf/virdomainmomentobjlist.h | 11 +++++++++--
>  src/conf/virdomainmomentobjlist.c |  9 ++++++++-
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/src/conf/virdomainmomentobjlist.h b/src/conf/virdomainmomentobjlist.h
> index 4067e928f4..7628df5e29 100644
> --- a/src/conf/virdomainmomentobjlist.h
> +++ b/src/conf/virdomainmomentobjlist.h
> @@ -78,8 +78,10 @@ typedef enum {
>      VIR_DOMAIN_MOMENT_LIST_TOPOLOGICAL = (1 << 1),
>      VIR_DOMAIN_MOMENT_LIST_LEAVES      = (1 << 2),
>      VIR_DOMAIN_MOMENT_LIST_NO_LEAVES   = (1 << 3),
> -    VIR_DOMAIN_MOMENT_LIST_METADATA    = (1 << 4),
> -    VIR_DOMAIN_MOMENT_LIST_NO_METADATA = (1 << 5),
> +    VIR_DOMAIN_MOMENT_LIST_CURRENT     = (1 << 4),
> +    VIR_DOMAIN_MOMENT_LIST_NO_CURRENT  = (1 << 5),
> +    VIR_DOMAIN_MOMENT_LIST_METADATA    = (1 << 6),
> +    VIR_DOMAIN_MOMENT_LIST_NO_METADATA = (1 << 7),

Why the reorder?


>  } virDomainMomentFilters;
> 
>  #define VIR_DOMAIN_MOMENT_FILTERS_METADATA \

ACK
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190708/be238dbf/attachment-0001.sig>