Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(balancer): respect max retries #12242

Closed
wants to merge 9 commits into from

Conversation

tzssangglass
Copy link
Member

@tzssangglass tzssangglass commented Dec 25, 2023

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

@fffonion
Copy link
Contributor

fffonion commented Jan 2, 2024

review companion: https://github.com/Kong/openresty-patches-review/pull/1/files

@tzssangglass
Copy link
Member Author

@tzssangglass
Copy link
Member Author

Openresty patches review companion may not related to this pr and cc @fffonion

@fffonion
Copy link
Contributor

fffonion commented Jan 9, 2024

a rebase will now solve the failing openresty patches companion CI @tzssangglass

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>
Signed-off-by: tzssangglass <tzssangglass@gmail.com>
Signed-off-by: tzssangglass <tzssangglass@gmail.com>
Signed-off-by: tzssangglass <tzssangglass@gmail.com>
Signed-off-by: tzssangglass <tzssangglass@gmail.com>
Signed-off-by: tzssangglass <tzssangglass@gmail.com>
Signed-off-by: tzssangglass <tzssangglass@gmail.com>
Signed-off-by: tzssangglass <tzssangglass@gmail.com>
Signed-off-by: tzssangglass <tzssangglass@gmail.com>
@tzssangglass
Copy link
Member Author

Due to some issues, I am unable to continue working on this PR until it is merged, so I have created a new PR #12346.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build/bazel chore Not part of the core functionality of kong, but still needed size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants