[stable/elasticsearch] Elasticsearch v7.1.1

[tetianakravchenko]

What this PR does / why we need it:

• switch to es version 7.1.1

• adjust accordingly elasticsearch.yaml ( add cluster.initial_master_nodes)

• add log4j2.properties for ESv7

• I've also added (hasPrefix "6." .Values.appVersion) for mounting log4j2.properties and setting NODE_INGEST, seems like that was missed.

Which issue this PR fixes

• fixes [stable/elasticsearch] support 6.8 and 7.1 es versions #14376

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

• DCO signed
• Chart Version bumped
• Variables are documented in the README.md
• Title of the PR starts with chart name (e.g. [stable/chart])

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tetianakravchenko
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: rendhalver

If they are not already assigned, you can assign the PR to them by writing /assign @rendhalver in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• stable/elasticsearch/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

Hi @tetianakravchenko. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[t-d-d]

Note that you can't do a rolling update from 6.7 to 7.* . So should this be a major version bump? I'm not sure. But at least there should be some documentation in the readme. https://www.elastic.co/guide/en/elasticsearch/reference/current/setup-upgrade.html

[tetianakravchenko]

@t-d-d thank you for pointing it out! I've added readme and bumped version to 6.8, as I mentioned it in the readme as a version supported to perform update (6.8 to 7.1) 713768d

[tetianakravchenko]

/assign @rendhalver

[Towmeykaw]

We talked about this change on a meeting regarding the depreciation of this chart in favor of the official elasticsearch chart. The official chart already support v7 and the question is if a restart is required then it might be a good time to switch to that chart. Is something stopping people from switching which may make this upgrade needed?

[anas-aso]

@Towmeykaw from the official chart : https://github.com/elastic/helm-charts/tree/master/elasticsearch

This functionality is in beta status and may be changed or removed completely in a future release.

I believe this is enough for a reason to not switch to the official chart (yet). In addition, the official chart doesn't support preStop/preStart hook which we need in our case.

[Crazybus]

This functionality is in beta status and may be changed or removed completely in a future release.

I believe this is enough for a reason to not switch to the official chart (yet).

Thanks for calling this out. I bumped the status from alpha to beta before I had the official messaging ready (to have it done on time for Kubecon). I have just submitted a PR (elastic/helm-charts#158) to update this. This is the official messaging that I need to put in, however I'm fully committed to following semver and not making breaking changes between major releases. Only one breaking change was made when going from 6.x to 7.x and this was a simple values.yaml change to remove the default storage class name (https://github.com/elastic/helm-charts/releases/tag/7.0.0-alpha1). The only difference between this being in beta and stable now is the support you get if you are an Elastic customer.

In addition, the official chart doesn't support preStop/preStart hook which we need in our case.

The Elastic helm chart uses a different approach which I have found to be much more reliable. Instead of attempting to fully drain a node when stopping and starting, it instead waits for the cluster to be fully green during initial startup. The big advantage of this is that you can easily rollout changes without needing to sync all data to a new node and back again. This can be combined with setting "index.unassigned.node_left.delayed_timeout": "5m" to make sure that shards are never re-synced unnecessarily during rolling restarts.

Another issue with using lifecycle hooks for this is with how the execution is handled in Kubernetes which makes them not a very reliable thing to use for something as important as this. If a single call fails then it means that a node might not be added back into the cluster.

The current grep is also going to cause issues for clusters with more than 10 nodes.

charts/stable/elasticsearch/templates/configmap.yaml

Line 117 in 3b8631a

if ! echo "${SHARDS_ALLOCATION}" | grep -E "${NODE_NAME}"; then

If you have 10 nodes then a grep for master-1 is also going to match for master-11 which could lead to some unexpected results.

[t-d-d]

@Towmeykaw But it seems you can update from 6.8 to 7.
If this chart is to be deprecated my vote would be to make that clear at the top of the ReadMe and allow only none-breaking PRs for a time. The intend would be to give users of this chart some notice and a chance to switch over to the Elastic chart.

[anas-aso]

@Crazybus Thanks for the detailed clarification. I agree with @t-d-d that it should be clear that this chart won't support any newer ES versions and that it's in maintenance mode only.

[jethr0null]

@t-d-d @anas-aso We were actually chatting about this just last week. Here is the PR with the initial messaging letting folks know about the intent to move over to the Elastic maintained Charts. The added text also includes a link to the migration guide which outlines the process for moving from the community chart to the Elastic chart.

[tetianakravchenko]

@Crazybus Thank you for the clarification!

I will close this PR.

[tothdavid]

@Towmeykaw I believe there are some unaddressed issues about migrating to the official elasticsearch helm chart: These may not seem very important for some but actually this helm chart (stable/elasticsearch) follows a very different approach than the one in the elastic repo. That is the official elastic helm chart sets up only one role of a cluster, which means that you need to create a separate helm chart installation for master, client, data, or whatever roles you want in your cluster. Also you need to make sure that the different parts of your cluster will be able to see each other as this can not be provided by the official helm chart because by its nature one helm chart instance is only aware of one role of the cluster.
Also there is another issue: this stable/elasticsearch chart is used by the stable/elastic-stack chart which will setup an entire Elastic Stack with elasticsearch, logstash, kibana, and whatever other components you may want. So if you make this chart deprecated and will not support anything newer than 6.7 that means that you basically deprecate the elastic-stack chart as well (and actually there is no notice of this issue there at all).
So to make this work like before the elastic-stack chart also needs to be updated to utilize the official elasticsearch chart either in a way that it would use it in 3 instances (master, client, data) or even there could be an intermediate chart created (e.g. elasticsearch-cluster) that would take care of this aspect and the elastic-stack could just depend on this intermediate one. Also there is no official kibana, logstash or other similar charts in that repo so we still need to use the ones here.
On the other hand it would be much-much easier to just merge this pull request which could provide support for newer elasticsearch versions for us who are fine with this chart and could continue using elastic-stack chart as well for the time while there is nothing similar provided by Elastic or while elastic-stack chart would not support the new chart.

[hectorj2f]

I agree with @t-d-d that it should be clear that this chart won't support any newer ES versions and that it's in maintenance mode only.

I agree with that too. 👍