Rewrite elasticsearch multi example to use child charts. #22

jordansissel · 2018-12-29T07:11:04Z

As a newbie to helm, I found child charts interesting. This change makes the multi example now use a single release with child charts instead of two separate helm releases.

In practice, I don't know if this is the best route, because we'll want to upgrade masters before upgrading data nodes, for example, so this PR is mostly food-for-thought.

Thoughts? :)

jordansissel · 2018-12-29T07:13:40Z

we'll want to upgrade masters before upgrading data nodes

I suppose in this example, we could set imageTag separately for each child chart in the values.yaml and do an upgrade that way:

set imageTag on master to new version
make install to upgrade just the masters
set imageTag on data role to new version
make install to ugprade just the data nodes

It'd be identical to the the upgrade process for current code (in master) which has two files (data + master) and separate helm releases, probably?

Edit: I just tried this, and it works as expected (set imageTag: ... on the master, then make install results in a rolling upgrade of the masters)

Crazybus

LGTM apart from needing a rebase to fix the integration tests and pull in the latest changes and version from master. The tests are currently failing because Google dropped support for GKE 1.9 clusters so the build is failing when trying to create that cluster.

Apart from the needed rebase I love everything about this. This is a much nicer way to handle clusters with multiple node groups in a "helm native" way. Thanks a lot for trying this out, it has been on the todo list for too long and I'm glad to see it is really simple and easy to set up!

This approach has made me realise something else that could be changed to improve managing a full stack with helm charts using this method. In this example you are setting masterService: "multi-master". If you also wanted to add Kibana to this setup you would now also need to add elasticsearchURL: "http://multi-master:9200" which would be a bit silly.

So I think this means we should standardise on the connection variables for Elasticsearch so that they can be reused for Elasticsearch, Kibana, Logstash, Beats + whatever else in the future. I'll create a separate issue for it, to keep this PR clean.

elasticsearch/examples/multi/Chart.yaml

jordansissel · 2018-12-31T22:41:19Z

If you also wanted to add Kibana to this setup

Indeed! I'll share my stack chart with you after new years which includes Kibana.

jordansissel · 2019-01-02T21:35:35Z

Rebased!

Crazybus · 2019-01-04T14:22:03Z

CI tests are failing because helm isn't initialised.

22:54:45 make[1]: Entering directory `/app/elasticsearch/examples/multi'
22:54:45 helm dep update
22:54:45 Error: Couldn't load repositories file (/app/.helm/repository/repositories.yaml).
22:54:45 You might need to run `helm init` (or `helm init --client-only` if tiller is already installed)

I think adding the helm init --client-only into the testing Dockerfile would be the cleanest way to fix it. That is defined in helpers/terraform/Dockerfile

rendhalver · 2019-01-07T19:15:58Z

Doesn't this go against the original design of the Chart?
One of the original intents for having multiple releases was to be able to swap out the data node without needing to delete the data. If we have one release for a cluster we could have the same problem.
https://github.com/elastic/helm-charts/tree/master/elasticsearch#usage-notes-and-getting-started

Crazybus · 2019-01-08T19:31:31Z

Doesn't this go against the original design of the Chart?
One of the original intents for having multiple releases was to be able to swap out the data node without needing to delete the data. If we have one release for a cluster we could have the same problem.

This still functions identically to the previous example. Instead of adding a new release and a new upgrade command you can instead add a new node block.

es-data2:
  <<: *defaults # include with the default settings
  nodeGroup: "data2"

  roles:
    master: "false"
    ingest: "true"
    data: "true"

To then demote the old nodes you would then bump the replicas down one at a time, or manually move the shards off them then remove the node block.

While this is the same in function the terminology in the readme indeed no longer matches. I think we can replace the terminology "release" with "install". The important piece being that the chart itself does not attempt to handle multiple groups of node itself.

Upgrading the master group first would involve bumping the imageTag for only the masters first and still works in the same way as it would have before.

The motivation for doing it this way was to remove code duplication when specifying configuration that is shared between multiple node groups. This could also be solved by including multiple values files.

One advantage of using this method is that it gives you a single version for your cluster. If you want to roll back the previous change there is only one version to rollback. Rather than manually needing to figure out the order that you made the changes to the separate releases.

@rendhalver If you do find this example confusing though maybe it is a sign that we shouldn't merge this. Or maybe instead have it as a separate example of how to manage multiple node groups in a single release.

rendhalver · 2019-01-08T20:55:43Z

It's not confusing I understand what you are trying to do.

I do admit I didn't read what the code does before I put in my comment.

I think having it as a separate example is a better idea.

If we start enforcing opinions then we are at risk of breaking the un-opinionated nature of the Chart.

rendhalver · 2019-01-25T17:35:28Z

On a side note I copied this Chart into our local repo and we have used it to deploy 4 new clusters and it works really well.
I vote for adding this as a new top level chart so people can make use of it.

jordansissel · 2019-02-07T05:00:03Z

@rendhalver I have some other ideas to make this chart style even more useful (so that settings can be inherited across parent->child chart values). Hopefully will have a PR to demo it this week. After that, we can discuss any future support for this style.

And thank you for testing! :)

rendhalver · 2019-02-07T13:08:28Z

That sounds pretty cool!
Tag me on your new PR. :)

jordansissel · 2019-02-07T17:44:11Z

@Crazybus I think this is ready for another round of reviews. CI tests failure seems unrelated (timeout waiting for the GKE cluster to come online)

The parent/child chart relationship is roughly: inheritance -> secure-elasticsearch -> elasticsearch The objective is to demonstrate a parent chart (secure-elasticsearch) which sets broad business-specific defaults for Elasticsearch, and this chart is then used to deploy master and data nodes, inheriting those settings. This example covers a few areas: 1) Having a child chart (secure-elasticsearch) add some baseline settings for the cluster. 2) A parent chart to inherit and amend the settings of the child secure-elasticsearch chart. 3) Using chart aliases to deploy master and data nodes independently Same result as multi in PR #22, but instead of using yaml anchors, this uses helm child charts. 4) Add a job which deploy the license payload. 5) Add a job for deploying cluster and index settings. This can be useful for setting cluster-wide things like concurrent_recoveries, or for index-level settings such as node_left recovery delay. These settings live along-side the node settings intended for easier reading. 6) (From previous commit) Elasticsearch node settings (elasticsearch.yml) is now specified by a structured value instead of a text block. The `Makefile` does a few things to help make this testable outside of Elastic's infrastructure. Mainly, all secrets and key material are generated by `make secrets`. This is instead of using the `vault read` method that other examples are using. Note: The `secure-elasticsearch` chart is in a parent directory because for some reason helm doesn't work if it were in ./charts/secure-elasticsearch due to some behavior in helm itself: helm/helm#2887

desaintmartin · 2019-08-27T12:49:31Z

Any update? Do you need help on this one? I can help bringing a multi example with several node groups (hot/warm/cold)

desaintmartin · 2019-08-27T12:52:42Z

elasticsearch/examples/multi/values.yaml

+# base settings for both node types (data + master)
+es-defaults: &defaults
+  clusterName: "multi"
+  masterService: "multi-master"


I think we don't need this, as the masterService is computed from cluster name and not group name.

desaintmartin · 2019-08-27T13:40:24Z

elasticsearch/examples/multi/values.yaml

+    ingest: "false"
+    data: "false"
+
+# To define common settings, it may be useful to use YAML anchors to use the same 


In helm 2.14.3, the anchor has to be defined BEFORE using it, so moving this part on top of the file works.

Crazybus · 2019-09-18T12:16:00Z

Any update? Do you need help on this one? I can help bringing a multi example with several node groups (hot/warm/cold)

Pleas do! (In a separate PR).

I think having it as a separate example is a better idea.

I agree with @rendhalver and think that we should add this is a example to the existing multi one that is being used for testing.

botelastic · 2020-01-07T15:57:59Z

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. To track this PR (even if closed), please open a corresponding issue if one does not already exist.

desaintmartin · 2020-01-20T14:16:12Z

Any update? Do you need help on this one? I can help bringing a multi example with several node groups (hot/warm/cold)

Pleas do! (In a separate PR).

5 months later, PR at #452

botelastic · 2020-04-19T14:21:00Z

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. To track this PR (even if closed), please open a corresponding issue if one does not already exist.

botelastic · 2020-05-19T15:10:01Z

This PR has been automatically closed because it has not had recent activity since being marked as stale. Please reopen when work resumes.

jordansissel requested a review from Crazybus December 29, 2018 07:11

Crazybus suggested changes Dec 31, 2018

View reviewed changes

elasticsearch/examples/multi/Chart.yaml Show resolved Hide resolved

Crazybus mentioned this pull request Dec 31, 2018

Standardize on Elasticsearch connection value names #23

Closed

jordansissel force-pushed the example/multi-use-child-charts branch 2 times, most recently from e92b7e2 to 03c5d6f Compare December 31, 2018 22:43

rendhalver mentioned this pull request Jan 25, 2019

Migrating stable/elasticsearch charts out of helm/charts helm/charts#10543

Closed

jordansissel force-pushed the example/multi-use-child-charts branch from 03c5d6f to 5244c4f Compare February 7, 2019 04:53

jordansissel mentioned this pull request Feb 8, 2019

Example: Inherited settings, child charts, and configuration jobs. #54

Closed

jordansissel added 3 commits March 12, 2019 22:22

Rewrite multi example to use child charts.

3ce42d7

Add helm init --client-only

70b9382

Bump version to match

5fffc89

jordansissel force-pushed the example/multi-use-child-charts branch from 4db3db4 to 5fffc89 Compare March 13, 2019 05:22

rendhalver mentioned this pull request Jul 22, 2019

Offical Elastic-Stack Chart #221

Closed

desaintmartin reviewed Aug 27, 2019

View reviewed changes

jmlrt added the enhancement New feature or request label Oct 17, 2019

botelastic bot added the triage/stale label Jan 7, 2020

botelastic bot removed the triage/stale label Jan 20, 2020

botelastic bot added the triage/stale label Apr 19, 2020

botelastic bot closed this May 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite elasticsearch multi example to use child charts. #22

Rewrite elasticsearch multi example to use child charts. #22

jordansissel commented Dec 29, 2018

jordansissel commented Dec 29, 2018 •

edited

Loading

Crazybus left a comment

jordansissel commented Dec 31, 2018

jordansissel commented Jan 2, 2019

Crazybus commented Jan 4, 2019

rendhalver commented Jan 7, 2019

Crazybus commented Jan 8, 2019

rendhalver commented Jan 8, 2019

rendhalver commented Jan 25, 2019

jordansissel commented Feb 7, 2019

rendhalver commented Feb 7, 2019

jordansissel commented Feb 7, 2019

desaintmartin commented Aug 27, 2019 •

edited

Loading

desaintmartin Aug 27, 2019 •

edited

Loading

desaintmartin Aug 27, 2019

Crazybus commented Sep 18, 2019

botelastic bot commented Jan 7, 2020

desaintmartin commented Jan 20, 2020 •

edited

Loading

botelastic bot commented Apr 19, 2020

botelastic bot commented May 19, 2020

Rewrite elasticsearch multi example to use child charts. #22

Rewrite elasticsearch multi example to use child charts. #22

Conversation

jordansissel commented Dec 29, 2018

jordansissel commented Dec 29, 2018 • edited Loading

Crazybus left a comment

Choose a reason for hiding this comment

jordansissel commented Dec 31, 2018

jordansissel commented Jan 2, 2019

Crazybus commented Jan 4, 2019

rendhalver commented Jan 7, 2019

Crazybus commented Jan 8, 2019

rendhalver commented Jan 8, 2019

rendhalver commented Jan 25, 2019

jordansissel commented Feb 7, 2019

rendhalver commented Feb 7, 2019

jordansissel commented Feb 7, 2019

desaintmartin commented Aug 27, 2019 • edited Loading

desaintmartin Aug 27, 2019 • edited Loading

Choose a reason for hiding this comment

desaintmartin Aug 27, 2019

Choose a reason for hiding this comment

Crazybus commented Sep 18, 2019

botelastic bot commented Jan 7, 2020

desaintmartin commented Jan 20, 2020 • edited Loading

botelastic bot commented Apr 19, 2020

botelastic bot commented May 19, 2020

jordansissel commented Dec 29, 2018 •

edited

Loading

desaintmartin commented Aug 27, 2019 •

edited

Loading

desaintmartin Aug 27, 2019 •

edited

Loading

desaintmartin commented Jan 20, 2020 •

edited

Loading