xds: Add cluster endpoint watchers to depndency manager #8744

eshitachandwani · 2025-12-04T10:36:07Z

This is part of A74 implementation which add CDS/EDS/DNS watchers to the dependency manager. It also adds a temporary flag that is disabled by default so that it is not used in the current RPC paths , but enabled in the dependency manager tests.

RELEASE NOTES: None

codecov · 2025-12-04T11:13:38Z

Codecov Report

❌ Patch coverage is 77.00348% with 66 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.32%. Comparing base (4c27cc8) to head (4b86074).
⚠️ Report is 11 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/xds/xdsdepmgr/xds_dependency_manager.go	80.41%	31 Missing and 16 partials ⚠️
internal/xds/xdsdepmgr/watch_service.go	59.57%	16 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8744      +/-   ##
==========================================
+ Coverage   83.28%   83.32%   +0.04%     
==========================================
  Files         418      418              
  Lines       32367    32663     +296     
==========================================
+ Hits        26958    27218     +260     
- Misses       4034     4061      +27     
- Partials     1375     1384       +9

Files with missing lines	Coverage Δ
internal/xds/xdsdepmgr/watch_service.go	`69.33% <59.57%> (-16.39%)`	⬇️
internal/xds/xdsdepmgr/xds_dependency_manager.go	`81.89% <80.41%> (-3.01%)`	⬇️

... and 30 files with indirect coverage changes

🚀 New features to boost your workflow:

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

internal/xds/xdsdepmgr/xds_dependency_manager.go

internal/xds/xdsdepmgr/watch_service.go

easwars · 2025-12-09T05:44:55Z

internal/xds/xdsdepmgr/xds_dependency_manager.go

+	// We want to stop using this cluster as we received a resource error so
+	// remove the last update.


Nit: Only onListenerResourceAmbientError and onRouteConfigResourceAmbientError currently have docstrings. A comment like this makes more sense to be part of the docstring (for all watchers). But as they will be repeated for all watchers, please keep it short and sweet.

internal/xds/xdsdepmgr/xds_dependency_manager.go

easwars · 2025-12-11T06:33:26Z

internal/xds/xdsdepmgr/watch_service.go

 	}
 }
+
+type clusterWatcher struct {


Why is it that the listener and route watchers store the cancel func inside themselves, while the cluster and endpoints watchers don't?

And a result of this difference the LDS/RDS watchers have a stop method, while the CDS/EDS watchers don't.

easwars · 2025-12-11T06:43:44Z

internal/xds/xdsdepmgr/xds_dependency_manager.go

+		}
+
+		m.clustersFromRouteConfig = newClusters
+	}


Why do we not create new cluster watchers from here since we know the list of clusters at this point?

easwars · 2025-12-11T06:44:28Z

internal/xds/xdsdepmgr/xds_dependency_manager.go

+	}
+}
+
+func (m *DependencyManager) populateClusterConfigLocked(name string, depth int, clusterMap map[string]*xdsresource.ClusterResult, edsSeen, dnsSeen, clusterSeen map[string]bool) (bool, []string, error) {


This method accepts a bunch of arguments, returns a bunch of values, and has complex logic in it. This needs to have a good docstring to help the reader.

Also, can we have better variable names for the arguments.

s/name/clusterName

What does clusterMap really represent? This needs a more descriptive name.

s/clusterSeen/clustersSeen etc

easwars · 2025-12-11T08:01:23Z

Can you also please check the codecov report here: #8744 (comment) to see if there are any valid lines that are missing test coverage. Thanks.

easwars · 2025-12-11T08:10:20Z

internal/xds/xdsdepmgr/xds_dependency_manager.go

+	}
+}
+
+func (m *DependencyManager) populateClusterConfigLocked(name string, depth int, clusterMap map[string]*xdsresource.ClusterResult, edsSeen, dnsSeen, clusterSeen map[string]bool) (bool, []string, error) {


Also, can we have better variable names for the arguments.

s/name/clusterName

What does clusterMap really represent? This needs a more descriptive name.

s/clusterSeen/clustersSeen etc

easwars · 2025-12-11T08:13:07Z

internal/xds/xdsdepmgr/xds_dependency_manager.go

+		return false, nil, nil
+	}
+
+	state := m.clusterWatchers[name]


