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

xds/resolver: fix flaky test TestResolverRemovedWithRPCs with a workaround #7804

Merged
merged 2 commits into from
Nov 5, 2024

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Nov 4, 2024

Fixes #7799

Once resources are removed from the management server (as part of this test), the xDS client used by the resolver will see discovery responses from the server with no resources. This will trigger a resource-not-found error on the resolver when the listener resource is missing in the response from the server. This will cause the resolver to stop watching the route configuration resource.

The test then re-adds these resources to the management server, and these two events can race and can lead to the management server sending the route configuration too early to the client (which at that point thinks that it has not requested for that resource, and hence drops it on the floor).

And later when the client requests for the same route configuration, the server does not send it because it thinks that it has already sent it to the client. See #7799 (comment) for a sequence of events where this leads to a problem.

The workaround is to continuously push a new version to the server with the same resources so that eventually, the route configuration is pushed to the client when it is in a state where it can accept it.

RELEASE NOTES: none

@dfawley
Copy link
Member

dfawley commented Nov 4, 2024

Please add a TODO to revert this, and let's make sure we have an issue to remind ourselves to revert this workaround when go-control-plane is fixed.

Copy link

codecov bot commented Nov 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.92%. Comparing base (d66fc3a) to head (6e49d13).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7804      +/-   ##
==========================================
+ Coverage   81.71%   81.92%   +0.21%     
==========================================
  Files         374      373       -1     
  Lines       38166    37700     -466     
==========================================
- Hits        31188    30887     -301     
+ Misses       5699     5538     -161     
+ Partials     1279     1275       -4     

see 27 files with indirect coverage changes

@easwars easwars merged commit 2de6df9 into grpc:master Nov 5, 2024
15 checks passed
@easwars easwars deleted the TestResolverRemovedWithRPCs branch November 5, 2024 00:27
misvivek pushed a commit to misvivek/grpc-go that referenced this pull request Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky test: TestResolverRemovedWithRPCs
2 participants