nic-6443 commented on code in PR #13629:
URL: https://github.com/apache/apisix/pull/13629#discussion_r3533915750
##########
apisix/healthcheck_manager.lua:
##########
@@ -284,14 +484,56 @@ local function timer_working_pool_check()
" current version: ", current_ver, " item version:
", item.version)
if item.version == current_ver then
need_destroy = false
+ elseif upstream.checks and upstream.nodes and #upstream.nodes
> 0
Review Comment:
There's still a nil window left on a checks-config change: this timer and
`timer_create_checker` both run every 1s and their relative order isn't
guaranteed. When this timer observes the checks change first, `deep_eq` fails
and we fall into the destroy path below (`dead` + `stop` +
`working_pool[resource_path] = nil`), only enqueueing a rebuild — so until the
next `timer_create_checker` tick, `fetch_checker` returns nil and health
filtering is bypassed. That's the same failure mode this PR fixes for node-only
changes, just narrowed to checks changes (the PR description's "it destroys
only when the resource is gone or the node count drops to 0" doesn't match this
branch either).
I think this branch can simply drop the `deep_eq` condition: keep the
checker alive whenever `upstream.checks` exists and `#upstream.nodes > 0`, and
let `timer_create_checker` decide reuse vs rebuild — its rebuild path already
does the atomic publish-new-then-stop-old swap. That closes the window and lets
you delete the `has_live_replacement`/`replacement_ver` bookkeeping entirely,
since the destroy path would then only run when no replacement can exist (where
`delayed_clear` is always correct).
It also fixes a smaller edge on a zero-traffic worker: destroy + enqueue v1
here, then a second checks change to v2 lands before `timer_create_checker`
drains → the waiting entry is dropped on the version mismatch, `working_pool`
is already empty so nothing re-enqueues, and the shm targets are left neither
probed nor purged until traffic resumes. With the checker kept in
`working_pool`, this self-heals on the next tick of this timer.
What do you think?
##########
apisix/healthcheck_manager.lua:
##########
@@ -209,22 +349,80 @@ local function timer_create_checker()
goto continue
end
- -- if a checker exists then delete it before creating a new one
+ -- No nodes means there is nothing to health-check. Don't build (or
+ -- rebuild into) an empty checker here; leave any teardown to
+ -- timer_working_pool_check, which destroys the checker when the
node
+ -- count drops to 0, so the two timers stay consistent.
+ if not upstream.nodes or #upstream.nodes == 0 then
+ goto continue
+ end
+
+ -- If a checker already exists and the `checks` config is unchanged
+ -- (only the upstream nodes changed), reconcile its targets in
place
+ -- instead of destroying and rebuilding it. A destroy-and-rebuild
+ -- leaves `up_checker == nil` for the rebuild window, during which
+ -- traffic is routed to nodes already known to be unhealthy, and it
+ -- throws away the checker's accumulated health state.
+ -- sync_checker_targets is the last condition so it only runs when
the
+ -- checker is reuse-eligible; if it reports a partial failure the
whole
+ -- guard is false and we fall through to a full rebuild below,
which
+ -- converges the upstream to the desired targets instead of
committing
+ -- the new version against a half-reconciled checker.
local existing_checker = working_pool[resource_path]
+ if existing_checker and existing_checker.checker
+ and not existing_checker.checker.dead
+ and upstream.checks
+ and upstream.nodes and #upstream.nodes > 0
Review Comment:
nit: `upstream.nodes and #upstream.nodes > 0` is already guaranteed by the
`goto continue` guard added above, so this condition can't be false here.
##########
apisix/healthcheck_manager.lua:
##########
@@ -209,22 +349,80 @@ local function timer_create_checker()
goto continue
end
- -- if a checker exists then delete it before creating a new one
+ -- No nodes means there is nothing to health-check. Don't build (or
+ -- rebuild into) an empty checker here; leave any teardown to
+ -- timer_working_pool_check, which destroys the checker when the
node
+ -- count drops to 0, so the two timers stay consistent.
+ if not upstream.nodes or #upstream.nodes == 0 then
+ goto continue
+ end
+
+ -- If a checker already exists and the `checks` config is unchanged
+ -- (only the upstream nodes changed), reconcile its targets in
place
+ -- instead of destroying and rebuilding it. A destroy-and-rebuild
+ -- leaves `up_checker == nil` for the rebuild window, during which
+ -- traffic is routed to nodes already known to be unhealthy, and it
+ -- throws away the checker's accumulated health state.
+ -- sync_checker_targets is the last condition so it only runs when
the
+ -- checker is reuse-eligible; if it reports a partial failure the
whole
+ -- guard is false and we fall through to a full rebuild below,
which
+ -- converges the upstream to the desired targets instead of
committing
+ -- the new version against a half-reconciled checker.
local existing_checker = working_pool[resource_path]
+ if existing_checker and existing_checker.checker
+ and not existing_checker.checker.dead
+ and upstream.checks
+ and upstream.nodes and #upstream.nodes > 0
+ and core.table.deep_eq(existing_checker.checks, upstream.checks)
+ and sync_checker_targets(existing_checker.checker, upstream)
then
+ add_working_pool(resource_path, resource_ver,
existing_checker.checker,
+ upstream.checks)
+ core.log.info("reused checker with incremental targets: ",
+ tostring(existing_checker.checker), " for
resource: ",
+ resource_path, " and version: ", resource_ver)
+ goto continue
+ end
+
+ -- The checks config changed (or no checker exists): rebuild the
+ -- checker. delayed_clear() MUST run before create_checker()
re-adds
+ -- the targets: the new checker shares the same shm target list,
and
+ -- add_target() only un-marks a target's purge_time when it is
re-added
+ -- *after* being marked. Clearing first lets surviving targets get
+ -- un-marked on re-add, while genuinely dropped targets keep their
+ -- purge_time and are cleaned up; clearing after create (the
reverse)
+ -- would leave the live checker's targets marked and purge them
later.
+ -- In this rebuild path the old checker is only stopped after the
new
+ -- one is published, so this path never leaves a nil/stopped
checker for
+ -- fetch_checker (the checks-change destroy in
timer_working_pool_check
+ -- is a separate path that enqueues a rebuild instead).
if existing_checker then
existing_checker.checker:delayed_clear(DELAYED_CLEAR_TIMEOUT)
- existing_checker.checker:stop()
- core.log.info("releasing existing checker: ",
tostring(existing_checker.checker),
- " for resource: ", resource_path, " and version:
",
- existing_checker.version)
end
local checker = create_checker(upstream)
if not checker then
+ -- create_checker failed (upstream healthcheck disabled or
+ -- healthcheck.new errored). The old checker's shm targets were
+ -- already delayed_clear'd above, so it can no longer
health-check
+ -- reliably; tear it down and drop it from the working pool
instead
+ -- of leaving a stopped/cleared checker that fetch_checker
would
+ -- still hand out (it only checks .dead).
+ if existing_checker then
+ existing_checker.checker.dead = true
Review Comment:
nit: worth a log line here — the old code logged "releasing existing
checker" on every teardown. When `create_checker` fails (e.g.
`disable_upstream_healthcheck` or a `healthcheck.new` error), the checker now
silently disappears from the working pool, which will be hard to trace from
logs.
##########
t/node/healthcheck-leak-bugfix.t:
##########
@@ -17,15 +17,16 @@
use t::APISIX 'no_plan';
repeat_each(1);
-log_level('warn');
+# the reuse path logs "reused checker with incremental targets" at info level
+log_level('info');
no_root_location();
no_shuffle();
run_tests();
__DATA__
-=== TEST 1: ensure the old check is cleared after configuration updated
+=== TEST 1: reuse the checker without clearing it when only the nodes change
Review Comment:
nit: the title says "when only the nodes change", but the test PUTs an
identical config — only `modifiedIndex` changes. Maybe "on a version-only
change" or "when the config content is unchanged".
##########
apisix/healthcheck_manager.lua:
##########
@@ -209,22 +349,80 @@ local function timer_create_checker()
goto continue
end
- -- if a checker exists then delete it before creating a new one
+ -- No nodes means there is nothing to health-check. Don't build (or
+ -- rebuild into) an empty checker here; leave any teardown to
+ -- timer_working_pool_check, which destroys the checker when the
node
+ -- count drops to 0, so the two timers stay consistent.
+ if not upstream.nodes or #upstream.nodes == 0 then
+ goto continue
+ end
+
+ -- If a checker already exists and the `checks` config is unchanged
+ -- (only the upstream nodes changed), reconcile its targets in
place
+ -- instead of destroying and rebuilding it. A destroy-and-rebuild
+ -- leaves `up_checker == nil` for the rebuild window, during which
+ -- traffic is routed to nodes already known to be unhealthy, and it
+ -- throws away the checker's accumulated health state.
+ -- sync_checker_targets is the last condition so it only runs when
the
+ -- checker is reuse-eligible; if it reports a partial failure the
whole
+ -- guard is false and we fall through to a full rebuild below,
which
+ -- converges the upstream to the desired targets instead of
committing
+ -- the new version against a half-reconciled checker.
local existing_checker = working_pool[resource_path]
+ if existing_checker and existing_checker.checker
+ and not existing_checker.checker.dead
+ and upstream.checks
+ and upstream.nodes and #upstream.nodes > 0
+ and core.table.deep_eq(existing_checker.checks, upstream.checks)
+ and sync_checker_targets(existing_checker.checker, upstream)
then
+ add_working_pool(resource_path, resource_ver,
existing_checker.checker,
+ upstream.checks)
+ core.log.info("reused checker with incremental targets: ",
+ tostring(existing_checker.checker), " for
resource: ",
+ resource_path, " and version: ", resource_ver)
+ goto continue
+ end
+
+ -- The checks config changed (or no checker exists): rebuild the
Review Comment:
nit (here and in a few other spots): some of the new comment blocks narrate
the issue background / review history rather than constraints the code can't
express. The ordering constraints themselves (`delayed_clear` before
`create_checker`, remove-before-add for the identity change) are definitely
worth keeping — I'd just trim the surrounding narrative so they stay readable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
