[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/5] blockcopy: virDomainBlockCopy with XML destination, typed params



On 08/24/14 05:32, Eric Blake wrote:
> This commit (finally) adds the virDomainBlockCopy API, with the
> intent that it will provide more power to the existing 'virsh
> blockcopy' command.
> 
> 'virsh blockcopy' was first added in Apr 2012 (v0.9.12), which
> corresponds to the upstream qemu 1.2 timeframe.  It was done as
> a hack on top of the existing virDomainBlockRebase() API call,
> for two reasons: 1) it was targetting a feature that landed first
> in downstream RHEL qemu, but had not stabilized in upstream qemu
> at the time (and indeed, 'drive-mirror' only landed upstream in
> qemu 1.3 with slight differences to the first RHEL attempt,
> and later gained further parameters like granularity and buf-size
> that are also worth exposing), and 2) extending an existing API
> allowed it to be backported without worrying about bumping .so
> versions.  A virDomainBlockCopy() API was proposed at that time
> [1], but we decided not to accept it into libvirt until after
> upstream qemu stabilized, and it ended up getting scrapped.
> Whether or not RHEL should have attempted adding a new feature
> without getting it upstream first is a debate that can be held
> another day; but enough time has now elapsed that we are ready to
> do the interface cleanly.
> 
> [1] https://www.redhat.com/archives/libvir-list/2012-April/msg00768.html
> 
> Delaying the creation of a clean API until now has also had a
> benefit: we've only recently learned of a shortcoming in the
> original design, namely, that it is unable to target a network
> destination (such as a gluster volume) because it hard-coded the
> assumption that the destination is a local file name.  Because
> of all the refactoring we've done to add virStorageSourcePtr, we
> are in a better position to declare an API that parses XML
> describing a host storage source as the copy destination, which
> was not possible had we implemented virDomainBlockCopy as it had
> been originally envisioned.
> 
> At least I had the foresight to create 'virsh blockcopy' as a
> separate command at the UI level (commit 1f06c00) rather than
> leaking the underlying API overload of virDomainBlockRebase onto
> shell users.
> 
> A note on the bandwidth option: virTypedParameters intentionally
> lacks unsigned long (since variable-width interaction between
> mixed 32- vs. 64-bit client/server setups is nasty), but we have
> to deal with the fact that we are interacting with existing older
> code that mistakenly chose unsigned long bandwidth at a point
> before we decided to prohibit it in all new API.  The typed
> parameter is therefore unsigned long long, and the implementation
> will have to do overflow detection on 32-bit platforms.
> 
> * include/libvirt/libvirt.h.in (virDomainBlockCopy): New API.
> (virDomainBlockJobType, virConnectDomainEventBlockJobStatus):
> Update related documentation.
> * src/libvirt.c (virDomainBlockCopy): Implement it.
> * src/libvirt_public.syms (LIBVIRT_1.2.8): Export it.
> * src/driver.h (_virDriver): New driver callback.
> 
> Signed-off-by: Eric Blake <eblake redhat com>
> ---
>  include/libvirt/libvirt.h.in |  57 +++++++++++++++++++--
>  src/driver.h                 |  11 +++-
>  src/libvirt.c                | 118 ++++++++++++++++++++++++++++++++++++++++++-
>  src/libvirt_public.syms      |   5 ++
>  4 files changed, 184 insertions(+), 7 deletions(-)
> 
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index 47ea695..89c8e63 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -2518,16 +2518,16 @@ typedef enum {
>       * flags), job ends on completion */
> 
>      VIR_DOMAIN_BLOCK_JOB_TYPE_COPY = 2,
> -    /* Block Copy (virDomainBlockRebase with flags), job exists as
> -     * long as mirroring is active */
> +    /* Block Copy (virDomainBlockCopy, or virDomainBlockRebase with
> +     * flags), job exists as long as mirroring is active */
> 
>      VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT = 3,
>      /* Block Commit (virDomainBlockCommit without flags), job ends on
>       * completion */
> 
>      VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT = 4,
> -    /* Active Block Commit (virDomainBlockCommit with flags), job
> -     * exists as long as sync is active */
> +    /* Active Block Commit (virDomainBlockCommit with flags), job exists
> +     * as long as sync is active */
> 
>  #ifdef VIR_ENUM_SENTINELS
>      VIR_DOMAIN_BLOCK_JOB_TYPE_LAST
> @@ -2597,6 +2597,53 @@ int           virDomainBlockRebase(virDomainPtr dom, const char *disk,
>                                     unsigned int flags);
> 
>  /**
> + * virDomainBlockCopyFlags:
> + *
> + * Flags available for virDomainBlockCopy().
> + */
> +typedef enum {
> +    VIR_DOMAIN_BLOCK_COPY_SHALLOW   = 1 << 0, /* Limit copy to top of source
> +                                                 backing chain */
> +    VIR_DOMAIN_BLOCK_COPY_REUSE_EXT = 1 << 1, /* Reuse existing external
> +                                                 file for a copy */
> +} virDomainBlockCopyFlags;
> +
> +/**
> + * VIR_DOMAIN_BLOCK_COPY_BANDWIDTH:
> + * Macro for the virDomainBlockCopy bandwidth tunable: it represents
> + * the maximum bandwidth (in MiB/s) used while getting the copy
> + * operation into the mirrored phase, with a type of ullong (although

MiB/s and ullong? thats an awful lot of speed. Shouldn't we go with
KiB/s? This might benefit slower networks. (although it may never
converge there)

> + * the hypervisor may restrict the set of valid values to a smaller
> + * range). Some hypervisors may lack support for this parameter, while
> + * still allowing a subsequent change of bandwidth via
> + * virDomainBlockJobSetSpeed().  The actual speed can be determined
> + * with virDomainGetBlockJobInfo().
> + */
> +#define VIR_DOMAIN_BLOCK_COPY_BANDWIDTH "bandwidth"
> +
> +/**
> + * VIR_DOMAIN_BLOCK_COPY_GRANULARITY:
> + * Macro for the virDomainBlockCopy granularity tunable: it represents
> + * the granularity at which the copy operation recognizes dirty pages,

I'd rather say "dirty blocks". Pages might indicate RAM memory pages.

> + * as an unsigned int, and must be a power of 2.
> + */
> +#define VIR_DOMAIN_BLOCK_COPY_GRANULARITY "granularity"
> +
> +/**
> + * VIR_DOMAIN_BLOCK_COPY_BUF_SIZE:
> + * Macro for the virDomainBlockCopy buffer size tunable: it represents
> + * how much data can be in flight between source and destination, as
> + * an unsigned int.

In bytes?

> + */
> +#define VIR_DOMAIN_BLOCK_COPY_BUF_SIZE "buf-size"
> +
> +int virDomainBlockCopy(virDomainPtr dom, const char *disk,
> +                       const char *destxml,
> +                       virTypedParameterPtr params,
> +                       int nparams,
> +                       unsigned int flags);
> +
> +/**
>   * virDomainBlockCommitFlags:
>   *
>   * Flags available for virDomainBlockCommit().
> @@ -4830,7 +4877,7 @@ typedef void (*virConnectDomainEventGraphicsCallback)(virConnectPtr conn,
>   * virConnectDomainEventBlockJobStatus:
>   *
>   * Tracks status of a virDomainBlockPull(), virDomainBlockRebase(),
> - * or virDomainBlockCommit() operation
> + * virDomainBlockCopy(), or virDomainBlockCommit() operation
>   */
>  typedef enum {
>      VIR_DOMAIN_BLOCK_JOB_COMPLETED = 0,
> diff --git a/src/driver.h b/src/driver.h
> index ba7c1fc..14933a7 100644
> --- a/src/driver.h
> +++ b/src/driver.h
> @@ -2,7 +2,7 @@
>   * driver.h: description of the set of interfaces provided by a
>   *           entry point to the virtualization engine
>   *
> - * Copyright (C) 2006-2013 Red Hat, Inc.
> + * Copyright (C) 2006-2014 Red Hat, Inc.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -1009,6 +1009,14 @@ typedef int
>                             unsigned int flags);
> 
>  typedef int
> +(*virDrvDomainBlockCopy)(virDomainPtr dom,
> +                         const char *path,
> +                         const char *destxml,
> +                         virTypedParameterPtr params,
> +                         int nparams,
> +                         unsigned int flags);
> +
> +typedef int
>  (*virDrvDomainBlockCommit)(virDomainPtr dom,
>                             const char *disk,
>                             const char *base,
> @@ -1382,6 +1390,7 @@ struct _virDriver {
>      virDrvDomainBlockJobSetSpeed domainBlockJobSetSpeed;
>      virDrvDomainBlockPull domainBlockPull;
>      virDrvDomainBlockRebase domainBlockRebase;
> +    virDrvDomainBlockCopy domainBlockCopy;
>      virDrvDomainBlockCommit domainBlockCommit;
>      virDrvConnectSetKeepAlive connectSetKeepAlive;
>      virDrvConnectIsAlive connectIsAlive;
> diff --git a/src/libvirt.c b/src/libvirt.c
> index 8349261..99f1dc1 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -19924,7 +19924,11 @@ virDomainBlockPull(virDomainPtr dom, const char *disk,
>   * The actual speed can be determined with virDomainGetBlockJobInfo().
>   *
>   * When @base is NULL and @flags is 0, this is identical to
> - * virDomainBlockPull().
> + * virDomainBlockPull().  When @flags contains VIR_DOMAIN_BLOCK_REBASE_COPY,
> + * this command is shorthand for virDomainBlockCopy() where the destination
> + * has file type, @bandwidth is passed as a typed parameter, and the flags
> + * control whether the destination format is raw, identical to the source,
> + * or probed from the reused file.
>   *
>   * Returns 0 if the operation has started, -1 on failure.
>   */
> @@ -19975,6 +19979,118 @@ virDomainBlockRebase(virDomainPtr dom, const char *disk,
> 
> 
>  /**
> + * virDomainBlockCopy:
> + * @dom: pointer to domain object
> + * @disk: path to the block device, or device shorthand
> + * @destxml: XML description of the copy destination
> + * @params: Pointer to block copy parameter objects, or NULL
> + * @nparams: Number of block copy parameters (this value can be the same or
> + *           less than the number of parameters supported)
> + * @flags: bitwise-OR of virDomainBlockCopyFlags
> + *
> + * Copy the guest-visible contents of a disk image to a new file described
> + * by @destxml.  The destination XML has a top-level element of <disk>, and
> + * resembles what is used when hot-plugging a disk via virDomainAttachDevice(),
> + * except that only sub-elements related to describing the new host resource
> + * are necessary (sub-elements related to the guest view, such as <target>,
> + * are ignored).  It is strongly recommended to include a <driver type='...'/>
> + * format designation for the destination, to avoid the potential of any
> + * security problem that might be caused by probing a file for its format.
> + *
> + * This command starts a long-running copy.  By default, the copy will pull
> + * the entire source chain into the destination file, but if @flags also
> + * contains VIR_DOMAIN_BLOCK_COPY_SHALLOW, then only the top of the source
> + * chain will be copied (the source and destination have a common backing
> + * file).  The format of the destination file is controlled by the <driver>
> + * sub-element of the XML.  The destination will be created unless the
> + * VIR_DOMAIN_BLOCK_COPY_REUSE_EXT flag is present stating that the file
> + * was pre-created with the correct format and metadata and sufficient
> + * size to hold the copy. In case the VIR_DOMAIN_BLOCK_COPY_SHALLOW flag
> + * is used the pre-created file has to exhibit the same guest visible contents
> + * as the backing file of the original image. This allows a management app to
> + * pre-create files with relative backing file names, rather than the default
> + * of absolute backing file names.
> + *
> + * A copy job has two parts; in the first phase, the @bandwidth parameter

@bandwidth is now provided as a typed param.

> + * affects how fast the source is pulled into the destination, and the job
> + * can only be canceled by reverting to the source file; progress in this
> + * phase can be tracked via the virDomainBlockJobInfo() command, with a
> + * job type of VIR_DOMAIN_BLOCK_JOB_TYPE_COPY.  The job transitions to the
> + * second phase when the job info states cur == end, and remains alive to
> + * mirror all further changes to both source and destination.  The user
> + * must call virDomainBlockJobAbort() to end the mirroring while choosing
> + * whether to revert to source or pivot to the destination.  An event is
> + * issued when the job ends, and depending on the hypervisor, an event may
> + * also be issued when the job transitions from pulling to mirroring.  If
> + * the job is aborted, a new job will have to start over from the beginning
> + * of the first phase.
> + *
> + * Some hypervisors will restrict certain actions, such as virDomainSave()
> + * or virDomainDetachDevice(), while a copy job is active; they may
> + * also restrict a copy job to transient domains.
> + *
> + * The @disk parameter is either an unambiguous source name of the
> + * block device (the <source file='...'/> sub-element, such as
> + * "/path/to/image"), or the device target shorthand (the
> + * <target dev='...'/> sub-element, such as "vda").  Valid names
> + * can be found by calling virDomainGetXMLDesc() and inspecting
> + * elements within //domain/devices/disk.
> + *
> + * The @params and @nparams arguments can be used to set hypervisor-specific
> + * tuning parameters, such as maximum bandwidth or granularity.
> + *
> + * This command is a superset of the older virDomainBlockRebase() when used
> + * with the VIR_DOMAIN_BLOCK_REBASE_COPY flag, and offers better control
> + * over the destination format, the ability to copy to a destination that
> + * is not a local file, and the possibility of additional tuning parameters.
> + *
> + * Returns 0 if the operation has started, -1 on failure.
> + */
> +int
> +virDomainBlockCopy(virDomainPtr dom, const char *disk,
> +                   const char *destxml,
> +                   virTypedParameterPtr params,
> +                   int nparams,
> +                   unsigned int flags)

Wow, XML, typed params and flags. Now that's future proof! :)


> +{
> +    virConnectPtr conn;
> +
> +    VIR_DOMAIN_DEBUG(dom,
> +                     "disk=%s, destxml=%s, params=%p, nparams=%d, flags=%x",
> +                     disk, destxml, params, nparams, flags);
> +    VIR_TYPED_PARAMS_DEBUG(params, nparams);
> +
> +    virResetLastError();
> +
> +    virCheckDomainReturn(dom, -1);
> +    conn = dom->conn;
> +
> +    virCheckReadOnlyGoto(conn->flags, error);
> +    virCheckNonNullArgGoto(disk, error);
> +    virCheckNonNullArgGoto(destxml, error);
> +    if (params)
> +        virCheckPositiveArgGoto(nparams, error);
> +    else
> +        virCheckZeroArgGoto(nparams, error);
> +
> +    if (conn->driver->domainBlockCopy) {
> +        int ret;
> +        ret = conn->driver->domainBlockCopy(dom, disk, destxml,
> +                                            params, nparams, flags);
> +        if (ret < 0)
> +            goto error;
> +        return ret;
> +    }
> +
> +    virReportUnsupportedError();
> +
> + error:
> +    virDispatchError(dom->conn);
> +    return -1;
> +}
> +
> +
> +/**
>   * virDomainBlockCommit:
>   * @dom: pointer to domain object
>   * @disk: path to the block device, or device shorthand
> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index 9f4016a..c3f3bd0 100644
> --- a/src/libvirt_public.syms
> +++ b/src/libvirt_public.syms
> @@ -670,4 +670,9 @@ LIBVIRT_1.2.7 {
>          virConnectGetDomainCapabilities;
>  } LIBVIRT_1.2.6;
> 
> +LIBVIRT_1.2.8 {
> +    global:
> +        virDomainBlockCopy;

One of us will have to rebase. I've recently posted a series that adds
API too :)

> +} LIBVIRT_1.2.7;
> +
>  # .... define new API here using predicted next version number ....
> 

Apart from a few DOC problems the API looks fine to me and should be
fairly future proof.

ACK to the design (once docs are fixed).

Peter

P.S.: I've run out of time to review the rest of this, but this should
be good enough to merge the rest a bit later.

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]