Add --in-place-skip-disruption-budget flag#8987
Add --in-place-skip-disruption-budget flag#8987k8s-ci-robot merged 1 commit intokubernetes:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: omerap12 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3f857bf to
73e666f
Compare
| | `eviction-tolerance` | float | 0.5 | Fraction of replica count that can be evicted for update, if more than one pod can be evicted. | | ||
| | `feature-gates` | mapStringBool | | A set of key=value pairs that describe feature gates for alpha/experimental features. Options are:<br>AllAlpha=true\|false (ALPHA - default=false)<br>AllBeta=true\|false (BETA - default=false)<br>InPlaceOrRecreate=true\|false (BETA - default=true)<br>PerVPAConfig=true\|false (ALPHA - default=false) | | ||
| | `ignored-vpa-object-namespaces` | string | | A comma-separated list of namespaces to ignore when searching for VPA objects. Leave empty to avoid ignoring any namespaces. These namespaces will not be cleaned by the garbage collector. | | ||
| | `in-place-skip-disruption-budget` | | | If true, VPA updater skips disruption budget checks for in-place pod updates when all containers have NotRequired resize policy (or no policy defined) for both CPU and memory resources. Disruption budgets are still respected when any container has RestartContainer resize policy for any resource. | |
There was a problem hiding this comment.
Just a question I'm not advocating for this, but would we ever consider this to be defaulted to on? InPlaceOrRecreate is still considered beta after all (I know we are impending 1.6.0), or is that too risky and/or would users not prefer this to be a defaulted option?
There was a problem hiding this comment.
I realize we spoke about this in the other issue and mentioned that this flag would have an alpha and beta phase: #8818 (comment)
If we are not feature-gating this, should we indicate the current state of phases in the flag description?
There was a problem hiding this comment.
Just a question I'm not advocating for this, but would we ever consider this to be defaulted to on?
I kinda like it defaulting to "on"
InPlaceOrRecreateis still considered beta after all (I know we are impending 1.6.0), or is that too risky and/or would users not prefer this to be a defaulted option?
Hmmm, since it's still in beta, may be we can make this change? We could delay the GA of InPlaceOrRecreate by a release.
Another option is to not default it to "on" in the VPA, but default it to "on" in the helm chart.
EDIT: thinking about it a little more, an inlace resize is still not a 100% safe operation. There's that edge case scenario where resizing RAM limits down could cause OOM. Since the VPA has historically put effort into protecting workloads by default, may be this should default to "off"
There was a problem hiding this comment.
I don't think this should be on by default, as Adrian said inlace resize is still not a 100% safe operation.
There was a problem hiding this comment.
Updated the description to include the the current state of the flag.
|
I was wondering if https://github.com/kubernetes/autoscaler/blob/master/vertical-pod-autoscaler/docs/features.md#in-place-updates-inplaceorrecreate also needs to be updated. There's a sentence "Updates still respect VPA's standard update conditions and timing restrictions" in there, that I think is implying that PDBs are respected, may be that needs some fleshing out and a reference to this new flag? |
Yeah. it should be updated. will fix that |
Signed-off-by: Omer Aplatony <omerap12@gmail.com>
|
/lgtm |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds
--in-place-skip-disruption-budgetflag that skips disruption budget checks for in-place pod updates when all containers have NotRequired resize policy. This allows non-disruptive in-place updates to proceed without being throttled by eviction tolerance, while still respecting disruption budgets for RestartContainer resize policy.Which issue(s) this PR fixes:
Fixes #8980
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: