Testclusters: implement support to install plugins#39116
Testclusters: implement support to install plugins#39116alpar-t merged 7 commits intoelastic:masterfrom
Conversation
|
Pinging @elastic/es-core-infra |
| } | ||
|
|
||
| public void loggedExec(Action<ExecSpec> action) { | ||
| LoggedExec.exec(project, action); |
There was a problem hiding this comment.
This method should really not even be part of LoggedExec. Perhaps better to just move all that stuff directly into it's own class, or even GradleServicesAdapter.
There was a problem hiding this comment.
The intent was to eliminate uses of LoggedExec as a task and keeps the methods only.
I agree there's opportunity for cleanup and improvement here, but wouldn't want to do it in this PR.
buildSrc/src/main/java/org/elasticsearch/gradle/Distribution.java
Outdated
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/Distribution.java
Outdated
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java
Outdated
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java
Show resolved
Hide resolved
| config.put("cluster.initial_master_nodes", "[" + nodeName + "]"); | ||
| } | ||
| try { | ||
| Files.delete(configFile); |
There was a problem hiding this comment.
I've seen a number of instances of us creating big strings in memory. LoggedExec is another, more problematic instance of this. Individually, these are not big issues but we need to be careful about this stuff as the build becomes more complex, and we rely more on the daemon, memory pressure will become an issue. We should try and keep this in mind and prefer streaming this stuff when possible.
There was a problem hiding this comment.
LoggedExec is a special case. We could redirect output to files, but most of the exec calls are tiny invocations, so the I/O overhead to disk is much more than writing into the heap. But I agree here we should consider writing the file through a stream instead of building up in a String.
There was a problem hiding this comment.
Agreed. If it hasn't been problematic yet it may just not be an issue. We still have a situation where a bad execution could potentially create a load of output that blows out the heap. We could pretty simply put some safeguards here to ensure the output doesn't exceed some configured threshold and truncate if it does. Separate discussion 😄
buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/TestClustersPlugin.java
Outdated
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/TestClustersPlugin.java
Show resolved
Hide resolved
rjernst
left a comment
There was a problem hiding this comment.
Looks pretty good, a few comments
| } | ||
| private void createWorkingDir(Path distroExtractDir) throws IOException { | ||
| services.sync(spec -> { | ||
| spec.from(distroExtractDir.toFile()); |
There was a problem hiding this comment.
In ClusterFormationTasks, we always have a fresh directory we extract into. This isn't great for build performance, but it means we don't have any possiblity of old stuff being used. From a reproducibility perspective, I think we should wipe the data/logs/temp dirs here?
There was a problem hiding this comment.
FYI, sync() does exactly this. In contrast to copy() a sync first deletes the target folder before performing the copy.
| config.put("cluster.initial_master_nodes", "[" + nodeName + "]"); | ||
| } | ||
| try { | ||
| Files.delete(configFile); |
There was a problem hiding this comment.
Why do we need to delete the config file when it is written below this?
There was a problem hiding this comment.
I removed it, it's an oversight.
| config.put("cluster.initial_master_nodes", "[" + nodeName + "]"); | ||
| } | ||
| try { | ||
| Files.delete(configFile); |
There was a problem hiding this comment.
LoggedExec is a special case. We could redirect output to files, but most of the exec calls are tiny invocations, so the I/O overhead to disk is much more than writing into the heap. But I agree here we should consider writing the file through a stream instead of building up in a String.
|
@mark-vieira @rjernst ready for another review |
* methods to run bin script * Add support for specifying and installing plugins * Add OS specific distirbution support * Add test to verify plugin installed * Remove use of Gradle internal OperatingSystem
* methods to run bin script * Add support for specifying and installing plugins * Add OS specific distirbution support * Add test to verify plugin installed * Remove use of Gradle internal OperatingSystem
This change extends the DSL and implements calling the commands to install plugins.
The plugin install dir is not configurable, for this reason we have to sync the working dir.
This fixes some other issues for which the plugin was muted and thus closes #37889.
Since the test was muted, some changes around distribution name are also required,
so there are some change in this PR that don't relate directly, but I didn't want to add new functionality without enabling the test.