xdsclient: support fallback within an authority #7701

easwars · 2024-10-04T12:45:10Z

#a71-xds-fallback
Fixes #6902

This PR adds support to fallback to a lower priority server (within an authority) when a higher priority goes down and also adds support to revert to a higher priority server when it comes back up.

This PR also adds e2e style tests for the scenarios specified in the fallback interop test spec.

RELEASE NOTES:

xdsclient: add support to fallback to lower priority servers when higher priority ones are down

codecov · 2024-11-01T16:46:41Z

Codecov Report

Attention: Patch coverage is 68.54839% with 39 lines in your changes missing coverage. Please review.

Project coverage is 81.77%. Comparing base (d66fc3a) to head (72930ff).
Report is 9 commits behind head on master.

Files with missing lines	Patch %	Lines
xds/internal/xdsclient/authority.go	68.54%	28 Missing and 11 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7701      +/-   ##
==========================================
+ Coverage   81.71%   81.77%   +0.05%     
==========================================
  Files         374      373       -1     
  Lines       38166    37844     -322     
==========================================
- Hits        31188    30947     -241     
+ Misses       5699     5597     -102     
- Partials     1279     1300      +21

Files with missing lines	Coverage Δ
xds/internal/xdsclient/client_new.go	`85.71% <ø> (+2.38%)`	⬆️
xds/internal/xdsclient/authority.go	`78.14% <68.54%> (-11.61%)`	⬇️

... and 32 files with indirect coverage changes

xds/internal/xdsclient/authority.go

purnesh42H · 2024-11-05T09:54:05Z

xds/internal/xdsclient/authority.go

+	fallbackChannel := a.xdsChannelConfigs[fallbackServerIdx]
+
+	// If the server to fallback to already has an xdsChannel, it means that
+	// this connectivity error is from a server with a higher priority. There


Didn't quite understand this comment. If there is an error from higher priority server, we should fallback to a lower priority server. How is having an existing channel for fallback server makes any difference?

Lets say the authority has two servers: primary and fallback. We start off with primary, and say it fails and therefore we switch to fallback. And lets say the connection to fallback works and we get all resources from it and we are happy. Now, we get another error from the primary (in fact, we will keep getting stream errors from it since we retry the stream with backoff). At this point, we will see that we already have a channel to the next server in the list which is the fallback server, and therefore we have nothing to do here.

purnesh42H · 2024-11-05T10:07:53Z

xds/internal/xdsclient/authority.go

