Make changes to updater to add the unboosting logic #8618
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
area/vertical-pod-autoscaler
needs-ok-to-test
|
Hi @kamarabbas99. Thanks for your PR. I'm waiting for a kubernetes 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
size/XL
|
/cc laoj2 |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
kamarabbas99
force-pushed
the
feature-cpu-upd
branch
2 times, most recently
from
6286cc8 to
79460ad
Compare
| } | ||
| err := inPlaceLimiter.InPlaceUpdate(pod, vpa, u.eventRecorder) | ||
| if err != nil { | ||
| klog.V(0).InfoS("Unboosting failed", "error", err, "pod", klog.KObj(pod)) |
There was a problem hiding this comment.
What happens if the unboosting operation fails each time? don't we want to fallback to eviction at some point?
There was a problem hiding this comment.
Dont we risk having an eviction loop in that case?
There was a problem hiding this comment.
We do. I need to look at the AEP if we discussed about it or not, but if we don't have an eviction there could be a case that the boosted workload will keep being boosted. maybe it's ok to leave it that way but we need to properly write this.
There was a problem hiding this comment.
kamarabbas99
force-pushed
the
feature-cpu-upd
branch
from
79460ad to
302be43
Compare
| if !PodReady(pod) { | ||
| return false | ||
| } |
There was a problem hiding this comment.
This function is checking for readiness and startup time duration. Does it make sense to remove the readiness check? I like it as a guard, but it seems surprising.
Or may be rename the function to include that the readiness is true?
cc @omerap12
There was a problem hiding this comment.
If we remove this check we have to rename this function for future use.
We already check if the pod is ready each time we use this function:
- https://github.com/kubernetes/autoscaler/pull/8618/files/302be4374a6e9f4e0909df7bb985d6a10c2ae809#diff-861cbdb52ed6e832a7821c3f6fe4d18123631b360f5042015cb5e8cadfa625b4R43
- https://github.com/kubernetes/autoscaler/pull/8618/files/302be4374a6e9f4e0909df7bb985d6a10c2ae809#diff-b864a8cf8e133025f0a68284881b2abfda1a098347250e322b8ed2b61b7590cfR252
But I worry for other folks who maybe want to use this function and won't check if the pod is ready
There was a problem hiding this comment.
I changed the name of function to include the readiness check, also removed the redundant readiness checks.
kamarabbas99
force-pushed
the
feature-cpu-upd
branch
from
302be43 to
7163588
Compare
kamarabbas99
force-pushed
the
feature-cpu-upd
branch
from
7163588 to
88a6748
Compare
|
/lgm |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kamarabbas99, 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
merged commit 32c68bd
into
kubernetes:experimental-cpu-boost
4 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Introduces changes in the updater component to unboost cpu if its applied by the admission-controller.
Also the original cpu request is added in annotation because we need to revert to original(not recommeded) if update mode is set to Off.
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: