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

[zhanghailiang <zhang.zhanghailiang@huawei.com>]

On 2015/10/3 0:02, Eric Blake wrote:
> On 09/02/2015 02:22 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>
>> ---
>
>> +++ b/migration/colo.c
>> @@ -0,0 +1,18 @@
>> +/*
>> + * COarse-grain LOck-stepping Virtual Machines for Non-stop Service (COLO)
>> + * (a.k.a. Fault Tolerance or Continuous Replication)
>> + *
>> + * Copyright (c) 2015 HUAWEI TECHNOLOGIES CO., LTD.
>> + * Copyright (c) 2015 FUJITSU LIMITED
>> + * Copyright (c) 2015 Intel Corporation
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>> + * later.  See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "migration/colo.h"
>> +
>> +bool colo_supported(void)
>> +{
>> +    return true;
>> +}
>
> So this exists because you have a configure option that says whether to
> include or exclude this .o file (the backup stub version returns false
> if this file is not included)...
>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 662e77e..593cac0 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -29,6 +29,7 @@
>>   #include "trace.h"
>>   #include "qapi/util.h"
>>   #include "qapi-event.h"
>> +#include "migration/colo.h"
>>
>>   #define MAX_THROTTLE  (32 << 20)      /* Migration speed throttling */
>>
>> @@ -345,6 +346,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;
>> +        }
>
> ...and here, you use the result to explicitly remove the colo migration
> property from the runtime result of query-migrate-capabilities if
> support was not compiled in.  That's a good thing, because it means that
> a client can call query-migrate-capabilities, and immediately know if
> qemu supports colo by whether the migration property is present.
>
> Especially since the new query-qmp-schema WILL show colo as a member of
> the enum of possible values.  Knowing that the enum supports a value
> doesn't tell you whether runtime supports use of the value, so your
> approach definitely helps in a place where static introspection doesn't
> solve everything.
>

Yes, that is the reason we add it.

>>           if (head == NULL) {
>>               head = g_malloc0(sizeof(*caps));
>>               caps = head;
>> @@ -485,6 +489,13 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>>       }
>>
>>       for (cap = params; cap; cap = cap->next) {
>> +        if (cap->value->capability == MIGRATION_CAPABILITY_COLO &&
>> +            !colo_supported()) {
>> +            error_setg(errp, "COLO is not currently supported, please"
>> +                             " configure with --enable-colo option in order to"
>> +                             " support COLO feature");
>> +            continue;
>
> Likewise, you explicitly reject attempts to change the property, where
> introspection says the enum supports the property.  It might be
> acceptable to silently permit attempts to set the capability to false
> (the value it already has) and only reject attempts to set it to true,
> but I'm not sure if the difference is worth it.
>

Ha, maybe you have forgotten, in last version, you pointed out that maybe it was
better to give this error message regardless of true/false being requested,
and i think it is reasonable, so i will keep it like this ;)

>> +        }
>>           s->enabled_capabilities[cap->value->capability] = cap->value->state;
>>       }
>>   }
>> @@ -913,6 +924,12 @@ int64_t migrate_xbzrle_cache_size(void)
>>       return s->xbzrle_cache_size;
>>   }
>>
>> +bool migrate_enable_colo(void)
>> +{
>> +    MigrationState *s = migrate_get_current();
>> +    return s->enabled_capabilities[MIGRATION_CAPABILITY_COLO];
>
> Function name is wrong - it sounds like an imperative command (call this
> to enable colo), when it is really a query command (call this to learn
> if colo is enabled).  Maybe migrate_colo_enabled()?
>

Sounds reasonable, will fix in next version.

>> +}
>> +
>>   /* migration thread support */
>>
>>   static void *migration_thread(void *opaque)
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 5f45571..b14d1f4 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -529,11 +529,15 @@
>>   # @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 the VM on the
>> +#        primary side will be migrated continuously to the VM on secondary
>> +#        side. (since 2.5)
>
> You may want to name this property 'x-colo' to mark it experimental;
> especially since there are still a lot of other things waiting to go in
> and we are getting closer to 2.5 freeze.  Making the property
> experimental will relieve the pressure of having to get everything right
> on the first try, although it also means that libvirt won't use the
> property until we graduate it from experimental 'x-colo' to
> fully-supported 'colo'.
>

Got it, will fix it in next version, thanks.