From f9ee64007ee31780510a6e407193627ece44dedf Mon Sep 17 00:00:00 2001 From: Abu Kashem Date: Mon, 17 May 2021 11:52:48 -0400 Subject: [PATCH] apf: always create missing bootstrap configuration object(s) --- .../flowcontrol/ensurer/flowschema.go | 5 +-- .../flowcontrol/ensurer/flowschema_test.go | 19 +++------- .../ensurer/prioritylevelconfiguration.go | 5 +-- .../prioritylevelconfiguration_test.go | 19 +++------- pkg/registry/flowcontrol/ensurer/strategy.go | 24 +++--------- .../flowcontrol/rest/storage_flowcontrol.go | 25 +------------ .../rest/storage_flowcontrol_test.go | 37 ------------------- 7 files changed, 21 insertions(+), 113 deletions(-) diff --git a/pkg/registry/flowcontrol/ensurer/flowschema.go b/pkg/registry/flowcontrol/ensurer/flowschema.go index 9e2b793626f..366bcd5acd8 100644 --- a/pkg/registry/flowcontrol/ensurer/flowschema.go +++ b/pkg/registry/flowcontrol/ensurer/flowschema.go @@ -46,13 +46,12 @@ type FlowSchemaRemover interface { // NewSuggestedFlowSchemaEnsurer returns a FlowSchemaEnsurer instance that // can be used to ensure a set of suggested FlowSchema configuration objects. -// shouldCreate indicates whether a missing 'suggested' FlowSchema object should be recreated. -func NewSuggestedFlowSchemaEnsurer(client flowcontrolclient.FlowSchemaInterface, shouldCreate bool) FlowSchemaEnsurer { +func NewSuggestedFlowSchemaEnsurer(client flowcontrolclient.FlowSchemaInterface) FlowSchemaEnsurer { wrapper := &flowSchemaWrapper{ client: client, } return &fsEnsurer{ - strategy: newSuggestedEnsureStrategy(wrapper, shouldCreate), + strategy: newSuggestedEnsureStrategy(wrapper), wrapper: wrapper, } } diff --git a/pkg/registry/flowcontrol/ensurer/flowschema_test.go b/pkg/registry/flowcontrol/ensurer/flowschema_test.go index f0d5512cd3a..81f28f5cf6c 100644 --- a/pkg/registry/flowcontrol/ensurer/flowschema_test.go +++ b/pkg/registry/flowcontrol/ensurer/flowschema_test.go @@ -43,27 +43,18 @@ func TestEnsureFlowSchema(t *testing.T) { }{ // for suggested configurations { - name: "suggested flow schema does not exist and we should ensure - the object should be created", + name: "suggested flow schema does not exist - the object should always be re-created", strategy: func(client flowcontrolclient.FlowSchemaInterface) FlowSchemaEnsurer { - return NewSuggestedFlowSchemaEnsurer(client, true) + return NewSuggestedFlowSchemaEnsurer(client) }, bootstrap: newFlowSchema("fs1", "pl1", 100).Object(), current: nil, expected: newFlowSchema("fs1", "pl1", 100).Object(), }, - { - name: "suggested flow schema does not exist and we should not ensure - the object should not be created", - strategy: func(client flowcontrolclient.FlowSchemaInterface) FlowSchemaEnsurer { - return NewSuggestedFlowSchemaEnsurer(client, false) - }, - bootstrap: newFlowSchema("fs1", "pl1", 100).Object(), - current: nil, - expected: nil, - }, { name: "suggested flow schema exists, auto update is enabled, spec does not match - current object should be updated", strategy: func(client flowcontrolclient.FlowSchemaInterface) FlowSchemaEnsurer { - return NewSuggestedFlowSchemaEnsurer(client, true) + return NewSuggestedFlowSchemaEnsurer(client) }, bootstrap: newFlowSchema("fs1", "pl1", 100).Object(), current: newFlowSchema("fs1", "pl1", 200).WithAutoUpdateAnnotation("true").Object(), @@ -72,7 +63,7 @@ func TestEnsureFlowSchema(t *testing.T) { { name: "suggested flow schema exists, auto update is disabled, spec does not match - current object should not be updated", strategy: func(client flowcontrolclient.FlowSchemaInterface) FlowSchemaEnsurer { - return NewSuggestedFlowSchemaEnsurer(client, true) + return NewSuggestedFlowSchemaEnsurer(client) }, bootstrap: newFlowSchema("fs1", "pl1", 100).Object(), current: newFlowSchema("fs1", "pl1", 200).WithAutoUpdateAnnotation("false").Object(), @@ -223,7 +214,7 @@ func TestSuggestedFSEnsureStrategy_ShouldUpdate(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - strategy := newSuggestedEnsureStrategy(&flowSchemaWrapper{}, false) + strategy := newSuggestedEnsureStrategy(&flowSchemaWrapper{}) newObjectGot, updateGot, err := strategy.ShouldUpdate(test.current, test.bootstrap) if err != nil { t.Errorf("Expected no error, but got: %v", err) diff --git a/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration.go b/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration.go index 9f8235a5c48..9af7bab8f90 100644 --- a/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration.go +++ b/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration.go @@ -46,13 +46,12 @@ type PriorityLevelRemover interface { // NewSuggestedPriorityLevelEnsurerEnsurer returns a PriorityLevelEnsurer instance that // can be used to ensure a set of suggested PriorityLevelConfiguration configuration objects. -// shouldCreate indicates whether a missing 'suggested' PriorityLevelConfiguration object should be recreated. -func NewSuggestedPriorityLevelEnsurerEnsurer(client flowcontrolclient.PriorityLevelConfigurationInterface, shouldCreate bool) PriorityLevelEnsurer { +func NewSuggestedPriorityLevelEnsurerEnsurer(client flowcontrolclient.PriorityLevelConfigurationInterface) PriorityLevelEnsurer { wrapper := &priorityLevelConfigurationWrapper{ client: client, } return &plEnsurer{ - strategy: newSuggestedEnsureStrategy(wrapper, shouldCreate), + strategy: newSuggestedEnsureStrategy(wrapper), wrapper: wrapper, } } diff --git a/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration_test.go b/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration_test.go index 9a20002df8d..5e9646af4f8 100644 --- a/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration_test.go +++ b/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration_test.go @@ -43,27 +43,18 @@ func TestEnsurePriorityLevel(t *testing.T) { }{ // for suggested configurations { - name: "suggested priority level configuration does not exist and we should ensure - new object should be created", + name: "suggested priority level configuration does not exist - the object should always be re-created", strategy: func(client flowcontrolclient.PriorityLevelConfigurationInterface) PriorityLevelEnsurer { - return NewSuggestedPriorityLevelEnsurerEnsurer(client, true) + return NewSuggestedPriorityLevelEnsurerEnsurer(client) }, bootstrap: newPLConfiguration("pl1").WithLimited(10).Object(), current: nil, expected: newPLConfiguration("pl1").WithLimited(10).Object(), }, - { - name: "suggested priority level configuration does not exist and we should not ensure - new object should not be created", - strategy: func(client flowcontrolclient.PriorityLevelConfigurationInterface) PriorityLevelEnsurer { - return NewSuggestedPriorityLevelEnsurerEnsurer(client, false) - }, - bootstrap: newPLConfiguration("pl1").WithLimited(10).Object(), - current: nil, - expected: nil, - }, { name: "suggested priority level configuration exists, auto update is enabled, spec does not match - current object should be updated", strategy: func(client flowcontrolclient.PriorityLevelConfigurationInterface) PriorityLevelEnsurer { - return NewSuggestedPriorityLevelEnsurerEnsurer(client, true) + return NewSuggestedPriorityLevelEnsurerEnsurer(client) }, bootstrap: newPLConfiguration("pl1").WithLimited(20).Object(), current: newPLConfiguration("pl1").WithAutoUpdateAnnotation("true").WithLimited(10).Object(), expected: newPLConfiguration("pl1").WithAutoUpdateAnnotation("true").WithLimited(20).Object(), @@ -71,7 +62,7 @@ func TestEnsurePriorityLevel(t *testing.T) { { name: "suggested priority level configuration exists, auto update is disabled, spec does not match - current object should not be updated", strategy: func(client flowcontrolclient.PriorityLevelConfigurationInterface) PriorityLevelEnsurer { - return NewSuggestedPriorityLevelEnsurerEnsurer(client, true) + return NewSuggestedPriorityLevelEnsurerEnsurer(client) }, bootstrap: newPLConfiguration("pl1").WithLimited(20).Object(), current: newPLConfiguration("pl1").WithAutoUpdateAnnotation("false").WithLimited(10).Object(), @@ -223,7 +214,7 @@ func TestSuggestedPLEnsureStrategy_ShouldUpdate(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - strategy := newSuggestedEnsureStrategy(&priorityLevelConfigurationWrapper{}, false) + strategy := newSuggestedEnsureStrategy(&priorityLevelConfigurationWrapper{}) newObjectGot, updateGot, err := strategy.ShouldUpdate(test.current, test.bootstrap) if err != nil { t.Errorf("Expected no error, but got: %v", err) diff --git a/pkg/registry/flowcontrol/ensurer/strategy.go b/pkg/registry/flowcontrol/ensurer/strategy.go index 85f3028b070..32fce862e77 100644 --- a/pkg/registry/flowcontrol/ensurer/strategy.go +++ b/pkg/registry/flowcontrol/ensurer/strategy.go @@ -49,9 +49,6 @@ type ensureStrategy interface { // This comes handy in logging. Name() string - // ShouldCreate returns true if a missing configuration object should be recreated. - ShouldCreate() bool - // ShouldUpdate accepts the current and the bootstrap configuration and determines // whether an update is necessary. // current is the existing in-cluster configuration object. @@ -107,10 +104,9 @@ type configurationObject interface { runtime.Object } -func newSuggestedEnsureStrategy(copier specCopier, shouldCreate bool) ensureStrategy { +func newSuggestedEnsureStrategy(copier specCopier) ensureStrategy { return &strategy{ copier: copier, - shouldCreate: shouldCreate, alwaysAutoUpdateSpec: false, name: "suggested", } @@ -119,7 +115,6 @@ func newSuggestedEnsureStrategy(copier specCopier, shouldCreate bool) ensureStra func newMandatoryEnsureStrategy(copier specCopier) ensureStrategy { return &strategy{ copier: copier, - shouldCreate: true, alwaysAutoUpdateSpec: true, name: "mandatory", } @@ -128,7 +123,6 @@ func newMandatoryEnsureStrategy(copier specCopier) ensureStrategy { // auto-update strategy for the configuration objects type strategy struct { copier specCopier - shouldCreate bool alwaysAutoUpdateSpec bool name string } @@ -137,10 +131,6 @@ func (s *strategy) Name() string { return s.name } -func (s *strategy) ShouldCreate() bool { - return s.shouldCreate -} - func (s *strategy) ShouldUpdate(current, bootstrap configurationObject) (runtime.Object, bool, error) { if current == nil || bootstrap == nil { return nil, false, nil @@ -235,16 +225,12 @@ func ensureConfiguration(wrapper configurationWrapper, strategy ensureStrategy, return fmt.Errorf("failed to retrieve %s type=%s name=%q error=%w", wrapper.TypeName(), configurationType, name, err) } - if strategy.ShouldCreate() { - if _, err := wrapper.Create(bootstrap); err != nil { - return fmt.Errorf("cannot create %s type=%s name=%q error=%w", wrapper.TypeName(), configurationType, name, err) - } - - klog.V(2).InfoS(fmt.Sprintf("Successfully created %s", wrapper.TypeName()), "type", configurationType, "name", name) - return nil + // we always re-create a missing configuration object + if _, err := wrapper.Create(bootstrap); err != nil { + return fmt.Errorf("cannot create %s type=%s name=%q error=%w", wrapper.TypeName(), configurationType, name, err) } - klog.V(5).InfoS(fmt.Sprintf("Skipping creation of %s", wrapper.TypeName()), "type", configurationType, "name", name) + klog.V(2).InfoS(fmt.Sprintf("Successfully created %s", wrapper.TypeName()), "type", configurationType, "name", name) return nil } diff --git a/pkg/registry/flowcontrol/rest/storage_flowcontrol.go b/pkg/registry/flowcontrol/rest/storage_flowcontrol.go index 9734cdde473..c49eb6cf411 100644 --- a/pkg/registry/flowcontrol/rest/storage_flowcontrol.go +++ b/pkg/registry/flowcontrol/rest/storage_flowcontrol.go @@ -21,8 +21,6 @@ import ( "fmt" "time" - apierrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/wait" flowcontrolbootstrap "k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap" "k8s.io/apiserver/pkg/registry/generic" @@ -153,8 +151,6 @@ func ensure(clientset flowcontrolclient.FlowcontrolV1beta1Interface) error { // We should not attempt creation of mandatory objects if ensuring the suggested // configuration resulted in an error. // This only happens when the stop channel is closed. - // We rely on the presence of the "exempt" priority level configuration object in the cluster - // to indicate whether we should ensure suggested configuration. return fmt.Errorf("failed ensuring suggested settings - %w", err) } @@ -169,30 +165,13 @@ func ensure(clientset flowcontrolclient.FlowcontrolV1beta1Interface) error { return nil } -// shouldCreateSuggested checks if the exempt priority level exists and returns -// whether the suggested flow schemas and priority levels should be ensured. -func shouldCreateSuggested(flowcontrolClientSet flowcontrolclient.FlowcontrolV1beta1Interface) (bool, error) { - if _, err := flowcontrolClientSet.PriorityLevelConfigurations().Get(context.TODO(), flowcontrol.PriorityLevelConfigurationNameExempt, metav1.GetOptions{}); err != nil { - if apierrors.IsNotFound(err) { - return true, nil - } - return false, err - } - return false, nil -} - func ensureSuggestedConfiguration(clientset flowcontrolclient.FlowcontrolV1beta1Interface) error { - shouldCreateSuggested, err := shouldCreateSuggested(clientset) - if err != nil { - return fmt.Errorf("failed to determine whether suggested configuration should be created - error: %w", err) - } - - fsEnsurer := ensurer.NewSuggestedFlowSchemaEnsurer(clientset.FlowSchemas(), shouldCreateSuggested) + fsEnsurer := ensurer.NewSuggestedFlowSchemaEnsurer(clientset.FlowSchemas()) if err := fsEnsurer.Ensure(flowcontrolbootstrap.SuggestedFlowSchemas); err != nil { return err } - plEnsurer := ensurer.NewSuggestedPriorityLevelEnsurerEnsurer(clientset.PriorityLevelConfigurations(), shouldCreateSuggested) + plEnsurer := ensurer.NewSuggestedPriorityLevelEnsurerEnsurer(clientset.PriorityLevelConfigurations()) return plEnsurer.Ensure(flowcontrolbootstrap.SuggestedPriorityLevelConfigurations) } diff --git a/pkg/registry/flowcontrol/rest/storage_flowcontrol_test.go b/pkg/registry/flowcontrol/rest/storage_flowcontrol_test.go index 3d389c9013b..4417f473f9e 100644 --- a/pkg/registry/flowcontrol/rest/storage_flowcontrol_test.go +++ b/pkg/registry/flowcontrol/rest/storage_flowcontrol_test.go @@ -20,45 +20,8 @@ import ( "context" "testing" "time" - - "github.com/stretchr/testify/assert" - flowcontrolv1beta1 "k8s.io/api/flowcontrol/v1beta1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap" - "k8s.io/client-go/kubernetes/fake" ) -func TestShouldEnsurePredefinedSettings(t *testing.T) { - testCases := []struct { - name string - existingPriorityLevel *flowcontrolv1beta1.PriorityLevelConfiguration - expected bool - }{ - { - name: "should ensure if exempt priority-level is absent", - existingPriorityLevel: nil, - expected: true, - }, - { - name: "should not ensure if exempt priority-level is present", - existingPriorityLevel: bootstrap.MandatoryPriorityLevelConfigurationExempt, - expected: false, - }, - } - - for _, testCase := range testCases { - t.Run(testCase.name, func(t *testing.T) { - c := fake.NewSimpleClientset() - if testCase.existingPriorityLevel != nil { - c.FlowcontrolV1beta1().PriorityLevelConfigurations().Create(context.TODO(), testCase.existingPriorityLevel, metav1.CreateOptions{}) - } - should, err := shouldCreateSuggested(c.FlowcontrolV1beta1()) - assert.NoError(t, err) - assert.Equal(t, testCase.expected, should) - }) - } -} - func TestContextFromChannelAndMaxWaitDurationWithChannelClosed(t *testing.T) { stopCh := make(chan struct{}) ctx, cancel := contextFromChannelAndMaxWaitDuration(stopCh, time.Hour)