feat(helm): optionally set pod disruption budgets #27163
Conversation
pradasouvanlasy
requested review from
craig-rueda,
dpgaspar and
villebro
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| {{- define "supersetCeleryBeat.selectorLabels" -}} | ||
| app: {{ include "superset.name" . }}-celerybeat | ||
| release: {{ .Release.Name }} | ||
| {{- end }} | ||
|
|
||
| {{- define "supersetCeleryFlower.selectorLabels" -}} | ||
| app: {{ include "superset.name" . }}-flower | ||
| release: {{ .Release.Name }} | ||
| {{- end }} | ||
|
|
||
| {{- define "supersetNode.selectorLabels" -}} | ||
| app: {{ include "superset.name" . }} | ||
| release: {{ .Release.Name }} | ||
| {{- end }} | ||
|
|
||
| {{- define "supersetWebsockets.selectorLabels" -}} | ||
| app: {{ include "superset.name" . }}-ws | ||
| release: {{ .Release.Name }} | ||
| {{- end }} | ||
|
|
||
| {{- define "supersetWorker.selectorLabels" -}} | ||
| app: {{ include "superset.name" . }}-worker | ||
| release: {{ .Release.Name }} | ||
| {{- end }} |
There was a problem hiding this comment.
Maybe also mention this refactor in the description
There was a problem hiding this comment.
Sure, I've updated the description. Please let me know if this looks clear enough
| @@ -248,6 +248,8 @@ supersetNode: | |||
| maxReplicas: 100 | |||
| targetCPUUtilizationPercentage: 80 | |||
| # targetMemoryUtilizationPercentage: 80 | |||
| # -- Sets the [pod disruption budget](https://kubernetes.io/docs/tasks/run-application/configure-pdb/) for supersetNode pods | |||
There was a problem hiding this comment.
Can you add an enabled field here, along with a set of sensible defaults for each of these blocks? That way, we can add the various settings to the schema and can document them appropriately. Using an enabled also makes the config a bit more explicit
e.g.
podDisruptionBudget:
enabled: false
minAvailable: 1
maxUnavailable: 1There was a problem hiding this comment.
Thanks, I will take this into account. However both variables are mutually exclusive so we would have to comment these in values.yaml and document somehow that PDB only accepts either minAvailable or maxUnavailable at a time - https://kubernetes.io/docs/tasks/run-application/configure-pdb/#specifying-a-poddisruptionbudget.
There was a problem hiding this comment.
Yep, that makes sense. For extra credit, you could fail if both are set:
{{- if .Values.podDisruptionBudget.minAvailable and .Values.podDisruptionBudget.maxAvailable }}
{{ required "Only one of minAvailable or maxAvailable should be set" .Values.podDisruptionBudget.minAvailable }}
{{- end}}
There was a problem hiding this comment.
I've made the changes and slightly refactored the PDB templates I'm contributing there to make better use of variable scoping - reducing a bit the boilerplate and avoiding to repeat .Values.supersetXXX.podDisruptionBudget everywhere.
Please tell me if the updated documentation looks clear enough as well. Thanks!
mistercrunch
added
🏷️ bot
SUMMARY
This PRs optionally adds pod disruption budgets against all Superset deployments. By default, they are not enabled to keep the current behavior but we're now able to set PDBs on any deployment if relevant.
This also comes with a slight refactoring on the selector labels for each deployment: they are now defined as named templates in
_helpers.tplto mitigate selectors duplication between deployments and their respective related pod disruption budgets.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
One can set any
supersetXxx.podDisruptionBudget.enabledastrueand add eitherminAvailableormaxUnavailablevariables and test whether the generated manifests look as expected.PDB manifests are not generated when the relevant
podDisruptionBudget.enabledisfalseand generation fails if any of the enabled PDB defines bothminAvailableandmaxUnavailableat the same time.ADDITIONAL INFORMATION