From 5160fee71a3125b2a0678eafdcfc52ffc4836807 Mon Sep 17 00:00:00 2001 From: tzssangglass Date: Thu, 18 Jan 2024 14:09:19 +0800 Subject: [PATCH] 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 --- ...ua-0.10.21_02-dyn_upstream_keepalive.patch | 486 ++++++++++-------- .../kong/balancer_respect_max_retries.yml | 3 + .../05-proxy/10-balancer/08-retries_spec.lua | 128 +++++ 3 files changed, 393 insertions(+), 224 deletions(-) create mode 100644 changelog/unreleased/kong/balancer_respect_max_retries.yml create mode 100644 spec/02-integration/05-proxy/10-balancer/08-retries_spec.lua diff --git a/build/openresty/patches/ngx_lua-0.10.21_02-dyn_upstream_keepalive.patch b/build/openresty/patches/ngx_lua-0.10.21_02-dyn_upstream_keepalive.patch index 23117eb0044..a54e45e5c80 100644 --- a/build/openresty/patches/ngx_lua-0.10.21_02-dyn_upstream_keepalive.patch +++ b/build/openresty/patches/ngx_lua-0.10.21_02-dyn_upstream_keepalive.patch @@ -1,10 +1,36 @@ -diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c ---- a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c 2022-12-02 10:58:50.054203731 +0800 -+++ b/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c 2022-12-05 18:22:15.351308080 +0800 -@@ -16,46 +16,104 @@ +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..a1c4a0d 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.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c +index e4ac57a..4c36593 100644 +--- a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c ++++ b/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c +@@ -16,46 +16,106 @@ #include "ngx_http_lua_directive.h" - - + + +typedef struct { + ngx_uint_t size; + ngx_uint_t connections; @@ -37,46 +63,46 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- + ngx_uint_t total_tries; + + int last_peer_state; -+ -+ ngx_str_t cpool_name; - + - ngx_http_lua_srv_conf_t *conf; - ngx_http_request_t *request; -+ void *data; - ++ ngx_str_t cpool_name; + - ngx_uint_t more_tries; - ngx_uint_t total_tries; -+ ngx_event_get_peer_pt original_get_peer; -+ ngx_event_free_peer_pt original_free_peer; - ++ void *data; + - struct sockaddr *sockaddr; - socklen_t socklen; ++ ngx_event_get_peer_pt original_get_peer; ++ ngx_event_free_peer_pt original_free_peer; ++ +#if (NGX_HTTP_SSL) + ngx_event_set_peer_session_pt original_set_session; + ngx_event_save_peer_session_pt original_save_session; +#endif -+ + +- ngx_str_t *host; +- in_port_t port; + ngx_http_request_t *request; + ngx_http_lua_srv_conf_t *conf; + ngx_http_lua_balancer_keepalive_pool_t *cpool; - -- ngx_str_t *host; -- in_port_t port; -+ ngx_str_t *host; - + - int last_peer_state; ++ ngx_str_t *host; ++ + struct sockaddr *sockaddr; + socklen_t socklen; + + unsigned keepalive:1; - + #if !(HAVE_NGX_UPSTREAM_TIMEOUT_FIELDS) - unsigned cloned_upstream_conf; /* :1 */ + unsigned cloned_upstream_conf:1; #endif }; - - + + -#if (NGX_HTTP_SSL) -static ngx_int_t ngx_http_lua_balancer_set_session(ngx_peer_connection_t *pc, - void *data); @@ -95,6 +121,8 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- - 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); @@ -123,13 +151,13 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- + +static char ngx_http_lua_balancer_keepalive_pools_table_key; +static struct sockaddr *ngx_http_lua_balancer_default_server_sockaddr; - - + + ngx_int_t -@@ -102,6 +160,61 @@ +@@ -102,6 +162,61 @@ ngx_http_lua_balancer_handler_inline(ngx_http_request_t *r, } - - + + +static ngx_int_t +ngx_http_lua_balancer_by_chunk(lua_State *L, ngx_http_request_t *r) +{ @@ -188,7 +216,7 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- char * ngx_http_lua_balancer_by_lua_block(ngx_conf_t *cf, ngx_command_t *cmd, void *conf) -@@ -125,16 +238,18 @@ +@@ -125,16 +240,18 @@ char * ngx_http_lua_balancer_by_lua(ngx_conf_t *cf, ngx_command_t *cmd, void *conf) { @@ -204,23 +232,23 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- ngx_http_upstream_srv_conf_t *uscf; + ngx_http_upstream_server_t *us; + ngx_http_lua_srv_conf_t *lscf = conf; - + dd("enter"); - + - /* must specify a content handler */ + /* content handler setup */ + if (cmd->post == NULL) { return NGX_CONF_ERROR; } -@@ -178,11 +293,42 @@ - +@@ -178,11 +295,42 @@ ngx_http_lua_balancer_by_lua(ngx_conf_t *cf, ngx_command_t *cmd, + lscf->balancer.src_key = cache_key; - + + /* balancer setup */ + uscf = ngx_http_conf_get_module_srv_conf(cf, ngx_http_upstream_module); - + + if (uscf->servers->nelts == 0) { + us = ngx_array_push(uscf->servers); + if (us == NULL) { @@ -254,11 +282,11 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- + lscf->balancer.original_init_upstream = + ngx_http_upstream_init_round_robin; } - + uscf->peer.init_upstream = ngx_http_lua_balancer_init; -@@ -198,14 +344,18 @@ - - +@@ -198,14 +346,18 @@ ngx_http_lua_balancer_by_lua(ngx_conf_t *cf, ngx_command_t *cmd, + + static ngx_int_t -ngx_http_lua_balancer_init(ngx_conf_t *cf, - ngx_http_upstream_srv_conf_t *us) @@ -272,21 +300,21 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- + if (lscf->balancer.original_init_upstream(cf, us) != NGX_OK) { return NGX_ERROR; } - + - /* this callback is called upon individual requests */ + lscf->balancer.original_init_peer = us->peer.init; + us->peer.init = ngx_http_lua_balancer_init_peer; - + return NGX_OK; -@@ -216,33 +366,38 @@ +@@ -216,33 +368,39 @@ static ngx_int_t ngx_http_lua_balancer_init_peer(ngx_http_request_t *r, ngx_http_upstream_srv_conf_t *us) { - ngx_http_lua_srv_conf_t *bcf; + ngx_http_lua_srv_conf_t *lscf; ngx_http_lua_balancer_peer_data_t *bp; - + - bp = ngx_pcalloc(r->pool, sizeof(ngx_http_lua_balancer_peer_data_t)); - if (bp == NULL) { + lscf = ngx_http_conf_upstream_srv_conf(us, ngx_http_lua_module); @@ -294,7 +322,7 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- + if (lscf->balancer.original_init_peer(r, us) != NGX_OK) { return NGX_ERROR; } - + - r->upstream->peer.data = &bp->rrp; - - if (ngx_http_upstream_init_round_robin_peer(r, us) != NGX_OK) { @@ -302,7 +330,7 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- + if (bp == NULL) { return NGX_ERROR; } - + + bp->conf = lscf; + bp->request = r; + bp->data = r->upstream->peer.data; @@ -312,7 +340,8 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- + 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; + bp->original_save_session = r->upstream->peer.save_session; @@ -320,7 +349,7 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- r->upstream->peer.set_session = ngx_http_lua_balancer_set_session; r->upstream->peer.save_session = ngx_http_lua_balancer_save_session; #endif - + - bcf = ngx_http_conf_upstream_srv_conf(us, ngx_http_lua_module); - - bp->conf = bcf; @@ -328,8 +357,8 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- - return NGX_OK; } - -@@ -250,25 +405,26 @@ + +@@ -250,25 +408,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) { @@ -350,27 +379,27 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- + ngx_http_lua_balancer_keepalive_item_t *item; + ngx_http_lua_balancer_peer_data_t *bp = data; + void *pdata; - + ngx_log_debug1(NGX_LOG_DEBUG_HTTP, pc->log, 0, - "lua balancer peer, tries: %ui", pc->tries); - - lscf = bp->conf; + "lua balancer: get peer, tries: %ui", pc->tries); - + r = bp->request; + lscf = bp->conf; - + ngx_http_lua_assert(lscf->balancer.handler && r); - + ctx = ngx_http_get_module_ctx(r, ngx_http_lua_module); - if (ctx == NULL) { ctx = ngx_http_lua_create_ctx(r); if (ctx == NULL) { -@@ -286,21 +442,23 @@ - +@@ -286,21 +445,23 @@ ngx_http_lua_balancer_get_peer(ngx_peer_connection_t *pc, void *data) + ctx->context = NGX_HTTP_LUA_CONTEXT_BALANCER; - + + bp->cpool = NULL; bp->sockaddr = NULL; bp->socklen = 0; @@ -380,7 +409,7 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- + bp->keepalive_timeout = 0; + bp->keepalive = 0; bp->total_tries++; - + - lmcf = ngx_http_get_module_main_conf(r, ngx_http_lua_module); - - /* balancer_by_lua does not support yielding and @@ -390,18 +419,18 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- - lmcf->balancer_peer_data = bp; + pdata = r->upstream->peer.data; + r->upstream->peer.data = bp; - + rc = lscf->balancer.handler(r, lscf, L); - + + r->upstream->peer.data = pdata; + if (rc == NGX_ERROR) { return NGX_ERROR; } -@@ -322,105 +480,444 @@ +@@ -322,79 +483,88 @@ ngx_http_lua_balancer_get_peer(ngx_peer_connection_t *pc, void *data) } } - + - if (bp->sockaddr && bp->socklen) { + if (ngx_http_lua_balancer_peer_set(bp)) { pc->sockaddr = bp->sockaddr; @@ -412,17 +441,23 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- - pc->name = bp->host; - - bp->rrp.peers->single = 0; - + if (bp->more_tries) { r->upstream->peer.tries += bp->more_tries; } - + - dd("tries: %d", (int) r->upstream->peer.tries); +- +- return NGX_OK; +- } +- +- return ngx_http_upstream_get_round_robin_peer(pc, &bp->rrp); +-} + if (ngx_http_lua_balancer_keepalive_is_enabled(bp)) { + ngx_http_lua_balancer_get_keepalive_pool(L, pc->log, + &bp->cpool_name, + &bp->cpool); -+ + + if (bp->cpool == NULL + && ngx_http_lua_balancer_create_keepalive_pool(L, pc->log, + &bp->cpool_name, @@ -432,52 +467,84 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- + { + return NGX_ERROR; + } -+ + +-static ngx_int_t +-ngx_http_lua_balancer_by_chunk(lua_State *L, ngx_http_request_t *r) +-{ +- u_char *err_msg; +- size_t len; +- ngx_int_t rc; + ngx_http_lua_assert(bp->cpool); -+ + +- /* init nginx context in Lua VM */ +- ngx_http_lua_set_req(L, r); + if (!ngx_queue_empty(&bp->cpool->cache)) { + q = ngx_queue_head(&bp->cpool->cache); -+ + +-#ifndef OPENRESTY_LUAJIT +- ngx_http_lua_create_new_globals_table(L, 0 /* narr */, 1 /* nrec */); + item = ngx_queue_data(q, ngx_http_lua_balancer_keepalive_item_t, + queue); + c = item->connection; -+ + +- /* {{{ make new env inheriting main thread's globals table */ +- lua_createtable(L, 0, 1 /* nrec */); /* the metatable for the new env */ +- ngx_http_lua_get_globals_table(L); +- lua_setfield(L, -2, "__index"); +- lua_setmetatable(L, -2); /* setmetatable({}, {__index = _G}) */ +- /* }}} */ + ngx_queue_remove(q); + ngx_queue_insert_head(&bp->cpool->free, q); -+ + +- lua_setfenv(L, -2); /* set new running env for the code closure */ +-#endif /* OPENRESTY_LUAJIT */ + c->idle = 0; + c->sent = 0; + c->log = pc->log; + c->read->log = pc->log; + c->write->log = pc->log; + c->pool->log = pc->log; -+ + +- lua_pushcfunction(L, ngx_http_lua_traceback); +- lua_insert(L, 1); /* put it under chunk and args */ + if (c->read->timer_set) { + ngx_del_timer(c->read); + } -+ + +- /* protected call user code */ +- rc = lua_pcall(L, 0, 1, 1); + pc->cached = 1; + pc->connection = c; -+ + +- lua_remove(L, 1); /* remove traceback function */ + ngx_log_debug3(NGX_LOG_DEBUG_HTTP, pc->log, 0, + "lua balancer: keepalive reusing connection %p, " + "requests: %ui, cpool: %p", + c, c->requests, bp->cpool); -+ + +- dd("rc == %d", (int) rc); + return NGX_DONE; + } -+ + +- if (rc != 0) { +- /* error occurred when running loaded code */ +- err_msg = (u_char *) lua_tolstring(L, -1, &len); + bp->cpool->connections++; -+ + +- if (err_msg == NULL) { +- err_msg = (u_char *) "unknown reason"; +- len = sizeof("unknown reason") - 1; + ngx_log_debug1(NGX_LOG_DEBUG_HTTP, pc->log, 0, + "lua balancer: keepalive no free connection, " + "cpool: %p", bp->cpool); -+ } - - return NGX_OK; - } - -- return ngx_http_upstream_get_round_robin_peer(pc, &bp->rrp); + } + +- ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, +- "failed to run balancer_by_lua*: %*s", len, err_msg); ++ return NGX_OK; ++ } + +- lua_settop(L, 0); /* clear remaining elems on stack */ + rc = bp->original_get_peer(pc, bp->data); + if (rc == NGX_ERROR) { + return rc; @@ -486,62 +553,44 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- + if (pc->sockaddr == ngx_http_lua_balancer_default_server_sockaddr) { + ngx_log_error(NGX_LOG_ERR, pc->log, 0, + "lua balancer: no peer set"); -+ -+ return NGX_ERROR; -+ } -+ -+ return rc; + + return NGX_ERROR; + } + +- lua_settop(L, 0); /* clear remaining elems on stack */ + return rc; } - - --static ngx_int_t --ngx_http_lua_balancer_by_chunk(lua_State *L, ngx_http_request_t *r) -+static void -+ngx_http_lua_balancer_free_peer(ngx_peer_connection_t *pc, void *data, -+ ngx_uint_t state) + +@@ -403,24 +573,364 @@ static void + ngx_http_lua_balancer_free_peer(ngx_peer_connection_t *pc, void *data, + ngx_uint_t state) { -- u_char *err_msg; -- size_t len; -- ngx_int_t rc; +- ngx_http_lua_balancer_peer_data_t *bp = data; + ngx_queue_t *q; + ngx_connection_t *c; + ngx_http_upstream_t *u; + ngx_http_lua_balancer_keepalive_item_t *item; + ngx_http_lua_balancer_keepalive_pool_t *cpool; + ngx_http_lua_balancer_peer_data_t *bp = data; - -- /* init nginx context in Lua VM */ -- ngx_http_lua_set_req(L, r); -+ ngx_log_debug1(NGX_LOG_DEBUG_HTTP, pc->log, 0, + + ngx_log_debug1(NGX_LOG_DEBUG_HTTP, pc->log, 0, +- "lua balancer free peer, tries: %ui", pc->tries); + "lua balancer: free peer, tries: %ui", pc->tries); - --#ifndef OPENRESTY_LUAJIT -- ngx_http_lua_create_new_globals_table(L, 0 /* narr */, 1 /* nrec */); ++ + u = bp->request->upstream; + c = pc->connection; - -- /* {{{ make new env inheriting main thread's globals table */ -- lua_createtable(L, 0, 1 /* nrec */); /* the metatable for the new env */ -- ngx_http_lua_get_globals_table(L); -- lua_setfield(L, -2, "__index"); -- lua_setmetatable(L, -2); /* setmetatable({}, {__index = _G}) */ -- /* }}} */ + +- if (bp->sockaddr && bp->socklen) { + if (ngx_http_lua_balancer_peer_set(bp)) { -+ bp->last_peer_state = (int) state; - -- lua_setfenv(L, -2); /* set new running env for the code closure */ --#endif /* OPENRESTY_LUAJIT */ -+ if (pc->tries) { -+ pc->tries--; -+ } - -- lua_pushcfunction(L, ngx_http_lua_traceback); -- lua_insert(L, 1); /* put it under chunk and args */ + bp->last_peer_state = (int) state; + + if (pc->tries) { + pc->tries--; + } + + if (ngx_http_lua_balancer_keepalive_is_enabled(bp)) { + cpool = bp->cpool; - -- /* protected call user code */ -- rc = lua_pcall(L, 0, 1, 1); ++ + if (state & NGX_PEER_FAILED + || c == NULL + || c->read->eof @@ -644,23 +693,24 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- + ngx_http_lua_balancer_free_keepalive_pool(pc->log, cpool); + } + } - -- lua_remove(L, 1); /* remove traceback function */ ++ + return; + } - -- dd("rc == %d", (int) rc); ++ + bp->original_free_peer(pc, bp->data, state); +} - -- if (rc != 0) { -- /* error occurred when running loaded code */ -- err_msg = (u_char *) lua_tolstring(L, -1, &len); - -- if (err_msg == NULL) { -- err_msg = (u_char *) "unknown reason"; -- len = sizeof("unknown reason") - 1; -- } ++ ++ ++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, @@ -670,29 +720,24 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- + ngx_uint_t i; + ngx_http_lua_balancer_keepalive_pool_t *upool; + ngx_http_lua_balancer_keepalive_item_t *items; - -- ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, -- "failed to run balancer_by_lua*: %*s", len, err_msg); ++ + /* get upstream connection pools table */ + lua_pushlightuserdata(L, ngx_http_lua_lightudata_mask( + balancer_keepalive_pools_table_key)); + lua_rawget(L, LUA_REGISTRYINDEX); /* pools? */ - -- lua_settop(L, 0); /* clear remaining elems on stack */ ++ + ngx_http_lua_assert(lua_istable(L, -1)); + + lua_pushlstring(L, (const char *)cpool_name->data, cpool_name->len); - ++ + size = sizeof(ngx_http_lua_balancer_keepalive_pool_t) + + sizeof(ngx_http_lua_balancer_keepalive_item_t) * cpool_size; + + upool = lua_newuserdata(L, size + cpool_name->len); /* pools upool */ + if (upool == NULL) { - return NGX_ERROR; - } - -- lua_settop(L, 0); /* clear remaining elems on stack */ -- return rc; ++ return NGX_ERROR; ++ } ++ + ngx_log_debug2(NGX_LOG_DEBUG_HTTP, log, 0, + "lua balancer: keepalive create pool, " + "name: %V, size: %ui", @@ -724,33 +769,24 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- + *cpool = upool; + + return NGX_OK; - } - - - 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_get_keepalive_pool(lua_State *L, + ngx_log_t *log, ngx_str_t *cpool_name, + ngx_http_lua_balancer_keepalive_pool_t **cpool) - { -- ngx_http_lua_balancer_peer_data_t *bp = data; ++{ + ngx_http_lua_balancer_keepalive_pool_t *upool; - -- ngx_log_debug1(NGX_LOG_DEBUG_HTTP, pc->log, 0, -- "lua balancer free peer, tries: %ui", pc->tries); ++ + /* get upstream connection pools table */ + lua_pushlightuserdata(L, ngx_http_lua_lightudata_mask( + balancer_keepalive_pools_table_key)); + lua_rawget(L, LUA_REGISTRYINDEX); /* pools? */ - -- if (bp->sockaddr && bp->socklen) { -- bp->last_peer_state = (int) state; ++ + if (lua_isnil(L, -1)) { + lua_pop(L, 1); /* orig stack */ - -- if (pc->tries) { -- pc->tries--; ++ + /* create upstream connection pools table */ + lua_createtable(L, 0, 0); /* pools */ + lua_pushlightuserdata(L, ngx_http_lua_lightudata_mask( @@ -793,15 +829,17 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- + + 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", @@ -872,20 +910,18 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- + + if (ngx_handle_read_event(c->read, 0) != NGX_OK) { + goto close; - } - - return; - } - -- /* fallback */ ++ } ++ ++ return; ++ } ++ +close: + + item = c->data; + c->log = ev->log; + + ngx_http_lua_balancer_close(c); - -- ngx_http_upstream_free_round_robin_peer(pc, data, state); ++ + ngx_queue_remove(&item->queue); + ngx_queue_insert_head(&item->cpool->free, &item->queue); + @@ -893,41 +929,41 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- + ngx_http_lua_balancer_free_keepalive_pool(ev->log, item->cpool); + } } - - -@@ -431,12 +928,12 @@ + + +@@ -431,12 +941,12 @@ ngx_http_lua_balancer_set_session(ngx_peer_connection_t *pc, void *data) { ngx_http_lua_balancer_peer_data_t *bp = data; - + - if (bp->sockaddr && bp->socklen) { + if (ngx_http_lua_balancer_peer_set(bp)) { /* TODO */ return NGX_OK; } - + - return ngx_http_upstream_set_round_robin_peer_session(pc, &bp->rrp); + return bp->original_set_session(pc, bp->data); } - - -@@ -445,13 +942,12 @@ + + +@@ -445,13 +955,12 @@ ngx_http_lua_balancer_save_session(ngx_peer_connection_t *pc, void *data) { ngx_http_lua_balancer_peer_data_t *bp = data; - + - if (bp->sockaddr && bp->socklen) { + if (ngx_http_lua_balancer_peer_set(bp)) { /* TODO */ return; } - + - ngx_http_upstream_save_round_robin_peer_session(pc, &bp->rrp); - return; + bp->original_save_session(pc, bp->data); } - + #endif -@@ -459,14 +955,14 @@ - +@@ -459,14 +968,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, - const u_char *addr, size_t addr_len, int port, char **err) @@ -945,13 +981,13 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- + ngx_http_upstream_t *u; + ngx_http_lua_ctx_t *ctx; + ngx_http_lua_balancer_peer_data_t *bp; - + if (r == NULL) { *err = "no request found"; -@@ -491,18 +987,6 @@ +@@ -491,18 +1000,6 @@ ngx_http_lua_ffi_balancer_set_current_peer(ngx_http_request_t *r, return NGX_ERROR; } - + - lmcf = ngx_http_get_module_main_conf(r, ngx_http_lua_module); - - /* we cannot read r->upstream->peer.data here directly because @@ -965,21 +1001,21 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- - } - ngx_memzero(&url, sizeof(ngx_url_t)); - + url.url.data = ngx_palloc(r->pool, addr_len); -@@ -526,6 +1010,8 @@ +@@ -526,6 +1023,8 @@ ngx_http_lua_ffi_balancer_set_current_peer(ngx_http_request_t *r, return NGX_ERROR; } - + + bp = (ngx_http_lua_balancer_peer_data_t *) u->peer.data; + if (url.addrs && url.addrs[0].sockaddr) { bp->sockaddr = url.addrs[0].sockaddr; bp->socklen = url.addrs[0].socklen; -@@ -536,6 +1022,72 @@ +@@ -536,6 +1035,72 @@ ngx_http_lua_ffi_balancer_set_current_peer(ngx_http_request_t *r, return NGX_ERROR; } - + + if (cpool_name_len == 0) { + bp->cpool_name = *bp->host; + @@ -1048,8 +1084,8 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- + return NGX_OK; } - -@@ -545,14 +1097,13 @@ + +@@ -545,14 +1110,13 @@ ngx_http_lua_ffi_balancer_set_timeouts(ngx_http_request_t *r, long connect_timeout, long send_timeout, long read_timeout, char **err) { @@ -1057,20 +1093,20 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- - ngx_http_upstream_t *u; + ngx_http_lua_ctx_t *ctx; + ngx_http_upstream_t *u; - + #if !(HAVE_NGX_UPSTREAM_TIMEOUT_FIELDS) ngx_http_upstream_conf_t *ucf; -#endif - ngx_http_lua_main_conf_t *lmcf; ngx_http_lua_balancer_peer_data_t *bp; +#endif - + if (r == NULL) { *err = "no request found"; -@@ -577,15 +1128,9 @@ +@@ -577,15 +1141,9 @@ ngx_http_lua_ffi_balancer_set_timeouts(ngx_http_request_t *r, return NGX_ERROR; } - + - lmcf = ngx_http_get_module_main_conf(r, ngx_http_lua_module); - - bp = lmcf->balancer_peer_data; @@ -1085,7 +1121,7 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- if (!bp->cloned_upstream_conf) { /* we clone the upstream conf for the current request so that * we do not affect other requests at all. */ -@@ -640,12 +1185,10 @@ +@@ -640,12 +1198,10 @@ ngx_http_lua_ffi_balancer_set_more_tries(ngx_http_request_t *r, int count, char **err) { #if (nginx_version >= 1007005) @@ -1099,12 +1135,12 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- + ngx_http_lua_ctx_t *ctx; + ngx_http_upstream_t *u; ngx_http_lua_balancer_peer_data_t *bp; - + if (r == NULL) { -@@ -671,13 +1214,7 @@ +@@ -671,13 +1227,7 @@ ngx_http_lua_ffi_balancer_set_more_tries(ngx_http_request_t *r, return NGX_ERROR; } - + - lmcf = ngx_http_get_module_main_conf(r, ngx_http_lua_module); - - bp = lmcf->balancer_peer_data; @@ -1113,10 +1149,10 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- - return NGX_ERROR; - } + bp = (ngx_http_lua_balancer_peer_data_t *) u->peer.data; - + #if (nginx_version >= 1007005) max_tries = r->upstream->conf->next_upstream_tries; -@@ -703,12 +1240,10 @@ +@@ -703,12 +1253,10 @@ int ngx_http_lua_ffi_balancer_get_last_failure(ngx_http_request_t *r, int *status, char **err) { @@ -1129,13 +1165,13 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- + ngx_http_upstream_state_t *state; ngx_http_lua_balancer_peer_data_t *bp; - ngx_http_lua_main_conf_t *lmcf; - + if (r == NULL) { *err = "no request found"; -@@ -733,13 +1268,7 @@ +@@ -733,13 +1281,7 @@ ngx_http_lua_ffi_balancer_get_last_failure(ngx_http_request_t *r, return NGX_ERROR; } - + - lmcf = ngx_http_get_module_main_conf(r, ngx_http_lua_module); - - bp = lmcf->balancer_peer_data; @@ -1144,16 +1180,17 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_balancer.c b/bundle/ngx_lua- - return NGX_ERROR; - } + bp = (ngx_http_lua_balancer_peer_data_t *) u->peer.data; - + if (r->upstream_states && r->upstream_states->nelts > 1) { state = r->upstream_states->elts; -diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_common.h b/bundle/ngx_lua-0.10.21/src/ngx_http_lua_common.h ---- a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_common.h 2022-12-02 10:58:50.050203715 +0800 -+++ b/bundle/ngx_lua-0.10.21/src/ngx_http_lua_common.h 2022-12-05 07:01:11.798290942 +0800 -@@ -240,13 +240,6 @@ +diff --git a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_common.h b/bundle/ngx_lua-0.10.21/src/ngx_http_lua_common.h +index ed88f0a..97d1942 100644 +--- a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_common.h ++++ b/bundle/ngx_lua-0.10.21/src/ngx_http_lua_common.h +@@ -240,13 +240,6 @@ struct ngx_http_lua_main_conf_s { ngx_http_lua_main_conf_handler_pt exit_worker_handler; ngx_str_t exit_worker_src; - + - ngx_http_lua_balancer_peer_data_t *balancer_peer_data; - /* neither yielding nor recursion is possible in - * balancer_by_lua*, so there cannot be any races among @@ -1164,9 +1201,9 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_common.h b/bundle/ngx_lua-0. ngx_chain_t *body_filter_chain; /* neither yielding nor recursion is possible in * body_filter_by_lua*, so there cannot be any races among -@@ -328,6 +321,10 @@ +@@ -328,6 +321,10 @@ union ngx_http_lua_srv_conf_u { #endif - + struct { + ngx_http_upstream_init_pt original_init_upstream; + ngx_http_upstream_init_peer_pt original_init_peer; @@ -1175,10 +1212,11 @@ diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_common.h b/bundle/ngx_lua-0. ngx_http_lua_srv_conf_handler_pt handler; ngx_str_t src; u_char *src_key; -diff -ruN a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_module.c b/bundle/ngx_lua-0.10.21/src/ngx_http_lua_module.c ---- a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_module.c 2022-12-02 10:58:50.050203715 +0800 -+++ b/bundle/ngx_lua-0.10.21/src/ngx_http_lua_module.c 2022-12-05 18:22:15.351308080 +0800 -@@ -1117,6 +1117,9 @@ +diff --git a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_module.c b/bundle/ngx_lua-0.10.21/src/ngx_http_lua_module.c +index fbeba12..49944c3 100644 +--- a/bundle/ngx_lua-0.10.21/src/ngx_http_lua_module.c ++++ b/bundle/ngx_lua-0.10.21/src/ngx_http_lua_module.c +@@ -1117,6 +1117,9 @@ ngx_http_lua_create_srv_conf(ngx_conf_t *cf) * lscf->srv.ssl_session_fetch_src = { 0, NULL }; * lscf->srv.ssl_session_fetch_src_key = NULL; * diff --git a/changelog/unreleased/kong/balancer_respect_max_retries.yml b/changelog/unreleased/kong/balancer_respect_max_retries.yml new file mode 100644 index 00000000000..1884ad1ce9f --- /dev/null +++ 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 diff --git a/spec/02-integration/05-proxy/10-balancer/08-retries_spec.lua b/spec/02-integration/05-proxy/10-balancer/08-retries_spec.lua new file mode 100644 index 00000000000..b3245055dfe --- /dev/null +++ b/spec/02-integration/05-proxy/10-balancer/08-retries_spec.lua @@ -0,0 +1,128 @@ +local helpers = require "spec.helpers" +local cjson = require "cjson" + +local function get_log(typ, n) + local entries + helpers.wait_until(function() + local client = assert(helpers.http_client(helpers.mock_upstream_host, + helpers.mock_upstream_port)) + local res = client:get("/read_log/" .. typ, { + headers = { + Accept = "application/json" + } + }) + local raw = assert.res_status(200, res) + local body = cjson.decode(raw) + + entries = body.entries + return #entries > 0 + end, 10) + if n then + assert(#entries == n, "expected " .. n .. " log entries, but got " .. #entries) + end + return entries +end + +for _, strategy in helpers.each_strategy() do + describe("Balancer: respect max retries [#" .. strategy .. "]", function() + local service + + lazy_setup(function() + local bp = helpers.get_db_utils(strategy, { + "routes", + "services", + "plugins", + }) + + service = bp.services:insert { + name = "retry_service", + host = "127.0.0.1", + port = 62351, + retries = 5, + } + + local route = bp.routes:insert { + service = service, + paths = { "/hello" }, + strip_path = false, + } + + bp.plugins:insert { + route = { id = route.id }, + name = "http-log", + config = { + queue = { + max_batch_size = 1, + max_coalescing_delay = 0.1, + }, + http_endpoint = "http://" .. helpers.mock_upstream_host + .. ":" + .. helpers.mock_upstream_port + .. "/post_log/http" + } + } + + local fixtures = { + http_mock = {} + } + + fixtures.http_mock.my_server_block = [[ + server { + listen 0.0.0.0:62351; + location /hello { + content_by_lua_block { + local request_counter = ngx.shared.request_counter + local first_request = request_counter:get("first_request") + if first_request == nil then + request_counter:set("first_request", "yes") + ngx.say("hello") + else + ngx.exit(ngx.HTTP_CLOSE) + end + } + } + } + ]] + + assert(helpers.start_kong({ + database = strategy, + nginx_conf = "spec/fixtures/custom_nginx.template", + nginx_http_lua_shared_dict = "request_counter 1m", + }, nil, nil, fixtures)) + end) + + lazy_teardown(function() + helpers.stop_kong() + end) + + it("exceeded limit", function() + -- First request should succeed and save connection to upstream in keepalive pool + local proxy_client1 = helpers.proxy_client() + local res = assert(proxy_client1:send { + method = "GET", + path = "/hello", + }) + + assert.res_status(200, res) + + proxy_client1:close() + + -- Second request should failed 1 times and retry 5 times and then return 502 + local proxy_client2 = helpers.proxy_client() + + res = assert(proxy_client2:send { + method = "GET", + path = "/hello", + }) + + assert.res_status(502, res) + + -- wait for the http-log plugin to flush the log + ngx.sleep(1) + + local entries = get_log("http", 2) + assert.equal(#entries[2].tries, 6) + assert.equal(entries[2].upstream_status, "502, 502, 502, 502, 502, 502") + end) + end) +end