BUG/MEDIUM: server: Properly check for streams before deletion

When a server delete command happens, before we actually delete the
server, we check if there are any streams still attached to the server,
and will refuse the deletion if it is so. Unfortunately, the check was
not exhaustive, we would check if there's any stream in the queue, if
there are any connection to the server established, and if served is
non-zero.
But there are at least two cases where a stream will have a reference to
the server, but none of those will be true : the first one is the case
where we tried a connection to the server, and it failed. At this point
we decremented served, but we will still hold a reference to the server
in target until a new server has been assigned. The second one happens
with cookie persistence, in which case target will be set to the server
address way before any connection is attempted, and so way before served
is incremented.
To fix that, introduce a new per-thread-group counter in the server
structure, nb_strm, that counts the streams whose target points to that
server. It is incremented when a stream's target is set to a server, and
decremented when the target changes to another server, to a non-server,
or the stream ends. To make this reliable, all assignments to a stream's
target now go through the new stream_set_target() helper, which keeps
nb_strm up to date and also clears sv_tgcounters when the target is no
longer a server, so we don't keep a dangling reference to the server's
counters (its failure counters have already been accounted for by that
point). To check if there are still any streams attached to the server,
the code just looks at all the per-thread-group counters, and if one of
them is non-zero, there are still streams on that server. This will
always be accurate because the server deletion command runs with thread
isolation.

This should be backported up to 3.0.

