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

Refactor opensearch-cluster helm chart #754

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

evheniyt
Copy link
Contributor

@evheniyt evheniyt commented Mar 7, 2024

Description

We found that the current opensearch-cluster helm chart supports only OpenSearchCluster configuration, also configuration itself is very limited because not all possible options were defined in the template.
For our needs, we have decided to improve this helm chart and think that our changes could be useful for the community.

The main differences in the new helm chart:

  • Supports all existing CRDs
  • Includes auto-generated README.md file with description for all possible configuration values
  • Includes Ingress configuration for Opensearch and Dashboards
  • Values.yaml has examples of operator-specific configurations, making it easier for users to understand what to set
  • Easy to maintain and extend

We were trying to build this chart by following the next logic. We didn't try to template all possible configurations for CRDs, instead, we relied on official parameters supported by specific sections of the CRD, e.g.

Instead of doing this:

  {{- if .Values.opensearchCluster.bootstrap }}
  bootstrap:
    {{- if .Values.opensearchCluster.bootstrap.additionalConfig }}
    additionalConfig:
      {{ toYaml .Values.opensearchCluster.bootstrap.additionalConfig | nindent 6 }}
    {{- end }}
  {{- end }}
  {{- if .Values.opensearchCluster.initHelper }}

We did:

  {{- with .Values.cluster.bootstrap }}
  bootstrap: {{ . | toYaml | nindent 4 }}
  {{- end }}

And defined all possible configuration options inside the .Values.cluster.bootstrap.

Pros of such logic:

  • it helps simplify the maintenance of the chart, when a new config option is added you just need to add its default value to values.yaml file. And even if for some reason it wasn't added, users still could use it;
  • all existing and new configuration options will be available for users;
  • configuration options have the same format and naming as it is defined in the operator doc, so users will be not confused by different naming in helm and off doc.
  • it allows to automatically check all defined options, so when users make a typo or put invalid configuration they will receive an error during deployment, helping them to understand what they did wrong.

If you apply it, I could help with maintaining this helm chart by fixing bugs, adding new features, etc.

Issues Resolved

Closes #699
Closes #9
Closes #855
Closes #866
Closes #902
Closes #667
Closes #904

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Yevhenii Tiutiunnyk <[email protected]>
@evheniyt
Copy link
Contributor Author

evheniyt commented Mar 7, 2024

FYI as this PR was prepared a few months ago it didn't include some templates for the CRDs. If you will give it a green light in terms of concept and implementation I will add all missing templates.

@prudhvigodithi
Copy link
Member

Hey @evheniyt thanks for your contribution, I like the idea to control the yaml files using the helm that can be used to create the cluster via the operator, can you include all the CRD's and fix the conflicts, we can go ahead and merge it.

Adding @bbarani @salyh @jochenkressin @pchmielnik @bbarani

@evheniyt
Copy link
Contributor Author

Sure, I will add all the missing CRDs soon. Also, will need to add upgrade instructions.
@prudhvigodithi what about chart version? currently, it is equal to operator version. Do you think we should continue with this approach or we could have a different version for chart?
With different version for chart it will be simpler to set it 3.0.0 meaning that there are breaking changes.

@prudhvigodithi
Copy link
Member

Thanks @evheniyt with existing setup and release process, the helm charts are updated to the appVersion (operator version) https://github.com/opensearch-project/opensearch-k8s-operator/blob/main/.github/workflows/release.yaml#L26-L31.

We should seperate out this sooner to ensure the helm charts development and release is fast paced. WDYT @salyh @jochenkressin @pchmielnik @swoehrl-mw ?

@swoehrl-mw
Copy link
Collaborator

@prudhvigodithi

We should seperate out this sooner to ensure the helm charts development and release is fast paced.

There is something to be said for keeping the versioning of cluster chart and operator the same to make it clear which versions match and are compatible. But on the other hand I can understand the cluster chart might need a faster/different release cycle than the operator.

@salyh
Copy link
Collaborator

salyh commented Mar 28, 2024

We should seperate out this sooner to ensure the helm charts development and release is fast paced. WDYT @salyh @jochenkressin @pchmielnik @swoehrl-mw ?

I agree

# Conflicts:
#	charts/opensearch-cluster/Chart.yaml
#	charts/opensearch-cluster/templates/opensearch-cluster-cr.yaml
@evheniyt
Copy link
Contributor Author

Added OpenSearchISMPolicy template

Signed-off-by: Yevhenii Tiutiunnyk <[email protected]>
Signed-off-by: Yevhenii Tiutiunnyk <[email protected]>
@evheniyt
Copy link
Contributor Author

I have updated Readme.

Also, I have tested helm chart upgrade process from v2 to v3 with default config from values.yaml. In general, because the new version of the helm chart has the same labels, annotations and follows the same default logic for cluster name, the upgrade from v2 to v3 (with default values) didn't trigger any resource redeployments.

Now it's ready for review @prudhvigodithi @swoehrl-mw @salyh

@waqarsky
Copy link

When is this going to be merged / released?

# Conflicts:
#	charts/opensearch-cluster/Chart.yaml
#	charts/opensearch-cluster/templates/opensearch-cluster-cr.yaml
#	charts/opensearch-cluster/values.yaml
Signed-off-by: Yevhenii Tiutiunnyk <[email protected]>
Signed-off-by: Yevhenii Tiutiunnyk <[email protected]>
# Conflicts:
#	charts/opensearch-cluster/templates/opensearch-cluster-cr.yaml
@evheniyt
Copy link
Contributor Author

@prudhvigodithi @swoehrl-mw I have resolved all conflicts.
When we could proceed with this MR?

@prudhvigodithi
Copy link
Member

Hey @evheniyt I will take a look at this PR today, @swoehrl-mw please add your thoughts as well. Once finalized along with this PR the version https://github.com/opensearch-project/opensearch-k8s-operator/blob/main/charts/opensearch-cluster/Chart.yaml#L7 can be incremented for a chart release (coming from #830).
Thank you
@getsaurabh02

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
5 participants