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 1151b3c7b2e3799ae105db29b3b19b64e1e519a6..d19c6ac9fce8748084b863db095d763c344e72eb 100644 (file)
--- 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 <out> 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 7269ac9b8f6ecea8226595c93c20a84fbfd5284a..cfbb8a314709f9a5fa7844e2231b49dc328e566c 100644 (file)
--- 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 d65de7458c35eb0c25db07934e581109319e7709..41e06c7ffa4ed6c63dd2c0723c1572f171dba8af 100644 (file)
--- 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 daff66629b85c95553092eaa278863aeb1ae27e7..b4cfc7949e014f2e26252dd895b15cab436ff2ff 100644 (file)
--- 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 c2fcb4efe7830cb35c42eea4784e382af636ea84..de1896ed6742d736b44ea21a2a468b9f5fa1a06c 100644 (file)
--- 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 7f5c8a9fbafae0f7a324a6e08f658fc90c4f0101..7cf0cc749bbb808ea3ab133bb8bca1cc55409a04 100644 (file)
--- 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 9858249cd7d6e57b7520b51b0b7bacc26c99ef24..7ecd00311d3e780b5e32141e96364e8eb3d7b18f 100644 (file)
--- 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 b5e912e3ea05a888e8c4f5ba39902fe5616e3b87..5ad865b88d3a31d8dd52063c1de8f31038dcbf16 100644 (file)
--- 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 dd6e9e68fed9271e84578adfe3e6edab263ba5c2..efba257147be99358591c2ef710767a1aa4c8a47 100644 (file)
--- 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;
 }