PodPriority validation and tests

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

pkg/api/pod/util.go

@@ -235,13 +235,11 @@ func UpdatePodCondition(status *api.PodStatus, condition *api.PodCondition) bool
 // DropDisabledFields removes disabled fields from the pod spec.
 // This should be called from PrepareForCreate/PrepareForUpdate for all resources containing a pod spec.
 func DropDisabledFields(podSpec, oldPodSpec *api.PodSpec) {
-	if !utilfeature.DefaultFeatureGate.Enabled(features.PodPriority) {
+	if !utilfeature.DefaultFeatureGate.Enabled(features.PodPriority) && !podPriorityInUse(oldPodSpec) {
+		// Set to nil pod's priority fields if the feature is disabled and the old pod
+		// does not specify any values for these fields.
 		podSpec.Priority = nil
 		podSpec.PriorityClassName = ""
-		if oldPodSpec != nil {
-			oldPodSpec.Priority = nil
-			oldPodSpec.PriorityClassName = ""
-		}
 	}
 
 	if !utilfeature.DefaultFeatureGate.Enabled(features.LocalStorageCapacityIsolation) {
@@ -419,3 +417,14 @@ func procMountInUse(podSpec *api.PodSpec) bool {
 	}
 	return false
 }
+
+// podPriorityInUse returns true if the pod spec is non-nil and has Priority or PriorityClassName set.
+func podPriorityInUse(podSpec *api.PodSpec) bool {
+	if podSpec == nil {
+		return false
+	}
+	if podSpec.Priority != nil || podSpec.PriorityClassName != "" {
+		return true
+	}
+	return false
+}

pkg/api/pod/util_test.go

@@ -612,3 +612,119 @@ func TestDropProcMount(t *testing.T) {
 		}
 	}
 }
+
+func TestDropPodPriority(t *testing.T) {
+	podPriority := int32(1000)
+	podWithoutPriority := func() *api.Pod {
+		return &api.Pod{
+			Spec: api.PodSpec{
+				Priority:          nil,
+				PriorityClassName: "",
+			},
+		}
+	}
+	podWithPriority := func() *api.Pod {
+		return &api.Pod{
+			Spec: api.PodSpec{
+				Priority:          &podPriority,
+				PriorityClassName: "",
+			},
+		}
+	}
+	podWithPriorityClassOnly := func() *api.Pod {
+		return &api.Pod{
+			Spec: api.PodSpec{
+				Priority:          nil,
+				PriorityClassName: "HighPriorityClass",
+			},
+		}
+	}
+	podWithBothPriorityFields := func() *api.Pod {
+		return &api.Pod{
+			Spec: api.PodSpec{
+				Priority:          &podPriority,
+				PriorityClassName: "HighPriorityClass",
+			},
+		}
+	}
+	podInfo := []struct {
+		description    string
+		hasPodPriority bool
+		pod            func() *api.Pod
+	}{
+		{
+			description:    "pod With no PodPriority fields set",
+			hasPodPriority: false,
+			pod:            podWithoutPriority,
+		},
+		{
+			description:    "feature disabled and pod With PodPriority field set but class name not set",
+			hasPodPriority: true,
+			pod:            podWithPriority,
+		},
+		{
+			description:    "feature disabled and pod With PodPriority ClassName field set but PortPriority not set",
+			hasPodPriority: true,
+			pod:            podWithPriorityClassOnly,
+		},
+		{
+			description:    "feature disabled and pod With both PodPriority ClassName and PodPriority fields set",
+			hasPodPriority: true,
+			pod:            podWithBothPriorityFields,
+		},
+		{
+			description:    "is nil",
+			hasPodPriority: false,
+			pod:            func() *api.Pod { return nil },
+		},
+	}
+
+	for _, enabled := range []bool{true, false} {
+		for _, oldPodInfo := range podInfo {
+			for _, newPodInfo := range podInfo {
+				oldPodHasPodPriority, oldPod := oldPodInfo.hasPodPriority, oldPodInfo.pod()
+				newPodHasPodPriority, newPod := newPodInfo.hasPodPriority, newPodInfo.pod()
+				if newPod == nil {
+					continue
+				}
+
+				t.Run(fmt.Sprintf("feature enabled=%v, old pod %v, new pod %v", enabled, oldPodInfo.description, newPodInfo.description), func(t *testing.T) {
+					defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PodPriority, enabled)()
+
+					var oldPodSpec *api.PodSpec
+					if oldPod != nil {
+						oldPodSpec = &oldPod.Spec
+					}
+					DropDisabledFields(&newPod.Spec, oldPodSpec)
+
+					// old pod should never be changed
+					if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) {
+						t.Errorf("old pod changed: %v", diff.ObjectReflectDiff(oldPod, oldPodInfo.pod()))
+					}
+
+					switch {
+					case enabled || oldPodHasPodPriority:
+						// new pod should not be changed if the feature is enabled, or if the old pod had PodPriority
+						if !reflect.DeepEqual(newPod, newPodInfo.pod()) {
+							t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodInfo.pod()))
+						}
+					case newPodHasPodPriority:
+						// new pod should be changed
+						if reflect.DeepEqual(newPod, newPodInfo.pod()) {
+							t.Errorf("new pod was not changed")
+						}
+						// new pod should not have PodPriority
+						if !reflect.DeepEqual(newPod, podWithoutPriority()) {
+							t.Errorf("new pod had PodPriority: %v", diff.ObjectReflectDiff(newPod, podWithoutPriority()))
+						}
+					default:
+						// new pod should not need to be changed
+						if !reflect.DeepEqual(newPod, newPodInfo.pod()) {
+							t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodInfo.pod()))
+						}
+					}
+				})
+			}
+		}
+	}
+}

pkg/apis/core/validation/validation.go

@@ -3046,10 +3046,8 @@ func ValidatePodSpec(spec *core.PodSpec, fldPath *field.Path) field.ErrorList {
 	}
 
 	if len(spec.PriorityClassName) > 0 {
-		if utilfeature.DefaultFeatureGate.Enabled(features.PodPriority) {
+		for _, msg := range ValidatePriorityClassName(spec.PriorityClassName, false) {
-			for _, msg := range ValidatePriorityClassName(spec.PriorityClassName, false) {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("priorityClassName"), spec.PriorityClassName, msg))
-				allErrs = append(allErrs, field.Invalid(fldPath.Child("priorityClassName"), spec.PriorityClassName, msg))
-			}
 		}
 	}