pkg: fix race condition in StrictFIFO priority ordering #8153
Conversation
Add pre-admission check to detect when a higher-priority workload was added to the queue after Heads() popped a lower-priority one. This prevents admitting lower-priority workloads before evicted higher-priority workloads in StrictFIFO mode. Signed-off-by: Sohan Kunkerkar <sohank2602@gmail.com>
k8s-ci-robot
added
release-note
✅ Deploy Preview for kubernetes-sigs-kueue ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sohankunkerkar 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
size/L
There was a problem hiding this comment.
Pull request overview
This PR fixes a race condition in StrictFIFO priority ordering where higher-priority workloads that are evicted and re-queued during an active scheduling cycle could be admitted after lower-priority workloads. The fix adds a check right before admission to ensure no higher-priority workloads are waiting in the queue.
Key Changes
- Added
Peek()method to the heap utility to inspect the head without removal - Added
HasStrictFIFOHigherPriorityPending()methods to check for higher priority workloads - Integrated priority check in scheduler before workload admission
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/heap/heap.go | Adds Peek() method to inspect heap head without removal |
| pkg/util/heap/heap_test.go | Comprehensive unit tests for Peek() functionality |
| pkg/cache/queue/cluster_queue.go | Implements priority checking logic for StrictFIFO queues |
| pkg/cache/queue/cluster_queue_test.go | Tests HasStrictFIFOHigherPriorityPending with various scenarios |
| pkg/cache/queue/manager.go | Adds manager-level wrapper for priority checking |
| pkg/scheduler/scheduler.go | Integrates priority check before workload admission |
The implementation is sound, with proper thread-safety through read locks, comprehensive test coverage, and efficient O(1) operations. The fix is placed at the correct point in the scheduling flow - after nomination and flavor assignment, but before actual admission - to catch race conditions while minimizing unnecessary checks.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sohankunkerkar
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
|
||
| // Check if a higher-priority workload was added to the queue after we popped this workload. | ||
| // This can happen due to race conditions during workload eviction. | ||
| if s.queues.HasStrictFIFOHigherPriorityPending(e.ClusterQueue, priority.Priority(e.Obj)) { |
There was a problem hiding this comment.
Hm, I have analyzed the logs and summarized it there: #8141 (comment)
The fix seems pretty complex, a lot of new code, which is very rarely useful, so I'm not sure about that PR. I'm not convinced this issue is worth the complication. In any case it requires more thinking / testing, so I was just thinking in the meanwhile we could just fix this test by removing the last line from the test :)
/hold
On the decision how we approach it
There was a problem hiding this comment.
Given that we have a number of flakes the priority for me is to eliminate them. I would leave this open as a dedicated issue, and then re-evaluate the fix.
There was a problem hiding this comment.
I just submitted the quick fix for the flake, ptal #8203
k8s-ci-robot
added
the
do-not-merge/hold
3 participants
What type of PR is this?
/kind bug
/kind flake
What this PR does / why we need it:
When a workload is evicted and re-queued, it can be added back to the queue while the scheduler is processing an older batch. This causes lower-priority workloads to be admitted before higher-priority ones. This fixes the issue by checking the queue for higher-priority workloads right before admission.
Which issue(s) this PR fixes:
Fixes #8141
Special notes for your reviewer:
Does this PR introduce a user-facing change?