-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
Change default lease name for vpa-recommender to "vpa-recommender-lease" #7498
Conversation
/assign @adrianmoisey |
/lgtm |
Oh, autoscaler/vertical-pod-autoscaler/deploy/vpa-rbac.yaml Lines 412 to 413 in 59aefbc
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To remove the lgtm label
/lgtm cancel |
/assign @raywainman |
@raywainman just a ping to remind you about this |
Thanks team. I've fixed autoscaler/vertical-pod-autoscaler/deploy/vpa-rbac.yaml. I will work on the Helm PR separately from this one as well, it is formally setting the flag name in the Helm Chart so is actually a bit of a separate problem from this PR. |
/lgtm
Which Helm PR are you referring to? |
I guess it is this one: cowboysysop/charts#781 The main point is: we can just merge this change, adjusting the default value for the lease and there is no dependency for the helm chart PR, as it always sets the lease name explicitly. /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: raywainman, voelzmo 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 |
The Helm chart doesn't set it explicitly, they're all commented out. However, we should try get that PR merged before we cut a release of the VPA |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Changes the default lease name for the vpa-recommender to
vpa-recommender-lease
as this can sometimes conflict with managed deployments of HPA and VPA. See #7461.Though changing the default here is possibly disruptive during upgrade, we believe it is the right path forward to avoid future outages. See the discussion in #7461 where we also discuss the possible scenarios during upgrade where there could temporarily be two leaders.
Which issue(s) this PR fixes:
#7461
Does this PR introduce a user-facing change?