From 5f75c35edf1ea0a10a64615c43b5868484c94f46 Mon Sep 17 00:00:00 2001 From: Abu Kashem Date: Fri, 26 Jan 2024 14:27:09 -0500 Subject: [PATCH] apiserver: allow zero value for the 'nominalConcurrencyShares' field --- pkg/features/kube_features.go | 2 +- .../prioritylevelconfiguration/strategy.go | 12 +- .../strategy_test.go | 231 +++++------------- .../apiserver/pkg/features/kube_features.go | 3 +- 4 files changed, 64 insertions(+), 184 deletions(-) diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 39c89fe764d..3e2163d2a8a 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -1207,7 +1207,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS genericfeatures.WatchList: {Default: false, PreRelease: featuregate.Alpha}, - genericfeatures.ZeroLimitedNominalConcurrencyShares: {Default: false, PreRelease: featuregate.Beta}, + genericfeatures.ZeroLimitedNominalConcurrencyShares: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.32 // inherited features from apiextensions-apiserver, relisted here to get a conflict if it is changed // unintentionally on either side: diff --git a/pkg/registry/flowcontrol/prioritylevelconfiguration/strategy.go b/pkg/registry/flowcontrol/prioritylevelconfiguration/strategy.go index f27d2270980..7f63e3223b6 100644 --- a/pkg/registry/flowcontrol/prioritylevelconfiguration/strategy.go +++ b/pkg/registry/flowcontrol/prioritylevelconfiguration/strategy.go @@ -24,9 +24,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/validation/field" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" - "k8s.io/apiserver/pkg/features" "k8s.io/apiserver/pkg/storage/names" - utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/kubernetes/pkg/api/legacyscheme" "k8s.io/kubernetes/pkg/apis/flowcontrol" "k8s.io/kubernetes/pkg/apis/flowcontrol/validation" @@ -102,7 +100,7 @@ func (priorityLevelConfigurationStrategy) Validate(ctx context.Context, obj runt // 'nominalConcurrencyShares' field of 'limited' for CREATE operation. // 1:30: lift this restriction, allow zero value via v1 or v1beta3 opts := validation.PriorityLevelValidationOptions{ - AllowZeroLimitedNominalConcurrencyShares: utilfeature.DefaultFeatureGate.Enabled(features.ZeroLimitedNominalConcurrencyShares), + AllowZeroLimitedNominalConcurrencyShares: true, } return validation.ValidatePriorityLevelConfiguration(obj.(*flowcontrol.PriorityLevelConfiguration), getRequestGroupVersion(ctx), opts) } @@ -128,7 +126,6 @@ func (priorityLevelConfigurationStrategy) AllowCreateOnUpdate() bool { // ValidateUpdate is the default update validation for an end user. func (priorityLevelConfigurationStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { newPL := obj.(*flowcontrol.PriorityLevelConfiguration) - oldPL := old.(*flowcontrol.PriorityLevelConfiguration) // 1.28 server is not aware of the roundtrip annotation, and will // default any 0 value persisted (for the NominalConcurrencyShares @@ -144,8 +141,7 @@ func (priorityLevelConfigurationStrategy) ValidateUpdate(ctx context.Context, ob // only if the existing object already contains a zero value. // 1:30: lift this restriction, allow zero value via v1 or v1beta3 opts := validation.PriorityLevelValidationOptions{ - AllowZeroLimitedNominalConcurrencyShares: utilfeature.DefaultFeatureGate.Enabled(features.ZeroLimitedNominalConcurrencyShares) || - hasZeroLimitedNominalConcurrencyShares(oldPL), + AllowZeroLimitedNominalConcurrencyShares: true, } return validation.ValidatePriorityLevelConfiguration(newPL, getRequestGroupVersion(ctx), opts) } @@ -214,7 +210,3 @@ func getRequestGroupVersion(ctx context.Context) schema.GroupVersion { } return schema.GroupVersion{} } - -func hasZeroLimitedNominalConcurrencyShares(obj *flowcontrol.PriorityLevelConfiguration) bool { - return obj != nil && obj.Spec.Limited != nil && obj.Spec.Limited.NominalConcurrencyShares == 0 -} diff --git a/pkg/registry/flowcontrol/prioritylevelconfiguration/strategy_test.go b/pkg/registry/flowcontrol/prioritylevelconfiguration/strategy_test.go index 5518aadcc70..a217339b035 100644 --- a/pkg/registry/flowcontrol/prioritylevelconfiguration/strategy_test.go +++ b/pkg/registry/flowcontrol/prioritylevelconfiguration/strategy_test.go @@ -25,9 +25,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" - "k8s.io/apiserver/pkg/features" - utilfeature "k8s.io/apiserver/pkg/util/feature" - featuregatetesting "k8s.io/component-base/featuregate/testing" "k8s.io/kubernetes/pkg/apis/flowcontrol" "k8s.io/utils/ptr" @@ -99,208 +96,98 @@ func TestPriorityLevelConfigurationValidation(t *testing.T) { } return scheme } - errExpectedFn := func(v int32, msg string) field.ErrorList { - return field.ErrorList{ - field.Invalid(field.NewPath("spec").Child("limited").Child("nominalConcurrencyShares"), int32(v), msg), - } - } tests := []struct { - name string - obj runtime.Object - old *flowcontrol.PriorityLevelConfiguration // for UPDATE only - zeroFeatureEnabled bool - scheme *runtime.Scheme - errExpected field.ErrorList + name string + obj runtime.Object + old *flowcontrol.PriorityLevelConfiguration // for UPDATE only + scheme *runtime.Scheme + errExpected field.ErrorList }{ { - name: "v1, feature disabled, create, zero value, error expected", - obj: v1ObjFn(ptr.To(int32(0))), - zeroFeatureEnabled: false, - scheme: v1SchemeFn(t), - errExpected: errExpectedFn(0, "must be positive"), + name: "v1, create, zero value, no error expected", + obj: v1ObjFn(ptr.To(int32(0))), + scheme: v1SchemeFn(t), + errExpected: nil, }, { - name: "v1, feature disabled, create, unset, no error expected", - obj: v1ObjFn(nil), - zeroFeatureEnabled: false, - scheme: v1SchemeFn(t), - errExpected: nil, + name: "v1, create, unset, no error expected", + obj: v1ObjFn(nil), + scheme: v1SchemeFn(t), + errExpected: nil, }, { - name: "v1, feature disabled, create, non-zero, no error expected", - obj: v1ObjFn(ptr.To(int32(1))), - zeroFeatureEnabled: false, - scheme: v1SchemeFn(t), - errExpected: nil, + name: "v1, create, non-zero, no error expected", + obj: v1ObjFn(ptr.To(int32(1))), + scheme: v1SchemeFn(t), + errExpected: nil, }, { - name: "v1, feature enabled, create, zero value, no error expected", - obj: v1ObjFn(ptr.To(int32(0))), - zeroFeatureEnabled: true, - scheme: v1SchemeFn(t), - errExpected: nil, + name: "v1beta3, create, zero value, no error expected", + obj: v1beta3ObjFn(0, true), + scheme: v1beta3SchemeFn(t), + errExpected: nil, }, { - name: "v1, feature enabled, create, unset, no error expected", - obj: v1ObjFn(nil), - zeroFeatureEnabled: true, - scheme: v1SchemeFn(t), - errExpected: nil, + name: "v1beta3, create, zero value without annotation, no error expected", + obj: v1beta3ObjFn(0, false), + scheme: v1beta3SchemeFn(t), + errExpected: nil, }, { - name: "v1, feature enabled, create, non-zero, no error expected", - obj: v1ObjFn(ptr.To(int32(1))), - zeroFeatureEnabled: true, - scheme: v1SchemeFn(t), - errExpected: nil, - }, - { - name: "v1beta3, feature disabled, create, zero value, error expected", - obj: v1beta3ObjFn(0, true), - zeroFeatureEnabled: false, - scheme: v1beta3SchemeFn(t), - errExpected: errExpectedFn(0, "must be positive"), - }, - { - name: "v1beta3, feature disabled, create, zero value without annotation, no error expected", - obj: v1beta3ObjFn(0, false), - zeroFeatureEnabled: false, - scheme: v1beta3SchemeFn(t), - errExpected: nil, - }, - { - name: "v1beta3, feature disabled, create, non-zero, no error expected", - obj: v1beta3ObjFn(1, false), - zeroFeatureEnabled: false, - scheme: v1beta3SchemeFn(t), - errExpected: nil, - }, - { - name: "v1beta3, feature enabled, create, zero value, no error expected", - obj: v1beta3ObjFn(0, true), - zeroFeatureEnabled: true, - scheme: v1beta3SchemeFn(t), - errExpected: nil, - }, - { - name: "v1beta3, feature enabled, create, zero value without annotation, no error expected", - obj: v1beta3ObjFn(0, false), - zeroFeatureEnabled: true, - scheme: v1beta3SchemeFn(t), - errExpected: nil, - }, - { - name: "v1beta3, feature enabled, create, non-zero, no error expected", - obj: v1beta3ObjFn(1, false), - zeroFeatureEnabled: true, - scheme: v1beta3SchemeFn(t), - errExpected: nil, + name: "v1beta3, create, non-zero, no error expected", + obj: v1beta3ObjFn(1, false), + scheme: v1beta3SchemeFn(t), + errExpected: nil, }, // the following use cases cover UPDATE { - name: "v1, feature disabled, update, zero value, existing has non-zero, error expected", - obj: v1ObjFn(ptr.To(int32(0))), - old: internalObjFn(1), - zeroFeatureEnabled: false, - scheme: v1SchemeFn(t), - errExpected: errExpectedFn(0, "must be positive"), + name: "v1, update, zero value, existing has non-zero, no error expected", + obj: v1ObjFn(ptr.To(int32(0))), + old: internalObjFn(1), + scheme: v1SchemeFn(t), + errExpected: nil, }, { - name: "v1, feature disabled, update, zero value, existing has zero, no error expected", - obj: v1ObjFn(ptr.To(int32(0))), - old: internalObjFn(0), - zeroFeatureEnabled: false, - scheme: v1SchemeFn(t), - errExpected: nil, + name: "v1, update, zero value, existing has zero, no error expected", + obj: v1ObjFn(ptr.To(int32(0))), + old: internalObjFn(0), + scheme: v1SchemeFn(t), + errExpected: nil, }, { - name: "v1, feature disabled, update, non-zero value, existing has zero, no error expected", - obj: v1ObjFn(ptr.To(int32(1))), - old: internalObjFn(0), - zeroFeatureEnabled: false, - scheme: v1SchemeFn(t), - errExpected: nil, + name: "v1, update, non-zero value, existing has zero, no error expected", + obj: v1ObjFn(ptr.To(int32(1))), + old: internalObjFn(0), + scheme: v1SchemeFn(t), + errExpected: nil, }, { - name: "v1, feature enabled, update, zero value, existing has non-zero, no error expected", - obj: v1ObjFn(ptr.To(int32(0))), - old: internalObjFn(1), - zeroFeatureEnabled: true, - scheme: v1SchemeFn(t), - errExpected: nil, + name: "v1beta3, update, zero value, existing has non-zero, no error expected", + obj: v1beta3ObjFn(0, true), + old: internalObjFn(1), + scheme: v1beta3SchemeFn(t), + errExpected: nil, }, { - name: "v1, feature enabled, update, zero value, existing has zero, no error expected", - obj: v1ObjFn(ptr.To(int32(0))), - old: internalObjFn(0), - zeroFeatureEnabled: true, - scheme: v1SchemeFn(t), - errExpected: nil, + name: "v1beta3, update, zero value, existing has zero, no error expected", + obj: v1beta3ObjFn(0, true), + old: internalObjFn(0), + scheme: v1beta3SchemeFn(t), + errExpected: nil, }, { - name: "v1, feature enabled, update, non-zero value, existing has zero, no error expected", - obj: v1ObjFn(ptr.To(int32(1))), - old: internalObjFn(0), - zeroFeatureEnabled: true, - scheme: v1SchemeFn(t), - errExpected: nil, - }, - { - name: "v1beta3, feature disabled, update, zero value, existing has non-zero, error expected", - obj: v1beta3ObjFn(0, true), - old: internalObjFn(1), - zeroFeatureEnabled: false, - scheme: v1beta3SchemeFn(t), - errExpected: errExpectedFn(0, "must be positive"), - }, - { - name: "v1beta3, feature disabled, update, zero value, existing has zero, no error expected", - obj: v1beta3ObjFn(0, true), - old: internalObjFn(0), - zeroFeatureEnabled: false, - scheme: v1beta3SchemeFn(t), - errExpected: nil, - }, - { - name: "v1beta3, feature disabled, update, non-zero value, existing has zero, no error expected", - obj: v1beta3ObjFn(1, false), - old: internalObjFn(0), - zeroFeatureEnabled: false, - scheme: v1beta3SchemeFn(t), - errExpected: nil, - }, - { - name: "v1beta3, feature enabled, update, zero value, existing has non-zero, no error expected", - obj: v1beta3ObjFn(0, true), - old: internalObjFn(1), - zeroFeatureEnabled: true, - scheme: v1beta3SchemeFn(t), - errExpected: nil, - }, - { - name: "v1beta3, feature enabled, update, zero value, existing has zero, no error expected", - obj: v1beta3ObjFn(0, true), - old: internalObjFn(0), - zeroFeatureEnabled: true, - scheme: v1beta3SchemeFn(t), - errExpected: nil, - }, - { - name: "v1beta3, feature enabled, update, non-zero value, existing has zero, no error expected", - obj: v1beta3ObjFn(1, false), - old: internalObjFn(0), - zeroFeatureEnabled: true, - scheme: v1beta3SchemeFn(t), - errExpected: nil, + name: "v1beta3, update, non-zero value, existing has zero, no error expected", + obj: v1beta3ObjFn(1, false), + old: internalObjFn(0), + scheme: v1beta3SchemeFn(t), + errExpected: nil, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ZeroLimitedNominalConcurrencyShares, test.zeroFeatureEnabled)() - scheme := test.scheme scheme.Default(test.obj) diff --git a/staging/src/k8s.io/apiserver/pkg/features/kube_features.go b/staging/src/k8s.io/apiserver/pkg/features/kube_features.go index 06803b84343..d3a21dc84d5 100644 --- a/staging/src/k8s.io/apiserver/pkg/features/kube_features.go +++ b/staging/src/k8s.io/apiserver/pkg/features/kube_features.go @@ -247,6 +247,7 @@ const ( // owner: @tkashem // beta: v1.29 + // GA: v1.30 // // Allow Priority & Fairness in the API server to use a zero value for // the 'nominalConcurrencyShares' field of the 'limited' section of a @@ -315,5 +316,5 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS ConsistentListFromCache: {Default: false, PreRelease: featuregate.Alpha}, - ZeroLimitedNominalConcurrencyShares: {Default: false, PreRelease: featuregate.Beta}, + ZeroLimitedNominalConcurrencyShares: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.32 }