Re: [Qemu-devel] [PATCH COLO-Frame v8 16/34] COLO failover: Introduce a new command to trigger a failover

[zhanghailiang]

On 2015/8/29 6:06, Eric Blake wrote:
> On 07/29/2015 02:45 AM, zhanghailiang wrote:
> > We leave users to use whatever heartbeat solution they want, if the heartbeat
> > is lost, or other errors they detect, they can use command
> > 'colo_lost_heartbeat' to tell COLO to do failover, COLO will do operations
> > accordingly.
> >
> > For example,
> > If send the command to PVM, Primary will exit COLO mode, and takeover,
> > if to Secondary, Secondary will do failover work and at last takeover server.
>
> Grammar suggestion:
>
> For example, if the command is sent to the PVM, the Primary side will
> exit COLO mode and take over operation. If sent to the Secondary, the
> secondary will run failover work, then take over server operation to
> become the new Primary.
>
> > Cc: Luiz Capitulino <address@hidden>
> > Cc: Eric Blake <address@hidden>
> > Cc: Markus Armbruster <address@hidden>
> > Signed-off-by: zhanghailiang <address@hidden>
> > Signed-off-by: Li Zhijian <address@hidden>
> > Signed-off-by: Yang Hongyang <address@hidden>
> > ---
> >   hmp-commands.hx              | 15 +++++++++++++++
> >   hmp.c                        |  8 ++++++++
> >   hmp.h                        |  1 +
> >   include/migration/colo.h     |  4 ++++
> >   include/migration/failover.h | 20 ++++++++++++++++++++
> >   migration/Makefile.objs      |  2 +-
> >   migration/colo-comm.c        | 11 +++++++++++
> >   migration/colo-failover.c    | 41 +++++++++++++++++++++++++++++++++++++++++
> >   migration/colo.c             |  1 +
> >   qapi-schema.json             | 25 +++++++++++++++++++++++++
> >   qmp-commands.hx              | 19 +++++++++++++++++++
> >   stubs/migration-colo.c       |  8 ++++++++
> >   12 files changed, 154 insertions(+), 1 deletion(-)
> >   create mode 100644 include/migration/failover.h
> >   create mode 100644 migration/colo-failover.c
>
> Just an interface review.
>
> > +++ b/migration/colo.c
> > @@ -15,6 +15,7 @@
> >   #include "trace.h"
> >   #include "qemu/error-report.h"
> >   #include "qemu/sockets.h"
> > +#include "migration/failover.h"
> >
> >   /* Fix me: Convert to use QAPI */
>
> Just noticing this comment; I'm assuming v9 will do more with qapi?

Yes, i will convert them to QAPI in v9.

> > +++ b/qapi-schema.json
> > @@ -666,6 +666,31 @@
> >               '*tls-port': 'int', '*cert-subject': 'str' } }
> >
> >   ##
> > +# @COLOMode
> > +#
> > +# The colo mode
> > +#
> > +# @unknown: unknown mode
> > +#
> > +# @primary: master side
> > +#
> > +# @secondary: slave side
> > +#
> > +# Since: 2.4
>
> 2.5
>
> > +##
> > +{ 'enum': 'COLOMode',
> > +  'data': [ 'unknown', 'primary', 'secondary'] }
>
> Where is this enum exposed to the user? Might be worth mentioning in the
> commit message if a later patch will use it, and/or reshuffle the series
> to have the enum introduced and exposed in one patch (perhaps always
> with 'unknown' value), then actually manipulated in later patches.

It will be exposed to users in the later colo exit event, but here we also use 
it internally in this
patch.

> > +
> > +##
> > +# @colo-lost-heartbeat
> > +#
> > +# Tell qemu that heartbeat is lost, request it to do takeover working.
>
> s/working/procedures/
>
> > +#
> > +# Since: 2.4
>
> 2.5
>
> > +##
> > +{ 'command': 'colo-lost-heartbeat' }
> > +
> > +##
> >   # @MouseInfo:
> >   #
> >   # Information about a mouse device.
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index bb49a1a..28a7962 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -781,6 +781,25 @@ Example:
> >   EQMP
> >
> >       {
> > +        .name       = "colo-lost-heartbeat",
> > +        .args_type  = "",
> > +        .mhandler.cmd_new = qmp_marshal_input_colo_lost_heartbeat,
>
> Markus' work on introspection will s/_input//; depending on what patches
> go in first, there will be an obvious merge needed.

I will address all the comments in next version, thanks. :)