Address naming + comments from sbueringer.

Author: godwinpang

pkg/config/controller.go

@@ -66,7 +66,7 @@ type Controller struct {
 	// improves leadership failover time, as the caches will be prepopulated before the controller
 	// transitions to be leader.
 	//
-	// When set to true, the controller will start its sources without transitioning to be leader.
+	// When set to true, the controller will start its sources without waiting to become leader.
 	// Defaults to false.
 	NeedWarmup *bool
 

pkg/controller/controller.go

@@ -1,4 +1,5 @@
-/* Copyright 2018 The Kubernetes Authors.
+/*
+Copyright 2018 The Kubernetes Authors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.

pkg/controller/controller_test.go

@@ -475,11 +475,11 @@ var _ = Describe("controller.Controller", func() {
 			Expect(ok).To(BeFalse())
 		})
 
-		It("should set ShouldWarmupWithoutLeadership correctly", func() {
+		It("should set NeedWarmup correctly", func() {
 			m, err := manager.New(cfg, manager.Options{})
 			Expect(err).NotTo(HaveOccurred())
 
-			// Test with ShouldWarmupWithoutLeadership set to true
+			// Test with NeedWarmup set to true
 			ctrlWithWarmup, err := controller.New("warmup-enabled-ctrl", m, controller.Options{
 				Reconciler: reconcile.Func(nil),
 				NeedWarmup: ptr.To(true),
@@ -491,7 +491,7 @@ var _ = Describe("controller.Controller", func() {
 			Expect(internalCtrlWithWarmup.NeedWarmup).NotTo(BeNil())
 			Expect(*internalCtrlWithWarmup.NeedWarmup).To(BeTrue())
 
-			// Test with ShouldWarmupWithoutLeadership set to false
+			// Test with NeedWarmup set to false
 			ctrlWithoutWarmup, err := controller.New("warmup-disabled-ctrl", m, controller.Options{
 				Reconciler: reconcile.Func(nil),
 				NeedWarmup: ptr.To(false),
@@ -503,7 +503,7 @@ var _ = Describe("controller.Controller", func() {
 			Expect(internalCtrlWithoutWarmup.NeedWarmup).NotTo(BeNil())
 			Expect(*internalCtrlWithoutWarmup.NeedWarmup).To(BeFalse())
 
-			// Test with ShouldWarmupWithoutLeadership not set (should default to nil)
+			// Test with NeedWarmup not set (should default to nil)
 			ctrlWithDefaultWarmup, err := controller.New("warmup-default-ctrl", m, controller.Options{
 				Reconciler: reconcile.Func(nil),
 			})
@@ -514,8 +514,8 @@ var _ = Describe("controller.Controller", func() {
 			Expect(internalCtrlWithDefaultWarmup.NeedWarmup).To(BeNil())
 		})
 
-		It("should inherit ShouldWarmupWithoutLeadership from manager config", func() {
-			// Test with manager default setting ShouldWarmupWithoutLeadership to true
+		It("should inherit NeedWarmup from manager config", func() {
+			// Test with manager default setting NeedWarmup to true
 			managerWithWarmup, err := manager.New(cfg, manager.Options{
 				Controller: config.Controller{
 					NeedWarmup: ptr.To(true),

pkg/internal/controller/controller.go

@@ -93,12 +93,12 @@ type Controller[request comparable] struct {
 	// didStartEventSources is used to indicate whether the event sources have been started.
 	didStartEventSources atomic.Bool
 
-	// didEventSourcesFinishSync is used to indicate whether the event sources have finished
+	// didEventSourcesFinishSyncSuccessfully is used to indicate whether the event sources have finished
 	// successfully. It stores a *bool where
 	// - nil: not finished syncing
 	// - true: finished syncing without error
 	// - false: finished syncing with error
-	didEventSourcesFinishSync atomic.Value
+	didEventSourcesFinishSyncSuccessfully atomic.Value
 
 	// LogConstructor is used to construct a logger to then log messages to users during reconciliation,
 	// or for example when a watch is started.
@@ -175,10 +175,11 @@ func (c *Controller[request]) Warmup(ctx context.Context) error {
 	return c.startEventSources(ctx)
 }
 
-// DidFinishWarmup implements the manager.WarmupRunnable interface.
-func (c *Controller[request]) DidFinishWarmup(ctx context.Context) bool {
+// WaitForWarmupComplete returns true if warmup has completed without error, and false if there was
+// an error during warmup.
+func (c *Controller[request]) WaitForWarmupComplete(ctx context.Context) bool {
 	err := wait.PollUntilContextCancel(ctx, syncedPollPeriod, true, func(ctx context.Context) (bool, error) {
-		didFinishSync, ok := c.didEventSourcesFinishSync.Load().(*bool)
+		didFinishSync, ok := c.didEventSourcesFinishSyncSuccessfully.Load().(*bool)
 		if !ok {
 			return false, errors.New("unexpected error: didEventSourcesFinishSync is not a bool pointer")
 		}
@@ -279,7 +280,7 @@ func (c *Controller[request]) startEventSources(ctx context.Context) error {
 	// CAS returns false if value is already true, so early exit since another goroutine must have
 	// called startEventSources previously
 	if !c.didStartEventSources.CompareAndSwap(false, true) {
-		c.LogConstructor(nil).Info("Skipping starting event sources since it was already started")
+		c.LogConstructor(nil).Info("Skipping starting event sources since they were previously started")
 		return nil
 	}
 
@@ -337,7 +338,7 @@ func (c *Controller[request]) startEventSources(ctx context.Context) error {
 	}
 	err := errGroup.Wait()
 
-	c.didEventSourcesFinishSync.Store(ptr.To(err == nil))
+	c.didEventSourcesFinishSyncSuccessfully.Store(ptr.To(err == nil))
 
 	return err
 }

pkg/internal/controller/controller_test.go

@@ -1036,8 +1036,8 @@ var _ = Describe("controller", func() {
 			err := ctrl.Warmup(ctx)
 			Expect(err).NotTo(HaveOccurred())
 
-			// Verify DidFinishWarmup returns true for successful sync
-			result := ctrl.DidFinishWarmup(ctx)
+			// Verify WaitForWarmupComplete returns true for successful sync
+			result := ctrl.WaitForWarmupComplete(ctx)
 			Expect(result).To(BeTrue())
 		})
 
@@ -1057,8 +1057,8 @@ var _ = Describe("controller", func() {
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(ContainSubstring("sync error"))
 
-			// Verify DidFinishWarmup returns false for unsuccessful sync
-			result := ctrl.DidFinishWarmup(ctx)
+			// Verify WaitForWarmupComplete returns false for unsuccessful sync
+			result := ctrl.WaitForWarmupComplete(ctx)
 			Expect(result).To(BeFalse())
 		})
 	})

pkg/manager/internal.go

@@ -439,7 +439,7 @@ func (cm *controllerManager) Start(ctx context.Context) (err error) {
 		return fmt.Errorf("failed to start other runnables: %w", err)
 	}
 
-	// Start and wait for sources to start.
+	// Start WarmupRunnables and wait for warmup to complete.
 	if err := cm.runnables.Warmup.Start(cm.internalCtx); err != nil {
 		return fmt.Errorf("failed to start warmup runnables: %w", err)
 	}

pkg/manager/manager.go

@@ -314,11 +314,11 @@ type LeaderElectionRunnable interface {
 	NeedLeaderElection() bool
 }
 
-// WarmupRunnable knows if a Runnable should be a warmup runnable. A warmup runnable is a runnable
+// warmupRunnable knows if a Runnable requires warmup. A warmup runnable is a runnable
 // that should be run when the manager is started, but before the leader election is acquired.
-// Note: Implementing this interface is only useful when LeaderElection is enabled, as the behavior
-// without leaderelection is to run LeaderElectionRunnables immediately.
-type WarmupRunnable interface {
+// Note: Implementing this interface is only useful when LeaderElection can be enabled, as the
+// behavior when leaderelection is not enabled is to run LeaderElectionRunnables immediately.
+type warmupRunnable interface {
 	// Warmup is the implementation of the warmup runnable.
 	Warmup(context.Context) error
 

pkg/manager/manager_test.go

@@ -1953,7 +1953,7 @@ var _ = Describe("manger.Manager", func() {
 		By("Creating a runnable that implements WarmupRunnable interface")
 		// Create a warmup runnable
 		warmupRunnable := warmupRunnableFunc{
-			RunFunc: func(ctx context.Context) error {
+			StartFunc: func(ctx context.Context) error {
 				// This is the main runnable that will be executed after leader election
 				<-ctx.Done()
 				return nil
@@ -1968,7 +1968,7 @@ var _ = Describe("manger.Manager", func() {
 
 		By("Creating a runnable that requires leader election")
 		leaderElectionRunnable := leaderElectionRunnableFunc{
-			RunFunc: func(ctx context.Context) error {
+			StartFunc: func(ctx context.Context) error {
 				// This will only be called after leader election is won
 				close(leaderElectionRunnableCalled)
 				<-ctx.Done()

pkg/manager/runnable_group.go

@@ -67,8 +67,8 @@ func (r *runnables) Add(fn Runnable) error {
 		})
 	case webhook.Server:
 		return r.Webhooks.Add(fn, nil)
-	case WarmupRunnable, LeaderElectionRunnable:
-		if warmupRunnable, ok := fn.(WarmupRunnable); ok {
+	case warmupRunnable, LeaderElectionRunnable:
+		if warmupRunnable, ok := fn.(warmupRunnable); ok {
 			if err := r.Warmup.Add(
 				RunnableFunc(warmupRunnable.Warmup),
 				func(ctx context.Context) bool {

pkg/manager/runnable_group_test.go

@@ -56,7 +56,7 @@ var _ = Describe("runnables", func() {
 
 	It("should add WarmupRunnable to the Warmup and LeaderElection group", func() {
 		warmupRunnable := warmupRunnableFunc{
-			RunFunc: func(c context.Context) error {
+			StartFunc: func(c context.Context) error {
 				<-c.Done()
 				return nil
 			},
@@ -72,8 +72,8 @@ var _ = Describe("runnables", func() {
 	})
 
 	It("should add WarmupRunnable that doesn't needs leader election to warmup group only", func() {
-		warmupRunnable := combinedRunnable{
-			RunFunc: func(c context.Context) error {
+		warmupRunnable := leaderElectionAndWarmupRunnable{
+			StartFunc: func(c context.Context) error {
 				<-c.Done()
 				return nil
 			},
@@ -93,8 +93,8 @@ var _ = Describe("runnables", func() {
 	})
 
 	It("should add WarmupRunnable that needs leader election to Warmup and LeaderElection group, not Others", func() {
-		warmupRunnable := combinedRunnable{
-			RunFunc: func(c context.Context) error {
+		warmupRunnable := leaderElectionAndWarmupRunnable{
+			StartFunc: func(c context.Context) error {
 				<-c.Done()
 				return nil
 			},
@@ -118,7 +118,7 @@ var _ = Describe("runnables", func() {
 		var warmupExecuted atomic.Bool
 
 		warmupRunnable := warmupRunnableFunc{
-			RunFunc: func(c context.Context) error {
+			StartFunc: func(c context.Context) error {
 				<-c.Done()
 				return nil
 			},
@@ -145,7 +145,7 @@ var _ = Describe("runnables", func() {
 		expectedErr := fmt.Errorf("expected warmup error")
 
 		warmupRunnable := warmupRunnableFunc{
-			RunFunc: func(c context.Context) error {
+			StartFunc: func(c context.Context) error {
 				<-c.Done()
 				return nil
 			},
@@ -354,29 +354,29 @@ var _ LeaderElectionRunnable = &leaderElectionRunnableFunc{}
 // leaderElectionRunnableFunc is a helper struct that implements LeaderElectionRunnable
 // for testing purposes.
 type leaderElectionRunnableFunc struct {
-	RunFunc                func(context.Context) error
+	StartFunc              func(context.Context) error
 	NeedLeaderElectionFunc func() bool
 }
 
 func (r leaderElectionRunnableFunc) Start(ctx context.Context) error {
-	return r.RunFunc(ctx)
+	return r.StartFunc(ctx)
 }
 
 func (r leaderElectionRunnableFunc) NeedLeaderElection() bool {
 	return r.NeedLeaderElectionFunc()
 }
 
-var _ WarmupRunnable = &warmupRunnableFunc{}
+var _ warmupRunnable = &warmupRunnableFunc{}
 
 // warmupRunnableFunc is a helper struct that implements WarmupRunnable
 // for testing purposes.
 type warmupRunnableFunc struct {
-	RunFunc    func(context.Context) error
+	StartFunc  func(context.Context) error
 	WarmupFunc func(context.Context) error
 }
 
 func (r warmupRunnableFunc) Start(ctx context.Context) error {
-	return r.RunFunc(ctx)
+	return r.StartFunc(ctx)
 }
 
 func (r warmupRunnableFunc) Warmup(ctx context.Context) error {
@@ -387,25 +387,25 @@ func (r warmupRunnableFunc) WaitForWarmupComplete(ctx context.Context) bool {
 	return true
 }
 
-// combinedRunnable implements both WarmupRunnable and LeaderElectionRunnable
-type combinedRunnable struct {
-	RunFunc                func(context.Context) error
+// leaderElectionAndWarmupRunnable implements both WarmupRunnable and LeaderElectionRunnable
+type leaderElectionAndWarmupRunnable struct {
+	StartFunc              func(context.Context) error
 	WarmupFunc             func(context.Context) error
 	NeedLeaderElectionFunc func() bool
 }
 
-func (r combinedRunnable) Start(ctx context.Context) error {
-	return r.RunFunc(ctx)
+func (r leaderElectionAndWarmupRunnable) Start(ctx context.Context) error {
+	return r.StartFunc(ctx)
 }
 
-func (r combinedRunnable) Warmup(ctx context.Context) error {
+func (r leaderElectionAndWarmupRunnable) Warmup(ctx context.Context) error {
 	return r.WarmupFunc(ctx)
 }
 
-func (r combinedRunnable) WaitForWarmupComplete(ctx context.Context) bool {
+func (r leaderElectionAndWarmupRunnable) WaitForWarmupComplete(ctx context.Context) bool {
 	return true
 }
 
-func (r combinedRunnable) NeedLeaderElection() bool {
+func (r leaderElectionAndWarmupRunnable) NeedLeaderElection() bool {
 	return r.NeedLeaderElectionFunc()
 }