Repository navigation
scheduler: prioritize workloads pending preemption to prevent thrashing - #13797
Conversation
kubernetes-prow
Bot
added
release-note
✅ Deploy Preview for kubernetes-sigs-kueue ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded pending-preemption detection and scheduler ordering. Integration coverage verifies admission waits for all evictions. Queue concurrency tests cover both supported strategies. ChangesPending preemption ordering
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
kubernetes-prow
Bot
added
cncf-cla: yes
|
Hi @Nilsachy. 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/M
Nilsachy
changed the title
|
/assign kshalot |
|
/assign mimowo |
kubernetes-prow
Bot
added
ok-to-test
|
let's also add an integration test for the scenario from the PR description |
… ClusterQueue configuration
kubernetes-prow
Bot
added
size/L
kubernetes-prow
Bot
removed
the
do-not-merge/work-in-progress
|
Thanks, I'm checking this for today's patch release |
| var result []Head | ||
| for _, v := range q.queued { | ||
| result = append(result, *v) | ||
| result = append(result, Head{Info: *v}) |
There was a problem hiding this comment.
Isn't there a problem here? We lose IsPreemptor state. So we either have to store Heads in q.queued or recompute IsPreemptor here (or revert the Head addition and add IsPreemptor to workload.Info and revisit this in a followup. It would reduce the diff in this PR.
There was a problem hiding this comment.
yeah, right. each entry already has Head. So, I think we can easily replace workload.Info with Head for q.queued.
There was a problem hiding this comment.
So, I meant we can easily replace workload.Info with Head for q.queued in this PR.
There was a problem hiding this comment.
I might be wrong but doesn't a workload enter secondPassQueue (workload.NeedsSecondPass) only if it already has its quota reserved? https://github.com/kubernetes-sigs/kueue/blob/main/pkg/workload/workload.go#L974
So in that case it will never have the IsPreemptor set to true anyway
There was a problem hiding this comment.
Ah, it looks like a really good point. We have !HasQuotaReservation(w) as requirment for second pass queue.
kueue/pkg/workload/workload.go
Lines 975 to 977 in b116e8c
There was a problem hiding this comment.
Although I agree that from a semantics perspective it would be cleaner. I think it is still worth doing. Wdyt?
There was a problem hiding this comment.
I think it makes sense and it would be more consistent. And also I think it would save us a few memory allocations.
But I don't see this is a blocker for this PR, leaving it up to you and Yuki
There was a problem hiding this comment.
Yes, we can do that, but can you work on it in follow-up PR? Because this PR now is blocking our today's patch release...
There was a problem hiding this comment.
@alien1403 PTAL at this discussion for more context on #14681
| workloads = append(workloads, wlCopy) | ||
| heads = append(heads, Head{ | ||
| Info: wlCopy, | ||
| IsPreemptor: cq.IsPreemptor(wl), |
There was a problem hiding this comment.
I think technically there is a small chance of races here, because we are calculating cq.IsPreemptor before taking the CQ Snapshot. So in theory they could be misaligned.
I think the risk is small though, it will just treat the WL as preemptor in one more cycle. Could be a follow-up fix maybe
There was a problem hiding this comment.
Good point, maybe this can be also part of follow up PR (together with semantics improvement mentioned above)
There was a problem hiding this comment.
I checked @kshalot 's comment carefully, and you're right since pw.set will be executed concurrently by the scheduler goroutine.
But, this can be a follow-up since it needs just one more cycle.
There was a problem hiding this comment.
@alien1403 PTAL at this discussion for more context on #14681
There was a problem hiding this comment.
So we have this structure: Heads -> Snapshot -> Nominate -> Requeue
In Heads, we read pw once and save IsPreemptor as a boolean. Later steps only read that boolean, and pw.set is only called at the very end in Requeue. As everything runs in order on one thread, nothing can touch pw between Heads and Snapshot
WDYT? cc: @kshalot @tenzen-y
There was a problem hiding this comment.
pw.clear() can be called in a separate goroutine in the Workload Reconciler:
kueue/pkg/controller/core/workload_controller.go
Line 1384 in 6424217
But I think this race condition is not worth considering anyway - if the workload is deleted after we already took it from the queue with Heads(), it doesn't really matter how it will be sorted in the iterator because the cycle will fail with a 404 at some point anyway 🤷 I'd disregard this
|
@Nilsachy we have to rebased onto the main branch. |
kubernetes-prow
Bot
added
the
needs-rebase
kubernetes-prow
Bot
removed
the
needs-rebase
tenzen-y
left a comment
tenzen-y
left a comment
There was a problem hiding this comment.
Thank you for this great improvement!
/lgtm
/approve
kubernetes-prow
Bot
added
the
lgtm
|
LGTM label has been added. DetailsGit tree hash: b4fd4d8418d564525263f4951301fdf1c0cfa2a1 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Nilsachy, tenzen-y 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
|
@Nilsachy can you manually prepare cherry pick PRs for 0.18 and 0.19? |
You can leverage hack/cherry_pick_pull.sh |
Sure I'll do it now |
|
@coderabbitai can you open a follow-up issue for #13797 (comment) and #13797 (comment) w/ kind/cleanup label, and assign @Nilsachy to the issue? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="kubernetes-sigs/kueue"
pr_number="13797"
echo "== Requested review comments =="
for comment_id in 3820759470 3820718664; do
gh api \
-H "Accept: application/vnd.github+json" \
"/repos/${repo}/pulls/comments/${comment_id}" \
--jq '{id, html_url, path, line, original_line, body, user: .user.login}'
done
echo
echo "== Cleanup label =="
gh label list --repo "$repo" --limit 200 \
--json name,color,description \
--jq '.[] | select(.name == "kind/cleanup")'Length of output: 1832 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="kubernetes-sigs/kueue"
pr_url="https://github.com/kubernetes-sigs/kueue/pull/13797"
comment_1="https://github.com/kubernetes-sigs/kueue/pull/13797#discussion_r3820759470"
comment_2="https://github.com/kubernetes-sigs/kueue/pull/13797#discussion_r3820718664"
body=$(cat <<EOF
## Summary
Ensure that preemptor state remains consistent between the queue state and the ClusterQueue scheduling snapshot.
## Rationale
The current implementation has two related follow-up concerns:
1. \`pkg/cache/queue/manager.go\` calculates \`cq.IsPreemptor\` before it takes the ClusterQueue snapshot. Concurrent state changes can make the calculated value and the snapshot state differ for one scheduling cycle.
2. \`pkg/cache/queue/second_pass_queue.go\` can lose \`IsPreemptor\` state when it stores queued workload data. The implementation needs a durable representation or a correct recomputation point.
## Affected areas
- \`pkg/cache/queue/manager.go\`
- \`pkg/cache/queue/second_pass_queue.go\`
- Preemptor-state propagation in queue snapshots and second-pass scheduling
## Acceptance criteria
- Preemptor state and the ClusterQueue snapshot use a consistent view of queue state.
- The second-pass queue preserves or correctly recomputes preemptor state.
- Tests cover concurrent queue-state changes and second-pass scheduling behavior.
- The change does not regress sticky pending-preemption scheduling.
## References
- PR: ${pr_url}
- Review comment on snapshot consistency: ${comment_1}
- Review comment on second-pass state preservation: ${comment_2}
Requested by `@tenzen-y`.
EOF
)
issue_url=$(gh issue create \
--repo "$repo" \
--title "Ensure consistent preemptor state in queue snapshots and second-pass scheduling" \
--body "$body" \
--label "kind/cleanup" \
--assignee "Nilsachy")
echo "Created issue: ${issue_url}"Length of output: 2080
It has the 🧠 Learnings usedYou are interacting with an AI system. |
|
/release-note-edit |
…ing preemption to prevent thrashing (#14682) * Automated cherry pick of #13797: scheduler: prioritize workloads pending preemption to prevent thrashing * chore: downgrade PrioritizePreemptorWorkloads feature version to 0.19 * refactor: remove redundant local queue weight tracking from ClusterQueue heap comparator * fix: update newClusterQueue constructor call in TestClusterQueueIsPreemptor to match signature
…ing preemption to prevent thrashing (#14683) * Automated cherry pick of #13797: scheduler: prioritize workloads pending preemption to prevent thrashing * chore: downgrade PrioritizePreemptorWorkloads feature gate version to 0.18 * update newClusterQueue call in ClusterQueueIsPreemptor test to match signature change
What type of PR is this?
/kind bug
What this PR does / why we need it:
When a workload requires preemption/reclamation of multiple target workloads across a cohort to acquire their combined quota, desynchronized eviction completion times can cause preemption thrashing and infinite preemption loops.
Specifically, if one target workload (
wlA) finishes evicting and releases its quota early, the preempting workload (preemptor-wl) cannot yet be admitted because it is still waiting for remaining target workloads (wlB) to finish evicting. During this window, because the pending quota forpreemptor-wlis not reserved or locked in the persistent cache across scheduling cycles:wlAis re-admitted,preemptor-wlremains blocked and must re-issue a preemption request againstwlA.wlBsubsequently completes eviction, it similarly steals the freed quota and gets re-admitted, causing preemptor-wl to issue another preemption againstwlB.preemptor-wlto acquire the combined quota it needs.To resolve this, this PR introduces a prioritization mechanism guarded by the
PrioritizePreemptorWorkloadsfeature gate that leverages ClusterQueue preemptor tracking (preemptorWorkload):preemptorWorkload) and its evaluated.metadata.generation. When popping queue heads inmanager.Heads(), we querycq.IsPreemptor(wl)(verifying strict generation matching) and stampIsPreemptor: truedirectly onto the scheduler'sHeadstruct.PrioritizePreemptorWorkloadsfeature gate, scheduler iterators (makeClassicalIteratorin Classical andentryComparer.lessin Fair Sharing) prioritize candidate workloads withIsPreemptor == trueat the top of the evaluation order.cq.AddUsage), even whenReclaimWithinCohort: Any(CanAlwaysReclaim) is configured. This effectively locks the quota freed during desynchronized target evictions and prevents lower-priority or competitor workloads from stealing it mid-eviction.Which issue(s) this PR fixes:
Fixes #13662
Does this PR introduce a user-facing change?
Summary by CodeRabbit