Distinguish preemption reasons #1942

mimowo · 2024-04-03T17:23:22Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

A new condition with type Preempted allows to distinguish different reasons for the preemption to happen

netlify · 2024-04-03T17:23:37Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`3a4fc2d`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/6616614b2f909d000869a295

mimowo · 2024-04-04T17:42:47Z

/assign @alculquicondor @astefanutti @tenzen-y

alculquicondor · 2024-04-04T18:13:47Z

/release-note-edit

A new condition with type Preempted allows to distinguish different reasons for the preemption to happen

Maybe also include the names and how they might be expanded in the future?

pkg/scheduler/preemption/preemption.go

apis/kueue/v1beta1/workload_types.go

pkg/workload/workload.go

test/integration/scheduler/preemption_test.go

pkg/scheduler/preemption/preemption.go

test/integration/scheduler/preemption_test.go

apis/kueue/v1beta1/workload_types.go

alculquicondor · 2024-04-09T17:05:25Z

test/integration/scheduler/preemption_test.go

+						Type:    kueue.WorkloadPreempted,
+						Status:  metav1.ConditionFalse,
+						Reason:  "InClusterQueue",
+						Message: fmt.Sprintf("Preempted to accommodate a workload (UID: %s) in the ClusterQueue", alphaMidWl.UID),


uhm... this is rather confusing, because now the workload isn't preempted.

Should we add a prefix? Like: Previously:

When we see only the message, it may bring any confusion. But doesn't avoid confusion since we have a Status: False field?

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.

Proposed a change along the lines in the last commit. PTAL.

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.

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

tenzen-y

LGTM

alculquicondor

/lgtm
/approve

alculquicondor · 2024-04-10T15:02:17Z

pkg/workload/workload.go

@@ -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()


FYI @vladikkuzn, given that you are working on #1939

k8s-ci-robot · 2024-04-10T15:02:33Z

LGTM label has been added.

Git tree hash: 1c5d2f5a818c1f699c4c742bb7b8d8325d3c55f9

k8s-ci-robot · 2024-04-10T15:02:35Z

[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:

~~OWNERS~~ [alculquicondor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* Distinguish the preemption reasons * Review comments * Review comments2 * Make the conditions more accurate

k8s-ci-robot requested review from alculquicondor and kerthcet April 3, 2024 17:23

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 3, 2024

mimowo force-pushed the distinguish-preemption-reasons branch from 130989a to 03a9385 Compare April 4, 2024 08:04

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 4, 2024

mimowo force-pushed the distinguish-preemption-reasons branch 2 times, most recently from 535e7cb to 4fcde5a Compare April 4, 2024 12:52

mimowo mentioned this pull request Apr 4, 2024

Distinguish between preemption within ClusterQueue, reclaim within Cohort and preemption with borrowing #1874

Closed

3 tasks

mimowo force-pushed the distinguish-preemption-reasons branch 2 times, most recently from 4a38c6d to ca51d57 Compare April 4, 2024 17:41

mimowo changed the title ~~WIP: Distinguish preemption reasons~~ Distinguish preemption reasons Apr 4, 2024

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 4, 2024

k8s-ci-robot assigned alculquicondor, astefanutti and tenzen-y Apr 4, 2024

mimowo force-pushed the distinguish-preemption-reasons branch from ca51d57 to 4e6d124 Compare April 4, 2024 17:44

alculquicondor reviewed Apr 4, 2024

View reviewed changes

tenzen-y reviewed Apr 5, 2024

View reviewed changes

pkg/scheduler/preemption/preemption.go Outdated Show resolved Hide resolved

test/integration/scheduler/preemption_test.go Outdated Show resolved Hide resolved

Distinguish the preemption reasons

caca6b2

mimowo force-pushed the distinguish-preemption-reasons branch from 6541c10 to 727ae15 Compare April 8, 2024 07:13

Review comments

e09a6f6

mimowo force-pushed the distinguish-preemption-reasons branch from 727ae15 to e09a6f6 Compare April 8, 2024 07:20

alculquicondor reviewed Apr 9, 2024

View reviewed changes

tenzen-y reviewed Apr 10, 2024

View reviewed changes

Review comments2

6734c2a

mimowo force-pushed the distinguish-preemption-reasons branch from 5eeaa40 to 6734c2a Compare April 10, 2024 09:33

Make the conditions more accurate

3a4fc2d

alculquicondor reviewed Apr 10, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 10, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 10, 2024

k8s-ci-robot merged commit 4905cfd into kubernetes-sigs:main Apr 10, 2024
14 checks passed

k8s-ci-robot added this to the v0.7 milestone Apr 10, 2024

vladikkuzn mentioned this pull request Apr 11, 2024

Set ObservedGeneration in conditions #1939

Merged

vsoch pushed a commit to researchapps/kueue that referenced this pull request Apr 18, 2024

Distinguish preemption reasons (kubernetes-sigs#1942)

7b82947

* Distinguish the preemption reasons * Review comments * Review comments2 * Make the conditions more accurate

vladikkuzn mentioned this pull request Apr 26, 2024

REQUEST: New membership for @vladikkuzn kubernetes/org#4911

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Distinguish preemption reasons #1942

Distinguish preemption reasons #1942

mimowo commented Apr 3, 2024 •

edited by k8s-ci-robot

netlify bot commented Apr 3, 2024 •

edited

mimowo commented Apr 4, 2024

alculquicondor commented Apr 4, 2024

alculquicondor Apr 9, 2024

tenzen-y Apr 10, 2024

mimowo Apr 10, 2024

mimowo Apr 10, 2024

alculquicondor Apr 10, 2024

tenzen-y Apr 10, 2024

tenzen-y left a comment

alculquicondor left a comment

alculquicondor Apr 10, 2024

k8s-ci-robot commented Apr 10, 2024

k8s-ci-robot commented Apr 10, 2024

Distinguish preemption reasons #1942

Distinguish preemption reasons #1942

Conversation

mimowo commented Apr 3, 2024 • edited by k8s-ci-robot

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

netlify bot commented Apr 3, 2024 • edited

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

mimowo commented Apr 4, 2024

alculquicondor commented Apr 4, 2024

alculquicondor Apr 9, 2024

Choose a reason for hiding this comment

tenzen-y Apr 10, 2024

Choose a reason for hiding this comment

mimowo Apr 10, 2024

Choose a reason for hiding this comment

mimowo Apr 10, 2024

Choose a reason for hiding this comment

alculquicondor Apr 10, 2024

Choose a reason for hiding this comment

tenzen-y Apr 10, 2024

Choose a reason for hiding this comment

tenzen-y left a comment

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

alculquicondor Apr 10, 2024

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 10, 2024

k8s-ci-robot commented Apr 10, 2024

mimowo commented Apr 3, 2024 •

edited by k8s-ci-robot

netlify bot commented Apr 3, 2024 •

edited