Repository: GoogleCloudPlatform/spark-on-k8s-operator
Pull request #1203 (2021-03-16 18:53:43)

fix: Remove priority from the spec when using priority class
By default pods created by kube will have a priority of 0 set in their
spec, or the value of the PriorityClass acting as a `globalDefault`.
When using the `batchSchedulerOptions.priorityClassName` the spec was
mutated by adding the `priorityClassName` to the spec, but the
`priority` was left unchanged.

This caused an error 
```
the integer value of priority (0) must not be
provided in pod spec; priority admission controller computed X from
the given PriorityClass name.
```
(where X is the priority associated to the PrioritClass kube object)

The patcher now properly remove the existing priority when a
priorityClassName is applied, letting kube apply the correct priority to 
the executor and driver pods.

See https://github.com/GoogleCloudPlatform/spark-on-k8s-operator/pull/1054

liyinan926: Can you run `go fmt` to format your changes?
HHK1: @liyinan926 Done

Patch:
diff --git a/pkg/webhook/patch.go b/pkg/webhook/patch.go
index d482c43..986baf7 100644
--- a/pkg/webhook/patch.go
+++ b/pkg/webhook/patch.go
@@ -63,17 +63,13 @@ func patchSparkPod(pod *corev1.Pod, app *v1beta2.SparkApplication) []patchOperat
 	patchOps = append(patchOps, addEnvVars(pod, app)...)
 	patchOps = append(patchOps, addEnvFrom(pod, app)...)
 	patchOps = append(patchOps, addHostAliases(pod, app)...)
+	patchOps = append(patchOps, addPriorityClassName(pod, app)...)
 
 	op := addSchedulerName(pod, app)
 	if op != nil {
 		patchOps = append(patchOps, *op)
 	}
 
-	op = addPriorityClassName(pod, app)
-	if op != nil {
-		patchOps = append(patchOps, *op)
-	}
-
 	if pod.Spec.Affinity == nil {
 		op := addAffinity(pod, app)
 		if op != nil {
@@ -530,17 +526,23 @@ func addSchedulerName(pod *corev1.Pod, app *v1beta2.SparkApplication) *patchOper
 	return &patchOperation{Op: "add", Path: "/spec/schedulerName", Value: *schedulerName}
 }
 
-func addPriorityClassName(pod *corev1.Pod, app *v1beta2.SparkApplication) *patchOperation {
+func addPriorityClassName(pod *corev1.Pod, app *v1beta2.SparkApplication) []patchOperation {
 	var priorityClassName *string
 
 	if app.Spec.BatchSchedulerOptions != nil {
 		priorityClassName = app.Spec.BatchSchedulerOptions.PriorityClassName
 	}
 
-	if priorityClassName == nil || *priorityClassName == "" {
-		return nil
+	var ops []patchOperation
+	if priorityClassName != nil && *priorityClassName != "" {
+		ops = append(ops, patchOperation{Op: "add", Path: "/spec/priorityClassName", Value: *priorityClassName})
+
+		if pod.Spec.Priority != nil {
+			ops = append(ops, patchOperation{Op: "remove", Path: "/spec/priority"})
+		}
 	}
-	return &patchOperation{Op: "add", Path: "/spec/priorityClassName", Value: *priorityClassName}
+
+	return ops
 }
 
 func addToleration(pod *corev1.Pod, toleration corev1.Toleration, first bool) patchOperation {


Test patch:
diff --git a/pkg/webhook/patch_test.go b/pkg/webhook/patch_test.go
index 545c33e..5b3b707 100644
--- a/pkg/webhook/patch_test.go
+++ b/pkg/webhook/patch_test.go
@@ -822,6 +822,7 @@ func TestPatchSparkPod_PriorityClassName(t *testing.T) {
 	//Driver priorityClassName should be populated when specified
 	assert.Equal(t, priorityClassName, modifiedDriverPod.Spec.PriorityClassName)
 
+	var defaultPriority int32 = 0
 	executorPod := &corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "spark-executor",
@@ -837,6 +838,7 @@ func TestPatchSparkPod_PriorityClassName(t *testing.T) {
 					Image: "spark-executor:latest",
 				},
 			},
+			Priority: &defaultPriority,
 		},
 	}
 
@@ -846,6 +848,7 @@ func TestPatchSparkPod_PriorityClassName(t *testing.T) {
 	}
 	//Executor priorityClassName should also be populated when specified in SparkApplicationSpec
 	assert.Equal(t, priorityClassName, modifiedExecutorPod.Spec.PriorityClassName)
+	assert.Nil(t, modifiedExecutorPod.Spec.Priority)
 }
 
 func TestPatchSparkPod_Sidecars(t *testing.T) {