[libvirt] [PATCH 0/5] admin: Fix updating of the client limits
This series slightly refactors the code in virnetserver.c in order to resolve https://bugzilla.redhat.com/show_bug.cgi?id=1357776. The problem resides in updating of the client limits which even though updated, thus "permitting" new connections to be established, do not re-enable polling for the socket file descriptor which results in all new connections still queueing on the socket. Erik Skultety (5): rpc: virnetserver: Rename ClientSetProcessingControls to ClientSetLimits rpc: virnetserver: Move virNetServerCheckLimits which is static up in the file rpc: virnetserver: Add code to CheckLimits to handle suspending of services admin: rpc: virnetserver: Fix updating of the client limits rpc: virnetserver: Remove dead code checking the client limits daemon/admin_server.c | 4 +-- src/rpc/virnetserver.c | 97 +++++++++++++++++++++++--------------------------- src/rpc/virnetserver.h | 6 ++-- 3 files changed, 50 insertions(+), 57 deletions(-) -- 2.5.5
The original naming was just a leftover that should have been fixed in commit 8b1f0469. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- daemon/admin_server.c | 4 ++-- src/rpc/virnetserver.c | 6 +++--- src/rpc/virnetserver.h | 6 +++--- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/daemon/admin_server.c b/daemon/admin_server.c index 9f24f68..25bfc7b 100644 --- a/daemon/admin_server.c +++ b/daemon/admin_server.c @@ -382,8 +382,8 @@ adminServerSetClientLimits(virNetServerPtr srv, VIR_SERVER_CLIENTS_UNAUTH_MAX))) maxClientsUnauth = param->value.ui; - if (virNetServerSetClientProcessingControls(srv, maxClients, - maxClientsUnauth) < 0) + if (virNetServerSetClientLimits(srv, maxClients, + maxClientsUnauth) < 0) return -1; return 0; diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 8c8af97..13b5bd3 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -1046,9 +1046,9 @@ virNetServerGetClient(virNetServerPtr srv, } int -virNetServerSetClientProcessingControls(virNetServerPtr srv, - long long int maxClients, - long long int maxClientsUnauth) +virNetServerSetClientLimits(virNetServerPtr srv, + long long int maxClients, + long long int maxClientsUnauth) { int ret = -1; size_t max, max_unauth; diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h index 38107b4..151bac8 100644 --- a/src/rpc/virnetserver.h +++ b/src/rpc/virnetserver.h @@ -116,8 +116,8 @@ size_t virNetServerGetCurrentClients(virNetServerPtr srv); size_t virNetServerGetMaxUnauthClients(virNetServerPtr srv); size_t virNetServerGetCurrentUnauthClients(virNetServerPtr srv); -int virNetServerSetClientProcessingControls(virNetServerPtr srv, - long long int maxClients, - long long int maxClientsUnauth); +int virNetServerSetClientLimits(virNetServerPtr srv, + long long int maxClients, + long long int maxClientsUnauth); #endif /* __VIR_NET_SERVER_H__ */ -- 2.5.5
Since virNetServerAddClient checks for the limits in order to temporarily suspend the services, thus not accepting any more clients, there is no reason why virNetServerCheckLimits, which is only responsible for re-enabling previously disabled services according to the limits, could not do both. To be able to do that however, it needs to be moved up in the file since it's static (and because it's just a helper and there's only one caller it should remain static). Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/rpc/virnetserver.c | 57 +++++++++++++++++++++++++------------------------- 1 file changed, 28 insertions(+), 29 deletions(-) diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 13b5bd3..0c502d9 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -234,6 +234,34 @@ static int virNetServerDispatchNewMessage(virNetServerClientPtr client, return ret; } +/** + * virNetServerCheckLimits: + * @srv: server to check limits on + * + * Check if limits like max_clients or max_anonymous_clients + * are satisfied and if so, re-enable accepting new clients. + * The @srv must be locked when this function is called. + */ +static void +virNetServerCheckLimits(virNetServerPtr srv) +{ + /* Enable services if we can accept a new client. + * The new client can be accepted if both max_clients and + * max_anonymous_clients wouldn't get overcommitted by + * accepting it. */ + VIR_DEBUG("Considering re-enabling services: " + "nclients=%zu nclients_max=%zu " + "nclients_unauth=%zu nclients_unauth_max=%zu", + srv->nclients, srv->nclients_max, + srv->nclients_unauth, srv->nclients_unauth_max); + if (srv->nclients < srv->nclients_max && + (!srv->nclients_unauth_max || + srv->nclients_unauth < srv->nclients_unauth_max)) { + /* Now it makes sense to accept() a new client. */ + VIR_INFO("Re-enabling services"); + virNetServerUpdateServicesLocked(srv, true); + } +} int virNetServerAddClient(virNetServerPtr srv, virNetServerClientPtr client) @@ -737,35 +765,6 @@ void virNetServerUpdateServices(virNetServerPtr srv, virObjectUnlock(srv); } -/** - * virNetServerCheckLimits: - * @srv: server to check limits on - * - * Check if limits like max_clients or max_anonymous_clients - * are satisfied and if so, re-enable accepting new clients. - * The @srv must be locked when this function is called. - */ -static void -virNetServerCheckLimits(virNetServerPtr srv) -{ - /* Enable services if we can accept a new client. - * The new client can be accepted if both max_clients and - * max_anonymous_clients wouldn't get overcommitted by - * accepting it. */ - VIR_DEBUG("Considering re-enabling services: " - "nclients=%zu nclients_max=%zu " - "nclients_unauth=%zu nclients_unauth_max=%zu", - srv->nclients, srv->nclients_max, - srv->nclients_unauth, srv->nclients_unauth_max); - if (srv->nclients < srv->nclients_max && - (!srv->nclients_unauth_max || - srv->nclients_unauth < srv->nclients_unauth_max)) { - /* Now it makes sense to accept() a new client. */ - VIR_INFO("Re-enabling services"); - virNetServerUpdateServicesLocked(srv, true); - } -} - void virNetServerDispose(void *obj) { virNetServerPtr srv = obj; -- 2.5.5
So far, virNetServerCheckLimits was only used to possibly re-enable accepting new clients that might have previously by disabled due to client limits violation (max_clients, max_anonymous_clients). This patch refactors virNetServerAddClient which is currently the only place where the services get disabled in order to use the virNetServerCheckLimits helper instead of checking the limits by itself. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/rpc/virnetserver.c | 45 ++++++++++++++++++++++----------------------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 0c502d9..c5caef3 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -239,24 +239,35 @@ static int virNetServerDispatchNewMessage(virNetServerClientPtr client, * @srv: server to check limits on * * Check if limits like max_clients or max_anonymous_clients - * are satisfied and if so, re-enable accepting new clients. + * are satisfied. If so, re-enable accepting new clients. If these are violated + * however, temporarily disable accepting new clients. * The @srv must be locked when this function is called. */ static void virNetServerCheckLimits(virNetServerPtr srv) { - /* Enable services if we can accept a new client. - * The new client can be accepted if both max_clients and - * max_anonymous_clients wouldn't get overcommitted by - * accepting it. */ - VIR_DEBUG("Considering re-enabling services: " - "nclients=%zu nclients_max=%zu " + VIR_DEBUG("Checking client-related limits to re-enable or temporarily " + "suspend services: nclients=%zu nclients_max=%zu " "nclients_unauth=%zu nclients_unauth_max=%zu", srv->nclients, srv->nclients_max, srv->nclients_unauth, srv->nclients_unauth_max); - if (srv->nclients < srv->nclients_max && - (!srv->nclients_unauth_max || - srv->nclients_unauth < srv->nclients_unauth_max)) { + + /* Check the max_anonymous_clients and max_clients limits so that we can + * decide whether the services should be temporarily suspended, thus not + * accepting any more clients for a while or re-enabling the previously + * suspended services in order to accept new clients again. + * A new client can only be accepted if both max_clients and + * max_anonymous_clients wouldn't get overcommitted by accepting it. + */ + if (srv->nclients >= srv->nclients_max || + (srv->nclients_unauth_max && + srv->nclients_unauth >= srv->nclients_unauth_max)) { + /* Temporarily stop accepting new clients */ + VIR_INFO("Temporarily suspending services"); + virNetServerUpdateServicesLocked(srv, false); + } else if (srv->nclients < srv->nclients_max && + (!srv->nclients_unauth_max || + srv->nclients_unauth < srv->nclients_unauth_max)) { /* Now it makes sense to accept() a new client. */ VIR_INFO("Re-enabling services"); virNetServerUpdateServicesLocked(srv, true); @@ -286,19 +297,7 @@ int virNetServerAddClient(virNetServerPtr srv, if (virNetServerClientNeedAuth(client)) virNetServerTrackPendingAuthLocked(srv); - if (srv->nclients_unauth_max && - srv->nclients_unauth == srv->nclients_unauth_max) { - /* Temporarily stop accepting new clients */ - VIR_INFO("Temporarily suspending services " - "due to max_anonymous_clients"); - virNetServerUpdateServicesLocked(srv, false); - } - - if (srv->nclients == srv->nclients_max) { - /* Temporarily stop accepting new clients */ - VIR_INFO("Temporarily suspending services due to max_clients"); - virNetServerUpdateServicesLocked(srv, false); - } + virNetServerCheckLimits(srv); virNetServerClientSetDispatcher(client, virNetServerDispatchNewMessage, -- 2.5.5
Commit 2737aaaf changed our policy for accepting new clients in a way, that instead of accepting new clients only to disconnect them immediately because that would overcommit the limit we temporarily disable polling for the dedicated file descriptor, so any new connection will queue on the socket. Commit 8b1f0469 then added the possibility to change the limits during runtime but it didn't re-enable polling for the previously disabled file descriptor, thus any new connection would still continue to queue on the socket. This patch forces an update of the services each time the limits were changed in some way. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1357776 Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/rpc/virnetserver.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index c5caef3..5b6bc4a 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -1071,6 +1071,8 @@ virNetServerSetClientLimits(virNetServerPtr srv, if (maxClientsUnauth >= 0) srv->nclients_unauth_max = maxClientsUnauth; + virNetServerCheckLimits(srv); + ret = 0; cleanup: virObjectUnlock(srv); -- 2.5.5
Prior to commit 2737aaaf, we allowed every client to connect successfully, however, if accepting the client would eventually lead to an overcommit of the limits, we would disconnect it immediately with "Too many active clients, dropping connection from...". Commits 4d693241 and e34fbb9e refactored the code in a way, that it is not possible for the client-related callback to be dispatched and the client to be accepted if the limits would permit to do so, therefore a check if a connection should be dropped due to limits violation has become a dead code that could be removed. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/rpc/virnetserver.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 5b6bc4a..f06643a 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -279,13 +279,6 @@ int virNetServerAddClient(virNetServerPtr srv, { virObjectLock(srv); - if (srv->nclients >= srv->nclients_max) { - virReportError(VIR_ERR_RPC, - _("Too many active clients (%zu), dropping connection from %s"), - srv->nclients_max, virNetServerClientRemoteAddrStringURI(client)); - goto error; - } - if (virNetServerClientInit(client) < 0) goto error; -- 2.5.5
On 07/20/2016 06:53 AM, Erik Skultety wrote:
Prior to commit 2737aaaf, we allowed every client to connect successfully, however, if accepting the client would eventually lead to an overcommit of the limits, we would disconnect it immediately with "Too many active clients, dropping connection from...". Commits 4d693241 and e34fbb9e refactored
Are these two commit id's from your local branch? EG, the previous two commits? I assume so, since they weren't found... Perhaps change the text to indicate "recent adjustments" or push the first four, get their upsteam commit id's and then replace these two with the "real" ones. John
the code in a way, that it is not possible for the client-related callback to be dispatched and the client to be accepted if the limits would permit to do so, therefore a check if a connection should be dropped due to limits violation has become a dead code that could be removed.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/rpc/virnetserver.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 5b6bc4a..f06643a 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -279,13 +279,6 @@ int virNetServerAddClient(virNetServerPtr srv, { virObjectLock(srv);
- if (srv->nclients >= srv->nclients_max) { - virReportError(VIR_ERR_RPC, - _("Too many active clients (%zu), dropping connection from %s"), - srv->nclients_max, virNetServerClientRemoteAddrStringURI(client)); - goto error; - } - if (virNetServerClientInit(client) < 0) goto error;
On 07/20/2016 06:53 AM, Erik Skultety wrote:
This series slightly refactors the code in virnetserver.c in order to resolve https://bugzilla.redhat.com/show_bug.cgi?id=1357776. The problem resides in updating of the client limits which even though updated, thus "permitting" new connections to be established, do not re-enable polling for the socket file descriptor which results in all new connections still queueing on the socket.
Erik Skultety (5): rpc: virnetserver: Rename ClientSetProcessingControls to ClientSetLimits rpc: virnetserver: Move virNetServerCheckLimits which is static up in the file rpc: virnetserver: Add code to CheckLimits to handle suspending of services admin: rpc: virnetserver: Fix updating of the client limits rpc: virnetserver: Remove dead code checking the client limits
daemon/admin_server.c | 4 +-- src/rpc/virnetserver.c | 97 +++++++++++++++++++++++--------------------------- src/rpc/virnetserver.h | 6 ++-- 3 files changed, 50 insertions(+), 57 deletions(-)
ACK series, but consider my note in 5/5 John
On 30/07/16 15:44, John Ferlan wrote:
On 07/20/2016 06:53 AM, Erik Skultety wrote:
This series slightly refactors the code in virnetserver.c in order to resolve https://bugzilla.redhat.com/show_bug.cgi?id=1357776. The problem resides in updating of the client limits which even though updated, thus "permitting" new connections to be established, do not re-enable polling for the socket file descriptor which results in all new connections still queueing on the socket.
Erik Skultety (5): rpc: virnetserver: Rename ClientSetProcessingControls to ClientSetLimits rpc: virnetserver: Move virNetServerCheckLimits which is static up in the file rpc: virnetserver: Add code to CheckLimits to handle suspending of services admin: rpc: virnetserver: Fix updating of the client limits rpc: virnetserver: Remove dead code checking the client limits
daemon/admin_server.c | 4 +-- src/rpc/virnetserver.c | 97 +++++++++++++++++++++++--------------------------- src/rpc/virnetserver.h | 6 ++-- 3 files changed, 50 insertions(+), 57 deletions(-)
ACK series, but consider my note in 5/5
Adjusted and pushed, thanks. Erik
participants (2)
-
Erik Skultety -
John Ferlan