Adding a priority class to a workload (none -> some)

pkg/controller/core/workload_controller.go

@@ -989,10 +989,16 @@ func (r *WorkloadReconciler) Update(e event.TypedUpdateEvent[*kueue.Workload]) b
 	return true
 }
 
+// removal is blocked by validation
 func workloadPriorityClassChanged(old, new *kueue.Workload) bool {
-	return workload.IsWorkloadPriorityClass(old) && workload.IsWorkloadPriorityClass(new) &&
-		workload.PriorityClassName(old) != "" && workload.PriorityClassName(new) != "" &&
-		workload.PriorityClassName(old) != workload.PriorityClassName(new)
+	if !workload.IsWorkloadPriorityClass(new) {
+		return false
+	}
+
+	oldName := workload.PriorityClassName(old)
+	newName := workload.PriorityClassName(new)
+
+	return newName != "" && oldName != newName
 }
 
 func (r *WorkloadReconciler) Generic(e event.TypedGenericEvent[*kueue.Workload]) bool {

pkg/controller/core/workload_controller_test.go

@@ -2734,3 +2734,77 @@ func TestReconcile(t *testing.T) {
 		}
 	}
 }
+
+// TestWorkloadPriorityClassChanged tests the workloadPriorityClassChanged function.
+// (none -> some) should be detected as a change and trigger reconciliation.
+func TestWorkloadPriorityClassChanged(t *testing.T) {
+	testCases := map[string]struct {
+		oldWorkload *kueue.Workload
+		newWorkload *kueue.Workload
+		wantChanged bool
+	}{
+		"no priority class on either workload": {
+			oldWorkload: utiltestingapi.MakeWorkload("wl", "ns").Obj(),
+			newWorkload: utiltestingapi.MakeWorkload("wl", "ns").Obj(),
+			wantChanged: false,
+		},
+		"same priority class on both workloads": {
+			oldWorkload: utiltestingapi.MakeWorkload("wl", "ns").
+				WorkloadPriorityClassRef("priority-1").
+				Obj(),
+			newWorkload: utiltestingapi.MakeWorkload("wl", "ns").
+				WorkloadPriorityClassRef("priority-1").
+				Obj(),
+			wantChanged: false,
+		},
+		"priority class changed from one to another": {
+			oldWorkload: utiltestingapi.MakeWorkload("wl", "ns").
+				WorkloadPriorityClassRef("priority-1").
+				Obj(),
+			newWorkload: utiltestingapi.MakeWorkload("wl", "ns").
+				WorkloadPriorityClassRef("priority-2").
+				Obj(),
+			wantChanged: true,
+		},
+		"priority class added (none -> some) - issue #8320": {
+			oldWorkload: utiltestingapi.MakeWorkload("wl", "ns").Obj(),
+			newWorkload: utiltestingapi.MakeWorkload("wl", "ns").
+				WorkloadPriorityClassRef("priority-1").
+				Obj(),
+			wantChanged: true,
+		},
+		"priority class removed (some -> none) - blocked by validation": {
+			oldWorkload: utiltestingapi.MakeWorkload("wl", "ns").
+				WorkloadPriorityClassRef("priority-1").
+				Obj(),
+			newWorkload: utiltestingapi.MakeWorkload("wl", "ns").Obj(),
+			// Removal is blocked by validation, so we don't need to detect it
+			wantChanged: false,
+		},
+		"PodPriorityClass (not WorkloadPriorityClass) changed - should not trigger": {
+			oldWorkload: utiltestingapi.MakeWorkload("wl", "ns").
+				PodPriorityClassRef("pod-priority-1").
+				Obj(),
+			newWorkload: utiltestingapi.MakeWorkload("wl", "ns").
+				PodPriorityClassRef("pod-priority-2").
+				Obj(),
+			wantChanged: false,
+		},
+		"PodPriorityClass added (none -> some) - should not trigger": {
+			oldWorkload: utiltestingapi.MakeWorkload("wl", "ns").Obj(),
+			newWorkload: utiltestingapi.MakeWorkload("wl", "ns").
+				PodPriorityClassRef("pod-priority-1").
+				Obj(),
+			wantChanged: false,
+		},
+	}
+
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			gotChanged := workloadPriorityClassChanged(tc.oldWorkload, tc.newWorkload)
+			if gotChanged != tc.wantChanged {
+				t.Errorf("workloadPriorityClassChanged() = %v, want %v", gotChanged, tc.wantChanged)
+			}
+		})
+	}
+}

pkg/controller/jobframework/validation.go

@@ -154,11 +154,13 @@ func validatedUpdateForEnabledWorkloadSlice(oldJob, newJob GenericJob) field.Err
 }
 
 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 !isSuspended && IsWorkloadPriorityClassNameEmpty(oldObj) {
 		if !IsWorkloadPriorityClassNameEmpty(newObj) {
 			return field.ErrorList{field.Invalid(workloadPriorityClassNamePath, WorkloadPriorityClassName(newObj), "WorkloadPriorityClass cannot be added to a non-suspended workload")}
 		}
 	}
