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

Changes required to install Elasticsearch 6 on staging #6421

Merged
merged 19 commits into from
Nov 14, 2024

Conversation

AmitPhulera
Copy link
Contributor

https://dimagi.atlassian.net/browse/SAAS-16062

Changes required to install Elasticsearch 6 on staging

Environments Affected

Staging

@zwets
Copy link
Contributor

zwets commented Oct 21, 2024

If I may chime in on this change, I would suggest also fixing this:

diff --git a/src/commcare_cloud/ansible/roles/elasticsearch/templates/config/jvm.options.j2 b/src/commcare_cloud/ansible/roles/elasticsearch/templates/config/jvm.options.j2
index 36761736b..50ba49a01 100644
--- a/src/commcare_cloud/ansible/roles/elasticsearch/templates/config/jvm.options.j2
+++ b/src/commcare_cloud/ansible/roles/elasticsearch/templates/config/jvm.options.j2
@@ -54,7 +54,7 @@
 14-:-XX:InitiatingHeapOccupancyPercent=30
 
 ## JVM temporary directory
--Djava.io.tmpdir=${ES_TMPDIR}
+-Djava.io.tmpdir={{ elasticsearch_data_dir }}/tmp
 
 ## heap dumps
 
@@ -67,7 +67,7 @@
 -XX:HeapDumpPath={{ elasticsearch_data_dir }}/logs/heapdump.hprof
 
 # specify an alternative path for JVM fatal error logs
--XX:ErrorFile={{ elasticsearch_data_dir }}logs/hs_err_pid%p.log
+-XX:ErrorFile={{ elasticsearch_data_dir }}/logs/hs_err_pid%p.log
 
 ## JDK 8 GC logging
 8:-XX:+PrintGCDetails

Regarding the fixes: jvm.options is a config file for the JVM, not a shell script, so -Djava.io.tmpdir=${ES_TMPDIR} results in a directory that is literally called /opt/elasticsearch-5.6.16/${ES_DIR} 😆

I did not submit my local fix) of simply commenting the line out, because on many systems the default java.io.tmpdir (/tmp, under which ES creates a unique directory jna--{number}) is mounted noexec, whereas apparently ES wants to execute scripts from it.

Clearly then this is an ES bug: rather than make deployers redirect the JVM tmpdir, they should do their JNA execution in some directory of their own, for instance simply tmp under their data directory. My fix above does exactly that.

Regardless of whether you stick to the directory called $ES_TMPDIR or call it tmp as I suggest, if it is not under /tmp you'll probably need to take care of its clean-up. Not sure if that's currently taken care of?

The second change in my diff above seemed obvious to me by just looking at it.

@AmitPhulera
Copy link
Contributor Author

@zwets Thank you for your suggestions. They were definitely an oversight from our end. I have incorporated them in eb11aa5.

Copy link
Contributor

@gherceg gherceg left a comment

Choose a reason for hiding this comment

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

One small comment but otherwise looks good

@@ -69,13 +63,6 @@ network.host: "{{ lookup('dig', inventory_hostname, wantlist=True)[0] }}"
indices.fielddata.cache.size: {{ elasticsearch_fielddata_cache_size }}
{% endif %}

{% if elasticsearch_enable_inline_groovy_scripts %}
{% if elasticsearch_version is version('5.6.16', '<=') %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just going off of this, these settings seemed to also apply to ES5 and therefore aren't strictly an ES2 setting? Are they still safe to remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this is safe to remove and is intentionally removed. This was there to in ES 5 to support if someone was using groovy scripts which are deprecated.
If someone installs commcare now they will not need es5 with groovy scripts so it is safe to remove for ES5 as well.

@AmitPhulera AmitPhulera merged commit af5f0f1 into master Nov 14, 2024
2 checks passed
@AmitPhulera AmitPhulera deleted the ap/es6-install branch November 14, 2024 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants