-
Notifications
You must be signed in to change notification settings - Fork 6.3k
Allow more comprehensive affinity config #5285
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems good to me, I did some digging and this does indeed look like a common practice. I also think it's fine to push a breaking change given we don't publish our helm charts anywhere and they are manually pulled.
However, will defer to @Matthew-Beckett to review and approve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR changes helm template to allow more comprehensive configuration in affinity. Instead of treating affinity as a static map, it expects a string passed there, which would be then processed as a template and therefore resolve any links, such as f.e. {{ include "code-server.name" . }} .
This then allows to configure something like this:
Above allows to properly spread pods over nodes without hardcoding any labels in values file.
This approach is somewhat common in helm ecosystem, f.e. see here:
https://github.com/codecentric/helm-charts/blob/31f44ca368cef7585760edc8f22c011cab1be284/charts/keycloakx/templates/statefulset.yaml#L184
The change is breaking though, so I am not 100 percent sure this has to be included, I am sending this as a suggestion and input for discussion.