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 84.14634% with 13 lines in your changes missing coverage. Please review.

Project coverage is 82.14%. Comparing base (724f450) to head (d43ade8).

Files with missing lines	Patch %	Lines
xds/internal/xdsclient/pool.go	83.78%	4 Missing and 2 partials ⚠️
xds/internal/resolver/xds_resolver.go	66.66%	2 Missing and 1 partial ⚠️
xds/internal/xdsclient/client_refcounted.go	80.00%	2 Missing and 1 partial ⚠️
xds/internal/xdsclient/client_new.go	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7898      +/-   ##
==========================================
+ Coverage   82.05%   82.14%   +0.09%     
==========================================
  Files         381      382       +1     
  Lines       38539    38558      +19     
==========================================
+ Hits        31622    31674      +52     
+ Misses       5602     5574      -28     
+ Partials     1315     1310       -5

Files with missing lines	Coverage Δ
xds/internal/testutils/fakeclient/client.go	`81.25% <ø> (ø)`
xds/internal/xdsclient/clientimpl.go	`74.84% <ø> (+1.88%)`	⬆️
xds/internal/xdsclient/clientimpl_dump.go	`100.00% <100.00%> (ø)`
xds/internal/xdsclient/xdsresource/errors.go	`100.00% <ø> (ø)`
xds/server.go	`82.20% <100.00%> (-0.84%)`	⬇️
xds/server_options.go	`100.00% <100.00%> (ø)`
xds/internal/xdsclient/client_new.go	`88.33% <85.71%> (+2.61%)`	⬆️
xds/internal/resolver/xds_resolver.go	`77.94% <66.66%> (-1.44%)`	⬇️
xds/internal/xdsclient/client_refcounted.go	`76.19% <80.00%> (-5.87%)`	⬇️
xds/internal/xdsclient/pool.go	`83.78% <83.78%> (ø)`

... and 26 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?

Yes, I have kept the DefaultPool exported for now. As you called out in other comment, we set the bootstrap config in DefaultPool if env var is set with correct bootstrap config or contents. The resolver and server will have DefaultPool set by default pool but we have options to override for testing.

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.

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.

Done

xds/internal/xdsclient/pool.go

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?

Added in the docstring

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.

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.

Done

we should also export bootstrap.newConfigFromContents. This would allow non-test usages for NewPool

We already have bootstrap.NewConfigForTesting which internally call bootstrap.newConfigFromContents

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?

Reverted. Was just testing something and forgot to put it back.

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 · 2024-12-28T06:09:22Z

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?

This change is only going to change the usages of unit tests and hermatic e2e tests (within grpc-go repo). PSM interop tests, prod and other integration tests should not observe any behavior change.

The reason for this change was to bring back old behavior to be able to create xds clients with different bootstrap config even if they point to 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 8 times, most recently from 9588254 to 325def2 Compare December 21, 2024 23:09

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

60a07da

…or same server

purnesh42H added 4 commits December 28, 2024 11:30

remove pool.mu.Unlock()

a0f6f73

multiple servers with unique bootstrap conig test

cb96d14

use pool everywhere instead of new client functions

c40fbb2

Fix breaking tests

52be3dc

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

fbb07a5

purnesh42H force-pushed the xdsclient-pool-per-bootstrap-content branch 2 times, most recently from c9274f0 to af40ea3 Compare December 28, 2024 07:37

Comment out tests with no bootstrap config

d43ade8

purnesh42H force-pushed the xdsclient-pool-per-bootstrap-content branch from af40ea3 to d43ade8 Compare December 28, 2024 07:42

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

purnesh42H Dec 28, 2024

danielzhaotongliu Dec 5, 2024

purnesh42H Dec 28, 2024

danielzhaotongliu Dec 5, 2024

purnesh42H Dec 6, 2024

danielzhaotongliu Dec 12, 2024

easwars Dec 13, 2024

purnesh42H Dec 28, 2024

danielzhaotongliu left a comment

danielzhaotongliu Dec 12, 2024

easwars Dec 13, 2024

purnesh42H Dec 28, 2024

easwars Dec 13, 2024

purnesh42H Dec 28, 2024

easwars Dec 13, 2024

purnesh42H Dec 28, 2024

easwars Dec 13, 2024

easwars Dec 13, 2024

easwars Dec 13, 2024

purnesh42H Dec 28, 2024

purnesh42H Dec 28, 2024

easwars Dec 13, 2024

purnesh42H Dec 28, 2024

easwars Dec 13, 2024

easwars Dec 13, 2024

easwars Dec 13, 2024

purnesh42H Dec 28, 2024 •

edited

Loading

easwars Dec 13, 2024

purnesh42H commented Dec 28, 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

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

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 Dec 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purnesh42H commented Dec 28, 2024

purnesh42H commented Dec 4, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading

purnesh42H Dec 6, 2024 •

edited

Loading

purnesh42H Dec 28, 2024 •

edited

Loading