Conversation
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.
Description
Two related changes that together let Kubernetes drive proxyd's startup and shutdown cleanly without dropping requests on either end.
1. Split
/healthzand add/readyz/healthzis unchanged — it returns 503 only when the server is draining, suitable for liveness probes (won't restart the pod on transient upstream/consensus flaps).A new
/readyzendpoint is added for readiness probes. It returns 503 when:Latest == 0.Otherwise it returns 200. Backend groups without consensus enabled are skipped, so
/readyzbehaves identically to/healthzfor non-consensus deployments and is safe to wire unconditionally.2. Idle-based graceful shutdown mode
Adds an opt-in graceful shutdown mode that swaps the existing "drain everything immediately" behaviour for an idle-based one. Behind a config flag; default behaviour is unchanged.
When enabled (
graceful_shutdown_idle = true), on shutdown signal proxyd will:/readyzto 503 (so Kubernetes removes the pod from Service endpoints)./healthzreturning 200 (so the kubelet does not restart the pod mid-shutdown)./or/{authorization}.graceful_shutdown_idle_seconds(default10).This lets in-flight clients finish naturally instead of seeing a wall of 503s the moment a pod starts terminating, while still giving k8s a clean signal to stop sending new traffic. Existing
/{authorization}paths bump the idle timer the same way unauthenticated/requests do — both go through the sameHandleRPC/HandleWSentry point.New config (in
[server]):Suggested k8s probe wiring
Implementation notes
HandleReadyzinproxyd/server.go: iteratesBackendGroupsand 503s if any consensus-aware group haslen(GetConsensusGroup()) == 0orGetLatestBlockNumber() == 0. Also 503s onisDrainingorisShuttingDown.Serverfields:gracefulShutdownIdle bool,gracefulShutdownIdleDuration time.Duration,isShuttingDown atomic.Bool,lastRequestNanos atomic.Int64.HandleRPCandHandleWSbumplastRequestNanosat handler entry, before any auth/rate-limit work, so retries still keep the pod alive (matches the "is anyone still using me" semantics).Drain()branches on the flag: legacy path is untouched (setsisDraining, sleepsgraceful_shutdown_seconds); idle path setsisShuttingDownonly, ticks every 1s, and returns whentime.Since(lastRequest) >= idleDuration.Drain()already closes WS abruptly, so this is not a regression — calling it out so reviewers are aware.Tests
Added
TestHandleReadyzinproxyd/server_test.go: a table-driven unit test covering the gate logic — drain, no backend groups, group without consensus enabled, empty consensus group,Latest == 0, ready group, and the multi-group case where any unready group fails the gate. Constructs*ConsensusPollerdirectly with an in-memory tracker, no real backends or goroutines required.The new idle-shutdown path is not covered by a dedicated unit test — it is intrinsically time-dependent (ticker +
time.SinceagainstlastRequestNanos), and a faithful test would either need a fake clock injected intoDrain()or a real-time wait, neither of which feels worth it for the size of the change. Manually verified end-to-end against a HA deployment (Redis-backed consensus tracker + Redis cache, multiple replicas):/readyzreturns 503 during cold start and flips to 200 once the first consensus cycle completes;/healthzstays 200 throughout./readyzreturns 200 on a deployment with no consensus-aware backend groups./readyzback to 503 while/healthzremains 200.graceful_shutdown_idle = trueand no traffic,Drain()returns after ~idle_secondsfrom start.Drain()blocked; stopping traffic causes it to exit ~idle_secondslater./readyzreturns 503 while/healthzand/keep returning 200.graceful_shutdown_seconds, then shut down).Happy to add a fake-clock unit test for
Drain()if reviewers want one.Additional context
Two motivating problems, both observed on rollouts of a HA proxyd deployment (Redis-backed consensus tracker + Redis cache):
Cold-start error storm. proxyd starts accepting RPC traffic before backend probes have completed (
probe_success_thresholddefaults to 2 → ~8s minimum), before the consensus poller has run its first cycle (soGetConsensusGroup()is empty andtracker.Latest == 0), and before the HA leader has been elected (stateHeartbeat()skips lock acquisition while local state is invalid). During this window the cache is also cold, so every request becomes a miss that cascades intoBackendGroup.Forward()and either returnsErrNoBackendsor, in CL mode,ErrCLConsensusSyncNotReady(-32025). The result is a noisy spike of 5xx responses on every cold start. A k8s readiness probe is the standard fix: a 503 on/readyzkeeps the pod out of the Service endpoints until consensus is ready, so the cold-start error storm never reaches clients. We deliberately did not overload/healthzfor this — consensus state is allowed to flap during normal operation, and failing liveness on a transient consensus break would cause k8s to kill an otherwise-healthy proxyd.Shutdown error storm. As soon as
Drain()is called today, every RPC request gets a 503 for the fullgraceful_shutdown_secondswindow — even though those requests could have been served correctly. K8s already handles "stop sending new traffic" via the readiness probe (now that/readyzexists); we don't need proxyd to also reject in-flight requests to achieve a clean rollout. The new mode delegates the "stop traffic" job to k8s and uses the idle window purely as a "is anyone still using me" check before exit.The two changes are intentionally bundled because the idle-shutdown mode is only safe with the new
/readyzendpoint: without a separate readiness signal, k8s would have no way to know it should stop sending traffic to a pod that is still happily serving on/.