[Test] Allow configuring configDir for the Java test cluster

[ywangd]

For creating and deleting projects in multi-project tests, we need create and delete settings and secrets files on the fly. This PR adds such feature to the Java test cluster with an option to specify the config directory.

[elasticsearchmachine]

Pinging @elastic/es-delivery (Team:Delivery)

[rjernst]

This won't allow for the more nuanced setup of file-settings (which normally happen through a symlinked directory that is swapped atomically). Why not expose the config directory directly and let the test do with it as it wants?

[mark-vieira]

This won't allow for the more nuanced setup of file-settings (which normally happen through a symlinked directory that is swapped atomically). Why not expose the config directory directly and let the test do with it as it wants?

👍

I'd prefer something like this rather than adding additional methods to LocalClusterHandle. We could add a method like withConfigDir(Path configDir) to LocalSpecBuilder and plumb that through to AbstractLocalClusterFactory. Then you can just create a temporary folder which you keep a handle to and can muck around with to your heart's content.

FWIW, if you need to update the contents of an existing file on the fly, this is already possible via MutableResource. If you need to create new files in the config dir however, that would need some kind of solution as described above.

[mark-vieira]

As a general principle we want to keep the ClusterHandle interfaces pretty minimal and not have clusters be mutable in that way. We somewhat broke this convention with the updateStoredSecureSettings() that I think we can actually replace that with MutableResource now.

[tvernum]

We could add a method like withConfigDir(Path configDir) to LocalSpecBuilder and plumb that through to AbstractLocalClusterFactory. Then you can just create a temporary folder which you keep a handle to and can muck around with to your heart's content.

Would that point all nodes to the same directory?

