Skip to content
This repository has been archived by the owner on May 16, 2023. It is now read-only.

[elasticsearch] #1495 Configure JVM options files #1496

Merged
merged 2 commits into from
Dec 16, 2021

Conversation

ebuildy
Copy link
Contributor

@ebuildy ebuildy commented Dec 15, 2021

  • Chart version not bumped (the versions are all bumped and released at the same time)
  • README.md updated with any new values or changes
  • Updated template tests in ${CHART}/tests/*.py
  • Updated integration tests in ${CHART}/examples/*/test/goss.yaml

need a big review!

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

afirth
afirth previously approved these changes Dec 15, 2021
Copy link

@afirth afirth left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Member

@jmlrt jmlrt left a comment

Choose a reason for hiding this comment

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

This mostly looks good but I did a few tests and by comparing the java command line arguments didn't see any differences with settings java options using ES_JAVA_OPTS or jvm.options.d files:

  • using esJavaOpts: "-XX:ActiveProcessorCount=3" value:
# shell into the Elasticsearch pod
elasticsearch@elasticsearch-master-2:~$ ps -ef | grep java
elastic+       7       1  4 10:03 ?        00:01:05 /usr/share/elasticsearch/jdk/bin/java -Xshare:auto -Des.networkaddress.cache.ttl=60 -Des.networkaddress.cache.negative.ttl=10 -XX:+AlwaysPreTouch -Xss1m -Djava.awt.headless=true -Dfile.encoding=UTF-8 -Djna.nosys=true -XX:-OmitStackTraceInFastThrow -XX:+ShowCodeDetailsInExceptionMessages -Dio.netty.noUnsafe=true -Dio.netty.noKeySetOptimization=true -Dio.netty.recycler.maxCapacityPerThread=0 -Dio.netty.allocator.numDirectArenas=0 -Dlog4j.shutdownHookEnabled=false -Dlog4j2.disable.jmx=true -Dlog4j2.formatMsgNoLookups=true -Djava.locale.providers=SPI,COMPAT --add-opens=java.base/java.io=ALL-UNNAMED -XX:+UseG1GC -Djava.io.tmpdir=/tmp/elasticsearch-8376734510333767193 -XX:+HeapDumpOnOutOfMemoryError -XX:+ExitOnOutOfMemoryError -XX:HeapDumpPath=data -XX:ErrorFile=logs/hs_err_pid%p.log -Xlog:gc*,gc+age=trace,safepoint:file=logs/gc.log:utctime,pid,tags:filecount=32,filesize=64m -Des.cgroups.hierarchy.override=/ -XX:ActiveProcessorCount=3 -Xms1024m -Xmx1024m -XX:MaxDirectMemorySize=536870912 -XX:G1HeapRegionSize=4m -XX:InitiatingHeapOccupancyPercent=30 -XX:G1ReservePercent=15 -Des.path.home=/usr/share/elasticsearch -Des.path.conf=/usr/share/elasticsearch/config -Des.distribution.flavor=default -Des.distribution.type=docker -Des.bundled_jdk=true -cp /usr/share/elasticsearch/lib/* org.elasticsearch.bootstrap.Elasticsearch -Ediscovery.seed_hosts=elasticsearch-master-headless -Enode.name=elasticsearch-master-2 -Enetwork.host=0.0.0.0 -Ecluster.name=elasticsearch -Ecluster.initial_master_nodes=elasticsearch-master-0,elasticsearch-master-1,elasticsearch-master-2, -Enode.roles=master,data,data_content,data_hot,data_warm,data_cold,ingest,ml,remote_cluster_client,transform,
  • using esJVMOptions:
esJVMOptions:
  processors.options: |
    -XX:ActiveProcessorCount=3
# shell into the Elasticsearch pod
elasticsearch@elasticsearch-master-2:~$ ps -ef | grep java
elastic+       7       1 38 10:35 ?        00:00:07 /usr/share/elasticsearch/jdk/bin/java -Xshare:auto -Des.networkaddress.cache.ttl=60 -Des.networkaddress.cache.negative.ttl=10 -XX:+AlwaysPreTouch -Xss1m -Djava.awt.headless=true -Dfile.encoding=UTF-8 -Djna.nosys=true -XX:-OmitStackTraceInFastThrow -XX:+ShowCodeDetailsInExceptionMessages -Dio.netty.noUnsafe=true -Dio.netty.noKeySetOptimization=true -Dio.netty.recycler.maxCapacityPerThread=0 -Dio.netty.allocator.numDirectArenas=0 -Dlog4j.shutdownHookEnabled=false -Dlog4j2.disable.jmx=true -Dlog4j2.formatMsgNoLookups=true -Djava.locale.providers=SPI,COMPAT --add-opens=java.base/java.io=ALL-UNNAMED -XX:+UseG1GC -Djava.io.tmpdir=/tmp/elasticsearch-12061671589181367672 -XX:+HeapDumpOnOutOfMemoryError -XX:+ExitOnOutOfMemoryError -XX:HeapDumpPath=data -XX:ErrorFile=logs/hs_err_pid%p.log -Xlog:gc*,gc+age=trace,safepoint:file=logs/gc.log:utctime,pid,tags:filecount=32,filesize=64m -XX:ActiveProcessorCount=3 -Des.cgroups.hierarchy.override=/ -Xms1024m -Xmx1024m -XX:MaxDirectMemorySize=536870912 -XX:G1HeapRegionSize=4m -XX:InitiatingHeapOccupancyPercent=30 -XX:G1ReservePercent=15 -Des.path.home=/usr/share/elasticsearch -Des.path.conf=/usr/share/elasticsearch/config -Des.distribution.flavor=default -Des.distribution.type=docker -Des.bundled_jdk=true -cp /usr/share/elasticsearch/lib/* org.elasticsearch.bootstrap.Elasticsearch -Ediscovery.seed_hosts=elasticsearch-master-headless -Enode.name=elasticsearch-master-2 -Enetwork.host=0.0.0.0 -Ecluster.name=elasticsearch -Ecluster.initial_master_nodes=elasticsearch-master-0,elasticsearch-master-1,elasticsearch-master-2, -Enode.roles=master,data,data_content,data_hot,data_warm,data_cold,ingest,ml,remote_cluster_client,transform,

I'm not sure if this should totally replace the possibility to define ES_JAVA_OPTS or if we still need to keep it in addition to the possibility to define jvm options files.

WDYT @elastic/es-delivery?

@jmlrt jmlrt added elasticsearch enhancement New feature or request labels Dec 16, 2021
@pugnascotia
Copy link
Contributor

I actually wonder if the chart can go further, and instead of you having to decide whether the options go into the env var or a file, you just declare the options you want, and the chart takes care of it. For example:

esJVMOptions:
    - "-XX:ActiveProcessorCount=3"
    - "-XshowSettings"

But if we're adding support for deploying a specific file, I don't think we should necessarily remove esJavaOpts since we're not removing it from ES itself.

@jmlrt
Copy link
Member

jmlrt commented Dec 16, 2021

Thanks, @pugnascotia.

I actually wonder if the chart can go further, and instead of you having to decide whether the options go into the env var or a file, you just declare the options you want, and the chart takes care of it. For example:

esJVMOptions:
    - "-XX:ActiveProcessorCount=3"
    - "-XshowSettings"

Well, we could eventually use this syntax and add every item in esJVMOptions to some hardcoded file in jvm.options.d, however, we can't add logic in Helm to parse the list and decide for every item whether it should go to env var or a file.

In addition, the esJVMOptions syntax here is a standard syntax in Helm and K8S world and is already used for defining the other config files in these Helm charts.

But if we're adding support for deploying a specific file, I don't think we should necessarily remove esJavaOpts since we're not removing it from ES itself.

OK 👍🏻

@jmlrt
Copy link
Member

jmlrt commented Dec 16, 2021

jenkins test this please

Copy link
Member

@jmlrt jmlrt left a comment

Choose a reason for hiding this comment

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

Some changes are required to make Black formatter happy in elastic+helm-charts+pull-request+lint-python/1279.

Can you run black elasticsearch/tests/elasticsearch_test.py?

Also can you add a line for esJVMOptions description in the README file?

@jmlrt
Copy link
Member

jmlrt commented Dec 16, 2021

Also NIT but maybe renaming esJVMOptions to esJvmOptions for camel case

@jmlrt
Copy link
Member

jmlrt commented Dec 16, 2021

jenkins test this please

@jmlrt
Copy link
Member

jmlrt commented Dec 16, 2021

CI tests will fail because of #1443 but manual test is OK

@jmlrt jmlrt merged commit d6adfc1 into elastic:main Dec 16, 2021
jmlrt pushed a commit to jmlrt/helm-charts that referenced this pull request Dec 16, 2021
* [elasticsearch] elastic#1495 Configure JVM options files

* [elasticsearch] elastic#1495 Configure JVM options files
jmlrt pushed a commit to jmlrt/helm-charts that referenced this pull request Dec 16, 2021
* [elasticsearch] elastic#1495 Configure JVM options files

* [elasticsearch] elastic#1495 Configure JVM options files
jmlrt pushed a commit to jmlrt/helm-charts that referenced this pull request Dec 16, 2021
* [elasticsearch] elastic#1495 Configure JVM options files

* [elasticsearch] elastic#1495 Configure JVM options files
jmlrt added a commit that referenced this pull request Dec 21, 2021
* [elasticsearch] #1495 Configure JVM options files

* [elasticsearch] #1495 Configure JVM options files

Co-authored-by: Thomas Decaux <ebuildy@gmail.com>
jmlrt added a commit that referenced this pull request Dec 21, 2021
* [elasticsearch] #1495 Configure JVM options files

* [elasticsearch] #1495 Configure JVM options files

Co-authored-by: Thomas Decaux <ebuildy@gmail.com>
jmlrt added a commit that referenced this pull request Dec 21, 2021
* [elasticsearch] #1495 Configure JVM options files

* [elasticsearch] #1495 Configure JVM options files

Co-authored-by: Thomas Decaux <ebuildy@gmail.com>
@jmlrt jmlrt mentioned this pull request Mar 8, 2022
@jmlrt jmlrt mentioned this pull request Apr 21, 2022
This was referenced Sep 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants