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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On Thu, Aug 29, 2024 at 01:56:43PM GMT, Markus Armbruster wrote:
>> 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.
>
> ...
>> > @@ -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.
>
> I also dislike this interface.  Having only one number plus an
> optional boolean that controls its scale is not as easy to reason
> about.
>
>> 
>> 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.
>
> Eww.  I don't see this as the compelling reason to switch to 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.
>
> This seems reasonable to me.

To Daniil as well.  Since Peter is okay with it on behalf of libvirt, so
am I.

>                               We have enough places in QAPI where we
> want mutual exclusion (we mark both fields optional, but expect the
> user to provide exactly one or get an error), that I wonder if it is
> worth making it a first-class construct in QAPI (maybe I'm spoiled by
> the OneOf designation[1] in protobuf[2] used by gRPC[3] in
> kubernetes).

"One of a set of optional members" is a constraint the QAPI language
cannot express, so we have to leave enforcing it to handwritten code,
and documenting it to handwritten comments.

If it could express it, the constraint would be visible in
introspection, and we'd have less handwritten code.  The price is
additional QAPI language and generator complexity.  Sensible tradeoff?
Hard to tell without patches.  Risk: we pay for patches only to find out
the answer is no.  Another risk: we pay for patches, then take them just
because we already paid for them, then continue to pay upkeep.

I don't mean to say such QAPI language enhancements are a bad idea.  I'm
just pointing out the tradeoff to weigh, and the risks to take.

There are more constraints that could be supported by the language,
e.g. integer ranges.

>               Including the scale in the name is a bonus reason to
> switch the interface.

Yes, because the unit isn't obvious from the type.  It is for things
like byte counts, where we've succeeded at sticking to one plain
unit[*].  We failed for time, so there it isn't obvious.

> [1] https://protobuf.dev/programming-guides/proto3/#oneof
> [2] https://protobuf.dev/overview/
> [3] https://grpc.io/
>
>> 
>> Let's get additional input from management application developers.  I
>> cc'ed some.
>> 
>> Related: NetdevSocketOptions member @reconnect.

Could use the same treatment for consistency.  Not a demand.


[*] With few exceptions, such certain rate limits that use MBytes/s.