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.

Sign up for GitHub

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

Jump to bottom

fix: fix delay clean logic when multiple healthchecker was started #146

Merged
merged 7 commits into from
Dec 22, 2023
Merged

fix: fix delay clean logic when multiple healthchecker was started #146

merged 7 commits into from
Dec 22, 2023

Conversation

oowl
Copy link
Member

@oowl oowl commented Dec 14, 2023 •
edited
Loading

When we have many healthchecker instance, if we need to delay clear anyone, it will add all delay_purge number to these target table, And then wait active healthcheck timer to check these number flag to execute real clear logic. Ideally, it's work expected, but we have one for loop bug in clear logic. which will case delay clear logic just execute once for first healthchecker instance. So this will cause delay clear can not remove some target expectedly.

https://github.com/Kong/lua-resty-healthcheck/blob/master/lib/resty/healthcheck.lua#L1725

image

FTI-5478

@oowl oowl mentioned this pull request Dec 15, 2023
Merged
@oowl
Copy link
Member Author

oowl commented Dec 15, 2023

It seems openresty has been remove old package from you deb package managment source, So i remove openresty 1.19 test

vagrant@ubuntu-focal:~$ cat /etc/os-release
NAME="Ubuntu"
VERSION="20.04.6 LTS (Focal Fossa)"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 20.04.6 LTS"
VERSION_ID="20.04"
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
VERSION_CODENAME=focal
UBUNTU_CODENAME=focal
vagrant@ubuntu-focal:~$ apt-cache madison openresty
 openresty | 1.21.4.3-1~focal1 | http://openresty.org/package/ubuntu focal/main amd64 Packages
 openresty | 1.21.4.2-1~focal1 | http://openresty.org/package/ubuntu focal/main amd64 Packages
 openresty | 1.21.4.1-1~focal1 | http://openresty.org/package/ubuntu focal/main amd64 Packages

@oowl oowl requested a review from locao December 15, 2023 08:35
locao
locao requested changes Dec 15, 2023
View reviewed changes
Copy link
Contributor

@locao locao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some questions before moving forward with this change.

lib/resty/healthcheck.lua Outdated Show resolved Hide resolved
t/with_worker-events/11-clear.t Show resolved Hide resolved
.github/workflows/build_and_test_with_worker_events.yml Show resolved Hide resolved
locao
locao previously approved these changes Dec 18, 2023
View reviewed changes
Copy link
Contributor

@locao locao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@ms2008
Copy link

ms2008 commented Dec 21, 2023

@oowl Concerns from #146 (comment) will it happen?

@oowl
Copy link
Member Author

oowl commented Dec 21, 2023

@oowl Concerns from #146 (comment) will it happen?

Yeah, It will happen in some cases, you can run the newly added test in the master branch code.

@oowl oowl requested review from locao and outsinre December 21, 2023 08:38
windmgc
windmgc approved these changes Dec 22, 2023
View reviewed changes
Copy link
Member

@windmgc windmgc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code change LGTM

@oowl oowl merged commit 94c3bba into master Dec 22, 2023
6 checks passed
@fffonion fffonion deleted the fix-active-healthcheck-clean-logic branch December 22, 2023 07:54
@oowl oowl mentioned this pull request Dec 22, 2023
Merged
windmgc pushed a commit to Kong/kong that referenced this pull request Dec 25, 2023
@oowl
chore(deps): bump lua-resty-healthcheck to 3.0.1 (#12237)
b2a4ffd 
Kong/lua-resty-healthcheck#146

FTI-5478
@oowl oowl mentioned this pull request Dec 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcdullbloom mcdullbloom mcdullbloom left review comments

@windmgc windmgc windmgc approved these changes

@ms2008 ms2008 ms2008 approved these changes

@outsinre outsinre outsinre approved these changes

@locao locao Awaiting requested review from locao

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@oowl @ms2008 @windmgc @locao @outsinre @mcdullbloom