Avoid unnecessary requeue operations in coscheduling #700
Conversation
k8s-ci-robot
added
release-note
✅ Deploy Preview for kubernetes-sigs-scheduler-plugins canceled.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Huang-Wei 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
approved
|
cc @denkensk |
|
it seems to be a valuable issue and fix to me👍 /lgtm /hold for @denkensk |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
pkg/coscheduling/core/core.go
Outdated
| @@ -48,12 +48,22 @@ const ( | |||
| PodGroupNotFound Status = "PodGroup not found" | |||
| Success Status = "Success" | |||
| Wait Status = "Wait" | |||
|
|
|||
| preFilterStateKey = "PreFilterCoscheduling" | |||
There was a problem hiding this comment.
Seems not a proper name, or permitStateKey?
There was a problem hiding this comment.
Yes, good catch...
| @@ -209,6 +226,19 @@ func (pgMgr *PodGroupManager) Permit(ctx context.Context, pod *corev1.Pod) Statu | |||
| if int32(assigned)+1 >= pg.Spec.MinMember { | |||
| return Success | |||
| } | |||
|
|
|||
| if assigned == 0 { | |||
There was a problem hiding this comment.
Based on my understanding, when assigned == 0, we should and only should trigger the activating, then in ActivateSiblings, once Activate is Ture, we should start the trigger but now we're the opposite. And what's more, shouldn't we set the Activate to False in the follow up.
There was a problem hiding this comment.
when assigned == 0, we should and only should trigger the activating, then in ActivateSiblings, once Activate is Ture, we should start the trigger but now we're the opposite.
That's the current case. Why do you think it's the opposite.
shouldn't we set the Activate to False in the follow up.
Nope b/c every scheduling cycle comes with its own CycleState instance.
There was a problem hiding this comment.
Make sense.
/lgtm
/hold cancel
k8s-ci-robot
removed
the
lgtm
|
/cherrypick release-1.28 |
|
@Huang-Wei: once the present PR merges, I will cherry-pick it on top of release-1.28 in a new PR and assign it to you. In response to this:
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/test-infra repository. |
k8s-ci-robot
removed
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
@Huang-Wei: new pull request created: #718 In response to this:
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/test-infra repository. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
The current coscheduling code aggressively actives a pod's PodGroup siblings in
Permit(). For example, if a deployment doesn't reach the minMember, all its pods are in pending state. Once it's scaled up to reach the minMember, it'd triggerActivateSiblings()N times - this causes a ton of unnecessary re-queue operations which cause the CPU usage to spike, and may relates with potential starvation reported in #682.Which issue(s) this PR fixes:
Fixes #682
Special notes for your reviewer:
Does this PR introduce a user-facing change?