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.

Sign up for GitHub

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

fix(helm): Allow pdb minAvailable to take precedence over default max… #7139

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix(helm): Allow pdb minAvailable to take precedence over default max… #7139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
640ee83
fix(helm): Allow pdb minAvailable to take precedence over default max…
ParichayDidwania Aug 7, 2024
2481e73
Merge branch 'master' into pdb-minavailable-precedence
ParichayDidwania Oct 6, 2024
1bd0e8d
Merge branch 'master' into pdb-minavailable-precedence
ParichayDidwania Feb 10, 2025
e02f665
Update Chart.yaml
ParichayDidwania Feb 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion charts/cluster-autoscaler/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,4 @@ name: cluster-autoscaler
sources:
- https://github.com/kubernetes/autoscaler/tree/master/cluster-autoscaler
type: application
version: 9.46.0
version: 9.46.1
9 changes: 6 additions & 3 deletions charts/cluster-autoscaler/templates/pdb.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ spec:
selector:
matchLabels:
{{ include "cluster-autoscaler.instance-name" . | indent 6 }}
{{- if .Values.podDisruptionBudget }}
{{ toYaml .Values.podDisruptionBudget | nindent 2 }}
{{- end }}
{{- if .Values.podDisruptionBudget.minAvailable }}
minAvailable: {{ .Values.podDisruptionBudget.minAvailable }}
{{- end }}
{{- if and .Values.podDisruptionBudget.maxUnavailable (not .Values.podDisruptionBudget.minAvailable) }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having an {{- else}} block after {{- if .Values.podDisruptionBudget.minAvailable }} here is simpler and equivalent to a separate {{- if and .Values.podDisruptionBudget.maxUnavailable (not .Values.podDisruptionBudget.minAvailable) }} block.

The way the current values.yaml data structure is implemented is sort of incorrect as a way to represent the podDisruptionBudget interface. See:

Especially:

You can specify only one of maxUnavailable and minAvailable in a single PodDisruptionBudget.

I think this PR points towards an improvement in the existing solution, but FYI we're in a non-ideal place here, as we're essentially putting it on the users to understand that they're only allowed to use one or the other of the child properties of the podDisruptionBudget values object, and that if they do specify both then minAvailable will be preferred, and maxUnavailable will be ignored. (There isn't a great way to do runtime template error validation in helm AFAIK .)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @gjtempleton on that last part above

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR points towards an improvement in the existing solution, but FYI we're in a non-ideal place here, as we're essentially putting it on the users to understand that they're only allowed to use one or the other of the child properties of the podDisruptionBudget values object, and that if they do specify both then minAvailable will be preferred, and maxUnavailable will be ignored. (There isn't a great way to do runtime template error validation in helm AFAIK .)

I have generally seen in other helm charts that they usually have the podDisruptionBudget block as empty in values.yaml. By default they set it off, and then allow the users to enter what they want.

Usually, when users enter both maxUnavailable and minAvailable together in such case, it will add both within the template instead of prioritizing one over above. Applying this generated template will then throw the error by kubernetes itself suggesting that only one of the 2 should be available. I think this is the correct way to go about it.

The reason I made it prioritize one of the two (maxUnavailable and minAvailable) and did not touch values.yaml was to ensure that it still works the same way for old users. But if that is not the case, then I can change the PR to make it how it should ideally be.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that all makes sense, and 💯 on making this back-compat for users.

Now that I think about it, maybe this is the most correct template solution:

  {{- if .Values.podDisruptionBudget.minAvailable and (not .Values.podDisruptionBudget.maxUnavailable) }}
  minAvailable: {{ .Values.podDisruptionBudget.minAvailable }}
  {{- end }}
  {{- if and .Values.podDisruptionBudget.maxUnavailable (not .Values.podDisruptionBudget.minAvailable) }}
  maxUnavailable: {{ .Values.podDisruptionBudget.maxUnavailable }}
  {{- end }}

If we do the above, if users declare both configuration flavors, it will be ignored by the template. I suppose this is not strictly back-compat, but the current template will not work in such a scenario anyways (I assume it will be rejected by apiserver webhook for the PodDisruptionBudget resource type).

wdyt?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the current template is rejected by the webhook if we declare both flavors. It is also rejected if i just add minAvailable in the override yaml file, because maxUnavailable is included by default in values.yaml.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking about this I think this is the best, back-compat way forward:

  {{- if .Values.podDisruptionBudget.minAvailable and (not .Values.podDisruptionBudget.maxUnavailable) }}
  minAvailable: {{ .Values.podDisruptionBudget.minAvailable }}
  {{- end }}
  {{- if and .Values.podDisruptionBudget.maxUnavailable (not .Values.podDisruptionBudget.minAvailable) }}
  maxUnavailable: {{ .Values.podDisruptionBudget.maxUnavailable }}
  {{- end }}
  {{- if and (not .Values.podDisruptionBudget.maxUnavailable) (not .Values.podDisruptionBudget.minAvailable) }}
  maxUnavailable: 1
  {{- end }}

And then would need to get rid of the default in values.yaml. I don't think we can statically assign defaults to one or the other based on the mutually exclusive API requirements here; moving the defaults into the template in the above way should solve for that.

Thanks for your patience!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jackfrancis Having defaults in the template is not a good practice in my opinion. This is because, users will always go through the values.yaml and override things from there. Having default should be the job of values.yaml. If someone just turns the pdb on from override.values.yaml, they would automatically end up with maxUnavailable as 1. Helm should only be used for templatizing fields, not providing defaults.

According to me, we should keep the maxUnavailable in values.yaml for backward compatibility, and just provide a user with a choice to add maxUnavailable or minAvailable from the override.values.yaml. Even if they don't provide anything of the flavours, they would know that maxUnavailable: 1 is applied because that is the default value in values.yaml

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here is that the values.yaml data interface doesn't support either/or property existence requirements.

just provide a user with a choice to add maxUnavailable or minAvailable from the override.values.yaml

How do we do the above exactly?

Copy link
Author

@ParichayDidwania ParichayDidwania Oct 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jackfrancis If we have this

{{- if .Values.podDisruptionBudget.minAvailable }}
minAvailable: {{ .Values.podDisruptionBudget.minAvailable }}
{{- end }}
{{- if and .Values.podDisruptionBudget.maxUnavailable (not .Values.podDisruptionBudget.minAvailable) }}
maxUnavailable: {{ .Values.podDisruptionBudget.maxUnavailable }}
{{- end }}

along with maxUnavailable: 1 in values.yaml. The users will know that they can use override.values.yaml to choose the flavour they want.

They can simply set in the override, minAvailable: 1 to override the default maxUnavailable.

In the other case, they can either let it be default and explicitly specify maxUnavailable: 1 in order to get this flavor.

This way they have a choice.

Now, if they add both, the ideal scenario is to add both from our end and let the kubernetes API throw an error, but since we need to make it backward compatible, we can't do that. This is why, we prioritize one over the other. And since, maxUnavailable is present by default, we have to prioritize minAvailable.

The solution you provided will yield the same result as mine, except the maxUnavailable default value will be visible to users in values.yaml, where the users would expect it to be, and not inside the template itself.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jackfrancis Any update on this?

maxUnavailable: {{ .Values.podDisruptionBudget.maxUnavailable }}
{{- end }}
{{- end -}}
Loading