Restrict which tasks can use testclusters #45198
Conversation
This PR fixes a problem between the interaction of test-clusters and build cache. Before this any task could have used a cluster without tracking it as input. With this change a new interface is introduced to track the tasks that can use clusters and we do consider the cluster as input for all of them.
alpar-t
added
>non-issue
:Delivery/Build
|
Pinging @elastic/es-core-infra |
Question: Do we have any examples of this actually happening? Also, keep in mind, it only actually matters if that task was also being cached. |
buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/TestClustersTask.java
Outdated
Show resolved
Hide resolved
| @@ -123,7 +120,7 @@ private void createListClustersTask(Project project, NamedDomainObjectContainer< | |||
| private void createUseClusterTaskExtension(Project project, NamedDomainObjectContainer<ElasticsearchCluster> container) { | |||
| // register an extension for all current and future tasks, so that any task can declare that it wants to use a | |||
| // specific cluster. | |||
| project.getTasks().all((Task task) -> | |||
| project.getTasks().withType(TestClustersTask.class, (Task task) -> | |||
There was a problem hiding this comment.
We should try and align these two methods somehow at some point. That is, calling testCluster or useCluster would be all that is needed to register all the necessary wiring here. We could something like have the testCluster method on TestClustersTask reach out and grab the test clusters plugin and perform the registration.
We don't have to do this as part of this PR though.
|
@elasticmachine run elasticsearch-ci/default-distro |
|
@elasticmachine run elasticsearch-ci/2 |
| } | ||
|
|
||
| public void maybeStartCluster(ElasticsearchCluster cluster) { | ||
| // we only start the cluster before the actions, so we'll not start it if the task is up-to-date |
There was a problem hiding this comment.
This comment doesn't make much sense in this context. We should move it to where we define the TaskActionListener.
| } | ||
|
|
||
| public void stopCluster(ElasticsearchCluster cluster, boolean taskFailed) { | ||
| if (taskFailed) { |
There was a problem hiding this comment.
This condition is backwards, it just so happens to work because we are also inverting the taskFailed predicate at the call site.
| }); | ||
| } | ||
| ((TestClustersAware) task).getClusters() | ||
| .forEach(cluster -> registry.stopCluster(cluster, state.getFailure() == null)); |
There was a problem hiding this comment.
We are passing the wrong value for taskFailed here. When getFailure() == null it means the task was successful. We need to either invert all this or rename the taskFailed param on stopCluster() to taskSucceeded or something so this all makes sense.
* Restrict which tasks can use testclusters This PR fixes a problem between the interaction of test-clusters and build cache. Before this any task could have used a cluster without tracking it as input. With this change a new interface is introduced to track the tasks that can use clusters and we do consider the cluster as input for all of them.
This PR fixes a problem between the interaction of test-clusters and
build cache.
Before this any task could have used a cluster without tracking it as
input.
With this change a new interface is introduced to track the tasks that
can use clusters and we do consider the cluster as input for all of
them.