[libvirt] [PATCH 1/5] blockcopy: virDomainBlockCopy with XML destination, typed params
Peter Krempa
pkrempa at redhat.com
Mon Aug 25 17:20:33 UTC 2014
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 at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140825/e2a78778/attachment-0001.sig>
More information about the libvir-list
mailing list