From d9ff283bfe844e8e3806eb2d264b2a6fa7815f66 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Tue, 4 Mar 2025 14:06:37 -0500 Subject: [PATCH] :warning: NewTypedUnmanaged: Stop requiring a manager Currently, `NewUnmanaged` and `NewTypedUnmanaged` require a manager but the only properties they ever access from it are `GetControllerOptions` and `GetLogger`. It makes no sense to require a `Manager` for something that is unmanaged, so remove that. Doing so naively means that we require two options parameters for these constructors, one that has the actual options and another one to default from which again is confusing. Remove the options struct to default from and instead implement a `DefaultFromConfig` that defaults the controller options `config.Controller` which also clarifies what belongs where. This defaulting gets automatically called in the managed constructors and is exported so anyone can use it. This change is breaking only for users of `New[Typed]Unmanaged` in that: * The signature changes * The defaulting of the `controller.Options` from `config.Controller` doesn't happen anymore, but it is still possible to do that explicitly --- pkg/config/controller.go | 13 ++++- pkg/controller/controller.go | 86 +++++++++++++++++++++------------- pkg/controller/example_test.go | 2 +- pkg/manager/manager.go | 4 ++ pkg/manager/manager_test.go | 41 ++++++++++++++++ 5 files changed, 111 insertions(+), 35 deletions(-) diff --git a/pkg/config/controller.go b/pkg/config/controller.go index 0b2aa0cb7b..a5655593ef 100644 --- a/pkg/config/controller.go +++ b/pkg/config/controller.go @@ -16,9 +16,15 @@ limitations under the License. package config -import "time" +import ( + "time" -// Controller contains configuration options for a controller. + "github.com/go-logr/logr" +) + +// Controller contains configuration options for controllers. It only includes options +// that makes sense for a set of controllers and is used for defaulting the options +// of multiple controllers. type Controller struct { // SkipNameValidation allows skipping the name validation that ensures that every controller name is unique. // Unique controller names are important to get unique metrics and logs for a controller. @@ -59,4 +65,7 @@ type Controller struct { // // Note: This flag is disabled by default until a future version. It's currently in beta. UsePriorityQueue *bool + + // Logger is the logger controllers should use. + Logger logr.Logger } diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 5c5b249ef5..9de959b48f 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -26,6 +26,7 @@ import ( "k8s.io/klog/v2" "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/config" "sigs.k8s.io/controller-runtime/pkg/controller/priorityqueue" "sigs.k8s.io/controller-runtime/pkg/internal/controller" "sigs.k8s.io/controller-runtime/pkg/manager" @@ -80,13 +81,53 @@ type TypedOptions[request comparable] struct { // Only use a custom NewQueue if you know what you are doing. NewQueue func(controllerName string, rateLimiter workqueue.TypedRateLimiter[request]) workqueue.TypedRateLimitingInterface[request] + // Logger will be used to build a default LogConstructor if unset. + Logger logr.Logger + // LogConstructor is used to construct a logger used for this controller and passed // to each reconciliation via the context field. LogConstructor func(request *request) logr.Logger + + // UsePriorityQueue configures the controllers queue to use the controller-runtime provided + // priority queue. + // + // Note: This flag is disabled by default until a future version. It's currently in beta. + UsePriorityQueue *bool +} + +// DefaultFromConfig defaults the config from a config.Controller +func (options *TypedOptions[request]) DefaultFromConfig(config config.Controller) { + if options.Logger.GetSink() == nil { + options.Logger = config.Logger + } + + if options.SkipNameValidation == nil { + options.SkipNameValidation = config.SkipNameValidation + } + + if options.MaxConcurrentReconciles <= 0 && config.MaxConcurrentReconciles > 0 { + options.MaxConcurrentReconciles = config.MaxConcurrentReconciles + } + + if options.CacheSyncTimeout == 0 && config.CacheSyncTimeout > 0 { + options.CacheSyncTimeout = config.CacheSyncTimeout + } + + if options.UsePriorityQueue == nil { + options.UsePriorityQueue = config.UsePriorityQueue + } + + if options.RecoverPanic == nil { + options.RecoverPanic = config.RecoverPanic + } + + if options.NeedLeaderElection == nil { + options.NeedLeaderElection = config.NeedLeaderElection + } } -// Controller implements a Kubernetes API. A Controller manages a work queue fed reconcile.Requests -// from source.Sources. Work is performed through the reconcile.Reconciler for each enqueued item. +// Controller implements an API. A Controller manages a work queue fed reconcile.Requests +// from source.Sources. Work is performed through the reconcile.Reconciler for each enqueued item. // Work typically is reads and writes Kubernetes objects to make the system state match the state specified // in the object Spec. type Controller = TypedController[reconcile.Request] @@ -119,7 +160,8 @@ func New(name string, mgr manager.Manager, options Options) (Controller, error) // // The name must be unique as it is used to identify the controller in metrics and logs. func NewTyped[request comparable](name string, mgr manager.Manager, options TypedOptions[request]) (TypedController[request], error) { - c, err := NewTypedUnmanaged(name, mgr, options) + options.DefaultFromConfig(mgr.GetControllerOptions()) + c, err := NewTypedUnmanaged(name, options) if err != nil { return nil, err } @@ -132,14 +174,14 @@ func NewTyped[request comparable](name string, mgr manager.Manager, options Type // caller is responsible for starting the returned controller. // // The name must be unique as it is used to identify the controller in metrics and logs. -func NewUnmanaged(name string, mgr manager.Manager, options Options) (Controller, error) { - return NewTypedUnmanaged(name, mgr, options) +func NewUnmanaged(name string, options Options) (Controller, error) { + return NewTypedUnmanaged(name, options) } // NewTypedUnmanaged returns a new typed controller without adding it to the manager. // // The name must be unique as it is used to identify the controller in metrics and logs. -func NewTypedUnmanaged[request comparable](name string, mgr manager.Manager, options TypedOptions[request]) (TypedController[request], error) { +func NewTypedUnmanaged[request comparable](name string, options TypedOptions[request]) (TypedController[request], error) { if options.Reconciler == nil { return nil, fmt.Errorf("must specify Reconciler") } @@ -148,10 +190,6 @@ func NewTypedUnmanaged[request comparable](name string, mgr manager.Manager, opt return nil, fmt.Errorf("must specify Name for Controller") } - if options.SkipNameValidation == nil { - options.SkipNameValidation = mgr.GetControllerOptions().SkipNameValidation - } - if options.SkipNameValidation == nil || !*options.SkipNameValidation { if err := checkName(name); err != nil { return nil, err @@ -159,7 +197,7 @@ func NewTypedUnmanaged[request comparable](name string, mgr manager.Manager, opt } if options.LogConstructor == nil { - log := mgr.GetLogger().WithValues( + log := options.Logger.WithValues( "controller", name, ) options.LogConstructor = func(in *request) logr.Logger { @@ -175,23 +213,15 @@ func NewTypedUnmanaged[request comparable](name string, mgr manager.Manager, opt } if options.MaxConcurrentReconciles <= 0 { - if mgr.GetControllerOptions().MaxConcurrentReconciles > 0 { - options.MaxConcurrentReconciles = mgr.GetControllerOptions().MaxConcurrentReconciles - } else { - options.MaxConcurrentReconciles = 1 - } + options.MaxConcurrentReconciles = 1 } if options.CacheSyncTimeout == 0 { - if mgr.GetControllerOptions().CacheSyncTimeout != 0 { - options.CacheSyncTimeout = mgr.GetControllerOptions().CacheSyncTimeout - } else { - options.CacheSyncTimeout = 2 * time.Minute - } + options.CacheSyncTimeout = 2 * time.Minute } if options.RateLimiter == nil { - if ptr.Deref(mgr.GetControllerOptions().UsePriorityQueue, false) { + if ptr.Deref(options.UsePriorityQueue, false) { options.RateLimiter = workqueue.NewTypedItemExponentialFailureRateLimiter[request](5*time.Millisecond, 1000*time.Second) } else { options.RateLimiter = workqueue.DefaultTypedControllerRateLimiter[request]() @@ -200,9 +230,9 @@ func NewTypedUnmanaged[request comparable](name string, mgr manager.Manager, opt if options.NewQueue == nil { options.NewQueue = func(controllerName string, rateLimiter workqueue.TypedRateLimiter[request]) workqueue.TypedRateLimitingInterface[request] { - if ptr.Deref(mgr.GetControllerOptions().UsePriorityQueue, false) { + if ptr.Deref(options.UsePriorityQueue, false) { return priorityqueue.New(controllerName, func(o *priorityqueue.Opts[request]) { - o.Log = mgr.GetLogger().WithValues("controller", controllerName) + o.Log = options.Logger.WithValues("controller", controllerName) o.RateLimiter = rateLimiter }) } @@ -212,14 +242,6 @@ func NewTypedUnmanaged[request comparable](name string, mgr manager.Manager, opt } } - if options.RecoverPanic == nil { - options.RecoverPanic = mgr.GetControllerOptions().RecoverPanic - } - - if options.NeedLeaderElection == nil { - options.NeedLeaderElection = mgr.GetControllerOptions().NeedLeaderElection - } - // Create controller with dependencies set return &controller.Controller[request]{ Do: options.Reconciler, diff --git a/pkg/controller/example_test.go b/pkg/controller/example_test.go index aea5943450..e3c4b6a092 100644 --- a/pkg/controller/example_test.go +++ b/pkg/controller/example_test.go @@ -129,7 +129,7 @@ func ExampleNewUnmanaged() { // Configure creates a new controller but does not add it to the supplied // manager. - c, err := controller.NewUnmanaged("pod-controller", mgr, controller.Options{ + c, err := controller.NewUnmanaged("pod-controller", controller.Options{ Reconciler: reconcile.Func(func(context.Context, reconcile.Request) (reconcile.Result, error) { return reconcile.Result{}, nil }), diff --git a/pkg/manager/manager.go b/pkg/manager/manager.go index 92906fe6ca..c3ae317b04 100644 --- a/pkg/manager/manager.go +++ b/pkg/manager/manager.go @@ -544,6 +544,10 @@ func setOptionsDefaults(options Options) Options { options.Logger = log.Log } + if options.Controller.Logger.GetSink() == nil { + options.Controller.Logger = options.Logger + } + if options.BaseContext == nil { options.BaseContext = defaultBaseContext } diff --git a/pkg/manager/manager_test.go b/pkg/manager/manager_test.go index 80d6e4de1d..2c0cfa5969 100644 --- a/pkg/manager/manager_test.go +++ b/pkg/manager/manager_test.go @@ -1056,6 +1056,47 @@ var _ = Describe("manger.Manager", func() { ))) }) + It("should default controller logger from manager logger", func() { + var lock sync.Mutex + var messages []string + options.Logger = funcr.NewJSON(func(object string) { + lock.Lock() + messages = append(messages, object) + lock.Unlock() + }, funcr.Options{}) + options.LeaderElection = false + + m, err := New(cfg, options) + Expect(err).NotTo(HaveOccurred()) + for _, cb := range callbacks { + cb(m) + } + + started := make(chan struct{}) + Expect(m.Add(RunnableFunc(func(ctx context.Context) error { + close(started) + return nil + }))).To(Succeed()) + + stopped := make(chan error) + ctx, cancel := context.WithCancel(context.Background()) + go func() { + stopped <- m.Start(ctx) + }() + + // Wait for runnables to start as a proxy for the manager being fully started. + <-started + cancel() + Expect(<-stopped).To(Succeed()) + + msg := "controller log message" + m.GetControllerOptions().Logger.Info(msg) + + Expect(messages).To(ContainElement( + ContainSubstring(msg), + )) + }) + It("should return both runnables and stop errors when both error", func() { m, err := New(cfg, options) Expect(err).NotTo(HaveOccurred())