feat(chart): VPA Updater PodDisruptionBudget

[phuhung273]

What type of PR is this?

/kind feature

What this PR does / why we need it:

Helm chart support PodDisruptionBudget for VPA Updater

Which issue(s) this PR fixes:

Relates #8587

Special notes for your reviewer:

Quick test using

helm upgrade --install vpa ./vertical-pod-autoscaler/charts/vertical-pod-autoscaler/ \
  -n vpa --create-namespace

Does this PR introduce a user-facing change?

NONE

[k8s-ci-robot]

Hi @phuhung273. Thanks for your PR.

I'm waiting for a github.com 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.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: phuhung273
Once this PR has been reviewed and has the lgtm label, please assign adrianmoisey 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:

• vertical-pod-autoscaler/charts/vertical-pod-autoscaler/OWNERS

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

[jackfrancis]

From the Kubernetes documentation:

You can specify only one of maxUnavailable and minAvailable in a single PodDisruptionBudget. maxUnavailable can only be used to control the eviction of pods that all have the same associated controller managing them. In the examples below, "desired replicas" is the scale of the controller managing the pods being selected by the PodDisruptionBudget.

See how we handle this in the Cluster Autoscaler chart:

https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/charts/cluster-autoscaler/templates/pdb.yaml

https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/charts/cluster-autoscaler/values.yaml#L334-L337

Essentially, we want to ensure we only default to one or the other (you've already done the equivalent of that by defaulting maxUnavailable to empty. But also IMO the checks in the template foo as well are useful:

• Fail processing if both values are set
• Only inject a min or max value if it is the only one set

[phuhung273]

Thanks for taking a look Jack. I can see that validation in

autoscaler/vertical-pod-autoscaler/charts/vertical-pod-autoscaler/templates/admission-controller-pdb.yaml

Lines 1 to 4 in f7fc231

	{{- if and .Values.admissionController.enabled .Values.admissionController.podDisruptionBudget.enabled -}}
	{{- if and .Values.admissionController.podDisruptionBudget.minAvailable .Values.admissionController.podDisruptionBudget.maxUnavailable }}
	{{- fail "Only one of admissionController.podDisruptionBudget.minAvailable or admissionController.podDisruptionBudget.maxUnavailable should be set." }}
	{{- end }}

But I decided not to do the same since k8s already has builtin validation

Error: 1 error occurred:
        * PodDisruptionBudget.policy "vpa-vertical-pod-autoscaler-updater" is invalid: spec: Invalid value: {"MinAvailable":1,"Selector":{"matchLabels":{"app.kubernetes.io/component":"updater","app.kubernetes.io/instance":"vpa","app.kubernetes.io/name":"vertical-pod-autoscaler"}},"MaxUnavailable":1,"UnhealthyPodEvictionPolicy":null}: minAvailable and maxUnavailable cannot be both set

Can you have a try and let me know if we need our own validation.

[adrianmoisey]

If there are more than 1 replicas running for the updater, it needs to have a lease, similar to

autoscaler/vertical-pod-autoscaler/charts/vertical-pod-autoscaler/templates/recommender-deployment.yaml

Lines 59 to 64 in 6177945

	- --leader-elect=true
	- --leader-elect-resource-namespace={{ .Values.recommender.leaderElection.resourceNamespace | default .Release.Namespace }}
	- --leader-elect-resource-name={{ .Values.recommender.leaderElection.resourceName }}
	- --leader-elect-lease-duration={{ .Values.recommender.leaderElection.leaseDuration }}
	- --leader-elect-renew-deadline={{ .Values.recommender.leaderElection.renewDeadline }}
	- --leader-elect-retry-period={{ .Values.recommender.leaderElection.retryPeriod }}

[phuhung273]

Thanks Adrian for taking a look. While adding leaderLection i found another issue: update-role and updater-rolebinding don't make sense. So decided to create a separate PR #8777 to discuss. Can you have a look at it first ?

[omerap12]

/ok-to-test