[libvirt PATCH] src: warn if client hits the max requests limit
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
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 :|
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
participants (2)
-
Daniel P. Berrangé -
Peter Krempa