5:22 a.m.
On 18.03.2016 16:47, Jiri Denemark wrote:
On Fri, Mar 04, 2016 at 14:20:54 +0300, Nikolay Shirokovskiy wrote:
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
> ---
> include/libvirt/libvirt-domain.h | 11 +++
> src/qemu/qemu_driver.c | 29 +++++--
> src/qemu/qemu_migration.c | 163 ++++++++++++++++++++++++++++++++-------
> src/qemu/qemu_migration.h | 23 ++++++
> src/qemu/qemu_monitor.c | 2 +-
> src/qemu/qemu_monitor.h | 1 +
> 6 files changed, 195 insertions(+), 34 deletions(-)
>
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index 79c25df..b3a176f 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -671,6 +671,8 @@ typedef enum {
> * when supported */
> VIR_MIGRATE_UNSAFE = (1 << 9), /* force migration even if it is
considered unsafe */
> VIR_MIGRATE_OFFLINE = (1 << 10), /* offline migrate */
> + /* Migration options could be specified further via
VIR_MIGRATE_PARAM_COMPRESSION
> + * otherwise default options will be used */
Either put a short version of the above comment to the comment for
VIR_MIGRATE_COMPRESSED or add such comment to migration APIs (or both),
but the comment here is likely to confuse our documentation generator.
> VIR_MIGRATE_COMPRESSED = (1 << 11), /* compress data during
migration */
> VIR_MIGRATE_ABORT_ON_ERROR = (1 << 12), /* abort migration on I/O
errors happened during migration */
> VIR_MIGRATE_AUTO_CONVERGE = (1 << 13), /* force convergence */
> @@ -773,6 +775,15 @@ typedef enum {
> */
> # define VIR_MIGRATE_PARAM_MIGRATE_DISKS "migrate_disks"
>
> +/**
> + * VIR_MIGRATE_PARAM_COMPRESSION:
> + *
> + * virDomainMigrate* params multiple field: string list of compression methods
> + * that are used to compress migration traffic.
> + */
What is a "string list"? Is it a single string with method names
separated by something, an array of strings, or something completely
different? Oh, looking at the rest of this patch, it's the case of
allowing multiple instances of the same parameter. The documentation
should say that, e.g., "name of the method used to compress migration
traffic. The parameter may be specified multiple times if more than one
method should be used".
> +
Remove the empty line here.
> +# define VIR_MIGRATE_PARAM_COMPRESSION "compression"
> +
> /* Domain migration. */
> virDomainPtr virDomainMigrate (virDomainPtr domain, virConnectPtr dconn,
> unsigned long flags, const char *dname,
...
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 64cbffa..5fcf132 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -3299,6 +3299,39 @@ qemuMigrationPrepareIncoming(virDomainObjPtr vm,
> }
>
> static int
> +qemuMigrationSetCompression(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + qemuDomainAsyncJob job,
> + qemuMigrationCompressionPtr compression,
> + unsigned int flags)
> +{
> + /*
> + * if compression methods are not set explicitly use flags to
> + * set default compression methods
> + */
> + if (compression == NULL || compression->methods == 0) {
> + return qemuMigrationSetOption(driver, vm,
> + QEMU_MONITOR_MIGRATION_CAPS_XBZRLE,
> + flags & VIR_MIGRATE_COMPRESSED,
> + job);
> + }
I think it would be cleaner if qemuMigrationCompressionParseParams got
the flags and set QEMU_MIGRATION_COMPESS_XBZRLE in compression->methods
in case no compression parametr is used. Doing that would even fix a bug
in your code, because even if no VIR_MIGRATE_PARAM_COMPRESSION parameter
is used, you still want to explicitly disable any supported compression
capability. In other words, you'd still have to call
qemuMigrationSetOption for both XBZRLE and MULTITHREAD in the if
statement above. Changing qemuMigrationCompressionParseParams to do the
right thing will avoid this code duplication.
Hi, Jiri. Thanx for reviewing.
I think we'd better stick with this approach. The reason is that we can get
here thru old API where only flags are set and compression parameter
is set to NULL on the way. As to explicitly setting state of all
compression methods we can resolve flag into qemuMigrationCompressMethods
and use same code as in case of custom compression.
> +
> + if (qemuMigrationSetOption(driver, vm,
> + QEMU_MONITOR_MIGRATION_CAPS_XBZRLE,
> + compression->methods &
QEMU_MIGRATION_COMPESS_XBZRLE,
> + job) < 0)
> + return -1;
> +
> + if (qemuMigrationSetOption(driver, vm,
> + QEMU_MONITOR_MIGRATION_CAPS_COMPRESS,
> + compression->methods &
QEMU_MIGRATION_COMPESS_MULTITHREAD,
> + job) < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +static int
> qemuMigrationPrepareAny(virQEMUDriverPtr driver,
> virConnectPtr dconn,
> const char *cookiein,
...
> @@ -6293,3 +6339,66 @@ qemuMigrationErrorReport(virQEMUDriverPtr driver,
> virSetError(err);
> virFreeError(err);
> }
> +
> +static int
> +qemuMigrationCompressMethodsAdd(qemuMigrationCompressMethods *methods,
> + qemuMigrationCompressMethods method)
> +{
> + if (*methods & method) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("Compression method is specified twice."));
> + return -1;
> + }
> +
> + *methods |= method;
> + return 0;
> +}
I don't think there is a reason to split the above trivial code into a
separate function. The main reason is that the following function should
be simplified.
> +
> +int
> +qemuMigrationCompressionParseParams(qemuMigrationCompressionPtr compression,
> + virTypedParameterPtr params, int nparams)
> +{
> + size_t i;
> +
> + for (i = 0; i < nparams; i++) {
> + if (STRNEQ(params[i].field, VIR_MIGRATE_PARAM_COMPRESSION))
> + continue;
I think using virTypedParamsGetStringList to get all
VIR_MIGRATE_PARAM_COMPRESSION parameters and then walking through them
in a loop would look a bit nicer.
Well, this way we can remove the the check above at the price of introducing
variables and memory managment. Also we are not going to use the extracted list outside
of
the function. Is it worth the effort?
> +
> + if (STREQ(params[i].value.s, "xbzrle")) {
> + if (qemuMigrationCompressMethodsAdd(&compression->methods,
> + QEMU_MIGRATION_COMPESS_XBZRLE) <
0)
> + return -1;
> + } else if (STREQ(params[i].value.s, "multithread")) {
> + if (qemuMigrationCompressMethodsAdd(&compression->methods,
> + QEMU_MIGRATION_COMPESS_MULTITHREAD)
< 0)
> + return -1;
> + } else {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("Unsupported compression method"));
> + return -1;
> + }
Anyway, this is very ugly. You should change
qemuMigrationCompressMethods to be usable with our VIR_ENUM_{IMPL,DECL}
macros and use them here and in the *DumpParams function.
AFAIU I can not use VIR_ENUM_* family because I have flags and not sequential
values in enum. I can introduce another family and set of related functions
to deal with flags. Some generics to parse flags to virTypedParameterPtr
back and forth. Do you think it is time to take this way?
> + }
And set QEMU_MIGRATION_COMPESS_XBZRLE method if no param is passed and
flags enabled compression.
> +
> + return 0;
> +}
> +
> +int
> +qemuMigrationCompressionDumpParams(qemuMigrationCompressionPtr compression,
> + virTypedParameterPtr *params,
> + int *nparams,
> + int *maxparams)
> +{
> + if ((compression->methods & QEMU_MIGRATION_COMPESS_XBZRLE) &&
> + virTypedParamsAddString(params, nparams, maxparams,
> + VIR_MIGRATE_PARAM_COMPRESSION,
> + "xbzrle") < 0)
> + return -1;
> +
> + if ((compression->methods & QEMU_MIGRATION_COMPESS_MULTITHREAD)
&&
> + virTypedParamsAddString(params, nparams, maxparams,
> + VIR_MIGRATE_PARAM_COMPRESSION,
> + "multithread") < 0)
> + return -1;
For backward compatibility this would just need to keep params empty if
xbzrle was the only compression method specified and use only flags to
express that.
I suggest that we don't mix flags and compression parameters here and
in parse function too. This way we do not get backward compatibility
issues. If VIR_MIGRATE_COMPRESSED is specified we don't touch
it and do not convert to compression parameters. This way the old
value space is passed transparently by the old means. On dump compression.methods
will be 0 and we do not add params unrecognized by older libvirt.
If we start to mix compression in flags and parameters we have
check on dump that new value space is representable in old value
space and probably have to keep this place up to date with
compression methods and their tunables.
The only drawback I see on this path is that we have somewhat obscure
check 'compression == NULL || compression->methods == 0) in
qemuMigrationSetCompression(why == 0 check for?). The first
check is for the non params API calls where compression is
just not setted. The second is for params API but when compression
is not specified by newly introduced means and we should fallback
to handling flag value.
> +
> + return 0;
> +}
> diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
> index 2c67a02..3cbe944 100644
> --- a/src/qemu/qemu_migration.h
> +++ b/src/qemu/qemu_migration.h
> @@ -25,6 +25,9 @@
> # include "qemu_conf.h"
> # include "qemu_domain.h"
>
> +typedef struct _qemuMigrationCompression qemuMigrationCompression;
> +typedef qemuMigrationCompression *qemuMigrationCompressionPtr;
> +
> /* All supported qemu migration flags. */
> # define QEMU_MIGRATION_FLAGS \
> (VIR_MIGRATE_LIVE | \
> @@ -53,6 +56,8 @@
> VIR_MIGRATE_PARAM_LISTEN_ADDRESS, VIR_TYPED_PARAM_STRING, \
> VIR_MIGRATE_PARAM_MIGRATE_DISKS, VIR_TYPED_PARAM_STRING | \
> VIR_TYPED_PARAM_MULTIPLE, \
> + VIR_MIGRATE_PARAM_COMPRESSION, VIR_TYPED_PARAM_STRING | \
> + VIR_TYPED_PARAM_MULTIPLE, \
> NULL
>
>
> @@ -72,6 +77,22 @@ typedef enum {
> } qemuMigrationJobPhase;
> VIR_ENUM_DECL(qemuMigrationJobPhase)
>
> +typedef enum {
> + QEMU_MIGRATION_COMPESS_XBZRLE = (1 << 0),
> + QEMU_MIGRATION_COMPESS_MULTITHREAD = (1 << 1),
> +} qemuMigrationCompressMethods;
> +
> +struct _qemuMigrationCompression {
> + qemuMigrationCompressMethods methods;
This should rather be unsigned long (or unsigned long long).
> +};
> +
> +int qemuMigrationCompressionParseParams(qemuMigrationCompressionPtr compression,
> + virTypedParameterPtr params, int nparams);
> +int qemuMigrationCompressionDumpParams(qemuMigrationCompressionPtr compression,
> + virTypedParameterPtr *params,
> + int *nparams,
> + int *maxparams);
> +
> int qemuMigrationJobStart(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> qemuDomainAsyncJob job)
...
Jirka