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

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

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 <lcapitulino@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: Li Zhijian <lizhijian@cn.fujitsu.com>
>> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
>> ---
>>   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. :)