Skip to content
New issue

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

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

Already on GitHub? # to your account

xds/server: create the xDS client when the xDS enabled gRPC server is created #6446

Merged
merged 3 commits into from
Jul 21, 2023

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Jul 14, 2023

Currently we create the xDS client when Serve() is called for the first time. This leads to unnecessary code complexity since we have to synchronize access to the xDS client and its close function inside the server. The following code paths touch the xDS client:

  • In Serve to see if one has been created, and create a new one if none exists
  • In Stop and GracefulStop to closed the client
  • In the ListenerWrapper to register LDS and RDS watches using it

This PR changes the code such that the xDS client is created when the xDS enabled gRPC server is created. Certain bootstrap configuration validation is also moved to here. Therefore we can fail the server creation when appropriate.

Note that with this change, since the mutex has been eliminated, Serve and Stop can race in the sense that Serve might not return grpc.ErrServerStopped in all cases, but this does not affect any other functionality and does not lead to leaking of the xDS client.

This PR also cleans up the tests to use a real management server and gets rid of using a fake xDS client.

#resource-agnostic-xdsclient-api

RELEASE NOTES: none

@easwars easwars requested a review from dfawley July 14, 2023 00:52
@easwars easwars added the Type: Internal Cleanup Refactors, etc label Jul 14, 2023
@easwars easwars added this to the 1.57 Release milestone Jul 14, 2023
@easwars
Copy link
Contributor Author

easwars commented Jul 14, 2023

@dfawley : The diff in xds/server_test.go is nasty. It might be easier to review that file as though it was a new one instead of using the diff. Thanks.

@easwars easwars modified the milestones: 1.57 Release, 1.58 Release Jul 14, 2023
@arvindbr8 arvindbr8 assigned easwars and unassigned dfawley Jul 18, 2023
@easwars
Copy link
Contributor Author

easwars commented Jul 18, 2023

Fixed test failures. Good for review now.

@easwars easwars assigned dfawley and unassigned easwars Jul 18, 2023
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks a lot better and even the tests are a bit simpler, too.

@dfawley dfawley assigned easwars and unassigned dfawley Jul 20, 2023
@easwars easwars merged commit d7f45cd into grpc:master Jul 21, 2023
1 check passed
@easwars easwars deleted the xds_server_cleanup branch July 21, 2023 00:29
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 2, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants