Re: [Qemu-devel] [PATCH COLO-Frame v12 25/38] qmp event: Add event notification for COLO error

[Eric Blake <eblake@redhat.com>]

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

On 12/15/2015 01:22 AM, zhanghailiang wrote:
> If some errors happen during VM's COLO FT stage, it's important to notify the users
> of this event. Together with 'colo_lost_heartbeat', users can intervene in COLO's
> 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 exited COLO mode.
> 
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> ---
> v11:
> - Fix several typos found by Eric
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---

> +++ b/docs/qmp-events.txt
> @@ -184,6 +184,23 @@ Example:
>  Note: The "ready to complete" status is always reset by a BLOCK_JOB_ERROR
>  event.
>  
> +COLO_EXIT
> +---------
> +
> +Emitted when VM finishes COLO mode due to some errors happening or
> +at the request of users.
> +
> +Data:
> +
> + - "mode": COLO mode, primary or secondary side (json-string)
> + - "reason":  the exit reason, internal error or external request. (json-string)
> + - "error": error message (json-string, operation)

s/operation/optional/
May want to word it as:

- "error": error message for human consumption (json-string, optional)

to point out that machines shouldn't parse it.

> +++ b/migration/colo.c
> @@ -18,6 +18,7 @@
>  #include "qemu/error-report.h"
>  #include "qemu/sockets.h"
>  #include "migration/failover.h"
> +#include "qapi-event.h"
>  
>  /* colo buffer */
>  #define COLO_BUFFER_BASE_SIZE (4 * 1024 * 1024)
> @@ -349,6 +350,11 @@ static void colo_process_checkpoint(MigrationState *s)
>  out:
>      if (ret < 0) {
>          error_report("%s: %s", __func__, strerror(-ret));

Unrelated: I mentioned in another thread that we may want to start
thinking about adding error_report_errno(); this would be another client.

> +++ b/qapi-schema.json
> @@ -778,6 +778,22 @@
>    'data': [ 'unknown', 'primary', 'secondary'] }
>  
>  ##
> +# @COLOExitReason
> +#
> +# The reason for a COLO exit
> +#
> +# @unknown: unknown reason
> +#

If we never return 'unknown', then it is not worth having it in the enum
(we can always add it later if we find a reason to have it; but adding
it now feels premature if the code base isn't using it).

Otherwise looks okay to me.

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