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

[zhanghailiang]

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 <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>
> > ---
>
> > +++ 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.