Address comments

Signed-off-by: Saswata Mukherjee <[email protected]>
stolostron · Dec 15, 2024 · fe15973 · fe15973
1 parent 1ac83e4
commit fe15973
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 32 deletions.
diff --git a/operators/multiclusterobservability/controllers/placementrule/manifestwork.go b/operators/multiclusterobservability/controllers/placementrule/manifestwork.go
@@ -984,27 +984,17 @@ func getObservabilityAddon(c client.Client, namespace string,
 	}
 
 	if _, ok := found.Annotations[addonSourceAnnotation]; !ok {
-		found.Annotations[addonSourceAnnotation] = "mco"
+		found.Annotations[addonSourceAnnotation] = addonSourceMCO
 	}
 
-	if found.Annotations[addonSourceAnnotation] == "mco" {
-		addon.Spec.EnableMetrics = mco.Spec.ObservabilityAddonSpec.EnableMetrics
-		addon.Spec.Interval = mco.Spec.ObservabilityAddonSpec.Interval
-		addon.Spec.ScrapeSizeLimitBytes = mco.Spec.ObservabilityAddonSpec.ScrapeSizeLimitBytes
-		addon.Spec.Workers = mco.Spec.ObservabilityAddonSpec.Workers
-		addon.Spec.Resources = config.GetOBAResources(mco.Spec.ObservabilityAddonSpec, mco.Spec.InstanceSize)
+	addon.Annotations = found.Annotations
 
-		addon.Annotations[addonSourceAnnotation] = "mco"
+	if found.Annotations[addonSourceAnnotation] == addonSourceMCO {
+		setObservabilityAddonSpec(addon, mco.Spec.ObservabilityAddonSpec, config.GetOBAResources(mco.Spec.ObservabilityAddonSpec, mco.Spec.InstanceSize))
 	}
 
-	if found.Annotations[addonSourceAnnotation] == "override" {
-		addon.Spec.EnableMetrics = found.Spec.EnableMetrics
-		addon.Spec.Interval = found.Spec.Interval
-		addon.Spec.ScrapeSizeLimitBytes = found.Spec.ScrapeSizeLimitBytes
-		addon.Spec.Workers = found.Spec.Workers
-		addon.Spec.Resources = found.Spec.Resources
-
-		addon.Annotations[addonSourceAnnotation] = "override"
+	if found.Annotations[addonSourceAnnotation] == addonSourceOverride {
+		setObservabilityAddonSpec(addon, &found.Spec, found.Spec.Resources)
 	}
 
 	return addon, nil

diff --git a/operators/multiclusterobservability/controllers/placementrule/obsaddon.go b/operators/multiclusterobservability/controllers/placementrule/obsaddon.go
@@ -6,16 +6,20 @@ package placementrule
 
 import (
 	"context"
+	"fmt"
+	"reflect"
 	"time"
 
 	"github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/pkg/config"
 
 	"golang.org/x/exp/slices"
+	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
+	obshared "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/shared"
 	obsv1beta1 "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/v1beta1"
 	mcov1beta2 "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/v1beta2"
 	"github.com/stolostron/multicluster-observability-operator/operators/pkg/util"
@@ -25,6 +29,8 @@ const (
 	obsAddonName          = "observability-addon"
 	obsAddonFinalizer     = "observability.open-cluster-management.io/addon-cleanup"
 	addonSourceAnnotation = "observability.open-cluster-management.io/addon-source"
+	addonSourceMCO        = "mco"
+	addonSourceOverride   = "override"
 )
 
 func deleteObsAddon(c client.Client, namespace string) error {
@@ -77,7 +83,7 @@ func createObsAddon(mco *mcov1beta2.MultiClusterObservability, c client.Client,
 			Name:      obsAddonName,
 			Namespace: namespace,
 			Annotations: map[string]string{
-				addonSourceAnnotation: "mco",
+				addonSourceAnnotation: addonSourceMCO,
 			},
 			Labels: map[string]string{
 				ownerLabelKey: ownerLabelValue,
@@ -86,10 +92,7 @@ func createObsAddon(mco *mcov1beta2.MultiClusterObservability, c client.Client,
 	}
 
 	if mco.Spec.ObservabilityAddonSpec != nil {
-		ec.Spec.EnableMetrics = mco.Spec.ObservabilityAddonSpec.EnableMetrics
-		ec.Spec.Interval = mco.Spec.ObservabilityAddonSpec.Interval
-		ec.Spec.ScrapeSizeLimitBytes = mco.Spec.ObservabilityAddonSpec.ScrapeSizeLimitBytes
-		ec.Spec.Workers = mco.Spec.ObservabilityAddonSpec.Workers
+		setObservabilityAddonSpec(ec, mco.Spec.ObservabilityAddonSpec, config.GetOBAResources(mco.Spec.ObservabilityAddonSpec, mco.Spec.InstanceSize))
 	}
 
 	found := &obsv1beta1.ObservabilityAddon{}
@@ -114,14 +117,12 @@ func createObsAddon(mco *mcov1beta2.MultiClusterObservability, c client.Client,
 	}
 
 	// Check if existing addon was created by MCO
-	if found.Annotations != nil && found.Annotations[addonSourceAnnotation] == "mco" {
+	if found.Annotations != nil && found.Annotations[addonSourceAnnotation] == addonSourceMCO {
 		// Only update if specs are different
-		if found.Spec != ec.Spec {
-			found.Spec = ec.Spec
+		if !reflect.DeepEqual(found.Spec, ec.Spec) {
 			err = c.Update(context.TODO(), found)
 			if err != nil {
-				log.Error(err, "Failed to update observabilityaddon cr")
-				return err
+				return fmt.Errorf("failed to update observabilityaddon cr: %w", err)
 			}
 			log.Info("observabilityaddon updated", "namespace", namespace)
 			return nil
@@ -177,3 +178,14 @@ func deleteFinalizer(c client.Client, obsaddon *obsv1beta1.ObservabilityAddon) e
 	}
 	return nil
 }
+
+// setObservabilityAddonSpec sets the ObservabilityAddon spec fields from the given MCO spec
+func setObservabilityAddonSpec(addonSpec *obsv1beta1.ObservabilityAddon, desiredSpec *obshared.ObservabilityAddonSpec, resources *corev1.ResourceRequirements) {
+	if desiredSpec != nil {
+		addonSpec.Spec.EnableMetrics = desiredSpec.EnableMetrics
+		addonSpec.Spec.Interval = desiredSpec.Interval
+		addonSpec.Spec.ScrapeSizeLimitBytes = desiredSpec.ScrapeSizeLimitBytes
+		addonSpec.Spec.Workers = desiredSpec.Workers
+		addonSpec.Spec.Resources = resources
+	}
+}
diff --git a/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go b/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go
@@ -649,15 +649,17 @@ func (r *PlacementRuleReconciler) SetupWithManager(mgr ctrl.Manager) error {
 			return false
 		},
 		UpdateFunc: func(e event.UpdateEvent) bool {
+			equalStatus := reflect.DeepEqual(e.ObjectNew.(*mcov1beta1.ObservabilityAddon).Status.Conditions,
+				e.ObjectOld.(*mcov1beta1.ObservabilityAddon).Status.Conditions)
+			equalSpec := reflect.DeepEqual(e.ObjectNew.(*mcov1beta1.ObservabilityAddon).Spec,
+				e.ObjectOld.(*mcov1beta1.ObservabilityAddon).Spec)
+			equalAnnotations := reflect.DeepEqual(e.ObjectNew.(*mcov1beta1.ObservabilityAddon).Annotations,
+				e.ObjectOld.(*mcov1beta1.ObservabilityAddon).Annotations)
+
 			if e.ObjectNew.GetName() == obsAddonName &&
 				e.ObjectNew.GetLabels()[ownerLabelKey] == ownerLabelValue &&
 				e.ObjectNew.GetNamespace() != localClusterName &&
-				!reflect.DeepEqual(e.ObjectNew.(*mcov1beta1.ObservabilityAddon).Status.Conditions,
-					e.ObjectOld.(*mcov1beta1.ObservabilityAddon).Status.Conditions) &&
-				!reflect.DeepEqual(e.ObjectNew.(*mcov1beta1.ObservabilityAddon).Spec,
-					e.ObjectOld.(*mcov1beta1.ObservabilityAddon).Spec) &&
-				!reflect.DeepEqual(e.ObjectNew.(*mcov1beta1.ObservabilityAddon).Annotations,
-					e.ObjectOld.(*mcov1beta1.ObservabilityAddon).Annotations) {
+				(!equalStatus || !equalSpec || !equalAnnotations) {
 				return true
 			}
 			return false