add: logic to reduce logging loops, addressed feedback

Author: VanillaSpoon

controllers/appwrapper_controller.go

@@ -90,24 +90,15 @@ const (
 func (r *AppWrapperReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	_ = log.FromContext(ctx)
 
-	// todo: Move the "machineCheck" locic out of reconcile loop.
+	// todo: Move the "machineCheck" logic out of reconcile loop.
 	// Only reason we are calling it here is that the client is not able to make
 	// calls until it is started, so SetupWithManager is not working.
 	if r.machineCheck == false {
-		if r.MachineType != MachineTypeMachineSet && r.ocmClusterID == "" {
-			if err := r.getOCMClusterID(); err != nil {
-				return ctrl.Result{}, err
-			}
-		}
-		hypershiftEnabled, err := r.checkHypershiftEnabled(ctx)
-		if err != nil {
-			return ctrl.Result{}, fmt.Errorf("error checking if hypershift is enabled: %w", err)
-		}
-		if hypershiftEnabled {
-			r.MachineType = MachineTypeNodePool
+		if err := r.setMachineType(ctx); err != nil {
+			return ctrl.Result{}, err
 		}
-		r.machineCheck = true
 	}
+
 	var appwrapper arbv1.AppWrapper
 
 	if err := r.Get(ctx, req.NamespacedName, &appwrapper); err != nil {
@@ -129,8 +120,10 @@ func (r *AppWrapperReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	}
 
 	if !appwrapper.ObjectMeta.DeletionTimestamp.IsZero() || appwrapper.Status.State == arbv1.AppWrapperStateCompleted {
-		if err := r.finalizeScalingDownMachines(ctx, &appwrapper); err != nil {
-			return ctrl.Result{}, err
+		if !r.annotationExists(&appwrapper, "scaledownComplete") && len(getInstanceRequired(appwrapper.Labels)) > 0 {
+			if err := r.finalizeScalingDownMachines(ctx, &appwrapper); err != nil {
+				return ctrl.Result{}, err
+			}
 		}
 		controllerutil.RemoveFinalizer(&appwrapper, finalizerName)
 		if err := r.Update(ctx, &appwrapper); err != nil {
@@ -159,12 +152,30 @@ func (r *AppWrapperReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	return ctrl.Result{}, nil
 }
 
+func (r *AppWrapperReconciler) setMachineType(ctx context.Context) error {
+	if r.MachineType != MachineTypeMachineSet && r.ocmClusterID == "" {
+		if err := r.getOCMClusterID(); err != nil {
+			return err
+		}
+	}
+	hypershiftEnabled, err := r.checkHypershiftEnabled(ctx)
+	if err != nil {
+		return fmt.Errorf("error checking if hypershift is enabled: %w", err)
+	}
+	if hypershiftEnabled {
+		r.MachineType = MachineTypeNodePool
+	}
+	r.machineCheck = true
+	return nil
+}
+
 func (r *AppWrapperReconciler) finalizeScalingDownMachines(ctx context.Context, appwrapper *arbv1.AppWrapper) error {
 	if appwrapper.Status.State == arbv1.AppWrapperStateCompleted {
 		deletionMessage = "completed"
 	} else {
 		deletionMessage = "deleted"
 	}
+
 	switch r.MachineType {
 	case MachineTypeMachineSet:
 		switch strings.ToLower(r.Config.MachineSetsStrategy) {
@@ -173,33 +184,39 @@ func (r *AppWrapperReconciler) finalizeScalingDownMachines(ctx context.Context,
 			if matchedAw != nil {
 				klog.Infof("Appwrapper %s %s, swapping machines to %s", appwrapper.Name, deletionMessage, matchedAw.Name)
 				if err := r.swapNodeLabels(ctx, appwrapper, matchedAw); err != nil {
+					klog.Errorf("Error swapping node labels for AppWrapper %s: %v", appwrapper.Name, err)
 					return err
 				}
 			} else {
 				klog.Infof("Appwrapper %s %s, scaling down machines", appwrapper.Name, deletionMessage)
 				if err := r.annotateToDeleteMachine(ctx, appwrapper); err != nil {
+					klog.Errorf("Error annotating to delete machine for AppWrapper %s: %v", appwrapper.Name, err)
 					return err
 				}
 			}
 		case "duplicate":
 			klog.Infof("Appwrapper %s scale-down machineset: %s ", deletionMessage, appwrapper.Name)
 			if err := r.deleteMachineSet(ctx, appwrapper); err != nil {
+				klog.Errorf("Error deleting MachineSet for AppWrapper %s: %v", appwrapper.Name, err)
 				return err
 			}
 		}
 
 	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)
 			return err
 		}
 
 	case MachineTypeMachinePool:
 		klog.Infof("Appwrapper %s scale-down machine pool: %s ", deletionMessage, appwrapper.Name)
 		if _, err := r.deleteMachinePool(ctx, appwrapper); err != nil {
+			klog.Errorf("Error deleting MachinePool for AppWrapper %s: %v", appwrapper.Name, err)
 			return err
 		}
 	}
+	r.setAnnotation(appwrapper, "scaledownComplete", "true")
 	return nil
 }
 
@@ -237,15 +254,12 @@ func (r *AppWrapperReconciler) getOCMSecret(ctx context.Context, secretRef *core
 
 func (r *AppWrapperReconciler) discoverInstanceTypes(aw *arbv1.AppWrapper) map[string]int {
 	demandMapPerInstanceType := make(map[string]int)
-	var instanceRequired []string
-	for k, v := range aw.Labels {
-		if k == "orderedinstance" {
-			instanceRequired = strings.Split(v, "_")
-		}
-	}
-
+	instanceRequired := getInstanceRequired(aw.Labels)
 	if len(instanceRequired) < 1 {
-		klog.Infof("Found AW %s that cannot be scaled due to missing orderedinstance label", aw.ObjectMeta.Name)
+		if _, exists := aw.Annotations["loggedNoInstances"]; !exists {
+			klog.Infof("Found AW %s that cannot be scaled due to missing orderedinstance label", aw.ObjectMeta.Name)
+			r.setAnnotation(aw, "loggedNoInstances", "true")
+		}
 		return demandMapPerInstanceType
 	}
 

controllers/utils.go

@@ -3,14 +3,35 @@ package controllers
 import (
 	"encoding/json"
 	"fmt"
-	"math/rand"
-	"time"
-
 	machinev1 "github.com/openshift/api/machine/v1beta1"
+	arbv1 "github.com/project-codeflare/multi-cluster-app-dispatcher/pkg/apis/controller/v1beta1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/klog"
+
+	"math/rand"
+	"strings"
+	"time"
 )
 
+func getInstanceRequired(labels map[string]string) []string {
+	if value, exists := labels["orderedinstance"]; exists {
+		return strings.Split(value, "_")
+	}
+	return []string{}
+}
+
+func (r *AppWrapperReconciler) annotationExists(appwrapper *arbv1.AppWrapper, key string) bool {
+	_, exists := appwrapper.Annotations[key]
+	return exists
+}
+
+func (r *AppWrapperReconciler) setAnnotation(appwrapper *arbv1.AppWrapper, key, value string) {
+	if appwrapper.Annotations == nil {
+		appwrapper.Annotations = make(map[string]string)
+	}
+	appwrapper.Annotations[key] = value
+}
+
 func resyncPeriod() func() time.Duration {
 	return func() time.Duration {
 		factor := rand.Float64() + 1