Re: [PATCH]: Spread checks

On Sun, Sep 23, 2007 at 10:44:15PM +0200, Krzysztof Oledzki wrote:
>
>
> On Tue, 18 Sep 2007, Willy Tarreau wrote:
>
> >On Tue, Sep 18, 2007 at 11:35:43AM +0200, Krzysztof Oledzki wrote:
> >>I noticed that each server receive all checks in a very short (<1ms) time
> >>(attached checklog2 file). I think that having 10s for 48 (16*3) tests it
> >>is possible to reduce both servers' and loadbalancer stress a little, by
> >>running each test every 10s/48 = ~ 0.2s.
> >
> >Yes, and this was even worse in the past because all servers for a same
> >group were checked at the same instant. There were people doing
> >load-balancing
> >on same machines with multiple ports who got regular load peaks during the
> >checks. So I have spread them apart within one backend.
> >
> >However, the problem still remains if you share the same server between
> >many instances. I'm not sure how I could improve this. Maybe add a
> >per-backend
> >start delay for the checks, which would be equal to min_inter/#backends.
> >As an
> >alternative right now, you can rotate your servers within different
> >backends.
> >
> >I think I could also add a global "spread-check" parameter allowing us to
> >add
> >some random time between all checks in order to spread them apart. It would
> >take a percentage parameter adding or removing that many percent to the
> >interval
> >after each check.
>
> Attached patch implements per-server start delay in a different way.
> Checks are now spread globally - not locally to one backend. It also makes
> them started faster - IMHO there is no need to add a 'server->inter' when
> calculating first execution.

the reason for the server->inter is that there are people using 10 servers entries which all point to the same machine on 10 different ports. The "inter" gives a hint about how often we expect the checks to be sent.

> Calculation were moved from cfgparse.c to
> checks.c. There is a new function start_checks() and now it is not called
> when haproxy is started in MODE_CHECK.
>
> With this patch it is also possible to set a global 'spread-check'
> parameter. It takes a percentage value (1..50, probably something near
> 5..10 is a good idea) so haproxy adds or removes that many percent to the
> oryginal interval after each check. My test shows that with 18 backends,
> 54 servers total and 10000ms/5% it takes about 45m to mix them completely.

I think that we should be *very* careful when subtracting random percentage. It is very easy to go down to zero that way, and have a server bombed by health-checks.

BTW, I'm suddenly thinking about something: when I build the servers map, I use all the server weights and arrange them so that all their occurrences are as far as possible from each other, while respecting their exact weight. It it works very well. I'm realizing that we need exactly the same principle for the checks. The "weight" here is simply the frequency at which the servers must be checked, which is 1/interval. So by using the exact same functions as is used to build the servers map, we could build a health-check map that way :

    foreach srv in all_servers :

        weight(srv) = max(all inter) / inter(srv)

    build_health_map(all_servers)

Afterwards, we would just have to cycle through this map to start the checks. It would even remove the need for one timer per server in the timers table. The immediate advantage is that they would all be spread apart upon startup, and we would not need the random anymore (eventhough it's would not harm to conserve the option).

What do you think about this ?

> I decided to use rand/srand pseudo-random number generator. I am aware it
> is not recommend for a good randomness but a) we do not need a good random
> generator here b) it is probably the most portable one.

Yes, I agree with you. It's not worth using random() as it's not secure code here.

> Best regards,
>
> Krzysztof Ol?dzki

Thanks,
Willy

