-
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
Distinguish preemption reasons #1942
Distinguish preemption reasons #1942
Conversation
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
130989a
to
03a9385
Compare
535e7cb
to
4fcde5a
Compare
4a38c6d
to
ca51d57
Compare
/assign @alculquicondor @astefanutti @tenzen-y |
ca51d57
to
4e6d124
Compare
/release-note-edit
Maybe also include the names and how they might be expanded in the future? |
6541c10
to
727ae15
Compare
727ae15
to
e09a6f6
Compare
Type: kueue.WorkloadPreempted, | ||
Status: metav1.ConditionFalse, | ||
Reason: "InClusterQueue", | ||
Message: fmt.Sprintf("Preempted to accommodate a workload (UID: %s) in the ClusterQueue", alphaMidWl.UID), |
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.
uhm... this is rather confusing, because now the workload isn't preempted.
Should we add a prefix? Like: Previously:
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.
When we see only the message, it may bring any confusion. But doesn't avoid confusion since we have a Status: False
field?
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.
Ok, so I only flipped status for consistency with how it was done for the "Evicted" condition.
However, I think ideally, the condition should indicate the reason for the "last" transition.
The API comment, Also, an example in the Job controller when transitioning from True to False (see here.
In this case of transitoning Evicted / Preempted conditions from True->False
, the reason for this transition is that it has now QuotaReserved. OTOH, it might be handy to still keep the information why the workload was preempted in the first place.
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.
Proposed a change along the lines in the last commit. PTAL.
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.
When we see only the message, it may bring any confusion. But doesn't avoid confusion since we have a
Status: False
field?
I have already received tickets from customers that missed the status: false line. So better avoid the confusion.
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 have already received tickets from customers that missed the status: false line. So better avoid the confusion.
I see. End-user's feedback is important. Thank you for sharing it with me.
/lgtm
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.
LGTM
5eeaa40
to
6734c2a
Compare
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.
/lgtm
/approve
@@ -397,8 +397,27 @@ func SetQuotaReservation(w *kueue.Workload, admission *kueue.Admission) { | |||
//reset Evicted condition if present. | |||
if evictedCond := apimeta.FindStatusCondition(w.Status.Conditions, kueue.WorkloadEvicted); evictedCond != nil { | |||
evictedCond.Status = metav1.ConditionFalse | |||
evictedCond.Reason = "QuotaReserved" | |||
evictedCond.Message = "Previously: " + evictedCond.Message | |||
evictedCond.LastTransitionTime = metav1.Now() |
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.
FYI @vladikkuzn, given that you are working on #1939
LGTM label has been added. Git tree hash: 1c5d2f5a818c1f699c4c742bb7b8d8325d3c55f9
|
[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 |
* Distinguish the preemption reasons * Review comments * Review comments2 * Make the conditions more accurate
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #1874
Special notes for your reviewer:
Does this PR introduce a user-facing change?