You could avoid another map lookup by doing

state, ok := m.clusterWatchers[name]; if !ok { ... }

easwars · 2025-12-11T08:13:42Z

internal/xds/xdsdepmgr/xds_dependency_manager.go

+	// If update is not yet received for this cluster, return.
+	if state.lastUpdate == nil && state.lastErr == nil {
+		return false, nil, nil
+	}
+
+	// If resource error is seen for this cluster, save it in the cluster map
+	// and return.
+	if state.lastUpdate == nil && state.lastErr != nil {
+		clusterMap[name] = &xdsresource.ClusterResult{Err: state.lastErr}
+		return true, nil, nil
+	}
+
+	update := state.lastUpdate
+	clusterMap[name] = &xdsresource.ClusterResult{
+		Config: xdsresource.ClusterConfig{
+			Cluster: update,
+		},
+	}


Can this be changed to a switch because it looks like all of this is handling different cases for an update/error being already present.

easwars · 2025-12-11T08:16:12Z

internal/xds/xdsdepmgr/xds_dependency_manager.go

+		var edsName string
+		if update.EDSServiceName == "" {
+			edsName = name
+		} else {
+			edsName = update.EDSServiceName
+		}


This can be shortened as follows:

edsName := clusterName if update.EDSServiceName != "" { edsName = update.EDSServiceName }

easwars · 2025-12-11T08:24:54Z

internal/xds/xdsdepmgr/watch_service.go

+}
+
+func (dr *dnsResolver) NewAddress(addresses []resolver.Address) {
+	dr.UpdateState(resolver.State{Addresses: addresses})


We know for a fact that our DNS resolver implementation does not call NewAddresses. Should we panic here instead?

easwars · 2025-12-11T08:25:33Z

internal/xds/xdsdepmgr/watch_service.go

+}
+
+func (dr *dnsResolver) ParseServiceConfig(string) *serviceconfig.ParseResult {
+	return &serviceconfig.ParseResult{Err: fmt.Errorf("service config not supported")}


I understand that this is existing behavior, but do you happen to know if this is what other languages do as well?

easwars · 2025-12-11T08:26:28Z

internal/xds/xdsdepmgr/watch_service.go

+
+// dnsResolverState watches updates for the given DNS hostname. It implements
+// resolver.ClientConn interface to work with the DNS resolver.
+type dnsResolver struct {


Can we make an interface that both the endpointsWatcher and the dnsWatcher can satisfy, so that the dependency manager can treat them as equals?

cds/eds

8ed3eef

eshitachandwani added this to the 1.78 Release milestone Dec 4, 2025

eshitachandwani added the Type: Internal Cleanup Refactors, etc label Dec 4, 2025

eshitachandwani added 5 commits December 4, 2025 16:10

change

a2479e4

spelling

ae5251b

spelling

b8a6507

all correct

4084ae5

order not checked

4862844

mbissa assigned eshitachandwani Dec 9, 2025

easwars reviewed Dec 9, 2025

View reviewed changes

eshitachandwani added 3 commits December 9, 2025 16:39

1st set of comment

15cc42e

change set to use bool in place of struct

ed3c1eb

comment lengths

4b86074

eshitachandwani requested a review from easwars December 9, 2025 18:24

eshitachandwani assigned easwars and unassigned eshitachandwani Dec 9, 2025

eshitachandwani requested a review from arjan-bal December 11, 2025 04:03

eshitachandwani assigned arjan-bal Dec 11, 2025

eshitachandwani modified the milestones: 1.78 Release, 1.79 Release Dec 11, 2025

easwars reviewed Dec 11, 2025

View reviewed changes

		// We want to stop using this cluster as we received a resource error so
		// remove the last update.

xds: Add cluster endpoint watchers to depndency manager #8744

Are you sure you want to change the base?

xds: Add cluster endpoint watchers to depndency manager #8744

Conversation

eshitachandwani commented Dec 4, 2025

Uh oh!

codecov bot commented Dec 4, 2025 • 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!

Choose a reason for hiding this comment

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!

easwars commented Dec 11, 2025

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Dec 4, 2025 •

edited

Loading