Distinguish between Preemption due to reclamation and fair sharing #2411
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
the
cncf-cla: yes
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
|
/assign @vladikkuzn @trasc |
vladikkuzn
commented
Jun 14, 2024
vladikkuzn
force-pushed
the
detailed-preemption-condition
branch
from
45eee46
to
abb03fe
Compare
vladikkuzn
commented
Jun 17, 2024
vladikkuzn
force-pushed
the
detailed-preemption-condition
branch
from
abb03fe
to
1223f92
Compare
vladikkuzn
commented
Jun 19, 2024
vladikkuzn
commented
Jun 19, 2024
vladikkuzn
force-pushed
the
detailed-preemption-condition
branch
from
1223f92
to
5e7d6b6
Compare
k8s-ci-robot
added
size/M
|
/cc @alculquicondor |
trasc
reviewed
Jun 19, 2024
|
/assign @gabesaba |
* Rewrite to Targets * Cover CohortFairSharing
vladikkuzn
force-pushed
the
detailed-preemption-condition
branch
from
3d8d403
to
2f68cca
Compare
k8s-ci-robot
removed
the
needs-rebase
vladikkuzn
force-pushed
the
detailed-preemption-condition
branch
from
2f68cca
to
d15223d
Compare
* Dedicated GetWorkloadReferences function * Extract humanReadablePreemptionReasons * !strategy && belowThreshold -> InCohortReclaimWhileBorrowingReason * ExpectPreemptedCondition test util
vladikkuzn
force-pushed
the
detailed-preemption-condition
branch
from
d15223d
to
cad0de8
Compare
k8s-ci-robot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, vladikkuzn 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 |
k8s-ci-robot
added
the
approved
tenzen-y
reviewed
Jul 9, 2024
* Rename WL -> WorkloadInfo * Move getWorkloadReferences to logging
vladikkuzn
force-pushed
the
detailed-preemption-condition
branch
from
5977741
to
7327a1e
Compare
|
/retest |
* Use slice in getWorkloadReferences
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 7b96a661745f64e63ec8b9cab6360cf63ea769ae
|
|
/retest |
|
/unhold |
k8s-ci-robot
removed
the
do-not-merge/hold
| log.V(3).Info("Preempted", "targetWorkload", klog.KObj(target.Obj), "reason", reason, "message", message) | ||
| p.recorder.Eventf(target.Obj, corev1.EventTypeNormal, "Preempted", message) | ||
| metrics.ReportEvictedWorkloads(target.ClusterQueue, kueue.WorkloadEvictedByPreemption) | ||
| log.V(3).Info("Preempted", "targetWorkload", klog.KObj(target.WorkloadInfo.Obj), "reason", target.Reason, "message", message) |
There was a problem hiding this comment.
Can you also please add targetClusterQueue?
There was a problem hiding this comment.
Added in #2538
WDYT should I also add preemptor's cluster queue?
There was a problem hiding this comment.
The preemptor's information is already part of the log (it comes from the scheduler.go file, which adds the fields to the contextual log).
| // the function. | ||
| allowBorrowing = false | ||
| } else { | ||
| reason = InCohortReclaimWhileBorrowingReason |
There was a problem hiding this comment.
There is a bug here.
If allowBorrowingBelowPriority is nil, then the reason should stay as InCohortReclamationReason.
There was a problem hiding this comment.
it should be more like
if allowBorrowingBelowPriority != nil {
if priority.Priority(candWl.Obj) >= *allowBorrowingBelowPriority {
allowBorrowing = false
} else {
reason = InCohortReclaimWhileBorrowingReason
}
}
|
It looks like we forgot to change the release notes to match the review changes. /release-note-edit |
kannon92
pushed a commit
to openshift-kannon92/kubernetes-sigs-kueue
that referenced
this pull request
Nov 19, 2024
…ubernetes-sigs#2411) * Distinguish between Preemption due to reclamation and fair sharing * Distinguish between Preemption due to reclamation and fair sharing * extract getPreemptionReason * borrowing in favour of reclamation * Distinguish between Preemption due to reclamation and fair sharing * Rewrite to Targets * Cover CohortFairSharing * Distinguish between Preemption due to reclamation and fair sharing * Dedicated GetWorkloadReferences function * Extract humanReadablePreemptionReasons * !strategy && belowThreshold -> InCohortReclaimWhileBorrowingReason * ExpectPreemptedCondition test util * Distinguish between Preemption due to reclamation and fair sharing * Rename WL -> WorkloadInfo * Move getWorkloadReferences to logging * Distinguish between Preemption due to reclamation and fair sharing * Use slice in getWorkloadReferences
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind feature
What this PR does / why we need it:
Introduces an additional reason in the Preemption condition to indicate that the preemption was due to fair sharing and not regular reclamation.
Which issue(s) this PR fixes:
Fixes #2404
Special notes for your reviewer:
Does this PR introduce a user-facing change?