Skip to content
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
merged 10 commits into from
May 25, 2021

Conversation

jmlrt
Copy link
Member

@jmlrt jmlrt commented May 12, 2021

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

@jmlrt jmlrt added elasticsearch enhancement New feature or request labels May 12, 2021
@jmlrt jmlrt requested a review from a team May 12, 2021 14:21
@jmlrt jmlrt changed the title elasticsearch node roles [elasticsearch] use new node.roles settings May 12, 2021
Copy link

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

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

Looks good but I'm not completely sure how these helm charts are versioned wrt to backward compatbility. Is this only going to apply to the charts used to deploy Elasticsearch 8.0?

data: "true"
ml: "false"
remote_cluster_client: "false"
- data

Choose a reason for hiding this comment

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

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" }}

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

master: "false"
ingest: "false"
data: "false"
roles: []
Copy link
Member Author

Choose a reason for hiding this comment

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

@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 🙏🏻

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.yml to detect that role is master and reuse it in Helm logic for example
  • The content of esConfig.elasticsearch.yml is directly mounted as a file inside the container so we can't merge it with some custom config generated by Helm

Copy link
Member Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@jasontedor Might have thoughts. I don't see a way to set an empty list via -E

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@jmlrt jmlrt requested review from rjernst and mark-vieira May 14, 2021 10:16
Copy link
Contributor

@Conky5 Conky5 left a comment

Choose a reason for hiding this comment

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

LGTM

@jmlrt
Copy link
Member Author

jmlrt commented May 25, 2021

jenkins test this please

1 similar comment
@jmlrt
Copy link
Member Author

jmlrt commented May 25, 2021

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 jmlrt merged commit c2689ff into elastic:master May 25, 2021
@jmlrt jmlrt deleted the elasticsearch-node-roles branch May 25, 2021 15:39
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[elasticsearch] Use node.roles instead of deprecated setting for Elasticsearch 7.9+
4 participants