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 81.55620% with 64 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.35%. Comparing base (6ed8acb) to head (09842ae).

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8744      +/-   ##
==========================================
- Coverage   83.39%   83.35%   -0.05%     
==========================================
  Files         419      419              
  Lines       32566    32861     +295     
==========================================
+ Hits        27159    27390     +231     
- Misses       4023     4067      +44     
- Partials     1384     1404      +20

Files with missing lines	Coverage Δ
internal/xds/resolver/xds_resolver.go	`88.76% <100.00%> (+0.12%)`	⬆️
internal/xds/xdsdepmgr/watch_service.go	`54.76% <54.76%> (-30.96%)`	⬇️
internal/xds/xdsdepmgr/xds_dependency_manager.go	`84.15% <85.09%> (-0.75%)`	⬇️

... and 12 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

internal/xds/xdsdepmgr/xds_dependency_manager.go

internal/xds/xdsdepmgr/watch_service.go

internal/xds/xdsdepmgr/xds_dependency_manager.go

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.

internal/xds/xdsdepmgr/xds_dependency_manager.go

internal/xds/xdsdepmgr/watch_service.go

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?

I am not sure if that will add any value , because in the populateCusterConfig we will anyway have to treat is separately

If they have a common interface, why would we have to treat it separately?

internal/xds/resolver/watch_service_test.go

internal/xds/resolver/xds_resolver.go

internal/xds/resolver/xds_resolver_test.go

internal/xds/xdsdepmgr/xds_dependency_manager.go

easwars · 2025-12-15T06:55:22Z

internal/xds/xdsdepmgr/xds_dependency_manager_test.go

 			cmpopts.IgnoreFields(xdsresource.RouteConfigUpdate{}, "Raw"),
+			cmpopts.IgnoreFields(xdsresource.ClusterUpdate{}, "Raw", "LBPolicy", "TelemetryLabels"),
+			cmpopts.IgnoreFields(xdsresource.EndpointsUpdate{}, "Raw"),
+			cmp.Transformer("ErrorsToString", func(in error) string {


Is this transformation meant for cluster errors and endpoint errors? If so, would it make more sense to have a function with the below contents, and have transformers for the specific fields that we are interested in, that invoke the function instead of blindly invoking it on all fields of type error?

Yes, its meant for cluster errors and endpoint errors , and those are the only two error fields in XDSConfig struct. So I dont think it will apply to anything else. And even if we add any error fields later , we might need to do this transformation since we get wrapped errors from XDSConfig

Without having transformers attached to specific fields, it becomes the responsibility of the reader to figure out which fields are affected by the transformation. Either have a comment saying which fields or affected, or change the transformation to apply to specific fields.

Added a comment.

easwars · 2025-12-15T06:56:46Z

internal/xds/xdsdepmgr/xds_dependency_manager_test.go

+	// Safely access the first address string for comparison.
+	addrA := ""
+	if len(a.Addresses) > 0 {
+		addrA = a.Addresses[0].Addr
+	}
+
+	addrB := ""
+	if len(b.Addresses) > 0 {
+		addrB = b.Addresses[0].Addr
+	}


How about using a stable hash and computing a hash for every endpoint and determining the order based on that hash?

Ping on this.

internal/xds/xdsdepmgr/xds_dependency_manager_test.go

easwars · 2025-12-15T06:58:21Z

internal/xds/xdsdepmgr/xds_dependency_manager_test.go

+								{ID: clients.Locality{
+									Region:  "region-1",
+									Zone:    "zone-1",
+									SubZone: "subzone-1",
+								},
+									Endpoints: []xdsresource.Endpoint{
+										{
+											Addresses:    []string{addr},
+											HealthStatus: xdsresource.EndpointHealthStatusUnknown,
+											Weight:       1,
+										},
+									},
+									Weight: 1},


This doesn't seem to be in compliance with this style guidenline: go/go-style/decisions#literal-matching-braces

Please fix.

Doesn't seem to be fixed yet.

internal/xds/xdsdepmgr/xds_dependency_manager_test.go

easwars · 2025-12-15T07:16:52Z

internal/xds/xdsdepmgr/xds_dependency_manager_test.go

+	// Drain the updates because management server keeps sending the error
+	// updates repeatedly causing the update from dependency manager to be
+	// blocked if we don't drain it.
+	select {
+	case <-ctx.Done():
+	case <-watcher.updateCh:
+	}


Since this pattern is used in a bunch of tests, could we have a watcher type that can handle this instead of having the main test goroutine handle this?

internal/xds/xdsdepmgr/xds_dependency_manager_test.go

easwars · 2025-12-16T23:25:55Z

internal/xds/xdsdepmgr/xds_dependency_manager.go

+		clusterConfigs[clusterName] = &xdsresource.ClusterResult{Err: m.annotateErrorWithNodeID(fmt.Errorf("aggregate cluster graph exceeds max depth (%d)", aggregateClusterMaxDepth))}
+		return true, nil, m.annotateErrorWithNodeID(fmt.Errorf("aggregate cluster graph exceeds max depth (%d)", aggregateClusterMaxDepth))


Can this be:

err := m.annotateErrorWithNodeID(fmt.Errorf("aggregate cluster graph exceeds max depth (%d)", aggregateClusterMaxDepth))} clusterConfigs[clusterName] = &xdsresource.ClusterResult{Err: err} return true, nil, err

easwars · 2025-12-16T23:27:43Z

internal/xds/xdsdepmgr/xds_dependency_manager.go

+	// When the cluster update is not one of the three types of cluster updates.
+	clusterConfigs[clusterName] = &xdsresource.ClusterResult{Err: m.annotateErrorWithNodeID(fmt.Errorf("cluster type %v of cluster %s not supported", update.ClusterType, clusterName))}


I think this is better handled as the default case in the switch above, and once we do that we won't even need the return statement here as every branch of the switch will have a return statement of its own.

And when we do that, we won't need this comment either.

easwars · 2025-12-16T23:28:33Z

internal/xds/xdsdepmgr/xds_dependency_manager.go

+	m.maybeSendUpdateLocked()
+}
+
+// RequestDNSReresolution calls all the the DNS resolvers ResolveNow.


s/resolvers/resolver's/

internal/xds/xdsdepmgr/xds_dependency_manager_test.go

easwars · 2025-12-16T23:32:08Z

internal/xds/xdsdepmgr/xds_dependency_manager_test.go

+								{ID: clients.Locality{
+									Region:  "region-1",
+									Zone:    "zone-1",
+									SubZone: "subzone-1",
+								},
+									Endpoints: []xdsresource.Endpoint{
+										{
+											Addresses:    []string{addr},
+											HealthStatus: xdsresource.EndpointHealthStatusUnknown,
+											Weight:       1,
+										},
+									},
+									Weight: 1},


Doesn't seem to be fixed yet.

easwars · 2025-12-16T23:32:15Z

internal/xds/xdsdepmgr/xds_dependency_manager_test.go

+	// Safely access the first address string for comparison.
+	addrA := ""
+	if len(a.Addresses) > 0 {
+		addrA = a.Addresses[0].Addr
+	}
+
+	addrB := ""
+	if len(b.Addresses) > 0 {
+		addrB = b.Addresses[0].Addr
+	}


Ping on this.

easwars · 2025-12-16T23:33:18Z

internal/xds/xdsdepmgr/xds_dependency_manager_test.go

 			cmpopts.IgnoreFields(xdsresource.RouteConfigUpdate{}, "Raw"),
