BUG/MEDIUM: lb-chash: always properly initialize lb_nodes with dynamic servers

An issue was introduced in 3.0 with commit faa8c3e024 ("MEDIUM: lb-chash:
Deterministic node hashes based on server address"): the new server_key
field and lb_nodes entries initialization were not updated for servers
added at run time with "add server": server_key remains zero and the key
used in lb_node remains the one depending only on the server's ID.

This will cause trouble when adding new servers with consistent hashing,
because the hash-key will be ignored until the server's weight changes
and the key difference is detected, leading to its recalculation.

This is essentially caused by the poorly placed lb_nodes initialization
that is specific to lb-chash and had to be replicated in the code dealing
with server addition.

This commit solves the problem by adding a new ->server_init() function
in the lbprm proxy struct, that is called by the server addition code.
This also allows to abandon the complex check for LB algos that was
placed there for that purpose. For now only lb-chash provides such a
function, and calls it as well during initial setup. This way newly
added servers always use the correct key now.

While it should also theoretically have had an impact on servers added
with the "random" algorithm, it's unlikely that the difference between
proper server keys and those based on their ID could have had any visible
effect.

This patch should be backported as far as 3.0. The backport may be eased
by a preliminary backport of previous commit "CLEANUP: lb-chash: free
lb_nodes from chash's deinit(), not global", though this is not strictly
necessary if context is manually adjusted.

diff --git a/include/haproxy/backend-t.h b/include/haproxy/backend-t.h
index 35f882f28add9df3cd289cd93066d651e3e8e42c..da3c10a5a472ca5ac31268b6f6d882410452dce7 100644 (file)
--- a/include/haproxy/backend-t.h
+++ b/include/haproxy/backend-t.h
@@ -192,6 +192,7 @@ struct lbprm {
        void (*server_requeue)(struct server *);         /* function used to place the server where it must be */
        void (*proxy_deinit)(struct proxy *);            /* to be called when we're destroying the proxy */
        void (*server_deinit)(struct server *);          /* to be called when we're destroying the server */
+       int (*server_init)(struct server *);             /* initialize a freshly added server (runtime); <0=fail. */
 };
 
 #endif /* _HAPROXY_BACKEND_T_H */

diff --git a/src/lb_chash.c b/src/lb_chash.c
index 9e902545864f4cf5576c71e40c18ba8a52e79b44..a852ae07dcf2c2fc7a655f788c88d63dd1b7e8c9 100644 (file)
--- a/src/lb_chash.c
+++ b/src/lb_chash.c
@@ -552,6 +552,26 @@ struct server *chash_get_next_server(struct proxy *p, struct server *srvtoavoid)
        return srv;
 }
 
+/* Allocates and initializes lb nodes for server <srv>. Returns < 0 on error.
+ * This is called by chash_init_server_tree() as well as from srv_alloc_lb()
+ * for runtime addition.
+ */
+int chash_server_init(struct server *srv)
+{
+       int node;
+
+       srv->lb_nodes = calloc(srv->lb_nodes_tot, sizeof(*srv->lb_nodes));
+       if (!srv->lb_nodes)
+               return -1;
+
+       srv->lb_server_key = chash_compute_server_key(srv);
+       for (node = 0; node < srv->lb_nodes_tot; node++) {
+               srv->lb_nodes[node].server = srv;
+               srv->lb_nodes[node].node.key = chash_compute_node_key(srv, node);
+       }
+       return 0;
+}
+
 /* Releases the allocated lb_nodes for this server */
 void chash_server_deinit(struct server *srv)
 {
@@ -568,11 +588,11 @@ int chash_init_server_tree(struct proxy *p)
 {
        struct server *srv;
        struct eb_root init_head = EB_ROOT;
-       int node;
 
        p->lbprm.set_server_status_up   = chash_set_server_status_up;
        p->lbprm.set_server_status_down = chash_set_server_status_down;
        p->lbprm.update_server_eweight  = chash_update_server_weight;
+       p->lbprm.server_init            = chash_server_init;
        p->lbprm.server_deinit          = chash_server_deinit;
        p->lbprm.server_take_conn = NULL;
        p->lbprm.server_drop_conn = NULL;
@@ -595,17 +615,11 @@ int chash_init_server_tree(struct proxy *p)
                srv->lb_tree = (srv->flags & SRV_F_BACKUP) ? &p->lbprm.chash.bck : &p->lbprm.chash.act;
                srv->lb_nodes_tot = srv->uweight * BE_WEIGHT_SCALE;
                srv->lb_nodes_now = 0;
-               srv->lb_nodes = calloc(srv->lb_nodes_tot,
-                                      sizeof(*srv->lb_nodes));
-               if (!srv->lb_nodes) {
+
+               if (chash_server_init(srv) < 0) {
                        ha_alert("failed to allocate lb_nodes for server %s.\n", srv->id);
                        return -1;
                }
-               srv->lb_server_key = chash_compute_server_key(srv);
-               for (node = 0; node < srv->lb_nodes_tot; node++) {
-                       srv->lb_nodes[node].server = srv;
-                       srv->lb_nodes[node].node.key = chash_compute_node_key(srv, node);
-               }
 
                if (srv_currently_usable(srv))
                        chash_queue_dequeue_srv(srv);

diff --git a/src/server.c b/src/server.c
index 2452f0382dea635638da2c771664cf8f83f25ac7..61663120ca5ba061ab97e69b34092bcb37dd1a89 100644 (file)
--- a/src/server.c
+++ b/src/server.c
@@ -5888,25 +5888,13 @@ static int cli_parse_enable_server(char **args, char *payload, struct appctx *ap
  */
 static int srv_alloc_lb(struct server *sv, struct proxy *be)
 {
-       int node;
-
        sv->lb_tree = (sv->flags & SRV_F_BACKUP) ?
                      &be->lbprm.chash.bck : &be->lbprm.chash.act;
        sv->lb_nodes_tot = sv->uweight * BE_WEIGHT_SCALE;
        sv->lb_nodes_now = 0;
 
-       if (((be->lbprm.algo & (BE_LB_KIND | BE_LB_PARM)) == (BE_LB_KIND_RR | BE_LB_RR_RANDOM)) ||
-           ((be->lbprm.algo & (BE_LB_KIND | BE_LB_HASH_TYPE)) == (BE_LB_KIND_HI | BE_LB_HASH_CONS))) {
-               sv->lb_nodes = calloc(sv->lb_nodes_tot, sizeof(*sv->lb_nodes));
-
-               if (!sv->lb_nodes)
-                       return 0;
-
-               for (node = 0; node < sv->lb_nodes_tot; node++) {
-                       sv->lb_nodes[node].server = sv;
-                       sv->lb_nodes[node].node.key = full_hash(sv->puid * SRV_EWGHT_RANGE + node);
-               }
-       }
+       if (be->lbprm.server_init && be->lbprm.server_init(sv) < 0)
+               return 0; // typically out of memory
 
        return 1;
 }