xds: add support for multiple xDS clients, for fallback #7347

easwars · 2024-06-24T19:52:22Z

This PR adds support for multiple xDS clients, as required by xDS fallback(A71).

Summary of changes:

xDS client creation APIs now require a name to be specified
- On the client side, the user's dial target is used, and on the server side a well-known dedicated key #server is used
- Tests pass t.Name as the xDS client name
- Also, added a GetForTesting method since it is no longer possible to simply call New and expect to get a reference to the singleton xDS client (as used by some tests to trigger resource not found errors)
- Changes to the following resolvers to pass user's dial target during xDS client creation
  - google_c2p
  - xds
xDS client changes to dumping resources
- Removed the DumpResources method from the XDSClient interface
- Added a DumpResources function to the xdsclient package that traverses all existing xDS clients, retrieves a dump from each of them, and adds the client_scope field to each one, and returns a response that contains resources from all the clients
CSDS service implementation changes
- No longer holds a reference to the xDS singleton. Instead invokes the DumpResources API provided by the xdsclient package (that retrieves configs from all xDS clients in the system).
- E2E test now creates more than one xDS client and verifies that the client_scope field is set appropriately.
Minor improvements to stringing functionality in the bootstrap package to improve test log readability.

#a71-xds-fallback

RELEASE NOTES: none

easwars · 2024-06-24T19:55:58Z

I probably need to add one more test where the xDS clients are talking to different management servers, and receiving different resources. But the review can start prior to that, and I should have that test written soonish.

easwars · 2024-06-24T20:44:54Z

TestDumpResources is failing because the test context is expiring before we test can read the expected config dump from the xDS clients. My guess at this point is that because the go-control-plane keeps resending resources when they are NACKed, the gRPC client is overwhelmed by ADS responses. But I have to confirm this theory. Will update when I have a breakthrough.

codecov · 2024-06-24T21:44:13Z

Codecov Report

Attention: Patch coverage is 80.21978% with 18 lines in your changes missing coverage. Please review.

Project coverage is 81.32%. Comparing base (4dd7f55) to head (ef92a2f).
Report is 22 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7347      +/-   ##
==========================================
+ Coverage   80.58%   81.32%   +0.73%     
==========================================
  Files         349      348       -1     
  Lines       34056    26744    -7312     
==========================================
- Hits        27445    21749    -5696     
+ Misses       5431     3795    -1636     
- Partials     1180     1200      +20

Files	Coverage Δ
xds/csds/csds.go	`69.23% <100.00%> (-3.50%)`	⬇️
xds/internal/resolver/xds_resolver.go	`80.71% <100.00%> (+1.78%)`	⬆️
xds/internal/xdsclient/authority.go	`86.09% <100.00%> (-2.85%)`	⬇️
xds/internal/xdsclient/clientimpl_dump.go	`100.00% <100.00%> (+15.00%)`	⬆️
xds/server.go	`81.74% <100.00%> (-1.00%)`	⬇️
xds/internal/xdsclient/clientimpl_loadreport.go	`69.23% <0.00%> (-4.11%)`	⬇️
internal/testutils/xds/e2e/logging.go	`50.00% <50.00%> (ø)`
xds/googledirectpath/googlec2p.go	`87.50% <33.33%> (+1.78%)`	⬆️
xds/internal/xdsclient/client_refcounted.go	`83.72% <92.30%> (ø)`
xds/internal/xdsclient/client_new.go	`86.04% <75.00%> (+1.76%)`	⬆️
... and 1 more

... and 292 files with indirect coverage changes

easwars · 2024-06-25T15:55:14Z

Tests are passing now. This is good to be looked at.

zasweq

Implementation looks good overall, mostly just style/naming nits. Some questions about scaling up tests.

xds/internal/xdsclient/client_new.go

xds/server_ext_test.go

xds/internal/xdsclient/clientimpl_dump.go

xds/csds/csds_e2e_test.go

zasweq · 2024-06-27T19:36:35Z

xds/internal/xdsclient/client_refcounted.go


