Repository: openkruise/kruise

Issue:
[BUG] Creating a WorkloadSpread with a patch for the priorityClassName fails
**What happened**:

Creating a WorloadSpread that patches `.spec.priorityClassName` for a deployment fails on the k8s api with:

```
Error creating: pods "*******-prod-app-79644979d8-" is forbidden: the integer value of priority (0) must not be provided in pod spec; priority admission controller computed 1000000000 from the given PriorityClass name
```

**What you expected to happen**:

The pods are distributed according to the WorkloadSpread

**How to reproduce it (as minimally and precisely as possible)**:

I don't have a minimal example yet but I'll try to get it next week

**Anything else we need to know?**:

Not sure if it's relevant but I'm on GKE

**Environment**:
- Kruise version: `v1.6.3`
- Kubernetes version (use `kubectl version`): `v1.28.7-gke.1026000`
- Install details (e.g. helm install args):
- Others:


Discussion:
@furykerry: @ludat the problem may be related to the order of multiple webhooks and admission plugins. Can you patch spec.priority along with with priorityClassName? @stale: This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
@ludat: @furykerry I'll try to do this this week, will report back@stale: This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
@ludat: It's been a while but I got around this.

Now I'm running a newer version of everything:
Kruise version: `v1.7.2`
Kubernetes version (use kubectl version): `v1.30.5-gke.1014001`

Now I'm getting a different error: from the manager I'm getting:

```
kruise-system kruise-controller-manager-699dbd5d86-kxhjv manager {
  "ts": 1735883474132.8025,
  "caller": "controller/controller.go:329",
  "msg": "\"msg\"=\"Reconciler error\" \"error\"=\"Operation cannot be fulfilled on workloadspreads.apps.kruise.io \\\"THEAPP-prod-app\\\": the object has been modified; please apply your changes to the latest version and try again\" \"controller\"=\"workloadspread-controller\" \"name\"=\"THEAPP-prod-app\" \"namespace\"=\"THEAPP\" \"object\"={\"name\":\"THEAPP-prod-app\",\"namespace\":\"THEAPP\"} \"reconcileID\"=\"8e73c4f5-bb0d-4238-b1cb-9f9488605d5d\""
}
```

Looks like a different problem, I'm seeing the `resourceVersion` on the workloadspread updated so it looks like it's a problem with the controller now

BTW: adding the priority explicitly changes nothing now@furykerry: > "msg": "\"msg\"=\"Reconciler error\" \"error\"=\"Operation cannot be fulfilled on workloadspreads.apps.kruise.io \\\"THEAPP-prod-app\\\": the object has been modified; please apply your changes to the latest version and try again\" \"controller\"=\"workloadspread-controller\" \"name\"=\"THEAPP-prod-app\" \"namespace\"=\"THEAPP\" \"object\"={\"name\":\"THEAPP-prod-app\",\"namespace\":\"THEAPP\"} \"reconcileID\"=\"8e73c4f5-bb0d-4238-b1cb-9f9488605d5d\""

the error "the object has been modified" is intermediate and occurs when multiple agents are trying to update the workloadspread(in this case, most likely is the workloadspread controller and webhook are trying to update the workloadspread simutanously). you can wait a moment and see if there is any error. 

btw, you can also share your manifests of workloadspread, and we can try to run the case locally

@ludat: nope, no more errors, I increased the log level so there are a lot of logs but no `error` log except that one and only on the manager pod.

sure, it's pretty basic:

```yaml
apiVersion: apps.kruise.io/v1alpha1
kind: WorkloadSpread
metadata:
  name: THEAPP-prod-app
  namespace: THEAPP
  resourceVersion: "815754606" # this is what changes
spec:
  scheduleStrategy:
    type: Fixed
  subsets:
  - maxReplicas: 1
    name: stable-nodes
    patch:
      metadata:
        labels:
          resource-pool: stable
    requiredNodeSelectorTerm:
      matchExpressions:
      - key: cloud.google.com/gke-provisioning
        operator: In
        values:
        - standard
  - name: the-rest
    patch:
      metadata:
        labels:
          resource-pool: elastic
  targetRef:
    apiVersion: apps/v1
    kind: Deployment
    name: THEAPP-prod-app
```@AiRanthem: @ludat Thank you for pointing out this issue, I've opened a PR(#1877)  to fix it. Meanwhile, I'm very interested in your use case of using WorkloadSpread to patch priorityClassName. Could you share what problems you're trying to solve with this capability or in what scenarios you're applying it?@ludat: sure! it's similar to one of your tutorials.

