feat: include more detailed preemption information in flavor attempt events #8024

mykysha · 2025-12-01T15:17:17Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Display more detailed information on preemption in the flavor assignment attempt block in events
Follow-up for #7646

Which issue(s) this PR fixes:

Part of #7137

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Observability: Add more details (the preemptionMode) to the QuotaReserved condition message,
and the related event, about the skipped flavors which were considered for preemption. 
Before: "Quota reserved in ClusterQueue preempt-attempts-cq, wait time since queued was 9223372037s; Flavors considered: main: on-demand(Preempt;insufficient unused quota for cpu in flavor on-demand, 1 more needed)"
After: "Quota reserved in ClusterQueue preempt-attempts-cq, wait time since queued was 9223372037s; Flavors considered: main: on-demand(preemptionMode=Preempt;insufficient unused quota for cpu in flavor on-demand, 1 more needed)"

netlify · 2025-12-01T15:17:24Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`18af99b`
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-kueue/deploys/6932e8394619af00080b6a62

mykysha · 2025-12-01T15:46:41Z

/cc @mbobrovskyi
PTAL

mimowo · 2025-12-02T09:30:54Z

/assign @pajakd
PTAL

mbobrovskyi

/lgtm
Thank you!

k8s-ci-robot · 2025-12-02T10:34:58Z

LGTM label has been added.

Git tree hash: 4d82f3da79961e33f3c41b42fdb963979e244ae9

pkg/scheduler/flavorassigner/flavor_assigner_attempts.go

pajakd · 2025-12-03T12:21:07Z

/lgtm

k8s-ci-robot · 2025-12-03T12:21:14Z

LGTM label has been added.

Git tree hash: 2d97397e46924187e0c4372262c880809617157e

mykysha · 2025-12-05T08:25:51Z

cc: @mimowo

mimowo · 2025-12-05T09:56:09Z

@mykysha is this correct? In particular the example "Before" ?
/release-note-edit

Observability: Add more details (the preemptionMode) to the QuotaReserved condition message,
and the related event, about the skipped flavors which were considered for preemption. 
Before: "Quota reserved in ClusterQueue preempt-attempts-cq, wait time since queued was 9223372037s; Flavors considered: main: on-demand(Preempt;insufficient unused quota for cpu in flavor on-demand, 1 more needed)"
After: "Quota reserved in ClusterQueue preempt-attempts-cq, wait time since queued was 9223372037s; Flavors considered: main: on-demand(preemptionMode=Preempt;insufficient unused quota for cpu in flavor on-demand, 1 more needed)"

mimowo · 2025-12-05T09:59:17Z

pkg/scheduler/flavorassigner/flavorassigner.go

+	case reclaim:
+		return ptr.To(preemptioncommon.Reclaim)
+	default:
+		return nil // preemptionMode has non-preemption values that do not address preemption.


I don't understand the comment TBH. Is it to say "there are no other options"?

If so I would follow the pattern as above in this function (it is confusing to see two functions side-by-side one returning panic one nil): https://github.com/kubernetes-sigs/kueue/pull/8024/files#diff-0870ec2c0ea330a7d087a6ef599905bf9b7164a0dd889d1b00ba17c31a1bfd05R429

Unless I'm missing something which might be, but then please rephrase the comment to make it more clear.

Changing from preemptioncommon.PreemptionPossibility to preemptionMode is safe, because all the PreemptionPossibility fields have their counterparts, but changing back not so as we introduce non-preemption related fit and noFit to the preemptionMode. I can change return nil to a panic message, but I'm not sure that this is needed.
Perhaps something like

case noFit, fit: return nil default: panic(fmt.Sprintf("illegal PreemptionPossibility: %d", preemptionPossibility))

would be better?

I see, so we have preemptionPossibility -> preemptionMode -> preemptionPossibility that is weird. Would it be possible to eliminate the preemptionMode entirely?

It is not a question about this PR, but I want to get your view on that since you researched the code recently.

For this PR I would go with, I assume you meant preemptionMode rather than possibilty here:

default: panic(fmt.Sprintf("unexpected preemptionMode: %d", mode))

Ok, or alternatively stay with

case noFit, fit: return nil default: panic(fmt.Sprintf("illegal preemptionMode: %d", preemptionMode))

but remove this guard chack https://github.com/kubernetes-sigs/kueue/pull/8024/files#diff-0ea412b6476f3b5faaa7e3ad1fb3ded2e8c568df3c84474c0c576df47223c56cR72-R74

Keeping the guards at both levels makes some code dead.

I think preemptionMode can be modified to contain both the FlavorAssignmentMode as well as the PreemptionPossibility and we would not need to have so many conversions. Another possibility is to add missing values to the FlavorAssignmentMode, but this change would be more intrusive.

sgtm, as mentioned I'm good to keep it for future depending how the code evolves

Removed the guard and went with the case nofit, fit: approach. Making changes to the preemptionMode would potentially remove the conversion altogether, but that could be addressed in a separate PR

mykysha · 2025-12-05T10:00:37Z

Before is definitely correct, but we can also change after to better show that Reclaim and NoCandidates are added, perhaps like

