From 790cf1c935ee5294329a0443d4737adfae719a52 Mon Sep 17 00:00:00 2001 From: Reed Schalo Date: Sun, 8 Jun 2025 15:19:11 -0700 Subject: [PATCH 01/13] feat: add default singleton rate limiter to replace requeue --- singleton/controller.go | 21 +++++++++++++++++++++ singleton/suite_test.go | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 singleton/suite_test.go diff --git a/singleton/controller.go b/singleton/controller.go index d1cb1dc..146e044 100644 --- a/singleton/controller.go +++ b/singleton/controller.go @@ -36,3 +36,24 @@ func Source() source.Source { }, }) } + +// In response to Requeue: True being deprecated via: https://github.com/kubernetes-sigs/controller-runtime/pull/3107/files +// This uses a bucket and per item delay but the item will be the same because the key is the controller name.. +// This implements the same behavior as Requeue: True. +type SingletonRateLimiter struct { + rateLimiter workqueue.TypedRateLimiter[string] + key string +} + +func NewSingletonRateLimiter(controllerName string) *SingletonRateLimiter { + return &SingletonRateLimiter{ + rateLimiter: workqueue.DefaultTypedControllerRateLimiter[string](), + key: controllerName, + } +} + +// Delay requeues the item according to the rate limiter. +// Used like "RequeueAfter: controller.RateLimiter.Delay()" +func (s *SingletonRateLimiter) Delay() time.Duration { + return s.rateLimiter.When(s.key) +} diff --git a/singleton/suite_test.go b/singleton/suite_test.go new file mode 100644 index 0000000..737e890 --- /dev/null +++ b/singleton/suite_test.go @@ -0,0 +1,33 @@ +package singleton_test + +import ( + "testing" + + "github.com/awslabs/operatorpkg/singleton" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func Test(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Singleton") +} + +var _ = Describe("SingletonRateLimiter", func() { + var rateLimiter *singleton.SingletonRateLimiter + + BeforeEach(func() { + rateLimiter = singleton.NewSingletonRateLimiter("test-controller") + }) + + It("should return increasing delays on subsequent calls", func() { + firstDelay := rateLimiter.Delay() + Expect(firstDelay).To(BeNumerically(">=", 0)) + + secondDelay := rateLimiter.Delay() + Expect(secondDelay).To(BeNumerically(">=", firstDelay)) + + thirdDelay := rateLimiter.Delay() + Expect(thirdDelay).To(BeNumerically(">=", secondDelay)) + }) +}) From 65907aa5f6891349b58caf8b3c39a9ebbd5bcf71 Mon Sep 17 00:00:00 2001 From: Reed Schalo Date: Tue, 10 Jun 2025 12:03:52 -0700 Subject: [PATCH 02/13] feat: Add requeue adapter for rate-limiting --- reconciler/reconciler.go | 80 ++++++++ reconciler/reconciler_test.go | 366 ++++++++++++++++++++++++++++++++++ singleton/controller.go | 67 ++++--- singleton/suite_test.go | 144 +++++++++++-- 4 files changed, 619 insertions(+), 38 deletions(-) create mode 100644 reconciler/reconciler.go create mode 100644 reconciler/reconciler_test.go diff --git a/reconciler/reconciler.go b/reconciler/reconciler.go new file mode 100644 index 0000000..3e6c75b --- /dev/null +++ b/reconciler/reconciler.go @@ -0,0 +1,80 @@ +package reconciler + +import ( + "context" + + "k8s.io/client-go/util/workqueue" + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +// Result is a wrapper around reconcile.Result that adds RequeueWithBackoff functionality. +type Result struct { + reconcile.Result + RequeueWithBackoff bool +} + +// KeyExtractor extracts a rate limiter key from a context and reconcile.Request. +type KeyExtractor[K any] interface { + Extract(ctx context.Context, req reconcile.Request) K +} + +// RequestKeyExtractor extracts a reconcile.Request key for rate limiting. +type RequestKeyExtractor struct{} + +// Extract returns the reconcile.Request as the key. +func (e RequestKeyExtractor) Extract(ctx context.Context, req reconcile.Request) reconcile.Request { + return req +} + +// Reconciler defines the interface for standard reconcilers +type Reconciler interface { + Reconcile(ctx context.Context) (Result, error) +} + +// ReconcilerFunc is a function type that implements the Reconciler interface. +type ReconcilerFunc func(ctx context.Context) (Result, error) + +// Reconcile implements the Reconciler interface. +func (f ReconcilerFunc) Reconcile(ctx context.Context) (Result, error) { + return f(ctx) +} + +// AsReconciler creates a reconciler from a standard reconciler +func AsReconciler(reconciler Reconciler) reconcile.Reconciler { + return AsGenericReconciler( + func(ctx context.Context, req reconcile.Request) (Result, error) { + return reconciler.Reconcile(ctx) + }, + RequestKeyExtractor{}, + ) +} + +// AsGenericReconciler creates a reconciler with a specific key extractor +func AsGenericReconciler[K comparable]( + reconcileFunc func(ctx context.Context, req reconcile.Request) (Result, error), + keyExtractor KeyExtractor[K], +) reconcile.Reconciler { + return AsGenericReconcilerWithRateLimiter( + reconcileFunc, + keyExtractor, + workqueue.DefaultTypedControllerRateLimiter[K](), + ) +} + +// AsGenericReconcilerWithRateLimiter creates a reconciler with a custom rate limiter +func AsGenericReconcilerWithRateLimiter[K comparable]( + reconcileFunc func(ctx context.Context, req reconcile.Request) (Result, error), + keyExtractor KeyExtractor[K], + rateLimiter workqueue.TypedRateLimiter[K], +) reconcile.Reconciler { + return reconcile.Func(func(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { + result, err := reconcileFunc(ctx, req) + if err != nil { + return reconcile.Result{}, err + } + if result.RequeueWithBackoff { + return reconcile.Result{RequeueAfter: rateLimiter.When(keyExtractor.Extract(ctx, req))}, nil + } + return result.Result, nil + }) +} diff --git a/reconciler/reconciler_test.go b/reconciler/reconciler_test.go new file mode 100644 index 0000000..0fa9915 --- /dev/null +++ b/reconciler/reconciler_test.go @@ -0,0 +1,366 @@ +package reconciler_test + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/awslabs/operatorpkg/reconciler" + "github.com/stretchr/testify/assert" + "k8s.io/client-go/util/workqueue" + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +// MockRateLimiter is a mock implementation of workqueue.TypedRateLimiter for testing +type MockRateLimiter[K comparable] struct { + whenFunc func(K) time.Duration + numRequeues int + backoffDuration time.Duration +} + +func (m *MockRateLimiter[K]) When(key K) time.Duration { + if m.whenFunc != nil { + return m.whenFunc(key) + } + m.numRequeues++ + return m.backoffDuration +} + +func (m *MockRateLimiter[K]) NumRequeues(key K) int { + return m.numRequeues +} + +func (m *MockRateLimiter[K]) Forget(key K) { + m.numRequeues = 0 +} + +// MockKeyExtractor is a mock implementation of KeyExtractor for testing +type MockKeyExtractor[K any] struct { + extractFunc func(context.Context, reconcile.Request) K + key K +} + +func (m *MockKeyExtractor[K]) Extract(ctx context.Context, req reconcile.Request) K { + if m.extractFunc != nil { + return m.extractFunc(ctx, req) + } + return m.key +} + +// MockReconciler is a mock implementation of Reconciler for testing +type MockReconciler struct { + reconcileFunc func(context.Context) (reconciler.Result, error) + result reconciler.Result + err error +} + +func (m *MockReconciler) Reconcile(ctx context.Context) (reconciler.Result, error) { + if m.reconcileFunc != nil { + return m.reconcileFunc(ctx) + } + return m.result, m.err +} + +func TestResult(t *testing.T) { + t.Run("Basic functionality", func(t *testing.T) { + t.Run("Without backoff", func(t *testing.T) { + // Create a Result with RequeueWithBackoff = false and a specific RequeueAfter duration + duration := 5 * time.Second + result := reconciler.Result{ + Result: reconcile.Result{ + RequeueAfter: duration, + }, + RequeueWithBackoff: false, + } + + // Verify that the wrapped Result has the same RequeueAfter value + assert.Equal(t, duration, result.RequeueAfter) + assert.False(t, result.RequeueWithBackoff) + }) + + t.Run("With requeue", func(t *testing.T) { + // Create a Result with Requeue = true and RequeueWithBackoff = false + result := reconciler.Result{ + Result: reconcile.Result{ + Requeue: true, + }, + RequeueWithBackoff: false, + } + + // Verify that the wrapped Result has Requeue = true + assert.True(t, result.Requeue) + assert.False(t, result.RequeueWithBackoff) + }) + }) +} + +func TestAsGenericReconciler(t *testing.T) { + t.Run("With string key", func(t *testing.T) { + t.Run("Result with backoff", func(t *testing.T) { + // Create a mock rate limiter + backoffDuration := 10 * time.Second + mockRateLimiter := &MockRateLimiter[string]{ + backoffDuration: backoffDuration, + } + + // Create a mock reconcile function that returns a Result with RequeueWithBackoff = true + reconcileFunc := func(ctx context.Context, req reconcile.Request) (reconciler.Result, error) { + return reconciler.Result{ + Result: reconcile.Result{}, + RequeueWithBackoff: true, + }, nil + } + + // Create a mock key extractor + testKey := "test-controller" + mockKeyExtractor := &MockKeyExtractor[string]{ + key: testKey, + } + + // Create the reconciler adapter + adapter := reconciler.AsGenericReconcilerWithRateLimiter( + reconcileFunc, + mockKeyExtractor, + mockRateLimiter, + ) + + // Call the adapter + ctx := context.Background() + req := reconcile.Request{} + result, err := adapter.Reconcile(ctx, req) + + // Verify the result + assert.NoError(t, err) + assert.Equal(t, backoffDuration, result.RequeueAfter) + assert.False(t, result.Requeue) + assert.Equal(t, 1, mockRateLimiter.NumRequeues(testKey)) + }) + + t.Run("Multiple backoffs", func(t *testing.T) { + // Create a mock rate limiter that increases backoff duration + initialBackoff := 1 * time.Second + mockLimiter := &MockRateLimiter[string]{} + mockLimiter.whenFunc = func(key string) time.Duration { + mockLimiter.numRequeues++ + return time.Duration(mockLimiter.numRequeues) * initialBackoff + } + + // Create a mock reconcile function that returns a Result with RequeueWithBackoff = true + reconcileFunc := func(ctx context.Context, req reconcile.Request) (reconciler.Result, error) { + return reconciler.Result{ + Result: reconcile.Result{}, + RequeueWithBackoff: true, + }, nil + } + + // Create a mock key extractor + testKey := "test-controller" + mockKeyExtractor := &MockKeyExtractor[string]{ + key: testKey, + } + + // Create the reconciler adapter + adapter := reconciler.AsGenericReconcilerWithRateLimiter( + reconcileFunc, + mockKeyExtractor, + mockLimiter, + ) + + // Call the adapter multiple times + ctx := context.Background() + req := reconcile.Request{} + + // First call + result1, err := adapter.Reconcile(ctx, req) + assert.NoError(t, err) + assert.Equal(t, 1*initialBackoff, result1.RequeueAfter) + + // Second call + result2, err := adapter.Reconcile(ctx, req) + assert.NoError(t, err) + assert.Equal(t, 2*initialBackoff, result2.RequeueAfter) + + // Third call + result3, err := adapter.Reconcile(ctx, req) + assert.NoError(t, err) + assert.Equal(t, 3*initialBackoff, result3.RequeueAfter) + }) + }) + + t.Run("With request key", func(t *testing.T) { + t.Run("Result with backoff", func(t *testing.T) { + // Create a mock rate limiter + backoffDuration := 10 * time.Second + mockRateLimiter := &MockRateLimiter[reconcile.Request]{ + backoffDuration: backoffDuration, + } + + // Create a mock reconcile function that returns a Result with RequeueWithBackoff = true + reconcileFunc := func(ctx context.Context, req reconcile.Request) (reconciler.Result, error) { + return reconciler.Result{ + Result: reconcile.Result{}, + RequeueWithBackoff: true, + }, nil + } + + // Create a mock key extractor + testReq := reconcile.Request{} + mockKeyExtractor := &MockKeyExtractor[reconcile.Request]{ + key: testReq, + } + + // Create the reconciler adapter + adapter := reconciler.AsGenericReconcilerWithRateLimiter( + reconcileFunc, + mockKeyExtractor, + mockRateLimiter, + ) + + // Call the adapter + ctx := context.Background() + result, err := adapter.Reconcile(ctx, testReq) + + // Verify the result + assert.NoError(t, err) + assert.Equal(t, backoffDuration, result.RequeueAfter) + assert.False(t, result.Requeue) + assert.Equal(t, 1, mockRateLimiter.NumRequeues(testReq)) + }) + }) + + t.Run("Error handling", func(t *testing.T) { + t.Run("Error propagation", func(t *testing.T) { + // Create a mock reconcile function that returns an error + expectedErr := errors.New("test error") + reconcileFunc := func(ctx context.Context, req reconcile.Request) (reconciler.Result, error) { + return reconciler.Result{}, expectedErr + } + + // Create a mock key extractor + mockKeyExtractor := &MockKeyExtractor[string]{ + key: "test-controller", + } + + // Create the reconciler adapter + adapter := reconciler.AsGenericReconciler( + reconcileFunc, + mockKeyExtractor, + ) + + // Call the adapter + ctx := context.Background() + req := reconcile.Request{} + _, err := adapter.Reconcile(ctx, req) + + // Verify that the error is propagated + assert.Error(t, err) + assert.Equal(t, expectedErr, err) + }) + }) + + t.Run("No backoff", func(t *testing.T) { + t.Run("Result without backoff", func(t *testing.T) { + // Create a mock reconcile function that returns a Result with RequeueWithBackoff = false + expectedRequeueAfter := 5 * time.Second + reconcileFunc := func(ctx context.Context, req reconcile.Request) (reconciler.Result, error) { + return reconciler.Result{ + Result: reconcile.Result{ + RequeueAfter: expectedRequeueAfter, + }, + RequeueWithBackoff: false, + }, nil + } + + // Create a mock key extractor + mockKeyExtractor := &MockKeyExtractor[string]{ + key: "test-controller", + } + + // Create the reconciler adapter + adapter := reconciler.AsGenericReconciler( + reconcileFunc, + mockKeyExtractor, + ) + + // Call the adapter + ctx := context.Background() + req := reconcile.Request{} + result, err := adapter.Reconcile(ctx, req) + + // Verify the result + assert.NoError(t, err) + assert.Equal(t, expectedRequeueAfter, result.RequeueAfter) + assert.False(t, result.Requeue) + }) + }) +} + +func TestKeyExtractors(t *testing.T) { + t.Run("RequestKeyExtractor", func(t *testing.T) { + t.Run("Extract request key", func(t *testing.T) { + // Create a request key extractor + extractor := reconciler.RequestKeyExtractor{} + + // Extract the key + ctx := context.Background() + req := reconcile.Request{} + key := extractor.Extract(ctx, req) + + // Verify the key + assert.Equal(t, req, key) + }) + }) +} + +func TestAsReconciler(t *testing.T) { + t.Run("Standard reconciler adapter", func(t *testing.T) { + // Create a mock reconciler + mockReconciler := &MockReconciler{ + result: reconciler.Result{ + Result: reconcile.Result{ + RequeueAfter: 5 * time.Second, + }, + RequeueWithBackoff: false, + }, + } + + // Create the reconciler adapter + adapter := reconciler.AsReconciler(mockReconciler) + + // Call the adapter + ctx := context.Background() + req := reconcile.Request{} + result, err := adapter.Reconcile(ctx, req) + + // Verify the result + assert.NoError(t, err) + assert.Equal(t, 5*time.Second, result.RequeueAfter) + assert.False(t, result.Requeue) + }) +} + +func TestReconcilerFunc(t *testing.T) { + t.Run("Implementation", func(t *testing.T) { + // Create a ReconcilerFunc + expectedResult := reconciler.Result{ + Result: reconcile.Result{ + RequeueAfter: 5 * time.Second, + }, + RequeueWithBackoff: false, + } + expectedErr := errors.New("test error") + + reconcileFunc := reconciler.ReconcilerFunc(func(ctx context.Context) (reconciler.Result, error) { + return expectedResult, expectedErr + }) + + // Call the function + ctx := context.Background() + result, err := reconcileFunc.Reconcile(ctx) + + // Verify the result + assert.Equal(t, expectedResult, result) + assert.Equal(t, expectedErr, err) + }) +} diff --git a/singleton/controller.go b/singleton/controller.go index 146e044..a80a552 100644 --- a/singleton/controller.go +++ b/singleton/controller.go @@ -4,6 +4,7 @@ import ( "context" "time" + "github.com/awslabs/operatorpkg/reconciler" "k8s.io/client-go/util/workqueue" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" @@ -17,43 +18,57 @@ const ( RequeueImmediately = 1 * time.Nanosecond ) +// Result is a type alias for reconciler.Result for backward compatibility +type Result = reconciler.Result + +// Reconciler defines the interface for singleton reconcilers type Reconciler interface { - Reconcile(ctx context.Context) (reconcile.Result, error) + Name() string + Reconcile(ctx context.Context) (Result, error) } -func AsReconciler(reconciler Reconciler) reconcile.Reconciler { - return reconcile.Func(func(ctx context.Context, r reconcile.Request) (reconcile.Result, error) { - return reconciler.Reconcile(ctx) - }) +// StringKeyExtractor extracts the controller name as the rate limiter key +type StringKeyExtractor struct { + reconciler Reconciler } -func Source() source.Source { - eventSource := make(chan event.GenericEvent, 1) - eventSource <- event.GenericEvent{} - return source.Channel(eventSource, handler.Funcs{ - GenericFunc: func(_ context.Context, _ event.GenericEvent, queue workqueue.TypedRateLimitingInterface[reconcile.Request]) { - queue.Add(reconcile.Request{}) - }, - }) +// Extract returns the controller name as the key +func (e StringKeyExtractor) Extract(ctx context.Context, req reconcile.Request) string { + return e.reconciler.Name() } // In response to Requeue: True being deprecated via: https://github.com/kubernetes-sigs/controller-runtime/pull/3107/files -// This uses a bucket and per item delay but the item will be the same because the key is the controller name.. +// This uses a bucket and per item delay but the item will be the same because the key is the controller name. // This implements the same behavior as Requeue: True. -type SingletonRateLimiter struct { - rateLimiter workqueue.TypedRateLimiter[string] - key string + +// AsReconciler creates a controller-runtime reconciler from a singleton reconciler +func AsReconciler(rec Reconciler) reconcile.Reconciler { + return reconciler.AsGenericReconciler( + func(ctx context.Context, req reconcile.Request) (Result, error) { + return rec.Reconcile(ctx) + }, + StringKeyExtractor{reconciler: rec}, + ) } -func NewSingletonRateLimiter(controllerName string) *SingletonRateLimiter { - return &SingletonRateLimiter{ - rateLimiter: workqueue.DefaultTypedControllerRateLimiter[string](), - key: controllerName, - } +// AsReconcilerWithRateLimiter creates a controller-runtime reconciler with a custom rate limiter +func AsReconcilerWithRateLimiter(rec Reconciler, rateLimiter workqueue.TypedRateLimiter[string]) reconcile.Reconciler { + return reconciler.AsGenericReconcilerWithRateLimiter( + func(ctx context.Context, req reconcile.Request) (Result, error) { + return rec.Reconcile(ctx) + }, + StringKeyExtractor{reconciler: rec}, + rateLimiter, + ) } -// Delay requeues the item according to the rate limiter. -// Used like "RequeueAfter: controller.RateLimiter.Delay()" -func (s *SingletonRateLimiter) Delay() time.Duration { - return s.rateLimiter.When(s.key) +// Source creates a source for singleton controllers +func Source() source.Source { + eventSource := make(chan event.GenericEvent, 1) + eventSource <- event.GenericEvent{} + return source.Channel(eventSource, handler.Funcs{ + GenericFunc: func(_ context.Context, _ event.GenericEvent, queue workqueue.TypedRateLimitingInterface[reconcile.Request]) { + queue.Add(reconcile.Request{}) + }, + }) } diff --git a/singleton/suite_test.go b/singleton/suite_test.go index 737e890..0e10b3c 100644 --- a/singleton/suite_test.go +++ b/singleton/suite_test.go @@ -1,11 +1,16 @@ package singleton_test import ( + "context" + "errors" "testing" + "time" + "github.com/awslabs/operatorpkg/reconciler" "github.com/awslabs/operatorpkg/singleton" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "sigs.k8s.io/controller-runtime/pkg/reconcile" ) func Test(t *testing.T) { @@ -13,21 +18,136 @@ func Test(t *testing.T) { RunSpecs(t, "Singleton") } -var _ = Describe("SingletonRateLimiter", func() { - var rateLimiter *singleton.SingletonRateLimiter +var ( + mockReconciler *MockReconciler + rec singleton.Reconciler +) - BeforeEach(func() { - rateLimiter = singleton.NewSingletonRateLimiter("test-controller") - }) +// MockReconciler for testing +type MockReconciler struct { + name string + result reconciler.Result + err error +} + +func (m *MockReconciler) Name() string { + return m.name +} + +func (m *MockReconciler) Reconcile(ctx context.Context) (reconciler.Result, error) { + return m.result, m.err +} + +var _ = Describe("Singleton Controller", func() { + Context("AsReconciler with rate limiting", func() { + BeforeEach(func() { + mockReconciler = &MockReconciler{ + name: "test-controller", + } + }) + + Context("when RequeueWithBackoff is false", func() { + It("should return the original result without backoff", func() { + mockReconciler.result = reconciler.Result{ + RequeueWithBackoff: false, + } + rec = mockReconciler + result, err := rec.Reconcile(context.Background()) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(BeZero()) + }) + It("should return the original result without backoff when RequeueWithBackoff is not set", func() { + mockReconciler.result = reconciler.Result{} + rec = mockReconciler + result, err := rec.Reconcile(context.Background()) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(BeZero()) + }) + }) + + Context("when RequeueWithBackoff is true", func() { + BeforeEach(func() { + mockReconciler.result = reconciler.Result{ + RequeueWithBackoff: true, + } + rec = mockReconciler + }) + + It("should return a result with RequeueAfter set", func() { + result, err := rec.Reconcile(context.Background()) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(BeNumerically(">=", 0)) + }) + + It("should use controller name for rate limiting", func() { + // Test with different controller names to ensure they're handled independently + controller1 := &MockReconciler{ + name: "controller-1", + result: reconciler.Result{RequeueWithBackoff: true}, + } + controller2 := &MockReconciler{ + name: "controller-2", + result: reconciler.Result{RequeueWithBackoff: true}, + } + + reconciler1 := singleton.AsReconciler(controller1) + reconciler2 := singleton.AsReconciler(controller2) + + // Each controller should get its own rate limiting + result1, err1 := reconciler1.Reconcile(context.Background(), reconcile.Request{}) + result2, err2 := reconciler2.Reconcile(context.Background(), reconcile.Request{}) + + Expect(err1).NotTo(HaveOccurred()) + Expect(err2).NotTo(HaveOccurred()) + Expect(result1.RequeueAfter).To(BeNumerically(">=", 0)) + Expect(result2.RequeueAfter).To(BeNumerically(">=", 0)) + }) + + It("should implement exponential backoff on repeated calls", func() { + // Multiple calls to the same controller should show increasing delays + delays := make([]time.Duration, 5) + + for i := 0; i < 5; i++ { + result, err := rec.Reconcile(context.Background()) + Expect(err).NotTo(HaveOccurred()) + delays[i] = result.RequeueAfter + } + + // Verify generally increasing pattern (allowing for some variance in rate limiting) + for i := 1; i < len(delays); i++ { + Expect(delays[i]).To(BeNumerically(">=", delays[i-1]), + "Delay at index %d (%v) should be >= delay at index %d (%v)", + i, delays[i], i-1, delays[i-1]) + } + }) + }) + + Context("when reconciler returns an error", func() { + BeforeEach(func() { + mockReconciler.result = reconciler.Result{RequeueWithBackoff: true} + mockReconciler.err = errors.New("test error") + rec = mockReconciler + }) - It("should return increasing delays on subsequent calls", func() { - firstDelay := rateLimiter.Delay() - Expect(firstDelay).To(BeNumerically(">=", 0)) + It("should return the error without processing backoff", func() { + result, err := rec.Reconcile(context.Background()) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(Equal("test error")) + Expect(result.RequeueAfter).To(BeZero()) + }) + }) - secondDelay := rateLimiter.Delay() - Expect(secondDelay).To(BeNumerically(">=", firstDelay)) + Context("integration with RequeueImmediately constant", func() { + It("should work with immediate requeue pattern", func() { + mockReconciler.result = reconciler.Result{ + Result: reconcile.Result{RequeueAfter: singleton.RequeueImmediately}, + } + rec = mockReconciler - thirdDelay := rateLimiter.Delay() - Expect(thirdDelay).To(BeNumerically(">=", secondDelay)) + result, err := rec.Reconcile(context.Background()) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(singleton.RequeueImmediately)) + }) + }) }) }) From b44f3688494407665091b094632799cf256c3bae Mon Sep 17 00:00:00 2001 From: Reed Schalo Date: Tue, 10 Jun 2025 12:08:32 -0700 Subject: [PATCH 03/13] clean up --- reconciler/{reconciler_test.go => suite_test.go} | 0 singleton/controller.go | 9 +++------ 2 files changed, 3 insertions(+), 6 deletions(-) rename reconciler/{reconciler_test.go => suite_test.go} (100%) diff --git a/reconciler/reconciler_test.go b/reconciler/suite_test.go similarity index 100% rename from reconciler/reconciler_test.go rename to reconciler/suite_test.go diff --git a/singleton/controller.go b/singleton/controller.go index a80a552..fc0285a 100644 --- a/singleton/controller.go +++ b/singleton/controller.go @@ -18,13 +18,10 @@ const ( RequeueImmediately = 1 * time.Nanosecond ) -// Result is a type alias for reconciler.Result for backward compatibility -type Result = reconciler.Result - // Reconciler defines the interface for singleton reconcilers type Reconciler interface { Name() string - Reconcile(ctx context.Context) (Result, error) + Reconcile(ctx context.Context) (reconciler.Result, error) } // StringKeyExtractor extracts the controller name as the rate limiter key @@ -44,7 +41,7 @@ func (e StringKeyExtractor) Extract(ctx context.Context, req reconcile.Request) // AsReconciler creates a controller-runtime reconciler from a singleton reconciler func AsReconciler(rec Reconciler) reconcile.Reconciler { return reconciler.AsGenericReconciler( - func(ctx context.Context, req reconcile.Request) (Result, error) { + func(ctx context.Context, req reconcile.Request) (reconciler.Result, error) { return rec.Reconcile(ctx) }, StringKeyExtractor{reconciler: rec}, @@ -54,7 +51,7 @@ func AsReconciler(rec Reconciler) reconcile.Reconciler { // AsReconcilerWithRateLimiter creates a controller-runtime reconciler with a custom rate limiter func AsReconcilerWithRateLimiter(rec Reconciler, rateLimiter workqueue.TypedRateLimiter[string]) reconcile.Reconciler { return reconciler.AsGenericReconcilerWithRateLimiter( - func(ctx context.Context, req reconcile.Request) (Result, error) { + func(ctx context.Context, req reconcile.Request) (reconciler.Result, error) { return rec.Reconcile(ctx) }, StringKeyExtractor{reconciler: rec}, From 956678b2673ac6826591215e88401d5a001ca963 Mon Sep 17 00:00:00 2001 From: Reed Schalo Date: Tue, 10 Jun 2025 12:15:41 -0700 Subject: [PATCH 04/13] fix presubmit --- go.mod | 2 ++ reconciler/suite_test.go | 19 +++++++++---------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/go.mod b/go.mod index 980a662..c21dad1 100644 --- a/go.mod +++ b/go.mod @@ -12,6 +12,7 @@ require ( github.com/prometheus/client_golang v1.22.0 github.com/prometheus/client_model v0.6.2 github.com/samber/lo v1.50.0 + github.com/stretchr/testify v1.10.0 go.uber.org/multierr v1.11.0 go.uber.org/zap v1.27.0 golang.org/x/time v0.11.0 @@ -50,6 +51,7 @@ require ( github.com/modern-go/reflect2 v1.0.2 // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/pkg/errors v0.9.1 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect github.com/prometheus/common v0.62.0 // indirect github.com/prometheus/procfs v0.15.1 // indirect github.com/spf13/pflag v1.0.5 // indirect diff --git a/reconciler/suite_test.go b/reconciler/suite_test.go index 0fa9915..d78892d 100644 --- a/reconciler/suite_test.go +++ b/reconciler/suite_test.go @@ -8,14 +8,13 @@ import ( "github.com/awslabs/operatorpkg/reconciler" "github.com/stretchr/testify/assert" - "k8s.io/client-go/util/workqueue" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) // MockRateLimiter is a mock implementation of workqueue.TypedRateLimiter for testing type MockRateLimiter[K comparable] struct { - whenFunc func(K) time.Duration - numRequeues int + whenFunc func(K) time.Duration + numRequeues int backoffDuration time.Duration } @@ -107,7 +106,7 @@ func TestAsGenericReconciler(t *testing.T) { // Create a mock reconcile function that returns a Result with RequeueWithBackoff = true reconcileFunc := func(ctx context.Context, req reconcile.Request) (reconciler.Result, error) { return reconciler.Result{ - Result: reconcile.Result{}, + Result: reconcile.Result{}, RequeueWithBackoff: true, }, nil } @@ -149,7 +148,7 @@ func TestAsGenericReconciler(t *testing.T) { // Create a mock reconcile function that returns a Result with RequeueWithBackoff = true reconcileFunc := func(ctx context.Context, req reconcile.Request) (reconciler.Result, error) { return reconciler.Result{ - Result: reconcile.Result{}, + Result: reconcile.Result{}, RequeueWithBackoff: true, }, nil } @@ -170,17 +169,17 @@ func TestAsGenericReconciler(t *testing.T) { // Call the adapter multiple times ctx := context.Background() req := reconcile.Request{} - + // First call result1, err := adapter.Reconcile(ctx, req) assert.NoError(t, err) assert.Equal(t, 1*initialBackoff, result1.RequeueAfter) - + // Second call result2, err := adapter.Reconcile(ctx, req) assert.NoError(t, err) assert.Equal(t, 2*initialBackoff, result2.RequeueAfter) - + // Third call result3, err := adapter.Reconcile(ctx, req) assert.NoError(t, err) @@ -199,7 +198,7 @@ func TestAsGenericReconciler(t *testing.T) { // Create a mock reconcile function that returns a Result with RequeueWithBackoff = true reconcileFunc := func(ctx context.Context, req reconcile.Request) (reconciler.Result, error) { return reconciler.Result{ - Result: reconcile.Result{}, + Result: reconcile.Result{}, RequeueWithBackoff: true, }, nil } @@ -350,7 +349,7 @@ func TestReconcilerFunc(t *testing.T) { RequeueWithBackoff: false, } expectedErr := errors.New("test error") - + reconcileFunc := reconciler.ReconcilerFunc(func(ctx context.Context) (reconciler.Result, error) { return expectedResult, expectedErr }) From ab74bfcebf0ee258133ccfb342880153c569254c Mon Sep 17 00:00:00 2001 From: Reed Schalo Date: Wed, 23 Jul 2025 15:04:08 -0700 Subject: [PATCH 05/13] pr responses --- go.mod | 2 - reconciler/reconciler.go | 49 ++--- reconciler/suite_test.go | 410 +++++++++++++-------------------------- singleton/controller.go | 42 ++-- singleton/suite_test.go | 59 ++++-- 5 files changed, 211 insertions(+), 351 deletions(-) diff --git a/go.mod b/go.mod index 65ae277..ee3b0db 100644 --- a/go.mod +++ b/go.mod @@ -12,7 +12,6 @@ require ( github.com/prometheus/client_golang v1.22.0 github.com/prometheus/client_model v0.6.2 github.com/samber/lo v1.50.0 - github.com/stretchr/testify v1.10.0 go.uber.org/multierr v1.11.0 go.uber.org/zap v1.27.0 golang.org/x/time v0.12.0 @@ -51,7 +50,6 @@ require ( github.com/modern-go/reflect2 v1.0.2 // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/pkg/errors v0.9.1 // indirect - github.com/pmezard/go-difflib v1.0.0 // indirect github.com/prometheus/common v0.62.0 // indirect github.com/prometheus/procfs v0.15.1 // indirect github.com/spf13/pflag v1.0.5 // indirect diff --git a/reconciler/reconciler.go b/reconciler/reconciler.go index 3e6c75b..0052910 100644 --- a/reconciler/reconciler.go +++ b/reconciler/reconciler.go @@ -13,19 +13,6 @@ type Result struct { RequeueWithBackoff bool } -// KeyExtractor extracts a rate limiter key from a context and reconcile.Request. -type KeyExtractor[K any] interface { - Extract(ctx context.Context, req reconcile.Request) K -} - -// RequestKeyExtractor extracts a reconcile.Request key for rate limiting. -type RequestKeyExtractor struct{} - -// Extract returns the reconcile.Request as the key. -func (e RequestKeyExtractor) Extract(ctx context.Context, req reconcile.Request) reconcile.Request { - return req -} - // Reconciler defines the interface for standard reconcilers type Reconciler interface { Reconcile(ctx context.Context) (Result, error) @@ -41,39 +28,27 @@ func (f ReconcilerFunc) Reconcile(ctx context.Context) (Result, error) { // AsReconciler creates a reconciler from a standard reconciler func AsReconciler(reconciler Reconciler) reconcile.Reconciler { - return AsGenericReconciler( - func(ctx context.Context, req reconcile.Request) (Result, error) { - return reconciler.Reconcile(ctx) - }, - RequestKeyExtractor{}, + return AsReconcilerWithRateLimiter( + reconciler, + workqueue.DefaultTypedControllerRateLimiter[reconcile.Request](), ) } -// AsGenericReconciler creates a reconciler with a specific key extractor -func AsGenericReconciler[K comparable]( - reconcileFunc func(ctx context.Context, req reconcile.Request) (Result, error), - keyExtractor KeyExtractor[K], -) reconcile.Reconciler { - return AsGenericReconcilerWithRateLimiter( - reconcileFunc, - keyExtractor, - workqueue.DefaultTypedControllerRateLimiter[K](), - ) -} - -// AsGenericReconcilerWithRateLimiter creates a reconciler with a custom rate limiter -func AsGenericReconcilerWithRateLimiter[K comparable]( - reconcileFunc func(ctx context.Context, req reconcile.Request) (Result, error), - keyExtractor KeyExtractor[K], - rateLimiter workqueue.TypedRateLimiter[K], +// AsReconcilerWithRateLimiter creates a reconciler with a specific key extractor +func AsReconcilerWithRateLimiter( + reconciler Reconciler, + rateLimiter workqueue.TypedRateLimiter[reconcile.Request], ) reconcile.Reconciler { return reconcile.Func(func(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { - result, err := reconcileFunc(ctx, req) + result, err := reconciler.Reconcile(ctx) if err != nil { return reconcile.Result{}, err } if result.RequeueWithBackoff { - return reconcile.Result{RequeueAfter: rateLimiter.When(keyExtractor.Extract(ctx, req))}, nil + return reconcile.Result{RequeueAfter: rateLimiter.When(req)}, nil + } + if result.RequeueAfter > 0 { + return reconcile.Result{RequeueAfter: result.RequeueAfter}, nil } return result.Result, nil }) diff --git a/reconciler/suite_test.go b/reconciler/suite_test.go index d78892d..9754aba 100644 --- a/reconciler/suite_test.go +++ b/reconciler/suite_test.go @@ -7,10 +7,16 @@ import ( "time" "github.com/awslabs/operatorpkg/reconciler" - "github.com/stretchr/testify/assert" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) +func Test(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Reconciler") +} + // MockRateLimiter is a mock implementation of workqueue.TypedRateLimiter for testing type MockRateLimiter[K comparable] struct { whenFunc func(K) time.Duration @@ -34,19 +40,6 @@ func (m *MockRateLimiter[K]) Forget(key K) { m.numRequeues = 0 } -// MockKeyExtractor is a mock implementation of KeyExtractor for testing -type MockKeyExtractor[K any] struct { - extractFunc func(context.Context, reconcile.Request) K - key K -} - -func (m *MockKeyExtractor[K]) Extract(ctx context.Context, req reconcile.Request) K { - if m.extractFunc != nil { - return m.extractFunc(ctx, req) - } - return m.key -} - // MockReconciler is a mock implementation of Reconciler for testing type MockReconciler struct { reconcileFunc func(context.Context) (reconciler.Result, error) @@ -61,226 +54,137 @@ func (m *MockReconciler) Reconcile(ctx context.Context) (reconciler.Result, erro return m.result, m.err } -func TestResult(t *testing.T) { - t.Run("Basic functionality", func(t *testing.T) { - t.Run("Without backoff", func(t *testing.T) { - // Create a Result with RequeueWithBackoff = false and a specific RequeueAfter duration - duration := 5 * time.Second - result := reconciler.Result{ - Result: reconcile.Result{ - RequeueAfter: duration, - }, - RequeueWithBackoff: false, - } - - // Verify that the wrapped Result has the same RequeueAfter value - assert.Equal(t, duration, result.RequeueAfter) - assert.False(t, result.RequeueWithBackoff) - }) - - t.Run("With requeue", func(t *testing.T) { - // Create a Result with Requeue = true and RequeueWithBackoff = false - result := reconciler.Result{ - Result: reconcile.Result{ - Requeue: true, - }, - RequeueWithBackoff: false, - } +var _ = Describe("Reconciler", func() { + Describe("Result", func() { + Context("basic functionality", func() { + It("should handle results without backoff", func() { + // Create a Result with RequeueWithBackoff = false and a specific RequeueAfter duration + duration := 5 * time.Second + result := reconciler.Result{ + Result: reconcile.Result{ + RequeueAfter: duration, + }, + RequeueWithBackoff: false, + } - // Verify that the wrapped Result has Requeue = true - assert.True(t, result.Requeue) - assert.False(t, result.RequeueWithBackoff) + // Verify that the wrapped Result has the same RequeueAfter value + Expect(result.RequeueAfter).To(Equal(duration)) + Expect(result.RequeueWithBackoff).To(BeFalse()) + }) }) }) -} - -func TestAsGenericReconciler(t *testing.T) { - t.Run("With string key", func(t *testing.T) { - t.Run("Result with backoff", func(t *testing.T) { - // Create a mock rate limiter - backoffDuration := 10 * time.Second - mockRateLimiter := &MockRateLimiter[string]{ - backoffDuration: backoffDuration, - } - // Create a mock reconcile function that returns a Result with RequeueWithBackoff = true - reconcileFunc := func(ctx context.Context, req reconcile.Request) (reconciler.Result, error) { - return reconciler.Result{ - Result: reconcile.Result{}, - RequeueWithBackoff: true, - }, nil - } - - // Create a mock key extractor - testKey := "test-controller" - mockKeyExtractor := &MockKeyExtractor[string]{ - key: testKey, - } - - // Create the reconciler adapter - adapter := reconciler.AsGenericReconcilerWithRateLimiter( - reconcileFunc, - mockKeyExtractor, - mockRateLimiter, - ) - - // Call the adapter - ctx := context.Background() - req := reconcile.Request{} - result, err := adapter.Reconcile(ctx, req) - - // Verify the result - assert.NoError(t, err) - assert.Equal(t, backoffDuration, result.RequeueAfter) - assert.False(t, result.Requeue) - assert.Equal(t, 1, mockRateLimiter.NumRequeues(testKey)) + Describe("AsReconciler with rate limiting", func() { + Context("when RequeueWithBackoff is false", func() { + It("should return the original result without backoff", func() { + // Create a mock reconciler + mockReconciler := &MockReconciler{ + result: reconciler.Result{ + Result: reconcile.Result{ + RequeueAfter: 5 * time.Second, + }, + RequeueWithBackoff: false, + }, + } + + // Create the reconciler adapter + adapter := reconciler.AsReconciler(mockReconciler) + + // Call the adapter + ctx := context.Background() + req := reconcile.Request{} + result, err := adapter.Reconcile(ctx, req) + + // Verify the result + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(5 * time.Second)) + }) + + It("should return the original result without backoff when RequeueWithBackoff is not set", func() { + mockReconciler := &MockReconciler{ + result: reconciler.Result{}, + } + + // Create the reconciler adapter + adapter := reconciler.AsReconciler(mockReconciler) + + // Call the adapter + ctx := context.Background() + req := reconcile.Request{} + result, err := adapter.Reconcile(ctx, req) + + // Verify the result + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(BeZero()) + }) }) - t.Run("Multiple backoffs", func(t *testing.T) { - // Create a mock rate limiter that increases backoff duration - initialBackoff := 1 * time.Second - mockLimiter := &MockRateLimiter[string]{} - mockLimiter.whenFunc = func(key string) time.Duration { - mockLimiter.numRequeues++ - return time.Duration(mockLimiter.numRequeues) * initialBackoff - } - - // Create a mock reconcile function that returns a Result with RequeueWithBackoff = true - reconcileFunc := func(ctx context.Context, req reconcile.Request) (reconciler.Result, error) { - return reconciler.Result{ - Result: reconcile.Result{}, - RequeueWithBackoff: true, - }, nil - } - - // Create a mock key extractor - testKey := "test-controller" - mockKeyExtractor := &MockKeyExtractor[string]{ - key: testKey, - } - - // Create the reconciler adapter - adapter := reconciler.AsGenericReconcilerWithRateLimiter( - reconcileFunc, - mockKeyExtractor, - mockLimiter, - ) + Context("when RequeueWithBackoff is true", func() { + It("should return a result with RequeueAfter set", func() { + // Create a mock reconciler that returns RequeueWithBackoff = true + mockReconciler := &MockReconciler{ + result: reconciler.Result{ + Result: reconcile.Result{}, + RequeueWithBackoff: true, + }, + } - // Call the adapter multiple times - ctx := context.Background() - req := reconcile.Request{} + // Create the reconciler adapter + adapter := reconciler.AsReconciler(mockReconciler) - // First call - result1, err := adapter.Reconcile(ctx, req) - assert.NoError(t, err) - assert.Equal(t, 1*initialBackoff, result1.RequeueAfter) + // Call the adapter + ctx := context.Background() + req := reconcile.Request{} + result, err := adapter.Reconcile(ctx, req) - // Second call - result2, err := adapter.Reconcile(ctx, req) - assert.NoError(t, err) - assert.Equal(t, 2*initialBackoff, result2.RequeueAfter) + // Verify the result - should have some backoff duration + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(BeNumerically(">", 0)) + }) + }) - // Third call - result3, err := adapter.Reconcile(ctx, req) - assert.NoError(t, err) - assert.Equal(t, 3*initialBackoff, result3.RequeueAfter) + Context("when reconciler returns an error", func() { + It("should return the error without processing backoff", func() { + // Create a mock reconciler that returns an error + expectedErr := errors.New("test error") + mockReconciler := &MockReconciler{ + result: reconciler.Result{RequeueWithBackoff: true}, + err: expectedErr, + } + + // Create the reconciler adapter + adapter := reconciler.AsReconciler(mockReconciler) + + // Call the adapter + ctx := context.Background() + req := reconcile.Request{} + result, err := adapter.Reconcile(ctx, req) + + // Verify that the error is propagated + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(expectedErr)) + Expect(result.RequeueAfter).To(BeZero()) + }) }) }) - t.Run("With request key", func(t *testing.T) { - t.Run("Result with backoff", func(t *testing.T) { - // Create a mock rate limiter + Describe("AsReconcilerWithRateLimiter", func() { + It("should use custom rate limiter for backoff", func() { backoffDuration := 10 * time.Second mockRateLimiter := &MockRateLimiter[reconcile.Request]{ backoffDuration: backoffDuration, } - // Create a mock reconcile function that returns a Result with RequeueWithBackoff = true - reconcileFunc := func(ctx context.Context, req reconcile.Request) (reconciler.Result, error) { - return reconciler.Result{ + // Create a mock reconciler that returns RequeueWithBackoff = true + mockReconciler := &MockReconciler{ + result: reconciler.Result{ Result: reconcile.Result{}, RequeueWithBackoff: true, - }, nil - } - - // Create a mock key extractor - testReq := reconcile.Request{} - mockKeyExtractor := &MockKeyExtractor[reconcile.Request]{ - key: testReq, - } - - // Create the reconciler adapter - adapter := reconciler.AsGenericReconcilerWithRateLimiter( - reconcileFunc, - mockKeyExtractor, - mockRateLimiter, - ) - - // Call the adapter - ctx := context.Background() - result, err := adapter.Reconcile(ctx, testReq) - - // Verify the result - assert.NoError(t, err) - assert.Equal(t, backoffDuration, result.RequeueAfter) - assert.False(t, result.Requeue) - assert.Equal(t, 1, mockRateLimiter.NumRequeues(testReq)) - }) - }) - - t.Run("Error handling", func(t *testing.T) { - t.Run("Error propagation", func(t *testing.T) { - // Create a mock reconcile function that returns an error - expectedErr := errors.New("test error") - reconcileFunc := func(ctx context.Context, req reconcile.Request) (reconciler.Result, error) { - return reconciler.Result{}, expectedErr - } - - // Create a mock key extractor - mockKeyExtractor := &MockKeyExtractor[string]{ - key: "test-controller", - } - - // Create the reconciler adapter - adapter := reconciler.AsGenericReconciler( - reconcileFunc, - mockKeyExtractor, - ) - - // Call the adapter - ctx := context.Background() - req := reconcile.Request{} - _, err := adapter.Reconcile(ctx, req) - - // Verify that the error is propagated - assert.Error(t, err) - assert.Equal(t, expectedErr, err) - }) - }) - - t.Run("No backoff", func(t *testing.T) { - t.Run("Result without backoff", func(t *testing.T) { - // Create a mock reconcile function that returns a Result with RequeueWithBackoff = false - expectedRequeueAfter := 5 * time.Second - reconcileFunc := func(ctx context.Context, req reconcile.Request) (reconciler.Result, error) { - return reconciler.Result{ - Result: reconcile.Result{ - RequeueAfter: expectedRequeueAfter, - }, - RequeueWithBackoff: false, - }, nil - } - - // Create a mock key extractor - mockKeyExtractor := &MockKeyExtractor[string]{ - key: "test-controller", + }, } - // Create the reconciler adapter - adapter := reconciler.AsGenericReconciler( - reconcileFunc, - mockKeyExtractor, - ) + // Create the reconciler adapter with custom rate limiter + adapter := reconciler.AsReconcilerWithRateLimiter(mockReconciler, mockRateLimiter) // Call the adapter ctx := context.Background() @@ -288,78 +192,34 @@ func TestAsGenericReconciler(t *testing.T) { result, err := adapter.Reconcile(ctx, req) // Verify the result - assert.NoError(t, err) - assert.Equal(t, expectedRequeueAfter, result.RequeueAfter) - assert.False(t, result.Requeue) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(backoffDuration)) + Expect(mockRateLimiter.NumRequeues(req)).To(Equal(1)) }) }) -} -func TestKeyExtractors(t *testing.T) { - t.Run("RequestKeyExtractor", func(t *testing.T) { - t.Run("Extract request key", func(t *testing.T) { - // Create a request key extractor - extractor := reconciler.RequestKeyExtractor{} - - // Extract the key - ctx := context.Background() - req := reconcile.Request{} - key := extractor.Extract(ctx, req) - - // Verify the key - assert.Equal(t, req, key) - }) - }) -} - -func TestAsReconciler(t *testing.T) { - t.Run("Standard reconciler adapter", func(t *testing.T) { - // Create a mock reconciler - mockReconciler := &MockReconciler{ - result: reconciler.Result{ + Describe("ReconcilerFunc", func() { + It("should implement the Reconciler interface", func() { + // Create a ReconcilerFunc + expectedResult := reconciler.Result{ Result: reconcile.Result{ RequeueAfter: 5 * time.Second, }, RequeueWithBackoff: false, - }, - } - - // Create the reconciler adapter - adapter := reconciler.AsReconciler(mockReconciler) + } + expectedErr := errors.New("test error") - // Call the adapter - ctx := context.Background() - req := reconcile.Request{} - result, err := adapter.Reconcile(ctx, req) + reconcileFunc := reconciler.ReconcilerFunc(func(ctx context.Context) (reconciler.Result, error) { + return expectedResult, expectedErr + }) - // Verify the result - assert.NoError(t, err) - assert.Equal(t, 5*time.Second, result.RequeueAfter) - assert.False(t, result.Requeue) - }) -} + // Call the function + ctx := context.Background() + result, err := reconcileFunc.Reconcile(ctx) -func TestReconcilerFunc(t *testing.T) { - t.Run("Implementation", func(t *testing.T) { - // Create a ReconcilerFunc - expectedResult := reconciler.Result{ - Result: reconcile.Result{ - RequeueAfter: 5 * time.Second, - }, - RequeueWithBackoff: false, - } - expectedErr := errors.New("test error") - - reconcileFunc := reconciler.ReconcilerFunc(func(ctx context.Context) (reconciler.Result, error) { - return expectedResult, expectedErr + // Verify the result + Expect(result).To(Equal(expectedResult)) + Expect(err).To(Equal(expectedErr)) }) - - // Call the function - ctx := context.Background() - result, err := reconcileFunc.Reconcile(ctx) - - // Verify the result - assert.Equal(t, expectedResult, result) - assert.Equal(t, expectedErr, err) }) -} +}) diff --git a/singleton/controller.go b/singleton/controller.go index fc0285a..7837dbd 100644 --- a/singleton/controller.go +++ b/singleton/controller.go @@ -24,39 +24,33 @@ type Reconciler interface { Reconcile(ctx context.Context) (reconciler.Result, error) } -// StringKeyExtractor extracts the controller name as the rate limiter key -type StringKeyExtractor struct { - reconciler Reconciler -} - -// Extract returns the controller name as the key -func (e StringKeyExtractor) Extract(ctx context.Context, req reconcile.Request) string { - return e.reconciler.Name() -} - // In response to Requeue: True being deprecated via: https://github.com/kubernetes-sigs/controller-runtime/pull/3107/files // This uses a bucket and per item delay but the item will be the same because the key is the controller name. // This implements the same behavior as Requeue: True. // AsReconciler creates a controller-runtime reconciler from a singleton reconciler func AsReconciler(rec Reconciler) reconcile.Reconciler { - return reconciler.AsGenericReconciler( - func(ctx context.Context, req reconcile.Request) (reconciler.Result, error) { - return rec.Reconcile(ctx) - }, - StringKeyExtractor{reconciler: rec}, - ) + return AsReconcilerWithRateLimiter(rec, workqueue.DefaultTypedControllerRateLimiter[string]()) } // AsReconcilerWithRateLimiter creates a controller-runtime reconciler with a custom rate limiter -func AsReconcilerWithRateLimiter(rec Reconciler, rateLimiter workqueue.TypedRateLimiter[string]) reconcile.Reconciler { - return reconciler.AsGenericReconcilerWithRateLimiter( - func(ctx context.Context, req reconcile.Request) (reconciler.Result, error) { - return rec.Reconcile(ctx) - }, - StringKeyExtractor{reconciler: rec}, - rateLimiter, - ) +func AsReconcilerWithRateLimiter( + rec Reconciler, + rateLimiter workqueue.TypedRateLimiter[string], +) reconcile.Reconciler { + return reconcile.Func(func(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { + result, err := rec.Reconcile(ctx) + if err != nil { + return reconcile.Result{}, err + } + if result.RequeueWithBackoff { + return reconcile.Result{RequeueAfter: rateLimiter.When(rec.Name())}, nil + } + if result.RequeueAfter > 0 { + return reconcile.Result{RequeueAfter: result.RequeueAfter}, nil + } + return result.Result, nil + }) } // Source creates a source for singleton controllers diff --git a/singleton/suite_test.go b/singleton/suite_test.go index 0e10b3c..caf70c4 100644 --- a/singleton/suite_test.go +++ b/singleton/suite_test.go @@ -20,9 +20,30 @@ func Test(t *testing.T) { var ( mockReconciler *MockReconciler - rec singleton.Reconciler ) +type MockRateLimiter[K comparable] struct { + whenFunc func(K) time.Duration + numRequeues int + backoffDuration time.Duration +} + +func (m *MockRateLimiter[K]) When(key K) time.Duration { + if m.whenFunc != nil { + return m.whenFunc(key) + } + m.numRequeues++ + return m.backoffDuration +} + +func (m *MockRateLimiter[K]) NumRequeues(key K) int { + return m.numRequeues +} + +func (m *MockRateLimiter[K]) Forget(key K) { + m.numRequeues = 0 +} + // MockReconciler for testing type MockReconciler struct { name string @@ -39,7 +60,7 @@ func (m *MockReconciler) Reconcile(ctx context.Context) (reconciler.Result, erro } var _ = Describe("Singleton Controller", func() { - Context("AsReconciler with rate limiting", func() { + Context("AsReconciler", func() { BeforeEach(func() { mockReconciler = &MockReconciler{ name: "test-controller", @@ -51,15 +72,13 @@ var _ = Describe("Singleton Controller", func() { mockReconciler.result = reconciler.Result{ RequeueWithBackoff: false, } - rec = mockReconciler - result, err := rec.Reconcile(context.Background()) + result, err := singleton.AsReconciler(mockReconciler).Reconcile(context.Background(), reconcile.Request{}) Expect(err).NotTo(HaveOccurred()) Expect(result.RequeueAfter).To(BeZero()) }) It("should return the original result without backoff when RequeueWithBackoff is not set", func() { mockReconciler.result = reconciler.Result{} - rec = mockReconciler - result, err := rec.Reconcile(context.Background()) + result, err := singleton.AsReconciler(mockReconciler).Reconcile(context.Background(), reconcile.Request{}) Expect(err).NotTo(HaveOccurred()) Expect(result.RequeueAfter).To(BeZero()) }) @@ -70,11 +89,10 @@ var _ = Describe("Singleton Controller", func() { mockReconciler.result = reconciler.Result{ RequeueWithBackoff: true, } - rec = mockReconciler }) It("should return a result with RequeueAfter set", func() { - result, err := rec.Reconcile(context.Background()) + result, err := singleton.AsReconciler(mockReconciler).Reconcile(context.Background(), reconcile.Request{}) Expect(err).NotTo(HaveOccurred()) Expect(result.RequeueAfter).To(BeNumerically(">=", 0)) }) @@ -108,7 +126,7 @@ var _ = Describe("Singleton Controller", func() { delays := make([]time.Duration, 5) for i := 0; i < 5; i++ { - result, err := rec.Reconcile(context.Background()) + result, err := singleton.AsReconciler(mockReconciler).Reconcile(context.Background(), reconcile.Request{}) Expect(err).NotTo(HaveOccurred()) delays[i] = result.RequeueAfter } @@ -126,11 +144,10 @@ var _ = Describe("Singleton Controller", func() { BeforeEach(func() { mockReconciler.result = reconciler.Result{RequeueWithBackoff: true} mockReconciler.err = errors.New("test error") - rec = mockReconciler }) It("should return the error without processing backoff", func() { - result, err := rec.Reconcile(context.Background()) + result, err := singleton.AsReconciler(mockReconciler).Reconcile(context.Background(), reconcile.Request{}) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(Equal("test error")) Expect(result.RequeueAfter).To(BeZero()) @@ -142,12 +159,28 @@ var _ = Describe("Singleton Controller", func() { mockReconciler.result = reconciler.Result{ Result: reconcile.Result{RequeueAfter: singleton.RequeueImmediately}, } - rec = mockReconciler - result, err := rec.Reconcile(context.Background()) + result, err := singleton.AsReconciler(mockReconciler).Reconcile(context.Background(), reconcile.Request{}) Expect(err).NotTo(HaveOccurred()) Expect(result.RequeueAfter).To(Equal(singleton.RequeueImmediately)) }) }) }) + Context("AsReconcilerWithRateLimiter", func() { + BeforeEach(func() { + mockReconciler.result = reconciler.Result{ + RequeueWithBackoff: true, + } + }) + It("should use the custom rate limiter", func() { + mockRateLimiter := &MockRateLimiter[string]{ + backoffDuration: 10 * time.Second, + whenFunc: func(req string) time.Duration { return 10 * time.Second }, + } + adapter := singleton.AsReconcilerWithRateLimiter(mockReconciler, mockRateLimiter) + result, err := adapter.Reconcile(context.Background(), reconcile.Request{}) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(10 * time.Second)) + }) + }) }) From d40e4b209e326d415b17cb510d4704b309001fce Mon Sep 17 00:00:00 2001 From: Reed Schalo Date: Wed, 23 Jul 2025 15:08:19 -0700 Subject: [PATCH 06/13] reduce diff --- reconciler/suite_test.go | 194 +++++++++++++++------------------------ 1 file changed, 72 insertions(+), 122 deletions(-) diff --git a/reconciler/suite_test.go b/reconciler/suite_test.go index 9754aba..c890d5c 100644 --- a/reconciler/suite_test.go +++ b/reconciler/suite_test.go @@ -55,116 +55,91 @@ func (m *MockReconciler) Reconcile(ctx context.Context) (reconciler.Result, erro } var _ = Describe("Reconciler", func() { - Describe("Result", func() { - Context("basic functionality", func() { - It("should handle results without backoff", func() { - // Create a Result with RequeueWithBackoff = false and a specific RequeueAfter duration - duration := 5 * time.Second - result := reconciler.Result{ + Describe("AsReconciler with rate limiting", func() { + It("should return the original result without backoff", func() { + backoff := 5 * time.Second + // Create a mock reconciler + mockReconciler := &MockReconciler{ + result: reconciler.Result{ Result: reconcile.Result{ - RequeueAfter: duration, + RequeueAfter: backoff, }, RequeueWithBackoff: false, - } + }, + } + + // Create the reconciler adapter + adapter := reconciler.AsReconciler(mockReconciler) + + // Call the adapter + ctx := context.Background() + req := reconcile.Request{} + result, err := adapter.Reconcile(ctx, req) - // Verify that the wrapped Result has the same RequeueAfter value - Expect(result.RequeueAfter).To(Equal(duration)) - Expect(result.RequeueWithBackoff).To(BeFalse()) - }) + // Verify the result + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(backoff)) }) - }) - Describe("AsReconciler with rate limiting", func() { - Context("when RequeueWithBackoff is false", func() { - It("should return the original result without backoff", func() { - // Create a mock reconciler - mockReconciler := &MockReconciler{ - result: reconciler.Result{ - Result: reconcile.Result{ - RequeueAfter: 5 * time.Second, - }, - RequeueWithBackoff: false, - }, - } - - // Create the reconciler adapter - adapter := reconciler.AsReconciler(mockReconciler) - - // Call the adapter - ctx := context.Background() - req := reconcile.Request{} - result, err := adapter.Reconcile(ctx, req) - - // Verify the result - Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(Equal(5 * time.Second)) - }) - - It("should return the original result without backoff when RequeueWithBackoff is not set", func() { - mockReconciler := &MockReconciler{ - result: reconciler.Result{}, - } - - // Create the reconciler adapter - adapter := reconciler.AsReconciler(mockReconciler) - - // Call the adapter - ctx := context.Background() - req := reconcile.Request{} - result, err := adapter.Reconcile(ctx, req) - - // Verify the result - Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(BeZero()) - }) + It("should return the original result without backoff when RequeueWithBackoff is not set", func() { + mockReconciler := &MockReconciler{ + result: reconciler.Result{}, + } + + // Create the reconciler adapter + adapter := reconciler.AsReconciler(mockReconciler) + + // Call the adapter + ctx := context.Background() + req := reconcile.Request{} + result, err := adapter.Reconcile(ctx, req) + + // Verify the result + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(BeZero()) }) - Context("when RequeueWithBackoff is true", func() { - It("should return a result with RequeueAfter set", func() { - // Create a mock reconciler that returns RequeueWithBackoff = true - mockReconciler := &MockReconciler{ - result: reconciler.Result{ - Result: reconcile.Result{}, - RequeueWithBackoff: true, - }, - } + It("should return a result with RequeueAfter set", func() { + // Create a mock reconciler that returns RequeueWithBackoff = true + mockReconciler := &MockReconciler{ + result: reconciler.Result{ + Result: reconcile.Result{}, + RequeueWithBackoff: true, + }, + } - // Create the reconciler adapter - adapter := reconciler.AsReconciler(mockReconciler) + // Create the reconciler adapter + adapter := reconciler.AsReconciler(mockReconciler) - // Call the adapter - ctx := context.Background() - req := reconcile.Request{} - result, err := adapter.Reconcile(ctx, req) + // Call the adapter + ctx := context.Background() + req := reconcile.Request{} + result, err := adapter.Reconcile(ctx, req) - // Verify the result - should have some backoff duration - Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(BeNumerically(">", 0)) - }) + // Verify the result - should have some backoff duration + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(BeNumerically(">", 0)) }) + It("should return the error without processing backoff", func() { + // Create a mock reconciler that returns an error + expectedErr := errors.New("test error") + mockReconciler := &MockReconciler{ + result: reconciler.Result{RequeueWithBackoff: true}, + err: expectedErr, + } - Context("when reconciler returns an error", func() { - It("should return the error without processing backoff", func() { - // Create a mock reconciler that returns an error - expectedErr := errors.New("test error") - mockReconciler := &MockReconciler{ - result: reconciler.Result{RequeueWithBackoff: true}, - err: expectedErr, - } - - // Create the reconciler adapter - adapter := reconciler.AsReconciler(mockReconciler) - - // Call the adapter - ctx := context.Background() - req := reconcile.Request{} - result, err := adapter.Reconcile(ctx, req) - - // Verify that the error is propagated - Expect(err).To(HaveOccurred()) - Expect(err).To(Equal(expectedErr)) - Expect(result.RequeueAfter).To(BeZero()) - }) + // Create the reconciler adapter + adapter := reconciler.AsReconciler(mockReconciler) + + // Call the adapter + ctx := context.Background() + req := reconcile.Request{} + result, err := adapter.Reconcile(ctx, req) + + // Verify that the error is propagated + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(expectedErr)) + Expect(result.RequeueAfter).To(BeZero()) }) }) @@ -197,29 +172,4 @@ var _ = Describe("Reconciler", func() { Expect(mockRateLimiter.NumRequeues(req)).To(Equal(1)) }) }) - - Describe("ReconcilerFunc", func() { - It("should implement the Reconciler interface", func() { - // Create a ReconcilerFunc - expectedResult := reconciler.Result{ - Result: reconcile.Result{ - RequeueAfter: 5 * time.Second, - }, - RequeueWithBackoff: false, - } - expectedErr := errors.New("test error") - - reconcileFunc := reconciler.ReconcilerFunc(func(ctx context.Context) (reconciler.Result, error) { - return expectedResult, expectedErr - }) - - // Call the function - ctx := context.Background() - result, err := reconcileFunc.Reconcile(ctx) - - // Verify the result - Expect(result).To(Equal(expectedResult)) - Expect(err).To(Equal(expectedErr)) - }) - }) }) From 393dc546a6619fb44a7991145eb7ca3ab81644fe Mon Sep 17 00:00:00 2001 From: Reed Schalo Date: Wed, 23 Jul 2025 15:11:36 -0700 Subject: [PATCH 07/13] further reduce diff --- singleton/suite_test.go | 54 +++++++++-------------------------------- 1 file changed, 12 insertions(+), 42 deletions(-) diff --git a/singleton/suite_test.go b/singleton/suite_test.go index caf70c4..578bfcf 100644 --- a/singleton/suite_test.go +++ b/singleton/suite_test.go @@ -83,20 +83,17 @@ var _ = Describe("Singleton Controller", func() { Expect(result.RequeueAfter).To(BeZero()) }) }) - Context("when RequeueWithBackoff is true", func() { BeforeEach(func() { mockReconciler.result = reconciler.Result{ RequeueWithBackoff: true, } }) - It("should return a result with RequeueAfter set", func() { result, err := singleton.AsReconciler(mockReconciler).Reconcile(context.Background(), reconcile.Request{}) Expect(err).NotTo(HaveOccurred()) Expect(result.RequeueAfter).To(BeNumerically(">=", 0)) }) - It("should use controller name for rate limiting", func() { // Test with different controller names to ensure they're handled independently controller1 := &MockReconciler{ @@ -120,7 +117,6 @@ var _ = Describe("Singleton Controller", func() { Expect(result1.RequeueAfter).To(BeNumerically(">=", 0)) Expect(result2.RequeueAfter).To(BeNumerically(">=", 0)) }) - It("should implement exponential backoff on repeated calls", func() { // Multiple calls to the same controller should show increasing delays delays := make([]time.Duration, 5) @@ -139,48 +135,22 @@ var _ = Describe("Singleton Controller", func() { } }) }) - - Context("when reconciler returns an error", func() { - BeforeEach(func() { - mockReconciler.result = reconciler.Result{RequeueWithBackoff: true} - mockReconciler.err = errors.New("test error") - }) - - It("should return the error without processing backoff", func() { - result, err := singleton.AsReconciler(mockReconciler).Reconcile(context.Background(), reconcile.Request{}) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(Equal("test error")) - Expect(result.RequeueAfter).To(BeZero()) - }) - }) - - Context("integration with RequeueImmediately constant", func() { - It("should work with immediate requeue pattern", func() { - mockReconciler.result = reconciler.Result{ - Result: reconcile.Result{RequeueAfter: singleton.RequeueImmediately}, - } - - result, err := singleton.AsReconciler(mockReconciler).Reconcile(context.Background(), reconcile.Request{}) - Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(Equal(singleton.RequeueImmediately)) - }) + It("should return the error without processing backoff", func() { + mockReconciler.result = reconciler.Result{RequeueWithBackoff: true} + mockReconciler.err = errors.New("test error") + result, err := singleton.AsReconciler(mockReconciler).Reconcile(context.Background(), reconcile.Request{}) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(Equal("test error")) + Expect(result.RequeueAfter).To(BeZero()) }) - }) - Context("AsReconcilerWithRateLimiter", func() { - BeforeEach(func() { + It("should work with RequeueImmediately", func() { mockReconciler.result = reconciler.Result{ - RequeueWithBackoff: true, + Result: reconcile.Result{RequeueAfter: singleton.RequeueImmediately}, } - }) - It("should use the custom rate limiter", func() { - mockRateLimiter := &MockRateLimiter[string]{ - backoffDuration: 10 * time.Second, - whenFunc: func(req string) time.Duration { return 10 * time.Second }, - } - adapter := singleton.AsReconcilerWithRateLimiter(mockReconciler, mockRateLimiter) - result, err := adapter.Reconcile(context.Background(), reconcile.Request{}) + + result, err := singleton.AsReconciler(mockReconciler).Reconcile(context.Background(), reconcile.Request{}) Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(Equal(10 * time.Second)) + Expect(result.RequeueAfter).To(Equal(singleton.RequeueImmediately)) }) }) }) From 4cdda31d048bcf4126b4153ef1f616dbb1fede5b Mon Sep 17 00:00:00 2001 From: Reed Schalo Date: Wed, 23 Jul 2025 15:12:57 -0700 Subject: [PATCH 08/13] remove describe --- reconciler/suite_test.go | 216 +++++++++++++++++++-------------------- 1 file changed, 106 insertions(+), 110 deletions(-) diff --git a/reconciler/suite_test.go b/reconciler/suite_test.go index c890d5c..c6fc158 100644 --- a/reconciler/suite_test.go +++ b/reconciler/suite_test.go @@ -55,121 +55,117 @@ func (m *MockReconciler) Reconcile(ctx context.Context) (reconciler.Result, erro } var _ = Describe("Reconciler", func() { - Describe("AsReconciler with rate limiting", func() { - It("should return the original result without backoff", func() { - backoff := 5 * time.Second - // Create a mock reconciler - mockReconciler := &MockReconciler{ - result: reconciler.Result{ - Result: reconcile.Result{ - RequeueAfter: backoff, - }, - RequeueWithBackoff: false, + It("should return the original result without backoff", func() { + backoff := 5 * time.Second + // Create a mock reconciler + mockReconciler := &MockReconciler{ + result: reconciler.Result{ + Result: reconcile.Result{ + RequeueAfter: backoff, }, - } - - // Create the reconciler adapter - adapter := reconciler.AsReconciler(mockReconciler) - - // Call the adapter - ctx := context.Background() - req := reconcile.Request{} - result, err := adapter.Reconcile(ctx, req) - - // Verify the result - Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(Equal(backoff)) - }) - - It("should return the original result without backoff when RequeueWithBackoff is not set", func() { - mockReconciler := &MockReconciler{ - result: reconciler.Result{}, - } - - // Create the reconciler adapter - adapter := reconciler.AsReconciler(mockReconciler) - - // Call the adapter - ctx := context.Background() - req := reconcile.Request{} - result, err := adapter.Reconcile(ctx, req) - - // Verify the result - Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(BeZero()) - }) - - It("should return a result with RequeueAfter set", func() { - // Create a mock reconciler that returns RequeueWithBackoff = true - mockReconciler := &MockReconciler{ - result: reconciler.Result{ - Result: reconcile.Result{}, - RequeueWithBackoff: true, - }, - } - - // Create the reconciler adapter - adapter := reconciler.AsReconciler(mockReconciler) - - // Call the adapter - ctx := context.Background() - req := reconcile.Request{} - result, err := adapter.Reconcile(ctx, req) - - // Verify the result - should have some backoff duration - Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(BeNumerically(">", 0)) - }) - It("should return the error without processing backoff", func() { - // Create a mock reconciler that returns an error - expectedErr := errors.New("test error") - mockReconciler := &MockReconciler{ - result: reconciler.Result{RequeueWithBackoff: true}, - err: expectedErr, - } - - // Create the reconciler adapter - adapter := reconciler.AsReconciler(mockReconciler) - - // Call the adapter - ctx := context.Background() - req := reconcile.Request{} - result, err := adapter.Reconcile(ctx, req) - - // Verify that the error is propagated - Expect(err).To(HaveOccurred()) - Expect(err).To(Equal(expectedErr)) - Expect(result.RequeueAfter).To(BeZero()) - }) + RequeueWithBackoff: false, + }, + } + + // Create the reconciler adapter + adapter := reconciler.AsReconciler(mockReconciler) + + // Call the adapter + ctx := context.Background() + req := reconcile.Request{} + result, err := adapter.Reconcile(ctx, req) + + // Verify the result + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(backoff)) }) - Describe("AsReconcilerWithRateLimiter", func() { - It("should use custom rate limiter for backoff", func() { - backoffDuration := 10 * time.Second - mockRateLimiter := &MockRateLimiter[reconcile.Request]{ - backoffDuration: backoffDuration, - } - - // Create a mock reconciler that returns RequeueWithBackoff = true - mockReconciler := &MockReconciler{ - result: reconciler.Result{ - Result: reconcile.Result{}, - RequeueWithBackoff: true, - }, - } + It("should return the original result without backoff when RequeueWithBackoff is not set", func() { + mockReconciler := &MockReconciler{ + result: reconciler.Result{}, + } - // Create the reconciler adapter with custom rate limiter - adapter := reconciler.AsReconcilerWithRateLimiter(mockReconciler, mockRateLimiter) + // Create the reconciler adapter + adapter := reconciler.AsReconciler(mockReconciler) - // Call the adapter - ctx := context.Background() - req := reconcile.Request{} - result, err := adapter.Reconcile(ctx, req) + // Call the adapter + ctx := context.Background() + req := reconcile.Request{} + result, err := adapter.Reconcile(ctx, req) + + // Verify the result + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(BeZero()) + }) + + It("should return a result with RequeueAfter set", func() { + // Create a mock reconciler that returns RequeueWithBackoff = true + mockReconciler := &MockReconciler{ + result: reconciler.Result{ + Result: reconcile.Result{}, + RequeueWithBackoff: true, + }, + } + + // Create the reconciler adapter + adapter := reconciler.AsReconciler(mockReconciler) + + // Call the adapter + ctx := context.Background() + req := reconcile.Request{} + result, err := adapter.Reconcile(ctx, req) + + // Verify the result - should have some backoff duration + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(BeNumerically(">", 0)) + }) + It("should return the error without processing backoff", func() { + // Create a mock reconciler that returns an error + expectedErr := errors.New("test error") + mockReconciler := &MockReconciler{ + result: reconciler.Result{RequeueWithBackoff: true}, + err: expectedErr, + } + + // Create the reconciler adapter + adapter := reconciler.AsReconciler(mockReconciler) + + // Call the adapter + ctx := context.Background() + req := reconcile.Request{} + result, err := adapter.Reconcile(ctx, req) + + // Verify that the error is propagated + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(expectedErr)) + Expect(result.RequeueAfter).To(BeZero()) + }) - // Verify the result - Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(Equal(backoffDuration)) - Expect(mockRateLimiter.NumRequeues(req)).To(Equal(1)) - }) + It("should use custom rate limiter for backoff", func() { + backoffDuration := 10 * time.Second + mockRateLimiter := &MockRateLimiter[reconcile.Request]{ + backoffDuration: backoffDuration, + } + + // Create a mock reconciler that returns RequeueWithBackoff = true + mockReconciler := &MockReconciler{ + result: reconciler.Result{ + Result: reconcile.Result{}, + RequeueWithBackoff: true, + }, + } + + // Create the reconciler adapter with custom rate limiter + adapter := reconciler.AsReconcilerWithRateLimiter(mockReconciler, mockRateLimiter) + + // Call the adapter + ctx := context.Background() + req := reconcile.Request{} + result, err := adapter.Reconcile(ctx, req) + + // Verify the result + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(backoffDuration)) + Expect(mockRateLimiter.NumRequeues(req)).To(Equal(1)) }) }) From d408d23c303d8aed3e88f04e0015b306b3729517 Mon Sep 17 00:00:00 2001 From: Reed Schalo Date: Wed, 23 Jul 2025 15:16:31 -0700 Subject: [PATCH 09/13] formatting --- reconciler/suite_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/reconciler/suite_test.go b/reconciler/suite_test.go index c6fc158..f6a0310 100644 --- a/reconciler/suite_test.go +++ b/reconciler/suite_test.go @@ -79,7 +79,6 @@ var _ = Describe("Reconciler", func() { Expect(err).NotTo(HaveOccurred()) Expect(result.RequeueAfter).To(Equal(backoff)) }) - It("should return the original result without backoff when RequeueWithBackoff is not set", func() { mockReconciler := &MockReconciler{ result: reconciler.Result{}, @@ -97,7 +96,6 @@ var _ = Describe("Reconciler", func() { Expect(err).NotTo(HaveOccurred()) Expect(result.RequeueAfter).To(BeZero()) }) - It("should return a result with RequeueAfter set", func() { // Create a mock reconciler that returns RequeueWithBackoff = true mockReconciler := &MockReconciler{ @@ -140,7 +138,6 @@ var _ = Describe("Reconciler", func() { Expect(err).To(Equal(expectedErr)) Expect(result.RequeueAfter).To(BeZero()) }) - It("should use custom rate limiter for backoff", func() { backoffDuration := 10 * time.Second mockRateLimiter := &MockRateLimiter[reconcile.Request]{ From c7b120d3893cb935691e811636887391571353fe Mon Sep 17 00:00:00 2001 From: Reed Schalo Date: Wed, 20 Aug 2025 07:45:20 -0700 Subject: [PATCH 10/13] pr responses --- reconciler/reconciler.go | 15 +++--- reconciler/suite_test.go | 94 ++++++++++++++++++++++++++++++------ singleton/controller.go | 23 +-------- singleton/suite_test.go | 102 ++++++++------------------------------- 4 files changed, 108 insertions(+), 126 deletions(-) diff --git a/reconciler/reconciler.go b/reconciler/reconciler.go index 0052910..91e6c63 100644 --- a/reconciler/reconciler.go +++ b/reconciler/reconciler.go @@ -10,7 +10,7 @@ import ( // Result is a wrapper around reconcile.Result that adds RequeueWithBackoff functionality. type Result struct { reconcile.Result - RequeueWithBackoff bool + Requeue bool } // Reconciler defines the interface for standard reconcilers @@ -22,8 +22,8 @@ type Reconciler interface { type ReconcilerFunc func(ctx context.Context) (Result, error) // Reconcile implements the Reconciler interface. -func (f ReconcilerFunc) Reconcile(ctx context.Context) (Result, error) { - return f(ctx) +func (r ReconcilerFunc) Func(ctx context.Context) (Result, error) { + return r(ctx) } // AsReconciler creates a reconciler from a standard reconciler @@ -34,7 +34,7 @@ func AsReconciler(reconciler Reconciler) reconcile.Reconciler { ) } -// AsReconcilerWithRateLimiter creates a reconciler with a specific key extractor +// AsReconcilerWithRateLimiter creates a reconciler with a custom rate-limiter func AsReconcilerWithRateLimiter( reconciler Reconciler, rateLimiter workqueue.TypedRateLimiter[reconcile.Request], @@ -44,12 +44,13 @@ func AsReconcilerWithRateLimiter( if err != nil { return reconcile.Result{}, err } - if result.RequeueWithBackoff { - return reconcile.Result{RequeueAfter: rateLimiter.When(req)}, nil - } if result.RequeueAfter > 0 { return reconcile.Result{RequeueAfter: result.RequeueAfter}, nil } + if result.Requeue { + return reconcile.Result{RequeueAfter: rateLimiter.When(req)}, nil + } + rateLimiter.Forget(req) return result.Result, nil }) } diff --git a/reconciler/suite_test.go b/reconciler/suite_test.go index f6a0310..3d7f832 100644 --- a/reconciler/suite_test.go +++ b/reconciler/suite_test.go @@ -63,7 +63,7 @@ var _ = Describe("Reconciler", func() { Result: reconcile.Result{ RequeueAfter: backoff, }, - RequeueWithBackoff: false, + Requeue: false, }, } @@ -79,7 +79,7 @@ var _ = Describe("Reconciler", func() { Expect(err).NotTo(HaveOccurred()) Expect(result.RequeueAfter).To(Equal(backoff)) }) - It("should return the original result without backoff when RequeueWithBackoff is not set", func() { + It("should return the original result without backoff when Requeue is not set", func() { mockReconciler := &MockReconciler{ result: reconciler.Result{}, } @@ -96,12 +96,12 @@ var _ = Describe("Reconciler", func() { Expect(err).NotTo(HaveOccurred()) Expect(result.RequeueAfter).To(BeZero()) }) - It("should return a result with RequeueAfter set", func() { - // Create a mock reconciler that returns RequeueWithBackoff = true + It("should return a result with Requeue set", func() { + // Create a mock reconciler that returns Requeue = true mockReconciler := &MockReconciler{ result: reconciler.Result{ - Result: reconcile.Result{}, - RequeueWithBackoff: true, + Result: reconcile.Result{}, + Requeue: true, }, } @@ -115,13 +115,36 @@ var _ = Describe("Reconciler", func() { // Verify the result - should have some backoff duration Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(BeNumerically(">", 0)) + Expect(result.RequeueAfter).To(Equal(5 * time.Millisecond)) + }) + It("should return a result with RequeueAfter when both RequeueAfter and Requeue are set", func() { + // Create a mock reconciler that returns Requeue = true + mockReconciler := &MockReconciler{ + result: reconciler.Result{ + Result: reconcile.Result{ + RequeueAfter: 10 * time.Second, + }, + Requeue: true, + }, + } + + // Create the reconciler adapter + adapter := reconciler.AsReconciler(mockReconciler) + + // Call the adapter + ctx := context.Background() + req := reconcile.Request{} + result, err := adapter.Reconcile(ctx, req) + + // Verify the result - should have some backoff duration + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(10 * time.Second)) }) It("should return the error without processing backoff", func() { // Create a mock reconciler that returns an error expectedErr := errors.New("test error") mockReconciler := &MockReconciler{ - result: reconciler.Result{RequeueWithBackoff: true}, + result: reconciler.Result{Requeue: true}, err: expectedErr, } @@ -139,16 +162,15 @@ var _ = Describe("Reconciler", func() { Expect(result.RequeueAfter).To(BeZero()) }) It("should use custom rate limiter for backoff", func() { - backoffDuration := 10 * time.Second mockRateLimiter := &MockRateLimiter[reconcile.Request]{ - backoffDuration: backoffDuration, + backoffDuration: 10 * time.Second, } - // Create a mock reconciler that returns RequeueWithBackoff = true + // Create a mock reconciler that returns Requeue = true mockReconciler := &MockReconciler{ result: reconciler.Result{ - Result: reconcile.Result{}, - RequeueWithBackoff: true, + Result: reconcile.Result{}, + Requeue: true, }, } @@ -162,7 +184,51 @@ var _ = Describe("Reconciler", func() { // Verify the result Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(Equal(backoffDuration)) + Expect(result.RequeueAfter).To(Equal(10 * time.Second)) Expect(mockRateLimiter.NumRequeues(req)).To(Equal(1)) }) + It("should implement exponential backoff on repeated calls", func() { + mockReconciler := &MockReconciler{ + result: reconciler.Result{ + Result: reconcile.Result{}, + Requeue: true, + }, + } + // Multiple calls to the same controller should show increasing delays + delays := make([]time.Duration, 5) + reconciler := reconciler.AsReconciler(mockReconciler) + + for i := range 5 { + result, err := reconciler.Reconcile(context.Background(), reconcile.Request{}) + Expect(err).NotTo(HaveOccurred()) + delays[i] = result.RequeueAfter + } + + // Verify generally increasing pattern + initialDelay := 5 * time.Millisecond + Expect(delays[0]).To(BeNumerically("==", initialDelay)) + for i := 1; i < len(delays); i++ { + initialDelay *= 2 + Expect(delays[i]).To(BeNumerically("==", initialDelay)) + Expect(delays[i]).To(BeNumerically(">", delays[i-1]), + "Delay at index %d (%v) should be >= delay at index %d (%v)", + i, delays[i], i-1, delays[i-1]) + } + }) + It("should forget an item when reconcile succeeds", func() { + mockReconciler := &MockReconciler{ + result: reconciler.Result{ + Result: reconcile.Result{}, + Requeue: false, + }, + } + // Multiple calls to the same controller should show zero requeue + reconciler := reconciler.AsReconciler(mockReconciler) + + for i := 0; i < 5; i++ { + result, err := reconciler.Reconcile(context.Background(), reconcile.Request{}) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(BeZero()) + } + }) }) diff --git a/singleton/controller.go b/singleton/controller.go index 7837dbd..7ef1b93 100644 --- a/singleton/controller.go +++ b/singleton/controller.go @@ -20,7 +20,6 @@ const ( // Reconciler defines the interface for singleton reconcilers type Reconciler interface { - Name() string Reconcile(ctx context.Context) (reconciler.Result, error) } @@ -30,27 +29,7 @@ type Reconciler interface { // AsReconciler creates a controller-runtime reconciler from a singleton reconciler func AsReconciler(rec Reconciler) reconcile.Reconciler { - return AsReconcilerWithRateLimiter(rec, workqueue.DefaultTypedControllerRateLimiter[string]()) -} - -// AsReconcilerWithRateLimiter creates a controller-runtime reconciler with a custom rate limiter -func AsReconcilerWithRateLimiter( - rec Reconciler, - rateLimiter workqueue.TypedRateLimiter[string], -) reconcile.Reconciler { - return reconcile.Func(func(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { - result, err := rec.Reconcile(ctx) - if err != nil { - return reconcile.Result{}, err - } - if result.RequeueWithBackoff { - return reconcile.Result{RequeueAfter: rateLimiter.When(rec.Name())}, nil - } - if result.RequeueAfter > 0 { - return reconcile.Result{RequeueAfter: result.RequeueAfter}, nil - } - return result.Result, nil - }) + return reconciler.AsReconciler(rec) } // Source creates a source for singleton controllers diff --git a/singleton/suite_test.go b/singleton/suite_test.go index 578bfcf..7b0d742 100644 --- a/singleton/suite_test.go +++ b/singleton/suite_test.go @@ -2,7 +2,6 @@ package singleton_test import ( "context" - "errors" "testing" "time" @@ -66,91 +65,28 @@ var _ = Describe("Singleton Controller", func() { name: "test-controller", } }) + It("should return a result with RequeueAfter that is scoped to a controller", func() { + // Test with different controllers to ensure they're handled independently + controller1 := &MockReconciler{ + name: "controller-1", + result: reconciler.Result{Requeue: true}, + } + controller2 := &MockReconciler{ + name: "controller-2", + result: reconciler.Result{Requeue: true}, + } - Context("when RequeueWithBackoff is false", func() { - It("should return the original result without backoff", func() { - mockReconciler.result = reconciler.Result{ - RequeueWithBackoff: false, - } - result, err := singleton.AsReconciler(mockReconciler).Reconcile(context.Background(), reconcile.Request{}) - Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(BeZero()) - }) - It("should return the original result without backoff when RequeueWithBackoff is not set", func() { - mockReconciler.result = reconciler.Result{} - result, err := singleton.AsReconciler(mockReconciler).Reconcile(context.Background(), reconcile.Request{}) - Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(BeZero()) - }) - }) - Context("when RequeueWithBackoff is true", func() { - BeforeEach(func() { - mockReconciler.result = reconciler.Result{ - RequeueWithBackoff: true, - } - }) - It("should return a result with RequeueAfter set", func() { - result, err := singleton.AsReconciler(mockReconciler).Reconcile(context.Background(), reconcile.Request{}) - Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(BeNumerically(">=", 0)) - }) - It("should use controller name for rate limiting", func() { - // Test with different controller names to ensure they're handled independently - controller1 := &MockReconciler{ - name: "controller-1", - result: reconciler.Result{RequeueWithBackoff: true}, - } - controller2 := &MockReconciler{ - name: "controller-2", - result: reconciler.Result{RequeueWithBackoff: true}, - } - - reconciler1 := singleton.AsReconciler(controller1) - reconciler2 := singleton.AsReconciler(controller2) - - // Each controller should get its own rate limiting - result1, err1 := reconciler1.Reconcile(context.Background(), reconcile.Request{}) - result2, err2 := reconciler2.Reconcile(context.Background(), reconcile.Request{}) - - Expect(err1).NotTo(HaveOccurred()) - Expect(err2).NotTo(HaveOccurred()) - Expect(result1.RequeueAfter).To(BeNumerically(">=", 0)) - Expect(result2.RequeueAfter).To(BeNumerically(">=", 0)) - }) - It("should implement exponential backoff on repeated calls", func() { - // Multiple calls to the same controller should show increasing delays - delays := make([]time.Duration, 5) - - for i := 0; i < 5; i++ { - result, err := singleton.AsReconciler(mockReconciler).Reconcile(context.Background(), reconcile.Request{}) - Expect(err).NotTo(HaveOccurred()) - delays[i] = result.RequeueAfter - } + reconciler1 := singleton.AsReconciler(controller1) + reconciler2 := singleton.AsReconciler(controller2) - // Verify generally increasing pattern (allowing for some variance in rate limiting) - for i := 1; i < len(delays); i++ { - Expect(delays[i]).To(BeNumerically(">=", delays[i-1]), - "Delay at index %d (%v) should be >= delay at index %d (%v)", - i, delays[i], i-1, delays[i-1]) - } - }) - }) - It("should return the error without processing backoff", func() { - mockReconciler.result = reconciler.Result{RequeueWithBackoff: true} - mockReconciler.err = errors.New("test error") - result, err := singleton.AsReconciler(mockReconciler).Reconcile(context.Background(), reconcile.Request{}) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(Equal("test error")) - Expect(result.RequeueAfter).To(BeZero()) - }) - It("should work with RequeueImmediately", func() { - mockReconciler.result = reconciler.Result{ - Result: reconcile.Result{RequeueAfter: singleton.RequeueImmediately}, - } + // Each controller should get its own rate limiting + result1, err1 := reconciler1.Reconcile(context.Background(), reconcile.Request{}) + result2, err2 := reconciler2.Reconcile(context.Background(), reconcile.Request{}) - result, err := singleton.AsReconciler(mockReconciler).Reconcile(context.Background(), reconcile.Request{}) - Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(Equal(singleton.RequeueImmediately)) + Expect(err1).NotTo(HaveOccurred()) + Expect(err2).NotTo(HaveOccurred()) + Expect(result1.RequeueAfter).To(BeNumerically(">=", 0)) + Expect(result2.RequeueAfter).To(BeNumerically(">=", 0)) }) }) }) From 3ab7250fbb53802635e31a29ba4172de1f31f45c Mon Sep 17 00:00:00 2001 From: Reed Schalo Date: Wed, 20 Aug 2025 15:08:06 -0700 Subject: [PATCH 11/13] pr responses --- reconciler/reconciler.go | 19 ++--- reconciler/suite_test.go | 180 ++++++++++++++++++++++----------------- singleton/controller.go | 10 ++- singleton/suite_test.go | 39 +-------- 4 files changed, 121 insertions(+), 127 deletions(-) diff --git a/reconciler/reconciler.go b/reconciler/reconciler.go index 91e6c63..c4f64c9 100644 --- a/reconciler/reconciler.go +++ b/reconciler/reconciler.go @@ -2,6 +2,7 @@ package reconciler import ( "context" + "time" "k8s.io/client-go/util/workqueue" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -9,21 +10,13 @@ import ( // Result is a wrapper around reconcile.Result that adds RequeueWithBackoff functionality. type Result struct { - reconcile.Result - Requeue bool + RequeueAfter time.Duration + Requeue bool } // Reconciler defines the interface for standard reconcilers type Reconciler interface { - Reconcile(ctx context.Context) (Result, error) -} - -// ReconcilerFunc is a function type that implements the Reconciler interface. -type ReconcilerFunc func(ctx context.Context) (Result, error) - -// Reconcile implements the Reconciler interface. -func (r ReconcilerFunc) Func(ctx context.Context) (Result, error) { - return r(ctx) + Reconcile(ctx context.Context, req reconcile.Request) (Result, error) } // AsReconciler creates a reconciler from a standard reconciler @@ -40,7 +33,7 @@ func AsReconcilerWithRateLimiter( rateLimiter workqueue.TypedRateLimiter[reconcile.Request], ) reconcile.Reconciler { return reconcile.Func(func(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { - result, err := reconciler.Reconcile(ctx) + result, err := reconciler.Reconcile(ctx, req) if err != nil { return reconcile.Result{}, err } @@ -51,6 +44,6 @@ func AsReconcilerWithRateLimiter( return reconcile.Result{RequeueAfter: rateLimiter.When(req)}, nil } rateLimiter.Forget(req) - return result.Result, nil + return reconcile.Result{}, nil }) } diff --git a/reconciler/suite_test.go b/reconciler/suite_test.go index 3d7f832..6d985f3 100644 --- a/reconciler/suite_test.go +++ b/reconciler/suite_test.go @@ -9,6 +9,7 @@ import ( "github.com/awslabs/operatorpkg/reconciler" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) @@ -20,7 +21,7 @@ func Test(t *testing.T) { // MockRateLimiter is a mock implementation of workqueue.TypedRateLimiter for testing type MockRateLimiter[K comparable] struct { whenFunc func(K) time.Duration - numRequeues int + numRequeues map[K]int backoffDuration time.Duration } @@ -28,135 +29,134 @@ func (m *MockRateLimiter[K]) When(key K) time.Duration { if m.whenFunc != nil { return m.whenFunc(key) } - m.numRequeues++ + // Default implementation + if m.numRequeues == nil { + m.numRequeues = make(map[K]int) + } + m.numRequeues[key] += 1 return m.backoffDuration } func (m *MockRateLimiter[K]) NumRequeues(key K) int { - return m.numRequeues + return m.numRequeues[key] } func (m *MockRateLimiter[K]) Forget(key K) { - m.numRequeues = 0 + delete(m.numRequeues, key) } // MockReconciler is a mock implementation of Reconciler for testing type MockReconciler struct { - reconcileFunc func(context.Context) (reconciler.Result, error) + reconcileFunc func(context.Context, reconcile.Request) (reconciler.Result, error) result reconciler.Result err error } -func (m *MockReconciler) Reconcile(ctx context.Context) (reconciler.Result, error) { +func (m *MockReconciler) Reconcile(ctx context.Context, req reconcile.Request) (reconciler.Result, error) { if m.reconcileFunc != nil { - return m.reconcileFunc(ctx) + return m.reconcileFunc(ctx, req) } return m.result, m.err } var _ = Describe("Reconciler", func() { - It("should return the original result without backoff", func() { - backoff := 5 * time.Second - // Create a mock reconciler + It("should return the result without backoff", func() { + mockReconciler := &MockReconciler{ + result: reconciler.Result{}, + } + + reconciler := reconciler.AsReconciler(mockReconciler) + + result, err := reconciler.Reconcile(context.Background(), reconcile.Request{}) + + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(0 * time.Second)) + }) + It("should return the result with backoff when Requeue is set", func() { mockReconciler := &MockReconciler{ result: reconciler.Result{ - Result: reconcile.Result{ - RequeueAfter: backoff, - }, - Requeue: false, + Requeue: true, }, } - // Create the reconciler adapter - adapter := reconciler.AsReconciler(mockReconciler) + reconciler := reconciler.AsReconciler(mockReconciler) - // Call the adapter - ctx := context.Background() - req := reconcile.Request{} - result, err := adapter.Reconcile(ctx, req) + result, err := reconciler.Reconcile(context.Background(), reconcile.Request{}) - // Verify the result Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(Equal(backoff)) + Expect(result.RequeueAfter).To(Equal(5 * time.Millisecond)) }) - It("should return the original result without backoff when Requeue is not set", func() { + It("should return the result with backoff when both RequeueAfter and Requeue are set", func() { mockReconciler := &MockReconciler{ - result: reconciler.Result{}, + result: reconciler.Result{ + RequeueAfter: 10 * time.Second, + Requeue: true, + }, } - // Create the reconciler adapter - adapter := reconciler.AsReconciler(mockReconciler) + reconciler := reconciler.AsReconciler(mockReconciler) - // Call the adapter - ctx := context.Background() - req := reconcile.Request{} - result, err := adapter.Reconcile(ctx, req) + result, err := reconciler.Reconcile(context.Background(), reconcile.Request{}) - // Verify the result Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(BeZero()) + Expect(result.RequeueAfter).To(Equal(10 * time.Second)) }) - It("should return a result with Requeue set", func() { - // Create a mock reconciler that returns Requeue = true + It("should return the result with backoff when RequeueAfter is set and Requeue is false", func() { mockReconciler := &MockReconciler{ result: reconciler.Result{ - Result: reconcile.Result{}, - Requeue: true, + RequeueAfter: 10 * time.Second, + Requeue: false, }, } - // Create the reconciler adapter - adapter := reconciler.AsReconciler(mockReconciler) + reconciler := reconciler.AsReconciler(mockReconciler) + + result, err := reconciler.Reconcile(context.Background(), reconcile.Request{}) - // Call the adapter - ctx := context.Background() - req := reconcile.Request{} - result, err := adapter.Reconcile(ctx, req) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(10 * time.Second)) + }) + It("should return the result with backoff when RequeueAfter is set to zero and Requeue is true", func() { + mockReconciler := &MockReconciler{ + result: reconciler.Result{ + RequeueAfter: 0 * time.Second, + Requeue: true, + }, + } + + reconciler := reconciler.AsReconciler(mockReconciler) + + result, err := reconciler.Reconcile(context.Background(), reconcile.Request{}) - // Verify the result - should have some backoff duration Expect(err).NotTo(HaveOccurred()) Expect(result.RequeueAfter).To(Equal(5 * time.Millisecond)) }) - It("should return a result with RequeueAfter when both RequeueAfter and Requeue are set", func() { - // Create a mock reconciler that returns Requeue = true + It("should return the result without backoff when RequeueAfter is set to zero and Requeue is false", func() { mockReconciler := &MockReconciler{ result: reconciler.Result{ - Result: reconcile.Result{ - RequeueAfter: 10 * time.Second, - }, - Requeue: true, + RequeueAfter: 0 * time.Second, + Requeue: false, }, } - // Create the reconciler adapter - adapter := reconciler.AsReconciler(mockReconciler) + reconciler := reconciler.AsReconciler(mockReconciler) - // Call the adapter - ctx := context.Background() - req := reconcile.Request{} - result, err := adapter.Reconcile(ctx, req) + result, err := reconciler.Reconcile(context.Background(), reconcile.Request{}) - // Verify the result - should have some backoff duration Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(Equal(10 * time.Second)) + Expect(result.RequeueAfter).To(Equal(0 * time.Millisecond)) }) It("should return the error without processing backoff", func() { - // Create a mock reconciler that returns an error expectedErr := errors.New("test error") mockReconciler := &MockReconciler{ result: reconciler.Result{Requeue: true}, err: expectedErr, } - // Create the reconciler adapter - adapter := reconciler.AsReconciler(mockReconciler) + reconciler := reconciler.AsReconciler(mockReconciler) - // Call the adapter - ctx := context.Background() - req := reconcile.Request{} - result, err := adapter.Reconcile(ctx, req) + result, err := reconciler.Reconcile(context.Background(), reconcile.Request{}) - // Verify that the error is propagated Expect(err).To(HaveOccurred()) Expect(err).To(Equal(expectedErr)) Expect(result.RequeueAfter).To(BeZero()) @@ -166,31 +166,59 @@ var _ = Describe("Reconciler", func() { backoffDuration: 10 * time.Second, } - // Create a mock reconciler that returns Requeue = true mockReconciler := &MockReconciler{ result: reconciler.Result{ - Result: reconcile.Result{}, Requeue: true, }, } - // Create the reconciler adapter with custom rate limiter - adapter := reconciler.AsReconcilerWithRateLimiter(mockReconciler, mockRateLimiter) + reconciler := reconciler.AsReconcilerWithRateLimiter(mockReconciler, mockRateLimiter) - // Call the adapter - ctx := context.Background() req := reconcile.Request{} - result, err := adapter.Reconcile(ctx, req) + result, err := reconciler.Reconcile(context.Background(), req) - // Verify the result Expect(err).NotTo(HaveOccurred()) Expect(result.RequeueAfter).To(Equal(10 * time.Second)) Expect(mockRateLimiter.NumRequeues(req)).To(Equal(1)) }) + It("should rate limit distinct items", func() { + mockRateLimiter := &MockRateLimiter[reconcile.Request]{ + backoffDuration: 10 * time.Second, + } + + mockReconciler := &MockReconciler{ + result: reconciler.Result{ + Requeue: true, + }, + } + + reconciler := reconciler.AsReconcilerWithRateLimiter(mockReconciler, mockRateLimiter) + + req1 := reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: "req1", + Namespace: "", + }, + } + result1, err1 := reconciler.Reconcile(context.Background(), req1) + req2 := reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: "req2", + Namespace: "", + }, + } + result2, err2 := reconciler.Reconcile(context.Background(), req2) + + Expect(err1).NotTo(HaveOccurred()) + Expect(result1.RequeueAfter).To(Equal(10 * time.Second)) + Expect(err2).NotTo(HaveOccurred()) + Expect(result2.RequeueAfter).To(Equal(10 * time.Second)) + Expect(mockRateLimiter.NumRequeues(req1)).To(Equal(1)) + Expect(mockRateLimiter.NumRequeues(req2)).To(Equal(1)) + }) It("should implement exponential backoff on repeated calls", func() { mockReconciler := &MockReconciler{ result: reconciler.Result{ - Result: reconcile.Result{}, Requeue: true, }, } @@ -204,7 +232,6 @@ var _ = Describe("Reconciler", func() { delays[i] = result.RequeueAfter } - // Verify generally increasing pattern initialDelay := 5 * time.Millisecond Expect(delays[0]).To(BeNumerically("==", initialDelay)) for i := 1; i < len(delays); i++ { @@ -218,11 +245,10 @@ var _ = Describe("Reconciler", func() { It("should forget an item when reconcile succeeds", func() { mockReconciler := &MockReconciler{ result: reconciler.Result{ - Result: reconcile.Result{}, Requeue: false, }, } - // Multiple calls to the same controller should show zero requeue + // Multiple calls to the same controller should show zero requeues reconciler := reconciler.AsReconciler(mockReconciler) for i := 0; i < 5; i++ { diff --git a/singleton/controller.go b/singleton/controller.go index 7ef1b93..0e67d8d 100644 --- a/singleton/controller.go +++ b/singleton/controller.go @@ -22,6 +22,13 @@ const ( type Reconciler interface { Reconcile(ctx context.Context) (reconciler.Result, error) } +type reconcilerAdapter struct { + Reconciler +} + +func (r *reconcilerAdapter) Reconcile(ctx context.Context, _ reconcile.Request) (reconciler.Result, error) { + return r.Reconciler.Reconcile(ctx) +} // In response to Requeue: True being deprecated via: https://github.com/kubernetes-sigs/controller-runtime/pull/3107/files // This uses a bucket and per item delay but the item will be the same because the key is the controller name. @@ -29,7 +36,8 @@ type Reconciler interface { // AsReconciler creates a controller-runtime reconciler from a singleton reconciler func AsReconciler(rec Reconciler) reconcile.Reconciler { - return reconciler.AsReconciler(rec) + adapter := &reconcilerAdapter{Reconciler: rec} + return reconciler.AsReconciler(adapter) } // Source creates a source for singleton controllers diff --git a/singleton/suite_test.go b/singleton/suite_test.go index 7b0d742..576d279 100644 --- a/singleton/suite_test.go +++ b/singleton/suite_test.go @@ -3,7 +3,6 @@ package singleton_test import ( "context" "testing" - "time" "github.com/awslabs/operatorpkg/reconciler" "github.com/awslabs/operatorpkg/singleton" @@ -17,43 +16,14 @@ func Test(t *testing.T) { RunSpecs(t, "Singleton") } -var ( - mockReconciler *MockReconciler -) - -type MockRateLimiter[K comparable] struct { - whenFunc func(K) time.Duration - numRequeues int - backoffDuration time.Duration -} - -func (m *MockRateLimiter[K]) When(key K) time.Duration { - if m.whenFunc != nil { - return m.whenFunc(key) - } - m.numRequeues++ - return m.backoffDuration -} - -func (m *MockRateLimiter[K]) NumRequeues(key K) int { - return m.numRequeues -} - -func (m *MockRateLimiter[K]) Forget(key K) { - m.numRequeues = 0 -} +var mockReconciler *MockReconciler // MockReconciler for testing type MockReconciler struct { - name string result reconciler.Result err error } -func (m *MockReconciler) Name() string { - return m.name -} - func (m *MockReconciler) Reconcile(ctx context.Context) (reconciler.Result, error) { return m.result, m.err } @@ -61,18 +31,14 @@ func (m *MockReconciler) Reconcile(ctx context.Context) (reconciler.Result, erro var _ = Describe("Singleton Controller", func() { Context("AsReconciler", func() { BeforeEach(func() { - mockReconciler = &MockReconciler{ - name: "test-controller", - } + mockReconciler = &MockReconciler{} }) It("should return a result with RequeueAfter that is scoped to a controller", func() { // Test with different controllers to ensure they're handled independently controller1 := &MockReconciler{ - name: "controller-1", result: reconciler.Result{Requeue: true}, } controller2 := &MockReconciler{ - name: "controller-2", result: reconciler.Result{Requeue: true}, } @@ -87,6 +53,7 @@ var _ = Describe("Singleton Controller", func() { Expect(err2).NotTo(HaveOccurred()) Expect(result1.RequeueAfter).To(BeNumerically(">=", 0)) Expect(result2.RequeueAfter).To(BeNumerically(">=", 0)) + Expect(result1.RequeueAfter).To(Equal(result2.RequeueAfter)) }) }) }) From 4ccfb14e7cf1794b18ecee1b2dea506c4f194df9 Mon Sep 17 00:00:00 2001 From: Reed Schalo Date: Wed, 20 Aug 2025 15:09:19 -0700 Subject: [PATCH 12/13] fix comments --- reconciler/reconciler.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/reconciler/reconciler.go b/reconciler/reconciler.go index c4f64c9..6375d6e 100644 --- a/reconciler/reconciler.go +++ b/reconciler/reconciler.go @@ -8,7 +8,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ) -// Result is a wrapper around reconcile.Result that adds RequeueWithBackoff functionality. +// Result adds Requeue functionality back to reconcile results. type Result struct { RequeueAfter time.Duration Requeue bool @@ -19,7 +19,7 @@ type Reconciler interface { Reconcile(ctx context.Context, req reconcile.Request) (Result, error) } -// AsReconciler creates a reconciler from a standard reconciler +// AsReconciler creates a reconciler with a default rate-limiter func AsReconciler(reconciler Reconciler) reconcile.Reconciler { return AsReconcilerWithRateLimiter( reconciler, From 2ef6efdc03d4bd49c4e59f0337d8a008ff33c9fd Mon Sep 17 00:00:00 2001 From: Reed Schalo Date: Thu, 21 Aug 2025 12:27:53 -0700 Subject: [PATCH 13/13] add forget to requeueAfter and move test to reconciler --- reconciler/reconciler.go | 1 + reconciler/suite_test.go | 26 ++++++++++++++++++ singleton/suite_test.go | 59 ---------------------------------------- 3 files changed, 27 insertions(+), 59 deletions(-) delete mode 100644 singleton/suite_test.go diff --git a/reconciler/reconciler.go b/reconciler/reconciler.go index 6375d6e..4921cb1 100644 --- a/reconciler/reconciler.go +++ b/reconciler/reconciler.go @@ -38,6 +38,7 @@ func AsReconcilerWithRateLimiter( return reconcile.Result{}, err } if result.RequeueAfter > 0 { + rateLimiter.Forget(req) return reconcile.Result{RequeueAfter: result.RequeueAfter}, nil } if result.Requeue { diff --git a/reconciler/suite_test.go b/reconciler/suite_test.go index 6d985f3..b432d39 100644 --- a/reconciler/suite_test.go +++ b/reconciler/suite_test.go @@ -257,4 +257,30 @@ var _ = Describe("Reconciler", func() { Expect(result.RequeueAfter).To(BeZero()) } }) + It("should return a result with RequeueAfter that is scoped to a controller", func() { + // Test with different controllers to ensure they're handled independently + controller1 := &MockReconciler{ + result: reconciler.Result{Requeue: true}, + } + controller2 := &MockReconciler{ + result: reconciler.Result{Requeue: true}, + } + + reconciler1 := reconciler.AsReconciler(controller1) + reconciler2 := reconciler.AsReconciler(controller2) + + // Each controller should get its own rate limiting + result1, err1 := reconciler1.Reconcile(context.Background(), reconcile.Request{}) + result2, err2 := reconciler2.Reconcile(context.Background(), reconcile.Request{}) + + Expect(err1).NotTo(HaveOccurred()) + Expect(err2).NotTo(HaveOccurred()) + Expect(result1.RequeueAfter).To(BeNumerically(">=", 0)) + Expect(result2.RequeueAfter).To(BeNumerically(">=", 0)) + Expect(result1.RequeueAfter).To(Equal(result2.RequeueAfter)) + + result2, err2 = reconciler2.Reconcile(context.Background(), reconcile.Request{}) + Expect(err2).NotTo(HaveOccurred()) + Expect(result1.RequeueAfter).NotTo(Equal(result2.RequeueAfter)) + }) }) diff --git a/singleton/suite_test.go b/singleton/suite_test.go deleted file mode 100644 index 576d279..0000000 --- a/singleton/suite_test.go +++ /dev/null @@ -1,59 +0,0 @@ -package singleton_test - -import ( - "context" - "testing" - - "github.com/awslabs/operatorpkg/reconciler" - "github.com/awslabs/operatorpkg/singleton" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - "sigs.k8s.io/controller-runtime/pkg/reconcile" -) - -func Test(t *testing.T) { - RegisterFailHandler(Fail) - RunSpecs(t, "Singleton") -} - -var mockReconciler *MockReconciler - -// MockReconciler for testing -type MockReconciler struct { - result reconciler.Result - err error -} - -func (m *MockReconciler) Reconcile(ctx context.Context) (reconciler.Result, error) { - return m.result, m.err -} - -var _ = Describe("Singleton Controller", func() { - Context("AsReconciler", func() { - BeforeEach(func() { - mockReconciler = &MockReconciler{} - }) - It("should return a result with RequeueAfter that is scoped to a controller", func() { - // Test with different controllers to ensure they're handled independently - controller1 := &MockReconciler{ - result: reconciler.Result{Requeue: true}, - } - controller2 := &MockReconciler{ - result: reconciler.Result{Requeue: true}, - } - - reconciler1 := singleton.AsReconciler(controller1) - reconciler2 := singleton.AsReconciler(controller2) - - // Each controller should get its own rate limiting - result1, err1 := reconciler1.Reconcile(context.Background(), reconcile.Request{}) - result2, err2 := reconciler2.Reconcile(context.Background(), reconcile.Request{}) - - Expect(err1).NotTo(HaveOccurred()) - Expect(err2).NotTo(HaveOccurred()) - Expect(result1.RequeueAfter).To(BeNumerically(">=", 0)) - Expect(result2.RequeueAfter).To(BeNumerically(">=", 0)) - Expect(result1.RequeueAfter).To(Equal(result2.RequeueAfter)) - }) - }) -})