Re: [Qemu-devel] [PATCH COLO-Frame v8 02/34] migration: Introduce capability 'colo' to migration

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3490 bytes --]

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 <quintela@redhat.com>
> Cc: Amit Shah <amit.shah@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]