+			cmpopts.IgnoreFields(xdsresource.ClusterUpdate{}, "Raw", "LBPolicy", "TelemetryLabels"),
+			cmpopts.IgnoreFields(xdsresource.EndpointsUpdate{}, "Raw"),
+			cmp.Transformer("ErrorsToString", func(in error) string {


Without having transformers attached to specific fields, it becomes the responsibility of the reader to figure out which fields are affected by the transformation. Either have a comment saying which fields or affected, or change the transformation to apply to specific fields.

easwars · 2025-12-16T23:36:06Z

internal/xds/xdsdepmgr/xds_dependency_manager_test.go

+							{ID: clients.Locality{
+								Region:  "region-1",
+								Zone:    "zone-1",
+								SubZone: "subzone-1",
+							},
+								Endpoints: []xdsresource.Endpoint{
+									{
+										Addresses:    []string{"localhost:8081"},
+										HealthStatus: xdsresource.EndpointHealthStatusUnknown,
+										Weight:       1,
+									},
+								},
+								Weight: 1},


Hasn't been fixed yet.

easwars · 2025-12-16T23:37:39Z

internal/xds/xdsdepmgr/xds_dependency_manager_test.go

+		updateCh: make(chan *xdsresource.XDSConfig),
+		errorCh:  make(chan error),
+	}
+	ctx, cancel := context.WithTimeout(context.Background(), 50*defaultTestTimeout)


If you are increasing it for testing, please change the value of the constant (in one place). This is very easy to miss in a review.

Got it! CHanged.

easwars · 2025-12-17T00:07:56Z

I made a small refactoring commit to split the populateClusterConfigLocked method into smaller ones to handle updates for specific cluster types.

I also added the default case for the switch at the end. Please note that I changed it return true, nil, nil instead of false, nil, nil as it was doing earlier, because in this case, we have received the cluster resource. It just happens to be a type that we don't support. That shouldn't block us from sending an update if everything else in the tree is resolved (since we are anyways storing the error in the cluster config).

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

internal/xds/xdsdepmgr/watch_service.go Outdated Show resolved Hide resolved

easwars reviewed Dec 11, 2025

View reviewed changes

internal/xds/xdsdepmgr/xds_dependency_manager.go Show resolved Hide resolved

easwars reviewed Dec 11, 2025

View reviewed changes

internal/xds/xdsdepmgr/xds_dependency_manager.go Outdated Show resolved Hide resolved

easwars reviewed Dec 11, 2025

View reviewed changes

eshitachandwani added 2 commits December 12, 2025 09:18

resolve comments

e369eeb

resolvenow coment

a98e575

eshitachandwani requested a review from easwars December 12, 2025 05:13

easwars reviewed Dec 12, 2025

View reviewed changes

eshitachandwani added 3 commits December 12, 2025 20:09

report error for resolver error

f04a355

generics try

0da35e0

resolve comments

1de452e

eshitachandwani requested a review from easwars December 12, 2025 18:00

test formatting

5ba419b

easwars reviewed Dec 15, 2025

View reviewed changes

easwars assigned eshitachandwani and unassigned easwars and arjan-bal Dec 15, 2025

changes

d4836bb

eshitachandwani requested a review from easwars December 16, 2025 06:40

eshitachandwani assigned easwars and unassigned eshitachandwani Dec 16, 2025

eshitachandwani added 3 commits December 16, 2025 22:38

Merge branch 'master' into new_cds_eds

9577679

Merge branch 'master' into new_cds_eds

38ef4ea

change addresses fields of xdsresource.Endpoint

229ce3b

easwars reviewed Dec 16, 2025

View reviewed changes

split populateClusterConfigLocked to keep it's size down

6df023c

easwars assigned eshitachandwani and unassigned easwars Dec 17, 2025

eshitachandwani added 4 commits December 17, 2025 08:43

small changes

ad3a3b3

annotate error

ab7a2aa

error test fix

4adea77

use has

09842ae

eshitachandwani requested a review from easwars December 17, 2025 05:51

eshitachandwani assigned easwars and unassigned eshitachandwani Dec 17, 2025

		clusterConfigs[clusterName] = &xdsresource.ClusterResult{Err: m.annotateErrorWithNodeID(fmt.Errorf("aggregate cluster graph exceeds max depth (%d)", aggregateClusterMaxDepth))}
		return true, nil, m.annotateErrorWithNodeID(fmt.Errorf("aggregate cluster graph exceeds max depth (%d)", aggregateClusterMaxDepth))

		// When the cluster update is not one of the three types of cluster updates.
		clusterConfigs[clusterName] = &xdsresource.ClusterResult{Err: m.annotateErrorWithNodeID(fmt.Errorf("cluster type %v of cluster %s not supported", update.ClusterType, clusterName))}

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

easwars commented Dec 11, 2025

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!

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!

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!

codecov bot commented Dec 4, 2025 •

edited

Loading