xdsclient: introduce pool to manage multiple xDS clients with same bootstrap content #7898

purnesh42H · 2024-12-04T20:22:30Z

RELEASE NOTES:

xds: fixed a bug which sometimes prevented a server to be used with multiple clients with different configurations.

codecov · 2024-12-04T20:29:12Z

Codecov Report

Attention: Patch coverage is 85.22727% with 13 lines in your changes missing coverage. Please review.

Project coverage is 81.97%. Comparing base (4c07bca) to head (43b394d).
Report is 14 commits behind head on master.

Files with missing lines	Patch %	Lines
xds/internal/xdsclient/pool.go	78.94%	8 Missing and 4 partials ⚠️
xds/internal/xdsclient/client_new.go	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7898      +/-   ##
==========================================
+ Coverage   81.84%   81.97%   +0.12%     
==========================================
  Files         377      378       +1     
  Lines       38120    38216      +96     
==========================================
+ Hits        31201    31326     +125     
+ Misses       5603     5578      -25     
+ Partials     1316     1312       -4

Files with missing lines	Coverage Δ
xds/internal/resolver/xds_resolver.go	`79.38% <ø> (ø)`
xds/internal/xdsclient/client_refcounted.go	`83.33% <100.00%> (+1.28%)`	⬆️
xds/internal/xdsclient/clientimpl_dump.go	`100.00% <100.00%> (ø)`
xds/server.go	`82.94% <100.00%> (-0.10%)`	⬇️
xds/server_options.go	`100.00% <100.00%> (ø)`
xds/internal/xdsclient/client_new.go	`86.66% <66.66%> (+0.30%)`	⬆️
xds/internal/xdsclient/pool.go	`78.94% <78.94%> (ø)`

... and 32 files with indirect coverage changes

danielzhaotongliu · 2024-12-05T22:12:12Z

xds/internal/xdsclient/pool.go

