From: Willy Tarreau Date: Thu, 21 Nov 2013 10:22:01 +0000 (+0100) Subject: BUG/MAJOR: server: weight calculation fails for map-based algorithms X-Git-Tag: v1.5-dev20~224 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=004e045f3141cedcfe578a102a2a0d4e68363a15;p=thirdparty%2Fhaproxy.git BUG/MAJOR: server: weight calculation fails for map-based algorithms A crash was reported by Igor at owind when changing a server's weight on the CLI. Lukas Tribus could reproduce a related bug where setting a server's weight would result in the new weight being multiplied by the initial one. The two bugs are the same. The incorrect weight calculation results in the total farm weight being larger than what was initially allocated, causing the map index to be out of bounds on some hashes. It's easy to reproduce using "balance url_param" with a variable param, or with "balance static-rr". It appears that the calculation is made at many places and is not always right and not always wrong the same way. Thus, this patch introduces a new function "server_recalc_eweight()" which is dedicated to this task of computing ->eweight from many other elements including uweight and current time (for slowstart), and all users now switch to use this function. The patch is a bit large but the code was not trivially fixable in a way that could guarantee this situation would not occur anymore. The fix is much more readable and has been verified to work with all algorithms, with both consistent and map-based hashes, and even with static-rr. Slowstart was tested as well, just like enable/disable server. The same bug is very likely present in 1.4 as well, so the patch will probably need to be backported eventhough it will not apply as-is. Thanks to Lukas and Igor for the information they provided to reproduce it. --- diff --git a/include/proto/server.h b/include/proto/server.h index 1151b3c7b2..d19c6ac9fc 100644 --- a/include/proto/server.h +++ b/include/proto/server.h @@ -58,6 +58,12 @@ struct srv_kw *srv_find_kw(const char *kw); /* Dumps all registered "server" keywords to the string pointer. */ void srv_dump_kws(char **out); +/* Recomputes the server's eweight based on its state, uweight, the current time, + * and the proxy's algorihtm. To be used after updating sv->uweight. The warmup + * state is automatically disabled if the time is elapsed. + */ +void server_recalc_eweight(struct server *sv); + /* * Parses weight_str and configures sv accordingly. * Returns NULL on success, error message string otherwise. diff --git a/src/checks.c b/src/checks.c index 7269ac9b8f..cfbb8a3147 100644 --- a/src/checks.c +++ b/src/checks.c @@ -481,18 +481,10 @@ void set_server_up(struct check *check) { if (s->slowstart > 0) { s->state |= SRV_WARMINGUP; - if (s->proxy->lbprm.algo & BE_LB_PROP_DYN) { - /* For dynamic algorithms, start at the first step of the weight, - * without multiplying by BE_WEIGHT_SCALE. - */ - s->eweight = s->uweight; - if (s->proxy->lbprm.update_server_eweight) - s->proxy->lbprm.update_server_eweight(s); - } task_schedule(s->warmup, tick_add(now_ms, MS_TO_TICKS(MAX(1000, s->slowstart / 20)))); } - if (s->proxy->lbprm.set_server_status_up) - s->proxy->lbprm.set_server_status_up(s); + + server_recalc_eweight(s); /* If the server is set with "on-marked-up shutdown-backup-sessions", * and it's not a backup server and its effective weight is > 0, @@ -1273,23 +1265,7 @@ static struct task *server_warmup(struct task *t) if ((s->state & (SRV_RUNNING|SRV_WARMINGUP|SRV_MAINTAIN)) != (SRV_RUNNING|SRV_WARMINGUP)) return t; - if (now.tv_sec < s->last_change || now.tv_sec >= s->last_change + s->slowstart) { - /* go to full throttle if the slowstart interval is reached */ - s->state &= ~SRV_WARMINGUP; - if (s->proxy->lbprm.algo & BE_LB_PROP_DYN) - s->eweight = s->uweight * BE_WEIGHT_SCALE; - if (s->proxy->lbprm.update_server_eweight) - s->proxy->lbprm.update_server_eweight(s); - } - else if (s->proxy->lbprm.algo & BE_LB_PROP_DYN) { - /* for dynamic algorithms, let's slowly update the weight */ - s->eweight = (BE_WEIGHT_SCALE * (now.tv_sec - s->last_change) + - s->slowstart - 1) / s->slowstart; - s->eweight *= s->uweight; - if (s->proxy->lbprm.update_server_eweight) - s->proxy->lbprm.update_server_eweight(s); - } - /* Note that static algorithms are already running at full throttle */ + server_recalc_eweight(s); /* probably that we can refill this server with a bit more connections */ check_for_pending(s); diff --git a/src/lb_chash.c b/src/lb_chash.c index d65de7458c..41e06c7ffa 100644 --- a/src/lb_chash.c +++ b/src/lb_chash.c @@ -382,7 +382,8 @@ void chash_init_server_tree(struct proxy *p) p->lbprm.wdiv = BE_WEIGHT_SCALE; for (srv = p->srv; srv; srv = srv->next) { - srv->prev_eweight = srv->eweight = srv->uweight * BE_WEIGHT_SCALE; + srv->eweight = (srv->uweight * p->lbprm.wdiv + p->lbprm.wmult - 1) / p->lbprm.wmult; + srv->prev_eweight = srv->eweight; srv->prev_state = srv->state; } diff --git a/src/lb_fas.c b/src/lb_fas.c index daff66629b..b4cfc7949e 100644 --- a/src/lb_fas.c +++ b/src/lb_fas.c @@ -252,7 +252,8 @@ void fas_init_server_tree(struct proxy *p) p->lbprm.wdiv = BE_WEIGHT_SCALE; for (srv = p->srv; srv; srv = srv->next) { - srv->prev_eweight = srv->eweight = srv->uweight * BE_WEIGHT_SCALE; + srv->eweight = (srv->uweight * p->lbprm.wdiv + p->lbprm.wmult - 1) / p->lbprm.wmult; + srv->prev_eweight = srv->eweight; srv->prev_state = srv->state; } diff --git a/src/lb_fwlc.c b/src/lb_fwlc.c index c2fcb4efe7..de1896ed67 100644 --- a/src/lb_fwlc.c +++ b/src/lb_fwlc.c @@ -244,7 +244,8 @@ void fwlc_init_server_tree(struct proxy *p) p->lbprm.wdiv = BE_WEIGHT_SCALE; for (srv = p->srv; srv; srv = srv->next) { - srv->prev_eweight = srv->eweight = srv->uweight * BE_WEIGHT_SCALE; + srv->eweight = (srv->uweight * p->lbprm.wdiv + p->lbprm.wmult - 1) / p->lbprm.wmult; + srv->prev_eweight = srv->eweight; srv->prev_state = srv->state; } diff --git a/src/lb_fwrr.c b/src/lb_fwrr.c index 7f5c8a9fba..7cf0cc749b 100644 --- a/src/lb_fwrr.c +++ b/src/lb_fwrr.c @@ -272,7 +272,8 @@ void fwrr_init_server_groups(struct proxy *p) p->lbprm.wdiv = BE_WEIGHT_SCALE; for (srv = p->srv; srv; srv = srv->next) { - srv->prev_eweight = srv->eweight = srv->uweight * BE_WEIGHT_SCALE; + srv->eweight = (srv->uweight * p->lbprm.wdiv + p->lbprm.wmult - 1) / p->lbprm.wmult; + srv->prev_eweight = srv->eweight; srv->prev_state = srv->state; } diff --git a/src/lb_map.c b/src/lb_map.c index 9858249cd7..7ecd00311d 100644 --- a/src/lb_map.c +++ b/src/lb_map.c @@ -180,7 +180,7 @@ void init_server_map(struct proxy *p) act = bck = 0; for (srv = p->srv; srv; srv = srv->next) { - srv->eweight = srv->uweight / pgcd; + srv->eweight = (srv->uweight * p->lbprm.wdiv + p->lbprm.wmult - 1) / p->lbprm.wmult; srv->prev_eweight = srv->eweight; srv->prev_state = srv->state; if (srv->state & SRV_BACKUP) diff --git a/src/proto_http.c b/src/proto_http.c index b5e912e3ea..5ad865b88d 100644 --- a/src/proto_http.c +++ b/src/proto_http.c @@ -2932,28 +2932,8 @@ int http_process_req_stat_post(struct stream_interface *si, struct http_txn *txn else sv->uweight = 0; - if (px->lbprm.algo & BE_LB_PROP_DYN) { - /* we must take care of not pushing the server to full throttle during slow starts */ - if ((sv->state & SRV_WARMINGUP) && (px->lbprm.algo & BE_LB_PROP_DYN)) - sv->eweight = (BE_WEIGHT_SCALE * (now.tv_sec - sv->last_change) + sv->slowstart - 1) / sv->slowstart; - else - sv->eweight = BE_WEIGHT_SCALE; - sv->eweight *= sv->uweight; - } else { - sv->eweight = sv->uweight; - } + server_recalc_eweight(sv); - /* static LB algorithms are a bit harder to update */ - if (px->lbprm.update_server_eweight) - px->lbprm.update_server_eweight(sv); - else if (sv->eweight) { - if (px->lbprm.set_server_status_up) - px->lbprm.set_server_status_up(sv); - } - else { - if (px->lbprm.set_server_status_down) - px->lbprm.set_server_status_down(sv); - } altered_servers++; total_servers++; break; diff --git a/src/server.c b/src/server.c index dd6e9e68fe..efba257147 100644 --- a/src/server.c +++ b/src/server.c @@ -158,6 +158,43 @@ static void __listener_init(void) srv_register_keywords(&srv_kws); } +/* Recomputes the server's eweight based on its state, uweight, the current time, + * and the proxy's algorihtm. To be used after updating sv->uweight. The warmup + * state is automatically disabled if the time is elapsed. + */ +void server_recalc_eweight(struct server *sv) +{ + struct proxy *px = sv->proxy; + unsigned w; + + if (now.tv_sec < sv->last_change || now.tv_sec >= sv->last_change + sv->slowstart) { + /* go to full throttle if the slowstart interval is reached */ + sv->state &= ~SRV_WARMINGUP; + } + + /* We must take care of not pushing the server to full throttle during slow starts. + * It must also start immediately, at least at the minimal step when leaving maintenance. + */ + if ((sv->state & SRV_WARMINGUP) && (px->lbprm.algo & BE_LB_PROP_DYN)) + w = (px->lbprm.wdiv * (now.tv_sec - sv->last_change) + sv->slowstart) / sv->slowstart; + else + w = px->lbprm.wdiv; + + sv->eweight = (sv->uweight * w + px->lbprm.wmult - 1) / px->lbprm.wmult; + + /* now propagate the status change to any LB algorithms */ + if (px->lbprm.update_server_eweight) + px->lbprm.update_server_eweight(sv); + else if (sv->eweight) { + if (px->lbprm.set_server_status_up) + px->lbprm.set_server_status_up(sv); + } + else { + if (px->lbprm.set_server_status_down) + px->lbprm.set_server_status_down(sv); + } +} + /* * Parses weight_str and configures sv accordingly. * Returns NULL on success, error message string otherwise. @@ -199,29 +236,7 @@ const char *server_parse_weight_change_request(struct server *sv, return "Backend is using a static LB algorithm and only accepts weights '0%' and '100%'.\n"; sv->uweight = w; - - if (px->lbprm.algo & BE_LB_PROP_DYN) { - /* we must take care of not pushing the server to full throttle during slow starts */ - if ((sv->state & SRV_WARMINGUP)) - sv->eweight = (BE_WEIGHT_SCALE * (now.tv_sec - sv->last_change) + sv->slowstart - 1) / sv->slowstart; - else - sv->eweight = BE_WEIGHT_SCALE; - sv->eweight *= sv->uweight; - } else { - sv->eweight = sv->uweight; - } - - /* static LB algorithms are a bit harder to update */ - if (px->lbprm.update_server_eweight) - px->lbprm.update_server_eweight(sv); - else if (sv->eweight) { - if (px->lbprm.set_server_status_up) - px->lbprm.set_server_status_up(sv); - } - else { - if (px->lbprm.set_server_status_down) - px->lbprm.set_server_status_down(sv); - } + server_recalc_eweight(sv); return NULL; }