> diff -Nur haproxy-1.3.12.2-orig/include/proto/checks.h haproxy-1.3.12.2-dev/include/proto/checks.h
> --- haproxy-1.3.12.2-orig/include/proto/checks.h 2007-09-20 08:47:17.000000000 +0200
> +++ haproxy-1.3.12.2-dev/include/proto/checks.h 2007-09-23 20:23:34.000000000 +0200
> @@ -26,6 +26,7 @@
> #include <common/config.h>
>
> void process_chk(struct task *t, struct timeval *next);
> +int start_checks();
>
> #endif /* _PROTO_CHECKS_H */
>
> diff -Nur haproxy-1.3.12.2-orig/include/types/global.h haproxy-1.3.12.2-dev/include/types/global.h
> --- haproxy-1.3.12.2-orig/include/types/global.h 2007-09-20 08:47:17.000000000 +0200
> +++ haproxy-1.3.12.2-dev/include/types/global.h 2007-09-23 21:40:01.000000000 +0200
> @@ -55,6 +55,7 @@
> int rlimit_memmax; /* default ulimit-d in megs value : 0=unset */
> int mode;
> int last_checks;
> + int spread_check;
> char *chroot;
> char *pidfile;
> int logfac1, logfac2;
> diff -Nur haproxy-1.3.12.2-orig/src/cfgparse.c haproxy-1.3.12.2-dev/src/cfgparse.c
> --- haproxy-1.3.12.2-orig/src/cfgparse.c 2007-09-20 08:47:17.000000000 +0200
> +++ haproxy-1.3.12.2-dev/src/cfgparse.c 2007-09-23 21:59:22.000000000 +0200
> @@ -450,7 +450,21 @@
> Alert("parsing [%s:%d] : too many syslog servers\n", file, linenum);
> return -1;
> }
> -
> + }
> + else if (!strcmp(args[0], "spread-check")) { /* random time between checks (1-50) */
> + if (global.spread_check != 0) {
> + Alert("parsing [%s:%d]: spread-check already specified. Continuing.\n", file, linenum);
> + return 0;
> + }
> + if (*(args[1]) == 0) {
> + Alert("parsing [%s:%d]: '%s' expects an integer argument.\n", file, linenum, args[0]);
> + return -1;
> + }
> + global.spread_check = atol(args[1]);
> + if (global.spread_check < 1 || global.spread_check > 50) {
> + Alert("parsing [%s:%d]: 'spread-check' needs a positive value in range 1..50.\n", file, linenum);
> + return -1;
> + }
> }
> else {
> Alert("parsing [%s:%d] : unknown keyword '%s' in '%s' section\n", file, linenum, args[0], "global");
> @@ -2261,7 +2275,6 @@
> char *args[MAX_LINE_ARGS + 1];
> int arg;
> int cfgerr = 0;
> - int nbchk, mininter;
> int confsect = CFG_NONE;
>
> struct proxy *curproxy = NULL;
> @@ -2708,56 +2721,6 @@
> newsrv = newsrv->next;
> }
>
> - /* now we'll start this proxy's health checks if any */
> - /* 1- count the checkers to run simultaneously */
> - nbchk = 0;
> - mininter = 0;
> - newsrv = curproxy->srv;
> - while (newsrv != NULL) {
> - if (newsrv->state & SRV_CHECKED) {
> - if (!mininter || mininter > newsrv->inter)
> - mininter = newsrv->inter;
> - nbchk++;
> - }
> - newsrv = newsrv->next;
> - }
> -
> - /* 2- start them as far as possible from each others while respecting
> - * their own intervals. For this, we will start them after their own
> - * interval added to the min interval divided by the number of servers,
> - * weighted by the server's position in the list.
> - */
> - if (nbchk > 0) {
> - struct task *t;
> - int srvpos;
> -
> - newsrv = curproxy->srv;
> - srvpos = 0;
> - while (newsrv != NULL) {
> - /* should this server be checked ? */
> - if (newsrv->state & SRV_CHECKED) {
> - if ((t = pool_alloc2(pool2_task)) == NULL) {
> - Alert("parsing [%s:%d] : out of memory.\n", file, linenum);
> - return -1;
> - }
> -
> - t->wq = NULL;
> - t->qlist.p = NULL;
> - t->state = TASK_IDLE;
> - t->process = process_chk;
> - t->context = newsrv;
> -
> - /* check this every ms */
> - tv_ms_add(&t->expire, &now,
> - newsrv->inter + mininter * srvpos / nbchk);
> - task_queue(t);
> - //task_wakeup(&rq, t);
> - srvpos++;
> - }
> - newsrv = newsrv->next;
> - }
> - }
> -
> curproxy = curproxy->next;
> }
> if (cfgerr > 0) {
> diff -Nur haproxy-1.3.12.2-orig/src/checks.c haproxy-1.3.12.2-dev/src/checks.c
> --- haproxy-1.3.12.2-orig/src/checks.c 2007-09-20 08:47:17.000000000 +0200
> +++ haproxy-1.3.12.2-dev/src/checks.c 2007-09-23 21:52:20.000000000 +0200
> @@ -13,7 +13,9 @@
> #include <errno.h>
> #include <fcntl.h>
> #include <stdio.h>
> +#include <stdlib.h>
> #include <string.h>
> +#include <time.h>
> #include <unistd.h>
> #include <sys/socket.h>
> #include <netinet/in.h>
> @@ -281,6 +283,7 @@
> __label__ new_chk, out;
> struct server *s = t->context;
> struct sockaddr_in sa;
> + int rv=0;
> int fd;
>
> //fprintf(stderr, "process_chk: task=%p\n", t);
> @@ -504,8 +507,13 @@
> set_server_down(s);
> s->curfd = -1;
> fd_delete(fd);
> + if (global.spread_check) {
> + rv = s->inter*global.spread_check/100;
> + rv -= (int) (2*rv*(rand()/(RAND_MAX+1.0)));
> + //fprintf(stderr, "process_chk: (%d+/-%d%%) random=%d\n", s->inter, global.spread_check, rv);
> + }
> while (tv_isle(&t->expire, &now))
> - tv_ms_add(&t->expire, &t->expire, s->inter);
> + tv_ms_add(&t->expire, &t->expire, s->inter+rv);
> goto new_chk;
> }
> /* if result is 0 and there's no timeout, we have to wait again */
> @@ -518,6 +526,64 @@
> return;
> }
>
> +/*
> + * Start health-check.
> + * Returns 0 if OK, -1 if error.
> + */
> +int start_checks() {
> +
> + struct proxy *px;
> + struct server *s;
> + struct task *t;
> + int nbchk=0, mininter=0, srvpos=0;
> +
> + /* 1- count the checkers to run simultaneously */
> + for(px=proxy; px; px=px->next)
> + for(s=px->srv; s; s=s->next) {
> + if (!(s->state & SRV_CHECKED))
> + continue;
> +
> + if (!mininter || mininter > s->inter)
> + mininter = s->inter;
> +
> + nbchk++;
> + }
> +
> + if (!nbchk)
> + return 0;
> +
> + srand((unsigned)time(NULL));
> +
> + /*
> + * 2- start them as far as possible from each others. For this, we will
> + * start them after their interval set to the min interval divided by
> + * the number of servers, weighted by the server's position in the list.
> + */
> + for(px=proxy; px; px=px->next)
> + for(s=px->srv; s; s=s->next) {
> + if (!(s->state & SRV_CHECKED))
> + continue;
> +
> + if ((t = pool_alloc2(pool2_task)) == NULL) {
> + Alert("Starting [%s:%s] check: out of memory.\n", px->id, s->id);
> + return -1;
> + }
> +
> + t->wq = NULL;
> + t->qlist.p = NULL;
> + t->state = TASK_IDLE;
> + t->process = process_chk;
> + t->context = s;
> +
> + /* check this every ms */
> + tv_ms_add(&t->expire, &now, mininter * srvpos / nbchk);
> + task_queue(t);
> +
> + srvpos++;
> + }
> +
> + return 0;
> +}
>
> /*
> * Local variables:
> diff -Nur haproxy-1.3.12.2-orig/src/haproxy.c haproxy-1.3.12.2-dev/src/haproxy.c
> --- haproxy-1.3.12.2-orig/src/haproxy.c 2007-09-20 08:47:17.000000000 +0200
> +++ haproxy-1.3.12.2-dev/src/haproxy.c 2007-09-23 21:53:36.000000000 +0200
> @@ -81,6 +81,7 @@
> #include <proto/acl.h>
> #include <proto/backend.h>
> #include <proto/buffers.h>
> +#include <proto/checks.h>
> #include <proto/client.h>
> #include <proto/fd.h>
> #include <proto/log.h>
> @@ -505,6 +506,7 @@
> Alert("Error reading configuration file : %s\n", cfg_cfgfile);
> exit(1);
> }
> +
> if (have_appsession)
> appsession_init();
>
> @@ -513,6 +515,9 @@
> exit(0);
> }
>
> + if (start_checks())
> + exit(1);
> +
> if (cfg_maxconn > 0)
> global.maxconn = cfg_maxconn;
>
Received on 2007/09/29 12:41