moonming commented on PR #13029: URL: https://github.com/apache/apisix/pull/13029#issuecomment-4849527976
Reopening to capture a better fix for the underlying problem. After tracing the code end-to-end, I don't think the choice is between `clear()` and `delayed_clear()` at all — both are wrong for different reasons. The real fix is to stop tearing the checker down on every node change and reconcile targets incrementally instead. ## 1. What actually happens today The checker is keyed by `resource_path` (`upstream#<resource_key>`), which is **stable across config versions**, so the old and new checker write to the **same shm** (`TARGET_LIST` plus the per-target state keys). On a k8s endpoint change (`_nodes_ver` bumps → version bumps), `timer_create_checker` does: ``` delayed_clear(10s) -- stamp every old target with purge_time = now + 10s stop() -- stop the old checker create_checker() -- build a brand-new checker, add_target() for the new node set ``` So **every node-set change tears the whole checker down and rebuilds it**, laundering health status through the shm: a surviving node gets re-added, sees `purge_time ~= nil`, and inherits its old status; a removed node is left with a `purge_time` and is purged ~10s later. This teardown/rebuild + delayed purge is the root of the stale window and its side effects. ## 2. Why `delayed_clear` exists — and why that reason is now weak `delayed_clear` came from #10312: an immediate `clear()` empties the target list, and concurrent in-flight requests that look up node health hit `target not found` → the node was treated as unavailable → the request failed. The 10s delay kept old targets alive until the new checker re-registered them. But that justification is largely gone today. In `fetch_node_status`, `target not found` is now explicitly treated as **unknown → usable (returns `true`)**: https://github.com/apache/apisix/blob/master/apisix/healthcheck_manager.lua#L116-L127 So the #10312 failure mode is already absorbed at the APISIX layer. The only thing `delayed_clear` still buys us is **preserving accumulated health status across the rebuild** — not preventing request failures. ## 3. The harder bug nobody flagged: IP reuse → stale health status A removed IP lingering in the shm for 10s is mostly harmless on its own — routing rebuilds its picker from the upstream `nodes` list, not from the healthcheck target list, so the departed IP is never selected and the residue self-cleans. The genuinely harmful case is **IP reuse**. If k8s reassigns a just-terminated pod's `ip:port` to a **new pod** within the 10s window (dense clusters, small pod CIDRs, hostNetwork), the new checker's `add_target(reused_ip, is_healthy = true)` finds the still-present old entry, revives it (`purge_time = nil`), and **inherits the previous pod's health status** for the new pod: https://github.com/api7/lua-resty-healthcheck/blob/master/lib/resty/healthcheck.lua#L503-L512 If the old pod was `unhealthy`, a brand-new healthy pod is starved of traffic; if it was `healthy`, traffic flows to a possibly-broken new pod until the next probe. Health status is indexed only by `ip:port:hostname`, and `delayed_clear` keeps that identity alive with stale state across a window where the identity can be reused. That is the real correctness defect `delayed_clear` introduces. ## 4. Proposed fix: incremental target reconciliation, reuse the checker Don't rebuild — keep the existing checker and reconcile only the target diff. Gate it so that only **node-set** changes take the fast path; a change to the **check parameters** themselves (`interval`, `http_path`, `type`, …) still requires a fresh checker. ```lua -- replace the delayed_clear + stop + create block in timer_create_checker local existing = working_pool[resource_path] if existing and checks_unchanged(existing, upstream) then -- node set changed only: reconcile in place, do NOT rebuild the checker local c = existing.checker local new_set = {} for _, n in ipairs(upstream.nodes) do new_set[n.host .. ":" .. (port or n.port)] = n end -- removed nodes: remove immediately, also clears their shm state keys for _, t in ipairs(c.targets) do if not new_set[t.ip .. ":" .. t.port] then c:remove_target(t.ip, t.port, t.hostname) -- takes effect now, no 10s window else new_set[t.ip .. ":" .. t.port] = nil -- unchanged: keep its health status end end -- added nodes: register immediately for _, n in pairs(new_set) do c:add_target(n.host, port or n.port, host, true, host_hdr) end existing.version = resource_ver -- bump version in place, keep the checker else -- check parameters changed: a fresh checker is genuinely required if existing then existing.checker:delayed_clear(DELAYED_CLEAR_TIMEOUT) existing.checker:stop() end local checker = create_checker(upstream) -- ... add_working_pool(resource_path, resource_ver, checker) end ``` `remove_target` writes the updated `TARGET_LIST` first and clears the per-target state only after — the same write ordering the #10312 fix relies on — so surviving nodes (never touched) see no race, and departed nodes have their state cleared immediately, which closes the IP-reuse hole: https://github.com/api7/lua-resty-healthcheck/blob/master/lib/resty/healthcheck.lua#L585-L623 ## 5. Why this beats both the status quo and immediate `clear()` | Approach | Removed IP | Surviving nodes' health status | #10312 race | |---|---|---|---| | current `delayed_clear` | lingers ~10s (+ IP-reuse bug) | preserved | none | | immediate `clear()` (this PR) | immediate | **all wiped** | **reintroduced** | | **incremental reconciliation** | **immediate** | **preserved** | **none** (survivors untouched) | Node-set changes vastly outnumber check-parameter changes in k8s, so the common path is the zero-race, zero-status-loss fast path; the rebuild path (with `delayed_clear`) only runs when the check config itself changes, where a fresh checker is actually warranted. ## 6. Note on the `preStop` suggestion A `preStop` hook solves graceful connection draining (letting in-flight connections finish before the pod exits) — a different, orthogonal problem. It does not fix the control-plane defect here: the healthchecker holding a stale target (or inheriting status via IP reuse) after the endpoint is already gone from the upstream node set. Good practice, but not the fix for this issue. --- @tyq010101 @Baoyuantop @moonming — I'd suggest we repurpose this PR toward the incremental-reconciliation approach (with a `checks_unchanged` gate + rebuild fallback), which fixes the reported k8s staleness at the root without reintroducing #10312 and without the health-status reset that plain `clear()` causes. Happy to help shape the `checks_unchanged` comparison and add e2e coverage for both the node-diff path and the check-config-change fallback. -- 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]
