fix(balancer): respect max retries #12346

tzssangglass · 2024-01-15T05:39:48Z

Summary

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.

Checklist

The Pull Request has tests
A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

Fix FTI-5616

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>

tzssangglass · 2024-01-17T06:20:04Z

The previously reviewed PR #12242 had some issues with the latest CI configuration file, so I pulled out a new branch from the master and ported it to this PR.

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>

This reverts commit bc80125.

build/openresty/patches/ngx_lua-0.10.25_01-dyn_upstream_keepalive.patch

…d when handling cached connection errors address comments of #12346 Signed-off-by: tzssangglass <tzssangglass@gmail.com>

…d when handling cached connection errors (#12468) * fix(balancer): ensure the `notify` callback is invoked only if defined when handling cached connection errors address comments of #12346 Signed-off-by: tzssangglass <tzssangglass@gmail.com> * fix Signed-off-by: tzssangglass <tzssangglass@gmail.com> --------- Signed-off-by: tzssangglass <tzssangglass@gmail.com>

…d when handling cached connection errors (#12468) * fix(balancer): ensure the `notify` callback is invoked only if defined when handling cached connection errors address comments of #12346 Signed-off-by: tzssangglass <tzssangglass@gmail.com> * fix Signed-off-by: tzssangglass <tzssangglass@gmail.com> --------- Signed-off-by: tzssangglass <tzssangglass@gmail.com> (cherry picked from commit 415ca0f)

* chore(patches): revert the "respect max retries" patch We have discovered potential segfault risk with the feature and we do not have enough time to review this in more depth, therefore we have decided to revert the change temporarily to further investigate. This reverts PR #12346. FTI-5616

* chore(patches): revert the "respect max retries" patch We have discovered potential segfault risk with the feature and we do not have enough time to review this in more depth, therefore we have decided to revert the change temporarily to further investigate. This reverts PR #12346. FTI-5616 (cherry picked from commit 99a9aa2)

…when handling cached connection errors address comments of Kong#12346 Signed-off-by: tzssangglass <tzssangglass@gmail.com>

This reverts commit 2c7d1d6.

This reverts commit 5160fee.

This reverts commit b0889c3.

This reverts commit 5160fee.

This reverts commit 2c7d1d6.

This reverts commit b0889c3.

This reverts commit aa7c405.

pull-request-size bot added the size/L label Jan 15, 2024

github-actions bot assigned tzssangglass Jan 15, 2024

github-actions bot added the build/bazel label Jan 15, 2024

tzssangglass mentioned this pull request Jan 15, 2024

fix(balancer): respect max retries #12242

Closed

3 tasks

tzssangglass marked this pull request as ready for review January 15, 2024 06:22

tzssangglass requested review from fffonion, oowl and ms2008 January 15, 2024 06:24

oowl approved these changes Jan 15, 2024

View reviewed changes

tzssangglass requested review from catbro666 and oowl January 15, 2024 06:24

ms2008 approved these changes Jan 15, 2024

View reviewed changes

catbro666 approved these changes Jan 18, 2024

View reviewed changes

windmgc merged commit ad62d3b into master Jan 18, 2024
30 checks passed

windmgc deleted the FTI-5616-2 branch January 18, 2024 06:09

chronolaw added a commit that referenced this pull request Jan 18, 2024

update dyn_upstream_keepalive.patch for #12346

5f12b3c

chronolaw added cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee and removed cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Jan 18, 2024

Kong deleted a comment from team-gateway-bot Jan 18, 2024

tzssangglass mentioned this pull request Jan 19, 2024

[backport -> release/2.8.x] fix(balancer): respect max retries (#12346) #12371

Merged

3 tasks

tzssangglass mentioned this pull request Jan 19, 2024

[backport -> release/3.2.x] fix(balancer): respect max retries (#12346) #12372

Merged

3 tasks

samugi added a commit that referenced this pull request Jan 24, 2024

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

3ba5bfe

This reverts commit bc80125.

kikito pushed a commit that referenced this pull request Jan 24, 2024

Revert "fix(balancer): respect max retries (#12346)" (#12411)

c8930f4

This reverts commit bc80125.

This was referenced Jan 24, 2024

fix(scripts): fix update-copyright in venv and remove unused repos #12413

Closed

fix(scripts): fix update-copyright in venv and remove unused repos #12414

Draft

chronolaw mentioned this pull request Jan 30, 2024

Random 502 error due to upstream keepalive #11160

Closed

1 task

chronolaw reviewed Jan 31, 2024

View reviewed changes

build/openresty/patches/ngx_lua-0.10.25_01-dyn_upstream_keepalive.patch Show resolved Hide resolved

chobits reviewed Jan 31, 2024

View reviewed changes

build/openresty/patches/ngx_lua-0.10.25_01-dyn_upstream_keepalive.patch Show resolved Hide resolved

tzssangglass added a commit that referenced this pull request Jan 31, 2024

fix(balancer): ensure the notify callback is invoked only if define…

667460f

…d when handling cached connection errors address comments of #12346 Signed-off-by: tzssangglass <tzssangglass@gmail.com>

tzssangglass mentioned this pull request Jan 31, 2024

fix(balancer): ensure the notify callback is invoked only if defined when handling cached connection errors #12468

Merged

3 tasks

team-gateway-bot mentioned this pull request Jan 31, 2024

[backport -> release/3.6.x] fix(balancer): ensure the notify callback is invoked only if defined when handling cached connection errors #12469

Merged

3 tasks

dndx mentioned this pull request Jan 31, 2024

chore(patches): revert the "respect max retries" patch #12470

Merged

team-gateway-bot mentioned this pull request Jan 31, 2024

[backport -> release/3.6.x] chore(patches): revert the "respect max retries" patch #12472

Merged

tzssangglass mentioned this pull request Feb 1, 2024

fix(balancer): ensure the notify callback is invoked only if defined when handling cached connection errors #12480

Closed

3 tasks

tzssangglass added a commit that referenced this pull request Feb 2, 2024

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

ad8df5e

This reverts commit 2c7d1d6.

tzssangglass added a commit that referenced this pull request Feb 2, 2024

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

1e2bbce

This reverts commit 5160fee.

tzssangglass added a commit that referenced this pull request Feb 2, 2024

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

168f6fa

This reverts commit b0889c3.

bungle pushed a commit that referenced this pull request Feb 2, 2024

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

688b9c1

This reverts commit 5160fee.

bungle pushed a commit that referenced this pull request Feb 2, 2024

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

905637c

This reverts commit 2c7d1d6.

bungle pushed a commit that referenced this pull request Feb 2, 2024

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

d411f63

This reverts commit b0889c3.

AndyZhang0707 added a commit that referenced this pull request Jul 18, 2024

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

a33ea19

This reverts commit aa7c405.

AndyZhang0707 added a commit that referenced this pull request Jul 26, 2024

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

219cd2a

This reverts commit aa7c405.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(balancer): respect max retries #12346

fix(balancer): respect max retries #12346

tzssangglass commented Jan 15, 2024

tzssangglass commented Jan 17, 2024

fix(balancer): respect max retries #12346

fix(balancer): respect max retries #12346

Conversation

tzssangglass commented Jan 15, 2024

Summary

Checklist

Issue reference

tzssangglass commented Jan 17, 2024