I don't think we need per-node config right now, but it's a legitimate scenario to test, and I assume someone will want it eventually (and we'll keep extending LocalSpecBuilder until it can do everything)

[mark-vieira]

Would that point all nodes to the same directory?

Depends if it's configured at the "cluster" or "node" level. Anything on LocalSpecBuilder supports both scenarios.

[ywangd]

Depends if it's configured at the "cluster" or "node" level.

I was looking into this bit as well. IIUC, for node level we will use something like .withNode(builder -> builder.withConfigDir(...)). But I can see an issue also because it also works at the "cluster" level, nothing really prevents it but it certainly wrong since at least the node roles must be different for different nodes. So I am wondering whether we can have something like withBaseWorkingDir(path) from which the actual configDir is resolved. That means it will expose everything that cluster uses not just the configDir. But that could be considered as a feature as well?

[mark-vieira]

But I can see an issue also because it also works at the "cluster" level, nothing really prevents it but it certainly wrong since at least the node roles must be different for different nodes.

Right, it wouldn't work to use it at the cluster level unless the nodes were also all configured the same (or there was just a single node).

[mark-vieira]

So I am wondering whether we can have something like withBaseWorkingDir(path) from which the actual configDir is resolved.

What would this be necessary for? We can already override the data dir and repo dir via corresponding settings. We'd basically just be adding a convenient way to do the same for the config dir. I can't think of any reason we should be mucking with other stuff in the distribution in normal tests.

[mark-vieira]

So I am wondering whether we can have something like withBaseWorkingDir(path) from which the actual configDir is resolved.

I'm also just remembering that we try when at all possible to symlink most of the ES distribution files to avoid excess copies. Therefore exposing those directories such that their contents could be modified in tests could cause all sorts of issues.

[ywangd]

The intetion for my suggestion of withBaseWorkingDir is to avoid potential illegal use cases for withConfigDir on the cluster level. But if you do not consider that as an issue, I am happy to go with it.

[ywangd]

Or we can throw in LocalClusterSpecBuilder#withConfigDir but allow LocalNodeSpecBuilder#withConfigDir.

[mark-vieira]

Or we can throw in LocalClusterSpecBuilder#withConfigDir but allow LocalNodeSpecBuilder#withConfigDir.

I'd allow it for convenience. Single node clusters, or cluster where all nodes are configured identically, still make up the majority of our tests.

Also, as mentioned above, this footgun already exists for other things. You can configure path.data, or path.logs at the cluster level for a multi-node cluster which obviously won't work.

[ywangd]

I updated the PR based on the suggestion of a withConfigDir method. I made a minor tweak to the method so that it takes a function instead of a raw path. The reasons are:

1. Randomization does not work in the static field initialization so it is difficult to use LuceneTestCase's temporary directory method. A function makes the evaluation lazy and avoids the issue.
2. The function can decide whether to return a configDir based on the node name. In the linked PR, the function returns a config dir if the node name begins with index. Otherwise it returns null which means use the test cluster's default config dir. This makes the method more usable at cluster level without the need for a withNode call which also adds some more boilerplate code.

This is now ready for another look. Thank you!

[mark-vieira]

I don't think it's necessary to use a Function here. I would contend that a simple withConfigDir(Path) API is simpler, and easily satisfies your use case.

1. Randomization does not work in the static field initialization so it is difficult to use LuceneTestCase's temporary directory method. A function makes the evaluation lazy and avoids the issue.

The solution here is to use TemporaryFolder junit rule as we do in many other places. This also avoid the awkwardness of the fact that you have to capture the return value of that function somewhere anyway, since you need to have a reference to the created folder in order to later write to it.

2. The function can decide whether to return a configDir based on the node name. In the linked PR, the function returns a config dir if the node name begins with index. Otherwise it returns null which means use the test cluster's default config dir. This makes the method more usable at cluster level without the need for a withNode call which also adds some more boilerplate code.

Conditioning on the node name seems like a pretty specific use case. I'm not sure this is that compelling vs just doing .node(0, n -> n.withConfigDir(temporaryFolder.getRoot())). That seems like considerably less boilerplate than what you have in your PR. No need for conditional logic or an AtomicReference. Just create a TemporaryFolder, add it to your RuleChain and pass it in to the cluster config.

[ywangd]

@mark-vieira I updated the PR to have a withConfigDir(Path) method as you suggested.

I made it work for my test. But I could not simply use a RuleChain for the temporaryFolder and the cluster (probably due to my lack of knowledge). The method .node(0, n -> n.withConfigDir(temporaryFolder.getRoot().toPath())) eagerly evaluates first and throws (because TemporaryFolder#create has not been called) depsite of having a RuleChain.outerRule(temporaryFolder).around(cluster). So I ended up initialize the temporary folder statically and clean it up separately with a @AfterClass. I wonder if there is a better way or maybe withConfigDir should take a supplier? I will ping you on the test PR for context. Thanks!

[rjernst]

I wonder if there is a better way or maybe withConfigDir should take a supplier?

We do something similar in the temp dir rule for entitlements, see EntitlementsTestRule. We must make sure to be lazy in evaluation of config for the cluster since the rule will not have been run yet when the cluster object is being built.

[mark-vieira]

I wonder if there is a better way or maybe withConfigDir should take a supplier? I will ping you on the test PR for context. Thanks!

I think adding a supplier variant here makes sense. We have this elsewhere for many configuration methods as there are some things we don't want to evaluate until the cluster starts.

[mark-vieira]

I think we need to update this javadoc now.

[ywangd]

Yep. updated in d8bb5cd

[mark-vieira]

Why do we need to do this path -> file -> path conversion?

[ywangd]

The last conversion gives a String. The reason is that temporary directory created by Lucene temporariy directory, e.g. LuceneTestCase#createTempDir returns a Lucene FilterDirectory which always works for resolve(String) but throws for resolve(Path) if the given Path argument is not also a FilterDirectory. Since I changed the other PR to use Junit's TemporaryDirectory, this is no longer an issue for now. But I think it might be worthwhile to keep the change so that it does not just break if someone decides to use Lucene temporary in future?

[mark-vieira]

I don't have super strong feelings about working around idiosyncrasies with lucene types. Can this be simplified to relativePath.toString()?

[ywangd]

Thanks for the reviews! I updated the PR to have a withConfigDir(Supplier<Path>) instead.

[mark-vieira]

FWIW, I don't think we need to plumb laziness all the way through to the spec here. We only build the spec once we attempt to start the server, at which point everything we depend on needs to be initialized anyway. Let's just make this a Path to make downstream usages simpler.

[ywangd]

Makes sense. Pushed 0651da0

[mark-vieira]

On minor comment, otherwise LGTM.

[ywangd]

@elasticmachine update branch

[mark-vieira]

💚 All backports created successfully

Status	Branch	Result
✅	8.x
✅	9.0
✅	8.18

Questions ?

Please refer to the Backport tool documentation