Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce ECK-managed resources Helm Charts #5781

Merged
merged 52 commits into from
Jul 26, 2022

Conversation

naemono
Copy link
Contributor

@naemono naemono commented Jun 15, 2022

closes #5505

This PR contains the first version of the ECK-managed resources helm charts including

  1. eck-stack chart, which is a parent chart for installing all of the stack components
  2. eck-elasticsearch chart for installing Elasticsearch
  3. eck-kibana chart for installing Kibana

The main idea is to keep the configuration as simple as possible. With this in mind, the default configuration will deploy:

  1. Elasticsearch quickstart example
  2. Kibana will not be deployed unless enabled with --set kibana.enabled=true

The examples in eck-stack/examples only include the quickstart example, and an additional example for both Elasticsearch, and Kibana, for ease of initial review. If preferred, we can add examples for all the options on this page. Let us know.

Not included in this PR:

  • Beats
  • Elastic Maps Server
  • Enterprise Search
  • APM Server
  • Agent

cc @jmlrt @Kushmaro @framsouza

TODO

  • Write documentation in the official ECK /docs directory.
  • Document using example values.
  • Potentially add tests.
  • Add CI/CD to release chart(s) (doesn't appear necessary, as existing process will release all charts in repo)
  • Documentation concerning this feature being Enterprise

Open Questions

  • How do we release this/these charts independent of the eck-operator? Do we see the need for this initially? (just create issue for this in the future?)
  • How do we use the dev helm charts repo with this/these charts? The dependencies on the Chart.yaml in eck-stack doesn't support this.
  • Do we want to document every single option in eck-ES, and eck-Kibana readme, similar to other charts? See example

@naemono naemono added the >feature Adds or discusses adding a feature to the product label Jun 15, 2022
@naemono naemono changed the title WIP: ECK Resources Helm Chart ECK Resources Helm Chart Jun 20, 2022
@naemono naemono marked this pull request as ready for review June 20, 2022 13:11
@pebrc pebrc mentioned this pull request Jun 21, 2022
Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice progress on the chart work! I left a few comments.

deploy/eck-elasticsearch/values.yaml Show resolved Hide resolved
deploy/eck-elasticsearch/values.yaml Outdated Show resolved Hide resolved
hack/helm/test.sh Outdated Show resolved Hide resolved
deploy/eck-stack/examples/elasticsearch/hot-warm-cold.yaml Outdated Show resolved Hide resolved
# securityContext:
# privileged: true
# runAsUser: 0
# command: ['sh', '-c', 'sysctl -w vm.max_map_count=262144']
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to duplicate more or less the values.yaml file of the chart. Do we need that? Also should the examples be placed in the corresponding charts?

deploy/eck-stack/examples/kibana/http-configuration.yaml Outdated Show resolved Hide resolved
name: eck-elasticsearch
description: A Helm chart to deploy Elasticsearch managed by the ECK Operator.
type: application
version: 0.1.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also recommend adding the appVersion set to the operator version.

deploy/eck-elasticsearch/templates/elasticsearch.yaml Outdated Show resolved Hide resolved
deploy/eck-elasticsearch/templates/elasticsearch.yaml Outdated Show resolved Hide resolved
deploy/eck-elasticsearch/templates/elasticsearch.yaml Outdated Show resolved Hide resolved
deploy/eck-stack/README.md Show resolved Hide resolved
deploy/eck-stack/templates/NOTES.txt Outdated Show resolved Hide resolved
deploy/eck-stack/values.yaml Outdated Show resolved Hide resolved
hack/helm/test.sh Outdated Show resolved Hide resolved
hack/helm/test.sh Show resolved Hide resolved
@naemono
Copy link
Contributor Author

naemono commented Jun 23, 2022

Issue 1

Ok, here's one of the larger issues that both @pebrc and @jmlrt noticed, and hit upon. I initially intended for the examples to only be in the eck-stack chart, but I see how it makes sense in either both locations, or in the single chart that the example supports. Unfortunately, this brings up an issue about duplication/DRY principles, as the example values that will work in eck-elasticsearch chart, will not just "work" in eck-stack chart, and here's why.

To set version of Elasticsearch in eck-elasticsearch chart:

helm install quickstart elastic/eck-elasticsearch -n elastic-stack --create-namespace --set version=8.1.0

To set version of Elasticsearch in eck-stack chart:

helm install quickstart elastic/eck-stack -n elastic-stack --create-namespace --set eck-elasticsearch.version=8.1.0

Note that all sub-charts in the eck-stack chart have to have their values prefixed with the exact chart name.

So these values would work for eck-elasticsearch chart, if passed via --values=my-elasticsearch-values.yaml

# my-elasticsearch-values.yaml
version: 8.1.0

But would be ignored for the eck-stack chart if passed via --values=my-elasticsearch-values.yaml, as there's no eck-elasticsearch prefix.

So if we want examples that work for both eck-stack Chart, and eck-* Charts, most of the values will have to be duplicated across directories.

Issue 2

@jmlrt noted a couple times about Chart.appVersion in his comments in this PR, and in a previous PR and noted:

I also recommend adding the appVersion set to the operator version.

I'd like to further discuss that, as I intentionally left it out, as it's usually tied to the version of the software that this chart is installing, and in our case, we're not "exactly" using the parent chart to install software.

  • Do we want to tie these charts to a specific version of the operator? If so, do we want to use that to check that some version >= Chart.appVersion is installed in the cluster prior to installation, and fail early if it's not installed?

@jmlrt
Copy link
Member

jmlrt commented Jun 24, 2022

@jmlrt noted a couple times about Chart.appVersion in his comments in this PR, and in a previous PR and noted:

I also recommend adding the appVersion set to the operator version.

I'd like to further discuss that, as I intentionally left it out, as it's usually tied to the version of the software that this chart is installing, and in our case, we're not "exactly" using the parent chart to install software.

  • Do we want to tie these charts to a specific version of the operator? If so, do we want to use that to check that some version >= Chart.appVersion is installed in the cluster prior to installation, and fail early if it's not installed?

Well, I don't have a strong opinion on that, and not sure if there are best practices for charts interacting with operators. I would tend to think that appVersion should be tied to the operator version because the chart will not install the operator, but, it will interact with the operator to install Elastic product. If you have to add breaking changes to the custom resources specs someday this could help to figure what operator version is compatible with a specific chart version without having to add hard constraints or maintain a compatibility matrix.

@naemono
Copy link
Contributor Author

naemono commented Jul 20, 2022

not redesigning now for a multi-instance version of the stack chart that is based on lists of Elasticsearches, Agents etc.

Thanks for the explanation. From my findings, I think the issue only really comes to light once you want to install a functional Fleet Agent setup, which absolutely requires 2x Agents in, logically, the same Chart, but I think that's an issue we can resolve when we build out the eck-elastic-agent chart (after this is merged). I would vote to leave the current Kibana/ES charts as is, and not expand them to support multiple resources, as the use case (stack-monitoring) more logically fits installing the eck-stack chart twice, once for the cluster to be monitored, and once again for the cluster to hold monitoring data.

ie

helm install prod-stack elastic/eck-stack -n elastic-stack --create-namespace
helm install monitoring-stack elastic/eck-stack -n elastic-stack

deploy/eck-stack/README.md Show resolved Hide resolved
deploy/eck-elasticsearch/values.yaml Outdated Show resolved Hide resolved
deploy/eck-elasticsearch/values.yaml Outdated Show resolved Hide resolved
deploy/eck-elasticsearch/values.yaml Outdated Show resolved Hide resolved
deploy/eck-elasticsearch/values.yaml Show resolved Hide resolved
deploy/eck-stack/templates/NOTES.txt Show resolved Hide resolved
deploy/eck-stack/examples/custom-elasticsearch-kibana.yaml Outdated Show resolved Hide resolved
deploy/eck-stack/examples/custom-elasticsearch-kibana.yaml Outdated Show resolved Hide resolved
naemono added 3 commits July 22, 2022 08:23
Adjust default values for eck-elasticsearch and eck-kibana.
Debug helm unittest script in CI
@naemono
Copy link
Contributor Author

naemono commented Jul 25, 2022

I'm working through why helm testing isn't wanting to work in CI...
Helm Unit tests in CI fixed

Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

./get_helm.sh -v v${HELM_VERSION} --no-sudo && \
rm get_helm.sh

RUN helm plugin install https://github.com/quintush/helm-unittest --version 0.2.8
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These new requirements should be mentioned in the dev-setup.md file maybe in a new section about developing the Helm charts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Added. Will get this merged after latest ci tasks finished.

@naemono naemono merged commit 74a941b into elastic:main Jul 26, 2022
@naemono naemono deleted the 5505-eck-resource-helm-charts branch July 26, 2022 15:26
@david-kow david-kow changed the title ECK Resources Helm Chart Introduce ECK-managed resources Helm Charts Aug 3, 2022
@david-kow david-kow added the release-highlight Candidate for the ECK release highlight summary label Aug 3, 2022
fantapsody pushed a commit to fantapsody/cloud-on-k8s that referenced this pull request Feb 7, 2023
Support an annotation on all Elastic resources to request a certain license level in the operator. Pre-req of elastic#5781

I had to move a few things around for this to break a few import cycles, I hope that the changes and new packages names are self-explanatory and generally an improvement over the status quo.
fantapsody pushed a commit to fantapsody/cloud-on-k8s that referenced this pull request Feb 7, 2023
* Adding initial revision of eck-stack, and eck-kibana charts.

* Update Chart.yaml to have kubeversion.
Add README.
Add notes for post-installtion.
Update values with more docs

* Update values file with more information.

* adding es resource

* Making things consistent across charts.

* Adding newlines
Adding top yaml markers

* Add public documentation for eck-stack helm chart.
Update example values to work properly.

* Adding helm test script.
Adding minimal tests for eck-stack helm chart.

* add newline

* Add makefile entry.
remove unused bits from script.
Fix naming in helm chart.

* Add helm test to ci checks

* make chart.yaml consistent

* Adding newlines
Updating wording in documentation.

* Update newline

* Update readme for eck-stack

* remove todo from eck-stack readme

* quote things to passify spellcheck

* Quote more vars.
Use $() instead of ``

* Fix sc2045

* Updates from first round of reviews.

* Update the documentation.
Run the dependencies prior to linting.

* Fix shellcheck

* Add note that ECK stack helm charts are enterprise

* Update license note in docs/orchestrating-elastic-stack-applications/stack-helm-chart.asciidoc

Co-authored-by: Peter Brachwitz <[email protected]>

* Update verbiage deploy/eck-stack/README.md

Co-authored-by: Peter Brachwitz <[email protected]>

* Update verbiage deploy/eck-stack/README.md

Co-authored-by: Peter Brachwitz <[email protected]>

* Add 'the' to documentation in  docs/orchestrating-elastic-stack-applications/stack-helm-chart.asciidoc

Co-authored-by: Peter Brachwitz <[email protected]>

* add 'the' again to docs in  docs/orchestrating-elastic-stack-applications/stack-helm-chart.asciidoc

Co-authored-by: Peter Brachwitz <[email protected]>

* Comment out affinity section, as it won't work by default.
Adjust kibana elasticsearcRef to be default for release name 'quickstart'.
Adjust wording for reference.

* Update public documentation to be more clear.

* Add dedicated master nodes to dedicated examples

* Fix elasticsearch test in eck-elasticsearch chart
comment out namespace in elasticsearchref in kibana example.
adjust kibana tests to tests for specifying namespace for elasticsearchref.
s/fullNameOverride/fullnameOverride
remove eck-stack examples with only ES or Kibana.
Add eck-stack example with both ES and Kibana customizations.
Adjust eck-stack tests for new example.

* Fix missing default labels in Elasticsearch.
Fix random '{}' in annotatinos in ES/Kibana.

* Make default quickstart elasticsearchRef work by default.

* Removing logic that inserts namespace for elasticsearchRef, as operator interally handles it.

* Move to referencing https://helm.elastic.co helm repository.

* Fix helm tests

* Add helm test to pr ci pipeline

* Add helm to ci-tools container.

* Handling Review comments

* Adjust default readme in deploy directory
Adjust default values for eck-elasticsearch and eck-kibana.
Debug helm unittest script in CI

* Debugging helm unittest

* Fix shellcheck complaining about debugging messages.

* Try and run helm unittest directly

* Install the correct plugin, with static version

* Adjust default values for eck-elasticsearch, and eck-kibana for the eck-stack chart.

* Fix test for default elasticsearch name

* Remove debugging from helm test script.

* Adjust chart.yaml back to using helm.elastic.co repository.

* Use sed to force local changes during helm testing, and not charts in helm.elastic.co.

* Making shellcheck happy

* Add helm documentation to dev-setup.md.

Co-authored-by: framsouza <[email protected]>
Co-authored-by: Peter Brachwitz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>feature Adds or discusses adding a feature to the product release-highlight Candidate for the ECK release highlight summary v2.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate Helm charts for individual resources managed by the ECK operator
6 participants