I'm trying to optimize the placement of some pods inside an heterogeneous cluster. I have some nodes which are stable and some spot. The stable ones are normal reserved instances but the spot ones are spot instances which may die at any point.

So some of my pods are critical and must be placed on the stable instances, and some are more ephemeral and could be placed on either stables or spot nodes.

The problem is that if an ephemeral pod ends up in a stable node (which is fine to save resources) but a new critical pod comes up, it should be able to evict that pod (so that it itself gets scheduled on the stable node and the ephemeral pod get assigned again by the scheduler). I'm managing to do that by increasing the priority of the critical pods so that kubernetes evicts pods that have less priority than the one I'm trying to schedule@ludat: One thing that I wonder @AiRanthem is if this is just hidding the original issue or if everything will work after this :thinking:

I guess we'll find out once this is deployed@AiRanthem: > sure! it's similar to one of your tutorials.
> 
> I'm trying to optimize the placement of some pods inside an heterogeneous cluster. I have some nodes which are stable and some spot. The stable ones are normal reserved instances but the spot ones are spot instances which may die at any point.
> 
> So some of my pods are critical and must be placed on the stable instances, and some are more ephemeral and could be placed on either stables or spot nodes.
> 
> The problem is that if an ephemeral pod ends up in a stable node (which is fine to save resources) but a new critical pod comes up, it should be able to evict that pod (so that it itself gets scheduled on the stable node and the ephemeral pod get assigned again by the scheduler). I'm managing to do that by increasing the priority of the critical pods so that kubernetes evicts pods that have less priority than the one I'm trying to schedule

@ludat Your case is awesome! PLZ provide some feedback after this commit has been released on whether it truly meets your scenario. I would like to promote this use case as a best practice.@ludat: @AiRanthem I just deployed the new version and it works! thanks!

Solution patch:
diff --git a/pkg/util/workloadspread/workloadspread.go b/pkg/util/workloadspread/workloadspread.go
index ff5cf0d3..1b340b12 100644
--- a/pkg/util/workloadspread/workloadspread.go
+++ b/pkg/util/workloadspread/workloadspread.go
@@ -648,6 +648,14 @@ func injectWorkloadSpreadIntoPod(ws *appsv1alpha1.WorkloadSpread, pod *corev1.Po
 			klog.ErrorS(err, "failed to unmarshal to Pod", "pod", modified)
 			return false, err
 		}
+		if newPod.Spec.PriorityClassName != pod.Spec.PriorityClassName {
+			// Workloadspread webhook is called after builtin admission plugin,
+			// which means the priority is already set before priorityClassName being patched.
+			// Mismatched priorityClassName and priority value will be rejected by apiserver.
+			// we have to clear priority to avoid the problem, and the builtin admission plugin
+			// will be reinvoked after kruise webhook to setting the correct priority value.
+			newPod.Spec.Priority = nil
+		}
 		*pod = *newPod
 	}
 



Test patch:
diff --git a/pkg/util/workloadspread/workloadspread_test.go b/pkg/util/workloadspread/workloadspread_test.go
index 81399693..e7107224 100644
--- a/pkg/util/workloadspread/workloadspread_test.go
+++ b/pkg/util/workloadspread/workloadspread_test.go
@@ -1121,6 +1121,52 @@ func TestWorkloadSpreadMutatingPod(t *testing.T) {
 				return errors.IsNotFound(err)
 			},
 		},
