Xds fallback #11254

larry-safran · 2024-06-03T22:19:13Z

Implementation of https://github.com/grpc/proposal/blob/master/A71-xds-fallback.md in Java

YifeiZhuang

This is good overall. I finally start to understand the spirit of it.

xds/src/main/java/io/grpc/xds/SharedXdsClientPoolProvider.java

xds/src/main/java/io/grpc/xds/XdsServerWrapper.java

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

xds/src/test/java/io/grpc/xds/XdsClientFallbackTest.java

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

YifeiZhuang · 2024-07-03T23:35:25Z

xds/src/main/java/io/grpc/xds/client/XdsClient.java

+     */
+    default void assignResourcesToOwner(XdsResourceType<?> type, Collection<String> resources,
+                                        Object owner) {
+      // no-op - useful for test cases where everything is mocked


nit:I'm confused by the comment.

When everything is mocked there is no reason to do anything, so the cases where you want your test to implement ResourceStore you don't need to explicitly define this method that you don't want to use.

xds/src/test/java/io/grpc/xds/XdsClientFallbackTest.java

YifeiZhuang · 2024-07-03T23:43:20Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

+    }
+
+    @Override
+    public void handleStreamReady(ServerInfo serverInfo) {


Is the onReady() callback also supported by stubby xdsTransportFactory? @ejona86

They have to have something that recognizes that a stream has come ready that can feed into these calls.

YifeiZhuang · 2024-08-22T22:06:34Z

xds/src/main/java/io/grpc/xds/client/ControlPlaneClient.java

-    if (resources != null) {
+
+    Collection<String> resources =
+        resourceStore.getSubscribedResources(serverInfo, resourceType, authority);


targetA(channelA) ----> xdsClientA resource1 (authority1) --> subscriber1 authority1 : [serverInfo1, serverInfo2] controlPlaneClient ---> serverInfo1 ---> serverInfo2 resource2 (authority2) --> subscriber2 authority2 : [serverInfo3, serverInfo4] controlPlaneClient ---> serverInfo3 ---> serverInfo4 resource3 (authority1) --> subscriber3 authority1 : [serverInfo1, serverInfo2] ---> serverInfo1 controlPlaneClient ---> serverInfo2(never happens) targetA(channelB) ----> xdsClientB resource1 (authority1) --> subscriber1 ...
Each xdsClinet is an instance and has its own the resourceSubscriber map, and separate ads streams for it's resources.
The resources to send request for in this adstream adsStream.sendDiscoveryReuqest should be the resources with the authority the same as in this adsStream.

xds/src/main/java/io/grpc/xds/client/XdsClient.java

…ority argument.

…d completely disjoint resource names to assuming that any duplicate names would be compatible, particularly cross authority.

larry-safran · 2024-09-05T01:51:36Z

Fallback is ready for review again.

YifeiZhuang

Sending what I have, mostly minor, I haven't looked deep enough.

YifeiZhuang · 2024-09-10T23:50:30Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

  @Nullable
  @Override
-  public Collection<String> getSubscribedResources(ServerInfo serverInfo,
-                                                   XdsResourceType<? extends ResourceUpdate> type) {
+  public Collection<String> getSubscribedResources(


Can we simplify this method to something like this?

resources.entrySet().filter( e -> e.serverInfos.contains(serverInfo)).collect...

I went with the authority approach instead of the ServerInfo because fallback is per authority, so you can have a situation where one authority was complete and another did fallback and requests resources, the first authority's resources would include the fallback server's ServerInfo. We don't want to get an update from the fallback server for the first authority so we don't want to request its resources.

census/src/test/java/io/grpc/census/CensusModulesTest.java

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

ejona86

Sending what I have.

xds/src/main/java/io/grpc/xds/GrpcXdsTransportFactory.java

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

xds/src/main/java/io/grpc/xds/client/XdsClient.java

xds/src/main/java/io/grpc/xds/GrpcXdsTransportFactory.java

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

…figure out the current CPC. Add all live ControlPlaneClients to active list rather than skipping ones in backoff. Changed isConnected() logic to be based on AdsStream call's success or failure

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

xds/src/test/java/io/grpc/xds/GrpcBootstrapperImplTest.java

ejona86 · 2024-10-31T15:22:14Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

+      }
+
+      for (String authority : getAuthoritiesForServerInfo(serverInfo)) {
+        if (controlPlaneClient.hasSubscribedResources(authority)) {


This looks suspicious. There's nothing related to the controlPlaneClient in this method. If we need this, it should just be a private method in XdsClientImpl/resourceStore. But it looks like implying it has something to do with the controlPlaneClient is causing this code to make a misleading decision.

Moved the checking for subscribed resources out of ControlPlaneClient as this is the only place it is used. I believe it originated from some idea of the ControlPlaneClient knowing which authorities it supported and therefore having ownership of the logic, but the code evolved so it no longer made sense.

xds/src/main/java/io/grpc/xds/GrpcXdsTransportFactory.java

ejona86 · 2024-10-31T16:06:02Z

Ugh. Github sent the review before I was done with it. Looking over the sent comments, I think they make sense alone/aren't wrong. But I'm still working on the review.

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

ejona86 · 2024-10-31T18:02:13Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

          args.nonce);
+      if (controlPlaneClient.isStartingUp()) {
+        controlPlaneClient.startingUpCompleted();
+        shutdownLowerPriorityCpcs(controlPlaneClient);


This looks wrong being only for if errors.isEmpty(). Why is it dependent on whether we are ACKing or NACKing?

ejona86 · 2024-10-31T18:09:14Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

+    private void internalHandleStreamReady(
+        ServerInfo serverInfo, ControlPlaneClient controlPlaneClient, String authority) {
+      ControlPlaneClient activeCpClient = getActiveCpc(authority);
+      addCpcToAuthority(authority, controlPlaneClient);


Why are we adding to the list here? Shouldn't we have done that in fallBackToCpc()? It looks like if it isn't registered for the authority yet, then that means it shouldn't be added because a higher priority server is being used for that authority.

Using getAuthoritiesForServerInfo() before getting here looks to be a mistake. It should be based on activeCpClients, not bootstrapinfo.

There is a need to add the first one in this method. You are right that it is cleaner to make that explicit rather than use the general method which could potentially cover up some problem in the fallback logic.

There is a need to add the first one in this method.

Is there? Why would the first be special? I see nothing that would guarantee this is the first ServerInfo in the authority's list, so it seems this could also make things out-of-order.

We're looping through all authorities that this serverInfo can potentially serve, not just the ones it has been requested on. By definition if it isn't in the list already it isn't supposed to be answering for this authority. So it seems clearly wrong to go about adding to the list.

We got here because the authority has subscribers. Is it even possible for an authority to have a subscriber, non-empty serverInfos, and yet not have any cpc in the list? It looks like watchXdsResource() would add to the list (via addCpcToAuthority()) when the subscriber was created.

I don't see the point of using getAuthoritiesForServerInfo() (which loops through all server info registrations) and then figure out if we care about the authority with hasSubscribers() and activeCpClients.get(authority).contains(readyCpc). Seems all we need to do is:

for (Map.Entry<String, List<ControlPlaneClient>> me : activeCpClients.entrySet()) { if (me.getValue().contains(controlPlaneClient)) { internalHandleStreamReady(controlPlaneClient, me.getValue()); } }

There's not need to check isShutDown() as shut down cleans up activeCpClients. With this construction, it is impossible for getActiveCpc() to return null. There's no need to check activeCpClient.isReady() && !activeCpClients.get(authority).contains(readyCpc).

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

ejona86 · 2024-10-31T18:18:09Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

      if (ignoreResourceDeletionEnabled && type.isFullStateOfTheWorld() && data != null) {
        if (!resourceDeletionIgnored) {
          logger.log(XdsLogLevel.FORCE_WARNING,
              "xds server {0}: ignoring deletion for resource type {1} name {2}}",
-              serverInfo.target(), type, resource);
+              getTarget(), type, resource);


Instead of re-discovering what caused this, can we just pass ControlPlaneClient, ServerInfo, or target into onAbsent()? We have that information in the two places we call it.

Changed it to get target from activeCpc.

I don't see a reason to use activeCpc (to call getActiveCpc() at all). Why re-discover what cpc caused this? It seems easy to add the argument. Would it be a pain to update tests or something like that?

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

Fix fallback logic for new watch triggering fallback to use the right CPC. Some non-functional clarity improvement Change hasSubscribers detection to be across all types and not trying to involve any logic in ControlPlaneClient.

larry-safran

Things done in commit 003348b

xds/src/test/java/io/grpc/xds/GrpcBootstrapperImplTest.java

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

larry-safran · 2024-10-31T18:39:39Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

+      }
+
+      for (String authority : getAuthoritiesForServerInfo(serverInfo)) {
+        if (controlPlaneClient.hasSubscribedResources(authority)) {


Moved the checking for subscribed resources out of ControlPlaneClient as this is the only place it is used. I believe it originated from some idea of the ControlPlaneClient knowing which authorities it supported and therefore having ownership of the logic, but the code evolved so it no longer made sense.

…Lists

… ADS stream gets an error. Retry after error is an error.

… be notified.

… there hasn't been an active client for that authority rather than using the generic add method all of the time.

… instead use locally available value.

…ill in the list for some other authority, call adjustResourceSubscriptions on all resource types.

ejona86

Sending what I have.

xds/src/main/java/io/grpc/xds/client/BootstrapperImpl.java

xds/src/main/java/io/grpc/xds/client/XdsClient.java

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

xds/src/test/java/io/grpc/xds/ControlPlaneRule.java

xds/src/main/java/io/grpc/xds/client/XdsClient.java

xds/src/main/java/io/grpc/xds/client/ControlPlaneClient.java

ejona86 · 2024-11-06T00:22:03Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

+          // try to fallback to lower priority control plane client
+          if (checkForFallback
+              && doFallbackForAuthority(
+                  cpcThatClosed, serverInfo, subscriber.serverInfos, subscriber.authority)) {


The docs for doFallbackForAuthority() say server info should be for the active CPC. But here it is identical to cpcThatClosed.getServerInfo().

Since we are only trying to do fallback for authorities for which the closed cpc was the active cpc, they are the same. However, since they are the same, and this is the only place that this method is called, there is no reason to pass the serverInfo.

authoritiesForClosedCpc = getActiveAuthorities(cpcThatClosed) ... if (subscriber.hasResult() || !authoritiesForClosedCpc.contains(subscriber.authority)) { continue; }

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

…rsion that takes a single serverInfo.

…o lookup the ControlPlaneClient instead of sending it

ejona86

Sending what I have.

ejona86 · 2024-11-06T17:40:33Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

+    int i = serverInfos.indexOf(activeServerInfo);
+    List<ServerInfo> fallbackServers = (i > -1)
+        ? serverInfos.subList(i + 1, serverInfos.size())
+        : null;


ImmutableList.of()? Then remove the fallbackServers != null check?

ejona86 · 2024-11-06T17:46:29Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

      }
    }
  }

-  public ControlPlaneClient getOrCreateControlPlaneClient(ServerInfo serverInfo) {
+  public ControlPlaneClient getOrCreateControlPlaneClient(ImmutableList<ServerInfo> serverInfos) {
+    if (serverInfos == null || serverInfos.isEmpty()) {


Remove isEmpty(), as it isn't needed.

The callers are going out of their way to avoid passing null here, so either delete this check or simplify the callers.

(A combined method with fallBackToCpc() as I discuss in other comments would also probably change what happens here, as you'd probably pass in the authority as the argument and do the lookup of the serverInfos within this method.)

ejona86 · 2024-11-07T17:05:06Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

+    }
+  }
+
+  public ControlPlaneClient getOrCreateControlPlaneClient(ImmutableList<ServerInfo> serverInfos) {


ejona86 · 2024-11-07T17:07:01Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

+
+          ControlPlaneClient cpcToUse = subscriber.loadControlPlane();
+
+          if (subscriber.errorDescription != null) {


This condition can leak a freshly-created cpcToUse. Move loadControlPlane() after the condition?

ejona86 · 2024-11-07T17:12:34Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

          }
+
+          doFallbackIfNecessary(cpcToUse, subscriber.authority);


This method re-discovers the code flow taken by getOrCreateControlPlaneClient(). Can that be integrated into one method to avoid the error-prone handling? Every call to getOrCreateControlPlaneClient() needs to add the fallback if necessary, and the only other call to getOrCreateControlPlaneClient() just calls fallBackToCpc() unconditionally. It would be better to create and update the state together in one method so we know the state remains consistent.

ejona86 · 2024-11-07T17:19:03Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

    }
+
+    // Everything existed and is in backoff so return null
+    return null;


Returning null here is confusing. The name would not suggest it could return null ("you said you'd create it"). And getOrCreateControlPlaneClient(ServerInfo) doesn't return null. So it is very hard to read code using this correctly. For example, the ControlPlaneClient cpc = getOrCreateControlPlaneClient(serverInfo); in this function (properly) doesn't check the null return, but it is type-sensitive whether that is appropriate. I suggest renaming this method (potentially also combining it with fallBackToCpc() that I mention elsewhere).

ejona86 · 2024-11-07T19:16:06Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

-
+  private final Map<ServerInfo, LoadStatsManager2> loadStatsManagerMap = new HashMap<>();
+  final Map<ServerInfo, LoadReportClient> serverLrsClientMap = new HashMap<>();
+  /** Map of authority to its active control plane client (affected by xds fallback). */


"active" here has a different meaning than getActiveCpc(). We should probably use a different term on one of them.

ejona86 · 2024-11-07T23:46:59Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

+    private void internalHandleStreamReady(
+        ServerInfo serverInfo, ControlPlaneClient controlPlaneClient, String authority) {
+      ControlPlaneClient activeCpClient = getActiveCpc(authority);
+      addCpcToAuthority(authority, controlPlaneClient);


There is a need to add the first one in this method.

Is there? Why would the first be special? I see nothing that would guarantee this is the first ServerInfo in the authority's list, so it seems this could also make things out-of-order.

We're looping through all authorities that this serverInfo can potentially serve, not just the ones it has been requested on. By definition if it isn't in the list already it isn't supposed to be answering for this authority. So it seems clearly wrong to go about adding to the list.

We got here because the authority has subscribers. Is it even possible for an authority to have a subscriber, non-empty serverInfos, and yet not have any cpc in the list? It looks like watchXdsResource() would add to the list (via addCpcToAuthority()) when the subscriber was created.

I don't see the point of using getAuthoritiesForServerInfo() (which loops through all server info registrations) and then figure out if we care about the authority with hasSubscribers() and activeCpClients.get(authority).contains(readyCpc). Seems all we need to do is:

for (Map.Entry<String, List<ControlPlaneClient>> me : activeCpClients.entrySet()) { if (me.getValue().contains(controlPlaneClient)) { internalHandleStreamReady(controlPlaneClient, me.getValue()); } }

There's not need to check isShutDown() as shut down cleans up activeCpClients. With this construction, it is impossible for getActiveCpc() to return null. There's no need to check activeCpClient.isReady() && !activeCpClients.get(authority).contains(readyCpc).

ejona86 · 2024-11-07T23:48:36Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

+      }
+
+      restartMatchingSubscriberTimers(authority);
+      readyCpc.sendDiscoveryRequests();


This isn't limited to the current authority, so won't this send discovery multiple times?

…eateControlPlaneClient and do fallback. Move some logic into watchXdsResource from private methods, such as doFallbackIfNecessary, and streamline the execution. Stop storing serverInfos in ResourceSubscriber and just construct it from authority as needed.

…ageControlPlaneClient

larry-safran force-pushed the xds_fallback branch from 96cecb2 to 72d9ac9 Compare June 6, 2024 00:56

larry-safran requested a review from ejona86 June 12, 2024 23:17

ejona86 requested a review from YifeiZhuang June 17, 2024 20:49

larry-safran marked this pull request as ready for review June 17, 2024 20:49

YifeiZhuang reviewed Jul 3, 2024

View reviewed changes

Checkpoint

25aa454

larry-safran force-pushed the xds_fallback branch from 709026a to 0c1eef8 Compare August 14, 2024 00:04

larry-safran requested a review from YifeiZhuang August 17, 2024 00:35

XdsClient Fallback working

d8f7094

larry-safran force-pushed the xds_fallback branch from af33b25 to d8f7094 Compare August 21, 2024 20:55

larry-safran mentioned this pull request Aug 22, 2024

Xds client split #11484

Merged

YifeiZhuang reviewed Aug 22, 2024

View reviewed changes

Merge branch 'master' into xds_fallback

d0e7ba2

larry-safran requested a review from YifeiZhuang August 28, 2024 23:05

larry-safran added 3 commits August 29, 2024 12:53

Eliminate version of getSubscribedResources that doesn't take an auth…

c2f0ecc

…ority argument.

Change logic based on changing from assuming that fallback servers ha…

2ab47a5

…d completely disjoint resource names to assuming that any duplicate names would be compatible, particularly cross authority.

Cleanup stuff added for debugging

38267d5

YifeiZhuang reviewed Sep 11, 2024

View reviewed changes

larry-safran added 2 commits September 13, 2024 11:13

Remove changes to CensusModulesTest

05d188f

Cleanup as recommended

dedb8fe

ejona86 reviewed Oct 18, 2024

View reviewed changes

Remove ControlPlaneClient from resource and instead use authority to …

22b5a0c

…figure out the current CPC. Add all live ControlPlaneClients to active list rather than skipping ones in backoff. Changed isConnected() logic to be based on AdsStream call's success or failure

ejona86 reviewed Oct 31, 2024

View reviewed changes

Restore experimental property in @after of test

003348b

Fix fallback logic for new watch triggering fallback to use the right CPC. Some non-functional clarity improvement Change hasSubscribers detection to be across all types and not trying to involve any logic in ControlPlaneClient.

larry-safran commented Oct 31, 2024

View reviewed changes

larry-safran added 2 commits October 31, 2024 15:59

Remove unnecessary checks for index not being at end when getting sub…

2c11225

…Lists

Remove unnecessary checks for index not being at end when getting sub…

7323f25

…Lists

larry-safran added 6 commits October 31, 2024 17:09

When response have been received, don't consider it an error when the…

fe384a8

… ADS stream gets an error. Retry after error is an error.

Fix logic for identifying missing resources whose subscribers need to…

4d2e4d8

… be notified.

Change internalHandleStreamReady() to onl add to activeCpClients when…

23b6740

… there hasn't been an active client for that authority rather than using the generic add method all of the time.

Eliminate use of compare function for control plane clients.

b5d69d3

Don't recalculate which server was deletion ignored from in onAbsent,…

5b28d7a

… instead use locally available value.

when a CPC is bumped by a higher priority server coming up, but is st…

1169f39

…ill in the list for some other authority, call adjustResourceSubscriptions on all resource types.

ejona86 reviewed Nov 6, 2024

View reviewed changes

larry-safran added 4 commits November 5, 2024 17:37

Address trivial review comments

8d22cfd

Simplify getOrCreateControlPlaneClient of a list by relying on the ve…

023c6cd

…rsion that takes a single serverInfo.

In handleStreamClosed used the response handler's stored ServerInfo t…

517bcc3

…o lookup the ControlPlaneClient instead of sending it

Eliminate unneeded argument from private method doFallbackForAuthority.

230c18d

ejona86 reviewed Nov 7, 2024

View reviewed changes

Eliminate unneeded null handling.

7975051

ejona86 reviewed Nov 7, 2024

View reviewed changes

larry-safran added 2 commits November 7, 2024 15:52

Eliminate doFallbackForAuthority since it became a simple call to man…

b979d70

…ageControlPlaneClient


		ControlPlaneClient cpcToUse = subscriber.loadControlPlane();

		if (subscriber.errorDescription != null) {

Xds fallback #11254

Are you sure you want to change the base?

Xds fallback #11254

Conversation

larry-safran commented Jun 3, 2024

YifeiZhuang 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

YifeiZhuang Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

larry-safran commented Sep 5, 2024

YifeiZhuang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 commented Oct 31, 2024

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

ejona86 Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

larry-safran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 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

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

YifeiZhuang Aug 22, 2024 •

edited

Loading

ejona86 Nov 7, 2024 •

edited

Loading