+// resource that has not yet been cached.
+//
+// Only executed in the context of a serializer callback.
+func (a *authority) uncachedWatcherExists() bool {


nit: name seems to convey "if any uncached watcher exist" where it should convey if there is a watcher that is looking for an uncached resource. May be rename to something like "uncachedResourceExistToWatch" or flip the bool to "allWatchableResourceCached" or simply "areAllRequestedResourceCached"

purnesh42H · 2024-11-05T10:12:27Z

xds/internal/xdsclient/authority.go

+// existing resources.
+//
+// Only executed in the context of a serializer callback.
+func (a *authority) triggerFallbackOnStreamFailure(failingServerConfig *bootstrap.ServerConfig) {


nit: this function is simply falling back and not checking any stream failures. I guess that's done by caller? In that case name can be simply "triggerFallback". Also, in docstring we should mention it switches to next fallback server from the list (i.e. next lower priority than current failing one) and if no more servers below current then it returns as no-op. May be name can be more explicit "switchToNextFallbackServerIfAvailable"?

Decided to go with fallbackToNextServerIfPossible since Available is a term that can be viewed differently in this context, i.e. that the server is actually available for traffic, which is not what we check here.

purnesh42H · 2024-11-05T10:18:17Z

xds/internal/xdsclient/authority.go

+					// unsubscribe, and remove the channel from the list of
+					// channels that this resource is subscribed to.
+					if xc == cfg {
+						state.xdsChannelConfigs = append(state.xdsChannelConfigs[:idx], state.xdsChannelConfigs[idx+1:]...)


can this panic due to idx + 1 because of all these mutations?

Yeah if it hits the last index does the index+1: panic, or is it just like nil or something?

Decided to go with a map instead of a slice and that significantly simplifies this piece of code.

zasweq

Some testing comments

xds/internal/xdsclient/tests/fallback_test.go

zasweq

LGTM

zasweq · 2024-11-05T23:50:58Z

xds/internal/xdsclient/authority.go

+					// unsubscribe, and remove the channel from the list of
+					// channels that this resource is subscribed to.
+					if xc == cfg {
+						xc.xc.unsubscribe(rType, resourceName)


Nit: I still get confused by xc.xc, can we switch the top level symbol to xcc or something

Done, in a few other places as well. Also, I renames the struct fields to be more descriptive.

zasweq · 2024-11-05T23:55:33Z

xds/internal/xdsclient/authority.go

+					// channels that this resource is subscribed to.
+					if xc == cfg {
+						xc.xc.unsubscribe(rType, resourceName)
+						delete(state.xdsChannelConfigs, xc)


These two operations can only hit once per xDS Channel right? I think you can break out of the for once this hits.

Done. Also, ended up inverting the conditional.

easwars force-pushed the xdsclient_ads_stream branch 7 times, most recently from 9c6247d to 0bf5608 Compare October 10, 2024 00:07

purnesh42H added this to the 1.69 Release milestone Oct 16, 2024

easwars force-pushed the xdsclient_ads_stream branch 4 times, most recently from c960691 to ed16d72 Compare October 23, 2024 18:21

easwars force-pushed the xdsclient_ads_stream branch from ed16d72 to 88b1de8 Compare November 1, 2024 16:24

xdsclient: complete refactor and fallback support

4909aa4

easwars force-pushed the xdsclient_ads_stream branch from 88b1de8 to 4909aa4 Compare November 1, 2024 16:35

easwars changed the title ~~xdsclient: complete refactor and fallback support~~ xdsclient: support fallback within an authority Nov 1, 2024

easwars requested review from zasweq and purnesh42H November 1, 2024 16:39

easwars assigned zasweq and purnesh42H Nov 1, 2024

easwars added the Type: Feature New features or improvements in behavior label Nov 1, 2024

easwars marked this pull request as ready for review November 1, 2024 16:40

zasweq requested changes Nov 4, 2024

View reviewed changes

first round of review comments from zasweq

2741b76

purnesh42H reviewed Nov 5, 2024

View reviewed changes

purnesh42H assigned easwars and unassigned purnesh42H Nov 5, 2024

zasweq requested changes Nov 5, 2024

View reviewed changes

more review comments from zasweq and purnesh42H

e23a323

zasweq approved these changes Nov 5, 2024

View reviewed changes

zasweq removed their assignment Nov 5, 2024

use more descriptive field names

72930ff

easwars merged commit b3393d9 into grpc:master Nov 6, 2024
15 checks passed

easwars deleted the xdsclient_ads_stream branch November 6, 2024 19:52

easwars mentioned this pull request Nov 6, 2024

xdsclient: reduce the number of ADS requests #7473

Closed

misvivek pushed a commit to misvivek/grpc-go that referenced this pull request Nov 7, 2024

xdsclient: support fallback within an authority (grpc#7701)

b7f2186

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xdsclient: support fallback within an authority #7701

xdsclient: support fallback within an authority #7701

easwars commented Oct 4, 2024 •

edited

Loading

codecov bot commented Nov 1, 2024 •

edited

Loading

purnesh42H Nov 5, 2024

easwars Nov 5, 2024

purnesh42H Nov 5, 2024

easwars Nov 5, 2024

purnesh42H Nov 5, 2024

easwars Nov 5, 2024

purnesh42H Nov 5, 2024

zasweq Nov 5, 2024

easwars Nov 5, 2024

zasweq left a comment

zasweq left a comment

zasweq Nov 5, 2024

easwars Nov 6, 2024

zasweq Nov 5, 2024

easwars Nov 6, 2024

xdsclient: support fallback within an authority #7701

xdsclient: support fallback within an authority #7701

Conversation

easwars commented Oct 4, 2024 • edited Loading

codecov bot commented Nov 1, 2024 • edited Loading

Codecov Report

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

zasweq left a comment

Choose a reason for hiding this comment

zasweq left a 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 Oct 4, 2024 •

edited

Loading

codecov bot commented Nov 1, 2024 •

edited

Loading