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

[ParichayDidwania]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Allow minAvailable to take precedence over default maxUnavailable in pdb of cluster-autoscaler helm chart

Which issue(s) this PR fixes:

Fixes #7128

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: ParichayDidwania / name: Parichay (640ee83, e02f665, 2481e73, 1bd0e8d)

[k8s-ci-robot]

Welcome @ParichayDidwania!

It looks like this is your first PR to kubernetes/autoscaler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/autoscaler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @ParichayDidwania. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[jackfrancis]

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:

• https://kubernetes.io/docs/tasks/run-application/configure-pdb/#specifying-a-poddisruptionbudget

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 .)

[jackfrancis]

cc @gjtempleton on that last part above

[ParichayDidwania]

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.

[jackfrancis]

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?

[ParichayDidwania]

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.

[jackfrancis]

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!

[ParichayDidwania]

@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?

[jackfrancis]

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?

[ParichayDidwania]

@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.

[ParichayDidwania]

@jackfrancis Any update on this?

[jackfrancis]

/assign

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[ParichayDidwania]

/remove-lifecycle stale

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ParichayDidwania
Once this PR has been reviewed and has the lgtm label, please assign gjtempleton for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• charts/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment