-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Description
Is your feature request related to a problem? Please describe
Currently in opensearch plugins use ./gradlew :integTest to run the ITs on local and also on the CIs. The commands create a local cluster and run the tests against it. On this local cluster CBs are disabled. Ref:
OpenSearch/buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java
Lines 1179 to 1182 in 767c07f
| // Temporarily disable the real memory usage circuit breaker. It depends on real memory usage which we have no full control | |
| // over and the REST client will not retry on circuit breaking exceptions yet (see #31986 for details). Once the REST client | |
| // can retry on circuit breaking exceptions, we can revert again to the default configuration. | |
| baseConfig.put("indices.breaker.total.use_real_memory", "false"); |
Due to this what happens is if there is some issues in the code that didn't get caught during CIs. This issue recently happened with k-NN plugin where in of the flow plugin was creating some large objects which was not required(Fix PR: opensearch-project/k-NN#2070). But none of the CIs were able to caught this and it was caught during the Jenkins IT runs as those runs happen remote cluster built using the generated distribution where the setting indices.breaker.total.use_real_memory is marked as true. In test cluster that gets created during CIs has this setting as False. Ref:
OpenSearch/buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java
Lines 1179 to 1182 in 767c07f
| // Temporarily disable the real memory usage circuit breaker. It depends on real memory usage which we have no full control | |
| // over and the REST client will not retry on circuit breaking exceptions yet (see #31986 for details). Once the REST client | |
| // can retry on circuit breaking exceptions, we can revert again to the default configuration. | |
| baseConfig.put("indices.breaker.total.use_real_memory", "false"); |
Some approaches I tried:
- I tried starting the integTest cluster using
./gradlew :integTest -Dtests.opensearch.indices.breaker.total.use_real_memory=truebut got this exception:
> Task :run FAILED
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':run'.
> Testclusters does not allow the following settings to be changed:[indices.breaker.total.use_real_memory] for node{::integTest-0}
which comes from this:
OpenSearch/buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java
Lines 1190 to 1197 in 767c07f
| HashSet<String> overriden = new HashSet<>(baseConfig.keySet()); | |
| overriden.retainAll(settings.keySet()); | |
| OVERRIDABLE_SETTINGS.forEach(overriden::remove); | |
| if (overriden.isEmpty() == false) { | |
| throw new IllegalArgumentException( | |
| "Testclusters does not allow the following settings to be changed:" + overriden + " for " + this | |
| ); | |
| } |
- I also tried to update the setting after cluster is created but this setting is not a dynamic setting hence, that approach also failed.
Describe the solution you'd like
Having a capability to indices.breaker.total.use_real_memory setting will ensure that plugins can run the integration tests and catch the issues related to Circuit breaker rather than finding them during the Jenkins builds or in production.
This setting is not a dynamic setting hence needs to be set during the run like this:
./gradlew :integTest -Dtests.opensearch.indices.breaker.total.use_real_memory=true
OpenSearch/buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java
Lines 1179 to 1182 in 767c07f
| // Temporarily disable the real memory usage circuit breaker. It depends on real memory usage which we have no full control | |
| // over and the REST client will not retry on circuit breaking exceptions yet (see #31986 for details). Once the REST client | |
| // can retry on circuit breaking exceptions, we can revert again to the default configuration. | |
| baseConfig.put("indices.breaker.total.use_real_memory", "false"); |
Solution
The solution is pretty simple for this, we just need to ensure that this setting is removed from overridden settings so that users can override it if required. I don't want to make this as by default true because of this:
OpenSearch/buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java
Lines 1179 to 1181 in 767c07f
| // Temporarily disable the real memory usage circuit breaker. It depends on real memory usage which we have no full control | |
| // over and the REST client will not retry on circuit breaking exceptions yet (see #31986 for details). Once the REST client | |
| // can retry on circuit breaking exceptions, we can revert again to the default configuration. |
Related component
Other
Describe alternatives you've considered
There is no alternative I can think of rather than building my own min distribution and then using it via -PcustomDistributionUrl flag
Additional context
No response