Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature Request] Allow indices.breaker.total.use_real_memory to be updated in Test clusters #15849

Closed
navneet1v opened this issue Sep 8, 2024 · 0 comments · Fixed by #15906
Labels
Build Build Tasks/Gradle Plugin, groovy scripts, build tools, Javadoc enforcement. enhancement Enhancement or improvement to existing feature or request untriaged

Comments

@navneet1v
Copy link
Contributor

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:

// 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:

// 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:

  1. I tried starting the integTest cluster using ./gradlew :integTest -Dtests.opensearch.indices.breaker.total.use_real_memory=true but 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:

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
);
}

  1. 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

// 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:

// 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Build Tasks/Gradle Plugin, groovy scripts, build tools, Javadoc enforcement. enhancement Enhancement or improvement to existing feature or request untriaged
Projects
None yet
1 participant