Re: [PATCH] : Prevent redispatcher from selecting the same server

On Wed, 26 Dec 2007, Willy Tarreau wrote:

> On Wed, Dec 26, 2007 at 02:45:17AM +0100, Krzysztof Oledzki wrote:
>> [RFC]: Prevent redispatcher from selecting the same server
>>
>> When haproxy decides that session needs to be redispatched it chose a server,
>> but there is no guarantee for it to be a different one. So, it often
>> happens that selected server is exactly the same that it was previously, so
>> a client ends up with a 503 error anyway, especially when one sever has
>> much bigger weight than others.
>>
>> This RFC quality patch fix RR and PH algos, so when a redispatching
>> occures assign_server() never selects previously assigned server again.
>>
>> TODO: SH/UH, test PH, more tests
>
> IMHO, this should only be done for non-deterministic algorithms such as
> roundrobin, and later leastconn. But people who use hashes really expect
> that the same client or request will be sent to the same server (or to an
> equivalent backup server when that may happen).

OK, what about PH, as there is a get_server_rr_with_conns fallback?

> And in fact, in the PH code, if you find the same server, you return NULL,
> which will immediately cause a 503. In this case, we'd probably prefer to
> still return the same server and perform a last connection attempt to it
> in the hope that it finally accepts the connection.

Not exactly as if s->srv ==NULL then s->srv = get_server_rr_with_conns(s->be, s->srv):

case BE_LB_ALGO_PH:

         /* URL Parameter hashing */
         s->srv = get_server_ph(s->be,
                                s->txn.req.sol + s->txn.req.sl.rq.u,
                                s->txn.req.sl.rq.u_l, s->srv);
         if (!s->srv) {
                 /* parameter not found, fall back to round robin on the map */
                 s->srv = get_server_rr_with_conns(s->be, s->srv);
                 if (!s->srv)
                         return SRV_STATUS_FULL;
         }
         break;

It should work, or maybe I overlooked something?

> Basically, this would mean that non-deterministic algorithms should try to
> evict the faulty server from the algorithm before choosing one (which is
> what you do in case of RR), but others should just ignore it. If the server
> is already down, another one will be reported. And if it's still up, it is
> said and documented that no other server should get the request.
OK.

> But generally, the idea seems good. I do also find it frustrating that when
> you only have two servers, you have 50% chances that a redispatch will get
> back to the faulty server. And it's in fact even harder to explain to people,
> so it seems that it doesn't appear logical :-)

Yep, exactly. However, I'm still not sure if this idea with keeping old value in sess->srv instead of putting in it sess->oldsrv is correct. It looks as a quite clean and tempting solution but I'm afraid that it may break something.

> This reminds me about a feature I've wanted to add for a long time now: two
> different check intervals. The normal one (current "inter") and a fast one,
> used during transitions (eg: "fastinter"), used to speed up state transition
> when a failed check has been detected. The idea is that once this is in place,
> it should be relatively simple to switch a server to fastinter as soon as it
> experiences a redispatch. This will make it possible to detect server failures
> very quickly even with slow checks.

Yes, and as I remember we had already discussed about it and agreed that it is important. Please also note that it often happens that a server is not defected but only saturated for ~1s so if we also add a retrydelay parameter than instead of 4 tcp resets in a row we may get a tcp ack, if we wait ~1s after each refused connection.

> For instance, if we consider that a server is checked every 10 seconds and
> has 3 retries ("fall 3"), 30 seconds will be necessary to detect a failure.
> Now if we use "inter 10s fastinter 500ms", we will need 11 seconds to detect
> a failure, or 1.5 second after the first redispatch occurs. In this example,
> this is 20 times faster than what we can currently achieve.

I have a half-ready patch for it. In my solution it is quite simple and looks similar to this:

if (((sv->health < s->rise + s->fall - 1) || (sv->health)) && s->inter > SRV_CHK_INTER_THRES) than

         cr = s->inter * global.tcr;
else

         cr = s->inter;

This tcr is for "transition check rate" so you have a global 1-100% paremeter for it, exactly like spread_checks.

Best regards,

                                 Krzysztof Olêdzki Received on 2007/12/26 14:30