[libvirt PATCH] src: warn if client hits the max requests limit

[Peter Krempa]

On Wed, Aug 24, 2022 at 16:21:02 +0100, Daniel P. Berrangé wrote:
> Since they are simply normal RPC messages, the keep alive packets are
> subject to the "max_client_requests" limit just like any API calls.
> 
> Thus, if a client hits the 'max_client_requests' limit and all the
> pending API calls take a long time to complete, it may result in
> keep-alives firing and dropping the client connection.
> 
> This has been seen by a number of users with the default value of
> max_client_requests=5, by issuing 5 concurrent live migration
> operations.
> 
> By printing a warning message when this happens, admins will be alerted
> to the fact that their active clients are exceeding the default client
> requests limit.
> 
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
> 
> I'm a little wary of this change. If we use anything less than VIR_WARN
> it is not going to be useful, as we need it visible by default. At the
> same time though I'm concerned that this might expose very many
> deployments using an unreasonably low max_client_requests value for
> their workload. For example OpenStack deployment tools have often left
> this on the default setting and have been known to exceed it with live
> migration running concurrently.
> 
> One possible optimization would be to only issue this warning once per
> connected client, so we don't spam repeatedly ?

I think it will be okay if we don't spam the logs too much. Without
rate limiting the warning somehow the big deployments in question may
simply keep hammering the warning into the logs, so I think you should
emit it only once.

> 
>  src/rpc/virnetserverclient.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
> index d57ca07167..0d82726194 100644
> --- a/src/rpc/virnetserverclient.c
> +++ b/src/rpc/virnetserverclient.c
> @@ -1259,6 +1259,10 @@ static virNetMessage *virNetServerClientDispatchRead(virNetServerClient *client)
>                  client->rx->buffer = g_new0(char, client->rx->bufferLength);
>                  client->nrequests++;
>              }
> +        } else {
> +            VIR_WARN("Client hit max requests limit %zd. This may result "
> +                     "in keep-alive timeouts. Consider tuning the "
> +                     "max_client_requests server parameter", client->nrequests);

Please add apostrophes around the %zd substitution. preferrably also
format the code to avoid linebreaks (at least mid sentence) for
greppability reasons.

>          }
>          virNetServerClientUpdateEvent(client);
>  
> -- 
> 2.37.2
>