fix(healthcheck) record last_run when healthcheck is scheduled rather than completed
#72
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Jun 1, 2021
|
Reopening to kick off Travis again. Running OpenResty |
Prevents a thundering herd issue whereby additional healthchecks are scheduled in the time in which it takes the healthcheck to complete.
onematchfox
force-pushed
the
fix/healthcheck-thundering-herd
branch
from
08cf81d
to
72c5443
Compare
|
Hi @onematchfox! Thank you for your submission and detailed explanation about the issue! I'm adding this to our tasks, so your PR will be reviewed in the next few days. |
ghost
mentioned this pull request
|
I've decided to write a small regression test case. The test isn't perfect but should do. If you run these tests against the branch |
nowNick
added a commit
that referenced
this pull request
Nov 8, 2023
This is a squashed commit that realigns master branch with 3.0.0 release. In order to do so the master branch was reverted back to 1.3.0 release (commit: dc2a6b6) and then the 3.0.0 release branch was merged to it (up to commit: a2bec67). Below you can see all the details of the squashed commits. --------- * release 1.4.0 * fix(healthcheck) use single timer for all active checks (#62) * fix(healthcheck) use single timer for all active checks * tests(*) removed tests that are not needed * docs(*) docs for release 1.4.0 * chore(ci) use newer openresty and luarocks releases (#68) * fix(healthcheck) single worker actively checks the status (#67) * release 1.4.1 * fix(healthcheck) record `last_run` when healthcheck is scheduled (#72) Prevents a thundering herd issue whereby additional healthchecks are scheduled in the time in which it takes the healthcheck to complete. * tests(active-probes) interval is respected (#73) * fix(healthcheck) record `last_run` when healthcheck is scheduled Prevents a thundering herd issue whereby additional healthchecks are scheduled in the time in which it takes the healthcheck to complete. * tests(active-probes) interval is respected Co-authored-by: Brian Fox <brianhfox@gmail.com> * fix(healthcheck) remove event watcher when stopping hc (#74) Co-authored-by: Brian Fox <brianhfox@gmail.com> Co-authored-by: Brian Fox <brianhfox@gmail.com> * tests(*) avoid some flakiness (#75) * release 1.4.2 * chore(*) add GitHub Actions workflows (#82) * chore(*) add GitHub Actions workflows * fix(healthcheck) lint error * Simplify start of the checking timer (#85) * simplify start of the checking timer, ensuring only one worker actively sends healthchecks. one timer per worker, but before doing anything, tries to acquire an expiration lock. if fails, try again later. if the "winning" worker ever fails to renew it, some other worker would get it. * chore(rockspec) added rockspec for release 1.5.0-1 Also: - updated scm-1 rockspec - bumped openresty version in CI tests * feat(*) add header support for active checks * feat(active) support map headers * feat(healthcheck) delayed_clear function (#88) Added new function delayed_clear. This function marks all targets to be removed, but do not actually remove them. If before the delay parameter any of them is re-added, it is unmarked for removal. This function makes it possible to keep target state during config changes, where the targets might be removed and then re-added. * chore(readme) 1.5.0 release (#91) * chore(readme) 1.5.0 release * docs(*) release 1.5.0 Also added docs missing to delayed_clear() function. * fix(healthcheck) Use pair instead ipair for hcs weak table (#93) * release 1.5.1 (#95) * chore(readme) update badges (#98) * docs(readme) updated with 1.4.x changes * chore(workflows) updates for 1.6.0 release - added latest openresty to the CI matrix - added tests for when lua-resty-worker-events or lua-resty-events are used * feat(healthcheck) support setting the events module (#105) * feat(healthcheck) support setting the events module * fix(healthcheck) defaults to lua-resty-worker-events * tests(workflows) fixed manual deps install * fix(healthcheck) check empty opts * chore(workflows) use last luarocks * test(workflows) use pre-built deps, test with or 1.13-1.21 * chore(workflows) install lua-resty-events in ci * tests(workflows) debug * fixed tests and resty-events usage * init resty-events in init_worker * fix(tests) init events module (#107) * add init_worker in 03-get_target_status.t * fix 03-get_target_status.t * fix 03-get_target_status_with_sleeps.t * fix 04-report_success.t * fix 05/06 * fix 07/08 * fix 09 * change 10 * fix 11 * fix 12 * change 13 * fix 15 * partial fix 16 * change 17 * fix 18 * change 13 * fix 16 * style 05 * fix 01/02 * use string.buffer in OpenResty 1.21.4.1 (#109) * use string.buffer in OpenResty 1.21.4.1 * remove cjson require * fix(healthcheck) use the events module set in defaults * tests(with_resty-events) disabled tests that need more work * fix(healthcheck) avoid breaking when opts are nil * tests(with_resty-events) removed unnecessary test * tests(with_resty-events) increased sleeps Co-authored-by: Chrono <chrono_cpp@me.com> * release 1.6.0 (#110) * docs(readme) release 1.6.0 * fix(rockspec) typo * chore(rockspec) release 1.6.0 * docs(*) release 1.6.0 * chore(*) localize string.format (#111) * fix(healthcheck) support any lua-resty-events 0.1.x (#118) * chore(workflows) bump deps versions * chore(helathcheck) support any lua-resty-events 0.1.x * fix(healthchecker) port 2.x lock fixes to 1.5.x (#113) * fix(healthchecker) port 2.x lock fixes to 1.5.x * chore(healthcheck) remove unused vars * chore(healthcheck) fix indent level * fix(healthcheck) correct duplicate handling in add_target * fix(healthchecker) handle fetch_target_list failure in checker callback * chore(healthcheck) apply suggestions from #112 Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com> * chore(healthcheck) increase verbosity for locked function failures (#114) * chore(healthcheck) increase verbosity for locked function failures * tests(healthcheck) add tests for run_locked() * fix(healthcheck) lower the cleanup check frequency the health-check timer also checks if targets must be removed. to safely remove targets, the targets list is locked. if this check runs on every health-check cycle and there are a large number of targets, a bazillion locks will be created. this change avoids that by lowering the frequency the cleanup list is checked. the side-effect is that targets marked for cleanup may exist for more time (2.5s) than expected, and some unexpected active checks could happen. * tests(clear) increase delay for delayed clear tests with less locks the wait for delayed clean is longer. * docs(readme) release 1.6.1 * chore(rockspecs) release 1.6.1 * release 1.6.1 * docs(readme) updated build badge * chore(ci) remove old openresty versions * feat(healthcheck) avoid duplication post in rebuild healthcheck scenario * release 1.6.2 * Added support for https_sni in healthcheck.lua (#49) * fix(mtls) use OpenResty's API for mtls (#99) * chore(ci): fix cache path (#136) ${{ env.* }} is not evaluated in `with` causing gha tries to cache `/`. * release 1.6.3 (#135) * release 3.0.0 (#142) * feat(ci/KAG-1800): add lint and sast workflows using shared actions * chore(ci): pin shared code quality actions * chore(*): backport - localize some functions A commit on master 80ee2e1 introduced localizing some functions. This commit backports that one. Backports: #92 * fix(healthcheck): fixed incorrect default http_statuses when new() was called multiple times (#83) * chore(lint): bump kong/public-shared-actions * docs(README): added 1.5.2 and 1.5.3 releases * chore(*) rename readme, add release instructions * chore(healthcheck): fix get_defaults function * fix(test): fix worker-events test * release 3.0.0 * chore(github): cancel in progress workflows when new pushed --------- Co-authored-by: saisatish karra <saisatish.karra@konghq.com> Co-authored-by: Shuoqing Ding <dsq704136@gmail.com> Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com> Co-authored-by: Thijs Schreijer <thijs@thijsschreijer.nl> * chore(*): revert commits back to 1.3.0 This reverts the master branch backs to the commit of dc2a6b6 so that we can skip over 2.0.0 release. The 1.3.0 release is the first common commit between master branch and 1.6.x (also 3.0.x) branches. * chore(docs): fix semgrep https warnings * docs(readme): update shield badges Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com> * chore(*): add 2.0.0 rockspecs and fix tests Release 2.0.x introduced some rockspecs with fixes. Reverting back to 1.3.0 and reapplying changes from 3.0.0 reversed those fixes. This commit reintroduces them. KAG-2704 --------- Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com> Co-authored-by: Brian Fox <brianhfox@gmail.com> Co-authored-by: Murillo Paula <murillo@murillopaula.com> Co-authored-by: Javier <javier.guerra@konghq.com> Co-authored-by: Thijs Schreijer <thijs@thijsschreijer.nl> Co-authored-by: Mayo <i@shoujo.io> Co-authored-by: Tomasz Nowak <tomanowa@gmail.com> Co-authored-by: Chrono <chrono_cpp@me.com> Co-authored-by: Michael Martin <flrgh@protonmail.com> Co-authored-by: Jun Ouyang <ouyangjun1999@gmail.com> Co-authored-by: HansK-p <42314815+HansK-p@users.noreply.github.com> Co-authored-by: Qi <call_far@outlook.com> Co-authored-by: Wangchong Zhou <fffonion@gmail.com> Co-authored-by: Aapo Talvensaari <aapo.talvensaari@gmail.com> Co-authored-by: saisatish karra <saisatish.karra@konghq.com> Co-authored-by: Shuoqing Ding <dsq704136@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
active_check_timerin this library is currently executed every0.1seconds.lua-resty-healthcheck/lib/resty/healthcheck.lua
Lines 1470 to 1493 in 9e9a936
This loop checks the value of
last_runto determine whether or not to schedule a healthcheck. However,last_runis only set once the check completes:lua-resty-healthcheck/lib/resty/healthcheck.lua
Lines 1031 to 1041 in 9e9a936
In the case that the healthcheck for a set of targets takes longer than
0.1seconds to complete, this results in additional healthchecks being scheduled rather than waiting for the configuredintervalto pass. This can cause multiple issues and even lead to a thundering herd style problem on upstreams that are having issues. To begin with, ifunhealthy failuresare set to a number less than the total number of checks that are scheduled in the period it takes to complete a healthcheck then a target will be marked asUNHEALTHYimmediately. E.g. I have a timeout of 1 second set, if an upstream broaches that time because of some network glitch then I could potentially have10healthcheck scheduled in the period in which it take the first to complete. If myUnhealthy timeouts/failureshappens to be 5 then my target could quite easily be marked asUNHEALTHYafter what should have been a single healthcheck. The problem is subsequently compounded when running a cluster of Kong nodes, particularly if one checks unhealthy targets more often that healthy (because one wants to mark the upstream/target as healthy quickly as possible). In my case this prevented an upstream from coming back up again.Supersedes #70 (resolves that issue in addition to the one described above).
Resolves Kong/kong#7409
Logs: