-
Notifications
You must be signed in to change notification settings - Fork 1.9k
custom command for elasticsearch master nodes #82
Conversation
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? |
# https://github.com/elastic/helm-charts/issues/63#issuecomment-471096950 | ||
# Run ES as a background task, and forward SIGTERM to it, then wait for it to exit | ||
trap 'kill $(jobs -p)' SIGTERM | ||
/usr/local/bin/docker-entrypoint.sh elasticsearch & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting the CMD to elasticsearch
breaks usage of environment variables with this docker image. Referring to what happens here: https://github.com/elastic/elasticsearch/blob/a520cc53dc2be904794299c9b00270d9ac9e80d9/distribution/docker/src/docker/bin/docker-entrypoint.sh#L32
Overriding the command here breaks anything which is using environment variables (which by default this chart does use).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed argument passing to es.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that broken usage of env vars. Maybe I couldn't found this problem because of my custom elasticsearch.yml
file.
Fixing broken env vars is beyond my knowledge of bash scripts, hope someone to fix this issue
wait | ||
# now keep the pod alive for 30s after ES dies so that we will refuse connections from | ||
# the new master rather than them needing to time out | ||
sleep 30 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 30 seconds? How do you know that this will be enough time? Magic numbers are not normally a good sign and should only be used if there is no way to determine how long we should wait.
Luckily it is possible to detect that a new master has been elected as I commented in #63 (comment). This is what I would prefer to see implemented. As it doesn't rely on magic numbers and it also means that we can continue to use the default entrypoint for the docker images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added sleepSecondsOnTermination
value and set 30
as default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making it configurable is certainly better than hard coding it. But this doesn't address my original feedback from #82 (comment). Why is this 30 seconds, and why is that long enough to prevent the problem defined in #63. Is this calculated based on something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just copied and pasted #63 (comment) . That's where 30 seconds came from. In my test environment, around 10 seconds were sufficient.
jenkins test this please |
I feel like maintaining a workaround in this chart isn't the right solution for this. A better approach might be to expose the Additionally, just like we have an |
#63
#63 (comment)
As fix from upstream will be in 7.1, this change seems to be needed now.