AlinsRan commented on code in PR #13629:
URL: https://github.com/apache/apisix/pull/13629#discussion_r3540515874
##########
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:
Fixed in 5c75c7b. timer_working_pool_check no longer destroys on a checks
change: whenever the upstream still defines checks and keeps at least one node,
it keeps the checker alive and enqueues the rebuild, so timer_create_checker
rebuilds it (building the new checker before stopping the old one) and
fetch_checker never returns nil. This also made the has_live_replacement gate
redundant, so I dropped it.
##########
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:
Removed.
##########
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:
Added a core.log.warn before dropping the checker from the working pool.
--
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]
