From 2069835094d0e7213ce10f183041635fd8ef4cd1 Mon Sep 17 00:00:00 2001 From: Mike Spreitzer Date: Mon, 26 Jun 2023 00:12:33 -0400 Subject: [PATCH] Tolerate only the two allowed field changes Signed-off-by: Mike Spreitzer --- .../ensurer/prioritylevelconfiguration.go | 11 +++- .../prioritylevelconfiguration_test.go | 57 ++++++++++++++++++- pkg/registry/flowcontrol/ensurer/strategy.go | 52 +++++++---------- 3 files changed, 86 insertions(+), 34 deletions(-) diff --git a/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration.go b/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration.go index c1815fcab20..b57f2525672 100644 --- a/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration.go +++ b/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration.go @@ -26,7 +26,7 @@ import ( func NewPriorityLevelConfigurationOps(client flowcontrolclient.PriorityLevelConfigurationInterface, lister flowcontrollisters.PriorityLevelConfigurationLister) ObjectOps[*flowcontrolv1beta3.PriorityLevelConfiguration] { return NewObjectOps[*flowcontrolv1beta3.PriorityLevelConfiguration](client, lister, (*flowcontrolv1beta3.PriorityLevelConfiguration).DeepCopy, - plcReplaceSpec, plcSpecEqual) + plcReplaceSpec, plcSpecEqualish) } func plcReplaceSpec(into, from *flowcontrolv1beta3.PriorityLevelConfiguration) *flowcontrolv1beta3.PriorityLevelConfiguration { @@ -35,8 +35,15 @@ func plcReplaceSpec(into, from *flowcontrolv1beta3.PriorityLevelConfiguration) * return copy } -func plcSpecEqual(expected, actual *flowcontrolv1beta3.PriorityLevelConfiguration) bool { +func plcSpecEqualish(expected, actual *flowcontrolv1beta3.PriorityLevelConfiguration) bool { copiedExpected := expected.DeepCopy() flowcontrolapisv1beta3.SetObjectDefaults_PriorityLevelConfiguration(copiedExpected) + if expected.Name == flowcontrolv1beta3.PriorityLevelConfigurationNameExempt { + if actual.Spec.Exempt == nil { + return false + } + copiedExpected.Spec.Exempt.NominalConcurrencyShares = actual.Spec.Exempt.NominalConcurrencyShares + copiedExpected.Spec.Exempt.LendablePercent = actual.Spec.Exempt.LendablePercent + } return equality.Semantic.DeepEqual(copiedExpected.Spec, actual.Spec) } diff --git a/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration_test.go b/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration_test.go index d76040c8b73..d344b7a8c9a 100644 --- a/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration_test.go +++ b/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration_test.go @@ -37,7 +37,7 @@ import ( func TestEnsurePriorityLevel(t *testing.T) { validExemptPL := func() *flowcontrolv1beta3.PriorityLevelConfiguration { copy := bootstrap.MandatoryPriorityLevelConfigurationExempt.DeepCopy() - copy.Annotations[flowcontrolv1beta3.AutoUpdateAnnotationKey] = "false" + copy.Annotations[flowcontrolv1beta3.AutoUpdateAnnotationKey] = "true" copy.Spec.Exempt.NominalConcurrencyShares = pointer.Int32(10) copy.Spec.Exempt.LendablePercent = pointer.Int32(50) return copy @@ -281,6 +281,41 @@ func TestPriorityLevelSpecChanged(t *testing.T) { }, }, } + ple1 := &flowcontrolv1beta3.PriorityLevelConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "exempt"}, + Spec: flowcontrolv1beta3.PriorityLevelConfigurationSpec{ + Type: flowcontrolv1beta3.PriorityLevelEnablementExempt, + Exempt: &flowcontrolv1beta3.ExemptPriorityLevelConfiguration{ + NominalConcurrencyShares: pointer.Int32(42), + LendablePercent: pointer.Int32(33), + }, + }, + } + ple2 := &flowcontrolv1beta3.PriorityLevelConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "exempt"}, + Spec: flowcontrolv1beta3.PriorityLevelConfigurationSpec{ + Type: flowcontrolv1beta3.PriorityLevelEnablementExempt, + Exempt: &flowcontrolv1beta3.ExemptPriorityLevelConfiguration{ + NominalConcurrencyShares: pointer.Int32(24), + LendablePercent: pointer.Int32(86), + }, + }, + } + pleWrong := &flowcontrolv1beta3.PriorityLevelConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "exempt"}, + Spec: flowcontrolv1beta3.PriorityLevelConfigurationSpec{ + Type: flowcontrolv1beta3.PriorityLevelEnablementLimited, + Limited: &flowcontrolv1beta3.LimitedPriorityLevelConfiguration{ + NominalConcurrencyShares: 1, + }, + }, + } + pleInvalid := &flowcontrolv1beta3.PriorityLevelConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "exempt"}, + Spec: flowcontrolv1beta3.PriorityLevelConfigurationSpec{ + Type: "widget", + }, + } testCases := []struct { name string expected *flowcontrolv1beta3.PriorityLevelConfiguration @@ -305,10 +340,28 @@ func TestPriorityLevelSpecChanged(t *testing.T) { actual: pl2, specChanged: true, }, + { + name: "tweaked exempt config", + expected: ple1, + actual: ple2, + specChanged: false, + }, + { + name: "exempt with wrong tag", + expected: ple1, + actual: pleWrong, + specChanged: true, + }, + { + name: "exempt with invalid tag", + expected: ple1, + actual: pleInvalid, + specChanged: true, + }, } for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { - w := !plcSpecEqual(testCase.expected, testCase.actual) + w := !plcSpecEqualish(testCase.expected, testCase.actual) 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)) diff --git a/pkg/registry/flowcontrol/ensurer/strategy.go b/pkg/registry/flowcontrol/ensurer/strategy.go index 84008b559d2..cb9b2cefa32 100644 --- a/pkg/registry/flowcontrol/ensurer/strategy.go +++ b/pkg/registry/flowcontrol/ensurer/strategy.go @@ -69,8 +69,10 @@ type objectLocalOps[ObjectType configurationObject] interface { // replaceSpec returns a deep copy of `into` except that the spec is a deep copy of `from` ReplaceSpec(into, from ObjectType) ObjectType - // specEqual says whether applying defaulting to `expected` makes its spec equal that of `actual` - SpecEqual(expected, actual ObjectType) bool + // SpecEqualish says whether applying defaulting to `expected` + // makes its spec more or less equal (as appropriate for the + // object at hand) that of `actual`. + SpecEqualish(expected, actual ObjectType) bool } // ObjectOps is the needed operations, both as a receiver from a server and server-independent, on configurationObjects @@ -109,21 +111,21 @@ type configurationObjectType interface { type objectOps[ObjectType configurationObjectType] struct { client[ObjectType] cache[ObjectType] - deepCopy func(ObjectType) ObjectType - replaceSpec func(ObjectType, ObjectType) ObjectType - specEqual func(expected, actual ObjectType) bool + deepCopy func(ObjectType) ObjectType + replaceSpec func(ObjectType, ObjectType) ObjectType + specEqualish func(expected, actual ObjectType) bool } func NewObjectOps[ObjectType configurationObjectType](client client[ObjectType], cache cache[ObjectType], deepCopy func(ObjectType) ObjectType, replaceSpec func(ObjectType, ObjectType) ObjectType, - specEqual func(expected, actual ObjectType) bool, + specEqualish func(expected, actual ObjectType) bool, ) ObjectOps[ObjectType] { return objectOps[ObjectType]{client: client, - cache: cache, - deepCopy: deepCopy, - replaceSpec: replaceSpec, - specEqual: specEqual} + cache: cache, + deepCopy: deepCopy, + replaceSpec: replaceSpec, + specEqualish: specEqualish} } func (oo objectOps[ObjectType]) DeepCopy(obj ObjectType) ObjectType { return oo.deepCopy(obj) } @@ -132,39 +134,30 @@ func (oo objectOps[ObjectType]) ReplaceSpec(into, from ObjectType) ObjectType { return oo.replaceSpec(into, from) } -func (oo objectOps[ObjectType]) SpecEqual(expected, actual ObjectType) bool { - return oo.specEqual(expected, actual) +func (oo objectOps[ObjectType]) SpecEqualish(expected, actual ObjectType) bool { + return oo.specEqualish(expected, actual) } // NewSuggestedEnsureStrategy returns an EnsureStrategy for suggested config objects func NewSuggestedEnsureStrategy[ObjectType configurationObjectType]() EnsureStrategy[ObjectType] { return &strategy[ObjectType]{ - alwaysAutoUpdateSpecFn: func(want, have ObjectType) bool { - return false - }, - name: "suggested", + alwaysAutoUpdateSpec: false, + name: "suggested", } } // NewMandatoryEnsureStrategy returns an EnsureStrategy for mandatory config objects func NewMandatoryEnsureStrategy[ObjectType configurationObjectType]() EnsureStrategy[ObjectType] { return &strategy[ObjectType]{ - alwaysAutoUpdateSpecFn: func(want, have ObjectType) bool { - if want.GetName() == flowcontrolv1beta3.PriorityLevelConfigurationNameExempt { - // we allow the cluster operator to update the spec of the - // singleton 'exempt' prioritylevelconfiguration object. - return false - } - return true - }, - name: "mandatory", + alwaysAutoUpdateSpec: true, + name: "mandatory", } } // auto-update strategy for the configuration objects type strategy[ObjectType configurationObjectType] struct { - alwaysAutoUpdateSpecFn func(want, have ObjectType) bool - name string + alwaysAutoUpdateSpec bool + name string } func (s *strategy[ObjectType]) Name() string { @@ -177,14 +170,13 @@ func (s *strategy[ObjectType]) ReviseIfNeeded(objectOps objectLocalOps[ObjectTyp return zero, false, nil } - autoUpdateSpec := s.alwaysAutoUpdateSpecFn(bootstrap, current) + autoUpdateSpec := s.alwaysAutoUpdateSpec if !autoUpdateSpec { autoUpdateSpec = shouldUpdateSpec(current) } updateAnnotation := shouldUpdateAnnotation(current, autoUpdateSpec) - // specChanged := autoUpdateSpec && wah.specsDiffer() - specChanged := autoUpdateSpec && !objectOps.SpecEqual(bootstrap, current) + specChanged := autoUpdateSpec && !objectOps.SpecEqualish(bootstrap, current) if !(updateAnnotation || specChanged) { // the annotation key is up to date and the spec has not changed, no update is necessary