Skip to content

Commit

Permalink
Fix detailed preemption reason bug
Browse files Browse the repository at this point in the history
* If allowBorrowingBelowPriority is nil, then the reason should stay as InCohortReclamationReason
* Add reason assertions to TestPreemption
  • Loading branch information
vladikkuzn committed Jul 17, 2024
1 parent 9f407b1 commit b0110e5
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 73 deletions.
36 changes: 19 additions & 17 deletions pkg/scheduler/preemption/preemption.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ type Preemptor struct {
fsStrategies []fsStrategy

// stubs
applyPreemption func(context.Context, *kueue.Workload, string, string) error
applyPreemption func(ctx context.Context, w *kueue.Workload, reason, message string) error
}

func New(cl client.Client, workloadOrdering workload.Ordering, recorder record.EventRecorder, fs config.FairSharing) *Preemptor {
Expand Down Expand Up @@ -247,22 +247,24 @@ func minimalPreemptions(log logr.Logger, wlReq resources.FlavorResourceQuantitie
}
if !sameCq {
reason = InCohortReclamationReason
if allowBorrowingBelowPriority != nil && priority.Priority(candWl.Obj) >= *allowBorrowingBelowPriority {
// We set allowBorrowing=false if there is a candidate with priority
// exceeding allowBorrowingBelowPriority added to targets.
//
// We need to be careful mutating allowBorrowing. We rely on the
// fact that once there is a candidate exceeding the priority added
// to targets, then at least one such candidate is present in the
// final set of targets (after the second phase of the function).
//
// This is true, because the candidates are ordered according
// to priorities (from lowest to highest, using candidatesOrdering),
// and the last added target is not removed in the second phase of
// the function.
allowBorrowing = false
} else {
reason = InCohortReclaimWhileBorrowingReason
if allowBorrowingBelowPriority != nil {
if priority.Priority(candWl.Obj) >= *allowBorrowingBelowPriority {
// We set allowBorrowing=false if there is a candidate with priority
// exceeding allowBorrowingBelowPriority added to targets.
//
// We need to be careful mutating allowBorrowing. We rely on the
// fact that once there is a candidate exceeding the priority added
// to targets, then at least one such candidate is present in the
// final set of targets (after the second phase of the function).
//
// This is true, because the candidates are ordered according
// to priorities (from lowest to highest, using candidatesOrdering),
// and the last added target is not removed in the second phase of
// the function.
allowBorrowing = false
} else {
reason = InCohortReclaimWhileBorrowingReason
}
}
}
snapshot.RemoveWorkload(candWl)
Expand Down
Loading

0 comments on commit b0110e5

Please sign in to comment.