fix(balancer): keep least_conn load state across upstream scaling#13666
Open
AlinsRan wants to merge 1 commit into
Open
fix(balancer): keep least_conn load state across upstream scaling#13666AlinsRan wants to merge 1 commit into
AlinsRan wants to merge 1 commit into
Conversation
d86e827 to
26cd8bc
Compare
26cd8bc to
7aa1efd
Compare
ca0cd95 to
f3c5a8e
Compare
When least_conn proxies long-lived connections (WebSocket) and the upstream is scaled, the load stays skewed on the original nodes: the newly added ones are not preferred and least_conn degrades to round-robin. The picker is cached by the upstream version, so it is rebuilt whenever the upstream changes - and scaling changes it. The binary heap holding the per-server scores lives inside the picker, so every score is reset to the base weight on rebuild and the connections already established are forgotten. The requests that are still in flight keep the picker they were routed with and release their server on it in the log phase, so their releases land on a heap nobody reads anymore, while the rebuilt heap never learns about them. Move both the heap and the in-flight connection counts out of the picker into a per-worker state keyed by the upstream resource key, which is stable across scaling (and across health status flips) unlike the picker version. The picker now reconciles that heap with the current node set instead of rebuilding it: surviving nodes keep their load, a freshly added node starts empty and is preferred right away, and a node that leaves keeps its count so its score is restored if it comes back. Every generation of pickers shares one view of the load, so a connection established before a rebuild is released against the heap that is actually in use. Because that heap can hold nodes a picker was not built with, a picker only ever hands out the nodes of its own conf, which is what the rest of balancer.lua assumes of the server it gets back. The state is held weakly, so it lives exactly as long as a picker that can still release a connection into it. pick_server skips the balancer entirely when the upstream has a single node. least_conn cannot afford that: the requests routed while the upstream was alone are the ones a later scale out needs to know about, and a single node is where a k8s deployment or a discovery service starts from. Route them through the balancer so they are counted. The score is derived from the connection count instead of being accumulated with +/- effect_weight, which keeps it exact over time. Now that the state outlives the picker, releasing a server the request no longer holds is no longer self-healing: it used to be washed away by the next rebuild, now it is written into shared state for good. Make after_balance own that: releasing a server clears ctx.balancer_server, so the field always names the server the request currently holds and a caller that retries simply gets a fresh one from the next pick. Three release sites relied on the caller to remember and did not - pick_server on entering a retry and on skipping an unhealthy node, and ai-proxy-multi's retry_on_error, which releases before it knows whether it will pick again and returns as-is when the fallback strategy does not cover the status code. Each of them left the log phase free to release the same server a second time. Reaching the first one needs nothing unusual: the retry count is derived from all nodes while only the healthy ones are picked from, so a single failing health check is enough. ctx.server_picker was also published only at the very end of pick_server, so a request that bailed out after picking a server never released it at all. The priority is part of the state key, since node sets of different priorities are disjoint and must not share a heap. A node that moves between priorities leaves its count behind in the old level, where it drains normally. Also call after_balance in the stream log phase, which it never did: for L4 the count was only ever incremented, so least_conn could not balance TCP long connections at all and ctx.balancer_tried_servers was leaked. Note that an upstream declared inline in a traffic-split rule has no stable resource key, so it keeps the previous behavior. Fixes #12217
f3c5a8e to
d968dc9
Compare
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.
Fixes #12217
The problem
Scale a
least_connupstream out and the new node is not preferred — the load stays on the original nodes. With WebSocket it never recovers, because those connections outlive the scale out.Reproduced by
t/node/least_conn3.tTEST 9 (real WebSocket sessions through APISIX). Two nodes, four live sessions, add a third node, open two more:Round-robin. The empty node gets half, an already-loaded node gets the other half.
Why
The per-server score heap lives inside the picker, and the picker is cached by upstream version. Scaling bumps the version, the heap is rebuilt, every score resets to
1/weight— the four live sessions are forgotten.Seeding the new heap from a side count doesn't fix it either. In-flight requests keep the picker they were routed with and release their server on that picker in the log phase, so those releases land on a heap nobody reads. A heap that is only correct at rebuild time drifts: once the old sessions drain, the surviving nodes stay penalized for load they no longer carry — the same imbalance, mirrored.
The fix
Move the heap and the in-flight counts out of the picker, into per-worker state keyed by
(resource_key, priority)— stable across scaling and health flips, unlike the picker version. The picker now reconciles that heap with the current node set instead of rebuilding it:Single-node upstreams now go through the balancer.
pick_serverused to skip it when#nodes == 1, so anything routed while the upstream was alone was never counted — and a 1→N scale out then saw the surviving node as empty. One node is where a k8s deployment or a discovery service starts, so this was most of the bug, not an edge case.The state is held weakly (
__mode = "v"), which is exactly the lifetime it needs: a picker holds its state, an in-flight request holds its picker, the picker cache holds the current one. So a state lives as long as anything can still release into it — and when nothing can, there is no connection left to count. Nothing to size, nothing to evict. (An LRU would rank states by how recently they were rebuilt; a stable upstream full of long-lived connections is precisely the one that is never rebuilt.)Scores are derived from the count,
(count + 1) / weight, instead of accumulated with± effect_weight. Same semantics, exact over time.Release discipline
Because the state now outlives the picker, releasing a server the request no longer holds is no longer self-healing — it used to be washed away by the next rebuild, now it is written into shared state for good.
So
after_balanceowns the invariant:ctx.balancer_servernames only the server the request currently holds, and releasing it clears the field. Three release sites relied on the caller to remember, and didn't:pick_server, on entering a retrypick_server, skipping an unhealthy nodeai-proxy-multi.retry_on_errorThe first needs nothing unusual to hit:
retriesis derived from all nodes while only the healthy ones are picked from, so one node failing its health check is enough.ai-proxy-multiis fixed by construction — the plugin isn't touched.Two more, in the same vein:
ctx.server_pickeris published as soon as a server is held, not at the end ofpick_server, so a request that bails out after picking one still releases it.get()would find nothing left, and a configuredretrieswould turn a retryable blip into a hard 502.Stream
stream_log_phase()never calledafter_balance. On L4 the count only ever went up, soleast_conncould not balance TCP long connections at all, andctx.balancer_tried_serversleaked.Scope and known limits
State is shared per
(resource_key, priority)per worker. An upstream with no stable resource key falls back to state private to its picker — today's behaviour, so no regression, but no fix either:up_confis synthesised in the plugin, and its own picker cache key already embeds the nodes table address, so there is nothing stable to key onai-proxy-multiinstances (ups_tab = {})conf_serverin the EE port (picker.new(nodes, {}))Two things routing single-node
least_connthrough the balancer changes:lrucache_server_picker+lrucache_addr+ a size-1 heap peek/update per request) — the same cost every multi-node upstream already pays, but a real one on the hottest path for the most common topology.parse_addr; it no longer does, so"::1:1980"now fails the way it already fails everywhere else. Pre-existing bug, not a new one — onorigin/master, a two-node upstream with unbracketed IPv6 hosts crashes identically (balancer.lua:238: attempt to concatenate field 'port' (a nil value)) for every balancer type. The Admin API rejects such nodes; only the DP-side check skips that validation. Worth fixing, but on its own: the addr string is built intransform_nodeand shared with chash and the health checker.No new config, no shared dict, on by default. Counting stays per-worker, matching the existing per-worker heap. Short requests return their count on completion, so steady-state behaviour is unchanged.
Tests
These three fail on
master:least_conn3.tTEST 9least_conn3.tTEST 10least_conn3.tTEST 7The rest pin the internals: count kept across a rebuild, drained node returns to the pool, scale-down, releases on the old picker seen by the new one, priority isolation, weight change, keyless-upstream fallback, and no double release (TEST 6 drives the real
balancer.pick_server).Also
least_conn.tTEST 5 (more retries than nodes — that path had no coverage) and TEST 6 (a single node is retried, because it is the only one there is), andt/stream-node/least_conn.tfor the L4 release.Checklist