+		{
+			name: "operation = create, pod priority class name is patched",
+			getPod: func() *corev1.Pod {
+				pod := podDemo.DeepCopy()
+				pod.Spec.Priority = utilpointer.Int32(10000)
+				pod.Spec.PriorityClassName = "high"
+				return pod
+			},
+			getWorkloadSpread: func() *appsv1alpha1.WorkloadSpread {
+				ws := workloadSpreadDemo.DeepCopy()
+				ws.Spec.Subsets[0].Patch = runtime.RawExtension{
+					Raw: []byte(`{"spec":{"priorityClassName":"low"}}`),
+				}
+				ws.Spec.Subsets[0].RequiredNodeSelectorTerm = nil
+				ws.Spec.Subsets[0].PreferredNodeSelectorTerms = nil
+				ws.Spec.Subsets[0].Tolerations = nil
+				return ws
+			},
+			getOperation: func() Operation {
+				return CreateOperation
+			},
+			expectPod: func() *corev1.Pod {
+				pod := podDemo.DeepCopy()
+				pod.Spec.Priority = nil
+				pod.Spec.PriorityClassName = "low"
+				pod.Spec.Affinity = &corev1.Affinity{NodeAffinity: &corev1.NodeAffinity{}}
+				pod.Annotations[MatchedWorkloadSpreadSubsetAnnotations] = `{"name":"test-ws","subset":"subset-a"}`
+				return pod
+			},
+			expectWorkloadSpread: func() *appsv1alpha1.WorkloadSpread {
+				ws := workloadSpreadDemo.DeepCopy()
+				ws.Spec.Subsets[0].Patch = runtime.RawExtension{
+					Raw: []byte(`{"spec":{"priorityClassName":"low"}}`),
+				}
+				ws.ResourceVersion = "1"
+				ws.Status.SubsetStatuses[0].MissingReplicas = 4
+				ws.Status.SubsetStatuses[0].CreatingPods[podDemo.Name] = metav1.Time{Time: time.Now()}
+				ws.Status.VersionedSubsetStatuses = map[string][]appsv1alpha1.WorkloadSpreadSubsetStatus{
+					VersionIgnored: ws.Status.SubsetStatuses,
+				}
+				ws.Spec.Subsets[0].RequiredNodeSelectorTerm = nil
+				ws.Spec.Subsets[0].PreferredNodeSelectorTerms = nil
+				ws.Spec.Subsets[0].Tolerations = nil
+				return ws
+			},
+		},
 	}
 	for _, cs := range cases {
 		t.Run(cs.name, func(t *testing.T) {
diff --git a/test/e2e/apps/workloadspread.go b/test/e2e/apps/workloadspread.go
index 290100f3..8bde5fa6 100644
--- a/test/e2e/apps/workloadspread.go
+++ b/test/e2e/apps/workloadspread.go
@@ -27,6 +27,7 @@ import (
 	"github.com/onsi/gomega"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	schedulingv1 "k8s.io/api/scheduling/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
@@ -1036,6 +1037,14 @@ var _ = SIGDescribe("workloadspread", func() {
 		})
 
 		framework.ConformanceIt("only one subset, zone-a=nil", func() {
+			priorityClass := &schedulingv1.PriorityClass{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-priority-class",
+				},
+				Value:         100,
+				GlobalDefault: false,
+			}
+			tester.CreatePriorityClass(priorityClass)
 			cloneSet := tester.NewBaseCloneSet(ns)
 			// create workloadSpread
 			targetRef := appsv1alpha1.TargetReference{
@@ -1056,7 +1065,7 @@ var _ = SIGDescribe("workloadspread", func() {
 				},
 				MaxReplicas: nil,
 				Patch: runtime.RawExtension{
-					Raw: []byte(`{"metadata":{"annotations":{"subset":"zone-a"}}}`),
+					Raw: []byte(`{"metadata":{"annotations":{"subset":"zone-a"}},"spec":{"priorityClassName":"test-priority-class"}}`),
 				},
 			}
 
@@ -1083,6 +1092,8 @@ var _ = SIGDescribe("workloadspread", func() {
 						gomega.Expect(injectWorkloadSpread.Name).To(gomega.Equal(workloadSpread.Name))
 						gomega.Expect(pod.Spec.Affinity).NotTo(gomega.BeNil())
 						gomega.Expect(pod.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms[0].MatchExpressions).To(gomega.Equal(subset1.RequiredNodeSelectorTerm.MatchExpressions))
+						gomega.Expect(pod.Spec.PriorityClassName).To(gomega.Equal("test-priority-class"))
+						gomega.Expect(*pod.Spec.Priority).To(gomega.BeEquivalentTo(100))
 						gomega.Expect(pod.Annotations[workloadspread.PodDeletionCostAnnotation]).To(gomega.Equal("100"))
 					}
 				} else {
@@ -1123,6 +1134,8 @@ var _ = SIGDescribe("workloadspread", func() {
 						gomega.Expect(injectWorkloadSpread.Name).To(gomega.Equal(workloadSpread.Name))
 						gomega.Expect(pod.Spec.Affinity).NotTo(gomega.BeNil())
 						gomega.Expect(pod.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms[0].MatchExpressions).To(gomega.Equal(subset1.RequiredNodeSelectorTerm.MatchExpressions))
+						gomega.Expect(pod.Spec.PriorityClassName).To(gomega.Equal("test-priority-class"))
+						gomega.Expect(*pod.Spec.Priority).To(gomega.BeEquivalentTo(100))
 						gomega.Expect(pod.Annotations[workloadspread.PodDeletionCostAnnotation]).To(gomega.Equal("100"))
 					}
 				} else {
@@ -1163,6 +1176,8 @@ var _ = SIGDescribe("workloadspread", func() {
 						gomega.Expect(injectWorkloadSpread.Name).To(gomega.Equal(workloadSpread.Name))
 						gomega.Expect(pod.Spec.Affinity).NotTo(gomega.BeNil())
 						gomega.Expect(pod.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms[0].MatchExpressions).To(gomega.Equal(subset1.RequiredNodeSelectorTerm.MatchExpressions))
+						gomega.Expect(pod.Spec.PriorityClassName).To(gomega.Equal("test-priority-class"))
+						gomega.Expect(*pod.Spec.Priority).To(gomega.BeEquivalentTo(100))
 						gomega.Expect(pod.Annotations[workloadspread.PodDeletionCostAnnotation]).To(gomega.Equal("100"))
 					}
 				} else {
diff --git a/test/e2e/framework/workloadspread_util.go b/test/e2e/framework/workloadspread_util.go
index e72d5a77..c13bcbd9 100644
--- a/test/e2e/framework/workloadspread_util.go
+++ b/test/e2e/framework/workloadspread_util.go
@@ -29,6 +29,7 @@ import (
 	appsv1 "k8s.io/api/apps/v1"
 	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
+	schedulingv1 "k8s.io/api/scheduling/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -40,6 +41,7 @@ import (
 	kubecontroller "k8s.io/kubernetes/pkg/controller"
 	imageutils "k8s.io/kubernetes/test/utils/image"
 	"k8s.io/utils/ptr"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 type WorkloadSpreadTester struct {
@@ -352,6 +354,17 @@ func (t *WorkloadSpreadTester) WaitForWorkloadSpreadRunning(ws *appsv1alpha1.Wor
 	}
 }
 
+func (t *WorkloadSpreadTester) CreatePriorityClass(pc *schedulingv1.PriorityClass) *schedulingv1.PriorityClass {
+	gomega.Eventually(func(g gomega.Gomega) {
+		Logf("create priorityClass (%s)", pc.Name)
+		_, err := t.C.SchedulingV1().PriorityClasses().Create(context.TODO(), pc, metav1.CreateOptions{})
+		g.Expect(client.IgnoreAlreadyExists(err)).NotTo(gomega.HaveOccurred())
+		Logf("create priorityClass (%s/%s) success", pc.Namespace, pc.Name)
+		pc, _ = t.C.SchedulingV1().PriorityClasses().Get(context.TODO(), pc.Name, metav1.GetOptions{})
+	}).WithTimeout(20 * time.Second).WithPolling(time.Second).Should(gomega.Succeed())
+	return pc
+}
+
 func (t *WorkloadSpreadTester) CreateCloneSet(cloneSet *appsv1alpha1.CloneSet) *appsv1alpha1.CloneSet {
 	Logf("create CloneSet (%s/%s)", cloneSet.Namespace, cloneSet.Name)
 	_, err := t.KC.AppsV1alpha1().CloneSets(cloneSet.Namespace).Create(context.TODO(), cloneSet, metav1.CreateOptions{})