+var (
+	// DefaultPool is the default pool for xds clients. It is created at the
+	// init time.
+	DefaultPool *Pool


What would be the reason to export this DefaultPool variable? Was it to make testing easier? I see most reference of this variable is within the xdsclient package, with only one reference in xds/server_options.go.
Would it be possible to test via Public APIs instead?

yeah server options have to set and get config for testing. I think we can provide the set and get method for default pool but then its equivalent of exporting a variable.

I agree with @danielzhaotongliu. We don't have to make this public. We should change the dial option and the server option to create a pool with the bootstrap config that was passed to it, and use that pool to create the xDS client. This is specified in the proposal in #7592 (comment)

Just coming back to this comment. I think we would have to export the default pool from the xdsclient package or use the internal package trick, because we want the xDS resolver and the xDS server to be able to use the default pool when they want to create xDS clients in non-test environments.

The internal trick won't work once we move the xDS client out into a separate repo (as part of the generic xDS client work). So, we are only left with the option of exporting this.

Do you both have other ideas?

danielzhaotongliu · 2024-12-05T22:21:20Z

xds/internal/resolver/xds_resolver.go

@@ -46,7 +46,8 @@ const Scheme = "xds"

 // newBuilderWithConfigForTesting creates a new xds resolver builder using a
 // specific xds bootstrap config, so tests can use multiple xds clients in
-// different ClientConns at the same time.
+// different ClientConns at the same time. It creates the new xds client int


typo/nit: "It creates a new xds client in the default pool with the provided config."

danielzhaotongliu · 2024-12-05T22:33:49Z

xds/internal/xdsclient/pool.go

+// expected to invoke once they are done using the client.  It is safe for the
+// caller to invoke this close function multiple times.
+func (p *Pool) NewClient(name string) (XDSClient, func(), error) {
+	config, err := bootstrap.GetConfiguration()


What would be the reason to not use the Pool.config field xDS bootstrap configuration as that was the config passed in/parsed/created during the NewPool function above?

NewClient is for the prod path which actually reads the bootstrap config from env vars and will only deal with default pool.

What about the use case where users ONLY set bootstrap config from the SetFallbackBootstrapConfig API and do not set env var? For this case, when creating the xDS client, would it be created via this method? Would they not require p.config instead since that is the field set from the implementation below (line 133-143)?

This method must use the config field inside the Pool. There is no point in reading the bootstrap configuration from the env vars or the fallback bootstrap configuration. In fact, once we do that, we don't have a reason to have Pool.NewClientForTesting. Tests could as well use pool.NewClient.

What do you think?

NewClient is for the prod path which actually reads the bootstrap config from env vars and will only deal with default pool

The bootstrap env vars should be read when the default pool is created at init time. And if bootstrap env vars are not set, the default pool should have a nil config and if NewClient is called without calling SetFallbackBootstrapConfig, i.e. config is nil when NewClient is called, then NewClient should fail with a useful error message.

danielzhaotongliu · 2024-12-05T22:35:23Z

xds/internal/xdsclient/pool.go

+	if err != nil {
+		return nil, nil, fmt.Errorf("xds: failed to get xDS bootstrap config: %v", err)
+	}
+	return newRefCounted(name, p, config, defaultWatchExpiryTimeout, defaultIdleChannelExpiryTimeout, backoff.DefaultExponential.Backoff)


For my own understanding, would there be a need now/or in the future for users or anyone to customize the expiry timeout?

only for testing. We usually provide the testing knobs set this values via methods but only for testing

With #7924, we no longer have the idle channel expiry timeout. We only have the watch expiry timeout. And so far, we haven't had any real usecase where a user wants to customize that expiry timeout.

danielzhaotongliu · 2024-12-05T22:39:55Z

xds/internal/xdsclient/pool.go

+	if opts.Contents != nil {
+		config, err := bootstrap.NewConfigForTesting(opts.Contents)
+		if err != nil {
+			p.mu.Unlock()


For my understanding, where is this p.mu.Lock() called?

yeah good catch. That was supposed to be removed. I removed it.

danielzhaotongliu

I might not be familiar enough with the Github testing process for gRPC Go, have we tested this end-to-end, or would the current tests suffice? I recall there are (PSM) interop tests, are those run post PR submission or manually triggered?

Are there any performance implications of this change?

danielzhaotongliu · 2024-12-12T23:14:12Z

xds/internal/xdsclient/pool.go

+// expected to invoke once they are done using the client.  It is safe for the
+// caller to invoke this close function multiple times.
+func (p *Pool) NewClient(name string) (XDSClient, func(), error) {
+	config, err := bootstrap.GetConfiguration()


What about the use case where users ONLY set bootstrap config from the SetFallbackBootstrapConfig API and do not set env var? For this case, when creating the xDS client, would it be created via this method? Would they not require p.config instead since that is the field set from the implementation below (line 133-143)?

easwars · 2024-12-13T22:02:15Z

xds/internal/xdsclient/pool.go

+)
+
+var (
+	// DefaultPool is the default pool for xds clients. It is created at the


Nit: Here and everywhere in this PR, please replace occurrences of xds client(s) with xDS client(s). We've been trying to be consistent with xDS client(s) recently. So, let's stick to that. Thanks.

easwars · 2024-12-13T22:02:47Z

xds/internal/xdsclient/pool.go

+	DefaultPool *Pool
+)
+
+// Pool represents pool of xds clients.


This docstring should mention that the pool of clients share the same bootstrap configuration.

easwars · 2024-12-13T22:04:37Z

xds/internal/xdsclient/pool.go

+	mu      sync.Mutex
+	clients map[string]*clientRefCounted
+	config  *bootstrap.Config


Nit: mu should ideally only have to guard clients. But here, we need it to guard config as well since SetFallbackBootstrapConfig writes to config. Maybe a comment here to mention that?

easwars · 2024-12-13T22:05:58Z

xds/internal/xdsclient/pool.go

+var (
+	// DefaultPool is the default pool for xds clients. It is created at the
+	// init time.
+	DefaultPool *Pool


I agree with @danielzhaotongliu. We don't have to make this public. We should change the dial option and the server option to create a pool with the bootstrap config that was passed to it, and use that pool to create the xDS client. This is specified in the proposal in #7592 (comment)

easwars · 2024-12-13T22:07:45Z

xds/internal/xdsclient/pool.go

+}
+
+// NewPool creates a new xds client pool with the given bootstrap contents.
+func NewPool(bootstrapContents []byte) (*Pool, error) {


I think it would be nicer to have this accept a *bootstrap.Config (which could optionally be nil) rather than having to parse the configuration here. With that change, this API will return a single value, *Pool, instead of two.

When we change this to accept a bootstrap.Config, we should also export bootstrap.newConfigFromContents. This would allow non-test usages for NewPool where the caller would use the exported bootstrap.newConfigFromContents to parse the JSON config and pass it to this function.

easwars · 2024-12-13T22:46:10Z

xds/internal/xdsclient/tests/resource_update_test.go

@@ -360,7 +360,7 @@ func (s) TestHandleListenerResponseFromManagementServer(t *testing.T) {
 // involving receipt of an RDS response from the management server. The test
 // verifies that the internal state of the xDS client (parsed resource and
 // metadata) matches expectations.
-func (s) TestHandleRouteConfigResponseFromManagementServer(t *testing.T) {
+func TestHandleRouteConfigResponseFromManagementServer(t *testing.T) {


Why is the s receiver being removed from this test?

easwars · 2024-12-13T22:47:13Z

xds/internal/xdsclient/tests/resource_update_test.go

 				WatchExpiryTimeout: defaultTestWatchExpiryTimeout,
+				Contents:           bc,


These tests should be using a pool to create an xDS client in. In fact, once we delete these NewXxx functions which don't operate on a pool, the tests will be left with no other choice.

easwars · 2024-12-13T22:48:51Z

xds/server.go

@@ -93,12 +93,11 @@ func NewGRPCServer(opts ...grpc.ServerOption) (*GRPCServer, error) {
 	// simplifies the code by eliminating the need for a mutex to protect the
 	// xdsC and xdsClientClose fields.
 	newXDSClient := newXDSClient


Instead of having this newXDSClient var which can be overridden by tests to customize xDS client creation, the xDS server (and possibly the xDS resolver) should contain an xdsClientPool as a global var which would be set to the default pool at init time and can be customized by tests.

easwars · 2024-12-13T22:49:59Z

xds/server_test.go

@@ -121,14 +121,18 @@ func (s) TestNewServer_Success(t *testing.T) {
 			desc: "without_xds_creds",
 			serverOpts: []grpc.ServerOption{
 				grpc.Creds(insecure.NewCredentials()),
-				BootstrapContentsForTesting(generateBootstrapContents(t, uuid.NewString(), nonExistentManagementServer)),
+				func() grpc.ServerOption {


Why do we need this func here?

easwars · 2024-12-13T22:53:19Z

xds/server_ext_test.go

+// matched route configuration. After it transitions back into not ready
+// (through an explicit LDS Resource Not Found), previously running RPC's
+// should be gracefully closed and still work, and new RPC's should fail.
+func (s) TestServingModeChanges_MultipleServers(t *testing.T) {


I don't think we need to test serving mode changes here. All we need to do here is to ensure that we can create two servers with two different bootstrap configurations. This would also mean that the two servers will request two different LDS resources from the management server, because the LDS resource name depends on the port on which the server is listening.

Then we could have two gRPC clients that talk to each of these servers and we ensure that RPCs work. We need to ensure that a gRPC client is actually talking to the server that it intends to talk to. This can be done with the Peer call option.

…or same server

purnesh42H added the Type: Bug label Dec 4, 2024

purnesh42H added this to the 1.69 Release milestone Dec 4, 2024

purnesh42H force-pushed the xdsclient-pool-per-bootstrap-content branch from 202847e to b561f1c Compare December 4, 2024 20:23

purnesh42H force-pushed the xdsclient-pool-per-bootstrap-content branch 2 times, most recently from 23023e9 to 12cb8a7 Compare December 4, 2024 21:16

purnesh42H requested a review from easwars December 4, 2024 21:17

purnesh42H assigned easwars Dec 4, 2024

purnesh42H force-pushed the xdsclient-pool-per-bootstrap-content branch from 12cb8a7 to 8787929 Compare December 5, 2024 04:37

purnesh42H modified the milestones: 1.69 Release, 1.70 Release Dec 5, 2024

purnesh42H force-pushed the xdsclient-pool-per-bootstrap-content branch from 8787929 to aa7e498 Compare December 5, 2024 16:30

danielzhaotongliu reviewed Dec 5, 2024

View reviewed changes

purnesh42H force-pushed the xdsclient-pool-per-bootstrap-content branch from 2ab7ba2 to 6e822a3 Compare December 6, 2024 09:42

purnesh42H requested a review from danielzhaotongliu December 6, 2024 09:46

purnesh42H force-pushed the xdsclient-pool-per-bootstrap-content branch 2 times, most recently from 4e62279 to 43b394d Compare December 6, 2024 18:47

danielzhaotongliu reviewed Dec 13, 2024

View reviewed changes

easwars reviewed Dec 13, 2024

View reviewed changes

easwars assigned purnesh42H and unassigned easwars Dec 13, 2024

purnesh42H force-pushed the xdsclient-pool-per-bootstrap-content branch 3 times, most recently from ee9dcb0 to 6d704a9 Compare December 20, 2024 11:33

purnesh42H added 5 commits December 22, 2024 03:38

use a pool to isolate xds clients with different bootstrap contents f…

9724396

…or same server

remove pool.mu.Unlock()

26ad051

multiple servers with unique bootstrap conig test

83c13ad

use pool everywhere instead of new client functions

6295269

Fix breaking tests

d11cf5f

purnesh42H force-pushed the xdsclient-pool-per-bootstrap-content branch from 6d704a9 to 8cade2a Compare December 21, 2024 22:18

purnesh42H force-pushed the xdsclient-pool-per-bootstrap-content branch 3 times, most recently from 4b295c9 to 9588254 Compare December 21, 2024 23:04

nil check for r.cc in r.onError, before calling r.cc.ReportError(err)

325def2

purnesh42H force-pushed the xdsclient-pool-per-bootstrap-content branch from 9588254 to 325def2 Compare December 21, 2024 23:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xdsclient: introduce pool to manage multiple xDS clients with same bootstrap content #7898

xdsclient: introduce pool to manage multiple xDS clients with same bootstrap content #7898

purnesh42H commented Dec 4, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading

danielzhaotongliu Dec 5, 2024

purnesh42H Dec 6, 2024 •

edited

Loading

easwars Dec 13, 2024

easwars Dec 13, 2024

danielzhaotongliu Dec 5, 2024

danielzhaotongliu Dec 5, 2024

purnesh42H Dec 6, 2024

danielzhaotongliu Dec 12, 2024

easwars Dec 13, 2024

danielzhaotongliu Dec 5, 2024

purnesh42H Dec 6, 2024

easwars Dec 13, 2024

danielzhaotongliu Dec 5, 2024

purnesh42H Dec 6, 2024

danielzhaotongliu left a comment

danielzhaotongliu Dec 12, 2024

easwars Dec 13, 2024

easwars Dec 13, 2024

easwars Dec 13, 2024

easwars Dec 13, 2024

easwars Dec 13, 2024

easwars Dec 13, 2024

easwars Dec 13, 2024

easwars Dec 13, 2024

easwars Dec 13, 2024

easwars Dec 13, 2024

easwars Dec 13, 2024

		WatchExpiryTimeout: defaultTestWatchExpiryTimeout,
		Contents: bc,

xdsclient: introduce pool to manage multiple xDS clients with same bootstrap content #7898

Are you sure you want to change the base?

xdsclient: introduce pool to manage multiple xDS clients with same bootstrap content #7898

Conversation

purnesh42H commented Dec 4, 2024 • edited Loading

codecov bot commented Dec 4, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

purnesh42H Dec 6, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielzhaotongliu 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purnesh42H commented Dec 4, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading

purnesh42H Dec 6, 2024 •

edited

Loading