diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index f55951d25c7..04148571c12 100644 --- a/pkg/api/pod/util.go +++ b/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 +} diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index 923e39eb671..a09513f96e8 100644 --- a/pkg/api/pod/util_test.go +++ b/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())) + } + } + }) + } + } + } +} diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 9996ebbdd20..dab04111b06 100644 --- a/pkg/apis/core/validation/validation.go +++ b/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) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("priorityClassName"), spec.PriorityClassName, msg)) - } + for _, msg := range ValidatePriorityClassName(spec.PriorityClassName, false) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("priorityClassName"), spec.PriorityClassName, msg)) } }