8000 ElasticSearch: Fix missing JRE in 5.x and 6.x by viren-nadkarni · Pull Request #12286 · localstack/localstack · GitHub
[go: up one dir, main page]
Skip to content

ElasticSearch: Fix missing JRE in 5.x and 6.x #12286

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Feb 24, 2025
Merged

ElasticSearch: Fix missing JRE in 5.x and 6.x #12286

merged 7 commits into from
Feb 24, 2025

Conversation

viren-nadkarni
Copy link
Member
@viren-nadkarni viren-nadkarni commented Feb 19, 2025 •
edited
Loading

Background

In #11462, we changed the ElasticSearch/OpenSearch installers to use the bundled JRE. However, based on #12231, it was identified that ElasticSearch 5.x and 6.x builds do not have a bundled JRE and instead require a separate JRE installation.

Changes

This PR introduces a specialised package installer for ElasticSearch 5.x and 6.x which manages the Java dependency through standardised approach.

Tests

Tested manually locally. On the CI, automated tests run only against the latest versions, which was also why this went undetected.

Related

Closes: #12231

@viren-nadkarni viren-nadkarni self-assigned this Feb 19, 2025
viren-nadkarni
viren-nadkarni commented Feb 19, 2025
View reviewed changes
localstack-core/localstack/services/opensearch/cluster.py Outdated
@@ -669,7 +670,7 @@ def os_user(self):
return constants.OS_USER_OPENSEARCH

def _ensure_installed(self):
elasticsearch_package.install(self.version)
elasticsearch_package.install(self.version, target=InstallTarget.VAR_LIBS)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

target uses InstallTarget.VAR_LIBS as a default anyway, but it is made explicit here because get_java_env_vars() method below needs it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: Unfortunately, this is not necessarily the case... If the installer is used by lpm, the InstallTarget.STATIC_LIBS will be used. So this will actually break when the package is installed at Docker build time. But this can easily be fixed by just making the get_java_env_vars independent of it (see comment below) 😉

@github-actions GitHub Actions
Copy link
github-actions bot commented Feb 19, 2025 •
edited
Loading

LocalStack Community integration with Pro

 2 files  ±    0   2 suites  ±0   8m 29s ⏱️ - 1h 41m 41s
48 tests  - 4 051  33 ✅  - 3 734  15 💤  - 317  0 ❌ ±0 
50 runs   - 4 051  33 ✅  - 3 734  17 💤  - 317  0 ❌ ±0 

Results for commit 9cdaea9. ± Comparison against base commit f269fe2.

This pull request removes 4051 tests. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

♻️ This comment has been updated with latest results.

@viren-nadkarni viren-nadkarni added the semver: patch Non-breaking changes which can be included in patch releases label Feb 19, 2025
viren-nadkarni
viren-nadkarni commented Feb 19, 2025
View reviewed changes
localstack-core/localstack/services/opensearch/cluster.py
# `discovery.type` had a different meaning in 5.x
if not self.version.startswith("Elasticsearch_5."):
settings["discovery.type"] = "single-node"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required because ES refuses to start if any config is invalid or unknown

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix @viren-nadkarni willing to give it a try once is released, I'm facing the same issue

@viren-nadkarni viren-nadkarni marked this pull request as ready for review February 20, 2025 08:51
@viren-nadkarni viren-nadkarni added this to the 4.2 milestone Feb 20, 2025
8000
alexrashed
alexrashed reviewed Feb 20, 2025
View reviewed changes
Copy link
Member
@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for jumping on this! Perfect implementation on the package installer to dynamically select the right installer for the version! 💯
It really only needs a small fix on the installer (see comments), then we are good to go! 🚀

localstack-core/localstack/services/opensearch/cluster.py Outdated
@@ -669,7 +670,7 @@ def os_user(self):
return constants.OS_USER_OPENSEARCH

def _ensure_installed(self):
elasticsearch_package.install(self.version)
elasticsearch_package.install(self.version, target=InstallTarget.VAR_LIBS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: Unfortunately, this is not necessarily the case... If the installer is used by lpm, the InstallTarget.STATIC_LIBS will be used. So this will actually break when the package is installed at Docker build time. But this can easily be fixed by just making the get_java_env_vars independent of it (see comment below) 😉

localstack-core/localstack/services/opensearch/packages.py Outdated
@@ -233,6 +236,12 @@ class ElasticsearchPackageInstaller(PackageInstaller):
def __init__(self, version: str):
super().__init__("elasticsearch", version)

def get_java_env_vars(self, target: InstallTarget) -> dict[str, str]:
install_dir = self._get_install_dir(target)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: This is the root of the issue outlined in the comment above. Here you really shouldn't use the target where the package is "about to be installed if it's not yet installed", but you should use get_installed_dir instead, which dynamically detects the installation directory where the package has already been installed.‏

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant suggestion, this will simplify everything. Fixed in 7eb8868

alexrashed
alexrashed approved these changes Feb 21, 2025
View reviewed changes
Copy link
Member
@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks for acting on the comment! Nice and clean! 🚀 💯

@viren-nadkarni viren-nadkarni merged commit 7224030 into master Feb 24, 2025
31 checks passed
< 9025 /span>
@viren-nadkarni viren-nadkarni deleted the es-legacy-release-jre branch February 24, 2025 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kenny-alvizuris-cko kenny-alvizuris-cko kenny-alvizuris-cko left review comments

@alexrashed alexrashed alexrashed approved these changes

@silv-io silv-io Awaiting requested review from silv-io silv-io is a code owner

Assignees

@viren-nadkarni viren-nadkarni

Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.

bug: Elastic Search 6.8 - JAVA_HOME doesn't exist
3 participants
@viren-nadkarni @alexrashed @kenny-alvizuris-cko
0