Re: [Qemu-devel] [PATCH COLO-Frame v10 18/38] COLO failover: Introduce a new command to trigger a failover

[Eric Blake <eblake@redhat.com>]

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

On 11/03/2015 04:56 AM, zhanghailiang wrote:
> We leave users to choose whatever heartbeat solution they want, if the heartbeat
> is lost, or other errors they detect, they can use experimental command
> 'x_colo_lost_heartbeat' to tell COLO to do failover, COLO will do operations
> accordingly.
> 
> 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>
> ---
> v10: Rename command colo_lost_hearbeat to experimental 'x_colo_lost_heartbeat'
> ---

> @@ -29,6 +30,9 @@ bool migration_incoming_enable_colo(void);
>  void migration_incoming_exit_colo(void);
>  void *colo_process_incoming_thread(void *opaque);
>  bool migration_incoming_in_colo_state(void);
> +
> +int get_colo_mode(void);

Should this return an enum type instead of an int?


> +++ b/migration/colo-comm.c
> @@ -20,6 +20,17 @@ typedef struct {
>  
>  static COLOInfo colo_info;
>  
> +int get_colo_mode(void)
> +{
> +    if (migration_in_colo_state()) {
> +        return COLO_MODE_PRIMARY;
> +    } else if (migration_incoming_in_colo_state()) {
> +        return COLO_MODE_SECONDARY;
> +    } else {
> +        return COLO_MODE_UNKNOWN;
> +    }
> +}

Particularly since it is always returning values of the same enum.

Not fatal to the patch, so much as a style issue.


> +void qmp_x_colo_lost_heartbeat(Error **errp)
> +{
> +    if (get_colo_mode() == COLO_MODE_UNKNOWN) {
> +        error_setg(errp, QERR_FEATURE_DISABLED, "colo");
> +        return;

We've slowly been trying to get rid of QERR_ usage.  But you aren't the
first user, and a global cleanup may be better. So I can overlook it for
now.

> +++ b/qapi-schema.json
> @@ -734,6 +734,32 @@
>              'checkpoint-reply', 'vmstate-send', 'vmstate-size',
>              'vmstate-received', 'vmstate-loaded' ] }
>  
> +##
> +# @COLOMode
> +#
> +# The colo mode
> +#
> +# @unknown: unknown mode
> +#
> +# @primary: master side
> +#
> +# @secondary: slave side
> +#
> +# Since: 2.5
> +##
> +{ 'enum': 'COLOMode',
> +  'data': [ 'unknown', 'primary', 'secondary'] }
> +
> +##
> +# @x-colo-lost-heartbeat
> +#
> +# Tell qemu that heartbeat is lost, request it to do takeover procedures.
> +#

The docs here are rather short, compared to your commit message (in
particular, the fact that it causes a different action depending on
whether it is sent to primary [takeover] or secondary [failover]).

> +# Since: 2.5

2.6 in both places.

-- 
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 --]