Repository navigation
Don't prefer a flavor with no preemption candidates over one that fits - #13616
kubernetes-prow[bot] merged 3 commits into
Conversation
With flavorFungibility.preference: PreemptionOverBorrowing, isPreferred compares borrowingLevel before preemptionMode. A flavor whose preemption simulation found no candidates could therefore outrank a later flavor that fits, purely because its quota was sourceable at a shallower level in the cohort tree. Admission in that flavor is impossible for the scheduling attempt, so the workload stayed pending indefinitely while the fitting flavor sat idle. Rank noPreemptionCandidates below all viable modes before applying the configured preference, keeping noFit as the worst mode. This is a no-op for the default preference and for BorrowingOverPreemption, which already compare preemptionMode first. Signed-off-by: Yiqing Wang <yiqing@wangemail.com>
kubernetes-prow
Bot
added
release-note
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
kubernetes-prow
Bot
added
cncf-cla: yes
|
Hi @YQ-Wang. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
kubernetes-prow
Bot
added
the
size/L
📝 WalkthroughWalkthroughThe flavor assigner now ranks viable modes with preemption candidates above modes without candidates. Tests cover hierarchical selection, comparison behavior, and scheduling fallback to a deeper flavor. ChangesFlavor preemption preference
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant FlavorAssigner
participant ClusterQueue
Scheduler->>FlavorAssigner: Compare hierarchical flavor modes
FlavorAssigner->>ClusterQueue: Evaluate preemption candidates and fit
ClusterQueue-->>FlavorAssigner: Return mode status
FlavorAssigner-->>Scheduler: Prefer the next viable flavor
Scheduler->>ClusterQueue: Admit workload on selected flavor
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The flavor is fully evaluated and then out-ranked, not skipped. "Skipping" a flavor already means something else in the flavor assigner: the nomination mapping and allowed-flavor annotation paths that continue before the flavor is evaluated at all. Signed-off-by: Yiqing Wang <yiqing@wangemail.com>
|
/ok-to-test |
kubernetes-prow
Bot
added
ok-to-test
|
/assign |
| // A flavor without preemption candidates cannot be admitted in this | ||
| // scheduling attempt. Rank it below viable modes before applying the | ||
| // configured fungibility preference, while retaining noFit as the worst mode. | ||
| aHasNoCandidates := a.preemptionMode == noPreemptionCandidates | ||
| bHasNoCandidates := b.preemptionMode == noPreemptionCandidates | ||
| if aHasNoCandidates != bHasNoCandidates { | ||
| return !aHasNoCandidates | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit: am I right that this would be equivalent?
(Just cloning the above block handling noFit).
| // A flavor without preemption candidates cannot be admitted in this | |
| // scheduling attempt. Rank it below viable modes before applying the | |
| // configured fungibility preference, while retaining noFit as the worst mode. | |
| aHasNoCandidates := a.preemptionMode == noPreemptionCandidates | |
| bHasNoCandidates := b.preemptionMode == noPreemptionCandidates | |
| if aHasNoCandidates != bHasNoCandidates { | |
| return !aHasNoCandidates | |
| } | |
| if a.preemptionMode == noPreemptionCandidates { | |
| return false | |
| } | |
| if b.preemptionMode == noPreemptionCandidates { | |
| return true | |
| } | |
In my feeling, this version would read naturally enough to not even need the // comment.
WDYT?
There was a problem hiding this comment.
Actually, this is not equivalent, because when a.preemptionMode = b.preemptionMode = noPreemptionCandidates it would return false, but before it would perform downstream checks.
Worth checking if we have unit tests for that.
There was a problem hiding this comment.
Yes, TestIsPreferred already covers two noPreemptionCandidates modes in both borrowing-level orderings. The lower borrowing level is preferred, and the reverse is not. The suggested sequential checks would fail the first case, so this behavior is explicitly protected.
There was a problem hiding this comment.
Ah, right. Then it's fine as it is.
| wantMode: Fit, | ||
| wantAssigment: rfMap{corev1.ResourceCPU: "two"}, | ||
| }, | ||
| "Select deeper flavor that fits when shallower flavor has no preemption candidates": { |
There was a problem hiding this comment.
Will it be obvious for future readers what "deeper flavor" or "shallower flavor" means?
(So far, in whole Kueue code, I see very little occurences of "shallow"/"deep" referring to Cohort hierarchy)
How about "deeper-borrowing" and "shallower-borrowing"? That'd make a stronger hint.
There was a problem hiding this comment.
Good point. I updated this to use deeper-borrowing and shallower-borrowing, and made the matching TestIsPreferred case consistent.
| if testClusterQueueFlavors == nil { | ||
| testClusterQueueFlavors = []kueue.FlavorQuotas{ | ||
| *utiltestingapi.MakeFlavorQuotas("one").Resource(corev1.ResourceCPU, "0").Obj(), | ||
| *utiltestingapi.MakeFlavorQuotas("two").Resource(corev1.ResourceCPU, "0").Obj(), | ||
| *utiltestingapi.MakeFlavorQuotas("three").Resource(corev1.ResourceCPU, "0").Obj(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: I'd put it explicitly on every test case. Possibly using a shared var / builder func if you like.
So that, closer to table tests philosophy, all test inputs are clearly listed in one place.
Especially in this case, where you have just 2 test cases relying on this defaulting.
There was a problem hiding this comment.
Done, testClusterQueueFlavors is now explicit in every case
kubernetes-prow
Bot
added
the
lgtm
|
LGTM label has been added. DetailsGit tree hash: a22eeceff13bf9eeaa5a6143eed10ba5846fdc64 |
kubernetes-prow
Bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mimowo, YQ-Wang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
the
approved
Signed-off-by: Yiqing Wang <yiqing@wangemail.com>
kubernetes-prow
Bot
removed
the
lgtm
|
/unhold |
kubernetes-prow
Bot
removed
the
do-not-merge/hold
kubernetes-prow
Bot
added
the
lgtm
|
LGTM label has been added. DetailsGit tree hash: bc4a35fe2eb967d971ee802ed79cc0ed91bcfaa5 |
|
/cherrypick release-0.19 |
|
@mimowo: #13616 failed to apply on top of branch "release-0.18": DetailsIn 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-sigs/prow repository. |
|
@mimowo: new pull request created: #13896 DetailsIn 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-sigs/prow repository. |
|
@YQ-Wang could you please prepare the CP for 0.18? |
|
/release-note-edit |
What type of PR is this?
/kind bug
What this PR does / why we need it:
With
flavorFungibility.preference: PreemptionOverBorrowing,isPreferredcomparesborrowingLevelbeforepreemptionMode. That lets a flavor whose preemption simulation found no candidates outrank a later flavor that fits, purely because its quota is sourceable at a shallower level in the cohort tree.The first flavor cannot admit the workload in that scheduling attempt, and the fitting flavor loses the comparison because borrowing depth is considered first, so the workload stays pending indefinitely while quota sits idle. Every cycle repeats the same decision.
This PR ranks
noPreemptionCandidatesbelow all viable modes before applying the configured preference, keepingnoFitas the worst mode.The change only affects
PreemptionOverBorrowing. The default preference andBorrowingOverPreemptionalready comparepreemptionModefirst, so they are unaffected.Which issue(s) this PR fixes:
Fixes #13607
Special notes for your reviewer:
/cc @amy
Tests are added at three levels, and all of them fail without the fix:
TestIsPreferredcovers the new ordering directly, including thatnoPreemptionCandidatesstill beatsnoFitand that borrowing level still breaks ties between two such flavors.TestHierarchicalcase pins the first flavor toNoCandidatesat borrowing level 1 through a fake oracle, while exercising the flavor assigner’s real selection path against a fitting second flavor.TestSchedulecase reproduces the issue end to end and asserts the workload is admitted into the second flavor. It needs a two-level cohort: with a flat cohort both flavors land at the same borrowing level, the existing tie-break onpreemptionModefires, and the bug does not appear.The middle case may look redundant next to the end-to-end one, but it guards against a specific kind of drift.
TestSchedulederives its borrowing levels from the real oracle and real cohort-height math, so if a future change moved the first flavor to the same level as the second, the levels would tie, the pre-existing tie-break would select the fitting flavor, and the test would keep passing without exercising this bug at all. Pinning the oracle result keeps one test anchored to the ordering itself. Happy to drop it if you would rather not carry the extra layer.One thing worth flagging, which I left out of scope. The docs and the KEP describe
PreemptionOverBorrowingover a binary borrow key:(Fit, NoBorrow)>(Preempt, NoBorrow)>(Fit, Borrow)>(Preempt, Borrow)The implementation compares the full
borrowingLeveldepth first, so in a hierarchical cohort(preempt, level 1)still outranks(fit, level 2)even when preemption candidates do exist. That is not starvation, since preemption makes progress, so I kept this PR narrow. Happy to follow up if you would like the graded comparison reconciled with the documented ordering.AI assistance was used to help implement and review this change. The author reviewed and tested the resulting code and takes responsibility for it.
Does this PR introduce a user-facing change?
Summary by CodeRabbit
Summary by CodeRabbit