Unit testing gRPC client/server with xDS?

[chids]

👋,

PR #7620 (issue #7605) introduced the io.grpc.xds.bootstrap property as an alternative to the GRPC_XDS_BOOTSTRAP env var. This makes it possible to programmatically provide the xDS config (i.e. via System.setProperty). However, it by no means make it simple to unit test client/servers using xDS.

I see a need for unit tests with a transient xDS server running as part of the test (i.e. io.envoyproxy.controlplane:java-control-plane or similar) to allow clients to discover a server via xDS, all within a unit test.

While this works in single test isolation having multiple such tests running in parallell seems problematic given that these test then either need to use the same bootstrap config and thus the same host:port for xDS across all tests or risk concurrency issues when multiple tests want to use their own bootstrap config and point to it using System.setProperty.

Does my reasoning make sense or am I coming at this the wrong way? Is there an approach on how to unit test gRPC with xDS that I simply missed?

[dapengzhang0]

Bootstrap is designed to be static object per JVM, so testing in parallel in a shared JVM is not a good idea if the tests don't use a shared bootstrap. I would suggest either run them in sequence or fork multiple JVM processes.

[ejona86]

Related: grpc/grpc-go#4124

[voidzcy]

I'll note that the bootstrap config will be read for at most once per JVM. All XdsClient instances (shared by Channels in-use) created (in case of Channels being created, shutdown and created again) will be based on the same bootstrap config. So even though the path to bootstrap file can be set dynamically via the system property, it needs to be set before the first Channel is created.

It seems that was not what the change for supporting setting bootstrap path via a system property wanted. What you really want seems to be a feature of re-bootstrapping. Our current design is to use a shared XdsClient instance for multiple Channels, in which the XdsClient might have a longer lifetime than a Channel. But this would be hard for supporting rebootstrap.

One solution I can think of is to have a Channel API that turns off XdsClient sharing so that each Channel will read the bootstrap and create an XdsClient for itself separately. This might be more desirable for testing purposes, especially for unit tests that runs on the same JVM.

[chids]

It seems that was not what the change for supporting setting bootstrap path via a system property wanted. What you really want seems to be a feature of re-bootstrapping.

@voidzcy, introducing the system property solves the basic production use case of specifying the bootstrap once from code (rather than having to set an env var before code execution). Alas, once we started to look past that and consider development experience and, for example, how to do junit driven testing of our xDS setups...that's when we ended up here 🙂.

Our current design is to use a shared XdsClient instance for multiple Channels, in which the XdsClient might have a longer lifetime than a Channel. But this would be hard for supporting rebootstrap.

Just to be clear, I think this is reasonable and a fine default behaviour. I do however think it would make sense to offer some alternative to allow more dynamic and custom setups where bootstrap can be done per channel/resolver or whatever construct makes most sense.

One solution I can think of is to have a Channel API that turns off XdsClient sharing so that each Channel will read the bootstrap and create an XdsClient for itself separately. This might be more desirable for testing purposes, especially for unit tests that runs on the same JVM. (emphasis mine)

I'm not familiar enough with the grpc-java codebase to have an opinion of what some reasonable approaches might be. I do think, as stated above, that the current behaviour is a fine default one. Whereas for the case of unit testing, allowing some form of user controlled injection, in my experience, tends to work best. I.e. something akin to allowing one to inject the bootstrap config on a case by case basis as a String, Reader or similar. Perhaps when creating the channel or through some footgun'y control of the resolver 🤷.

All the above aside, we have been discussing this internally in parallell and think we ought to be able to address our testing needs with the current state of things. By creating a thread safe singleton xDS server where our junit tests can (de)register services they should be able to run in parallel and be fairly isolated from each other. Much like what @dapengzhang0 hinted at the first comment on this issue.

🙇 to @ejona86 for linking the issue in grpc-go.

[ejona86]

One solution I can think of is to have a Channel API that turns off XdsClient sharing so that each Channel will read the bootstrap and create an XdsClient for itself separately.

It'll be hard to do that via a Channel API, as the option itself needs to be plumbed to the XdsNameResolver. It'd probably be better to add a constructor to XdsNameResolverProvider to provide a bootstrap, and then manually registering that XdsNameResolverProvider in NameResolverRegistry. The test would the register at the beginning and deregister at the end. Note that tests may want to change the scheme to be "xds-mytest" or some such to allow running tests in parallel.

