AlinsRan commented on code in PR #13629:
URL: https://github.com/apache/apisix/pull/13629#discussion_r3540516233
##########
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:
Trimmed the narrative on the reuse/rebuild comment blocks; kept the ordering
constraints (delayed_clear before create_checker, remove-before-add).
##########
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:
Renamed to "on a version-only change".
--
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]
