A75: Implement the new LB policy topology for the non-aggregate cluster by mswierq · Pull Request #9234 · grpc/grpc-go

mswierq · 2026-07-15T11:20:49Z

Implements A75 for the non-aggregate cluster.

This change implements the new LB policy topology for the non-aggregate clusters only, the aggregate cluster uses the legacy approach. Refactoring of the new topology for the aggregate cluster case will be delivered in a follow-up PR. This change does not break non-aggregate to aggregate (and vice versa) switch.

RELEASE NOTES: none

…ers only

codecov · 2026-07-15T11:26:28Z

Codecov Report

❌ Patch coverage is 92.30769% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.55%. Comparing base (3b0f758) to head (aa09429).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/xds/balancer/cdsbalancer/cdsbalancer.go	85.71%	5 Missing ⚠️
internal/xds/balancer/cdsbalancer/configbuilder.go	96.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9234      +/-   ##
==========================================
+ Coverage   87.52%   87.55%   +0.03%     
==========================================
  Files         430      430              
  Lines       30678    30741      +63     
==========================================
+ Hits        26850    26916      +66     
+ Misses       3827     3824       -3     
  Partials        1        1

Files with missing lines	Coverage Δ
internal/xds/balancer/cdsbalancer/configbuilder.go	`95.60% <96.42%> (+0.18%)`	⬆️
internal/xds/balancer/cdsbalancer/cdsbalancer.go	`77.17% <85.71%> (+5.17%)`	⬆️

... and 32 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

easwars · 2026-07-17T16:44:52Z

/gemini review

gemini-code-assist

Code Review

This pull request updates the CDS balancer to support the gRFC A75 topology for single (non-aggregate) clusters, routing them through an outlier detection policy instead of a priority policy. The changes include updating the configuration builder to construct the new balancer tree (outlier_detection -> cluster_impl -> priority -> wrr_locality -> round_robin) and updating corresponding tests. The review feedback highlights several critical areas in the new code paths where missing configuration data (such as nil cluster configs, endpoint configs, or empty priorities) could lead to nil pointer dereferences or out-of-bounds panics, and suggests adding defensive checks to ensure robustness.

This reverts commit fbed93a.

…b-policy-tree-refactoring

Pranjali-2501 · 2026-08-12T07:28:45Z

+	if isAggregate {
+		childCfgBytes, endpoints, err = buildAggregateClusterConfigJSON(b.priorities, &b.xdsLBPolicy)
+	} else {
+		childCfgBytes, endpoints, err = buildLeafClusterConfigJSON(b.priorities, &b.xdsLBPolicy)
+	}


I think the variable topLBName can be set here only instead of setting it in another if-else block.

after reverting gracefulswitch this actually has to stay that way, because we need to check first if the top policy type has changed due to update (e.g. update from non-aggregate to aggregate cluster)

I think we can call buildAggregateClusterConfigJSON at the time of identifying the topLBName. Building the Cluster Config is not dependant on creating the child balancer of CDS.

Suggested change

if isAggregate {

childCfgBytes, endpoints, err = buildAggregateClusterConfigJSON(b.priorities, &b.xdsLBPolicy)

} else {

childCfgBytes, endpoints, err = buildLeafClusterConfigJSON(b.priorities, &b.xdsLBPolicy)

}

var (

topLBName string

childCfgBytes []byte

endpoints []resolver.Endpoint

err error

)

if isAggregate {

topLBName = priority.Name

childCfgBytes, endpoints, err = buildAggregateClusterConfigJSON(b.priorities, &b.xdsLBPolicy)

} else {

topLBName = outlierdetection.Name

childCfgBytes, endpoints, err = buildLeafClusterConfigJSON(b.priorities, &b.xdsLBPolicy)

}

if err != nil {

return fmt.Errorf("failed to build child policy config: %v", err)

}

Pranjali-2501 · 2026-08-12T07:28:59Z

+			select {
+			case <-exitIdleCh:
+			default:


Why do we need this here?

This might be called more than once in test cases where balancers are being reconfigured, e.g. transition from non-aggregate to aggregate cluster (TestAggregatedClusterSuccess_SwitchBetweenLeafAndAggregate).

However, those channels are not used in the mentioned aggregate test case. We may consider having these exitIdleCh and closeCh channels configurable and remove this default option. WDYT?

Yes, have a default case in a switch introduced non-deterministic behavior in tests leading to hard to debug flaky tests. If all we care about is whether ExitIdle was invoked or Close was invoked, we would even change these fields to a grpcsync.Event instead of a vanilla channel. The former type provides both a channel (for tests to block) and an atomic (for tests to verify state without blocking).

Ping on this one.

…b-policy-tree-refactoring

This reverts commit 51e986e.

Pranjali-2501

LGTM with some minor comments.

…b-policy-tree-refactoring

easwars

Posting some comments that I seem to have queued up a while back.

Doing a full pass in sometime though.

easwars · 2026-09-11T00:38:14Z

+			select {
+			case <-exitIdleCh:
+			default:


Yes, have a default case in a switch introduced non-deterministic behavior in tests leading to hard to debug flaky tests. If all we care about is whether ExitIdle was invoked or Close was invoked, we would even change these fields to a grpcsync.Event instead of a vanilla channel. The former type provides both a channel (for tests to block) and an atomic (for tests to verify state without blocking).

…b-policy-tree-refactoring

easwars · 2026-09-11T17:59:06Z

 //
 // Each priorityConfig corresponds to one leaf cluster retrieved from XDSConfig
 // for the top-level cluster.
 type priorityConfig struct {


The name priorityConfig has become quite confusing, especially under the new A75 topology. Each instance of this struct actually represents the state and configuration of a single leaf cluster (EDS or LOGICAL_DNS), while the leaf cluster itself contains multiple priority levels within its EDSUpdate.Localities.

Having this type named priorityConfig causes confusion across the implementation:

In cdsBalancer, b.priorities has len == 1 for non-aggregate clusters.

In buildLeafClusterConfigJSON, we take a slice priorities []*priorityConfig and unconditionally index priorities[0] (risking a panic if empty), even though a non-aggregate cluster inherently only ever has a single leaf cluster config.

Can we rename this type to leafClusterConfig (which matches buildLeafClusterConfig and the docstring on line 48), and clean up the surrounding fields and signatures as follows:

Rename the type and update its docstring in configbuilder.go

Update buildLeafClusterConfigJSON and buildLeafClusterConfig signatures

Instead of taking a slice, buildLeafClusterConfigJSON should take a single *leafClusterConfig

And buildLeafClusterConfig parameter renamed from p *priorityConfig to leaf *leafClusterConfig

Update fields and helpers in cdsbalancer.go:

Rename priorityConfigs map[string]*priorityConfig to leafConfigs map[string]*leafClusterConfig (or leafClusters)

Rename priorities []*priorityConfig to leafClusters []*leafClusterConfig

Rename updatePriorityConfig(...) to updateLeafClusterConfig(...)

In updateChildConfig(), pass the single leaf cluster config cleanly

Update aggregate helpers in configbuilder.go:

buildAggregateClusterConfigJSON(leafClusters []*leafClusterConfig, ...)

buildAggregateClusterConfig(leafClusters []*leafClusterConfig, ...)

This makes the distinction between leaf clusters and priority levels crystal clear throughout both files and removes the unchecked slice indexing.

easwars · 2026-09-11T18:02:00Z

+	if b.childLB != nil && b.childLBName != childPolicyName {
+		b.childLB.Close()
+		b.childLB = nil
+	}


When closing and setting b.childLB = nil here, we should also reset b.childLBName = "" and b.childConfigParser = nil. Otherwise, if newChildBalancer fails below, the balancer is left with b.childLB == nil but holding stale values for childLBName and childConfigParser.

The same cleanup is needed in closeChildPolicyAndReportTF() and Close(). We could add a small helper:

func (b *cdsBalancer) closeChildPolicy() { if b.childLB != nil { b.childLB.Close() b.childLB = nil b.childLBName = "" b.childConfigParser = nil } }

and use it in all three places to keep the three fields in sync.

easwars · 2026-09-11T18:05:31Z

 	// do not need to be guarded by a mutex.
 	cc                balancer.ClientConn   // ClientConn interface passed to child LB.
 	bOpts             balancer.BuildOptions // BuildOptions passed to child LB.
 	childConfigParser balancer.ConfigParser // Config parser for cluster_resolver LB policy.


The trailing comment on this field is stale now. Also, this field is no longer set at build time and no longer is read-only after that. So, this should be moved to the block below which contains fields that are accessed from methods implementing the balancer.Balancer interface.

easwars · 2026-09-11T18:27:12Z

+		var retEndpoint resolver.Endpoint
+		for _, e := range endpoints {
+			retEndpoint.Addresses = append(retEndpoint.Addresses, e.Addresses...)
+		}


If endpoints is empty (e.g. DNS resolution returned no addresses), this currently creates an endpoint with Addresses == nil, attaches attributes to it, and appends it to retEndpoints. As a result, retEndpoints contains a single endpoint with 0 addresses, which child balancers do not expect.

In contrast, the aggregate DNS builder (buildClusterImplConfigForDNS) explicitly checks if len(endpoints) == 0 { return pName, lbconfig, nil }.

Could we guard the endpoint construction and append with if len(endpoints) > 0 { ... } so retEndpoints remains empty when there are no DNS endpoints?

Also, please consider adding an empty_endpoints test case to TestBuildLeafClusterConfig_DNS in configbuilder_test.go to cover this scenario.

easwars · 2026-09-11T18:29:33Z

+	return ret, endpoints, nil
+}
+
+func buildLeafClusterConfig(p *priorityConfig, xdsLBPolicy *internalserviceconfig.BalancerConfig) (*outlierdetection.LBConfig, []resolver.Endpoint, error) {


While this function's signature is set up to return an error, it never returns one. Should we add a default case to the switch clusterUpdate.ClusterType and return an error?

easwars · 2026-09-11T18:30:38Z

+				IgnoreReresolutionRequests: true,
+			}
+		}
+	case xdsresource.ClusterTypeLogicalDNS:


Could we preserve the explanatory comments from buildClusterImplConfigForDNS here? Specifically, explaining why we aggregate into a single logical endpoint, why an empty locality attribute is added (so locality-based policies like weighted_target continue to work), and why the locality weight is set to 1. They provide helpful context for why these attributes are needed for LOGICAL_DNS.

A75: Implement the new LB policy topology for the non-aggregate clust…

533dafb

…ers only

easwars requested a review from Pranjali-2501 July 15, 2026 18:13

easwars assigned Pranjali-2501 Jul 15, 2026

easwars added Type: Feature New features or improvements in behavior Area: xDS Includes everything xDS related, including LB policies used with xDS. labels Jul 15, 2026

easwars added this to the 1.84 Release milestone Jul 15, 2026

mswierq added 2 commits July 17, 2026 09:36

Merge branch 'master' into a75/non-aggregate-lb-policy-tree-refactoring

5516779

A75: Add missing test cases

9b3c703

gemini-code-assist Bot reviewed Jul 17, 2026

View reviewed changes

Address Gemini review - fix nil checks

fbed93a

Pranjali-2501 reviewed Aug 1, 2026

View reviewed changes

Pranjali-2501 reviewed Aug 3, 2026

View reviewed changes

Comment thread internal/xds/balancer/cdsbalancer/configbuilder.go Outdated

Comment thread internal/xds/balancer/cdsbalancer/configbuilder.go

Comment thread internal/xds/balancer/cdsbalancer/cdsbalancer.go

Comment thread internal/xds/balancer/cdsbalancer/configbuilder.go Outdated

Pranjali-2501 assigned mswierq and unassigned Pranjali-2501 Aug 3, 2026

mswierq added 6 commits August 3, 2026 12:26

Revert "Address Gemini review - fix nil checks"

c1ab2d9

This reverts commit fbed93a.

Fix comments and legacy code cleanup

785be23

Use gracefulswitch.Balancer

51e986e

Fix functions and variables names

e7c0475

Do not create clusterImpl for each priority

d6a83ca

Merge remote-tracking branch 'origin/master' into a75/non-aggregate-l…

31d6b4c

…b-policy-tree-refactoring

mswierq requested a review from Pranjali-2501 August 4, 2026 11:13

easwars assigned Pranjali-2501 and unassigned mswierq Aug 11, 2026

Pranjali-2501 reviewed Aug 12, 2026

View reviewed changes

Pranjali-2501 assigned mswierq and unassigned Pranjali-2501 Aug 12, 2026

mswierq added 2 commits August 12, 2026 09:42

Merge remote-tracking branch 'origin/master' into a75/non-aggregate-l…

53b4209

…b-policy-tree-refactoring

Address feedback

9f43e37

mswierq requested a review from Pranjali-2501 August 12, 2026 14:54

easwars assigned Pranjali-2501 and unassigned mswierq Aug 12, 2026

Revert "Use gracefulswitch.Balancer"

f2fc6b5

This reverts commit 51e986e.

eshitachandwani modified the milestones: 1.84 Release, 1.85 Release Aug 18, 2026

Pranjali-2501 approved these changes Aug 26, 2026

View reviewed changes

Comment thread internal/xds/balancer/cdsbalancer/configbuilder_test.go Outdated

Comment thread internal/xds/balancer/cdsbalancer/configbuilder_test.go Outdated

Pranjali-2501 assigned easwars and unassigned Pranjali-2501 Aug 26, 2026

Pranjali-2501 requested a review from easwars August 26, 2026 05:51

Pranjali-2501 assigned mswierq Aug 26, 2026

mswierq added 2 commits August 28, 2026 11:42

Merge remote-tracking branch 'origin/master' into a75/non-aggregate-l…

6c54ef5

…b-policy-tree-refactoring

Address feedback

ce1c7f0

Pranjali-2501 unassigned mswierq Aug 31, 2026

easwars reviewed Sep 10, 2026

View reviewed changes

Comment thread internal/xds/balancer/cdsbalancer/cdsbalancer.go Outdated

Comment thread internal/xds/balancer/cdsbalancer/cdsbalancer.go

easwars reviewed Sep 11, 2026

View reviewed changes

mswierq added 5 commits September 11, 2026 12:00

Address feedback: nits and new names

a0841ec

Merge remote-tracking branch 'origin/master' into a75/non-aggregate-l…

c45c12b

…b-policy-tree-refactoring

Remove unused serviceConifg

506f940

Address feedback: consume lb config in a detereministic way

5657854

Address feedback: use grpcsync.Event

aa09429

mswierq requested a review from easwars September 11, 2026 17:42

easwars reviewed Sep 11, 2026

View reviewed changes

easwars assigned mswierq and unassigned easwars Sep 11, 2026

easwars reviewed Sep 11, 2026

View reviewed changes

easwars added the Status: Requires Reporter Clarification label Sep 11, 2026

Uh oh!

Conversation

mswierq commented Jul 15, 2026 • edited by easwars Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

easwars commented Jul 17, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Pranjali-2501 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

easwars left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mswierq commented Jul 15, 2026 •

edited by easwars

Loading

codecov Bot commented Jul 15, 2026 •

edited

Loading