-
Notifications
You must be signed in to change notification settings - Fork 385
Add topologySpreadConstraints
support for server pods
#863
Add topologySpreadConstraints
support for server pods
#863
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.
Thanks so much for the contribution @lawliet89 !! This looks great. Just added a comment about ensuring we template the value for only valid kube versions.
@@ -63,6 +63,10 @@ spec: | |||
{{- if .Values.server.tolerations }} | |||
tolerations: | |||
{{ tpl .Values.server.tolerations . | nindent 8 | trim }} | |||
{{- end }} | |||
{{- if .Values.server.topologySpreadConstraints }} |
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.
Could you add a check here similar to:
{{- if or ( gt .Capabilities.KubeVersion.Major "1" ) ( ge .Capabilities.KubeVersion.Minor "18" ) }}
?
There is a warning in the values file, but this might be helpful in ensuring it doesn't accidentally get templated when say re-using a values file. We have tests that do something similar in ui-ingress.bats
for reference!
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.
I have added the checks.
A side note: do you think it's better to just have this fail instead of being silently ignored when a user tries to use this on a cluster < 1.18 though?
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.
YES! I like this idea a lot. Would you mind updating the PR to fail if the user attempts to set this value on a cluster < 1.18?
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.
I have added that in.
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.
🙏 thank you @lawliet89
Changes proposed in this PR:
topologySpreadConstraints
support for server podsHow I've tested this PR:
server.topologySpreadConstraints
set.How I expect reviewers to test this PR:
server.topologySpreadConstraints
set.Checklist:
Note:
This helps to spread server pods evenly into all AZs.
podAntiAffinity
set to the topology key.