Extender preemption should respect IsInterested() #66291
Conversation
k8s-ci-robot
added
release-note
|
Thanks for picking up 👍 |
|
/assign @bsalamat A preemption related minor fix, kindly ping for review. |
|
/lgtm |
k8s-ci-robot
added
lgtm
Co-authored-by: Harry Zhang <resouer@gmail.com> Co-authored-by: Chun Chen <ramichen@tencent.com>
k8s-ci-robot
removed
lgtm
| @@ -1349,7 +1373,7 @@ func TestPreempt(t *testing.T) { | |||
| t.Errorf("unexpected error in preemption: %v", err) | |||
| } | |||
| if (node != nil && node.Name != test.expectedNode) || (node == nil && len(test.expectedNode) != 0) { | |||
| t.Errorf("expected node: %v, got: %v", test.expectedNode, node) | |||
| t.Errorf("expected node: %v, got: %v", test.expectedNode, node.GetName()) | |||
There was a problem hiding this comment.
Note for reviewers: logging out whole node object here seems too verbose.
|
cc @kubernetes/sig-scheduling-pr-reviews , any comments? |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/cc @bsalamat |
| @@ -282,7 +282,7 @@ func (g *genericScheduler) processPreemptionWithExtenders( | |||
| ) (map[*v1.Node]*schedulerapi.Victims, error) { | |||
| if len(nodeToVictims) > 0 { | |||
| for _, extender := range g.extenders { | |||
| if extender.SupportsPreemption() { | |||
| if extender.SupportsPreemption() && extender.IsInterested(pod) { | |||
There was a problem hiding this comment.
Here's a case in my mind: ext_1 interests pod_1, ext_2 interests pod_2, both pods can preempt pod_3, pod_4, pod_5 (priorities in order); if pod_3 and pod_5 on the same pod, scheduler may preempt pod_3 and pod_5 because of race condition, as the extender did not know pod's new status until get event from apiserver.
Did we discuss this case when introducing preemption extender?
There was a problem hiding this comment.
In "both pods can preempt pod_3, pod_4, pod_5 (priorities in order); if pod_3 and pod_5 on the same pod", what do you mean pods or pod here?
There was a problem hiding this comment.
Note that dup or re-preemption in this case is tolerable by system.
We did talk about information sync in extender side, for now, we left it for extender to handle as it will not break existing logic.
There was a problem hiding this comment.
we left it for extender to handle as it will not break existing logic.
Extender need to get Pod's latest status to "handle" it (e.g. nominatedNode and deleteTimestamp from apiserver), we can not make sure extender get the latest status when we schedule next pod. That's similar case that we introduced Bind. Do we know who's the user of preemption extender? I'm ok to keep current behaviour if no user or the user accept that; as I'd like to keep it simple enough.
There was a problem hiding this comment.
In "both pods can preempt pod_3, pod_4, pod_5 (priorities in order); if pod_3 and pod_5 on the same pod", what do you mean pods or pod here?
@xiaoxubeii , pods is pod_1 and pod_2; pod should be node, typo :)
There was a problem hiding this comment.
cc @ravigadde for user input
There was a problem hiding this comment.
@resouer My experience has been with one extender only. Diff seems reasonable.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bsalamat, resouer, xiaoxubeii, yastij 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
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
Extender preemption should respect IsInterested()
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #66289
Special notes for your reviewer:
The bug is reported and the first commit is co-authored by: @chenchun
Release note: