[FEATURE] Add OPENSEARCH_JAVA_HOME env to override JAVA_HOME

[reta]

Signed-off-by: Andriy Redko [email protected]

Description

Add OPENSEARCH_JAVA_HOME environment variable to override JAVA_HOME variable. The OPENSEARCH_JAVA_HOME takes the precedence of JAVA_HOME, which in turn takes the precedence over bundled JDK.

Issues Resolved

Closes #1872

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[opensearch-ci-bot]

Can one of the admins verify this patch?

[opensearch-ci-bot]

❌   Gradle Check failure 19f843606c84569b0aec411588dba12f4e415ab4
Log 2106

Reports 2106

[opensearch-ci-bot]

❌   Gradle Check failure 2dd8617e6870dff7edfd66e5267bb62458e8b279
Log 2107

Reports 2107

[opensearch-ci-bot]

✅   Gradle Check success 14e0061928de1cae901a08d88d99b6966b1c69ad
Log 2115

Reports 2115

[opensearch-ci-bot]

✅   Gradle Check success da3eddf
Log 2121

Reports 2121

[dblock]

@stockholmux you're cool with this?

[stockholmux]

Is this wrong documentation being codified or wrong documentation being a good idea? 😂

My only issue is that #1872 should really be fixed in documentation.

At a higher level: if @reta has a specific use case and it doesn't cause other problems, it seems reasonable. The specific use case in this comment is a little vague to me - maybe it could be a little more concrete and put into an issue so this PR does fix or enhance something real (not bad docs).

@jcgraybill might have thoughts here too.

[reta]

Is this wrong documentation being codified or wrong documentation being a good idea? joy

Thank @stockholmux , yeah we could treat it both ways :D but I think is is actually missed feature

My only issue is that #1872 should really be fixed in documentation.

Sure, we could do that and drop the feature

At a higher level: if @reta has a specific use case and it doesn't cause other problems, it seems reasonable. The specific use case in this comment is a little vague to me - maybe it could be a little more concrete and put into an issue so this PR does fix or enhance something real (not bad docs).

I think the use case is very concrete, will try to flesh it out into the sub components:

1. Assume the user has an application which requires JDK-17 and connects to local Opensearch instance
2. The local Opensearch instance requires JDK-8 (to use CMS collector)
3. In this case, there are no way but to have 2 JDK distributions available on the host: JDK-8 and JDK-17. Setting JAVA_HOME to either of those will break one of the components.
4. By introducing OPENSEARCH_JAVA_HOME we are detaching any application / host specific concerns from the Opensearch by providing the way to configure dedicated JVM.

Hope it makes it more concrete, I have run into that many times with Elasticsearch deployments.

@jcgraybill might have thoughts here too.

👍

[andrross]

In this case, there are no way but to have 2 JDK distributions available on the host: JDK-8 and JDK-17. Setting JAVA_HOME to either of those will break one of the components.

Is it possible to set the JAVA_HOME environment variable only in the shell that launches OpenSearch so that it does not impact anything else on the machine? It might require the user to create a wrapper script or something similar but it might be simpler overall rather than adding additional logic into OpenSearch itself.

[reta]

It might require the user to create a wrapper script or something similar but it might be simpler overall rather than adding additional logic into OpenSearch itself.

Exactly, thanks @andrross , so we either off-load it to every current or/and future user which runs into that or do it once in the OpenSearch, I am fine with any of those.

PS: Also, forgot to mention, migration from Elasticsearch would be easier: ES_JAVA_HOME -> OPENSEARCH_JAVA_HOME :-)

[andrross]

My default opinion would be to avoid building this into OpenSearch, but having a drop-in replacement for ES_JAVA_HOME to make migration easier seems pretty compelling!

[dblock]

I went ahead and merged it. Please open issues for/document this/backport to 1.x.

[jcgraybill]

Hey @dblock this is still being discussed and wasn't ready to merge.

OpenSearch isn't the only software that's included in the distribution or the project that uses JAVA_HOME. If we're going to consider this change, it really should be a project-wide standard behavior, for both the java and node runtimes, and we should ensure it gets properly documented before rolling out. I can see @reta's point about how this makes migrations easier, but a new configuration knob should behave the same across the whole suite of software to not become an ease-of-use challenge.

[reta]

@jcgraybill the funny fact (my apologies if this is something you already know), the feature request / bug came out of properly documented feature (citing official docs here) which turns out to be not implemented.

OpenSearch isn't the only software that's included in the distribution or the project that uses JAVA_HOME. If we're going to consider this change, it really should be a project-wide standard behavior, for both the java and node runtimes,

For JVM runtime it is clear, it would make sense to make it a project-wide standard behavior. If there is a need to replicate similar deployment model for node runtimes - please create an issue so we could discuss and work on it (sadly I am just not aware of this part of the project).

... but a new configuration knob should behave the same across the whole suite of software

The JVM settings are irrelevant to node fe, so extrapolating them to the whole suite would not be useful. But there are standalone JVM components like performance analyzer which would make sense to consider (I will create an issue for that). If you have other components in mind, please add them or just create an issue, so we could make it " ... a project-wide standard behavior ... " 👍

Thank you.

[jcgraybill]

Haha, yes, I do remember that. @davidlago can you check, I think some of the security configuration scripts use JAVA_HOME.

OpenSearch Dashboards has different behavior to specify its node.js runtime: it looks like it tries the bundled binary then falls back to any that it can find in the user's path. There were mixed signals in the Kibana documentation about the extent to which this was supported. If we're going to support it at all, I'd love to see us introduce an OSD_NODE_HOME environment variable so that this affordance is similar across the project.

[dblock]

Since this got merged into 1.x we should decide quickly whether we want to roll it back.

@julesg Are you saying we should not have this feature at all, or are you saying we should add support for OPENSEARCH_JAVA_HOME anywhere it's missing?

• In security I see 3 trivial references in https://github.com/opensearch-project/security/search?q=JAVA_HOME in tools that we can fix quickly, https://github.com/opensearch-project/security/blob/4c182a216954b441cb698fe3f4d07f34a88b6d2f/tools/audit_config_migrater.sh, https://github.com/opensearch-project/security/blob/4c182a216954b441cb698fe3f4d07f34a88b6d2f/tools/hash.sh and https://github.com/opensearch-project/security/blob/4c182a216954b441cb698fe3f4d07f34a88b6d2f/tools/securityadmin.sh
• I see one in https://github.com/opensearch-project/performance-analyzer/blob/747dbe98812d6e83eeea303f074f9a98d420e721/pa_bin/performance-analyzer-agent

I think that's it.

[reta]

@dblock I am about to look at performance-analyzer, not sure why we would need to revert instead of making it " ... a project-wide standard behavior ... " as @jcgraybill suggested?

[dblock]

@dblock I am about to look at performance-analyzer, not sure why we would need to revert instead of making it " ... a project-wide standard behavior ... " as @jcgraybill suggested?

Right. I only wanted to hear a confirmation that we don't need to discuss whether this will become project-wide behavior beyond here/now (keyword: "consider").

[stockholmux]

Is there a better place to track this effort since it's spanning multiple repos?
(I'm guessing opensearch-project/project-website#628 is dependent upon not only this but the project wide action(s)?)

[jcgraybill]

Yes, @dblock or @reta could you create a meta-issue for this? This change should be coordinated across all the places it's made, plus docs and OpenSearch Dashboards, so that everything changes in the same version. I think once the full extent of the change is visible, it'll be clearer to see whether this new configuration knob adds useful enough power to outweigh any extra complexity.

[dblock]

I see how they can be related, but I don't think we should lump OpenSearch Java home and OpenSearch Dashboards node home in the same feature.

I'm reopening this one and making it into a meta issue.

[jcgraybill]

The project's default approach should be that the same action should produce the same behavior in all the software a single user is likely to use: that's a design principle that reduces complexity and makes things easier to use. So we should take a project-wide perspective for things like this, and I do think configuring the runtime should be treated as a single behavior.

[dblock]

The project's default approach should be that the same action should produce the same behavior in all the software a single user is likely to use: that's a design principle that reduces complexity and makes things easier to use. So we should take a project-wide perspective for things like this, and I do think configuring the runtime should be treated as a single behavior.

I opened opensearch-project/OpenSearch-Dashboards#1219.