Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

grpc-js-xds: Refactor xDS stream state and add resource timer #2117

Merged
merged 6 commits into from Aug 8, 2022

Conversation

murgatroid99
Copy link
Member

@murgatroid99 murgatroid99 commented May 13, 2022

The main goal of this change is to add the resource timer, which is described in this section of the xDS docs. Essentially, the protocol does not consistently tell us when a resource does not exist, so we assume that the server will respond to any request within 15 seconds, and if the client does not get the requested resource in that time, it assumes that it does not exist. Since the timer is watching server response latency, we only run it while we have an active ADS stream. The specific algorithm is as follows:

  • When a watcher subscribes to a new resource name, if the ADS stream is alive, start the timer for that resource name.
  • When the ADS stream dies, cancel all timers.
  • When a new ADS stream starts, start the timer for all resource names for which we do not have a cached response.
  • When we receive a resource, cancel its timer.
  • When the timer fires, report that the resource does not exist.

Some significant changes were needed to the watchers structure to implement this in a reasonable way, and the simplest way to do that was to refactor most of the logic into the new BaseXdsStreamState abstract class. This refactor ended up slightly modifying the nonexistent resource reporting for LDS and CDS, to account for whether we already have a cached response. Otherwise I think it should all be functionally the same.

I also added a change to clear the nonce when the ADS stream ends, because apparently we're supposed to do that too. It's not really related to this change but I was touching that part of the code anyway and it's a quick change.

@murgatroid99
Copy link
Member Author

The tests failed (https://source.cloud.google.com/results/invocations/08de3287-754d-4983-8a6d-d23120284303/targets), but I think it's actually a configuration issue unrelated to this change. The test logs show that the driver never successfully confirms that the servers start, and the client logs appear to show the correct general behavior, in terms of correctly processing the xDS responses.

I think it would be best to just submit and let it run with the regular config.

@murgatroid99 murgatroid99 merged commit 9e34f3c into grpc:master Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants