From 06948ce4c2e99e20ebd31b783e82f0cdc3adc701 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sun, 7 May 2023 18:28:23 -0400 Subject: [PATCH] :warn: Flatten fields in controller.Options Currently, `controller.Options` embedds `config.Controller`. This makes its usage pretty annoying, to set `MaxConcurrentReconciles` for example, one has to do it like this: ``` controller.Options{config.Options{MaxConcurrentReconciles: 8}} ``` This makes it harder to find what options exist and causes a lot of churn for downstream consumers. Re-Define the fields from `config.Controller` in `controller.Options` instead to avoid that. This also fixes some defaulting bugs where we wouldn't default `MaxConcurrentReconciles` and `CacheSyncTimeout` from the `config.Controller` setting in the manager. --- pkg/builder/controller_test.go | 2 +- pkg/controller/controller.go | 32 ++++++- pkg/controller/controller_test.go | 135 ++++++++++++++++++++++++++++-- 3 files changed, 156 insertions(+), 13 deletions(-) diff --git a/pkg/builder/controller_test.go b/pkg/builder/controller_test.go index eb5ae8197d..dd98c2e565 100644 --- a/pkg/builder/controller_test.go +++ b/pkg/builder/controller_test.go @@ -217,7 +217,7 @@ var _ = Describe("application", func() { instance, err := ControllerManagedBy(m). For(&appsv1.ReplicaSet{}). Owns(&appsv1.ReplicaSet{}). - WithOptions(controller.Options{Controller: config.Controller{MaxConcurrentReconciles: maxConcurrentReconciles}}). + WithOptions(controller.Options{MaxConcurrentReconciles: maxConcurrentReconciles}). Build(noop) Expect(err).NotTo(HaveOccurred()) Expect(instance).NotTo(BeNil()) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index f2652d10a4..6732b6f709 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -25,7 +25,6 @@ import ( "k8s.io/client-go/util/workqueue" "k8s.io/klog/v2" - "sigs.k8s.io/controller-runtime/pkg/config" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/internal/controller" "sigs.k8s.io/controller-runtime/pkg/manager" @@ -37,7 +36,20 @@ import ( // Options are the arguments for creating a new Controller. type Options struct { - config.Controller + // MaxConcurrentReconciles is the maximum number of concurrent Reconciles which can be run. Defaults to 1. + MaxConcurrentReconciles int + + // CacheSyncTimeout refers to the time limit set to wait for syncing caches. + // Defaults to 2 minutes if not set. + CacheSyncTimeout time.Duration + + // RecoverPanic indicates whether the panic caused by reconcile should be recovered. + // Defaults to the Controller.RecoverPanic setting from the Manager if unset. + RecoverPanic *bool + + // NeedLeaderElection indicates whether the controller needs to use leader election. + // Defaults to true, which means the controller will use leader election. + NeedLeaderElection *bool // Reconciler reconciles an object Reconciler reconcile.Reconciler @@ -116,11 +128,19 @@ func NewUnmanaged(name string, mgr manager.Manager, options Options) (Controller } if options.MaxConcurrentReconciles <= 0 { - options.MaxConcurrentReconciles = 1 + if mgr.GetControllerOptions().MaxConcurrentReconciles > 0 { + options.MaxConcurrentReconciles = mgr.GetControllerOptions().MaxConcurrentReconciles + } else { + options.MaxConcurrentReconciles = 1 + } } if options.CacheSyncTimeout == 0 { - options.CacheSyncTimeout = 2 * time.Minute + if mgr.GetControllerOptions().CacheSyncTimeout != 0 { + options.CacheSyncTimeout = mgr.GetControllerOptions().CacheSyncTimeout + } else { + options.CacheSyncTimeout = 2 * time.Minute + } } if options.RateLimiter == nil { @@ -131,6 +151,10 @@ func NewUnmanaged(name string, mgr manager.Manager, options Options) (Controller options.RecoverPanic = mgr.GetControllerOptions().RecoverPanic } + if options.NeedLeaderElection == nil { + options.NeedLeaderElection = mgr.GetControllerOptions().NeedLeaderElection + } + // Create controller with dependencies set return &controller.Controller{ Do: options.Reconciler, diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 5f20f87f1c..e2432b43d3 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -154,10 +154,8 @@ var _ = Describe("controller.Controller", func() { Expect(err).NotTo(HaveOccurred()) c, err := controller.New("new-controller", m, controller.Options{ - Controller: config.Controller{ - RecoverPanic: pointer.Bool(false), - }, - Reconciler: reconcile.Func(nil), + RecoverPanic: pointer.Bool(false), + Reconciler: reconcile.Func(nil), }) Expect(err).NotTo(HaveOccurred()) @@ -168,6 +166,129 @@ var _ = Describe("controller.Controller", func() { Expect(*ctrl.RecoverPanic).To(BeFalse()) }) + It("should default NeedLeaderElection from the manager", func() { + m, err := manager.New(cfg, manager.Options{Controller: config.Controller{NeedLeaderElection: pointer.Bool(true)}}) + Expect(err).NotTo(HaveOccurred()) + + c, err := controller.New("new-controller", m, controller.Options{ + Reconciler: reconcile.Func(nil), + }) + Expect(err).NotTo(HaveOccurred()) + + ctrl, ok := c.(*internalcontroller.Controller) + Expect(ok).To(BeTrue()) + + Expect(ctrl.NeedLeaderElection()).To(BeTrue()) + }) + + It("should not override NeedLeaderElection on the controller", func() { + m, err := manager.New(cfg, manager.Options{Controller: config.Controller{NeedLeaderElection: pointer.Bool(true)}}) + Expect(err).NotTo(HaveOccurred()) + + c, err := controller.New("new-controller", m, controller.Options{ + NeedLeaderElection: pointer.Bool(false), + Reconciler: reconcile.Func(nil), + }) + Expect(err).NotTo(HaveOccurred()) + + ctrl, ok := c.(*internalcontroller.Controller) + Expect(ok).To(BeTrue()) + + Expect(ctrl.NeedLeaderElection).To(BeFalse()) + }) + + It("Should default MaxConcurrentReconciles from the manager if set", func() { + m, err := manager.New(cfg, manager.Options{Controller: config.Controller{MaxConcurrentReconciles: 5}}) + Expect(err).NotTo(HaveOccurred()) + + c, err := controller.New("new-controller", m, controller.Options{ + Reconciler: reconcile.Func(nil), + }) + Expect(err).NotTo(HaveOccurred()) + + ctrl, ok := c.(*internalcontroller.Controller) + Expect(ok).To(BeTrue()) + + Expect(ctrl.MaxConcurrentReconciles).To(BeEquivalentTo(5)) + }) + + It("Should default MaxConcurrentReconciles to 1 if unset", func() { + m, err := manager.New(cfg, manager.Options{}) + Expect(err).NotTo(HaveOccurred()) + + c, err := controller.New("new-controller", m, controller.Options{ + Reconciler: reconcile.Func(nil), + }) + Expect(err).NotTo(HaveOccurred()) + + ctrl, ok := c.(*internalcontroller.Controller) + Expect(ok).To(BeTrue()) + + Expect(ctrl.MaxConcurrentReconciles).To(BeEquivalentTo(1)) + }) + + It("Should leave MaxConcurrentReconciles if set", func() { + m, err := manager.New(cfg, manager.Options{}) + Expect(err).NotTo(HaveOccurred()) + + c, err := controller.New("new-controller", m, controller.Options{ + Reconciler: reconcile.Func(nil), + MaxConcurrentReconciles: 5, + }) + Expect(err).NotTo(HaveOccurred()) + + ctrl, ok := c.(*internalcontroller.Controller) + Expect(ok).To(BeTrue()) + + Expect(ctrl.MaxConcurrentReconciles).To(BeEquivalentTo(5)) + }) + + It("Should default CacheSyncTimeout from the manager if set", func() { + m, err := manager.New(cfg, manager.Options{Controller: config.Controller{CacheSyncTimeout: 5}}) + Expect(err).NotTo(HaveOccurred()) + + c, err := controller.New("new-controller", m, controller.Options{ + Reconciler: reconcile.Func(nil), + }) + Expect(err).NotTo(HaveOccurred()) + + ctrl, ok := c.(*internalcontroller.Controller) + Expect(ok).To(BeTrue()) + + Expect(ctrl.CacheSyncTimeout).To(BeEquivalentTo(5)) + }) + + It("Should default CacheSyncTimeout to 2 minutes if unset", func() { + m, err := manager.New(cfg, manager.Options{}) + Expect(err).NotTo(HaveOccurred()) + + c, err := controller.New("new-controller", m, controller.Options{ + Reconciler: reconcile.Func(nil), + }) + Expect(err).NotTo(HaveOccurred()) + + ctrl, ok := c.(*internalcontroller.Controller) + Expect(ok).To(BeTrue()) + + Expect(ctrl.CacheSyncTimeout).To(BeEquivalentTo(2 * time.Minute)) + }) + + It("Should leave CacheSyncTimeout if set", func() { + m, err := manager.New(cfg, manager.Options{}) + Expect(err).NotTo(HaveOccurred()) + + c, err := controller.New("new-controller", m, controller.Options{ + Reconciler: reconcile.Func(nil), + CacheSyncTimeout: 5, + }) + Expect(err).NotTo(HaveOccurred()) + + ctrl, ok := c.(*internalcontroller.Controller) + Expect(ok).To(BeTrue()) + + Expect(ctrl.CacheSyncTimeout).To(BeEquivalentTo(5)) + }) + It("should default NeedLeaderElection on the controller to true", func() { m, err := manager.New(cfg, manager.Options{}) Expect(err).NotTo(HaveOccurred()) @@ -188,10 +309,8 @@ var _ = Describe("controller.Controller", func() { Expect(err).NotTo(HaveOccurred()) c, err := controller.New("new-controller", m, controller.Options{ - Controller: config.Controller{ - NeedLeaderElection: pointer.Bool(false), - }, - Reconciler: rec, + NeedLeaderElection: pointer.Bool(false), + Reconciler: rec, }) Expect(err).NotTo(HaveOccurred())