This repository has been archived by the owner on May 16, 2023. It is now read-only.
[elasticsearch] use new node.roles settings #1186
Merged
+74
−73
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit update Elasticsearch chart to use the new node.roles settings introduced in elastic/elasticsearch#54998.
jmlrt
changed the title
mark-vieira
reviewed
May 12, 2021
| data: "true" | ||
| ml: "false" | ||
| remote_cluster_client: "false" | ||
| - data |
There was a problem hiding this comment.
I believe this should have the ingest role as well.
| @@ -295,15 +295,12 @@ spec: | |||
| valueFrom: | |||
| fieldRef: | |||
| fieldPath: metadata.name | |||
| {{- if eq .Values.roles.master "true" }} | |||
There was a problem hiding this comment.
This is coming from the Helm template right? Not elasticsearch.yml? So effectively we've changed how you specify roles to only support the new node.roles setting? If so, how do we handle compatibility with versions of Elasticsearch that don't have the node.roles setting?
There was a problem hiding this comment.
Yes this is coming from the Helm template to define some environment variables into Elasticsearch container.
If so, how do we handle compatibility with versions of Elasticsearch that don't have the node.roles setting?
helm-charts repo is using release branches following the same model as elastic/elasticsearch. We expect to merge this PR only on master branch so it will only be released with helm-charts 8.x which will use Elasticsearch 8.x Docker images.
Currently master branch is using 8.0.0-SNAPSHOT image which is why CI tests is failing, while all [released versions](https://github.com/elastic/helm-charts/releases] are using aligned Elasticsearch versions.
Note that anybody can override the imageTag value, to use the chart from master branch with an old Elasticsearch version for example. However this is not something we support and we don't try to handle backward compatibility in this scenario.
jmlrt
commented
May 12, 2021
| master: "false" | ||
| ingest: "false" | ||
| data: "false" | ||
| roles: [] |
There was a problem hiding this comment.
@mark-vieira
This configuration doesn't work to set a coordinating node and instead adds all default roles to the node.
I followed https://www.elastic.co/guide/en/elasticsearch/reference/current/modules-node.html#coordinating-only-node, but we define node.roles as environment variable is the chart instead of configuring it in elasticsearch.yaml so this configuration translates to node.roles=, environment variable in the Elasticsearch container
I did a few tries but couldn't find the correct syntax for setting client node with environment variable.
Some example reproduced using an Elasticsearch container outside of Helm charts:
➜ ~ docker run -e "node.roles=," docker.elastic.co/elasticsearch/elasticsearch:8.0.0-SNAPSHOT|grep role
{"@timestamp":"2021-05-12T16:50:06.917Z", "log.level": "INFO", "message":"node name [afa554bbd8cc], node ID [8TGL3o2qRkeuNw3TCbaxFg], cluster name [docker-cluster], roles [data, remote_cluster_client, ingest, master, ml]", "service.name":"ES_ECS","process.thread.name":"main","log.logger":"org.elasticsearch.node.Node","type":"server","node.name":"afa554bbd8cc","cluster.name":"docker-cluster"}
...➜ ~ docker run -e "node.roles=" docker.elastic.co/elasticsearch/elasticsearch:8.0.0-SNAPSHOT|grep role
{"@timestamp":"2021-05-12T16:50:50.805Z", "log.level": "INFO", "message":"node name [574604b8f312], node ID [K9RDU2WUSHGVZRMWlAeGqQ], cluster name [docker-cluster], roles [remote_cluster_client, master, ml, ingest, transform, data]", "service.name":"ES_ECS","process.thread.name":"main","log.logger":"org.elasticsearch.node.Node","type":"server","node.name":"574604b8f312","cluster.name":"docker-cluster"}
...➜ ~ docker run -e "node.roles=[]" docker.elastic.co/elasticsearch/elasticsearch:8.0.0-SNAPSHOT|grep role
uncaught exception in thread [main]
{"@timestamp":"2021-05-12T16:51:19.595Z", "log.level":"ERROR", "message":"uncaught exception in thread [main]", "service.name":"ES_ECS","process.thread.name":"main","log.logger":"org.elasticsearch.bootstrap.ElasticsearchUncaughtExceptionHandler","type":"server","node.name":"cbd6de59c5ee","cluster.name":"docker-cluster","error.type":"org.elasticsearch.bootstrap.StartupException","error.message":"java.lang.IllegalArgumentException: unknown role [[]]","error.stack_trace":"java.lang.IllegalArgumentException: unknown role [[]]\n\tat org.elasticsearch.cluster.node.DiscoveryNode.getRoleFromRoleName(DiscoveryNode.java:454)\n\tat java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)\n\tat java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1625)\n\tat java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)\n\tat java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)\n\tat java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)\n\tat java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)\n\tat java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:578)\n\tat org.elasticsearch.common.settings.Setting.lambda$listSetting$44(Setting.java:1474)\n\tat org.elasticsearch.common.settings.Setting.get(Setting.java:441)\n\tat org.elasticsearch.common.settings.Setting.get(Setting.java:435)\n\tat org.elasticsearch.cluster.node.DiscoveryNode.getRolesFromSettings(DiscoveryNode.java:222)\n\tat org.elasticsearch.cluster.node.DiscoveryNode.isDataNode(DiscoveryNode.java:82)\n\tat org.elasticsearch.env.NodeEnvironment.<init>(NodeEnvironment.java:289)\n\tat org.elasticsearch.node.Node.<init>(Node.java:335)\n\tat org.elasticsearch.node.Node.<init>(Node.java:268)\n\tat org.elasticsearch.bootstrap.Bootstrap$5.<init>(Bootstrap.java:225)\n\tat org.elasticsearch.bootstrap.Bootstrap.setup(Bootstrap.java:225)\n\tat org.elasticsearch.bootstrap.Bootstrap.init(Bootstrap.java:387)\n\tat org.elasticsearch.bootstrap.Elasticsearch.init(Elasticsearch.java:170)\n\tat org.elasticsearch.bootstrap.Elasticsearch.execute(Elasticsearch.java:161)\n\tat org.elasticsearch.cli.EnvironmentAwareCommand.execute(EnvironmentAwareCommand.java:86)\n\tat org.elasticsearch.cli.Command.mainWithoutErrorHandling(Command.java:127)\n\tat org.elasticsearch.cli.Command.main(Command.java:90)\n\tat org.elasticsearch.bootstrap.Elasticsearch.main(Elasticsearch.java:126)\n\tat org.elasticsearch.bootstrap.Elasticsearch.main(Elasticsearch.java:92)\nFor complete error details, refer to the log at /usr/share/elasticsearch/logs/docker-cluster.log\n"}
java.lang.IllegalArgumentException: unknown role [[]]
...➜ ~ docker run -e "node.roles=null" docker.elastic.co/elasticsearch/elasticsearch:8.0.0-SNAPSHOT|grep role
{"@timestamp":"2021-05-12T16:52:18.560Z", "log.level":"ERROR", "message":"uncaught exception in thread [main]", "service.name":"ES_ECS","process.thread.name":"main","log.logger":"org.elasticsearch.bootstrap.ElasticsearchUncaughtExceptionHandler","type":"server","node.name":"e7767eaf1286","cluster.name":"docker-cluster","error.type":"org.elasticsearch.bootstrap.StartupException","error.message":"java.lang.IllegalArgumentException: unknown role [null]","error.stack_trace":"java.lang.IllegalArgumentException: unknown role [null]\n\tat org.elasticsearch.cluster.node.DiscoveryNode.getRoleFromRoleName(DiscoveryNode.java:454)\n\tat java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)\n\tat java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1625)\n\tat java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)\n\tat java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)\n\tat java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)\n\tat java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)\n\tat java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:578)\n\tat org.elasticsearch.common.settings.Setting.lambda$listSetting$44(Setting.java:1474)\n\tat org.elasticsearch.common.settings.Setting.get(Setting.java:441)\n\tat org.elasticsearch.common.settings.Setting.get(Setting.java:435)\n\tat org.elasticsearch.cluster.node.DiscoveryNode.getRolesFromSettings(DiscoveryNode.java:222)\n\tat org.elasticsearch.cluster.node.DiscoveryNode.isDataNode(DiscoveryNode.java:82)\n\tat org.elasticsearch.env.NodeEnvironment.<init>(NodeEnvironment.java:289)\n\tat org.elasticsearch.node.Node.<init>(Node.java:335)\n\tat org.elasticsearch.node.Node.<init>(Node.java:268)\n\tat org.elasticsearch.bootstrap.Bootstrap$5.<init>(Bootstrap.java:225)\n\tat org.elasticsearch.bootstrap.Bootstrap.setup(Bootstrap.java:225)\n\tat org.elasticsearch.bootstrap.Bootstrap.init(Bootstrap.java:387)\n\tat org.elasticsearch.bootstrap.Elasticsearch.init(Elasticsearch.java:170)\n\tat org.elasticsearch.bootstrap.Elasticsearch.execute(Elasticsearch.java:161)\n\tat org.elasticsearch.cli.EnvironmentAwareCommand.execute(EnvironmentAwareCommand.java:86)\n\tat org.elasticsearch.cli.Command.mainWithoutErrorHandling(Command.java:127)\n\tat org.elasticsearch.cli.Command.main(Command.java:90)\n\tat org.elasticsearch.bootstrap.Elasticsearch.main(Elasticsearch.java:126)\n\tat org.elasticsearch.bootstrap.Elasticsearch.main(Elasticsearch.java:92)\nFor complete error details, refer to the log at /usr/share/elasticsearch/logs/docker-cluster.log\n"}
uncaught exception in thread [main]
java.lang.IllegalArgumentException: unknown role [null]
...Can you help finding the good syntax 🙏🏻
There was a problem hiding this comment.
Ah, so the helm charts don't actually generate an elasticsearch.yaml file? Instead we rely on environment variables exclusively?
@rjernst the -E stuff is handled by the core settings stuff isn't it? I don't think that's Docker-specific. How would one denote an empty list via that syntax?
There was a problem hiding this comment.
We allow users to define elasticsearch.yaml config using esConfig value, but for everything that is configured by the chart itself using custom values we have to use environment variables for a few reasons:
- Helm can't parse the content of
esConfig.elasticsearch.ymlto detect that role is master and reuse it in Helm logic for example - The content of
esConfig.elasticsearch.ymlis directly mounted as a file inside the container so we can't merge it with some custom config generated by Helm
There was a problem hiding this comment.
seems related to elastic/elasticsearch#65577 and elastic/cloud-on-k8s#3718
There was a problem hiding this comment.
Yeah, given that issue is still open I'm not sure we have a real solution here for coordinating nodes. My only other though it to just use voting_only as that's pretty much functionally equivalent. The only different being that node will also participate in master node elections, which is probably ok. The real purpose of a coordinating node is to ensure it's only workload is handling requests.
There was a problem hiding this comment.
@jasontedor Might have thoughts. I don't see a way to set an empty list via -E
There was a problem hiding this comment.
I found a workaround in e1bf227 by adding empty roles list in both roles chart value (to override default value) and node.roles settings in elasticsearch.yml.
I think we can use this workaround for now, but it could be great to find a way to define coordinating node properly using environment variable only.
this is needed because `data_frozen` role doesn't exist in Elasticsearch < 7.12.0
|
jenkins test this please |
1 similar comment
|
jenkins test this please |
pull bot
pushed a commit
to areller/helm-charts
that referenced
this pull request
May 25, 2021
* [elasticsearch] use new node.roles settings This commit update Elasticsearch chart to use the new node.roles settings introduced in elastic/elasticsearch#54998. * [elasticsearch] update doc * [elasticsearch] update examples * add link to roles doc * add workaround for coordinating node * update roles list * fixup! update roles list * remove data_frozen from default roles this is needed because `data_frozen` role doesn't exist in Elasticsearch < 7.12.0 * fixup! remove data_frozen from default roles
pull bot
pushed a commit
to areller/helm-charts
that referenced
this pull request
May 25, 2021
This commit update Elasticsearch chart to use the new node.roles settings introduced in elastic/elasticsearch#54998.
pull bot
pushed a commit
to areller/helm-charts
that referenced
this pull request
May 25, 2021
This commit update Elasticsearch chart to use the new node.roles settings introduced in elastic/elasticsearch#54998.
pull bot
pushed a commit
to areller/helm-charts
that referenced
this pull request
May 25, 2021
* [elasticsearch] use new node.roles settings This commit update Elasticsearch chart to use the new node.roles settings introduced in elastic/elasticsearch#54998. * [elasticsearch] update doc * [elasticsearch] update examples * add link to roles doc * add workaround for coordinating node * update roles list * fixup! update roles list * remove data_frozen from default roles this is needed because `data_frozen` role doesn't exist in Elasticsearch < 7.12.0 * fixup! remove data_frozen from default roles
jmlrt
added a commit
to nflaig/helm-charts
that referenced
this pull request
Jul 6, 2021
* [elasticsearch] use new node.roles settings This commit update Elasticsearch chart to use the new node.roles settings introduced in elastic/elasticsearch#54998. * [elasticsearch] update doc * [elasticsearch] update examples * add link to roles doc * add workaround for coordinating node * update roles list * fixup! update roles list * remove data_frozen from default roles this is needed because `data_frozen` role doesn't exist in Elasticsearch < 7.12.0 * fixup! remove data_frozen from default roles
jmlrt
added a commit
to nflaig/helm-charts
that referenced
this pull request
Jul 6, 2021
This commit update Elasticsearch chart to use the new node.roles settings introduced in elastic/elasticsearch#54998.
jmlrt
added a commit
to nflaig/helm-charts
that referenced
this pull request
Jul 6, 2021
This commit update Elasticsearch chart to use the new node.roles settings introduced in elastic/elasticsearch#54998.
jmlrt
added a commit
to nflaig/helm-charts
that referenced
this pull request
Jul 6, 2021
* [elasticsearch] use new node.roles settings This commit update Elasticsearch chart to use the new node.roles settings introduced in elastic/elasticsearch#54998. * [elasticsearch] update doc * [elasticsearch] update examples * add link to roles doc * add workaround for coordinating node * update roles list * fixup! update roles list * remove data_frozen from default roles this is needed because `data_frozen` role doesn't exist in Elasticsearch < 7.12.0 * fixup! remove data_frozen from default roles
jmlrt
added a commit
to nflaig/helm-charts
that referenced
this pull request
Jul 6, 2021
This commit update Elasticsearch chart to use the new node.roles settings introduced in elastic/elasticsearch#54998.
jmlrt
added a commit
to jmlrt/helm-charts
that referenced
this pull request
Sep 8, 2022
This commit fix the node.roles variable for client role with Elasticsearch version > 8.3.0. As client nodes are nodes that don't have any other roles, setting the client roles is done by configuring `node.roles: []` (empty list). Elasticsearch chart usually define `node.roles` as an environment variable, however, for client nodes, setting an empty list as value of an environment variable isn't recognized by Elasticsearch so we were required to also add it to the `elasticsearch.yaml` config file (more details in elastic#1186 (comment)). Starting with Elasticsearch 8.3.0 this is not working anymore and Elasticsearch fails to start is a `node.roles` environment variable is defined with an empty list as value. This commit define the `node.roles` environment variable only if the `roles` list isn't empty. Fixes also the tests for the `multi` example. Relates to elastic#1186
jmlrt
added a commit
that referenced
this pull request
Sep 9, 2022
This commit fix the node.roles variable for client role with Elasticsearch version > 8.3.0. As client nodes are nodes that don't have any other roles, setting the client roles is done by configuring `node.roles: []` (empty list). Elasticsearch chart usually define `node.roles` as an environment variable, however, for client nodes, setting an empty list as value of an environment variable isn't recognized by Elasticsearch so we were required to also add it to the `elasticsearch.yaml` config file (more details in #1186 (comment)). Starting with Elasticsearch 8.3.0 this is not working anymore and Elasticsearch fails to start is a `node.roles` environment variable is defined with an empty list as value. This commit define the `node.roles` environment variable only if the `roles` list isn't empty. Fixes also the tests for the `multi` example. Relates to #1186
galina-tochilkin
pushed a commit
to mtp-devops/3d-party-helm
that referenced
this pull request
Dec 20, 2022
This commit fix the node.roles variable for client role with Elasticsearch version > 8.3.0. As client nodes are nodes that don't have any other roles, setting the client roles is done by configuring `node.roles: []` (empty list). Elasticsearch chart usually define `node.roles` as an environment variable, however, for client nodes, setting an empty list as value of an environment variable isn't recognized by Elasticsearch so we were required to also add it to the `elasticsearch.yaml` config file (more details in elastic/helm-charts#1186 (comment)). Starting with Elasticsearch 8.3.0 this is not working anymore and Elasticsearch fails to start is a `node.roles` environment variable is defined with an empty list as value. This commit define the `node.roles` environment variable only if the `roles` list isn't empty. Fixes also the tests for the `multi` example. Relates to elastic/helm-charts#1186
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR update Elasticsearch chart to use the new node.roles
settings introduced in elastic/elasticsearch#54998.
Replace #1057
Fix #855
cc @elastic/es-delivery