feat: VPA InPlace updateMode #8888
Conversation
Signed-off-by: Omer Aplatony <[email protected]>
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
k8s-ci-robot
added
area/vertical-pod-autoscaler
kind/api-change
Signed-off-by: Omer Aplatony <[email protected]>
|
This is a draft since we need the AEP to be merged first. |
Signed-off-by: Omer Aplatony <[email protected]>
Signed-off-by: Omer Aplatony <[email protected]>
k8s-ci-robot
added
size/XL
| return podList | ||
| } | ||
|
|
||
| func setupPodsForInPlaceMode(f *framework.Framework, hamsterCPU, hamsterMemory string, withRecommendation bool) *apiv1.PodList { |
There was a problem hiding this comment.
Good catch, I had planned to use it but ended up going in a different direction ( removed in 69c5daf)
| WithTarget(containerName, "200m"). | ||
| WithLowerBound(containerName, "200m"). | ||
| WithUpperBound(containerName, "200m"). |
There was a problem hiding this comment.
if you want to use this function, then I think the function's arguments should be changed. The first argument should be a value with CPU units (or empty string based on previous calls), like millicores, and the second one should be a value with a memory unit.
There was a problem hiding this comment.
I agree, but since this pattern appears throughout the entire codebase, I decided to keep it consistent for now. I agree it should be updated, but I don’t think it’s within the scope of this PR.
| case vpa_types.UpdateModeInPlace: | ||
| if !features.Enabled(features.InPlace) { | ||
| return utils.InPlaceEvict |
There was a problem hiding this comment.
do these lines mean that when the new mode (InPlace) is used in a VPA object, but its feature flag is disabled, we evict the pod? May I ask the reason?
Personally I would expect that we don't do anything with this type of pod, just log this as a event
There was a problem hiding this comment.
ach actually I think we can't reach that switch case because of this line: https://github.com/omerap12/autoscaler/blob/c0016e9870aadf2a9a5905d658f5c6c894fb2603/vertical-pod-autoscaler/pkg/updater/logic/updater.go#L274-L276
There was a problem hiding this comment.
Yes, though we may want to add one more check here as well. I'm not entirely sure - open to suggestions.
Signed-off-by: Omer Aplatony <[email protected]>
3 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
VPA InPlace updateMode support.
Which issue(s) this PR fixes:
Fixes #8720
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: