Fix proactive scale up injecting fake pods for scheduling gated pods #8580
Conversation
k8s-ci-robot
added
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: abdelrahman882 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/M
abdelrahman882
force-pushed
the
proactive-scaleup-schgates
branch
from
286ba74 to
ff346b2
Compare
| } | ||
|
|
||
| func filterOutSchedulingGatedPods(groups map[types.UID]podGroup, allPods []*apiv1.Pod) map[types.UID]podGroup { | ||
| if groups != nil { |
There was a problem hiding this comment.
Checking for nil seems fine, but if this is the case do we want to return nil back (L58 below). tl;dr it doesn't seem like we would ever expect a nil map and so if there's a chance we might get one and want to defensively handle that we might add a 2nd error response object to the func?
There was a problem hiding this comment.
I agree, returned error, also added a check len(groups) > 0 before getting the gated pods, so we son't loop over all pods in case we don't have groups at all.
abdelrahman882
force-pushed
the
proactive-scaleup-schgates
branch
from
ff346b2 to
4627816
Compare
k8s-ci-robot
added
size/L
cluster-autoscaler/processors/podinjection/pod_injection_processor_test.go
Outdated
Show resolved
Hide resolved
| groupedPods := groupPods(append(scheduledPods, unschedulablePods...), controllers) | ||
| var podsToInject []*apiv1.Pod | ||
|
|
||
| allPods, err := ctx.AllPodLister().List() |
There was a problem hiding this comment.
Why list all pods? You can decide if a pod has a scheduling gate or not just by looking at it, no need to cross reference the list of pods with itself I think?
There was a problem hiding this comment.
tl;dr We have to check the list of pods as the one we have doesn't include the gated pods
Pods sent to the processors are the (unschedulable pods + unprocessed pods), the the gated pods are not included there, that's why we inject fake pods for them.
So we have to list all pods and get those that got ignored in list pods before
abdelrahman882
force-pushed
the
proactive-scaleup-schgates
branch
from
4627816 to
030ad2c
Compare
abdelrahman882
force-pushed
the
proactive-scaleup-schgates
branch
from
030ad2c to
a281a40
Compare
| for _, podOwnerRef := range pod.OwnerReferences { | ||
| // SchedulingGated pods can't be unschedualable nor unprocessed nor scheduled so it is not expected | ||
| // to have them as group sample nor in pod count, so decreasing desiredReplicas by one is enough | ||
| if grp, found := groups[podOwnerRef.UID]; found { |
There was a problem hiding this comment.
This should only subtract when podOwnerRef.Controller != nil && *podOwnerRef.Controller to be in sync with updatePodGroups below. But actually, instead of adding replicas and then subtracting them here, wouldn't it suffice to pass scheduling gated pods to groupPods? We're already passing a combined list of scheduled and unschedulable pods there.
What type of PR is this?
/kind bug
What this PR does / why we need it:
Proactive scale up is listing the controllers, check the desired replicas and inject fake pods to proactively scale up even before these pods are created and marked as unschedulable.
Proactive scale up is using the following formula for the number of fake pods to inject per controller:
Number of fake pods = number of controller desired pods - (scheduled pods + unschedualable pods + unprocessed pods)
The problem here is that scheduling gated pods are not being excluded along with unschedualable and unprocessed so proactive scale up injects fake pods for these gated pods ignoring the condition. that happens each loop which leads to not scaling down that empty space.
This PR subtracts the number of pods with scheduling gates from the number of fake pods to inject.
Which issue(s) this PR fixes:
NONE
Special notes for your reviewer:
ctx.AllPodLister().List()should be using a lister, so we are not having extra api call here to list the pods rather we just get it from cache.PodInjectionPodListProcessorin case of any error filtering out scheduling gated pods, I choose to only log a warning and not return an error so it doesn't block the rest of the processors asCombinedPodListProcessorstops the processing in case any one returns an error, which means any following processor (all others) will not be executedDoes this PR introduce a user-facing change?