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

On Sun, 10 Feb 2008, Willy Tarreau wrote:

> Hi Krzysztof,

Hi,

> I finally found some time to get back to userland :-)
> I've carefully inspected your patch. Initially I thought that the FWRR
> change was wrong but it's OK, it's my understanding which was wrong !
>
> However, I'm not sure that I completely get the logic behind the changes
> in assign_server_and_queue() :
>
>> @@ -1060,9 +1076,8 @@ int assign_server_and_queue(struct session *s)
>>
>> if (s->flags & SN_ASSIGNED) {
>> if (s->srv && s->srv->maxqueue > 0 && s->srv->nbpend >= s->srv->maxqueue) {
>> - s->flags &= ~(SN_DIRECT | SN_ASSIGNED | SN_ADDR_SET);
>> - s->srv = NULL;
>> - http_flush_cookie_flags(&s->txn);
>> + /* it's left to the dispatcher to choose a server */
>> + s->flags &= ~(SN_ASSIGNED | SN_ADDR_SET);
>> } else {
>> /* a server does not need to be assigned, perhaps because we're in
>> * direct mode, or in dispatch or transparent modes where the server
>> @@ -1081,7 +1096,22 @@ int assign_server_and_queue(struct session *s)
>> }
>>
>> /* a server needs to be assigned */
>> + srv = s->srv;
>> err = assign_server(s);
>> +
>> + if (srv && srv != s->srv) {
>> + /* This session was previously dispatched to another server:
>> + * - clear SN_DIRECT
>> + * - flush a cookie
>> + * - set SN_REDISP if it was successfully redispatched
>> + */
>> +
>> + s->flags &= ~(SN_DIRECT);
>> + http_flush_cookie_flags(&s->txn);
>> + if (srv)
>> + s->flags |= SN_REDISP;
>> + }
>> +
>
> I think that the redispatch will not work anymore if we hit srv->maxqueue
> for a server : since the SN_DIRECT flag is not cleared anymore, assign_server()
> will return exactly the same server. Am I right or am I missing something ?

Right. This is wrong. Initially I fought it is required for a proper logs (we don't want for example to log that a new cookie was assigned when it wasn't) but after your comments I rechecked the code and it seems I should have leave SN_REDISP untouched.

> Then, maybe we have the same problem here, and at every other place where
> the redispatch code gets touched :
>
>> @@ -1409,10 +1439,8 @@ int srv_retryable_connect(struct session *t)
>> }
>> t->be->redispatches++;
>>
>> - t->flags &= ~(SN_DIRECT | SN_ASSIGNED | SN_ADDR_SET);
>> - t->flags |= SN_REDISP;
>> - t->srv = NULL; /* it's left to the dispatcher to choose a server */
>> - http_flush_cookie_flags(&t->txn);
>> + /* it's left to the dispatcher to choose a server */
>> + t->flags &= ~(SN_ASSIGNED | SN_ADDR_SET);
>> return 0;
>> }
>
> etc...
>
> Please try to explain me if I'm wrong.

No, I was wrong so there is nothing to explain. ;) I'll revert those stupid changes. Thank you.

Best regards,

                                 Krzysztof Oledzki Received on 2008/02/10 14:40