xdsclient: update watcher API as per gRFC A88 #7977

purnesh42H · 2025-01-02T21:17:12Z

This is the first part of implementing gRFC A88 (grpc/proposal#466).

This introduces the new watcher API but does not change any of the existing behavior. This table summarizes the API changes and behavior for each case:

Case	Old API	New API	Behavior
Resource timer fires	`OnResourceDoesNotExist()`	`OnResourceChanged(NOT_FOUND)`	Fail data plane RPCs
LDS or CDS resource deletion	`OnResourceDoesNotExist()`	`OnResourceChanged(NOT_FOUND)`	Drop resource and fail data plane RPCs
xDS channel reports TRANSIENT_FAILURE	`OnError()`	`OnResourceChanged(status)` if resource NOT already cached; `OnAmbientError(status)` otherwise	Continue using cached resource, if any; otherwise, fail data plane RPCs
ADS stream terminates without receiving a response	`OnError()`	`OnResourceChanged(status)` if resource NOT already cached; `OnAmbientError(status)` otherwise	Continue using cached resource, if any; otherwise, fail data plane RPCs
Invalid resource update (client NACK)	`OnError()`	`OnResourceChanged(status)` if resource NOT already cached; `OnAmbientError(status)` otherwise	Continue using cached resource, if any; otherwise, fail data plane RPCs
Valid resource update	`OnUpdate(resource)`	`OnResourceChanged(resource)`	use the new resource

RELEASE NOTES:

xds: TBD

codecov · 2025-01-06T12:07:59Z

Codecov Report

Attention: Patch coverage is 88.80597% with 15 lines in your changes missing coverage. Please review.

Project coverage is 82.00%. Comparing base (6819ed7) to head (479b7d3).

Files with missing lines	Patch %	Lines
xds/internal/server/listener_wrapper.go	47.05%	7 Missing and 2 partials ⚠️
.../balancer/clusterresolver/resource_resolver_eds.go	82.35%	2 Missing and 1 partial ⚠️
xds/internal/resolver/xds_resolver.go	75.00%	2 Missing ⚠️
xds/internal/testutils/resource_watcher.go	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7977      +/-   ##
==========================================
- Coverage   82.02%   82.00%   -0.02%     
==========================================
  Files         410      410              
  Lines       40233    40250      +17     
==========================================
+ Hits        33000    33007       +7     
- Misses       5865     5872       +7     
- Partials     1368     1371       +3

Files with missing lines	Coverage Δ
internal/xds/bootstrap/bootstrap.go	`65.72% <ø> (ø)`
xds/internal/balancer/cdsbalancer/cdsbalancer.go	`84.59% <100.00%> (-0.04%)`	⬇️
...s/internal/balancer/cdsbalancer/cluster_watcher.go	`100.00% <100.00%> (ø)`
...rnal/balancer/clusterresolver/resource_resolver.go	`94.44% <100.00%> (ø)`
xds/internal/resolver/watch_service.go	`100.00% <100.00%> (+8.57%)`	⬆️
xds/internal/server/rds_handler.go	`86.81% <100.00%> (-0.29%)`	⬇️
xds/internal/xdsclient/authority.go	`80.25% <100.00%> (+0.47%)`	⬆️
xds/internal/xdsclient/clientimpl_watchers.go	`84.81% <100.00%> (+0.39%)`	⬆️
...nal/xdsclient/xdsresource/cluster_resource_type.go	`77.14% <100.00%> (ø)`
...l/xdsclient/xdsresource/endpoints_resource_type.go	`75.00% <100.00%> (ø)`
... and 7 more

... and 19 files with indirect coverage changes

🚀 New features to boost your workflow:

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

xds/internal/xdsclient/xdsresource/resource_type.go

…cks for ambient and resource error

purnesh42H · 2025-03-26T20:23:21Z

@easwars I have made the changes to match the final a88 watcher interface ResourceChanged, ResourceError and AmbientError including the conditions to call ResourceError or AmbientError based on cache.

Everything else is working fine but looks like this TestServeAndCloseDoNotRace is causing race https://github.com/grpc/grpc-go/actions/runs/14091957805/job/39470559995?pr=7977. Server is stopping in the middle of the test. I can't seem to figure how the watcher change is affecting this. Could you help take a look? Thanks.

purnesh42H · 2025-03-26T20:37:41Z

@easwars I have made the changes to match the final a88 watcher interface ResourceChanged, ResourceError and AmbientError including the conditions to call ResourceError or AmbientError based on cache.

Everything else is working fine but looks like this TestServeAndCloseDoNotRace is causing race https://github.com/grpc/grpc-go/actions/runs/14091957805/job/39470559995?pr=7977. Server is stopping in the middle of the test. I can't seem to figure how the watcher change is affecting this. Could you help take a look? Thanks.

Ah, look like it was failing because of calling ResourceError when channel not found in authority.go which was calling ListenerWrapper onLDSResourceError which was switching serving mode. Seems like that's unexpected behavior? Calling AmbientError fixed it. Is it correct to consider channel not found to be AmbientError while registering the watcher?

purnesh42H · 2025-03-27T04:25:03Z

@easwars I have made the changes to match the final a88 watcher interface ResourceChanged, ResourceError and AmbientError including the conditions to call ResourceError or AmbientError based on cache.
Everything else is working fine but looks like this TestServeAndCloseDoNotRace is causing race https://github.com/grpc/grpc-go/actions/runs/14091957805/job/39470559995?pr=7977. Server is stopping in the middle of the test. I can't seem to figure how the watcher change is affecting this. Could you help take a look? Thanks.

Ah, look like it was failing because of calling ResourceError when channel not found in authority.go which was calling ListenerWrapper onLDSResourceError which was switching serving mode. Seems like that's unexpected behavior? Calling AmbientError fixed it. Is it correct to consider channel not found to be AmbientError while registering the watcher?

Actually, I thought over it and right thing to do here is handle this case in listener wrapper. Like previously done, we handle lds error only in case of resource-not-found. Let me know if that's okay.

markdroth · 2025-03-27T15:46:46Z

Ah, look like it was failing because of calling ResourceError when channel not found in authority.go which was calling ListenerWrapper onLDSResourceError which was switching serving mode. Seems like that's unexpected behavior? Calling AmbientError fixed it. Is it correct to consider channel not found to be AmbientError while registering the watcher?

If we start a watch for a resource and the authority of that resource is not found in the bootstrap config, then the right behavior is to report a non-ambient error to the watcher.

In general, we should never call OnAmbientError() for a watcher before we call OnResourceChanged(). If we get a transient error before we've seen a valid resource, then the transient error should be reported to OnResourceChanged() instead of OnAmbientError().

purnesh42H · 2025-03-27T18:05:00Z

Ah, look like it was failing because of calling ResourceError when channel not found in authority.go which was calling ListenerWrapper onLDSResourceError which was switching serving mode. Seems like that's unexpected behavior? Calling AmbientError fixed it. Is it correct to consider channel not found to be AmbientError while registering the watcher?

If we start a watch for a resource and the authority of that resource is not found in the bootstrap config, then the right behavior is to report a non-ambient error to the watcher.

In general, we should never call OnAmbientError() for a watcher before we call OnResourceChanged(). If we get a transient error before we've seen a valid resource, then the transient error should be reported to OnResourceChanged() instead of OnAmbientError().

yeah handled it in ListenerWrapper. Still calling ResourceError for channel not found

markdroth · 2025-03-27T18:42:55Z

yeah handled it in ListenerWrapper

I haven't looked at the code, but that seems wrong to me. There should not need to be any special handling for individual resource types -- the logic for this in XdsClient should be completely resource-type-agnostic.

The only thing that should be handled in individual resource types are things that are different from other resource types, like determining what resource type to pass to the watcher.

purnesh42H · 2025-03-27T19:50:59Z

yeah handled it in ListenerWrapper

I haven't looked at the code, but that seems wrong to me. There should not need to be any special handling for individual resource types -- the logic for this in XdsClient should be completely resource-type-agnostic.

The only thing that should be handled in individual resource types are things that are different from other resource types, like determining what resource type to pass to the watcher.

This error for channel not found while registering watch https://github.com/grpc/grpc-go/pull/7977/files#diff-b4a775482793516a8ed23bd6a58a805d316f5abb6468580691c2b0556e22606aR619 was recently added. Listener wrapper for grpc xds server had a logic to stop serving only if error is of type ResourceNotFound so i just kept that instead of stopping always https://github.com/grpc/grpc-go/pull/7977/files#diff-e4706c72ae912399b7f8ee6f04cec2374ef7a7679b12358f201ddb0b45e34146R453-R457. If we don't do that TestServeAndCloseDoNotRace is causing the race because we are creating and stopping grpc xds servers in a loop . @easwars do you think we should rewrite this test?

markdroth · 2025-03-27T20:24:26Z

Listener wrapper for grpc xds server had a logic to stop serving only if error is of type ResourceNotFound so i just kept that instead of stopping always https://github.com/grpc/grpc-go/pull/7977/files#diff-e4706c72ae912399b7f8ee6f04cec2374ef7a7679b12358f201ddb0b45e34146R453-R457.

I don't think that check is correct -- it should be removed. If the XdsClient delivers an error, it should always be passed through to the watcher.

That's an inherent part of the API change we're making in this PR. The errors that we were previously ignoring in OnError() are now being passed to OnAmbientError() instead. If you are seeing an error reported to OnError(), that means we actually want to drop the resource we saw previously, if any.

easwars · 2025-03-27T21:46:16Z

Yes, I agree that we should not have checks for "resource-not-found" errors in the individual watchers. In fact, I'm thinking we might not even need those error types like xdsresource.ErrorTypeResourceNotFound anymore. If ResourceError is invoked, then the watcher must stop using the previous configuration and if AmbientError is invoked, it should continue using the previous configuration and the xDS client should guarantee that AmbientError is only invoked when a previous copy of the resource was delivered via ResourceChanged.

easwars · 2025-03-27T21:10:43Z

xds/internal/balancer/cdsbalancer/cdsbalancer_test.go

@@ -813,7 +813,7 @@ func (s) TestResolverError(t *testing.T) {

 	// Grab the wrapped connection from the listener wrapper. This will be used
 	// to verify the connection is closed.
-	ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
+	ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout*100000)


Please undo this change.

Just FYI: In a large PR, things like this can be easily missed during the review process. Please be a little more mindful of such changes going forward.

easwars · 2025-03-27T21:20:53Z

xds/internal/xdsclient/metrics_test.go

+			 "server_uri": %q,
+			 "channel_creds": [{"type": "insecure"}]
+		 }]`, mgmtServer.Address)),


It looks like it was indented properly earlier and this change is messing up the indenting.

easwars · 2025-03-27T21:22:42Z

xds/internal/xdsclient/tests/cds_watchers_test.go

 	// When used with a go-control-plane management server that continuously
 	// resends resources which are NACKed by the xDS client, using a `Replace()`
-	// here and in OnResourceDoesNotExist() simplifies tests which will have
+	// here and in ResourceError() simplifies tests which will have


Nit: here and in AmbientError

easwars · 2025-03-27T21:22:59Z

xds/internal/xdsclient/tests/eds_watchers_test.go

 	// When used with a go-control-plane management server that continuously
 	// resends resources which are NACKed by the xDS client, using a `Replace()`
-	// here and in OnResourceDoesNotExist() simplifies tests which will have
+	// here and in ResourceError() simplifies tests which will have


easwars · 2025-03-27T21:23:10Z

xds/internal/xdsclient/tests/lds_watchers_test.go

 	// When used with a go-control-plane management server that continuously
 	// resends resources which are NACKed by the xDS client, using a `Replace()`
-	// here and in OnResourceDoesNotExist() simplifies tests which will have
+	// here and in ResourceError() simplifies tests which will have


easwars · 2025-03-27T21:23:28Z

xds/internal/xdsclient/tests/misc_watchers_test.go

 	// When used with a go-control-plane management server that continuously
 	// resends resources which are NACKed by the xDS client, using a `Replace()`
-	// here and in OnResourceDoesNotExist() simplifies tests which will have
+	// here and in ResourceError() simplifies tests which will have


easwars · 2025-03-27T21:23:35Z

xds/internal/xdsclient/tests/rds_watchers_test.go

 	// When used with a go-control-plane management server that continuously
 	// resends resources which are NACKed by the xDS client, using a `Replace()`
-	// here and in OnResourceDoesNotExist() simplifies tests which will have
+	// here and in ResourceError() simplifies tests which will have


easwars · 2025-03-27T21:25:48Z

xds/internal/xdsclient/tests/resource_update_test.go

@@ -161,7 +161,7 @@ func (s) TestHandleListenerResponseFromManagementServer(t *testing.T) {
 					Value:   []byte{1, 2, 3, 4},
 				}},
 			},
-			wantErr: "Listener not found in received response",
+			wantErr: fmt.Sprintf("xds: resource \"%v\" of type \"ListenerResource\" does not exist", resourceName1),


Would using %q instead of having to do \"%v\" make this easier to read?

Same with \"ListenerResource\". You could do a %q in the format string and include "ListenerResource" as one of the arguments to fmt.Sprintf.

easwars · 2025-03-27T21:29:48Z

xds/internal/xdsclient/xdsresource/resource_type.go

+// of the update has completed.  For example, if processing a resource requires
+// watching new resources, those watches should be completed before done is
+// called, which can happen after the ResourceWatcher method has returned.


Could you please simplify/clarify/rephrase this sentence:

// For example, if processing a resource requires
// watching new resources, those watches should be completed before done is
// called, which can happen after the ResourceWatcher method has returned.

Specifically, what do you mean by "those watches should be completed before done is called"? I believe, just registration of the watch, right? So, that needs to be clarified.

And I don't understand what "which can happen after the ResourceWatcher method has returned" this means.

easwars · 2025-03-27T21:50:16Z

Everything else is working fine but looks like this TestServeAndCloseDoNotRace is causing race grpc/grpc-go/actions/runs/14091957805/job/39470559995?pr=7977. Server is stopping in the middle of the test.

Sorry, I don't have the cycles to debug this failure. But if you can dig a little deeper and get more info on the sequence of events leading to the failure, and why exactly the flake is happening, I might be able to help better. Once we have that information, we will be able to decide if we need to rewrite the test or if there is something wrong we are doing in the xDS client. Thanks.

purnesh42H added Type: Feature New features or improvements in behavior Area: xDS Includes everything xDS related, including LB policies used with xDS. labels Jan 2, 2025

purnesh42H added this to the 1.70 Release milestone Jan 2, 2025

purnesh42H self-assigned this Jan 2, 2025

purnesh42H force-pushed the a88-watcher-api branch 7 times, most recently from 8d198b7 to a9b45c0 Compare January 3, 2025 16:28

purnesh42H requested a review from markdroth January 3, 2025 16:58

purnesh42H assigned dfawley and markdroth and unassigned purnesh42H Jan 3, 2025

purnesh42H requested a review from dfawley January 3, 2025 16:59

purnesh42H force-pushed the a88-watcher-api branch 2 times, most recently from 4009e3e to 57dbf23 Compare January 6, 2025 12:03

purnesh42H force-pushed the a88-watcher-api branch 4 times, most recently from 43c9adb to 89f475a Compare January 6, 2025 12:26

xdsclient: update watcher API as per gRFC A88

a48b5bb

purnesh42H force-pushed the a88-watcher-api branch from 89f475a to a48b5bb Compare January 6, 2025 12:35

purnesh42H requested a review from easwars January 7, 2025 07:14

purnesh42H assigned easwars Jan 7, 2025

purnesh42H added 2 commits January 8, 2025 22:52

revert notififying watchers on stream close if one response is received

89caa23

revert notifying watcher if previous updated deleted it

25dce25

easwars reviewed Jan 9, 2025

View reviewed changes

xds/internal/xdsclient/xdsresource/resource_type.go Outdated Show resolved Hide resolved

xds/internal/xdsclient/xdsresource/resource_type.go Outdated Show resolved Hide resolved

xds/internal/xdsclient/xdsresource/resource_type.go Outdated Show resolved Hide resolved

arjan-bal modified the milestones: 1.71 Release, 1.72 Release Feb 19, 2025

purnesh42H added 2 commits March 26, 2025 22:21

change based on final resource watcher interface with separate callba…

726ce38

…cks for ambient and resource error

merge with main branch

b7f1007

purnesh42H force-pushed the a88-watcher-api branch from 1815062 to b320a7d Compare March 26, 2025 19:11

fix some tests

33280a0

purnesh42H force-pushed the a88-watcher-api branch from b320a7d to 33280a0 Compare March 26, 2025 19:34

ambient error when channel not found

466d5fe

purnesh42H requested a review from easwars March 26, 2025 20:41

purnesh42H assigned easwars and unassigned purnesh42H Mar 26, 2025

handle lds error only for resource-not-found in listener wrapper

479b7d3

purnesh42H assigned dfawley Mar 27, 2025

easwars reviewed Mar 27, 2025

View reviewed changes

easwars assigned purnesh42H and unassigned easwars and dfawley Mar 27, 2025

purnesh42H mentioned this pull request Apr 2, 2025

xds: generic xds client resource watching e2e #8183

Open

eshitachandwani modified the milestones: 1.72 Release, 1.73 Release Apr 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xdsclient: update watcher API as per gRFC A88 #7977

xdsclient: update watcher API as per gRFC A88 #7977

purnesh42H commented Jan 2, 2025 •

edited

Loading

codecov bot commented Jan 6, 2025 •

edited

Loading

purnesh42H commented Mar 26, 2025

purnesh42H commented Mar 26, 2025 •

edited

Loading

purnesh42H commented Mar 27, 2025

markdroth commented Mar 27, 2025

purnesh42H commented Mar 27, 2025

markdroth commented Mar 27, 2025

purnesh42H commented Mar 27, 2025

markdroth commented Mar 27, 2025

easwars commented Mar 27, 2025

easwars Mar 27, 2025

easwars Mar 27, 2025

easwars Mar 27, 2025

easwars Mar 27, 2025

easwars Mar 27, 2025

easwars Mar 27, 2025

easwars Mar 27, 2025

easwars Mar 27, 2025

easwars Mar 27, 2025

easwars commented Mar 27, 2025

xdsclient: update watcher API as per gRFC A88 #7977

Are you sure you want to change the base?

xdsclient: update watcher API as per gRFC A88 #7977

Conversation

purnesh42H commented Jan 2, 2025 • edited Loading

codecov bot commented Jan 6, 2025 • edited Loading

Codecov Report

purnesh42H commented Mar 26, 2025

purnesh42H commented Mar 26, 2025 • edited Loading

purnesh42H commented Mar 27, 2025

markdroth commented Mar 27, 2025

purnesh42H commented Mar 27, 2025

markdroth commented Mar 27, 2025

purnesh42H commented Mar 27, 2025

markdroth commented Mar 27, 2025

easwars commented Mar 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars commented Mar 27, 2025

purnesh42H commented Jan 2, 2025 •

edited

Loading

codecov bot commented Jan 6, 2025 •

edited

Loading

purnesh42H commented Mar 26, 2025 •

edited

Loading