+	// Cannot REMOVE a priority class from a workload (regardless of suspended/running)
 	if IsWorkloadPriorityClassNameEmpty(newObj) {
 		if !IsWorkloadPriorityClassNameEmpty(oldObj) {
 			return field.ErrorList{field.Invalid(workloadPriorityClassNamePath, WorkloadPriorityClassName(newObj), "WorkloadPriorityClass cannot be removed from a workload")}

pkg/workload/workload.go

@@ -1437,7 +1437,9 @@ func PriorityClassName(wl *kueue.Workload) string {
 
 func IsWorkloadPriorityClass(wl *kueue.Workload) bool {
 	return wl.Spec.PriorityClassRef != nil &&
+		// is wl priority class of kueue type
 		wl.Spec.PriorityClassRef.Kind == kueue.WorkloadPriorityClassKind &&
+		// is wl priority class in kueue group
 		wl.Spec.PriorityClassRef.Group == kueue.WorkloadPriorityClassGroup
 }
 

test/integration/singlecluster/scheduler/preemption_test.go

@@ -1132,4 +1132,89 @@ var _ = ginkgo.Describe("Preemption", func() {
 			util.ExpectWorkloadsToBeAdmitted(ctx, k8sClient, lowWl)
 		})
 	})
+
+	// Issue #8320: Adding a priority class to a workload (none -> some) is not reconciled
+	// https://github.com/kubernetes-sigs/kueue/issues/8320
+	//
+	// When a WorkloadPriorityClass is added to a suspended workload, the workload's
+	// spec.priority should be reconciled from the WorkloadPriorityClass value, and
+	// the workload should be re-queued with the new priority for scheduling.
+	ginkgo.Context("When adding a WorkloadPriorityClass to a suspended workload", func() {
+		var (
+			cq            *kueue.ClusterQueue
+			q             *kueue.LocalQueue
+			highPrioClass *kueue.WorkloadPriorityClass
+		)
+
+		ginkgo.BeforeEach(func() {
+			highPrioClass = utiltestingapi.MakeWorkloadPriorityClass("high-priority").
+				PriorityValue(highPriority).
+				Obj()
+			util.MustCreate(ctx, k8sClient, highPrioClass)
+
+			cq = utiltestingapi.MakeClusterQueue("cq").
+				ResourceGroup(*utiltestingapi.MakeFlavorQuotas("alpha").Resource(corev1.ResourceCPU, "2").Obj()).
+				Preemption(kueue.ClusterQueuePreemption{
+					WithinClusterQueue: kueue.PreemptionPolicyLowerPriority,
+				}).
+				Obj()
+			util.MustCreate(ctx, k8sClient, cq)
+
+			q = utiltestingapi.MakeLocalQueue("q", ns.Name).ClusterQueue(cq.Name).Obj()
+			util.MustCreate(ctx, k8sClient, q)
+		})
+
+		ginkgo.AfterEach(func() {
+			gomega.Expect(util.DeleteWorkloadsInNamespace(ctx, k8sClient, ns)).To(gomega.Succeed())
+			util.ExpectObjectToBeDeleted(ctx, k8sClient, cq, true)
+			util.ExpectObjectToBeDeleted(ctx, k8sClient, highPrioClass, true)
+		})
+
+		ginkgo.It("Should reconcile the priority when WorkloadPriorityClass is added to suspended workload", func() {
+			ginkgo.By("Creating a workload with low priority that gets admitted")
+			lowWl := utiltestingapi.MakeWorkload("low-wl", ns.Name).
+				Queue(kueue.LocalQueueName(q.Name)).
+				Priority(lowPriority).
+				Request(corev1.ResourceCPU, "2").
+				Obj()
+			util.MustCreate(ctx, k8sClient, lowWl)
+			util.ExpectWorkloadsToHaveQuotaReservation(ctx, k8sClient, cq.Name, lowWl)
+
+			ginkgo.By("Creating a workload WITHOUT a priority class (stays pending)")
+			noPrioWl := utiltestingapi.MakeWorkload("no-prio-wl", ns.Name).
+				Queue(kueue.LocalQueueName(q.Name)).
+				Request(corev1.ResourceCPU, "2").
+				Obj()
+			util.MustCreate(ctx, k8sClient, noPrioWl)
+			util.ExpectWorkloadsToBePending(ctx, k8sClient, noPrioWl)
+
+			ginkgo.By("Adding a WorkloadPriorityClass to the pending workload")
+			// Note: Due to CEL validation, we must set both priorityClassRef AND priority together.
+			// In real usage, the job controller sets both when creating/updating the workload.
+			// The bug in #8320 is that the workload controller's change detection
+			// (workloadPriorityClassChanged) fails to detect when priority class is ADDED
+			// (oldPriorityClassName == "" -> newPriorityClassName != "").
+			// However, when adding a priority class, we set the priority to a WRONG value (0)
+			// to simulate that the job controller created the workload but the reconciler
+			// needs to update the priority from the WorkloadPriorityClass.
+			gomega.Eventually(func(g gomega.Gomega) {
+				wl := &kueue.Workload{}
+				g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(noPrioWl), wl)).To(gomega.Succeed())
+				// Set priorityClassRef to reference the high-priority WorkloadPriorityClass
+				wl.Spec.PriorityClassRef = kueue.NewWorkloadPriorityClassRef("high-priority")
+				// Set priority to 0 (wrong value) - the controller should reconcile this to highPriority
+				wl.Spec.Priority = ptr.To[int32](0)
+				g.Expect(k8sClient.Update(ctx, wl)).To(gomega.Succeed())
+			}, util.Timeout, util.Interval).Should(gomega.Succeed())
+
+			ginkgo.By("Verifying the workload priority is reconciled to the correct value from the WorkloadPriorityClass")
+			util.ExpectWorkloadsWithWorkloadPriority(ctx, k8sClient, "high-priority", highPriority, client.ObjectKeyFromObject(noPrioWl))
+
+			ginkgo.By("Verifying preemption occurs and the high-priority workload gets admitted")
+			util.ExpectWorkloadsToBePreempted(ctx, k8sClient, lowWl)
+			util.FinishEvictionForWorkloads(ctx, k8sClient, lowWl)
+			util.ExpectWorkloadsToHaveQuotaReservation(ctx, k8sClient, cq.Name, noPrioWl)
+			util.ExpectWorkloadsToBePending(ctx, k8sClient, lowWl)
+		})
+	})
 })