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

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

Posted by Daniel P. Berrangé 1 year, 8 months ago

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@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 ?

 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);
         }
         virNetServerClientUpdateEvent(client);
 
-- 
2.37.2

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

Posted by Peter Krempa 1 year, 7 months ago

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

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

Posted by Daniel P. Berrangé 1 year, 7 months ago

ping

On Wed, Aug 24, 2022 at 04:21:02PM +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@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 ?
> 
>  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);
>          }
>          virNetServerClientUpdateEvent(client);
>  
> -- 
> 2.37.2
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|