fix(balancer): respect max retries (#12346)

In the balancer phase, when obtaining a connection from the upstream connection pool, the `cached` attribute of the peer connection is set to 1(`pc->cached = 1;`), indicating that the connection is obtained from the cache. If an error occurs during the use of this connection, such as "upstream prematurely closed connection" the system will increase the `tries` attribute of the peer connection by executing `u->peer.tries++`. `tries` represents the maximum number of attempts to connect to an upstream server. It is equal to the normal 1 attempt + `retries` (default value is 5) = 6. The occurrence of `u->peer.tries++` is unexpected and it results in the actual retry count exceeding 6 in worst cases. This PR restores tries by callbacks to the balancer when `u->peer.tries++` is unexpectedly set. FIX [FTI-5616](https://konghq.atlassian.net/browse/FTI-5616) Signed-off-by: tzssangglass <tzssangglass@gmail.com>
Kong · Jan 22, 2024 · bc80125 · bc80125
1 parent caab84f
commit bc80125
Show file tree

Hide file tree

Showing 3 changed files with 202 additions and 33 deletions.
diff --git a/build/openresty/patches/ngx_lua-0.10.25_01-dyn_upstream_keepalive.patch b/build/openresty/patches/ngx_lua-0.10.25_01-dyn_upstream_keepalive.patch
@@ -1,8 +1,33 @@
+diff --git a/bundle/nginx-1.21.4/src/http/ngx_http_upstream.c b/bundle/nginx-1.21.4/src/http/ngx_http_upstream.c
+index b07e564..9e25905 100644
+--- a/bundle/nginx-1.21.4/src/http/ngx_http_upstream.c
++++ b/bundle/nginx-1.21.4/src/http/ngx_http_upstream.c
+@@ -4304,6 +4304,7 @@ ngx_http_upstream_next(ngx_http_request_t *r, ngx_http_upstream_t *u,
+     if (u->peer.cached && ft_type == NGX_HTTP_UPSTREAM_FT_ERROR) {
+         /* TODO: inform balancer instead */
+         u->peer.tries++;
++        u->peer.notify(&u->peer, u->peer.data, NGX_HTTP_UPSTREAM_NOFITY_CACHED_CONNECTION_ERROR);
+     }
+
+     switch (ft_type) {
+diff --git a/bundle/nginx-1.21.4/src/http/ngx_http_upstream.h b/bundle/nginx-1.21.4/src/http/ngx_http_upstream.h
+index a385222..1cd214c 100644
+--- a/bundle/nginx-1.21.4/src/http/ngx_http_upstream.h
++++ b/bundle/nginx-1.21.4/src/http/ngx_http_upstream.h
+@@ -56,6 +56,8 @@
+ #define NGX_HTTP_UPSTREAM_IGN_VARY           0x00000200
+
+
++#define NGX_HTTP_UPSTREAM_NOFITY_CACHED_CONNECTION_ERROR    0x1
++
+ typedef struct {
+     ngx_uint_t                       status;
+     ngx_msec_t                       response_time;
 diff --git a/bundle/ngx_lua-0.10.25/src/ngx_http_lua_balancer.c b/bundle/ngx_lua-0.10.25/src/ngx_http_lua_balancer.c
-index af4da73..407c115 100644
+index af4da73..99d073a 100644
 --- a/bundle/ngx_lua-0.10.25/src/ngx_http_lua_balancer.c
 +++ b/bundle/ngx_lua-0.10.25/src/ngx_http_lua_balancer.c
-@@ -16,46 +16,104 @@
+@@ -16,46 +16,106 @@
  #include "ngx_http_lua_directive.h"
 
 
@@ -96,6 +121,8 @@ index af4da73..407c115 100644
 -    ngx_http_request_t *r);
  static void ngx_http_lua_balancer_free_peer(ngx_peer_connection_t *pc,
      void *data, ngx_uint_t state);
++static void ngx_http_lua_balancer_notify_peer(ngx_peer_connection_t *pc,
++    void *data, ngx_uint_t type);
 +static ngx_int_t ngx_http_lua_balancer_create_keepalive_pool(lua_State *L,
 +    ngx_log_t *log, ngx_str_t *cpool_name, ngx_uint_t cpool_size,
 +    ngx_http_lua_balancer_keepalive_pool_t **cpool);
@@ -127,7 +154,7 @@ index af4da73..407c115 100644
 
 
  ngx_int_t
-@@ -102,6 +160,61 @@ ngx_http_lua_balancer_handler_inline(ngx_http_request_t *r,
+@@ -102,6 +162,61 @@ ngx_http_lua_balancer_handler_inline(ngx_http_request_t *r,
  }
 
 
@@ -189,7 +216,7 @@ index af4da73..407c115 100644
  char *
  ngx_http_lua_balancer_by_lua_block(ngx_conf_t *cf, ngx_command_t *cmd,
      void *conf)
-@@ -125,18 +238,20 @@ char *
+@@ -125,18 +240,20 @@ char *
  ngx_http_lua_balancer_by_lua(ngx_conf_t *cf, ngx_command_t *cmd,
      void *conf)
  {
@@ -218,7 +245,7 @@ index af4da73..407c115 100644
      if (cmd->post == NULL) {
          return NGX_CONF_ERROR;
      }
-@@ -188,11 +303,42 @@ ngx_http_lua_balancer_by_lua(ngx_conf_t *cf, ngx_command_t *cmd,
+@@ -188,11 +305,42 @@ ngx_http_lua_balancer_by_lua(ngx_conf_t *cf, ngx_command_t *cmd,
 
      lscf->balancer.src_key = cache_key;
 
@@ -261,7 +288,7 @@ index af4da73..407c115 100644
      }
 
      uscf->peer.init_upstream = ngx_http_lua_balancer_init;
-@@ -208,14 +354,18 @@ ngx_http_lua_balancer_by_lua(ngx_conf_t *cf, ngx_command_t *cmd,
+@@ -208,14 +356,18 @@ ngx_http_lua_balancer_by_lua(ngx_conf_t *cf, ngx_command_t *cmd,
 
 
  static ngx_int_t
@@ -284,7 +311,7 @@ index af4da73..407c115 100644
      us->peer.init = ngx_http_lua_balancer_init_peer;
 
      return NGX_OK;
-@@ -226,33 +376,38 @@ static ngx_int_t
+@@ -226,33 +378,39 @@ static ngx_int_t
  ngx_http_lua_balancer_init_peer(ngx_http_request_t *r,
      ngx_http_upstream_srv_conf_t *us)
  {
@@ -317,6 +344,7 @@ index af4da73..407c115 100644
 +    r->upstream->peer.data = bp;
      r->upstream->peer.get = ngx_http_lua_balancer_get_peer;
      r->upstream->peer.free = ngx_http_lua_balancer_free_peer;
++    r->upstream->peer.notify = ngx_http_lua_balancer_notify_peer;
 
  #if (NGX_HTTP_SSL)
 +    bp->original_set_session = r->upstream->peer.set_session;
@@ -334,7 +362,7 @@ index af4da73..407c115 100644
      return NGX_OK;
  }
 
-@@ -260,25 +415,26 @@ ngx_http_lua_balancer_init_peer(ngx_http_request_t *r,
+@@ -260,25 +418,26 @@ ngx_http_lua_balancer_init_peer(ngx_http_request_t *r,
  static ngx_int_t
  ngx_http_lua_balancer_get_peer(ngx_peer_connection_t *pc, void *data)
  {
@@ -372,7 +400,7 @@ index af4da73..407c115 100644
      if (ctx == NULL) {
          ctx = ngx_http_lua_create_ctx(r);
          if (ctx == NULL) {
-@@ -296,21 +452,23 @@ ngx_http_lua_balancer_get_peer(ngx_peer_connection_t *pc, void *data)
+@@ -296,21 +455,23 @@ ngx_http_lua_balancer_get_peer(ngx_peer_connection_t *pc, void *data)
 
      ctx->context = NGX_HTTP_LUA_CONTEXT_BALANCER;
 
@@ -403,7 +431,7 @@ index af4da73..407c115 100644
      if (rc == NGX_ERROR) {
          return NGX_ERROR;
      }
-@@ -332,79 +490,88 @@ ngx_http_lua_balancer_get_peer(ngx_peer_connection_t *pc, void *data)
+@@ -332,79 +493,88 @@ ngx_http_lua_balancer_get_peer(ngx_peer_connection_t *pc, void *data)
          }
      }
 
@@ -537,7 +565,7 @@ index af4da73..407c115 100644
      return rc;
  }
 
-@@ -413,24 +580,354 @@ static void
+@@ -413,24 +583,364 @@ static void
  ngx_http_lua_balancer_free_peer(ngx_peer_connection_t *pc, void *data,
      ngx_uint_t state)
  {
@@ -677,6 +705,16 @@ index af4da73..407c115 100644
 +}
 +
 +
++static void
++ngx_http_lua_balancer_notify_peer(ngx_peer_connection_t *pc, void *data,
++    ngx_uint_t type)
++{
++    if (type == NGX_HTTP_UPSTREAM_NOFITY_CACHED_CONNECTION_ERROR) {
++        pc->tries--;
++    }
++}
++
++
 +static ngx_int_t
 +ngx_http_lua_balancer_create_keepalive_pool(lua_State *L, ngx_log_t *log,
 +    ngx_str_t *cpool_name, ngx_uint_t cpool_size,
@@ -795,15 +833,17 @@ index af4da73..407c115 100644
 +
 +    if (lua_isnil(L, -1)) {
 +        lua_pop(L, 1); /* orig stack */
-+        return;
-+    }
-+
+         return;
+     }
+
+-    /* fallback */
 +    ngx_http_lua_assert(lua_istable(L, -1));
 +
 +    lua_pushlstring(L, (const char *)cpool->cpool_name.data, cpool->cpool_name.len);
 +    lua_pushnil(L); /* pools nil */
 +    lua_rawset(L, -3); /* pools */
-+
+
+-    ngx_http_upstream_free_round_robin_peer(pc, data, state);
 +    ngx_log_debug2(NGX_LOG_DEBUG_HTTP, log, 0,
 +                   "lua balancer: keepalive free pool, "
 +                   "name: %V, cpool: %p",
@@ -876,16 +916,14 @@ index af4da73..407c115 100644
 +            goto close;
 +        }
 +
-         return;
-     }
-
--    /* fallback */
++        return;
++    }
++
 +close:
 +
 +    item = c->data;
 +    c->log = ev->log;
-
--    ngx_http_upstream_free_round_robin_peer(pc, data, state);
++
 +    ngx_http_lua_balancer_close(c);
 +
 +    ngx_queue_remove(&item->queue);
@@ -897,7 +935,7 @@ index af4da73..407c115 100644
  }
 
 
-@@ -441,12 +938,12 @@ ngx_http_lua_balancer_set_session(ngx_peer_connection_t *pc, void *data)
+@@ -441,12 +951,12 @@ ngx_http_lua_balancer_set_session(ngx_peer_connection_t *pc, void *data)
  {
      ngx_http_lua_balancer_peer_data_t  *bp = data;
 
@@ -912,7 +950,7 @@ index af4da73..407c115 100644
  }
 
 
-@@ -455,13 +952,12 @@ ngx_http_lua_balancer_save_session(ngx_peer_connection_t *pc, void *data)
+@@ -455,13 +965,12 @@ ngx_http_lua_balancer_save_session(ngx_peer_connection_t *pc, void *data)
  {
      ngx_http_lua_balancer_peer_data_t  *bp = data;
 
@@ -928,7 +966,7 @@ index af4da73..407c115 100644
  }
 
  #endif
-@@ -469,14 +965,14 @@ ngx_http_lua_balancer_save_session(ngx_peer_connection_t *pc, void *data)
+@@ -469,14 +978,14 @@ ngx_http_lua_balancer_save_session(ngx_peer_connection_t *pc, void *data)
 
  int
  ngx_http_lua_ffi_balancer_set_current_peer(ngx_http_request_t *r,
@@ -950,7 +988,7 @@ index af4da73..407c115 100644
 
      if (r == NULL) {
          *err = "no request found";
-@@ -501,18 +997,6 @@ ngx_http_lua_ffi_balancer_set_current_peer(ngx_http_request_t *r,
+@@ -501,18 +1010,6 @@ ngx_http_lua_ffi_balancer_set_current_peer(ngx_http_request_t *r,
          return NGX_ERROR;
      }
 
@@ -969,7 +1007,7 @@ index af4da73..407c115 100644
      ngx_memzero(&url, sizeof(ngx_url_t));
 
      url.url.data = ngx_palloc(r->pool, addr_len);
-@@ -536,6 +1020,8 @@ ngx_http_lua_ffi_balancer_set_current_peer(ngx_http_request_t *r,
+@@ -536,6 +1033,8 @@ ngx_http_lua_ffi_balancer_set_current_peer(ngx_http_request_t *r,
          return NGX_ERROR;
      }
 
@@ -978,7 +1016,7 @@ index af4da73..407c115 100644
      if (url.addrs && url.addrs[0].sockaddr) {
          bp->sockaddr = url.addrs[0].sockaddr;
          bp->socklen = url.addrs[0].socklen;
-@@ -546,6 +1032,72 @@ ngx_http_lua_ffi_balancer_set_current_peer(ngx_http_request_t *r,
+@@ -546,6 +1045,72 @@ ngx_http_lua_ffi_balancer_set_current_peer(ngx_http_request_t *r,
          return NGX_ERROR;
      }
 
@@ -1051,7 +1089,7 @@ index af4da73..407c115 100644
      return NGX_OK;
  }
 
-@@ -555,14 +1107,13 @@ ngx_http_lua_ffi_balancer_set_timeouts(ngx_http_request_t *r,
+@@ -555,14 +1120,13 @@ ngx_http_lua_ffi_balancer_set_timeouts(ngx_http_request_t *r,
      long connect_timeout, long send_timeout, long read_timeout,
      char **err)
  {
@@ -1069,7 +1107,7 @@ index af4da73..407c115 100644
 
      if (r == NULL) {
          *err = "no request found";
-@@ -587,15 +1138,9 @@ ngx_http_lua_ffi_balancer_set_timeouts(ngx_http_request_t *r,
+@@ -587,15 +1151,9 @@ ngx_http_lua_ffi_balancer_set_timeouts(ngx_http_request_t *r,
          return NGX_ERROR;
      }
 
@@ -1087,7 +1125,7 @@ index af4da73..407c115 100644
      if (!bp->cloned_upstream_conf) {
          /* we clone the upstream conf for the current request so that
           * we do not affect other requests at all. */
-@@ -650,12 +1195,10 @@ ngx_http_lua_ffi_balancer_set_more_tries(ngx_http_request_t *r,
+@@ -650,12 +1208,10 @@ ngx_http_lua_ffi_balancer_set_more_tries(ngx_http_request_t *r,
      int count, char **err)
  {
  #if (nginx_version >= 1007005)
@@ -1103,7 +1141,7 @@ index af4da73..407c115 100644
      ngx_http_lua_balancer_peer_data_t  *bp;
 
      if (r == NULL) {
-@@ -681,13 +1224,7 @@ ngx_http_lua_ffi_balancer_set_more_tries(ngx_http_request_t *r,
+@@ -681,13 +1237,7 @@ ngx_http_lua_ffi_balancer_set_more_tries(ngx_http_request_t *r,
          return NGX_ERROR;
      }
 
@@ -1118,7 +1156,7 @@ index af4da73..407c115 100644
 
  #if (nginx_version >= 1007005)
      max_tries = r->upstream->conf->next_upstream_tries;
-@@ -713,12 +1250,10 @@ int
+@@ -713,12 +1263,10 @@ int
  ngx_http_lua_ffi_balancer_get_last_failure(ngx_http_request_t *r,
      int *status, char **err)
  {
@@ -1134,7 +1172,7 @@ index af4da73..407c115 100644
 
      if (r == NULL) {
          *err = "no request found";
-@@ -743,13 +1278,7 @@ ngx_http_lua_ffi_balancer_get_last_failure(ngx_http_request_t *r,
+@@ -743,13 +1291,7 @@ ngx_http_lua_ffi_balancer_get_last_failure(ngx_http_request_t *r,
          return NGX_ERROR;
      }
 

diff --git a/changelog/unreleased/kong/balancer_respect_max_retries.yml b/changelog/unreleased/kong/balancer_respect_max_retries.yml
@@ -0,0 +1,3 @@
+message: Fix an issue that the actual number of retry times exceeds the `retries` setting.
+type: bugfix
+scope: Core