-
Notifications
You must be signed in to change notification settings - Fork 228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support Preemption while borrowing #1397
Support Preemption while borrowing #1397
Conversation
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
d318009
to
fcba607
Compare
1def45b
to
73626b0
Compare
23332e0
to
c54df4e
Compare
/test pull-kueue-test-integration-main |
67a8bce
to
9eb1cd1
Compare
287cf0d
to
e04ed60
Compare
e04ed60
to
f867bbe
Compare
7915368
to
f4d79cf
Compare
f4d79cf
to
19dfb93
Compare
if cq.Preemption.BorrowWithinCohort != nil && cq.Preemption.BorrowWithinCohort.Policy != kueue.BorrowWithinCohortPolicyNever { | ||
// when preemption with borrowing is enabled, we can succeeded admitting the | ||
// workload if preemption is used. | ||
if (rQuota.BorrowingLimit == nil || rQuota.BorrowingLimit != nil && val <= rQuota.Nominal+*rQuota.BorrowingLimit) && val <= cohortAvailable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (rQuota.BorrowingLimit == nil || rQuota.BorrowingLimit != nil && val <= rQuota.Nominal+*rQuota.BorrowingLimit) && val <= cohortAvailable { | |
if (rQuota.BorrowingLimit == nil || val <= rQuota.Nominal+*rQuota.BorrowingLimit) && val <= cohortAvailable { |
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm no longer sure if this works.
What if, in a previous iteration, we were already borrowing? Then workloadFits
would return false and break this loop. So far so good.
However, the next loop (check if any of the workloads can be added back) would be prevented from borrowing, even though it was ok to borrow. As a result, we won't be able to add back any workload, possibly leading to preempting more workloads than necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if, in a previous iteration, we were already borrowing? Then workloadFits would return false and break this loop. So far so good.
The workloadFits
breaks when returning true
(not false), snippet from below:
if workloadFits(wlReq, cq, allowBorrowing) {
fits = true
break
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohhhh, right.
So the mechanics roughly like this:
- allowBorrowing=true, workload doesn't fit,
- we pass the threshold, workload still doesn't fit, we can continue deleting workloads, but now we cannot allow borrowing.
- Eventually, the workload fits. We are already deleting workloads past the threshold, so if we put workloads back, we cannot allow borrowing anyway.
Ack.
19451bf
to
3d2c282
Compare
/lgtm |
LGTM label has been added. Git tree hash: 8c988c2cf685a17850d5e3d9efa0cc27f8a24529
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, mimowo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/kind api-change |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Part of #1337
Special notes for your reviewer:
Does this PR introduce a user-facing change?