feat(chart): VPA Updater leaderElection #8777
Conversation
k8s-ci-robot
added
release-note-none
|
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 Once the patch is verified, the new status will be reflected by the 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
added
needs-ok-to-test
k8s-ci-robot
added
the
size/M
omerap12
left a comment
omerap12
left a comment
There was a problem hiding this comment.
Thanks for this, overall looks ok but two comments from my end :)
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: RoleBinding | ||
| metadata: | ||
| name: {{ include "vertical-pod-autoscaler.updater.fullname" . }}-leader-locking | ||
| namespace: {{ .Release.Namespace }} | ||
| roleRef: | ||
| apiGroup: rbac.authorization.k8s.io | ||
| kind: Role | ||
| name: {{ include "vertical-pod-autoscaler.updater.fullname" . }}-leader-locking | ||
| subjects: | ||
| - kind: ServiceAccount | ||
| name: {{ include "vertical-pod-autoscaler.updater.fullname" . }} | ||
| namespace: {{ .Release.Namespace }} | ||
| {{- end -}} |
There was a problem hiding this comment.
Since all other components use a dedicated role-binding file, can we align this one as well? I don’t mind if the role definition is included in the same file as the role binding, but we should keep it consistent with the other components.
There was a problem hiding this comment.
Agree. I was copying from this file https://github.com/kubernetes/autoscaler/blob/master/vertical-pod-autoscaler/charts/vertical-pod-autoscaler/templates/recommender-leader-election-rbac.yaml. But yeah can see most files are following role + rolebinding pattern.
There was a problem hiding this comment.
Yeah we should align all components :)
vertical-pod-autoscaler/charts/vertical-pod-autoscaler/templates/updater-role.yaml
Outdated
Show resolved
Hide resolved
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
phuhung273
force-pushed
the
update-leader-election
branch
2 times, most recently
from
7894677 to
d7c1988
Compare
| - apiGroups: | ||
| - "coordination.k8s.io" | ||
| resourceNames: | ||
| - vpa-updater |
There was a problem hiding this comment.
Removing vpa-updater from resourceNames and still work. Waiting if Adrian knows other edge cases.
There was a problem hiding this comment.
Yup, I think that's safe to remove
There was a problem hiding this comment.
Thank you, it is removed
phuhung273
force-pushed
the
update-leader-election
branch
from
d7c1988 to
71433cc
Compare
Signed-off-by: phuhung273 <[email protected]>
phuhung273
force-pushed
the
update-leader-election
branch
from
71433cc to
6645a73
Compare
omerap12
left a comment
omerap12
left a comment
There was a problem hiding this comment.
Thanks!
/assign @adrianmoisey
k8s-ci-robot
added
the
approved
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adrianmoisey, omerap12, phuhung273 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/retest |
4 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
leaderElectionfor VPA Updaterupdater-roleto only create whenleaderElection.enabled. The role name and its permissions clearly states that it belongs toleaderElectioninstead ofserviceAccount.Which issue(s) this PR fixes:
Relates #8587
Special notes for your reviewer:
Quick test using
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: