Re: [PATCH] : rework checks handling

Hi Krzysztof,

On Fri, Jan 11, 2008 at 01:16:17AM +0100, Krzysztof Oledzki wrote:
> >From eac023632cc39cc646feefd48a8bea82129cbfbc Mon Sep 17 00:00:00 2001
> From: Krzysztof Piotr Oledzki <ole#ans.pl>
> Date: Fri, 11 Jan 2008 01:11:22 +0100
> Subject: [MEDIUM]: rework checks handling
>
> This RFC-quality patch adds two new variables: fastinter and downinter.
> When server state is:
> - non-transitionally UP -> inter (no change)
> - transitionally UP (going down), unchecked or transitionally DOWN (going up) -> fastinter
> - down -> downinter
>
> It allows to set something like:
> server sr6 127.0.51.61:80 cookie s6 check inter 10000 downinter 20000 fastinter 500 fall 3 weight 40
> In the above example haproxy uses 10000ms between checks but as soon as
> one check fails fastinter (500ms) is used. If server is down
> downinter (20000) is used or fastinter (500ms) if one check pass.
> Fastinter is also used when haproxy starts.

Fine!

> BTW: I think we need some improvements to parser as adding additional keywords
> is getting to much complicated. I'll try to prepare a rfc-patch soon.

OK, I agree with you on this.

> New "timeout.check" variable was added, if set haproxy uses it as an additional
> read timeout, but only after a connection was already established. I was
> thinking about using "timeout.server" here but as most people set this
> with an addition reserve but still want checks to kick out laggy servers.
> Please also note that in most cases check request is much simpler
> and faster to handle than normal requests so this timeout should be smaller.

That's a good idea. I must say that for a long time, I never felt very comfortable with timeouts set to "inter" on slow checks.

> I also changed the timeout used for connection establishing from "inter"
> to "timeout.connect", as in most cases "inter" is _much_ to long for this purpose.

That makes much sense too.

> After all that changes I was brave enough to add my copyright info into
> the src/server.c as I'm the author of all two functions located in this file
> Hope it is OK.

Yes of course, you're welcome (and encouraged) to do so!

> I'm going to add some documentation about fastinter/downinter and timeout.check
> if this patch is acceptable and of course in the final version I'll comment all
> introduced debug fpritfs. ;)

OK, overall it looks fine. Here's a quick review.

> if (ret == s->proxy->check_len) {
> + /* we allow up to <timeout.check> if nonzero or <timeout.server> for a responce */
> + fprintf(stderr, "event_srv_chk_w, ms=%lu\n",
> + __tv_to_ms(&s->proxy->timeout.check));
> + if (__tv_to_ms(&s->proxy->timeout.check))
> + tv_add(&t->expire, &now, &s->proxy->timeout.check);

Here, you should use tv_isset() to check if the timeout is set. And there's a special function to check then add a timeout : tv_add_ifset(). You would then replace all the if() that way :

      tv_add_ifset(&t->expire, &now, &s->proxy->timeout.check);

> - /* FIXME: we allow up to <inter> for a connection to establish, but we should use another parameter */
> - tv_ms_add(&t->expire, &now, s->inter);
> + fprintf(stderr, "process_chk: 4+, %lu\n", __tv_to_ms(&s->proxy->timeout.connect));
> + /* we allow up to <timeout.connect> for a connection to establish */
> + tv_add(&t->expire, &now, &s->proxy->timeout.connect);

Here, you should check if timeout.connect or inter should be used. It is important IMHO to ensure that we never wait longer than the interval, so that people running with *very* long contimeouts (eg those who needed this due to the queue or tarpit) do not slow down their checks.

Take a look in process_session() for tv_bound(). It performs a min() on struct timeval, and may be used for this. Eg:

     struct timeval tv_con;

     tv_ms_add(&t->expire, &now, get_inter(s));
     tv_add(&tv_con, &now, &s->proxy->timeout.connect);
     tv_bound(&t->expire, &tv_con); /* if tv_con is earlier, it will be assigned to t->expire */
 



> -		//fprintf(stderr, "process_chk: 7\n");

> -		/* FIXME: we allow up to <inter> for a connection to establish, but we should use another parameter */

> +		fprintf(stderr, "process_chk: 7, %lu\n", __tv_to_ms(&s->proxy->timeout.connect));

> +		/* we allow up to <timeout.connect> for a connection to establish */

>  		while (tv_isle(&t->expire, &now))

> -			tv_ms_add(&t->expire, &t->expire, s->inter);

> +			tv_add(&t->expire, &t->expire, &s->proxy->timeout.connect);

Same here, bound the connect time to the interval.

Feel free to post with those small changes, I'll happily merge it!

Cheers,
Willy Received on 2008/01/11 07:04