update: logging to match current method

Author: VanillaSpoon

controllers/appWrapper_controller_test.go

@@ -11,7 +11,6 @@ import (
 
 func (r *AppWrapperReconciler) TestDiscoverInstanceTypes(t *testing.T) {
 	g := gomega.NewGomegaWithT(t)
-	ctx := context.Background()
 	tests := []struct {
 		name        string
 		input       *arbv1.AppWrapper

controllers/appwrapper_controller.go

@@ -18,7 +18,6 @@ package controllers
 
 import (
 	"context"
-	"fmt"
 	ocmsdk "github.com/openshift-online/ocm-sdk-go"
 	"github.com/project-codeflare/instascale/pkg/config"
 	arbv1 "github.com/project-codeflare/multi-cluster-app-dispatcher/pkg/apis/controller/v1beta1"
@@ -157,14 +156,16 @@ func (r *AppWrapperReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 }
 
 func (r *AppWrapperReconciler) setMachineType(ctx context.Context) error {
+	logger := ctrl.LoggerFrom(ctx)
 	if r.MachineType != MachineTypeMachineSet && r.ocmClusterID == "" {
-		if err := r.getOCMClusterID(); err != nil {
+		if err := r.getOCMClusterID(ctx); err != nil {
 			return err
 		}
 	}
 	hypershiftEnabled, err := r.checkHypershiftEnabled(ctx)
 	if err != nil {
-		return fmt.Errorf("error checking if hypershift is enabled: %w", err)
+		logger.Error(err, "error checking if hypershift is enabled")
+		return err
 	}
 	if hypershiftEnabled {
 		r.MachineType = MachineTypeNodePool
@@ -194,7 +195,9 @@ func (r *AppWrapperReconciler) finalizeScalingDownMachines(ctx context.Context,
 					"newAppWrapper", matchedAw,
 				)
 				if err := r.swapNodeLabels(ctx, appwrapper, matchedAw); err != nil {
-					klog.Errorf("Error swapping node labels for AppWrapper %s: %v", appwrapper.Name, err)
+					logger.Error(err, "Error swapping node labels for AppWrapper",
+						"appwrapper", appwrapper,
+					)
 					return err
 				}
 			} else {
@@ -204,7 +207,9 @@ func (r *AppWrapperReconciler) finalizeScalingDownMachines(ctx context.Context,
 					"deletionMessage", deletionMessage,
 				)
 				if err := r.annotateToDeleteMachine(ctx, appwrapper); err != nil {
-					klog.Errorf("Error annotating to delete machine for AppWrapper %s: %v", appwrapper.Name, err)
+					logger.Error(err, "Error annotating to delete machine for AppWrapper",
+						"appwrapper", appwrapper,
+					)
 					return err
 				}
 			}
@@ -215,29 +220,33 @@ func (r *AppWrapperReconciler) finalizeScalingDownMachines(ctx context.Context,
 				"deletionMessage", deletionMessage,
 			)
 			if err := r.deleteMachineSet(ctx, appwrapper); err != nil {
-				klog.Errorf("Error deleting MachineSet for AppWrapper %s: %v", appwrapper.Name, err)
+				logger.Error(err, "Error deleting MachineSet for AppWrapper",
+					"appwrapper", appwrapper)
 				return err
 			}
 		}
-	} else {
+
+	case MachineTypeNodePool:
 		logger.Info(
-			"AppWrapper deleted, scaling down machine pool",
+			"AppWrapper deleted, scaling down nodepool",
 			"appWrapper", appwrapper,
 			"deletionMessage", deletionMessage,
 		)
-	}
-
-	case MachineTypeNodePool:
-		klog.Infof("Appwrapper %s scale-down node pool: %s ", deletionMessage, appwrapper.Name)
 		if _, err := r.deleteNodePool(ctx, appwrapper); err != nil {
-			klog.Errorf("Error deleting NodePool for AppWrapper %s: %v", appwrapper.Name, err)
+			logger.Error(err, "Error deleting NodePool for AppWrapper",
+				"appwrapper", appwrapper)
 			return err
 		}
 
 	case MachineTypeMachinePool:
-		klog.Infof("Appwrapper %s scale-down machine pool: %s ", deletionMessage, appwrapper.Name)
+		logger.Info(
+			"AppWrapper deleted, scaling down machine pool",
+			"appWrapper", appwrapper,
+			"deletionMessage", deletionMessage,
+		)
 		if _, err := r.deleteMachinePool(ctx, appwrapper); err != nil {
-			klog.Errorf("Error deleting MachinePool for AppWrapper %s: %v", appwrapper.Name, err)
+			logger.Error(err, "Error deleting MachinePool for AppWrapper",
+				"appwrapper", appwrapper)
 			return err
 		}
 	}
@@ -260,17 +269,16 @@ func (r *AppWrapperReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Ma
 	r.MachineType = MachineTypeMachineSet // default to MachineSet
 	if ocmSecretRef := r.Config.OCMSecretRef; ocmSecretRef != nil {
 		if ocmSecret, err := r.getOCMSecret(ctx, ocmSecretRef); err != nil {
-			return fmt.Errorf("error reading OCM Secret from ref %q: %w", ocmSecretRef, err)
+			logger.Error(err, "error reading OCM Secret from ref",
+				"ref", ocmSecretRef)
+			return err
 		} else if token := ocmSecret.Data["token"]; len(token) > 0 {
 			r.ocmToken = string(token)
 			r.MachineType = MachineTypeMachinePool
 		} else {
-			return fmt.Errorf("token is missing from OCM Secret %q", ocmSecretRef)
-		}
-		if ok, err := r.machinePoolExists(); err != nil {
+			logger.Error(err, "token is missing from OCM Secret",
+				"ref", ocmSecretRef)
 			return err
-		} else if ok {
-			logger.Info("Using machine pools for cluster auto-scaling")
 		}
 	}
 
@@ -293,10 +301,12 @@ func (r *AppWrapperReconciler) discoverInstanceTypes(ctx context.Context, aw *ar
 			"appWrapper", aw,
 		)
 		if _, exists := aw.Annotations["loggedNoInstances"]; !exists {
-			klog.Infof("Found AW %s that cannot be scaled due to missing orderedinstance label", aw.Name)
+			logger.Info("Found AW that cannot be scaled due to missing orderedinstance label",
+				"appwrapper", aw)
 			r.setAnnotation(aw, "loggedNoInstances", "true")
 			if err := r.Update(ctx, aw); err != nil {
-				klog.Errorf("Error adding annotation to AW %s: %v", aw.Name, err)
+				logger.Error(err, "Error adding annotation to AW",
+					"appwrapper", aw)
 			}
 		}
 		return demandMapPerInstanceType

controllers/machinepools.go

@@ -2,13 +2,11 @@ package controllers
 
 import (
 	"context"
-	"fmt"
 	"strings"
 
 	cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
 	arbv1 "github.com/project-codeflare/multi-cluster-app-dispatcher/pkg/apis/controller/v1beta1"
 
-	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 )
 
@@ -104,52 +102,3 @@ func (r *AppWrapperReconciler) deleteMachinePool(ctx context.Context, aw *arbv1.
 	})
 	return ctrl.Result{Requeue: false}, nil
 }
-
-func (r *AppWrapperReconciler) machinePoolExists() (bool, error) {
-	connection, err := r.createOCMConnection()
-	if err != nil {
-		return false, fmt.Errorf("error creating OCM connection: %w", err)
-	}
-	defer connection.Close()
-
-	machinePools := connection.ClustersMgmt().V1().Clusters().Cluster(r.ocmClusterID).MachinePools()
-	return machinePools != nil, nil
-}
-
-// getOCMClusterID determines the internal clusterID to be used for OCM API calls
-func (r *AppWrapperReconciler) getOCMClusterID(ctx context.Context) error {
-	logger := ctrl.LoggerFrom(ctx)
-	cv := &configv1.ClusterVersion{}
-	err := r.Get(ctx, types.NamespacedName{Name: "version"}, cv)
-	if err != nil {
-		return fmt.Errorf("can't get clusterversion: %v", err)
-	}
-
-	internalClusterID := string(cv.Spec.ClusterID)
-
-	connection, err := r.createOCMConnection()
-	if err != nil {
-		logger.Error(err, "Error creating OCM connection")
-	}
-	defer connection.Close()
-
-	// Get the client for the resource that manages the collection of clusters:
-	collection := connection.ClustersMgmt().V1().Clusters()
-
-	response, err := collection.List().Search(fmt.Sprintf("external_id = '%s'", internalClusterID)).Size(1).Page(1).SendContext(ctx)
-	if err != nil {
-		logger.Error(err, "Error getting cluster id")
-	}
-
-	response.Items().Each(func(cluster *cmv1.Cluster) bool {
-		r.ocmClusterID = cluster.ID()
-		logger.Info(
-			"Cluster Info",
-			"clusterId", cluster.ID(),
-			"clusterName", cluster.Name(),
-			"clusterState", cluster.State(),
-		)
-		return true
-	})
-	return nil
-}

controllers/nodepools.go

@@ -2,21 +2,19 @@ package controllers
 
 import (
 	"context"
-	"fmt"
-	"os"
 	"strings"
 
 	cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
 	arbv1 "github.com/project-codeflare/multi-cluster-app-dispatcher/pkg/apis/controller/v1beta1"
 
-	"k8s.io/klog"
 	ctrl "sigs.k8s.io/controller-runtime"
 )
 
 func (r *AppWrapperReconciler) scaleNodePool(ctx context.Context, aw *arbv1.AppWrapper, demandPerInstanceType map[string]int) (ctrl.Result, error) {
+	logger := ctrl.LoggerFrom(ctx)
 	connection, err := r.createOCMConnection()
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "Error creating OCM connection: %v", err)
+		logger.Error(err, "Error creating OCM connection")
 		return ctrl.Result{}, err
 	}
 	defer connection.Close()
@@ -32,7 +30,6 @@ func (r *AppWrapperReconciler) scaleNodePool(ctx context.Context, aw *arbv1.AppW
 
 		numberOfMachines := 0
 		response.Items().Each(func(nodePool *cmv1.NodePool) bool {
-
 			if nodePool.AWSNodePool().InstanceType() == userRequestedInstanceType && hasAwLabel(nodePool.Labels(), aw) {
 				numberOfMachines = nodePool.Replicas()
 				return false
@@ -43,29 +40,43 @@ func (r *AppWrapperReconciler) scaleNodePool(ctx context.Context, aw *arbv1.AppW
 		if numberOfMachines != replicas {
 			m := make(map[string]string)
 			m[aw.Name] = aw.Name
-			klog.Infof("The instanceRequired array: %v", userRequestedInstanceType)
+			logger.Info("The instanceRequired array",
+				"InstanceRequired", userRequestedInstanceType)
 
 			nodePoolID := strings.ReplaceAll(aw.Name+"-"+userRequestedInstanceType, ".", "-")
 
 			createNodePool, err := cmv1.NewNodePool().AWSNodePool(cmv1.NewAWSNodePool().InstanceType(userRequestedInstanceType)).ID(nodePoolID).Replicas(replicas).Labels(m).Build()
 			if err != nil {
-				klog.Errorf(`Error building NodePool: %v`, err)
+				logger.Error(
+					err, "Error building NodePool",
+					"userRequestedInstanceType", userRequestedInstanceType,
+				)
 			}
-			klog.Infof("Built NodePool with instance type %v and name %v", userRequestedInstanceType, createNodePool.ID())
+			logger.Info(
+				"Sending NodePool creation request",
+				"instanceType", userRequestedInstanceType,
+				"nodePoolName", createNodePool.ID(),
+			)
 			response, err := clusterNodePools.Add().Body(createNodePool).SendContext(ctx)
 			if err != nil {
-				klog.Errorf(`Error creating NodePool: %v`, err)
+				logger.Error(err, "Error creating NodePool")
+			} else {
+				logger.Info(
+					"Successfully created NodePool",
+					"nodePoolName", createNodePool.ID(),
+					"response", response,
+				)
 			}
-			klog.Infof("Created NodePool: %v", response)
 		}
 	}
 	return ctrl.Result{Requeue: false}, nil
 }
 
 func (r *AppWrapperReconciler) deleteNodePool(ctx context.Context, aw *arbv1.AppWrapper) (ctrl.Result, error) {
+	logger := ctrl.LoggerFrom(ctx)
 	connection, err := r.createOCMConnection()
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "Error creating OCM connection: %v", err)
+		logger.Error(err, "Error creating OCM connection")
 		return ctrl.Result{}, err
 	}
 	defer connection.Close()
@@ -79,39 +90,50 @@ func (r *AppWrapperReconciler) deleteNodePool(ctx context.Context, aw *arbv1.App
 		if strings.Contains(id, aw.Name) {
 			targetNodePool, err := connection.ClustersMgmt().V1().Clusters().Cluster(r.ocmClusterID).NodePools().NodePool(id).Delete().SendContext(ctx)
 			if err != nil {
-				klog.Infof("Error deleting target nodepool %v", targetNodePool)
+				logger.Error(
+					err, "Error deleting nodepool",
+					"nodePool", targetNodePool,
+				)
+			} else {
+				logger.Info(
+					"Successfully scaled down target nodepool",
+					"nodePool", targetNodePool,
+				)
 			}
-			klog.Infof("Successfully Scaled down target nodepool %v", id)
 		}
 		return true
 	})
 	return ctrl.Result{Requeue: false}, nil
 }
 
 func (r *AppWrapperReconciler) checkHypershiftEnabled(ctx context.Context) (bool, error) {
+	logger := ctrl.LoggerFrom(ctx)
 	connection, err := r.createOCMConnection()
 	if err != nil {
-		return false, fmt.Errorf("error creating OCM connection: %w", err)
+		logger.Error(err, "Error creating OCM connection")
+		return false, err
 	}
 	defer connection.Close()
 
 	clusterResource := connection.ClustersMgmt().V1().Clusters().Cluster(r.ocmClusterID)
 
 	response, err := clusterResource.Get().SendContext(ctx)
 	if err != nil {
-		return false, fmt.Errorf("error fetching cluster details: %w", err)
+		logger.Error(err, "error fetching cluster details")
+		return false, err
 	}
 
 	body := response.Body()
 	if body == nil {
-		return false, fmt.Errorf("empty response body")
+		logger.Error(err, "Empty resource body when checking Hypershift enabled status")
+		return false, err
 	}
 
 	hypershiftEnabled := false
 	if body.Hypershift() != nil {
 		hypershiftEnabled = body.Hypershift().Enabled()
 	}
 
-	fmt.Printf("Hypershift enabled status: %v\n", hypershiftEnabled)
+	logger.Info("Checked Hypershift enabled status", "status", hypershiftEnabled)
 	return hypershiftEnabled, nil
 }