fix(healthcheck): cold-start readiness gate for lazily-created, unprobed checkers - #13896
Closed
aanogueira wants to merge 4 commits into
Closed
Conversation
…iness gate (PS-12691) A freshly created health-check target defaults to internal_health=healthy with zero probes (add_target's hardcoded is_healthy=true), so a pod that restarts while its backend is already unhealthy briefly routes real traffic to it until the first active probe corrects the target's state. hack/patches/lua-resty-healthcheck-probed-gate.patch (stacks on the existing apache#13888 reconcile patch) adds a per-target "probed" shm flag, written the first time an active check is actually dispatched for that target (success, failure, or timeout all count -- attempted, not "healthy"), and a module function resty.healthcheck.all_targets_probed(name, shm_name) to query it from any worker. apisix/healthcheck_manager.lua adds two accessors for a readiness plugin to use: ensure_checker(resource_path), which proactively seeds a checker for a resource even with zero prior traffic (checker creation is otherwise entirely lazy, seeded only by fetch_checker() from the live request path -- an idle-but-critical upstream would never get a checker built without this), and is_resource_probed(resource_path), which delegates to the new library function. Extends t/node/healthcheck-fresh-node-default-healthy.t (TEST 3, TEST 4) to cover both: all_targets_probed() flips false->true only after a real probe fires, and ensure_checker() builds a checker with zero prior traffic. Companion plugin-side change (mollie-health-check.lua, edge-app) wires these into the readiness endpoint. Signed-off-by: Andre Nogueira <aanogueira@protonmail.com>
The lua-resty-healthcheck-probed-gate.patch committed in 12fa7f2 stacks on this reconcile patch (its own header already documents that), but the patch file itself was left out of that commit -- add it now, along with the two regression tests that reproduce the bug it fixes: a worker that misses the worker_events broadcast for a target's health-state change never re-converges with shm, since the broadcast has no delivery guarantee and is never raised again once a target is already at the reported state. Already raised upstream as api7/lua-resty-healthcheck#59; this local copy is for this repo's own kind-repro validation harness. Signed-off-by: Andre Nogueira <aanogueira@protonmail.com>
…or the readiness gate A single active-check attempt does not guarantee a target's real health state is known: with e.g. unhealthy.http_failures = 2 configured, internal_health only actually converges after two consecutive attempts. The boolean "probed" flag from 12fa7f2 flipped true after the first attempt regardless, so a readiness gate built on it could still open before a target's true state was reached -- confirmed live via a kind end-to-end trial, where real traffic kept leaking for 6 seconds after the pod was already marked Ready. Changes the shm value from a boolean to a per-target attempt counter (incremented in run_single_check, not just set), and all_targets_probed(name, shm_name, min_attempts) now compares against an explicit threshold instead of requiring only one attempt. apisix.healthcheck_manager.is_resource_probed computes that threshold from the checker's own config: max(unhealthy.http_failures, .tcp_failures, .timeouts, healthy.successes). Also fixes a second gap found in the same trial: ensure_checker seeded a checker using the resource's raw, potentially-unresolved node list. Domain-name upstream nodes are otherwise only resolved by apisix.upstream.get_by_id -> parse_domain_in_up, which runs exclusively on the live request path -- a checker built ahead of traffic would start probing under the unresolved domain-string identity, then get silently rebuilt (wiping its accumulated probe count) the moment real traffic first resolved the domain and bumped _nodes_ver. ensure_checker now resolves proactively via the same parse_domain_in_up path, using the has_domain flag already set on the shared config object by the /upstreams config watcher's filter callback. Adds TEST 5 to cover the threshold behavior directly: all_targets_probed must stay false after one attempt when min_attempts=2, and only flip true after the second. Validated end to end in a local kind cluster: pod-restart-while-unhealthy now shows zero leaked requests before or after the readiness transition, down from 64 leaked responses in the prior boolean-based version. Signed-off-by: Andre Nogueira <aanogueira@protonmail.com>
…fixes Local, Docker/kind-based reproduction and validation setup used throughout this branch's work, kept for anyone re-verifying either fix or extending it: - Dockerfile: builds APISIX from this repo's source, optionally applying the apache#13888 reconcile patch and/or the probed-gate patch, with an optional events-queue-shrink knob for forcing the apache#13888 queue-overflow path deterministically. - run-repro.sh / coldstart-trial.sh / coldstart-gate-trial.sh / coldstart-gate-kind-trial.sh: plain-docker and kind end-to-end trials measuring real leaked traffic against a mock backend, stock vs patched. - kind-cluster.yaml, manifests/, mock/, plugin/: supporting cluster config, mock upstream backends, and a staged copy of edge-app's mollie-health-check plugin (needed since a Docker build context cannot reach outside this repo). Not part of either patch itself -- a local testing aid only. Signed-off-by: Andre Nogueira <aanogueira@protonmail.com>
This file contains hidden or 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
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.
Problem
A freshly created health-check target defaults to
internal_health = healthywith zero probes (add_target's hardcodedis_healthy=true), so a pod that restarts while its backend is already unhealthy briefly routes real traffic to it until the first active probe corrects the target's state. Separately, checker creation itself is entirely lazy -- seeded only byfetch_checker()on the live request path -- so an idle-but-critical upstream with no prior traffic would never get a checker built at all ahead of a readiness check.There is currently no way to tell "healthy" (a real check passed) apart from "healthy" (the zero-probe default) from outside the checker, so nothing can gate readiness on "has this actually been checked yet."
Changes
hack/patches/lua-resty-healthcheck-probed-gate.patch(companion library-side change, api7/lua-resty-healthcheck -- see note below): adds a per-target probe-attempt counter in shm, incremented each time an active check is actually dispatched for a target (success, failure, or timeout all count -- attempted, not "healthy"), and a module functionall_targets_probed(name, shm_name, min_attempts)so any worker can ask "has every target had at leastmin_attemptsreal checks yet?" A single attempt is not always enough: with e.g.unhealthy.http_failures = 2configured,internal_healthonly converges after two consecutive attempts, so a boolean "was it ever probed" flag is not sufficient on its own -- confirmed via a live kind end-to-end trial, where a boolean-based version still leaked real traffic for 6 seconds after the pod was already marked Ready.apisix/healthcheck_manager.lua: adds two accessors for a readiness plugin to use --ensure_checker(resource_path): proactively seeds a checker for a resource even with zero prior traffic, reusing the existingtimer_create_checkerconstruction path. Also resolves domain-name upstream nodes viaparse_domain_in_upup front -- that resolution otherwise only happens on the live request path, so a checker built ahead of traffic would start probing under an unresolved identity and get silently rebuilt (wiping its probe count) the moment real traffic first resolved the domain.is_resource_probed(resource_path): delegates to the library'sall_targets_probed, computing the required attempt threshold from the checker's own config (max(unhealthy.http_failures, .tcp_failures, .timeouts, healthy.successes)).t/node/healthcheck-fresh-node-default-healthy.t: new tests covering lazy checker creation (fetch_checkerreturnsfalseuntil the next timer tick),ensure_checkerbuilding a checker with zero prior traffic,all_targets_probedflipping only after a real probe, and the multi-attempt threshold behavior specifically (TEST 5: staysfalseafter 1 attempt whenmin_attempts=2, flipstrueonly after the 2nd).hack/kind-repro/: a local, Docker/kind-based validation harness (not part of the fix itself) used to reproduce and validate this end to end -- pod-restart-while-unhealthy now shows zero leaked requests before or after the readiness transition, down from 64 leaked responses with an earlier boolean-based version of this same gate.Dependency note
The probe-counter shm mechanism lives in the vendored
lua-resty-healthcheck-api7library, not this repo.hack/patches/contains the patch and a full reference copy of the patched file for this repo's own local validation harness; the library-side change itself will be proposed as a standalone PR against api7/lua-resty-healthcheck (separate from api7/lua-resty-healthcheck#59, an unrelated, already-open fix for #13888 that this patch happens to stack on top of in this repo's local patch files only -- the two are independent fixes for independent issues).Testing
t/node/healthcheck-fresh-node-default-healthy.t(this repo)/payrequests and the/health_check_internal/readytransition, stock vs. patched (seehack/kind-repro/coldstart-gate-kind-trial.sh)