[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH COLO-Frame v8 02/34] migration: Introduce capabi
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH COLO-Frame v8 02/34] migration: Introduce capability 'colo' to migration |
Date: |
Fri, 28 Aug 2015 15:54:28 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 |
On 07/29/2015 02:45 AM, zhanghailiang wrote:
> We add helper function colo_supported() to indicate whether
> colo is supported or not, with which we use to control whether or not
> showing 'colo' string to users, they can use qmp command
> 'query-migrate-capabilities' or hmp command 'info migrate_capabilities'
> to learn if colo is supported.
>
> Cc: Juan Quintela <address@hidden>
> Cc: Amit Shah <address@hidden>
> Cc: Eric Blake <address@hidden>
> Cc: Markus Armbruster <address@hidden>
> Signed-off-by: zhanghailiang <address@hidden>
> Signed-off-by: Yang Hongyang <address@hidden>
> Signed-off-by: Gonglei <address@hidden>
> ---
Just focusing on the interface.
> @@ -338,6 +339,9 @@ MigrationCapabilityStatusList
> *qmp_query_migrate_capabilities(Error **errp)
>
> caps = NULL; /* silence compiler warning */
> for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) {
> + if (i == MIGRATION_CAPABILITY_COLO && !colo_supported()) {
> + continue;
> + }
Interesting - you completely omit the capability if it can't be set to
true; so the presence of the capability is therefore the witness of
whether it works. Which means it is a true runtime probe, rather than a
static introspection, and therefore more accurate :)
> if (head == NULL) {
> head = g_malloc0(sizeof(*caps));
> caps = head;
> @@ -478,6 +482,13 @@ void
> qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
> }
>
> for (cap = params; cap; cap = cap->next) {
> + if (cap->value->capability == MIGRATION_CAPABILITY_COLO &&
> + cap->value->state && !colo_supported()) {
> + error_setg(errp, "COLO is not currently supported, please"
> + " configure with --enable-colo option in order
> to"
> + " support COLO feature");
> + continue;
> + }
This says that you error out if colo:true is requested but not
supported, but silently ignore colo:false even when it is unsupported.
Isn't it better to error out that colo is unsupported, regardless of
enabled true/false being requested, since you explicitly avoided
advertising the feature?
> +++ b/qapi-schema.json
> @@ -529,11 +529,14 @@
> # @auto-converge: If enabled, QEMU will automatically throttle down the guest
> # to speed up convergence of RAM migration. (since 1.6)
> #
> +# @colo: If enabled, migration will never end, and the state of VM in
> primary side
Long line. Please wrap to fit within 80 columns.
> +# will be migrated continuously to VM in secondary side. (since 2.4)
You've missed 2.4; change this to 2.5.
Grammar suggestion:
s/of VM in primary/of the VM on the primary/
s/to VM in secondary/to the VM on the secondary/
> +++ b/qmp-commands.hx
> @@ -3434,6 +3434,7 @@ Query current migration capabilities
> - "rdma-pin-all" : RDMA Pin Page state (json-bool)
> - "auto-converge" : Auto Converge state (json-bool)
> - "zero-blocks" : Zero Blocks state (json-bool)
> + - "colo" : COLO FT state (json-bool)
Acronym soup. Might be nice to state more human-readable text about what
'colo' actually controls (stating COarse-grain LOcking fault tolerance
would help).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH COLO-Frame v8 02/34] migration: Introduce capability 'colo' to migration,
Eric Blake <=