Re: [Qemu-devel] [PATCH COLO-Frame v8 19/34] qmp event: Add event notification for COLO error

[Eric Blake]

On 07/29/2015 02:45 AM, zhanghailiang wrote:
> If some errors happen during VM's COLO FT stage, it's import to notify the 
> users

s/import/important/

> this event, Togehter with 'colo_lost_heartbeat', users can intervene in COLO's

s/this event,/of this event./
s/Togehter/Together/

> failover work immediately.
> If users don't want to get involved in COLO's failover verdict,
> it is still necessary to notify users that we exit COLO mode.
> 
> Cc: Markus Armbruster <address@hidden>
> Cc: Michael Roth <address@hidden>
> Signed-off-by: zhanghailiang <address@hidden>
> Signed-off-by: Li Zhijian <address@hidden>
> ---
>  docs/qmp/qmp-events.txt | 16 ++++++++++++++++
>  migration/colo.c        | 11 ++++++++++-
>  qapi/event.json         | 15 +++++++++++++++
>  3 files changed, 41 insertions(+), 1 deletion(-)
> 

Interface review:

> +++ b/docs/qmp/qmp-events.txt
> @@ -488,6 +488,22 @@ Example:
>  {"timestamp": {"seconds": 1432121972, "microseconds": 744001},
>   "event": "MIGRATION", "data": {"status": "completed"}}
>  
> +COLO_EXIT
> +---------

This file is alphabetical prior to your patch; please insert your event
prior to DEVICE_DELETED.

> +
> +Emitted when VM finish COLO mode due to some errors happening or

s/finish/finishes/

> +the request of users.
> +
> +Data: None.
> +
> + - "mode": COLO mode, 'primary' or 'secondary'
> + - "error": Error message (json-string, optional)
> +
> +Example:
> +
> +{"timestamp": {"seconds": 2032141960, "microseconds": 417172},
> + "event": "COLO_EXIT", "data": {"mode": "primary"}}

It might also be useful to provide a machine-parseable parameter on
whether the exit was due to an internal error vs. an external request.
Maybe by adding 'flag':'bool', since presence or absence of 'error' is
not as nice.

> +++ b/migration/colo.c

> @@ -581,7 +590,7 @@ out:
>              error_report("Secondary VM will take over work");
>              break;
>          }
> -        usleep(200*1000);
> +        usleep(200 * 1000);
>      }

Unrelated whitespace tweak. Please squash this hunk into the patch that
introduced the problem.

> +++ b/qapi/event.json
> @@ -255,6 +255,21 @@
>    'data': {'status': 'MigrationStatus'}}
>  
>  ##
> +# @COLO_EXIT
> +#
> +# Emitted when VM finish COLO mode due to some errors happening or

s/finish/finishes/

> +# the request of users.
> +#
> +# @mode: 'primary' or 'secondeary'.

s/secondeary/secondary/

> +#
> +# @error:  #optional, error message. Only present on error happening.
> +#
> +# Since: 2.4

2.5

> +##
> +{ 'event': 'COLO_EXIT',
> +  'data': {'mode': 'str', '*error':'str'}}

Please use 'mode':'COLOMode' (we already have an enum; for a finite set
of strings, it's nicer to use the enum than the open-coded 'str').

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature