Victim reprieve logic in SelectVictimsOnNode reprieves lower priority pods first instead of higher priority pods
kind/bug
### Description
In `pkg/scheduler/actions/preempt/preempt.go`, the `SelectVictimsOnNode` function has a bug in the victim reprieve logic.
The code comment states that it should "reprieve higher priority pods first", but the actual implementation reprieves lower priority pods first due to order reversal
introduced by `BuildVictimsPriorityQueue`.
The root cause is that:
1. `sort.Slice` sorts victims by K8s pod priority [high→low]
2. `BuildVictimsPriorityQueue` uses `!ssn.TaskOrderFn` with negation, so `Pop()` returns [low→high]
3. The reprieve loop traverses [low→high], reprieving lower priority pods first
This causes higher priority pods to be more likely evicted as victims, which contradicts the expected priority-based preemption behavior.
### Steps to reproduce the issue
This is a code logic bug that can be reproduced through code analysis. To observe the behavior in practice:
1. Deploy a cluster with Volcano scheduler
2. Create multiple Jobs with different priorities (use `priorityClassName`)
3. Fill up node resources so that a new Job triggers preemption
4. Observe the victim selection in preempt action:
- Set log level to 3 or higher: `-v=3`
- Check logs for `SelectVictimsOnNode` and `reprievePod` output
5. Observe that lower priority pods are reprieved first, while higher priority pods are more likely to be kept as victims
**Code analysis steps:**
1. Review `pkg/scheduler/actions/preempt/preempt.go`, function `SelectVictimsOnNode` (L682-816)
2. Check the sorting logic at L739-741
3. Check `BuildVictimsPriorityQueue` in `pkg/scheduler/framework/session_plugins.go` (L1090-1115)
4. Notice the `!` negation in priority comparison at L1098 and L1109
5. The `Pop()` order is opposite to `sort.Slice` order
### Describe the results you received and expected
**Received:**
When reprieving potential victims, the code traverses `potentialVictims` in [low→high] priority order. Lower priority pods are reprieved (spared) first, and higher priority
pods are more likely to be kept as victims.
**Expected:**
According to the code comment at L739-741:
> "Sort potentialVictims by pod priority from high to low, which ensures to reprieve higher priority pods first."
Higher priority pods should be reprieved first. When resources allow some victims to be spared, higher priority pods should be spared before lower priority ones.
**Example:**
Assume we have 3 potential victims: Pod A (high priority), Pod B (medium priority), Pod C (low priority).
Current behavior:
- Reprieve order: C → B → A (low to high)
- If only 1 pod can be spared, Pod C (low priority) is spared
Expected behavior:
- Reprieve order: A → B → C (high to low)
- If only 1 pod can be spared, Pod A (high priority) is spared
### What version of Volcano are you using?
master branch (commit: 499546a81)
### Any other relevant information
**Relevant code locations:**
1. `pkg/scheduler/actions/preempt/preempt.go`:
- `SelectVictimsOnNode` function (L682-816)
- Victim sorting at L739-741
- Reprieve loop at L804-809
2. `pkg/scheduler/framework/session_plugins.go`:
- `BuildVictimsPriorityQueue` function (L1090-1115)
- Note the `!` negation at L1098: `!ssn.TaskOrderFn(l, r)`
- Note the `!` negation at L1109: `!ssn.JobOrderFn(lvJob, rvJob)`
**Suggested fix:**
Reverse `potentialVictims` before the reprieve loop at L804:
```go
// Reverse potentialVictims to reprieve higher priority pods first
for i, j := 0, len(potentialVictims)-1; i < j; i, j = i+1, j-1 {
potentialVictims[i], potentialVictims[j] = potentialVictims[j], potentialVictims[i]
}
// Now we try to reprieve non-violating victims.
for _, p := range potentialVictims {
if _, err := reprievePod(p); err != nil {
return nil, api.AsStatus(err)
}
}
Note: This issue doesn't require specific Kubernetes version or OS configuration to reproduce, as it's a code logic bug visible through source code analysis.
1 条评论