Add sloglint and improvise error messages

  - Added `sloglint` to enabled linters in `.golangci.yaml`
  - Updated various controllers  for structured logging
    - `blue_secret_controller.go`
    - `maintenance_mode_controller.go`
    - `manager.go`
    - `drpolicy_controller.go`
    - `mirrorpeersecret_controller.go`

Signed-off-by: vbadrina <[email protected]>

.golangci.yaml

@@ -5,6 +5,11 @@ run:
   timeout: 5m
   modules-download-mode: readonly
 
+
+linters:
+  enable:
+    - sloglint
+
 linters-settings:
   errcheck:
     # report about not checking of errors in type assertions: `a := b.(MyStruct)`;

addons/blue_secret_controller.go

@@ -46,7 +46,7 @@ func (r *BlueSecretReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		},
 	}
 
-	r.Logger.Info("Setting up controller with manager", "controller", "bluesecret_controller")
+	r.Logger.Info("Setting up controller with manager")
 
 	return ctrl.NewControllerManagedBy(mgr).
 		Named("bluesecret_controller").
@@ -58,25 +58,26 @@ func (r *BlueSecretReconciler) SetupWithManager(mgr ctrl.Manager) error {
 func (r *BlueSecretReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	var err error
 	var secret corev1.Secret
+	logger := r.Logger.With("secret", req.NamespacedName.String())
 
-	r.Logger.Info("Starting reconciliation for BlueSecret", "secret", req.NamespacedName.String())
+	logger.Info("Starting reconciliation for BlueSecret")
 	err = r.SpokeClient.Get(ctx, req.NamespacedName, &secret)
 	if err != nil {
 		if errors.IsNotFound(err) {
-			r.Logger.Info("BlueSecret not found, possibly deleted", "secret", req.NamespacedName.String())
+			logger.Info("BlueSecret not found, possibly deleted")
 			return ctrl.Result{}, nil
 		}
-		r.Logger.Error("Failed to retrieve BlueSecret from Kubernetes", "error", err, "secret", req.NamespacedName.String())
+		logger.Error("Failed to retrieve BlueSecret from Kubernetes", "error", err)
 		return ctrl.Result{}, err
 	}
 
-	r.Logger.Info("Successfully retrieved BlueSecret", "secret", req.NamespacedName.String())
+	logger.Info("Successfully retrieved BlueSecret")
 	err = r.syncBlueSecretForRook(ctx, secret)
 	if err != nil {
-		r.Logger.Error("Failed to synchronize BlueSecret", "error", err, "secret", req.NamespacedName.String())
+		logger.Error("Failed to synchronize BlueSecret", "error", err)
 		return ctrl.Result{}, err
 	}
 
-	r.Logger.Info("Reconciliation complete for BlueSecret", "secret", req.NamespacedName.String())
+	logger.Info("Reconciliation complete for BlueSecret")
 	return ctrl.Result{}, nil
 }

addons/maintenance_mode_controller.go

@@ -183,9 +183,7 @@ func (r *MaintenanceModeReconciler) isStorageClusterMirroringEnabled(ctx context
 	var sc ocsv1.StorageCluster
 	scName := getOwnerName(cephCluster)
 	if scName == "" {
-		err := fmt.Errorf("no StorageCluster found for given CephCluster %s", cephCluster.Name)
-		r.Logger.Error("No StorageCluster found for CephCluster", "CephCluster", cephCluster.Name, "error", err)
-		return false, err
+		return false, fmt.Errorf("no StorageCluster found for given CephCluster %s", cephCluster.Name)
 	}
 
 	err := r.SpokeClient.Get(ctx, types.NamespacedName{Name: scName, Namespace: cephCluster.Namespace}, &sc)
@@ -209,7 +207,10 @@ func getOwnerName(cluster rookv1.CephCluster) string {
 }
 
 func SetStatus(logger *slog.Logger, mmode *ramenv1alpha1.MaintenanceMode, mode ramenv1alpha1.MMode, state ramenv1alpha1.MModeState, err error) {
-	logger.Info("Updating status", "mode", mode, "status", state, "generation", mmode.Generation, "error", err)
+	if err != nil {
+		logger.Info("Updating status", "mode", mode, "status", state, "generation", mmode.Generation, "error", err)
+	}
+
 	mmode.Status.State = state
 	mmode.Status.ObservedGeneration = mmode.Generation
 	k8smeta.SetStatusCondition(&mmode.Status.Conditions, newCondition(state, mode, mmode.Generation, err))

addons/manager.go

@@ -61,16 +61,14 @@ func init() {
 }
 
 func NewAddonAgentCommand() *cobra.Command {
-	o := &AddonAgentOptions{
-		Logger: slog.New(slog.NewTextHandler(os.Stdout, nil)),
-	}
+	o := &AddonAgentOptions{}
 
 	cmd := &cobra.Command{
 		Use:   "addons",
 		Short: "Start the addon agents",
 		Run: func(cmd *cobra.Command, args []string) {
 			ctx := ctrl.SetupSignalHandler()
-			o.RunAgent(ctx)
+			o.RunAgent(ctx, slog.New(slog.NewTextHandler(os.Stdout, nil)))
 		},
 	}
 
@@ -87,7 +85,6 @@ type AddonAgentOptions struct {
 	HubKubeconfigFile    string
 	SpokeClusterName     string
 	DRMode               string
-	Logger               *slog.Logger
 }
 
 func (o *AddonAgentOptions) AddFlags(cmd *cobra.Command) {
@@ -103,8 +100,7 @@ func (o *AddonAgentOptions) AddFlags(cmd *cobra.Command) {
 }
 
 // RunAgent starts the controllers on agent to process work from hub.
-func (o *AddonAgentOptions) RunAgent(ctx context.Context) {
-	logger := o.Logger
+func (o *AddonAgentOptions) RunAgent(ctx context.Context, logger *slog.Logger) {
 	logger.Info("Starting addon agents.")
 
 	cc, err := addonutils.NewConfigChecker("agent kubeconfig checker", o.HubKubeconfigFile)
@@ -117,10 +113,10 @@ func (o *AddonAgentOptions) RunAgent(ctx context.Context) {
 	go setup.ServeHealthProbes(ctx.Done(), ":8000", cc.Check, logger)
 
 	logger.Info("Starting spoke manager")
-	go runSpokeManager(ctx, *o)
+	go runSpokeManager(ctx, *o, logger)
 
 	logger.Info("Starting hub manager")
-	go runHubManager(ctx, *o)
+	go runHubManager(ctx, *o, logger)
 
 	logger.Info("Addon agent is running, waiting for context cancellation")
 	<-ctx.Done()
@@ -145,8 +141,7 @@ func GetClientConfig(kubeConfigFile string) (*rest.Config, error) {
 	return clientconfig, nil
 }
 
-func runHubManager(ctx context.Context, options AddonAgentOptions) {
-	logger := options.Logger
+func runHubManager(ctx context.Context, options AddonAgentOptions, logger *slog.Logger) {
 	hubConfig, err := GetClientConfig(options.HubKubeconfigFile)
 	if err != nil {
 		logger.Error("Failed to get kubeconfig", "error", err)
@@ -206,8 +201,7 @@ func runHubManager(ctx context.Context, options AddonAgentOptions) {
 	}
 }
 
-func runSpokeManager(ctx context.Context, options AddonAgentOptions) {
-	logger := options.Logger
+func runSpokeManager(ctx context.Context, options AddonAgentOptions, logger *slog.Logger) {
 	hubConfig, err := GetClientConfig(options.HubKubeconfigFile)
 	if err != nil {
 		logger.Error("Failed to get kubeconfig", "error", err)
@@ -244,14 +238,14 @@ func runSpokeManager(ctx context.Context, options AddonAgentOptions) {
 	currentNamespace := os.Getenv("POD_NAMESPACE")
 
 	if err = mgr.Add(manager.RunnableFunc(func(ctx context.Context) error {
-		logger.Info("Waiting for MaintenanceMode CRD to be established")
+		logger.Info("Waiting for MaintenanceMode CRD to be established. MaintenanceMode controller is not running yet.")
 		// Wait for 45s as it takes time for MaintenanceMode CRD to be created.
 		return runtimewait.PollUntilContextCancel(ctx, 15*time.Second, true,
 			func(ctx context.Context) (done bool, err error) {
 				var crd extv1.CustomResourceDefinition
 				readErr := mgr.GetAPIReader().Get(ctx, types.NamespacedName{Name: "maintenancemodes.ramendr.openshift.io"}, &crd)
 				if readErr != nil {
-					klog.Error("Unable to get MaintenanceMode CRD", readErr)
+					logger.Error("Unable to get MaintenanceMode CRD", readErr)
 					// Do not initialize err as we want to retry.
 					// err!=nil or done==true will end polling.
 					done = false

controllers/drpolicy_controller.go

@@ -60,7 +60,7 @@ func (r *DRPolicyReconciler) SetupWithManager(mgr ctrl.Manager) error {
 }
 
 func (r *DRPolicyReconciler) getMirrorPeerForClusterSet(ctx context.Context, clusterSet []string) (*multiclusterv1alpha1.MirrorPeer, error) {
-	logger := r.Logger.With("method", "getMirrorPeerForClusterSet", "ClusterSet", clusterSet)
+	logger := r.Logger
 
 	var mpList multiclusterv1alpha1.MirrorPeerList
 	err := r.HubClient.List(ctx, &mpList)

controllers/mirrorpeersecret_controller.go

@@ -57,8 +57,8 @@ type MirrorPeerSecretReconciler struct {
 
 // Reconcile standard reconcile function for MirrorPeerSecret controller
 func (r *MirrorPeerSecretReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
-	logger := r.Logger
-	logger.Info("Starting reconciliation for Secret", "RequestNamespace", req.Namespace, "RequestName", req.Name)
+	logger := r.Logger.With("Request", req.String())
+	logger.Info("Starting reconciliation for Secret")
 
 	result, err := r.mirrorPeerSecretReconcile(ctx, r.Client, req)
 
@@ -71,8 +71,7 @@ func (r *MirrorPeerSecretReconciler) Reconcile(ctx context.Context, req ctrl.Req
 	return result, err
 }
 
-func (r *MirrorPeerSecretReconciler) updateSecretWithHubRecoveryLabel(ctx context.Context, rc client.Client, peerSecret corev1.Secret) error {
-	logger := r.Logger
+func updateSecretWithHubRecoveryLabel(ctx context.Context, rc client.Client, logger *slog.Logger, peerSecret corev1.Secret) error {
 	logger.Info("Adding backup labels to the secret", "SecretName", peerSecret.Name, "Namespace", peerSecret.Namespace)
 
 	if peerSecret.ObjectMeta.Labels == nil {
@@ -92,8 +91,8 @@ func (r *MirrorPeerSecretReconciler) updateSecretWithHubRecoveryLabel(ctx contex
 }
 
 func (r *MirrorPeerSecretReconciler) mirrorPeerSecretReconcile(ctx context.Context, rc client.Client, req ctrl.Request) (ctrl.Result, error) {
-	logger := r.Logger
-	logger.Info("Reconciling secret", "Request", req.NamespacedName)
+	logger := r.Logger.With("Request", req.NamespacedName)
+	logger.Info("Reconciling secret")
 
 	var peerSecret corev1.Secret
 	err := rc.Get(ctx, req.NamespacedName, &peerSecret)
@@ -112,7 +111,7 @@ func (r *MirrorPeerSecretReconciler) mirrorPeerSecretReconcile(ctx context.Conte
 			return ctrl.Result{}, err
 		}
 		if !utils.HasHubRecoveryLabels(&peerSecret) {
-			err = r.updateSecretWithHubRecoveryLabel(ctx, rc, peerSecret)
+			err = updateSecretWithHubRecoveryLabel(ctx, rc, logger, peerSecret)
 			if err != nil {
 				logger.Error("Failed to add hub recovery labels", "error", err, "Secret", peerSecret.Name)
 				return ctrl.Result{}, err