[elasticsearch] make service configurable #123

kimxogus · 2019-05-03T19:29:28Z

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

In case of migration, master node name's prefix can be different. As we are already getting master node name only, I think checking if string is empty is enough

elasticmachine · 2019-05-03T19:29:29Z

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?

kimxogus · 2019-05-03T19:44:27Z

elasticsearch/templates/service.yaml

@@ -26,11 +34,10 @@ metadata:
    release: {{ .Release.Name | quote }}
    chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
    app: "{{ template "uname" . }}"
-  annotations:
-    # Create endpoints also if the related pod isn't ready
-    service.alpha.kubernetes.io/tolerate-unready-endpoints: "true"


This annotation is deprecated kubernetes/kubernetes#63742

kimxogus · 2019-05-03T19:45:16Z

also added some service values

Crazybus · 2019-05-07T12:50:32Z

elasticsearch/templates/statefulset.yaml

@@ -242,7 +242,7 @@ spec:
          cleanup () {
            while true ; do
              local master="$(http "/_cat/master?h=node")"
-              if [[ $master == "{{ template "uname" . }}"* && $master != "${NODE_NAME}" ]]; then
+              if [[ $master && $master != "${NODE_NAME}" ]]; then


Why change this? It just seems less safe than what was already in there. Yes it will fail if there is an empty string, however if the API returns anything else (or some kind of weird error) then it is going to be a non-empty string and this will pass.

Did you find an issue with the current logic that this fixes?

I migrated es cluster from a cluster(all nodes have master, ingest and data role) to this chart. Previous master was not prefixed by uname, so I had to wait several minutes during rollout caused by configuration change when nodes from both old and new cluster exists before exclude old nodes from cluster shard allocation.
I think this is unnecessary because --fail option is passed to curl(https://github.com/elastic/helm-charts/blob/master/elasticsearch/templates/statefulset.yaml#L239), script will exit if an error is returned from API($master won't have a value).
As h=node query param is passed, $master string will only contain node name of master node if it's not empty.

Was my explanation not enough? Then I can restore this because this only happens in specific migration case.

I think this is unnecessary because --fail option is passed to curl(https://github.com/elastic/helm-charts/blob/master/elasticsearch/templates/statefulset.yaml#L239), script will exit if an error is returned from API($master won't have a value).

Thanks for noticing this. This combination of set -e and curl --fail will mean that this script might exit early if the call fails. Ideally we want this loop to keep on running until we can see that a new master exists that isn't the current pod. Exiting early is not what we want to happen here.

My concern is whether or not the API will return an error when there is no master or not. If the API returns a weird message like "no master yet" with a 200 then it's possible for the script to exit too early. Looking at the code or Elasticsearch it looks like it will actually return a dash (-) if there is no master found.

Then I can restore this because this only happens in specific migration case.

Did it just mean that Kubernetes waited for the 120 second timeout while stopping each of the masters?

I think what actually makes a lot more sense is to check that it starts with {{ template "masterService" . }}. That way this will always point to the prefix for the right master even during migrations like in https://github.com/elastic/helm-charts/blob/master/elasticsearch/examples/migration/README.md

Was my explanation not enough?

Sorry for the slow reply. I'm currently on a work trip and only have a few quiet moments to sneak some work in. I'll be at Kubecon in Barcelona next week too so things will be slow for a little while. If you are at Kubecon come and say hi at the Elastic booth :)

Did it just mean that Kubernetes waited for the 120 second timeout while stopping each of the masters?

Yes, I had to wait 120 seconds per each master node.

I think what actually makes a lot more sense is to check that it starts with {{ template "masterService" . }}. That way this will always point to the prefix for the right master even during migrations like in /elasticsearch/examples/migration/README.md@master

Changed to {{ template "masterService" . }}!

I'm currently on a work trip and only have a few quiet moments to sneak some work in. I'll be at Kubecon in Barcelona next week too so things will be slow for a little while. If you are at Kubecon come and say hi at the Elastic booth :)

I'm not going to this Kubecon, but I hope to visit and say hi someday :)

elasticsearch/templates/service.yaml

elasticsearch/README.md

Crazybus · 2019-05-07T12:56:04Z

jenkins test this please

elasticsearch/templates/statefulset.yaml

Crazybus · 2019-05-15T22:13:57Z

elasticsearch/templates/statefulset.yaml

@@ -242,7 +242,7 @@ spec:
          cleanup () {
            while true ; do
              local master="$(http "/_cat/master?h=node")"
-              if [[ $master == "{{ template "uname" . }}"* && $master != "${NODE_NAME}" ]]; then
+              if [[ $master && $master != "${NODE_NAME}" ]]; then


I think this is unnecessary because --fail option is passed to curl(https://github.com/elastic/helm-charts/blob/master/elasticsearch/templates/statefulset.yaml#L239), script will exit if an error is returned from API($master won't have a value).

Thanks for noticing this. This combination of set -e and curl --fail will mean that this script might exit early if the call fails. Ideally we want this loop to keep on running until we can see that a new master exists that isn't the current pod. Exiting early is not what we want to happen here.

My concern is whether or not the API will return an error when there is no master or not. If the API returns a weird message like "no master yet" with a 200 then it's possible for the script to exit too early. Looking at the code or Elasticsearch it looks like it will actually return a dash (-) if there is no master found.

Then I can restore this because this only happens in specific migration case.

Did it just mean that Kubernetes waited for the 120 second timeout while stopping each of the masters?

I think what actually makes a lot more sense is to check that it starts with {{ template "masterService" . }}. That way this will always point to the prefix for the right master even during migrations like in https://github.com/elastic/helm-charts/blob/master/elasticsearch/examples/migration/README.md

Was my explanation not enough?

Sorry for the slow reply. I'm currently on a work trip and only have a few quiet moments to sneak some work in. I'll be at Kubecon in Barcelona next week too so things will be slow for a little while. If you are at Kubecon come and say hi at the Elastic booth :)

…-master-check

Crazybus

Apart from the table formatting this LGTM!

Thanks again for another great contribution :)

elasticsearch/README.md

Crazybus · 2019-05-27T13:38:49Z

jenkins test this please

Crazybus

LGTM!

Crazybus · 2019-06-12T13:48:57Z

jenkins test this please

Crazybus · 2019-06-12T14:24:48Z

Thank you for another great contribution!

check only if master node string is empty

09d4e6b

kimxogus commented May 3, 2019

View reviewed changes

refactor services

8fcbca9

kimxogus force-pushed the elasticsearch/fix-master-check branch from 7fda166 to 8fcbca9 Compare May 3, 2019 19:57

kimxogus added 2 commits May 4, 2019 05:02

update pytest spec

14aff5a

rearrange values

8ed5081

kimxogus changed the title ~~check only if master node string is empty~~ check only if master node string is empty and service improvements May 3, 2019

Merge branch 'master' into elasticsearch/fix-master-check

c490beb

Crazybus suggested changes May 7, 2019

View reviewed changes

kimxogus added 2 commits May 7, 2019 23:20

fix test failure, update README

5dc0e48

set empty string if curl fails

da6c398

kimxogus commented May 7, 2019

View reviewed changes

elasticsearch/templates/statefulset.yaml Show resolved Hide resolved

Crazybus suggested changes May 15, 2019

View reviewed changes

kimxogus added 2 commits May 16, 2019 09:14

change required master prefix

dc4820c

Merge remote-tracking branch 'upstream/master' into elasticsearch/fix…

0ceb09d

…-master-check

kimxogus changed the title ~~check only if master node string is empty and service improvements~~ [elasticsearch] make service improvements May 20, 2019

kimxogus changed the title ~~[elasticsearch] make service improvements~~ [elasticsearch] make service configurable May 20, 2019

Crazybus suggested changes May 27, 2019

View reviewed changes

elasticsearch/README.md Outdated Show resolved Hide resolved

kimxogus added 3 commits June 7, 2019 17:41

Merge branch 'master' into elasticsearch/fix-master-check

0e75063

fix README.md

d7bbd0a

fix test failure

1a911be

Crazybus approved these changes Jun 12, 2019

View reviewed changes

Crazybus mentioned this pull request Jun 12, 2019

NotePort option for elasticsearch backend #157

Closed

Crazybus merged commit 3c7a09c into elastic:master Jun 12, 2019

kimxogus deleted the elasticsearch/fix-master-check branch June 19, 2019 08:35

spommerening mentioned this pull request Jun 24, 2019

[elasticsearch] Service type doesn't get set using helm chart available at elastic/elasticsearch but works with cloned chart. #185

Closed

Crazybus mentioned this pull request Jul 30, 2019

publishNotReadyAddresses not working #237

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[elasticsearch] make service configurable #123

[elasticsearch] make service configurable #123

kimxogus commented May 3, 2019 •

edited

Loading

elasticmachine commented May 3, 2019

kimxogus May 3, 2019 •

edited

Loading

kimxogus commented May 3, 2019

Crazybus May 7, 2019

kimxogus May 7, 2019

kimxogus May 13, 2019

Crazybus May 15, 2019

kimxogus May 16, 2019

Crazybus commented May 7, 2019

Crazybus May 15, 2019

Crazybus left a comment

Crazybus commented May 27, 2019

Crazybus left a comment

Crazybus commented Jun 12, 2019

Crazybus commented Jun 12, 2019

[elasticsearch] make service configurable #123

[elasticsearch] make service configurable #123

Conversation

kimxogus commented May 3, 2019 • edited Loading

elasticmachine commented May 3, 2019

kimxogus May 3, 2019 • edited Loading

Choose a reason for hiding this comment

kimxogus commented May 3, 2019

Crazybus May 7, 2019

Choose a reason for hiding this comment

kimxogus May 7, 2019

Choose a reason for hiding this comment

kimxogus May 13, 2019

Choose a reason for hiding this comment

Crazybus May 15, 2019

Choose a reason for hiding this comment

kimxogus May 16, 2019

Choose a reason for hiding this comment

Crazybus commented May 7, 2019

Crazybus May 15, 2019

Choose a reason for hiding this comment

Crazybus left a comment

Choose a reason for hiding this comment

Crazybus commented May 27, 2019

Crazybus left a comment

Choose a reason for hiding this comment

Crazybus commented Jun 12, 2019

Crazybus commented Jun 12, 2019

kimxogus commented May 3, 2019 •

edited

Loading

kimxogus May 3, 2019 •

edited

Loading