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
Next Next commit
[elasticsearch] use new node.roles settings
This commit update Elasticsearch chart to use the new node.roles
settings introduced in elastic/elasticsearch#54998.
  • Loading branch information
jmlrt committed May 12, 2021
commit 8f8c40c9b6e228e5be07f1bd6b283967cc758626
6 changes: 6 additions & 0 deletions elasticsearch/templates/_helpers.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,12 @@ We truncate at 63 chars because some Kubernetes name fields are limited to this
{{- end -}}
{{- end -}}

{{- define "elasticsearch.roles" -}}
{{- range $.Values.roles -}}
{{ . }},
{{- end -}}
{{- end -}}

{{- define "elasticsearch.esMajorVersion" -}}
{{- if .Values.esMajorVersion -}}
{{ .Values.esMajorVersion }}
Expand Down
13 changes: 3 additions & 10 deletions elasticsearch/templates/statefulset.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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.

{{- if ge (int (include "elasticsearch.esMajorVersion" .)) 7 }}
{{- if has "master" .Values.roles }}
- name: cluster.initial_master_nodes
value: "{{ template "elasticsearch.endpoints" . }}"
{{- else }}
- name: discovery.zen.minimum_master_nodes
value: "{{ .Values.minimumMasterNodes }}"
{{- end }}
{{- end }}
- name: node.roles
value: "{{ template "elasticsearch.roles" . }}"
{{- if lt (int (include "elasticsearch.esMajorVersion" .)) 7 }}
- name: discovery.zen.ping.unicast.hosts
value: "{{ template "elasticsearch.masterService" . }}-headless"
Expand All @@ -319,10 +316,6 @@ spec:
- name: ES_JAVA_OPTS
value: "{{ .Values.esJavaOpts }}"
{{- end }}
{{- range $role, $enabled := .Values.roles }}
- name: node.{{ $role }}
value: "{{ $enabled }}"
{{- end }}
{{- if .Values.extraEnvs }}
{{ toYaml .Values.extraEnvs | indent 10 }}
{{- end }}
Expand Down
21 changes: 4 additions & 17 deletions elasticsearch/tests/elasticsearch_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,7 @@ def test_defaults():
{"name": "discovery.seed_hosts", "value": uname + "-headless"},
{"name": "network.host", "value": "0.0.0.0"},
{"name": "cluster.name", "value": clusterName},
{"name": "node.master", "value": "true"},
{"name": "node.data", "value": "true"},
{"name": "node.ingest", "value": "true"},
{"name": "node.roles", "value": "master,ingest,data,remote_cluster_client,ml,"},
]

c = r["statefulset"][uname]["spec"]["template"]["spec"]["containers"][0]
Expand Down Expand Up @@ -174,7 +172,7 @@ def test_overriding_the_image_and_tag():
def test_set_initial_master_nodes():
config = """
roles:
master: "true"
- master
"""
r = helm_template(config)
env = r["statefulset"][uname]["spec"]["template"]["spec"]["containers"][0]["env"]
Expand All @@ -192,7 +190,7 @@ def test_set_initial_master_nodes():
def test_dont_set_initial_master_nodes_if_not_master():
config = """
roles:
master: "false"
- data
"""
r = helm_template(config)
env = r["statefulset"][uname]["spec"]["template"]["spec"]["containers"][0]["env"]
Expand All @@ -203,7 +201,7 @@ def test_dont_set_initial_master_nodes_if_not_master():
def test_set_discovery_seed_host():
config = """
roles:
master: "true"
- master
"""
r = helm_template(config)
env = r["statefulset"][uname]["spec"]["template"]["spec"]["containers"][0]["env"]
Expand All @@ -216,17 +214,6 @@ def test_set_discovery_seed_host():
assert e["name"] != "discovery.zen.ping.unicast.hosts"


def test_enabling_machine_learning_role():
config = """
roles:
ml: "true"
"""
r = helm_template(config)
env = r["statefulset"][uname]["spec"]["template"]["spec"]["containers"][0]["env"]

assert {"name": "node.ml", "value": "true"} in env


def test_adding_extra_env_vars():
config = """
extraEnvs:
Expand Down
12 changes: 6 additions & 6 deletions elasticsearch/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@ nodeGroup: "master"
masterService: ""

# Elasticsearch roles that will be applied to this nodeGroup
# These will be set as environment variables. E.g. node.master=true
# These will be set as environment variables. E.g. node.roles=master
roles:
master: "true"
ingest: "true"
data: "true"
remote_cluster_client: "true"
ml: "true"
- master
- ingest
- data
- remote_cluster_client
- ml

replicas: 3
minimumMasterNodes: 2
Expand Down