[libvirt] [PATCH v8 04/21] backup: Introduce virDomainBackup APIs

Peter Krempa pkrempa at redhat.com
Wed Apr 24 13:50:46 UTC 2019


On Wed, Apr 17, 2019 at 09:09:04 -0500, Eric Blake wrote:
> Introduce a few new public APIs related to incremental backups.  This
> builds on the previous notion of a checkpoint (without an existing
> checkpoint, the new API is a full backup, differing from
> virDomainBlockCopy in the point of time chosen and in operation on
> multiple disks at once); and also allows creation of a new checkpoint
> at the same time as starting the backup (after all, an incremental
> backup is only useful if it covers the state since the previous
> backup).  Snapshot creation is also a point in time at which creating
> a checkpoint atomically can be useful; as checkpoints are independent
> objects, it is not worth embedding <domaincheckpoint> inside
> <domainsnapshot>, and therefore we need a more powerful version of
> virDomainSnapshotCreateXML(), where we borrow from the naming pattern
> of virDomainMigrate2() and friends.
> 
> A backup job also affects filtering a listing of domains, as well as
> adding event reporting for signaling when a push model backup
> completes (where the hypervisor creates the backup); note that the
> pull model does not have an event (starting the backup lets a third
> party access the data, and only the third party knows when it is
> finished).
> 
> Since multiple backup jobs can be run in parallel in the future (well,
> qemu doesn't support it yet, but we don't want to preclude the idea),
> virDomainBackupBegin() returns a positive job id, and the id is also
> visible in the backup XML. But until a future libvirt release adds a
> bunch of APIs related to parallel job management where job ids will
> actually matter, the documentation is also clear that job id 0 means
> the 'currently running backup job' (provided one exists), for use in
> virDomainBackupGetXMLDesc() and virDomainBackupEnd().
> 
> The full list of new APIs:
>         virDomainBackupBegin;
>         virDomainBackupEnd;
>         virDomainBackupGetXMLDesc;
>         virDomainSnapshotCreateXML2;
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
>  include/libvirt/libvirt-domain-snapshot.h |   8 +-
>  include/libvirt/libvirt-domain.h          |  41 +++-
>  src/driver-hypervisor.h                   |  21 +++
>  src/qemu/qemu_blockjob.h                  |   1 +
>  examples/object-events/event-test.c       |   3 +
>  src/conf/domain_conf.c                    |   2 +-
>  src/libvirt-domain-snapshot.c             |  89 +++++++++
>  src/libvirt-domain.c                      | 219 ++++++++++++++++++++++
>  src/libvirt_public.syms                   |   4 +
>  tools/virsh-domain.c                      |   8 +-
>  10 files changed, 390 insertions(+), 6 deletions(-)

[...]

> 
> @@ -78,6 +78,12 @@ virDomainSnapshotPtr virDomainSnapshotCreateXML(virDomainPtr domain,
>                                                  const char *xmlDesc,
>                                                  unsigned int flags);
> 
> +/* Take a snapshot of the current VM state, possibly creating a checkpoint */
> +virDomainSnapshotPtr virDomainSnapshotCreateXML2(virDomainPtr domain,
> +                                                 const char *xmlDesc,
> +                                                 const char *checkpointXml,
> +                                                 unsigned int flags);
> +

This feels weird in this patch. It does not deal with backup in any way. It would
be better to have this separately or at least together with checkpoint
creating.

>  typedef enum {
>      VIR_DOMAIN_SNAPSHOT_XML_SECURE         = VIR_DOMAIN_XML_SECURE, /* dump security sensitive information too */
>  } virDomainSnapshotXMLFlags;

[...]

> @@ -1376,6 +1382,17 @@ typedef int
>  (*virDrvDomainCheckpointDelete)(virDomainCheckpointPtr checkpoint,
>                                  unsigned int flags);
> 
> +typedef int
> +(*virDrvDomainBackupBegin)(virDomainPtr domain, const char *diskXml,
> +                           const char *checkpointXml, unsigned int flags);
> +
> +typedef char *
> +(*virDrvDomainBackupGetXMLDesc)(virDomainPtr domain, int id,
> +                                unsigned int flags);
> +
> +typedef int
> +(*virDrvDomainBackupEnd)(virDomainPtr domain, int id, unsigned int flags);

Since the getter APIs are overloaded on top of shouldn't this be
overloaded with qemuDomainAbortJob as well?

> +
>  typedef struct _virHypervisorDriver virHypervisorDriver;
>  typedef virHypervisorDriver *virHypervisorDriverPtr;

[...]


> diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h
> index c7325c6daf..95f53dde16 100644
> --- a/src/qemu/qemu_blockjob.h
> +++ b/src/qemu/qemu_blockjob.h
> @@ -55,6 +55,7 @@ typedef enum {
>      QEMU_BLOCKJOB_TYPE_COPY = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY,
>      QEMU_BLOCKJOB_TYPE_COMMIT = VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT,
>      QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT = VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT,
> +    QEMU_BLOCKJOB_TYPE_BACKUP = VIR_DOMAIN_BLOCK_JOB_TYPE_BACKUP,

Here it also tries to behave like a block job. That might be a problem
if a user tries to virDomainBlockJobAbort it as you can't do that
partially. See at the end for more explanation.


[...]

> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 44438c3be8..67ae2a9ecd 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -10351,6 +10351,12 @@ virDomainBlockRebase(virDomainPtr dom, const char *disk,
>   * over the destination format, the ability to copy to a destination that
>   * is not a local file, and the possibility of additional tuning parameters.
>   *
> + * The copy created by this API is not finalized until the job ends,
> + * and does not lend itself to incremental backups (beyond what
> + * VIR_DOMAIN_BLOCK_COPY_SHALLOW provides) nor to third-party control
> + * over the data being copied.  For those features, use
> + * virDomainBackupBegin().

I'd not mention this at all.

> + *
>   * Returns 0 if the operation has started, -1 on failure.
>   */
>  int

In general I'm worried about the design semantics here. I see three
distinct pattern happening:

1) The backup tries to behave as a domain job for querying and event
2) The backup tries to behave as a separate kind of job for ending it
3) The backup is also documented as a block job.

Intermixing all of these kinds will create "interresting" semantics. The
domain job can be aborted using virDomainAbortJob API while the backup
can't but it can be queried as such. The backup also is listed as a
block job but virDomainBlockJobAbort can handle only a single
disk-related job, but a backup spans multiple disks.

Also the stats reporting will break as soon as multiple jobs are
supported as it overloads virDomainGetJobStats.
-------------- 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/20190424/227e44d0/attachment-0001.sig>


More information about the libvir-list mailing list