-	}
+func newRefCountedWithConfig(name string, fallbackConfig *bootstrap.Config, watchExpiryTimeout, idleAuthorityTimeout time.Duration) (XDSClient, func(), error) {


Top level comment of get or create if not present? Or change name?

Added a comment.

Same question as for the Close, is the reason to keep these names this happens implicitly with objects that are ref counted?

Explained the reasoning in the comment for close.

xds/internal/xdsclient/client_new.go

easwars · 2024-06-27T23:36:09Z

Looks like there is a flake in the csds e2e test. I will continue to look at that, but this is still good for a second pass. Thanks.

zasweq · 2024-06-28T00:22:10Z

xds/internal/xdsclient/client_refcounted.go

+// newRefCountedWithConfig creates a new reference counted xDS client
+// implementation for name, if one does not exist already. The passed in
+// fallback config is used when bootstrap environment variables are not defined.


Nit: "gets a reference to the existing xDS Client, or creates one if one does not exist already" or something alongside those lines for first context. To explicitly call out what happens if the xDS Client already exists for this name.

zasweq · 2024-06-28T00:27:33Z

xds/internal/xdsclient/client_refcounted_test.go

+
+// Tests the scenario where there are multiple calls to New() with different
+// names. Verifies that reference counts are tracked correctly and that only
+// when all references are released, the client is closed.


Nit: switch "are released, the client is closed" to something that emphasizes the plurality of the clients maybe "are released for a client, only that specific client is closed" or something along those lines.

zasweq

Looks great!

zasweq · 2024-06-28T00:31:01Z

xds/internal/xdsclient/client_refcounted_test.go

+	// Close the first client completely.
+	closeFunc1()
+	for i := 0; i < count; i++ {
+		closeFuncs1[i]()


Optional: here and the close for 2: the test above tests the idempotency implemented through the once func. Maybe call some of these twice?

Sure. Done.

easwars · 2024-07-01T16:10:51Z

The CSDS test is failing on Github Actions because of the way go-control-plane continuously resends NACKed resources. This results in a busy loop on the xdsClient where it receives a resource and NACKs it repeatedly. This results in starvation of the DumpResources call from the CSDS service.

I'm planning to put this PR on hold for now, fix grpc/grpc#34099 and get back here.

easwars · 2024-07-02T22:21:48Z

@zasweq : Based on our offline discussion, I made a commit that skips the test on arm64. Waiting for GA to pass before I merge this. That way, I can work on the other PR implementing ADS stream flow control, and once I have that, I will be able to tweak this test and get it to work.

zasweq · 2024-07-02T22:24:42Z

Sgtm, thinking about it I agree with Doug. To minimize the chance of merge conflicts probably best to get this in sooner rather than later.

xds: add support for multiple xDS clients, for fallback

a3e2ec7

easwars added the Type: Feature New features or improvements in behavior label Jun 24, 2024

easwars added this to the 1.66 Release milestone Jun 24, 2024

ensure args are passed in properly to t.Logf

4162d1d

easwars closed this Jun 24, 2024

easwars reopened this Jun 24, 2024

easwars added 2 commits June 25, 2024 14:45

deflake existing tests

3892040

add a new test with many to many xds clients to servers

9d704f0

easwars requested a review from zasweq June 25, 2024 17:00

easwars assigned zasweq Jun 25, 2024

zasweq reviewed Jun 27, 2024

View reviewed changes

zasweq assigned easwars and unassigned zasweq Jun 27, 2024

review comments pass #1

0ea40bf

easwars assigned zasweq and unassigned easwars Jun 27, 2024

zasweq reviewed Jun 28, 2024

View reviewed changes

zasweq approved these changes Jun 28, 2024

View reviewed changes

zasweq assigned easwars and unassigned zasweq Jun 28, 2024

zasweq reviewed Jun 28, 2024

View reviewed changes

review comments pass #2

6079e82

skip test in arm64

ef92a2f

easwars merged commit 4e9b596 into grpc:master Jul 2, 2024
13 checks passed

easwars deleted the support_multiple_xds_clients branch July 2, 2024 22:27

easwars mentioned this pull request Jul 2, 2024

xds/csds: unskip test after ADS stream flow control changes are merged #7383

Open

printchard pushed a commit to printchard/grpc-go that referenced this pull request Jul 3, 2024

xds: add support for multiple xDS clients, for fallback (grpc#7347)

de08608

easwars mentioned this pull request Jul 9, 2024

xds: bootstrap config is not emitted to logs in a human readable way #7322

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xds: add support for multiple xDS clients, for fallback #7347

xds: add support for multiple xDS clients, for fallback #7347

easwars commented Jun 24, 2024 •

edited

Loading

easwars commented Jun 24, 2024

easwars commented Jun 24, 2024

codecov bot commented Jun 24, 2024 •

edited

Loading

easwars commented Jun 25, 2024

zasweq left a comment

zasweq Jun 27, 2024

easwars Jun 27, 2024

zasweq Jun 28, 2024

easwars Jun 28, 2024

easwars commented Jun 27, 2024

zasweq Jun 28, 2024

easwars Jun 28, 2024

zasweq Jun 28, 2024

easwars Jun 28, 2024

zasweq left a comment

zasweq Jun 28, 2024

easwars Jun 28, 2024

easwars commented Jul 1, 2024

easwars commented Jul 2, 2024

zasweq commented Jul 2, 2024


		}
		func newRefCountedWithConfig(name string, fallbackConfig *bootstrap.Config, watchExpiryTimeout, idleAuthorityTimeout time.Duration) (XDSClient, func(), error) {

xds: add support for multiple xDS clients, for fallback #7347

xds: add support for multiple xDS clients, for fallback #7347

Conversation

easwars commented Jun 24, 2024 • edited Loading

easwars commented Jun 24, 2024

easwars commented Jun 24, 2024

codecov bot commented Jun 24, 2024 • edited Loading

Codecov Report

easwars commented Jun 25, 2024

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 Jun 27, 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

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

easwars commented Jul 1, 2024

easwars commented Jul 2, 2024

zasweq commented Jul 2, 2024

easwars commented Jun 24, 2024 •

edited

Loading

codecov bot commented Jun 24, 2024 •

edited

Loading