After: "Quota reserved in ClusterQueue preempt-attempts-cq, wait time since queued was 9223372037s; Flavors considered: main: on-demand(preemptionMode=Preempt/Reclaim/NoCandidates;insufficient unused quota for cpu in flavor on-demand, 1 more needed)"

?

mimowo · 2025-12-05T10:26:01Z

sgtm, updating:

Observability: Add more details (the preemptionMode) to the QuotaReserved condition message,
and the related event, about the skipped flavors which were considered for preemption. 
Before: "Quota reserved in ClusterQueue preempt-attempts-cq, wait time since queued was 9223372037s; Flavors considered: main: on-demand(Preempt;insufficient unused quota for cpu in flavor on-demand, 1 more needed)"
After: "Quota reserved in ClusterQueue preempt-attempts-cq, wait time since queued was 9223372037s; Flavors considered: main: on-demand(preemptionMode=Preempt/Reclaim/NoCandidates;insufficient unused quota for cpu in flavor on-demand, 1 more needed)"

mimowo · 2025-12-05T10:29:57Z

pkg/scheduler/flavorassigner/flavor_assigner_attempts.go


+			existingPreemption := existing.PreemptionPossibility
+			if at.PreemptionPossibility != nil && (existingPreemption == nil || *at.PreemptionPossibility < *existingPreemption) {
+				existing.PreemptionPossibility = at.PreemptionPossibility


I think this is error prone and confusing to introduce the variable existingPreemption, and use it only for "read" but for "update" using the underlying existing variable.

Either don't introduce the new variable just update existing, or update the local copy. Generally mutating the local copy than the original pointer is safer, but this code already updates the underlying copy, just in a tricky fashion.

Changed here, now only the copy gets updated

events

mimowo · 2025-12-05T14:18:51Z

/lgtm
/approve

k8s-ci-robot · 2025-12-05T14:18:59Z

LGTM label has been added.

Git tree hash: e227c17e8643c0e5c4c38583bafa564f1e66378f

k8s-ci-robot · 2025-12-05T14:19:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mbobrovskyi, mimowo, mykysha

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~~ [mimowo]

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

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Dec 1, 2025

k8s-ci-robot requested review from gabesaba and tenzen-y December 1, 2025 15:17

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 1, 2025

k8s-ci-robot requested a review from mbobrovskyi December 1, 2025 15:46

mykysha force-pushed the feat/preemtion-flavor-attempts branch from a8489ac to c9777aa Compare December 2, 2025 09:18

k8s-ci-robot assigned pajakd Dec 2, 2025

mykysha force-pushed the feat/preemtion-flavor-attempts branch 2 times, most recently from 3232203 to 8120d29 Compare December 2, 2025 10:26

mbobrovskyi approved these changes Dec 2, 2025

View reviewed changes

k8s-ci-robot assigned mbobrovskyi Dec 2, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 2, 2025

pajakd reviewed Dec 2, 2025

View reviewed changes

pkg/scheduler/flavorassigner/flavor_assigner_attempts.go Outdated Show resolved Hide resolved

mykysha force-pushed the feat/preemtion-flavor-attempts branch from 8120d29 to 9bb800c Compare December 2, 2025 14:38

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 2, 2025

k8s-ci-robot requested review from mbobrovskyi and pajakd December 2, 2025 14:38

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 3, 2025

mimowo reviewed Dec 5, 2025

View reviewed changes

feat: include more detailed preemption information in flavor attempt

18af99b

events

mykysha force-pushed the feat/preemtion-flavor-attempts branch from 9bb800c to 18af99b Compare December 5, 2025 14:12

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 5, 2025

k8s-ci-robot assigned mimowo Dec 5, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 5, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 5, 2025

k8s-ci-robot merged commit 0b3b6aa into kubernetes-sigs:main Dec 5, 2025
28 checks passed

k8s-ci-robot added this to the v0.16 milestone Dec 5, 2025

mykysha deleted the feat/preemtion-flavor-attempts branch December 5, 2025 18:11

feat: include more detailed preemption information in flavor attempt events #8024

feat: include more detailed preemption information in flavor attempt events #8024

Conversation

mykysha commented Dec 1, 2025 • edited by k8s-ci-robot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

netlify bot commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Uh oh!

mykysha commented Dec 1, 2025

Uh oh!

mimowo commented Dec 2, 2025

Uh oh!

mbobrovskyi left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Dec 2, 2025

Uh oh!

Uh oh!

pajakd commented Dec 3, 2025

Uh oh!

k8s-ci-robot commented Dec 3, 2025

Uh oh!

mykysha commented Dec 5, 2025

Uh oh!

mimowo commented Dec 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mykysha commented Dec 5, 2025

Uh oh!

mimowo commented Dec 5, 2025

Uh oh!

mimowo Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mimowo commented Dec 5, 2025

Uh oh!

k8s-ci-robot commented Dec 5, 2025

Uh oh!

k8s-ci-robot commented Dec 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mykysha commented Dec 1, 2025 •

edited by k8s-ci-robot

Loading

netlify bot commented Dec 1, 2025 •

edited

Loading

mimowo Dec 5, 2025 •

edited

Loading