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 4 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.

easwars · 2025-12-09T05:17:11Z

internal/xds/xdsdepmgr/xds_dependency_manager.go


+// EnableCDSEDSlooksups is a flag used to control whether the CDS/EDS watchers in
+// the dependency manager should be used. It is made false by default.
+var EnableCDSEDSlooksups = false


Does this need to be exported?

Also, the CDSEDS part is really hard to read. Maybe something like enableClusterAndEndpointsWatch?

Changed the name. And yes, it needs to be exported since we are turning it on in tests which we have in the package xdsdepmgr_test. It will anyway be removed when the next PR goes in.

easwars · 2025-12-09T05:17:38Z

internal/xds/xdsdepmgr/xds_dependency_manager.go

 var logger = grpclog.Component("xds")

+// EnableCDSEDSlooksups is a flag used to control whether the CDS/EDS watchers in
+// the dependency manager should be used. It is made false by default.


Nit: Skip the It is made false by default since it is evident that it it being set to false in the line below this.

easwars · 2025-12-09T05:18:14Z

internal/xds/xdsdepmgr/xds_dependency_manager.go

+	rdsResourceName        string
+	currentRouteConfig     *xdsresource.RouteConfigUpdate
+	currentVirtualHost     *xdsresource.VirtualHost
+	clusterFromRouteConfig map[string]struct{}


Nit: s/clusterFromRouteConfig/clustersFromRouteConfig

Also, if this is supposed to be a Set, the idiomatic Go way of defining a Set is to have a map with the value type being bool and not struct{}.

Ohh... I always thought using struct{} as value was the idiomatic type, since it is more memory efficient, but changed.

internal/xds/xdsdepmgr/xds_dependency_manager.go

easwars · 2025-12-09T05:21:05Z

internal/xds/xdsdepmgr/xds_dependency_manager.go

+	listenerWatcher        *listenerWatcher
+	currentListenerUpdate  *xdsresource.ListenerUpdate
+	routeConfigWatcher     *routeConfigWatcher
+	rdsResourceName        string
+	currentRouteConfig     *xdsresource.RouteConfigUpdate
+	currentVirtualHost     *xdsresource.VirtualHost


Nit: Prior to the new fields for cds/eds watchers and updates, the fields were organized as follows:

xxxWatcher

xxxRelatedUpdate1

xxxRelatedUpdate2

yyyWatcher

yyyRelatedUpdate1

yyyRelatedUpdate2

The newly added ones are mixed up. Could we retain the same order for fields. Thanks.

easwars · 2025-12-09T05:30:19Z

internal/xds/xdsdepmgr/watch_service.go

Hmm ... why did we decide to call this file watch_service.go? Maybe, in a follow-up, we should rename this to watchers.go as it now contains all watchers.

The file containing LDS RDS in the xds_resolver was called watch_service.go , so I just reused that. Will change it in a later PR

easwars · 2025-12-09T05:30:30Z

internal/xds/xdsdepmgr/watch_service.go

 }
+
+type clusterWatcher struct {
+	name   string


Please rename this field to resourceName to match the listenerWatcher and the routeConfigWatcher.

easwars · 2025-12-09T05:31:12Z

internal/xds/xdsdepmgr/watch_service.go

+	e.depMgr.onClusterAmbientError(e.name, err, onDone)
+}
+
+// clusterWatcherState groups the state associated with a clusterWatcher.


There are no comments on the listener and routeConfig watchers. Maybe, we can follow the same approach, since the watcher types are not very interesting.

easwars · 2025-12-09T05:31:32Z

internal/xds/xdsdepmgr/watch_service.go

+	cancelWatch func()          // Cancel func to cancel the watch.
+	// Most recent update received for this cluster.
+	lastUpdate *xdsresource.ClusterUpdate
+	err        error


Should this also be called lastErr? or something more descriptive?

easwars · 2025-12-09T05:31:53Z

internal/xds/xdsdepmgr/watch_service.go

+	}
+}
+
+type endpointWatcher struct {


s/endpointWatcher/endpointsWatcher/

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-09T05:47:15Z

internal/xds/xdsdepmgr/xds_dependency_manager.go


+// EnableCDSEDSlooksups is a flag used to control whether the CDS/EDS watchers in
+// the dependency manager should be used. It is made false by default.
+var EnableCDSEDSlooksups = false


Also, the CDSEDS part is really hard to read. Maybe something like enableClusterAndEndpointsWatch?

easwars · 2025-12-09T05:54:23Z

internal/xds/xdsdepmgr/xds_dependency_manager.go

+	// Get the clusters to be watched from the routes in the virtual host. If
+	// the CLusterSpecifierField is set, we ignore it for now as the clusters
+	// will be determined dynamically for it.
+	newClusters := make(map[string]struct{})
+
+	for _, rt := range matchVH.Routes {
+		for _, cluster := range rt.WeightedClusters {
+			newClusters[cluster.Name] = struct{}{}
+		}
+	}


Should this move into the flag protected block as well?

Yes, missed that. Thank you! Changed.

easwars · 2025-12-09T05:57:20Z

internal/xds/xdsdepmgr/xds_dependency_manager.go

 		m.routeConfigWatcher.stop()
 	}
+	for name, cluster := range m.clusterWatchers {
+		go cluster.cancelWatch()


Why are these cancels in a goroutine? If there is a valid reason, it needs to be documented. And, we need to ensure that we wait for those goroutines to complete before returning from Close.

It was just something I was trying out. Removing it.

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

		// 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!

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!

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!

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!

2 participants

codecov bot commented Dec 4, 2025 •

edited

Loading