add priority class to wl none -> some #8480

andrewseif · 2026-01-08T23:53:35Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Adding a priority class to a workload (none -> some) is now reconciled, and behaving as explained/expected in the ticket

Which issue(s) this PR fixes:

Fixes #8320

Special notes for your reviewer:

Does this PR introduce a user-facing change?

no

NONE

netlify · 2026-01-08T23:53:41Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`1e72145`
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-kueue/deploys/69626cc7b22d540008161a09

k8s-ci-robot · 2026-01-08T23:53:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: andrewseif
Once this PR has been reviewed and has the lgtm label, please assign tenzen-y for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

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 · 2026-01-08T23:53:45Z

Hi @andrewseif. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

mbobrovskyi · 2026-01-09T06:52:13Z

/ok-to-test

mbobrovskyi · 2026-01-09T06:58:11Z

@andrewseif could you please rebase PR?

mbobrovskyi · 2026-01-09T07:00:22Z

pkg/controller/jobframework/validation.go

 }

 func ValidateUpdateForWorkloadPriorityClassName(isSuspended bool, oldObj, newObj client.Object) field.ErrorList {
+	// Cannot ADD a priority class to a NON-suspended (running) workload && wpc is empty


If we want to support adding a priority class, I think we should allow it.

as far as I understand the issue mentioned suspended workloads, this piece of code is only affecting running on.

If we change this if condition, it means we allow priority change to running workloads (if I get the code right?)

Ah, I see, you right, we want to fix only suspended workloads.

mbobrovskyi · 2026-01-09T08:22:22Z

pkg/controller/core/workload_controller.go

+	oldName := workload.PriorityClassName(old)
+	newName := workload.PriorityClassName(new)
+
+	return newName != "" && oldName != newName


You are checking only name here. What about priority?

in this code block we check that the new wl has a wpc

if !workload.IsWorkloadPriorityClass(new) { return false }
and that's what we care about since we want to allow none wpc -> some wpc

mbobrovskyi · 2026-01-09T09:06:05Z

pkg/controller/core/workload_controller.go

-		return true
-	}
-	// Check if priority value changed (for WorkloadPriorityClass value updates).
-	return priority.Priority(old) != priority.Priority(new)


What about this?

I think this is more restrictive? its checking (as far as I understand) the priority itself (low or high...etc), but I don't think it would work in a none ->some scenario?

in my case we are checking if old not equal new, and if new isn't empty. so no (some -> none scenario).

Please don't remove this logic.

I reverted the changes to this logic, but removed the culprit which is checking old prio, please check and tell me if that's ok now 😄

pkg/controller/core/workload_controller.go

Co-authored-by: Mykhailo Bobrovskyi <[email protected]>

mbobrovskyi · 2026-01-10T15:04:53Z

pkg/controller/core/workload_controller.go

+	// Check that old wl priority class is of type kueue.WorkloadPriorityClass.
+	if workload.IsPodPriorityClass(old) {
 		return false
 	}


So now this no need, right?

for one sec, I thought that your suggestion removed it, my bad, fixed

mbobrovskyi · 2026-01-10T15:10:41Z

pkg/controller/core/workload_controller_test.go

+				Obj(),
+			wantChanged: false,
+		},
+		"PodPriorityClass added (none -> some) - should not trigger": {


It would also be great to add a test case where we change the priority value. It should work only for WorkloadPriorityClass.

this got me thinking, is there a case where Kind changed from PodPriority to WorkloadPriority but uses same name? do we need to cover that?

Yeah, it would be great to cover this case as well.

pkg/workload/workload.go

Co-authored-by: Mykhailo Bobrovskyi <[email protected]>

mbobrovskyi · 2026-01-10T15:24:49Z

test/integration/singlecluster/scheduler/preemption_test.go

+			util.MustCreate(ctx, k8sClient, normalWl)
+			util.ExpectWorkloadsToHaveQuotaReservation(ctx, k8sClient, cq.Name, normalWl)
+
+			ginkgo.By("Creating a suspended workload WITHOUT priority class")


I think you mean deactivated workload, right?

Can we add test case here instead? And also apply case from issue.

I think you mean deactivated workload, right?

if I am guessing correctly, in this context you mean:
if I am using .spec.active = true it means deactivated.
if I am using .spec.suspended = true it means suspended?

if so then its deactivated.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. labels Jan 8, 2026

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 8, 2026

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 8, 2026

k8s-ci-robot requested review from kannon92 and mimowo January 8, 2026 23:53

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 8, 2026

mbobrovskyi reviewed Jan 9, 2026

View reviewed changes

andrewseif force-pushed the add-wpc-to-suspended-wl branch from fa60457 to f719eea Compare January 9, 2026 08:05

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 9, 2026

mbobrovskyi reviewed Jan 9, 2026

View reviewed changes

pkg/controller/core/workload_controller.go Outdated Show resolved Hide resolved

andrewseif added 7 commits January 10, 2026 14:16

bugfix and add unit test

52a3e03

add e2e testing

960d7d3

update e2e testing

a2a18f9

update function name

4c6a28a

fix function name in testing

dd27de7

revert to old logic but removing empty string for old

076aa7a

add validation for old wl

ca335de

andrewseif force-pushed the add-wpc-to-suspended-wl branch from bd900af to ca335de Compare January 10, 2026 12:19

fix linter issue

79d6a4b

Update pkg/controller/core/workload_controller.go

116e539

Co-authored-by: Mykhailo Bobrovskyi <[email protected]>

mbobrovskyi reviewed Jan 10, 2026

View reviewed changes

remove redundant validation

03674a6

mbobrovskyi reviewed Jan 10, 2026

View reviewed changes

pkg/workload/workload.go Outdated Show resolved Hide resolved

Update pkg/workload/workload.go

1e72145

Co-authored-by: Mykhailo Bobrovskyi <[email protected]>

mbobrovskyi reviewed Jan 10, 2026

View reviewed changes

add priority class to wl none -> some #8480

Are you sure you want to change the base?

add priority class to wl none -> some #8480

Conversation

andrewseif commented Jan 8, 2026

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 Jan 8, 2026 • 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!

k8s-ci-robot commented Jan 8, 2026

Uh oh!

k8s-ci-robot commented Jan 8, 2026

Uh oh!

mbobrovskyi commented Jan 9, 2026

Uh oh!

mbobrovskyi commented Jan 9, 2026

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!

Choose a reason for hiding this comment

Uh oh!

andrewseif Jan 9, 2026 • 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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

netlify bot commented Jan 8, 2026 •

edited

Loading

andrewseif Jan 9, 2026 •

edited

Loading