fix(xds): enable locality weighted lb for ConsistentHash Maglev policy - #9823
fix(xds): enable locality weighted lb for ConsistentHash Maglev policy#9823pujitha24 wants to merge 7 commits into
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7501a69b5c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Enable locality weighted load balancing so that backendRef weights, which are | ||
| // expressed as locality weights, are honored by the Maglev table, matching the | ||
| // behavior of the other load balancing policies. | ||
| consistentHash.LocalityWeightedLbConfig = &commonv3.LocalityLbConfig_LocalityWeightedLbConfig{} |
There was a problem hiding this comment.
This user-visible fix is covered only by an xDS golden that verifies the field is serialized, so it would still pass if Envoy accepted the configuration but failed to produce the intended 5/95 distribution or stable client pinning. The existing e2e suite tests weighted backends and ConsistentHash separately, but no test combines them; add an e2e case reproducing the weighted single-cluster scenario and asserting both distribution and consistency.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think this e2e test coverage is still valid and required? what do you think?
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a07c33a2be
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Enable locality weighted load balancing so that backendRef weights, which are | ||
| // expressed as locality weights, are honored by the Maglev table, matching the | ||
| // behavior of the other load balancing policies. | ||
| consistentHash.LocalityWeightedLbConfig = &commonv3.LocalityLbConfig_LocalityWeightedLbConfig{} |
There was a problem hiding this comment.
When ConsistentHash is combined with topology-derived zonal localities but no explicit WeightedZones, this unconditional setting changes Maglev from distributing hashes across hosts to distributing them across equally weighted zones: buildZonalLocalities now assigns every zone a weight of 1. Consequently, a backend with 1 endpoint in one zone and 9 in another changes from roughly 10/90 traffic to 50/50, potentially overloading the smaller zone. Enable locality weighting only when backendRef or explicit zone weights actually need honoring, rather than for every Maglev cluster.
Useful? React with 👍 / 👎.
|
Good catch — you're right that One caveat worth flagging: Maglev only exposes |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9823 +/- ##
=======================================
Coverage 81.29% 81.30%
=======================================
Files 263 263
Lines 40870 40871 +1
=======================================
+ Hits 33227 33230 +3
+ Misses 7643 7641 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| @@ -0,0 +1,4 @@ | |||
| Fixed backendRef weights being silently ignored when a route's backendRefs collapse into a | |||
| single cluster (no per-backendRef filters) with a ConsistentHash load balancer configured. | |||
| Envoy Gateway now always enables locality weighted load balancing on the generated Maglev | |||
There was a problem hiding this comment.
Should this be updated now since locality weighted load balancing is not enabled by default?
Envoy Gateway now enables locality-weighted load balancing on the generated Maglev policy when
collapsed backendRef weights or explicit weighted zones need to be honored.
|
Updated the release note — it now says locality-weighted lb is enabled when collapsed backendRef weights or explicit weighted zones need to be honored, matching the conditional logic from the earlier commit instead of "always enables". |
backendRef weights were silently ignored when a route's backendRefs
collapsed into a single Envoy cluster (no per-backendRef filters,
expressed as weighted localities) with a BackendTrafficPolicy setting
loadBalancer.type: ConsistentHash. Envoy's Maglev policy only enabled
locality weighted load balancing when the unrelated WeightedZones
feature was configured, while the other LB policies (least_request,
random, round_robin) always enable it via buildLocalityLbConfig.
Without it, Envoy treats all localities as equally weighted regardless
of the configured backendRef weights.
Drop the WeightedZones condition and always set LocalityWeightedLbConfig
on the generated Maglev policy, matching the other LB policies and
restoring the configured backendRef weights. Client pinning to a single
backend for a given hash key is preserved either way, since the Maglev
table is built from host weight times locality weight.
Motivation: reported in the linked issue, with two backendRefs (weights
5/95) to a single destination behind a ConsistentHash BackendTrafficPolicy,
neither the 5/95 split was honored nor was the intended per-client pinning
achieved on the intended ratio.
Approach: internal/xds/translator/cluster.go, ConsistentHash branch -
replace the WeightedZones-gated assignment of LocalityWeightedLbConfig
with an unconditional one. Added an xds-ir translator test case,
http-route-weighted-backend-consistent-hash-single-cluster, mirroring
the existing http-route-weighted-backend-consistent-hash case but
without per-backendRef filters so it exercises the single-cluster/
weighted-localities path from the issue's repro. Several pre-existing
Maglev golden fixtures (ext-auth-retry, jwt-with-backend-tls-retry,
load-balancer, oidc-backend-cluster-provider, oidc-provider-traffic-
features) gained the same localityWeightedLbConfig: {} line since they
exercise the same code path incidentally.
Validation: confirmed the new test fails without the fix (missing
localityWeightedLbConfig: {} on the Maglev cluster) and passes with it.
Ran `go build ./...` (passes) and
`go test ./internal/xds/translator/...` (passes, including the new and
all pre-existing xds-ir golden cases). Ran golangci-lint on the changed
package (0 issues). The repo's vendored shellcheck binary in `make lint`
is x86_64-only and could not run on this arm64 host; unrelated to the
Go code changed here.
Report: envoyproxy#9775
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Assisted-by: claude-sonnet-5 (via Claude Code)
…glev LocalityWeightedLbConfig is now always enabled for the ConsistentHash (Maglev) policy, but buildZonalLocalities (the PreferLocal/topology-aware routing path) never set LocalityLbEndpoints.LoadBalancingWeight, unlike its sibling functions. With locality-weighted lb enabled and no locality weight set, Envoy treats all localities as zero-weight, leaving Maglev with no selectable host. Give every zone an explicit, equal weight, matching the default already used by buildWeightedZonalLocalities and buildWeightedLocalities; the field is a no-op for LB policies that don't enable locality-weighted lb. Adds a regression test combining ConsistentHash with a PreferLocal destination split across zones. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
… merging main Merging upstream/main pulled in a new xds-ir translator fixture (udp-route-consistent-hash-source-ip, from envoyproxy#9826) added after this branch was cut. Its golden cluster output predates this PR's change to always enable locality-weighted lb on the Maglev policy, so gen-check failed comparing against a stale expectation. Regenerated with `--override-testdata=true`; the only diff is the same `localityWeightedLbConfig: {}` line already present in this PR's other Maglev golden fixtures. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
… needs honoring Unconditionally enabling LocalityWeightedLbConfig for the ConsistentHash (Maglev) policy broke plain per-key host pinning for single-destination clusters (confirmed by the HeaderBasedConsistentHashLoadBalancing e2e failure on this PR) and, per review feedback, skewed traffic across zones of uneven size for the PreferLocal/topology-aware path. Only set it when there's an actual locality weight to honor: explicit WeightedZones, or multiple backendRefs collapsed into one cluster as weighted destination settings (the original issue envoyproxy#9775 scenario). Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
The repo's golangci-lint forbidigo config bans k8s.io/utils/ptr.To in favor of a local variable + address-of, so the new TestBuildXdsClusterConsistentHashLocalityWeighting test case was failing lint. Switch to the local-var pattern already used elsewhere in the repo (e.g. internal/gatewayapi/route.go). Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
…sistentHash The earlier commit narrowed LocalityWeightedLbConfig to be conditional rather than always-on, but the release note text still said "always enables". Update it to match, per review feedback. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Adds an e2e test reproducing the single-cluster weighted-locality scenario this PR fixes (two backendRefs, no per-backendRef filters, ConsistentHash), asserting both that a hash key pins to one backend and that a 95/5 weight split is honored across distinct keys. The existing xds-ir golden test only verifies the field is serialized, not the runtime behavior, per review feedback. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
f4ad466 to
2fdcc19
Compare
3 participants
What this PR does / why we need it:
When a route splits traffic across backendRefs with no per-backendRef filters (so the
split collapses into a single Envoy cluster expressed as weighted "localities" instead
of separate weighted clusters) and a
BackendTrafficPolicysetsloadBalancer.type: ConsistentHash, the configured backendRef weights are silentlyignored. Envoy's Maglev load balancing policy only enables locality-weighted load
balancing when the unrelated
WeightedZonesfeature is configured, whereas the otherLB policies (
least_request,random,round_robin) always enable it. Without it,Envoy treats all localities as equally weighted regardless of the configured
backendRef weights.
This drops the
WeightedZonescondition and always setsLocalityWeightedLbConfigonthe generated Maglev policy, matching the other LB policies and honoring the
configured backendRef weights. Client pinning to a single backend is preserved either
way, since the Maglev table is built from host weight times locality weight.
Which issue(s) this PR fixes:
Fixes #
Validation:
Added an xds-ir translator test case,
http-route-weighted-backend-consistent-hash-single-cluster, mirroring the issue'sexact repro (two backendRefs to the same destination, weights 5/95, no per-backendRef
filters,
ConsistentHashon a header) so it exercises the single-cluster/weighted-localities path (as opposed to the existing
http-route-weighted-backend-consistent- hashcase, which exercises the separate weighted-clusters path). Confirmed this testfails without the fix (missing
localityWeightedLbConfig: {}on the Maglev cluster)and passes with it.
Ran:
go build ./...— passes.go test ./internal/xds/translator/...— passes, including the new and allpre-existing xds-ir golden test cases (several pre-existing Maglev golden fixtures
gained the same
localityWeightedLbConfig: {}line, since they exercise the samecode path incidentally).
go tool -modfile=tools/go.mod golangci-lint run --config=tools/linter/golangci-lint/.golangci.yml ./internal/xds/translator/...— 0 issues.Note:
make lint'sshellcheckstep could not run in this environment (therepo-vendored
tools/bin/shellcheckbinary is x86_64-only and this is an arm64 Mac);this is unrelated to the Go code changed here and golangci-lint on the affected
package is clean.
This is a narrow, mechanical fix confirmed against the maintainer's own root-cause
analysis on the issue (
internal/xds/translator/cluster.go, theConsistentHashbranch).
PR Checklist
git commit -s). See DCO: Sign your work.make lint's Go linting (golangci-lint) passes for the affected package; the unit tests and build pass.make generate gen-checkN/A: no generated files touched.make lint'sshellchecksub-step could not be run in this environment (x86_64-only vendored binary on an arm64 host) — unrelated to this change.release-notes/current/bug_fixes/9775-consistenthash-locality-weighted-lb.md.AI assistance: this change was drafted with Claude Code.
Fixes #9775