In Java that doesn't seem horrible. If Go is still explicitly plumbing the XdsClient that would work in Go (I see you can deregister resolvers for testing). C is no long explicitly plumbing the XdsClient, so it is only available globally. C thus would have to have a global API or they add back the explicit plumbing that they recently removed. If C has explicit plumbing then it could use a channel arg approach.

All off that only addresses client-side. Server-side could have an option on the builder to set the bootstrap.

Yet another option is to just to have a global API that changes the bootstrap. Under-the-hood we would stop using the old XdsClient and create a new XdsClient the next time it is needed. That could probably be done in each language, but doesn't allow running tests in parallel.

[chids]

It'd probably be better to add a constructor to XdsNameResolverProvider to provide a bootstrap, and then manually registering that XdsNameResolverProvider in NameResolverRegistry. The test would the register at the beginning and deregister at the end. Note that tests may want to change the scheme to be "xds-mytest" or some such to allow running tests in parallel.

@ejona86, allowing/expecting tests to setup xDS resolution using a manually instantiated resolver with a custom scheme seems like a nice approach. This, I assume, would offer the ability to setup one shared resolver for a suite of tests or, should one want to, use per-test resolvers each with their own unique scheme.

[ejona86]

This, I assume, would offer the ability to setup one shared resolver for a suite of tests or, should one want to, use per-test resolvers each with their own unique scheme.

Yep. Any tests run in series could reuse a scheme if they wanted, even if they were using different resolver instances. But yeah, it'd be up to the tests to manage the resource however they wish.

[chids]

I think #7900 is touching this issue, given what it's doing in terms of adding support for override constructs in SharedXdsClientPoolProvider.java and the programmatic construction of the bootstrap config in GoogleCloudToProdNameResolver.java.

[voidzcy]

We recently opened up a window to allow the resolver inject its self-generated bootstrap to be used for the whole system. This was originally designed for some Google internal infrastructure's use case. But it seems to be quite close to what grpc/grpc-go#4124 wanted, if it is made to be public. One thing that is not user-friendly for Java is that the JSON is pretty cumbersome to work with. We are using Map<String, ?>, it could be unhandy, but we could think about if this can be made better.

I will bring this up to discuss with other languages if we want to expose such thing for users wanting to programmatically set the bootstrap.

[ejona86]

This hasn't been forgotten about. We have a potential solution but it is making C nervous and they want more time to find alternatives. That's been dragging on because there's been limited "free" time, but at this point we've afforded them two additional weeks to try to come up with alternative ideas. We'll default to the current approach that has been cooking if nothing comes from that.

[voidzcy]

I've also reminded this is actually very similar to how we provided solutions for service config (ManagedChannelBuilder.defaultServiceConfig()).

Going through the XdsServerBuilder makes me think if it is also possible to have an XdsChannelBuilder (which is just a thin wrapper) with defaultBootstrap() API.

[ejona86]

Going through the XdsServerBuilder makes me think if it is also possible to have an XdsChannelBuilder (which is just a thin wrapper) with defaultBootstrap() API.

While we could, I think we're quite happy that the normal API works for xds. For server-side that would be easy. For client-side, even with a wrapper, we'd need to somehow plumb the pieces together to inject the configuration within the implementation of XdsChannelBuilder. So we'd sort of still have the same problem, the only difference is whether users see that plumbing or a custom API.

[ejona86]

We've come to agreement. Java and Go will allow you to inject the bootstrap into a new XdsNameResolver instance and have the user register it. C core will have a channel argument for passing in the bootstrap. It is unspecified and an implementation decision whether XdsClients will be shared between channels.

More concretely for Java, something like

Map<String,?> bootstrap = ...;
NameResolverProvider xds = XdsNameResolverProvider.createForTest("xds-mytest",  bootstrap);
NameResolverRegistry.getDefaultRegistry().register(xds);

// After test, could:
NameResolverRegistry.getDefaultRegistry().deregister(xds);

I honestly don't know if we want to put the method on XdsNameResolverProvider or somewhere else.