From 686d56b6f3f2c177ed02da090ff87b3e5392b84c Mon Sep 17 00:00:00 2001 From: Helder Santana Date: Fri, 11 Oct 2024 14:43:17 +0200 Subject: [PATCH] configurable sync period for independent custom resources # Conflicts: # pkg/controller/atlasdatabaseuser/atlasdatabaseuser_controller_test.go --- cmd/manager/main.go | 5 +++ .../atlasdatabaseuser_controller.go | 6 ++- .../atlasdatabaseuser_controller_test.go | 10 +++-- pkg/controller/workflow/result.go | 4 +- pkg/operator/builder.go | 41 ++++++++++++++----- pkg/operator/builder_test.go | 37 ++++++++++------- 6 files changed, 70 insertions(+), 33 deletions(-) diff --git a/cmd/manager/main.go b/cmd/manager/main.go index c3c01d40b8..af6cd8901d 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -23,6 +23,7 @@ import ( "log" "os" "strings" + "time" "github.com/go-logr/zapr" "go.uber.org/zap" @@ -51,6 +52,7 @@ const ( objectDeletionProtectionDefault = true subobjectDeletionProtectionDefault = false subobjectDeletionProtectionMessage = "Note: sub-object deletion protection is IGNORED because it does not work deterministically." + independentSyncPeriod = 15 // time in minutes ) func main() { @@ -82,6 +84,7 @@ func main() { WithAtlasDomain(config.AtlasDomain). WithAPISecret(config.GlobalAPISecret). WithDeletionProtection(config.ObjectDeletionProtection). + WithIndependentSyncPeriod(time.Duration(config.IndependentSyncPeriod) * time.Minute). Build(ctx) if err != nil { setupLog.Error(err, "unable to start operator") @@ -107,6 +110,7 @@ type Config struct { LogEncoder string ObjectDeletionProtection bool SubObjectDeletionProtection bool + IndependentSyncPeriod int FeatureFlags *featureflags.FeatureFlags } @@ -128,6 +132,7 @@ func parseConfiguration() Config { "when a Custom Resource is deleted") flag.BoolVar(&config.SubObjectDeletionProtection, subobjectDeletionProtectionFlag, subobjectDeletionProtectionDefault, "Defines if the operator overwrites "+ "(and consequently delete) subresources that were not previously created by the operator. "+subobjectDeletionProtectionMessage) + flag.IntVar(&config.IndependentSyncPeriod, "independent-sync-period", independentSyncPeriod, "The default time, in minutes, between reconciliations for independent custom resources. (default 15, minimum 5)") appVersion := flag.Bool("v", false, "prints application version") flag.Parse() diff --git a/pkg/controller/atlasdatabaseuser/atlasdatabaseuser_controller.go b/pkg/controller/atlasdatabaseuser/atlasdatabaseuser_controller.go index 36a3af972e..91f46b11f6 100644 --- a/pkg/controller/atlasdatabaseuser/atlasdatabaseuser_controller.go +++ b/pkg/controller/atlasdatabaseuser/atlasdatabaseuser_controller.go @@ -19,6 +19,7 @@ package atlasdatabaseuser import ( "context" "fmt" + "time" "go.uber.org/zap" corev1 "k8s.io/api/core/v1" @@ -65,6 +66,7 @@ type AtlasDatabaseUserReconciler struct { ObjectDeletionProtection bool SubObjectDeletionProtection bool FeaturePreviewOIDCAuthEnabled bool + independentSyncPeriod time.Duration dbUserService dbuser.AtlasUsersService deploymentService deployment.AtlasDeploymentsService @@ -203,7 +205,7 @@ func (r *AtlasDatabaseUserReconciler) ready(ctx *workflow.Context, atlasDatabase EnsureStatusOption(status.AtlasDatabaseUserPasswordVersion(passwordVersion)) if atlasDatabaseUser.Spec.ExternalProjectRef != nil { - return workflow.Requeue(workflow.StandaloneResourceRequeuePeriod).ReconcileResult() + return workflow.Requeue(r.independentSyncPeriod).ReconcileResult() } return workflow.OK().ReconcileResult() @@ -275,6 +277,7 @@ func NewAtlasDatabaseUserReconciler( predicates []predicate.Predicate, atlasProvider atlas.Provider, deletionProtection bool, + independentSyncPeriod time.Duration, featureFlags *featureflags.FeatureFlags, logger *zap.Logger, ) *AtlasDatabaseUserReconciler { @@ -287,5 +290,6 @@ func NewAtlasDatabaseUserReconciler( AtlasProvider: atlasProvider, ObjectDeletionProtection: deletionProtection, FeaturePreviewOIDCAuthEnabled: featureFlags.IsFeaturePresent(featureflags.FeatureOIDC), + independentSyncPeriod: independentSyncPeriod, } } diff --git a/pkg/controller/atlasdatabaseuser/atlasdatabaseuser_controller_test.go b/pkg/controller/atlasdatabaseuser/atlasdatabaseuser_controller_test.go index 7548281201..5409181779 100644 --- a/pkg/controller/atlasdatabaseuser/atlasdatabaseuser_controller_test.go +++ b/pkg/controller/atlasdatabaseuser/atlasdatabaseuser_controller_test.go @@ -5,6 +5,7 @@ import ( "errors" "reflect" "testing" + "time" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" @@ -385,7 +386,7 @@ func TestReady(t *testing.T) { api.TrueCondition(api.DatabaseUserReadyType), }, }, - "don't requeue when it's a standalone resource": { + "requeue when it's a standalone resource": { dbUser: &akov2.AtlasDatabaseUser{ ObjectMeta: metav1.ObjectMeta{ Name: "user1", @@ -408,7 +409,7 @@ func TestReady(t *testing.T) { }, }, passwordVersion: "1", - expectedResult: workflow.Requeue(workflow.StandaloneResourceRequeuePeriod).ReconcileResult(), + expectedResult: workflow.Requeue(10 * time.Minute).ReconcileResult(), expectedConditions: []api.Condition{ api.TrueCondition(api.ReadyType), api.TrueCondition(api.DatabaseUserReadyType), @@ -430,8 +431,9 @@ func TestReady(t *testing.T) { logger := zaptest.NewLogger(t).Sugar() c := &AtlasDatabaseUserReconciler{ - Client: k8sClient, - Log: logger, + Client: k8sClient, + Log: logger, + independentSyncPeriod: 10 * time.Minute, } ctx := &workflow.Context{ Context: context.Background(), diff --git a/pkg/controller/workflow/result.go b/pkg/controller/workflow/result.go index d5c81ed111..ec7e4235a6 100644 --- a/pkg/controller/workflow/result.go +++ b/pkg/controller/workflow/result.go @@ -7,9 +7,7 @@ import ( ) const ( - DefaultRetry = time.Second * 10 - StandaloneResourceRequeuePeriod = time.Minute * 15 - DefaultTimeout = time.Minute * 20 + DefaultRetry = time.Second * 10 ) type Result struct { diff --git a/pkg/operator/builder.go b/pkg/operator/builder.go index 6cdf9f18bc..aa60777335 100644 --- a/pkg/operator/builder.go +++ b/pkg/operator/builder.go @@ -2,6 +2,7 @@ package operator import ( "context" + "errors" "fmt" "os" "time" @@ -37,9 +38,12 @@ import ( ) const ( - DefaultAtlasDomain = "https://cloud.mongodb.com/" - DefaultSyncPeriod = 3 * time.Hour - DefaultLeaderElectionID = "06d035fb.mongodb.com" + DefaultAtlasDomain = "https://cloud.mongodb.com/" + DefaultSyncPeriod = 3 * time.Hour + DefaultIndependentSyncPeriod = 15 * time.Minute + DefaultLeaderElectionID = "06d035fb.mongodb.com" + + minimumIndependentSyncPeriod = 5 * time.Minute ) type ManagerProvider interface { @@ -56,14 +60,15 @@ type Builder struct { managerProvider ManagerProvider scheme *runtime.Scheme - config *rest.Config - namespaces []string - logger *zap.Logger - syncPeriod time.Duration - metricAddress string - probeAddress string - leaderElection bool - leaderElectionID string + config *rest.Config + namespaces []string + logger *zap.Logger + syncPeriod time.Duration + independentSyncPeriod time.Duration + metricAddress string + probeAddress string + leaderElection bool + leaderElectionID string atlasDomain string predicates []predicate.Predicate @@ -139,6 +144,11 @@ func (b *Builder) WithDeletionProtection(deletionProtection bool) *Builder { return b } +func (b *Builder) WithIndependentSyncPeriod(period time.Duration) *Builder { + b.independentSyncPeriod = period + return b +} + // WithSkipNameValidation skips name validation in controller-runtime // to prevent duplicate controller names. // @@ -152,6 +162,10 @@ func (b *Builder) WithSkipNameValidation(skip bool) *Builder { func (b *Builder) Build(ctx context.Context) (manager.Manager, error) { mergeDefaults(b) + if b.independentSyncPeriod < minimumIndependentSyncPeriod { + return nil, errors.New("wrong value for independentSyncPeriod. Value should be greater or equal to 5") + } + cacheOpts := cache.Options{ SyncPeriod: &b.syncPeriod, } @@ -233,6 +247,7 @@ func (b *Builder) Build(ctx context.Context) (manager.Manager, error) { b.predicates, b.atlasProvider, b.deletionProtection, + b.independentSyncPeriod, b.featureFlags, b.logger, ) @@ -330,6 +345,10 @@ func mergeDefaults(b *Builder) { b.syncPeriod = DefaultSyncPeriod } + if b.independentSyncPeriod == 0 { + b.independentSyncPeriod = DefaultIndependentSyncPeriod + } + if b.metricAddress == "" { b.metricAddress = "0" } diff --git a/pkg/operator/builder_test.go b/pkg/operator/builder_test.go index cbef985083..59b4120ae8 100644 --- a/pkg/operator/builder_test.go +++ b/pkg/operator/builder_test.go @@ -2,6 +2,7 @@ package operator import ( "context" + "errors" "testing" "time" @@ -35,9 +36,6 @@ type managerMock struct { client client.Client scheme *runtime.Scheme - gotHealthzCheck string - gotReadyzCheck string - opts ctrl.Options } @@ -45,7 +43,7 @@ func (m *managerMock) GetCache() cache.Cache { return &informertest.FakeInformers{} } -func (m *managerMock) Add(runnable manager.Runnable) error { +func (m *managerMock) Add(_ manager.Runnable) error { return nil } @@ -61,7 +59,7 @@ func (m *managerMock) GetScheme() *runtime.Scheme { return m.scheme } -func (m *managerMock) GetEventRecorderFor(name string) record.EventRecorder { +func (m *managerMock) GetEventRecorderFor(_ string) record.EventRecorder { return record.NewFakeRecorder(100) } @@ -73,7 +71,7 @@ func (m *managerMock) GetFieldIndexer() client.FieldIndexer { return &informertest.FakeInformers{} } -func (m *managerMock) New(config *rest.Config, options manager.Options) (manager.Manager, error) { +func (m *managerMock) New(_ *rest.Config, options manager.Options) (manager.Manager, error) { m.opts = options m.scheme = options.Scheme m.client = fake.NewClientBuilder(). @@ -83,13 +81,11 @@ func (m *managerMock) New(config *rest.Config, options manager.Options) (manager return m, nil } -func (m *managerMock) AddHealthzCheck(name string, check healthz.Checker) error { - m.gotHealthzCheck = name +func (m *managerMock) AddHealthzCheck(_ string, _ healthz.Checker) error { return nil } -func (m *managerMock) AddReadyzCheck(name string, check healthz.Checker) error { - m.gotReadyzCheck = name +func (m *managerMock) AddReadyzCheck(_ string, _ healthz.Checker) error { return nil } @@ -99,6 +95,7 @@ func TestBuildManager(t *testing.T) { expectedSyncPeriod time.Duration expectedClusterWideCache bool expectedNamespacedCache bool + expectedError error }{ "should build the manager with default values": { configure: func(b *Builder) {}, @@ -120,6 +117,7 @@ func TestBuildManager(t *testing.T) { WithNamespaces("ns1"). WithLogger(zaptest.NewLogger(t)). WithSyncPeriod(time.Hour). + WithIndependentSyncPeriod(15 * time.Minute). WithMetricAddress(":9090"). WithProbeAddress(":9091"). WithLeaderElection(true). @@ -134,6 +132,15 @@ func TestBuildManager(t *testing.T) { expectedClusterWideCache: false, expectedNamespacedCache: true, }, + "should error when independentSyncPeriod is misconfigured": { + configure: func(b *Builder) { + b.WithIndependentSyncPeriod(4 * time.Minute) + }, + expectedSyncPeriod: DefaultSyncPeriod, + expectedClusterWideCache: false, + expectedNamespacedCache: true, + expectedError: errors.New("wrong value for independentSyncPeriod. Value should be greater or equal to 5"), + }, } for name, tt := range tests { @@ -147,11 +154,13 @@ func TestBuildManager(t *testing.T) { // this is necessary for tests builder.WithSkipNameValidation(true) _, err := builder.Build(context.Background()) - require.NoError(t, err) + require.Equal(t, tt.expectedError, err) - assert.Equal(t, tt.expectedSyncPeriod, *mgrMock.opts.Cache.SyncPeriod) - assert.Equal(t, tt.expectedClusterWideCache, len(mgrMock.opts.Cache.ByObject) > 0) - assert.Equal(t, tt.expectedNamespacedCache, len(mgrMock.opts.Cache.DefaultNamespaces) > 0) + if err == nil { + assert.Equal(t, tt.expectedSyncPeriod, *mgrMock.opts.Cache.SyncPeriod) + assert.Equal(t, tt.expectedClusterWideCache, len(mgrMock.opts.Cache.ByObject) > 0) + assert.Equal(t, tt.expectedNamespacedCache, len(mgrMock.opts.Cache.DefaultNamespaces) > 0) + } }) } }