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 80.76923% with 30 lines in your changes missing coverage. Please review.

Project coverage is 82.45%. Comparing base (724f450) to head (516a01f).
Report is 41 commits behind head on master.

Files with missing lines	Patch %	Lines
xds/internal/testutils/resource_watcher.go	20.00%	11 Missing and 1 partial ⚠️
xds/internal/server/listener_wrapper.go	42.10%	9 Missing and 2 partials ⚠️
.../balancer/clusterresolver/resource_resolver_eds.go	78.57%	2 Missing and 1 partial ⚠️
xds/internal/resolver/xds_resolver.go	66.66%	2 Missing ⚠️
xds/internal/xdsclient/clientimpl_watchers.go	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7977      +/-   ##
==========================================
+ Coverage   82.28%   82.45%   +0.17%     
==========================================
  Files         381      388       +7     
  Lines       38539    39082     +543     
==========================================
+ Hits        31712    32227     +515     
  Misses       5535     5535              
- Partials     1292     1320      +28

Files with missing lines	Coverage Δ
xds/internal/balancer/cdsbalancer/cdsbalancer.go	`83.33% <100.00%> (+1.58%)`	⬆️
...s/internal/balancer/cdsbalancer/cluster_watcher.go	`100.00% <100.00%> (ø)`
xds/internal/resolver/watch_service.go	`100.00% <100.00%> (+8.57%)`	⬆️
xds/internal/server/rds_handler.go	`90.69% <100.00%> (-0.61%)`	⬇️
xds/internal/xdsclient/authority.go	`77.21% <100.00%> (+0.34%)`	⬆️
...nal/xdsclient/xdsresource/cluster_resource_type.go	`78.37% <100.00%> (+1.23%)`	⬆️
...l/xdsclient/xdsresource/endpoints_resource_type.go	`76.47% <100.00%> (+1.47%)`	⬆️
...al/xdsclient/xdsresource/listener_resource_type.go	`86.44% <100.00%> (+0.47%)`	⬆️
...ds/internal/xdsclient/xdsresource/resource_type.go	`100.00% <ø> (ø)`
...dsclient/xdsresource/route_config_resource_type.go	`76.47% <100.00%> (+1.47%)`	⬆️
... and 5 more

... and 54 files with indirect coverage changes

xds/internal/xdsclient/xdsresource/resource_type.go

easwars · 2025-01-10T19:09:43Z

xds/internal/xdsclient/xdsresource/resource_type.go

+	// will never be nil together.
+	//
+	// Watcher is expected to use the most recent value passed to
+	// OnResourceChanged(), regardless of whether that's a resource or an error


Nit: s/ whether that's a resource/whether that's a ResourceData/

xds/internal/xdsclient/xdsresource/resource_type.go

xds/internal/xdsclient/xdsresource/cluster_resource_type.go

xds/internal/xdsclient/xdsresource/endpoints_resource_type.go

xds/internal/xdsclient/xdsresource/listener_resource_type.go

xds/internal/resolver/xds_resolver.go

xds/internal/balancer/clusterresolver/resource_resolver_eds.go

xds/internal/balancer/cdsbalancer/cdsbalancer.go

easwars · 2025-01-29T19:22:23Z

xds/internal/xdsclient/xdsresource/resource_type.go

