From 424b23bb15d21d8c710e101b6f3a86c24d0249d3 Mon Sep 17 00:00:00 2001 From: Abu Kashem Date: Thu, 20 Oct 2022 18:50:14 -0400 Subject: [PATCH] apiserver: fix defaulting for apf bootstrap configuration --- .../internalbootstrap/defaults_test.go | 48 ++++++++++++------- .../flowcontrol/validation/validation_test.go | 1 + .../prioritylevelconfiguration_test.go | 9 +++- .../pkg/apis/flowcontrol/bootstrap/default.go | 8 ++++ 4 files changed, 47 insertions(+), 19 deletions(-) diff --git a/pkg/apis/flowcontrol/internalbootstrap/defaults_test.go b/pkg/apis/flowcontrol/internalbootstrap/defaults_test.go index dc5d24338c9..3e2482b01ce 100644 --- a/pkg/apis/flowcontrol/internalbootstrap/defaults_test.go +++ b/pkg/apis/flowcontrol/internalbootstrap/defaults_test.go @@ -17,31 +17,45 @@ limitations under the License. package internalbootstrap import ( + "fmt" "testing" + "github.com/google/go-cmp/cmp" flowcontrol "k8s.io/api/flowcontrol/v1beta3" apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap" ) -func TestMandatoryAlreadyDefaulted(t *testing.T) { +func TestBootstrapConfigurationWithDefaulted(t *testing.T) { scheme := NewAPFScheme() - for _, obj := range bootstrap.MandatoryFlowSchemas { - obj2 := obj.DeepCopyObject().(*flowcontrol.FlowSchema) - scheme.Default(obj2) - if apiequality.Semantic.DeepEqual(obj, obj2) { - t.Logf("Defaulting makes no change to %#+v", *obj) - } else { - t.Errorf("Defaulting changed %#+v to %#+v", *obj, *obj2) - } + + bootstrapFlowSchemas := make([]*flowcontrol.FlowSchema, 0) + bootstrapFlowSchemas = append(bootstrapFlowSchemas, bootstrap.MandatoryFlowSchemas...) + bootstrapFlowSchemas = append(bootstrapFlowSchemas, bootstrap.SuggestedFlowSchemas...) + for _, original := range bootstrapFlowSchemas { + t.Run(fmt.Sprintf("FlowSchema/%s", original.Name), func(t *testing.T) { + defaulted := original.DeepCopyObject().(*flowcontrol.FlowSchema) + scheme.Default(defaulted) + if apiequality.Semantic.DeepEqual(original, defaulted) { + t.Logf("Defaulting makes no change to FlowSchema: %q", original.Name) + return + } + t.Errorf("Expected defaulting to not change FlowSchema: %q, diff: %s", original.Name, cmp.Diff(original, defaulted)) + }) } - for _, obj := range bootstrap.MandatoryPriorityLevelConfigurations { - obj2 := obj.DeepCopyObject().(*flowcontrol.PriorityLevelConfiguration) - scheme.Default(obj2) - if apiequality.Semantic.DeepEqual(obj, obj2) { - t.Logf("Defaulting makes no change to %#+v", *obj) - } else { - t.Errorf("Defaulting changed %#+v to %#+v", *obj, *obj2) - } + + bootstrapPriorityLevels := make([]*flowcontrol.PriorityLevelConfiguration, 0) + bootstrapPriorityLevels = append(bootstrapPriorityLevels, bootstrap.MandatoryPriorityLevelConfigurations...) + bootstrapPriorityLevels = append(bootstrapPriorityLevels, bootstrap.SuggestedPriorityLevelConfigurations...) + for _, original := range bootstrapPriorityLevels { + t.Run(fmt.Sprintf("PriorityLevelConfiguration/%s", original.Name), func(t *testing.T) { + defaulted := original.DeepCopyObject().(*flowcontrol.PriorityLevelConfiguration) + scheme.Default(defaulted) + if apiequality.Semantic.DeepEqual(original, defaulted) { + t.Logf("Defaulting makes no change to PriorityLevelConfiguration: %q", original.Name) + return + } + t.Errorf("Expected defaulting to not change PriorityLevelConfiguration: %q, diff: %s", original.Name, cmp.Diff(original, defaulted)) + }) } } diff --git a/pkg/apis/flowcontrol/validation/validation_test.go b/pkg/apis/flowcontrol/validation/validation_test.go index 698848fcc56..11825564988 100644 --- a/pkg/apis/flowcontrol/validation/validation_test.go +++ b/pkg/apis/flowcontrol/validation/validation_test.go @@ -1021,6 +1021,7 @@ func TestPriorityLevelConfigurationValidation(t *testing.T) { Type: flowcontrol.PriorityLevelEnablementLimited, Limited: &flowcontrol.LimitedPriorityLevelConfiguration{ NominalConcurrencyShares: 5, + LendablePercent: pointer.Int32(0), LimitResponse: flowcontrol.LimitResponse{ Type: flowcontrol.LimitResponseTypeReject, }}}, diff --git a/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration_test.go b/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration_test.go index 3db69b57944..8c87dceda98 100644 --- a/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration_test.go +++ b/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration_test.go @@ -30,10 +30,10 @@ import ( flowcontrollisters "k8s.io/client-go/listers/flowcontrol/v1beta3" "k8s.io/client-go/tools/cache" flowcontrolapisv1beta3 "k8s.io/kubernetes/pkg/apis/flowcontrol/v1beta3" + "k8s.io/utils/pointer" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" - "github.com/stretchr/testify/assert" ) func TestEnsurePriorityLevel(t *testing.T) { @@ -257,6 +257,7 @@ func TestPriorityLevelSpecChanged(t *testing.T) { Type: flowcontrolv1beta3.PriorityLevelEnablementLimited, Limited: &flowcontrolv1beta3.LimitedPriorityLevelConfiguration{ NominalConcurrencyShares: flowcontrolapisv1beta3.PriorityLevelConfigurationDefaultNominalConcurrencyShares, + LendablePercent: pointer.Int32(0), LimitResponse: flowcontrolv1beta3.LimitResponse{ Type: flowcontrolv1beta3.LimitResponseTypeReject, }, @@ -291,7 +292,10 @@ func TestPriorityLevelSpecChanged(t *testing.T) { for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { w := priorityLevelSpecChanged(testCase.expected, testCase.actual) - assert.Equal(t, testCase.specChanged, w) + if testCase.specChanged != w { + t.Errorf("Expected priorityLevelSpecChanged to return %t, but got: %t - diff: %s", testCase.specChanged, w, + cmp.Diff(testCase.expected, testCase.actual)) + } }) } } @@ -472,6 +476,7 @@ func (b *plBuilder) WithLimited(nominalConcurrencyShares int32) *plBuilder { b.object.Spec.Type = flowcontrolv1beta3.PriorityLevelEnablementLimited b.object.Spec.Limited = &flowcontrolv1beta3.LimitedPriorityLevelConfiguration{ NominalConcurrencyShares: nominalConcurrencyShares, + LendablePercent: pointer.Int32(0), LimitResponse: flowcontrolv1beta3.LimitResponse{ Type: flowcontrolv1beta3.LimitResponseTypeReject, }, diff --git a/staging/src/k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap/default.go b/staging/src/k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap/default.go index b12f4c27479..cbfaca84e9c 100644 --- a/staging/src/k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap/default.go +++ b/staging/src/k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap/default.go @@ -23,6 +23,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apiserver/pkg/authentication/serviceaccount" "k8s.io/apiserver/pkg/authentication/user" + "k8s.io/utils/pointer" ) // The objects that define an apiserver's initial behavior. The @@ -96,6 +97,7 @@ var ( Type: flowcontrol.PriorityLevelEnablementLimited, Limited: &flowcontrol.LimitedPriorityLevelConfiguration{ NominalConcurrencyShares: 5, + LendablePercent: pointer.Int32(0), LimitResponse: flowcontrol.LimitResponse{ Type: flowcontrol.LimitResponseTypeReject, }, @@ -168,6 +170,7 @@ var ( Type: flowcontrol.PriorityLevelEnablementLimited, Limited: &flowcontrol.LimitedPriorityLevelConfiguration{ NominalConcurrencyShares: 30, + LendablePercent: pointer.Int32(0), LimitResponse: flowcontrol.LimitResponse{ Type: flowcontrol.LimitResponseTypeQueue, Queuing: &flowcontrol.QueuingConfiguration{ @@ -184,6 +187,7 @@ var ( Type: flowcontrol.PriorityLevelEnablementLimited, Limited: &flowcontrol.LimitedPriorityLevelConfiguration{ NominalConcurrencyShares: 40, + LendablePercent: pointer.Int32(0), LimitResponse: flowcontrol.LimitResponse{ Type: flowcontrol.LimitResponseTypeQueue, Queuing: &flowcontrol.QueuingConfiguration{ @@ -201,6 +205,7 @@ var ( Type: flowcontrol.PriorityLevelEnablementLimited, Limited: &flowcontrol.LimitedPriorityLevelConfiguration{ NominalConcurrencyShares: 10, + LendablePercent: pointer.Int32(0), LimitResponse: flowcontrol.LimitResponse{ Type: flowcontrol.LimitResponseTypeQueue, Queuing: &flowcontrol.QueuingConfiguration{ @@ -218,6 +223,7 @@ var ( Type: flowcontrol.PriorityLevelEnablementLimited, Limited: &flowcontrol.LimitedPriorityLevelConfiguration{ NominalConcurrencyShares: 40, + LendablePercent: pointer.Int32(0), LimitResponse: flowcontrol.LimitResponse{ Type: flowcontrol.LimitResponseTypeQueue, Queuing: &flowcontrol.QueuingConfiguration{ @@ -235,6 +241,7 @@ var ( Type: flowcontrol.PriorityLevelEnablementLimited, Limited: &flowcontrol.LimitedPriorityLevelConfiguration{ NominalConcurrencyShares: 100, + LendablePercent: pointer.Int32(0), LimitResponse: flowcontrol.LimitResponse{ Type: flowcontrol.LimitResponseTypeQueue, Queuing: &flowcontrol.QueuingConfiguration{ @@ -252,6 +259,7 @@ var ( Type: flowcontrol.PriorityLevelEnablementLimited, Limited: &flowcontrol.LimitedPriorityLevelConfiguration{ NominalConcurrencyShares: 20, + LendablePercent: pointer.Int32(0), LimitResponse: flowcontrol.LimitResponse{ Type: flowcontrol.LimitResponseTypeQueue, Queuing: &flowcontrol.QueuingConfiguration{