Switch controller-manager to logr (2) (gardener#5268)

* Switch controllerregistration controller to logr Rename reconcilers and queues a bit and add some doc strings to make it easier to understand what the controller does and how it works. * Rename files to reflect code structure Done in a separate commit, otherwise diff is unreviewable. * Rename files to reflect code structure (2) Done in another separate commit, otherwise diff is unreviewable. * Switch controllerdeployment controller to logr
krgostev · Apr 21, 2022 · 89b1edb · 89b1edb
1 parent 256602f
commit 89b1edb
Show file tree

Hide file tree

Showing 22 changed files with 2,061 additions and 2,021 deletions.
diff --git a/pkg/controllermanager/controller/controllerdeployment/controllerdeployment.go b/pkg/controllermanager/controller/controllerdeployment/controllerdeployment.go
@@ -20,7 +20,8 @@ import (
 	"sync"
 	"time"
 
-	"github.com/sirupsen/logrus"
+	"github.com/go-logr/logr"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/util/workqueue"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -31,28 +32,32 @@ import (
 	"github.com/gardener/gardener/pkg/controllerutils"
 )
 
-// FinalizerName is the finalizer used by this controller.
-const FinalizerName = "core.gardener.cloud/controllerdeployment"
+const (
+	// FinalizerName is the finalizer used by this controller.
+	FinalizerName = "core.gardener.cloud/controllerdeployment"
+	// ControllerName is the name of this controller.
+	ControllerName = "controllerdeployment"
+)
 
 // Controller controls ManagedSeedSets.
 type Controller struct {
 	controllerDeploymentReconciler reconcile.Reconciler
-	hasSyncedFuncs                 []cache.InformerSynced
-
-	controllerDeploymentQueue workqueue.RateLimitingInterface
+	controllerDeploymentQueue      workqueue.RateLimitingInterface
 
+	log                    logr.Logger
+	hasSyncedFuncs         []cache.InformerSynced
 	numberOfRunningWorkers int
 	workerCh               chan int
-
-	logger *logrus.Logger
 }
 
 // New creates a new Gardener controller for ControllerDeployments.
 func New(
 	ctx context.Context,
+	log logr.Logger,
 	clientMap clientmap.ClientMap,
-	logger *logrus.Logger,
 ) (*Controller, error) {
+	log = log.WithName(ControllerName)
+
 	gardenClient, err := clientMap.GetClient(ctx, keys.ForGarden())
 	if err != nil {
 		return nil, fmt.Errorf("could not get garden client: %w", err)
@@ -64,10 +69,10 @@ func New(
 	}
 
 	controller := &Controller{
-		controllerDeploymentReconciler: NewReconciler(logger, gardenClient.Client()),
+		controllerDeploymentReconciler: NewReconciler(gardenClient.Client()),
 		controllerDeploymentQueue:      workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "ManagedSeedSet"),
+		log:                            log,
 		workerCh:                       make(chan int),
-		logger:                         logger,
 	}
 
 	controllerDeploymentInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
@@ -87,22 +92,21 @@ func (c *Controller) Run(ctx context.Context, workers int) {
 	var waitGroup sync.WaitGroup
 
 	if !cache.WaitForCacheSync(ctx.Done(), c.hasSyncedFuncs...) {
-		c.logger.Error("Timed out waiting for caches to sync")
+		c.log.Error(wait.ErrWaitTimeout, "Timed out waiting for caches to sync")
 		return
 	}
 
 	// Count number of running workers
 	go func() {
 		for res := range c.workerCh {
 			c.numberOfRunningWorkers += res
-			c.logger.Debugf("Current number of running ControllerDeployment workers is %d", c.numberOfRunningWorkers)
 		}
 	}()
 
-	c.logger.Info("ControllerDeployment controller initialized.")
+	c.log.Info("ControllerDeployment controller initialized")
 
 	for i := 0; i < workers; i++ {
-		controllerutils.CreateWorker(ctx, c.controllerDeploymentQueue, "ControllerDeployment", c.controllerDeploymentReconciler, &waitGroup, c.workerCh)
+		controllerutils.CreateWorker(ctx, c.controllerDeploymentQueue, "ControllerDeployment", c.controllerDeploymentReconciler, &waitGroup, c.workerCh, controllerutils.WithLogger(c.log))
 	}
 
 	// Shutdown handling
@@ -111,10 +115,10 @@ func (c *Controller) Run(ctx context.Context, workers int) {
 
 	for {
 		if c.controllerDeploymentQueue.Len() == 0 && c.numberOfRunningWorkers == 0 {
-			c.logger.Debug("No running ControllerDeployment worker and no items left in the queues. Terminated ControllerDeployment controller...")
+			c.log.V(1).Info("No running ControllerDeployment worker and no items left in the queues. Terminating ControllerDeployment controller...")
 			break
 		}
-		c.logger.Debugf("Waiting for %d ControllerDeployment worker(s) to finish (%d item(s) left in the queues)...", c.numberOfRunningWorkers, c.controllerDeploymentQueue.Len())
+		c.log.V(1).Info("Waiting for ControllerDeployment workers to finish...", "numberOfRunningWorkers", c.numberOfRunningWorkers, "queueLength", c.controllerDeploymentQueue.Len())
 		time.Sleep(5 * time.Second)
 	}
 

diff --git a/pkg/controllermanager/controller/controllerdeployment/controllerdeployment_control.go b/pkg/controllermanager/controller/controllerdeployment/controllerdeployment_control.go
@@ -18,11 +18,11 @@ import (
 	"context"
 	"fmt"
 
-	"github.com/sirupsen/logrus"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/client-go/tools/cache"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	gardencorev1beta1 "github.com/gardener/gardener/pkg/apis/core/v1beta1"
@@ -33,6 +33,7 @@ import (
 func (c *Controller) controllerDeploymentAdd(obj interface{}) {
 	key, err := cache.MetaNamespaceKeyFunc(obj)
 	if err != nil {
+		c.log.Error(err, "Couldn't get key for object", "object", obj)
 		return
 	}
 	c.controllerDeploymentQueue.Add(key)
@@ -43,26 +44,26 @@ func (c *Controller) controllerDeploymentUpdate(_, newObj interface{}) {
 }
 
 // NewReconciler creates a new instance of a reconciler which reconciles ControllerDeployments.
-func NewReconciler(l logrus.FieldLogger, gardenClient client.Client) reconcile.Reconciler {
+func NewReconciler(gardenClient client.Client) reconcile.Reconciler {
 	return &controllerDeploymentReconciler{
-		logger:       l,
 		gardenClient: gardenClient,
 	}
 }
 
 type controllerDeploymentReconciler struct {
-	logger       logrus.FieldLogger
 	gardenClient client.Client
 }
 
 func (c *controllerDeploymentReconciler) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) {
+	log := logf.FromContext(ctx)
+
 	controllerDeployment := &gardencorev1beta1.ControllerDeployment{}
 	if err := c.gardenClient.Get(ctx, kutil.Key(req.Name), controllerDeployment); err != nil {
 		if apierrors.IsNotFound(err) {
-			c.logger.Debugf("[CONTROLLERDEPLOYMENT RECONCILE] %s - skipping because ControllerDeployment has been deleted", req.Name)
+			log.Info("Object is gone, stop reconciling")
 			return reconcile.Result{}, nil
 		}
-		return reconcile.Result{}, err
+		return reconcile.Result{}, fmt.Errorf("error retrieving object from store: %w", err)
 	}
 
 	if controllerDeployment.DeletionTimestamp != nil {

diff --git a/pkg/controllermanager/controller/controllerdeployment/controllerdeployment_control_test.go b/pkg/controllermanager/controller/controllerdeployment/controllerdeployment_control_test.go
@@ -18,6 +18,7 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/go-logr/logr"
 	"github.com/golang/mock/gomock"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -35,7 +36,6 @@ import (
 	fakeclientmap "github.com/gardener/gardener/pkg/client/kubernetes/clientmap/fake"
 	"github.com/gardener/gardener/pkg/client/kubernetes/clientmap/keys"
 	fakeclientset "github.com/gardener/gardener/pkg/client/kubernetes/fake"
-	"github.com/gardener/gardener/pkg/logger"
 	mockcache "github.com/gardener/gardener/pkg/mock/controller-runtime/cache"
 	mockclient "github.com/gardener/gardener/pkg/mock/controller-runtime/client"
 	kutil "github.com/gardener/gardener/pkg/utils/kubernetes"
@@ -75,7 +75,7 @@ var _ = Describe("Controller", func() {
 			)
 
 			var err error
-			controller, err = New(ctx, fakeclientmap.NewClientMapBuilder().WithClientSetForKey(keys.ForGarden(), fakeclientset.NewClientSetBuilder().WithCache(clientCache).Build()).Build(), logger.NewNopLogger())
+			controller, err = New(ctx, logr.Discard(), fakeclientmap.NewClientMapBuilder().WithClientSetForKey(keys.ForGarden(), fakeclientset.NewClientSetBuilder().WithCache(clientCache).Build()).Build())
 			Expect(err).To(Not(HaveOccurred()))
 
 			controllerDeploymentName = "controller-deployment"
@@ -155,7 +155,7 @@ var _ = Describe("Controller", func() {
 		BeforeEach(func() {
 			controllerDeploymentName = "controllerDeployment"
 			fakeErr = fmt.Errorf("fake err")
-			reconciler = NewReconciler(logger.NewNopLogger(), c)
+			reconciler = NewReconciler(c)
 			controllerDeployment = &gardencorev1beta1.ControllerDeployment{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:            controllerDeploymentName,

diff --git a/pkg/controllermanager/controller/controllerregistration/backupbucket_control.go b/pkg/controllermanager/controller/controllerregistration/backupbucket_control.go
@@ -30,7 +30,7 @@ func (c *Controller) backupBucketAdd(obj interface{}) {
 		return
 	}
 
-	c.controllerRegistrationSeedQueue.Add(*backupBucket.Spec.SeedName)
+	c.seedQueue.Add(*backupBucket.Spec.SeedName)
 }
 
 func (c *Controller) backupBucketUpdate(oldObj, newObj interface{}) {

diff --git a/pkg/controllermanager/controller/controllerregistration/backupbucket_control_test.go b/pkg/controllermanager/controller/controllerregistration/backupbucket_control_test.go
@@ -21,7 +21,7 @@ import (
 	. "github.com/onsi/gomega"
 )
 
-var _ = Describe("Controller", func() {
+var _ = Describe("BackupBucket", func() {
 	var (
 		queue *fakeQueue
 		c     *Controller
@@ -32,7 +32,7 @@ var _ = Describe("Controller", func() {
 	BeforeEach(func() {
 		queue = &fakeQueue{}
 		c = &Controller{
-			controllerRegistrationSeedQueue: queue,
+			seedQueue: queue,
 		}
 	})
 

diff --git a/pkg/controllermanager/controller/controllerregistration/backupentry_control.go b/pkg/controllermanager/controller/controllerregistration/backupentry_control.go
@@ -30,7 +30,7 @@ func (c *Controller) backupEntryAdd(obj interface{}) {
 		return
 	}
 
-	c.controllerRegistrationSeedQueue.Add(*backupEntry.Spec.SeedName)
+	c.seedQueue.Add(*backupEntry.Spec.SeedName)
 }
 
 func (c *Controller) backupEntryUpdate(oldObj, newObj interface{}) {

diff --git a/pkg/controllermanager/controller/controllerregistration/backupentry_control_test.go b/pkg/controllermanager/controller/controllerregistration/backupentry_control_test.go
@@ -21,7 +21,7 @@ import (
 	. "github.com/onsi/gomega"
 )
 
-var _ = Describe("Controller", func() {
+var _ = Describe("BackupEntry", func() {
 	var (
 		queue *fakeQueue
 		c     *Controller
@@ -32,7 +32,7 @@ var _ = Describe("Controller", func() {
 	BeforeEach(func() {
 		queue = &fakeQueue{}
 		c = &Controller{
-			controllerRegistrationSeedQueue: queue,
+			seedQueue: queue,
 		}
 	})
 

diff --git a/pkg/controllermanager/controller/controllerregistration/controllerdeployment_control.go b/pkg/controllermanager/controller/controllerregistration/controllerdeployment_control.go
@@ -18,7 +18,6 @@ import (
 	"context"
 
 	gardencorev1beta1 "github.com/gardener/gardener/pkg/apis/core/v1beta1"
-	"github.com/gardener/gardener/pkg/logger"
 )
 
 func (c *Controller) controllerDeploymentAdd(ctx context.Context, obj interface{}) {
@@ -29,7 +28,7 @@ func (c *Controller) controllerDeploymentAdd(ctx context.Context, obj interface{
 
 	controllerRegistrationList := &gardencorev1beta1.ControllerRegistrationList{}
 	if err := c.gardenClient.List(ctx, controllerRegistrationList); err != nil {
-		logger.Logger.Errorf("error listing controllerregistrations: %+v", err)
+		c.log.Error(err, "Error listing ControllerRegistrations")
 		return
 	}
 

diff --git a/pkg/controllermanager/controller/controllerregistration/controllerinstallation_control.go b/pkg/controllermanager/controller/controllerregistration/controllerinstallation_control.go
@@ -25,7 +25,7 @@ func (c *Controller) controllerInstallationAdd(obj interface{}) {
 		return
 	}
 
-	c.controllerRegistrationSeedQueue.Add(controllerInstallation.Spec.SeedRef.Name)
+	c.seedQueue.Add(controllerInstallation.Spec.SeedRef.Name)
 }
 
 func (c *Controller) controllerInstallationUpdate(oldObj, newObj interface{}) {

diff --git a/...ontrollermanager/controller/controllerregistration/controllerinstallation_control_test.go b/...ontrollermanager/controller/controllerregistration/controllerinstallation_control_test.go
@@ -22,7 +22,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 )
 
-var _ = Describe("Controller", func() {
+var _ = Describe("ControllerInstallation", func() {
 	var (
 		queue *fakeQueue
 		c     *Controller
@@ -33,7 +33,7 @@ var _ = Describe("Controller", func() {
 	BeforeEach(func() {
 		queue = &fakeQueue{}
 		c = &Controller{
-			controllerRegistrationSeedQueue: queue,
+			seedQueue: queue,
 		}
 	})
-Original file line number
+Diff line change
@@ Expand Up / @@ -30,7 +30,7 @@ func (c *Controller) backupBucketAdd(obj interface{}) { @@
     		return
     	}
-    	c.controllerRegistrationSeedQueue.Add(*backupBucket.Spec.SeedName)
+    	c.seedQueue.Add(*backupBucket.Spec.SeedName)
     }
     func (c *Controller) backupBucketUpdate(oldObj, newObj interface{}) {
@@ Expand Down @@