@@ -58,27 +58,58 @@ type Producer interface {
 // from the xDS server.
 type OnDoneFunc func()

+// ResourceDataOrError is a struct that contains either ResourceData or error.


Please skip the "is a struct" part.

easwars · 2025-01-29T19:26:14Z

xds/internal/xdsclient/xdsresource/resource_type.go

+	// It is invoked under different error conditions including but not
 	// limited to the following:
-	//	- authority mentioned in the resource is not found
-	//	- resource name parsing error
-	//	- resource deserialization error
-	//	- resource validation error
-	//	- ADS stream failure
-	//	- connection failure
-	OnError(error, OnDoneFunc)
-
-	// OnResourceDoesNotExist is invoked for a specific error condition where
-	// the requested resource is not found on the xDS management server.
-	OnResourceDoesNotExist(OnDoneFunc)
+	//      - authority mentioned in the resource is not found
+	//      - resource name parsing error
+	//      - resource validation error (if resource is not cached)
+	//      - ADS stream failure (if resource is not cached)
+	//      - connection failure (if resource is not cached)


I feel that the number of cases for which OnResourceChanged() can be invoked are way too many now to be listing here. So, I feel we can get rid of this block completely. Wdyt?

I have removed it. I have mentioned gRFC A88 at the top to refer for exhaustive list. I saw other places in code where we just mention the grfc and don't link. Also, the proposal is not merged yet.

One more i felt worth mentioning is about whether to continue reusing the existing resource or not. For OnResourceChanged(), they should stop using it.

easwars · 2025-01-29T19:28:31Z

xds/internal/xdsclient/xdsresource/resource_type.go

+	// If resource is already cached, it is invoked under different error
+	// conditions including but not limited to the following:
+	//      - resource validation error
+	//      - ADS stream failure
+	//      - connection failure


This also depends on a number of things like the server feature being set etc. So, I think we can get rid of this block as well. Somewhere in ResourceWatcher documentation, we should link to A88 (probably at the top) and say that it contains an exhaustive list of what method is invoked under what conditions.

Please update the documentation of the individual watchers as well based on the two comments here. Thanks.

I have removed it. I have mentioned gRFC A88 at the top to refer for exhaustive list. I saw other places in code where we just mention the grfc and don't link. Also, the proposal is not merged yet.

One more i felt worth mentioning is about whether to continue reusing the existing resource or not. For OnAmbientError(), they may continue using it.

purnesh42H · 2025-01-31T11:15:25Z

@easwars ptal

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 Show resolved Hide resolved

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

easwars assigned purnesh42H and unassigned easwars Jan 9, 2025

update OnResourceChanged() param to ResourceDataOrErr

4c7a204

purnesh42H requested a review from easwars January 10, 2025 16:35

purnesh42H assigned easwars and unassigned purnesh42H Jan 10, 2025

easwars reviewed Jan 10, 2025

View reviewed changes

easwars assigned purnesh42H and unassigned easwars, markdroth and dfawley Jan 10, 2025

easwars review 2

1fdeb0d

purnesh42H force-pushed the a88-watcher-api branch from 385ff44 to 1fdeb0d Compare January 13, 2025 20:22

purnesh42H requested a review from easwars January 13, 2025 20:23

purnesh42H assigned easwars and unassigned purnesh42H Jan 13, 2025

arjan-bal modified the milestones: 1.70 Release, 1.71 Release Jan 22, 2025

easwars reviewed Jan 30, 2025

View reviewed changes

easwars assigned purnesh42H and unassigned easwars Jan 30, 2025

update ResourceWatcher documentation

516a01f

purnesh42H requested a review from easwars January 31, 2025 11:15

purnesh42H assigned easwars and unassigned purnesh42H Jan 31, 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

easwars Jan 10, 2025

purnesh42H Jan 13, 2025

easwars Jan 29, 2025

purnesh42H Jan 31, 2025

easwars Jan 29, 2025

purnesh42H Jan 31, 2025 •

edited

Loading

easwars Jan 29, 2025

easwars Jan 29, 2025

purnesh42H Jan 31, 2025 •

edited

Loading

purnesh42H commented Jan 31, 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

easwars Jan 10, 2025

Choose a reason for hiding this comment

purnesh42H Jan 13, 2025

Choose a reason for hiding this comment

easwars Jan 29, 2025

Choose a reason for hiding this comment

purnesh42H Jan 31, 2025

Choose a reason for hiding this comment

easwars Jan 29, 2025

Choose a reason for hiding this comment

purnesh42H Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

easwars Jan 29, 2025

Choose a reason for hiding this comment

easwars Jan 29, 2025

Choose a reason for hiding this comment

purnesh42H Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

purnesh42H commented Jan 31, 2025

purnesh42H commented Jan 2, 2025 •

edited

Loading

codecov bot commented Jan 6, 2025 •

edited

Loading

purnesh42H Jan 31, 2025 •

edited

Loading

purnesh42H Jan 31, 2025 •

edited

Loading