diff --git a/include/haproxy/server-t.h b/include/haproxy/server-t.h
index c2bd6c7db9d5662badae0a9e6491c93e989ad023..75e1cf6033bdcd3b8f8cef0d882e5a2ff3bf564e 100644 (file)
--- a/include/haproxy/server-t.h
+++ b/include/haproxy/server-t.h
@@ -296,6 +296,7 @@ struct srv_per_tgroup {
        struct server *next_full;               /* next server in the temporary full list */
        unsigned int last_other_tgrp_served;    /* Last other tgrp we dequeued from */
        unsigned int self_served;               /* Number of connection we dequeued from our own queue */
+       unsigned int nb_strm;                   /* nb streams in this tgroup referencing this server */
        unsigned int dequeuing;                 /* non-zero = dequeuing in progress (atomic) */
        unsigned int next_takeover;             /* thread ID to try to steal connections from next time */
        struct eb_root *lb_tree;                 /* For LB algos with split between thread groups, the tree to be used, for each group */

diff --git a/include/haproxy/stream.h b/include/haproxy/stream.h
index fe66d6cb433f5cd40975ae89e37f1fdddb3d483e..af5d2f45f0accbfdb5d58a4afb8c3778051b559c 100644 (file)
--- a/include/haproxy/stream.h
+++ b/include/haproxy/stream.h
@@ -29,6 +29,7 @@
 #include <haproxy/obj_type.h>
 #include <haproxy/pool-t.h>
 #include <haproxy/queue.h>
+#include <haproxy/server.h>
 #include <haproxy/session.h>
 #include <haproxy/stconn.h>
 #include <haproxy/stick_table.h>
@@ -432,10 +433,34 @@ static inline void stream_report_term_evt(struct stconn *sc, enum strm_term_even
        sc->term_evts_log = tevt_report_event(sc->term_evts_log, loc, type);
 }
 
+/*
+ * Sets the stream's target, and take care of nb_strm and sv_tgcounters if
+ * the target is a server.
+ */
+static inline void stream_set_target(struct stream *s, enum obj_type *target)
+{
+       struct server *o = objt_server(s->target);
+       struct server *n = objt_server(target);
+
+       if (o != n) {
+               if (n) {
+                       _HA_ATOMIC_INC(&n->per_tgrp[tgid - 1].nb_strm);
+                       s->sv_tgcounters = n->counters.shared.tg ? n->counters.shared.tg[tgid - 1] : NULL;
+               }
+               else
+                       s->sv_tgcounters = NULL;
+               if (o)
+                       _HA_ATOMIC_DEC(&o->per_tgrp[tgid - 1].nb_strm);
+       }
+       s->target = target;
+}
+
+/* Convenience wrapper of stream_set_target() for callers that already hold a
+ * struct server, so they can pass it directly.
+ */
 static inline void stream_set_srv_target(struct stream *s, struct server *srv)
 {
-       s->target = &srv->obj_type;
-       s->sv_tgcounters = srv->counters.shared.tg[tgid - 1];
+       stream_set_target(s, &srv->obj_type);
 }
 
 int stream_set_timeout(struct stream *s, enum act_timeout_name name, int timeout);

diff --git a/src/backend.c b/src/backend.c
index 4001135ba2daae31ea2bfc93fdfb20c4d4ddfcc6..388e4ad032b064d369d3cedf88f90582a2e578ab 100644 (file)
--- a/src/backend.c
+++ b/src/backend.c
@@ -668,7 +668,7 @@ int assign_server(struct stream *s)
         */
 
        srv = NULL;
-       s->target = NULL;
+       stream_set_target(s, NULL);
 
        if ((s->be->lbprm.algo & BE_LB_KIND) != BE_LB_KIND_HI &&
            ((s->sess->flags & SESS_FL_PREFER_LAST) ||
@@ -3041,7 +3041,7 @@ int tcp_persist_rdp_cookie(struct stream *s, struct channel *req, int an_bit)
        if (*p != '.')
                goto no_cookie;
 
-       s->target = NULL;
+       stream_set_target(s, NULL);
        while (srv) {
                if (srv->addr.ss_family == AF_INET &&
                    port == srv->svc_port &&

diff --git a/src/cache.c b/src/cache.c
index 319396b0061db407f9b6731373397110fd1bb04e..dbee7ae7afeac4a2d8b7c168a25ba425ab9562c3 100644 (file)
--- a/src/cache.c
+++ b/src/cache.c
@@ -2725,6 +2725,9 @@ enum act_return http_action_req_cache_use(struct act_rule *rule, struct proxy *p
                        return ACT_RET_CONT;
                }
 
+               /* This runs before any server assignment, so s->target holds no
+                * server here and there is no nb_strm reference to drop.
+                */
                s->target = &http_cache_applet.obj_type;
                if ((appctx = sc_applet_create(s->scb, objt_applet(s->target)))) {
                        struct cache_appctx *ctx = applet_reserve_svcctx(appctx, sizeof(*ctx));

diff --git a/src/cli.c b/src/cli.c
index 059e256cb7ee4f6d784e915eef8dc2648d3a5f33..4c79ecbc850443cc9d2a7340ecb6d51a01cee665 100644 (file)
--- a/src/cli.c
+++ b/src/cli.c
@@ -3615,15 +3615,7 @@ read_again:
                        else
                                target_pid = pcli->next_pid;
                        /* we can connect now */
-                       s->target = pcli_pid_to_server(target_pid);
-                       if (objt_server(s->target)) {
-                               struct server *srv = __objt_server(s->target);
-
-                               if (srv->counters.shared.tg)
-                                       s->sv_tgcounters = srv->counters.shared.tg[tgid - 1];
-                               else
-                                       s->sv_tgcounters = NULL;
-                       }
+                       stream_set_target(s, pcli_pid_to_server(target_pid));
 
                        if (!s->target)
                                goto server_disconnect;
@@ -3788,7 +3780,7 @@ int pcli_wait_for_response(struct stream *s, struct channel *rep, int an_bit)
                                process_srv_queue(__objt_server(s->target));
                }
 
-               s->target = NULL;
+               stream_set_target(s, NULL);
 
                /* Always release our endpoint */
                s->srv_conn = NULL;
@@ -3827,7 +3819,7 @@ int pcli_wait_for_response(struct stream *s, struct channel *rep, int an_bit)
                s->logs.logwait = strm_fe(s)->to_log;
                s->logs.level = 0;
                stream_del_srv_conn(s);
-               s->target = NULL;
+               stream_set_target(s, NULL);
                /* re-init store persistence */
                s->store_count = 0;
                s->uniq_id = _HA_ATOMIC_FETCH_ADD(&global.req_count, 1);

diff --git a/src/http_ana.c b/src/http_ana.c
index 019adaece8d8a38cc4f0d58d1909700f9987f922..75369fe183d589fbe73e20f55bb3e1b0d64ead7f 100644 (file)
--- a/src/http_ana.c
+++ b/src/http_ana.c
@@ -472,6 +472,7 @@ int http_process_req_common(struct stream *s, struct channel *req, int an_bit, s
         * reqdeny can still block them. This clearly needs to change in 1.6!
         */
        if (!s->target && http_stats_check_uri(s, txn, px)) {
+               /* s->target was NULL (checked above), no nb_strm reference to drop */
                s->target = &http_stats_applet.obj_type;
                if (unlikely(!sc_applet_create(s->scb, objt_applet(s->target)))) {
                        s->logs.request_ts = now_ns;

diff --git a/src/http_client.c b/src/http_client.c
index 3bdbb80803a7778ca9281a8645a4bca50f9131a2..a4c96e8577ba9b0ae67b7049964bebada60f32ab 100644 (file)
--- a/src/http_client.c
+++ b/src/http_client.c
@@ -958,15 +958,7 @@ int httpclient_applet_init(struct appctx *appctx)
        }
 
        s = appctx_strm(appctx);
-       s->target = target;
-       if (objt_server(s->target)) {
-               struct server *srv = __objt_server(s->target);
-
-               if (srv->counters.shared.tg)
-                       s->sv_tgcounters = __objt_server(s->target)->counters.shared.tg[tgid - 1];
-               else
-                       s->sv_tgcounters = NULL;
-       }
+       stream_set_target(s, target);
 
        /* set the "timeout server" */
        s->scb->ioto = hc->timeout_server;

diff --git a/src/server.c b/src/server.c
index 15dd282acaf2803b51111e137d9e4b08d210c2df..4bdcb8be00484be25c2a8cd17e1af53def74dabd 100644 (file)
--- a/src/server.c
+++ b/src/server.c
@@ -2095,7 +2095,13 @@ static int srv_parse_strict_maxconn(char **args, int *cur_arg, struct proxy *px,
 /* Returns 1 if the server has streams pointing to it, and 0 otherwise. */
 static int srv_has_streams(struct server *srv)
 {
-       return !!_HA_ATOMIC_LOAD(&srv->served);
+       int i;
+
+       for (i = 0; i < global.nbtgroups; i++) {
+               if (_HA_ATOMIC_LOAD(&srv->per_tgrp[i].nb_strm))
+                       return 1;
+       }
+       return 0;
 }
 
 /* Shutdown all connections of a server. The caller must pass a termination

diff --git a/src/stream.c b/src/stream.c
index 862f5576b948aabe69967c1a8f4160a2229a9542..76414565d96e4dc16dbae14b7770030d9c59fd01 100644 (file)
--- a/src/stream.c
+++ b/src/stream.c
@@ -770,6 +770,8 @@ void stream_free(struct stream *s)
                }
        }
 
+       stream_set_target(s, NULL);
+
        pool_free(pool_head_stream, s);
 
        /* We may want to free the maximum amount of pools if the proxy is stopping */
@@ -1115,7 +1117,7 @@ enum act_return process_use_service(struct act_rule *rule, struct proxy *px,
        /* Initialises the applet if it is required. */
        if (flags & ACT_OPT_FIRST) {
                /* Register applet. this function schedules the applet. */
-               s->target = &rule->applet.obj_type;
+               stream_set_target(s, &rule->applet.obj_type);
                appctx = sc_applet_create(s->scb, objt_applet(s->target));
                if (unlikely(!appctx))
                        return ACT_RET_ERR;