Re: [PATCH] Don't increment server connections too much + fix retries

On Thu, 28 Feb 2008, Willy Tarreau wrote:

> Hi Krzysztof,
>
> [part 1]
>
> On Mon, Feb 04, 2008 at 02:17:01AM +0100, Krzysztof Oledzki wrote:
>>> From 9af493edbe56489524f57c22d58aca0584931c8a Mon Sep 17 00:00:00 2001
>> From: Krzysztof Piotr Oledzki <ole#ans.pl>
>> Date: Mon, 4 Feb 2008 02:10:09 +0100
>> Subject: [BUG] Don't increment server connections too much + fix retries
>>
>> Commit 98937b875798e10fac671d109355cde29d2a411a while fixing
>> one bug introduced another one. With "retries 4" and
>> "option redispatch" haproxy tries to connect 4 times to
>> one server server and 1 time to a second one. However
>> logs showed 5 connections to the first server (the
>> last one was counted twice) and 2 to the second.
>>
>> This patch also fixes srv->retries and be->retries increments.
>>
>> Now I get: 3 retries and 1 error in a first server (4 cum_sess)
>> and 1 error in a second server (1 cum_sess) with:
>> retries 4
>> option redispatch
>>
>> and: 4 retries and 1 error (5 cum_sess) with:
>> retries 4
>>
>> So, the number of connections effectively served by a server is:
>> srv->cum_sess - srv->failed_conns - srv->retries
>
> [part 2]
>
>> Finally!
>>
>> However, IMHO the fix from 98937b875798e10fac671d109355cde29d2a411a
>> is wrong. Now be->cum_sess < SUM(servers->cum_sess) as with 1 sesssion
>> processed by a backed I get 5 "sessions" processed by servers. This
>> is very... abnormal. IMHO we should only increment srv->cum_sess
>> once and only when a connection was successfully established but we
>> need an additional variable: srv->conns.
>>
>> So, if it is OK I will send an additional patch that renames
>> "srv->cum_sess" into "srv->conns" and reintroduces "srv->cum_sess".
>>
>> The final situation is going to look like this:
>> be->cum_sess: connections (sessions) sent from a frontend to a backend
>> be->failed_conns: unhandled connections (sessions) by a backend
>> be->cum_sess - be->failed_conns: properly handled connections (sessions) by a backend
>>
>> srv->cum_sess: connections (sessions) successfully sent to a server
>> srv->conns: connections sent to a server (both successfully and unsuccessfully)
>> srv->conns - srv->failed_conns - srv->retries: connections effectively served by a server
>>
>> Please note that:
>> * SUM(servers->failed_conns) >= be->failed_conns as one session may
>> be sent to two servers and both can be down
>>
>> * SUM(servers->cum_sess) <= be->cum_sess as it is possible for a session
>> to be unhandled when it was not possible to connect to a server
>>
>> Comments?
>
> At first I thought that the attached patch would perform both [part1]
> and [part2] above. Maybe it was late when I read it, but now that I
> re-read it, it looks to me like the patch just addresses [part1], which
> is fine for me to apply. I didn't want to merge a change for #2 along
> with a fix for #1 at the same time.
>
> If you can confirm my thoughts, I'm happy to merge it.

Exactly. Part #2 is only a suggestion supposed to start a discussion about this subject.

Best regards,

                                 Krzysztof Olêdzki Received on 2008/02/29 12:25