Re: [PATCH] chardev: allow specifying finer-grained reconnect timeouts

[Markus Armbruster <armbru@redhat.com>]

Daniil Tatianin <d-tatianin@yandex-team.ru> writes:

> The "reconnect" option only allows to specify the time in seconds,
> which is way too long for certain workflows.
>
> We have a lightweight disk backend server, which takes about 20ms to
> live update, but due to this limitation in QEMU, previously the guest
> disk controller would hang for one second because it would take this
> long for QEMU to reinitialize the socket connection.
>
> Make it possible to specify a smaller timeout by treating the value in
> "reconnect" as milliseconds via the new "reconnect-is-ms" option.
>
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>

Your use case demonstrates that a granularity of seconds was the wrong
choice for the reconnection delay.

[...]

> diff --git a/qapi/char.json b/qapi/char.json
> index ef58445cee..61aeccf09d 100644
> --- a/qapi/char.json
> +++ b/qapi/char.json
> @@ -272,8 +272,13 @@
>  #     (default: false) (Since: 3.1)
>  #
>  # @reconnect: For a client socket, if a socket is disconnected, then
> -#     attempt a reconnect after the given number of seconds.  Setting
> -#     this to zero disables this function.  (default: 0) (Since: 2.2)
> +#     attempt a reconnect after the given number of seconds (unless
> +#     @reconnect-is-ms is set to true, in that case the number is
> +#     treated as milliseconds).  Setting this to zero disables
> +#     this function.  (default: 0) (Since: 2.2)
> +#
> +# @reconnect-is-ms: The value specified in @reconnect should be treated
> +#     as milliseconds.  (default: false) (Since: 9.2)
>  #
>  # Since: 1.4
>  ##
> @@ -287,7 +292,8 @@
>              '*telnet': 'bool',
>              '*tn3270': 'bool',
>              '*websocket': 'bool',
> -            '*reconnect': 'int' },
> +            '*reconnect': 'int',
> +            '*reconnect-is-ms': 'bool' },
>    'base': 'ChardevCommon' }
>  
>  ##

I don't like this interface.

   PRO: compatible extension; no management application updates needed
   unless they want to support sub-second delays.

   CON: specifying a sub-second delay takes two parameters, which is
   awkward.

   CON: trap in combination with -set.  Before the patch, something like
   -set chardev.ID.reconnect=N means N seconds no matter what.
   Afterwards, it depends on the value of reconnect-is-ms, which may be
   set far away.  Mitigating factor: -set is obscure.

Alternatives:

1. Change @reconnect to 'number', specify sub-second delays as
   fractions.

   PRO: compatible extension; no management application updates needed
   unless they want to support sub-second delays.

   CON: first use of floating-point for time in seconds in QAPI, as far
   as I can see.

   CON: QemuOpts can't do floating-point.

2. Deprecate @reconnect in favour of a new member with a more suitable
   unit.  Error if both are present.

   PRO: after @reconnect is gone, the interface is what it arguably
   should've been from the start.

   CON: incompatible change.  Management application updates needed
   within the deprecation grace period.

Let's get additional input from management application developers.  I
cc'ed some.

Related: NetdevSocketOptions member @reconnect.