* fix: wrong use of apivalication.ValidateImmutableField

# change: order of parameters is (newObj,oldObj,fieldPath) not is (oldObj,newObj,fieldPath) # change: add e2e test for queueName changed workload when job is suspend and running Signed-off-by: xuxianzhang <[email protected]>
kubernetes-sigs · Jul 8, 2024 · 8f915ec · 8f915ec
1 parent f35494d
commit 8f915ec
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 4 deletions.
diff --git a/pkg/controller/jobframework/validation.go b/pkg/controller/jobframework/validation.go
@@ -87,16 +87,16 @@ func ValidateLabelAsCRDName(job GenericJob, crdNameLabel string) field.ErrorList
 func validateUpdateForQueueName(oldJob, newJob GenericJob) field.ErrorList {
 	var allErrs field.ErrorList
 	if !newJob.IsSuspended() {
-		allErrs = append(allErrs, apivalidation.ValidateImmutableField(QueueName(oldJob), QueueName(newJob), queueNameLabelPath)...)
+		allErrs = append(allErrs, apivalidation.ValidateImmutableField(QueueName(newJob), QueueName(oldJob), queueNameLabelPath)...)
 	}
 
 	oldWlName, _ := PrebuiltWorkloadFor(oldJob)
 	newWlName, _ := PrebuiltWorkloadFor(newJob)
-	allErrs = append(allErrs, apivalidation.ValidateImmutableField(oldWlName, newWlName, labelsPath.Key(constants.PrebuiltWorkloadLabel))...)
+	allErrs = append(allErrs, apivalidation.ValidateImmutableField(newWlName, oldWlName, labelsPath.Key(constants.PrebuiltWorkloadLabel))...)
 	return allErrs
 }
 
 func validateUpdateForWorkloadPriorityClassName(oldJob, newJob GenericJob) field.ErrorList {
-	allErrs := apivalidation.ValidateImmutableField(workloadPriorityClassName(oldJob), workloadPriorityClassName(newJob), workloadPriorityClassNamePath)
+	allErrs := apivalidation.ValidateImmutableField(workloadPriorityClassName(newJob), workloadPriorityClassName(oldJob), workloadPriorityClassNamePath)
 	return allErrs
 }
diff --git a/pkg/controller/jobs/job/job_webhook_test.go b/pkg/controller/jobs/job/job_webhook_test.go
@@ -374,7 +374,31 @@ func TestValidateUpdate(t *testing.T) {
 				Suspend(false).
 				Label(constants.PrebuiltWorkloadLabel, "new-workload").
 				Obj(),
-			wantErr: apivalidation.ValidateImmutableField("old-workload", "new-workload", prebuiltWlNameLabelPath),
+			wantErr: apivalidation.ValidateImmutableField("new-workload", "old-workload", prebuiltWlNameLabelPath),
+		},
+		{
+			name: "immutable queue name not suspend",
+			oldJob: testingutil.MakeJob("job", "default").
+				Suspend(false).
+				Label(constants.QueueLabel, "old-queue").
+				Obj(),
+			newJob: testingutil.MakeJob("job", "default").
+				Suspend(false).
+				Label(constants.QueueLabel, "new-queue").
+				Obj(),
+			wantErr: apivalidation.ValidateImmutableField("new-queue", "old-queue", queueNameLabelPath),
+		},
+		{
+			name: "queue name can changes when it is  suspend",
+			oldJob: testingutil.MakeJob("job", "default").
+				Suspend(true).
+				Label(constants.QueueLabel, "old-queue").
+				Obj(),
+			newJob: testingutil.MakeJob("job", "default").
+				Suspend(true).
+				Label(constants.QueueLabel, "new-queue").
+				Obj(),
+			wantErr: nil,
 		},
 	}