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

Generalized support for overwriting/extending initContainers, volumes, volumeMounts #32

Merged
merged 13 commits into from
Feb 27, 2024

Conversation

BWibo
Copy link
Member

@BWibo BWibo commented Feb 20, 2024

@gislab-augsburg, @klml, @eidottermihi, @MandanaMoshref:
Sorry this took me so long, but finally here is a first draft. It basically pics up what I suggest here. It should allow using OpenShift just by adapting the configuration when depolying the chart. Check out the examples below to see how it works.

This shall replace the work proposed in #31 with a more general and flexible mechanism to allow user of this chart to:

  • Overwrite, or extend the default initContainer, volume, volumeMount of this chart
  • Overwrite includes to disable the default initContainer, volume, volumeMount specified in this chart

What is different compared to the work in #31?

  • The mechanism implemented here allows to freely configure initContainer, volume, volumeMount. This is at deploy time with helm -f values.yml or using helm --set. Thus, if any specific requirements arise in the future (e.g. have an additional file writable for OpenShift using a volume), this mean just a configuration change and not a new release of the chart.
    In Sddi urban 2.9.9: Changes for OpenShift deploy, further configurability and reduced dependencies #31, e.g. only specific files are replaced by volumes. The solution is specific for the issue of supporting OpenShift.
  • The given defaults can be overwritten, disabled or extended.
  • It is possible to use templates in the definitions. They need to be quoted!!

ToDos

Requirements

  • This might require Helm >= v3.14.0 what I used for testing.

Examples

Here are some example to illustrate usage of this. In all examples we create a custom values.yml files with our configuration for the deployment. The we run: helm install sddi sddi-ckan/sddi-ckan -f values.yml

Disable default initContainers

This will disable the default init containers.

values.yml:

ckan:
  initContainers: []

Replace default initContainers

This will overwrite the default init containers with the one specified below.

values.yml:

ckan:
  initContainers:
    - name: custom-init-data
      image: my/customimage:v1.2.3
      command: ["sh", "-c", "chown -Rv 999:999 /custom/path"]

Extend the default initContainers

This will extend the default init containers of the chart. Note: The template specified below will be rendered!

values.yml:

ckan:
  extraInitContainers:
    - name: additional-init-data
      image: my/customimage:v1.2.3
      command: ["sh", "-c", "chown -Rv 0:0 {{ .Values.persistence.storagePath }}"]

@BWibo BWibo added type: feature Brand new functionality, features, pages, workflows, endpoints, etc. work: complicated The situation is complicated, good practices used. labels Feb 20, 2024
@BWibo BWibo self-assigned this Feb 20, 2024
@BWibo BWibo marked this pull request as ready for review February 23, 2024 23:41
@BWibo
Copy link
Member Author

BWibo commented Feb 23, 2024

@gislab-augsburg, @klml, @eidottermihi, @MandanaMoshref:
I would appreciate if you could test if this works on OpenShift. If your struggling to get the configuration right, let me know, I can help. We can setup a meeting too, if you need some further explanantion or assistance.

@klml
Copy link
Contributor

klml commented Feb 24, 2024

@BWibo tnak you very much for your PR, we will test your solution the next week.

@gislab-augsburg
Copy link

gislab-augsburg commented Feb 26, 2024

@BWibo:
I tested your solution in LHM Openshift and it works like a charm :)
You are absolutely right, it is better than the changes proposed in #31, more flexible and general. With the ability to freely configure initContainers, volumes and volumeMounts, this feature solves all our issues and we are free to include further configMaps etc. if we will need some in the future. Thank you so much 🙏

I only had a minor issue on the first try with volume sizeLimit:

configuration in our values-lhm-tum.yaml:

  # Extra Volumes
  extraVolumes:
    # Volumes for paths that need to be writable
    - name: srv-app-i18n
      emptyDir:
      sizeLimit: 500Mi
    - name: srv-app-data
      emptyDir:
      sizeLimit: 500Mi

results in :

mb@nbo00370518:~/ckan-helm$ helm upgrade --install -f values-lhm-tum.yaml -f values-dev-secret-tum.yaml ckan ../tum-sddi/sddi-ckan-k8s/charts/sddi-ckan/
W0226 12:15:54.671209    1389 warnings.go:70] unknown field "spec.template.spec.volumes[1].sizeLimit"
W0226 12:15:54.671585    1389 warnings.go:70] unknown field "spec.template.spec.volumes[2].sizeLimit"
Release "ckan" has been upgraded. Happy Helming!
NAME: ckan
LAST DEPLOYED: Mon Feb 26 12:15:51 2024
NAMESPACE: udpkatalog-dev
STATUS: deployed
REVISION: 132
TEST SUITE: None

But I'm not sure if sizeLimit is really necessary, @klml what do you think, do we need it?

@BWibo
Copy link
Member Author

BWibo commented Feb 26, 2024

@gislab-augsburg Thx for testing and really cool this works for you guys. I think there is a syntax error regarding the size level. There is one level of indention missing. This should do the trick:

  # Extra Volumes
  extraVolumes:
    # Volumes for paths that need to be writable
    - name: srv-app-i18n
      emptyDir:
        sizeLimit: 500Mi
    - name: srv-app-data
      emptyDir:
        sizeLimit: 500Mi

@gislab-augsburg Can you please confirm if this works?

As soon as this is done and we have a solution for #33, I'lll prepare a release.

@BWibo BWibo mentioned this pull request Feb 26, 2024
@BWibo BWibo changed the base branch from main to release/3.0.0 February 26, 2024 21:37
@BWibo BWibo mentioned this pull request Feb 26, 2024
2 tasks
@gislab-augsburg
Copy link

gislab-augsburg commented Feb 27, 2024

@gislab-augsburg Thx for testing and really cool this works for you guys. I think there is a syntax error regarding the size level. There is one level of indention missing. This should do the trick:

  # Extra Volumes
  extraVolumes:
    # Volumes for paths that need to be writable
    - name: srv-app-i18n
      emptyDir:
        sizeLimit: 500Mi
    - name: srv-app-data
      emptyDir:
        sizeLimit: 500Mi

@gislab-augsburg Can you please confirm if this works?

As soon as this is done and we have a solution for #33, I'lll prepare a release.

@BWibo Yes, it works :) You were right, it was just an indentation mistake. From our side, you can merge and prepare the release.

#33 is solved, did I get it right? From our side, there are no problems with solr when:

  • using the branch feature/customVol-Mnt-Init
  • overwriting ckan initContainers and so removing init-data in our values-lhm.yaml
  • having the default podSecurityContext: {} in sddi-ckan-k8s/charts/sddi-ckan/charts/solr/values.yaml without changing this in our values-lhm.yaml
  • not creating an initContainer for solr

@BWibo
Copy link
Member Author

BWibo commented Feb 27, 2024

OK, cool. I'll create a beta release first for testing.
Yes #33 is resolved, but it should be tested. Let's do this in the beta release (#34) where all new features are included.

@BWibo BWibo merged commit 3c80f59 into release/3.0.0 Feb 27, 2024
@BWibo BWibo deleted the feature/customVol-Mnt-Init branch March 2, 2024 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: soon type: feature Brand new functionality, features, pages, workflows, endpoints, etc. work: complicated The situation is complicated, good practices used.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants