Add proactive scaleup #7145
Add proactive scaleup #7145
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
cncf-cla: yes
|
Hi @abdelrahman882. 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
the
size/XXL
abdelrahman882
force-pushed
the
proactive-scaleup
branch
from
2dee804 to
294b959
Compare
cluster-autoscaler/main.go
Outdated
| @@ -265,6 +268,9 @@ var ( | |||
| "Eg. flag usage: '10000:20,1000:100,0:60'") | |||
| provisioningRequestsEnabled = flag.Bool("enable-provisioning-requests", false, "Whether the clusterautoscaler will be handling the ProvisioningRequest CRs.") | |||
| frequentLoopsEnabled = flag.Bool("frequent-loops-enabled", false, "Whether clusterautoscaler triggers new iterations more frequently when it's needed") | |||
| proactiveScaleupEnabled = flag.Bool("enable-proactive-scaleup", false, "Whether to enable/disable proactive scale-ups, defaults to false") | |||
| podInjectionLimit = flag.Int("pod-injection-limit", 5000, "Limits total number of pods while injecting fake pods. If unschedulable pods already exceeds the limit, pod injection is disabled but pods are not truncated.") | |||
| nodeLimit = flag.Int("node-limit", 15000, "Limits total number of nodes in cluster to avoid overloading KCP, only used when --enable-proactive-scaleup is set to true. Defaults to 15k nodes") | |||
There was a problem hiding this comment.
What is the reason to introduce this flag? It is essentially the same as --max-nodes-total. As a user, how would I decide when to use one vs the other?
cluster-autoscaler/main.go
Outdated
| podListProcessor = pods.NewCombinedPodListProcessor([]pods.PodListProcessor{podInjectionPodListProcessor, podListProcessor, enforceInjectedPodsLimitProcessor}) | ||
|
|
||
| // FakePodsScaleUpStatusProcessor processor needs to be the first processor in ScaleUpStatusProcessor as it filters out fake pods from | ||
| // Scale Up status so that we don't emit events, visibility logs, ..etc for them. |
There was a problem hiding this comment.
nit: Visibility events are a GKE specific concept, so I wouldn't mention them here.
abdelrahman882
force-pushed
the
proactive-scaleup
branch
3 times, most recently
from
ed601a8 to
c488df7
Compare
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
The code changes look good to me, can you re-add the release note box in the first comment and add a more user-facing release note there? For instance: |
| return desiredReplicas | ||
| } | ||
|
|
||
| func workQueue(job *batchv1.Job) bool { |
There was a problem hiding this comment.
Can we add a comment here describing what this method evaluates since it's a bit hard to read/reason
Or maybe we can use an if/else statement to simplify it a bit
There was a problem hiding this comment.
Clarified with a comment
|
|
||
| // Removes NoScaleUpInfo entries which contain a fake pod from the input list | ||
| // Returns a list of NoScaleUpInfo which doesn't contain any fake pods | ||
| func filterNoScaleUpInfos(noScaleUpInfos []status.NoScaleUpInfo) ([]status.NoScaleUpInfo, []types.UID) { |
There was a problem hiding this comment.
If we can use generics here, maybe we can just have one method instead of filterNoScaleUpInfos & filterFakePods
@x13n I remember we talked about this before but I don't remember the decision :'D
There was a problem hiding this comment.
I think one more generic filtering here would be ok, though these two functions are not identical, so I'm not sure if it is enough shared code to justify introducing a shared generic. I'm fine either way, would leave it up to @abdelrahman882 to pick one.
There was a problem hiding this comment.
Both are sharing almost same logic (filtering fake pods from a list of pods or pod wrappers), I will go with one generic function to avoid having duplicated code and logging messages
k8s-ci-robot
added
the
needs-rebase
abdelrahman882
force-pushed
the
proactive-scaleup
branch
from
c488df7 to
01e9433
Compare
k8s-ci-robot
removed
the
needs-rebase
|
Thanks for the changes! /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abdelrahman882, x13n 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
What type of PR is this?
/kind feature
What this PR does / why we need it:
Introduce proactive scale up, to proactively scale up nodes based on replica count by injecting fake pods that triggers scale up logic which eliminates the need for waiting for the real pod to be marked as not scheduled then act and scale up
Does this PR introduce a user-facing change?
Cluster Autoscaler can now trigger large scale ups before all pending pods are created. This behavior is disabled by default and can be enabled with
--enable-proactive-scaleupflag.enable-proactive-scaleup : To enable the new feature
pod-injection-limit : Limits the total number of injected pods