xds: change cdsbalancer to use update from dependency manager by eshitachandwani · Pull Request #8907 · grpc/grpc-go

eshitachandwani · 2026-02-16T09:50:27Z

This PR is part of A74 changes. This PR changes following:

Changes the CDS balancer to use the CDS and EDS/DNS update from XDSConfig received by xds_resolver instead of starting its own CDS watchers.
Remove cluster resolver completely
CDS balancer creates the priority config and creates priority balancer as its child.
Remove serializer from CDS balancer since it was there to serializer watcher updates and ClientConn updates, now that there are no watchers and clientConn updates are already serialized , serializer is no longer needed.
Update tests in cdsbalancer package to check the priority config instead of cluster_resolver config.
xds_resolver tracks reference counts for weighted cluster and cluster specifier plugin seperately. xds_resolver subscribes to the weighted cluster in dependency manager when referenced for first time and unsubscribes when references in xds_resolver go to zero.
Incase of LDS/RDS resource error , we now directly send empty service config , instead of sending a complete service config with clusters that RPCs are still referenced to.
Minor test fixes.

RELEASE NOTES:

xds:
- Ambient errors for cluster resources are now logged exclusively in the dependency manager and are no longer propagated to Load Balancing (LB) policies.
- When re-resolution is requested, all clusters of type LOGICAL_DNS will be re-resolved simultaneously, rather than only a single cluster.
- Upon receipt of a listener or route resource error, all in-flight RPCs will now fail immediately.
- Any error encountered during the creation or update of a priority configuration will now transition the channel to a TRANSIENT_FAILURE state.

codecov · 2026-02-16T12:24:10Z

Codecov Report

❌ Patch coverage is 80.40000% with 49 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.38%. Comparing base (944f058) to head (700015c).
⚠️ Report is 16 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/xds/balancer/cdsbalancer/cdsbalancer.go	69.28%	30 Missing and 13 partials ⚠️
internal/xds/balancer/cdsbalancer/configbuilder.go	95.12%	2 Missing ⚠️
internal/xds/resolver/serviceconfig.go	90.00%	1 Missing and 1 partial ⚠️
internal/xds/xdsdepmgr/xds_dependency_manager.go	91.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8907      +/-   ##
==========================================
+ Coverage   80.74%   83.38%   +2.63%     
==========================================
  Files         416      410       -6     
  Lines       33434    32570     -864     
==========================================
+ Hits        26996    27157     +161     
+ Misses       4641     4038     -603     
+ Partials     1797     1375     -422

Files with missing lines	Coverage Δ
...ds/balancer/cdsbalancer/configbuilder_childname.go	`100.00% <ø> (ø)`
...rnal/xds/balancer/clustermanager/clustermanager.go	`80.95% <100.00%> (+9.52%)`	⬆️
internal/xds/resolver/xds_resolver.go	`91.87% <100.00%> (+3.16%)`	⬆️
internal/xds/balancer/cdsbalancer/configbuilder.go	`91.94% <95.12%> (ø)`
internal/xds/resolver/serviceconfig.go	`86.39% <90.00%> (-1.76%)`	⬇️
internal/xds/xdsdepmgr/xds_dependency_manager.go	`88.27% <91.66%> (+18.96%)`	⬆️
internal/xds/balancer/cdsbalancer/cdsbalancer.go	`69.78% <69.28%> (-16.48%)`	⬇️

... and 52 files with indirect coverage changes

🚀 New features to boost your workflow:

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

eshitachandwani · 2026-02-20T07:55:34Z

Regarding all the comments talking about eating the error , we were doing that since that was the current behaviour of cluster resolver i.e. it was eating errors instead of returning the error or putting the channel in TF. After an offline discussion we have decided to change the behaviour and any error in CDS LB policy i.e. any error relating to CDS not liking the update is returned from UpdateClientConnState effectively effectively moving the particular channel in TF

easwars

I see that you have replied saying "done" on a bunch of comments, but I don't see it being done. Are you missing some commits here?

eshitachandwani · 2026-02-25T11:49:51Z

I see that you have replied saying "done" on a bunch of comments, but I don't see it being done. Are you missing some commits here?

I had pushed all the changes and can see the changes reflected in the PR. Can you please try again and let me know if the older and current changes are still not visibile.

easwars

Mostly LGTM at this point. Will make another pass shortly.

easwars

LGTM. Just minor nits.

easwars · 2026-02-27T02:33:43Z

 	// Start an xDS management server that pushes the EDS resource names onto a
 	// channel when requested.


This comment needs to be updated.

easwars · 2026-02-27T02:34:16Z

+			// Check if we have a request for both EDS resources. If so, fire
+			// the event to unblock the test.


Nit: What this event signifies should ideally be documented at the place where the event is defined.

easwars · 2026-02-27T04:41:33Z

-	watchers         map[string]*watcherState     // Set of watchers and associated state, keyed by cluster name.
-	lbCfg            *lbConfig                    // Current load balancing configuration.
+	childLB           balancer.Balancer                     // Child policy, built upon resolution of the cluster graph.
+	xdsClient         xdsclient.XDSClient                   // xDS client to watch Cluster resources.


Nit: Clarify in this comment that this is only for dynamic subscriptions.

This is not needed for dynamic subscription but is passed so that cluster_impl can use it for Load reporting. Here it is mainly used to get nodeID. So removed the comment.

easwars · 2026-02-27T04:44:19Z

-	lbCfg            *lbConfig                    // Current load balancing configuration.
+	childLB           balancer.Balancer                     // Child policy, built upon resolution of the cluster graph.
+	xdsClient         xdsclient.XDSClient                   // xDS client to watch Cluster resources.
+	clusterConfigs    map[string]*xdsresource.ClusterResult // Map of cluster name to the last received result for that cluster.


Nit: Keep these trialing comments short and sweet.

Something like:

Cluster name to the last received result for that cluster

Hostname to priority config for that leaf cluster
...

For dynamic cluster unsubscription

True if a dynamic cluster has been subscribed to

etc .. etc

easwars · 2026-02-27T04:47:16Z

Nit: Please remove mentions of the serializer as it does not exist anymore.

easwars · 2026-02-27T05:03:42Z

@@ -168,16 +162,22 @@ type cdsBalancer struct {

 	xdsHIPtr *unsafe.Pointer // Accessed atomically.


It would be nice to state why this needs to be accessed atomically.

Since this will move to clusterimpl, will change the comment there.

easwars · 2026-02-27T05:04:46Z

-	xdsClient        xdsclient.XDSClient          // xDS client to watch Cluster resources.
-	watchers         map[string]*watcherState     // Set of watchers and associated state, keyed by cluster name.
-	lbCfg            *lbConfig                    // Current load balancing configuration.
+	childLB           balancer.Balancer                     // Child policy, built upon resolution of the cluster graph.


Also, it would be nice to state that all fields below are accessed only from methods on the balancer.Balancer interface. And because grpc guarantees that those methods will never be called concurrently, we don't need any extra serialization for accessing these fields.

easwars · 2026-02-27T05:11:28Z

+			sortedNames := make([]string, len(names))
+			copy(sortedNames, names)


I think this can be replaced with a single line if you use slices.Clone.

easwars · 2026-02-27T05:14:16Z

+			names := req.GetResourceNames()
+			sortedNames := make([]string, len(names))
+			copy(sortedNames, names)
+			sort.Strings(sortedNames)


Nit: Use slices.Sort here as well? gemini says its better than sort.Strings(). I know it doesn't matter here. But whatever.

easwars · 2026-02-27T05:14:38Z

+	const clusterName1 = clusterName + "-cluster-1"
+	const clusterName2 = clusterName + "-cluster-2"
+
+	gotEDSRequests := grpcsync.NewEvent()


This probably needs a better name, and a comment.

easwars · 2026-02-27T05:20:55Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a significant and well-executed refactoring of the xDS implementation, primarily focusing on the CDS balancer. The core change involves centralizing resource watching logic within the dependency manager, which simplifies the CDS balancer by removing its responsibility for managing its own resource watchers. As part of this, the cluster_resolver balancer is completely removed, and the CDS balancer now uses the priority balancer as its child. The error handling for xDS resources has also been improved, making the system more robust. The tests have been thoroughly updated to reflect these architectural changes. My review includes one suggestion to improve the robustness of a test case.

eshitachandwani added 7 commits February 11, 2026 19:54

1st changes

7f1ea5b

working fine here

0f2c145

add eds/dns priority config helper func,minor impr

3c28dbc

remove serializer

d4ca200

minor space fixes

8c76bfe

call unsubs, correct OD update

d382910

merge master

6053ded

eshitachandwani added this to the 1.80 Release milestone Feb 16, 2026

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

change waitForResourceNames to not check sequence

9d5b5b8

eshitachandwani requested a review from easwars February 16, 2026 12:23

eshitachandwani assigned easwars Feb 16, 2026

easwars reviewed Feb 18, 2026

View reviewed changes

easwars assigned eshitachandwani and unassigned easwars Feb 18, 2026

CDS balancer returns error instead of eating error

e8ceb89

eshitachandwani added 2 commits February 20, 2026 14:30

minor improvements

526f16f

minor improvements

f538c3e

eshitachandwani assigned easwars and unassigned eshitachandwani Feb 20, 2026

easwars reviewed Feb 24, 2026

View reviewed changes

easwars assigned eshitachandwani and unassigned easwars Feb 24, 2026

eshitachandwani added 3 commits February 25, 2026 17:20

improve comment and reresolution TF test

f434b9c

fix end ambient error test

09504a5

fix end ambient error test

a904dd4

eshitachandwani added 2 commits February 25, 2026 23:15

fix newline

e09816e

fix a test

4fe0773

eshitachandwani assigned easwars and unassigned eshitachandwani Feb 26, 2026

easwars reviewed Feb 27, 2026

View reviewed changes

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

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

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

Comment thread internal/xds/resolver/xds_resolver.go

minor comment fixes

28b82de

easwars approved these changes Feb 27, 2026

View reviewed changes

easwars assigned eshitachandwani and unassigned easwars Feb 27, 2026

gemini-code-assist Bot reviewed Feb 27, 2026

View reviewed changes

Comment thread internal/xds/balancer/clusterimpl/tests/balancer_test.go

comment changes

700015c

eshitachandwani merged commit 1d4fa8a into grpc:master Mar 2, 2026
14 checks passed

		// Start an xDS management server that pushes the EDS resource names onto a
		// channel when requested.

		// Check if we have a request for both EDS resources. If so, fire
		// the event to unblock the test.

		@@ -168,16 +162,22 @@ type cdsBalancer struct {

		xdsHIPtr *unsafe.Pointer // Accessed atomically.

		sortedNames := make([]string, len(names))
		copy(sortedNames, names)

Conversation

eshitachandwani commented Feb 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eshitachandwani commented Feb 20, 2026

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!

eshitachandwani commented Feb 25, 2026

Uh oh!

easwars left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

easwars left a comment

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!

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!

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

eshitachandwani commented Feb 16, 2026 •

edited

Loading

codecov Bot commented Feb 16, 2026 •

edited

Loading