[ACM-16286]: Ensure MCO spec addon options are propagated correctly (#…

…1718) (#1725)

* [ACM-16286]: Ensure MCO spec addon options are propagated correctly



* [ACM-16286]: Update all observabilityaddon manifests



* [ACM-16286]: Ensure hub and spoke obsaddon are synced based on spec



* [ACM-16286]: Allow customizing ObsAddon in spoke ns



* [ACM-16286]: Comments



* Add addon source annotation



* Add handling for empty map



* Add handling for nil maps



* Add annotation predicate, update in mco mode



* Fix unit test, make addon annotation constant



* Update comment



* Address comments



* Use semantic deepequal, add test for setter



* Set spoke addon correctly after comparison



---------

Signed-off-by: Saswata Mukherjee <[email protected]>

operators/endpointmetrics/config/crd/bases/observability.open-cluster-management.io_observabilityaddons.yaml

@@ -1,9 +1,8 @@
----
 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.4.1
+    controller-gen.kubebuilder.io/version: v0.14.0
   creationTimestamp: null
   name: observabilityaddons.observability.open-cluster-management.io
 spec:
@@ -23,33 +22,109 @@ spec:
         description: ObservabilityAddon is the Schema for the observabilityaddon API
         properties:
           apiVersion:
-            description: 'APIVersion defines the versioned schema of this representation
-              of an object. Servers should convert recognized schemas to the latest
-              internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
+            description: |-
+              APIVersion defines the versioned schema of this representation of an object.
+              Servers should convert recognized schemas to the latest internal value, and
+              may reject unrecognized values.
+              More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources
             type: string
           kind:
-            description: 'Kind is a string value representing the REST resource this
-              object represents. Servers may infer this from the endpoint the client
-              submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
+            description: |-
+              Kind is a string value representing the REST resource this object represents.
+              Servers may infer this from the endpoint the client submits requests to.
+              Cannot be updated.
+              In CamelCase.
+              More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds
             type: string
           metadata:
             type: object
           spec:
-            description: ObservabilityAddonSpec is the spec of observability addon
+            description: ObservabilityAddonSpec is the spec of observability addon.
             properties:
               enableMetrics:
                 default: true
                 description: EnableMetrics indicates the observability addon push
                   metrics to hub server.
                 type: boolean
               interval:
-                default: 30
+                default: 300
                 description: Interval for the observability addon push metrics to
                   hub server.
                 format: int32
                 maximum: 3600
                 minimum: 15
                 type: integer
+              resources:
+                description: Resource requirement for metrics-collector
+                properties:
+                  claims:
+                    description: |-
+                      Claims lists the names of resources, defined in spec.resourceClaims,
+                      that are used by this container.
+
+
+                      This is an alpha field and requires enabling the
+                      DynamicResourceAllocation feature gate.
+
+
+                      This field is immutable. It can only be set for containers.
+                    items:
+                      description: ResourceClaim references one entry in PodSpec.ResourceClaims.
+                      properties:
+                        name:
+                          description: |-
+                            Name must match the name of one entry in pod.spec.resourceClaims of
+                            the Pod where this field is used. It makes that resource available
+                            inside a container.
+                          type: string
+                      required:
+                      - name
+                      type: object
+                    type: array
+                    x-kubernetes-list-map-keys:
+                    - name
+                    x-kubernetes-list-type: map
+                  limits:
+                    additionalProperties:
+                      anyOf:
+                      - type: integer
+                      - type: string
+                      pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
+                      x-kubernetes-int-or-string: true
+                    description: |-
+                      Limits describes the maximum amount of compute resources allowed.
+                      More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/
+                    type: object
+                  requests:
+                    additionalProperties:
+                      anyOf:
+                      - type: integer
+                      - type: string
+                      pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
+                      x-kubernetes-int-or-string: true
+                    description: |-
+                      Requests describes the minimum amount of compute resources required.
+                      If Requests is omitted for a container, it defaults to Limits if that is explicitly specified,
+                      otherwise to an implementation-defined value. Requests cannot exceed Limits.
+                      More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/
+                    type: object
+                type: object
+              scrapeSizeLimitBytes:
+                default: 1073741824
+                description: |-
+                  ScrapeSizeLimitBytes is the max size in bytes for a single metrics scrape from in-cluster Prometheus.
+                  Default is 1 GiB.
+                type: integer
+              workers:
+                default: 1
+                description: |-
+                  Workers is the number of workers in metrics-collector that work in parallel to
+                  push metrics to hub server. If set to > 1, metrics-collector will shard
+                  /federate calls to Prometheus, based on matcher rules provided by allowlist.
+                  Ensure that number of matchers exceeds number of workers.
+                format: int32
+                minimum: 1
+                type: integer
             type: object
           status:
             description: ObservabilityAddonStatus defines the observed state of ObservabilityAddon
@@ -90,5 +165,5 @@ status:
   acceptedNames:
     kind: ""
     plural: ""
-  conditions: []
-  storedVersions: []
+  conditions: null
+  storedVersions: null

operators/multiclusterobservability/controllers/placementrule/manifestwork.go

@@ -35,7 +35,6 @@ import (
 	addonv1alpha1 "open-cluster-management.io/api/addon/v1alpha1"
 	workv1 "open-cluster-management.io/api/work/v1"
 
-	mcoshared "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/shared"
 	mcov1beta1 "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/v1beta1"
 	mcov1beta2 "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/v1beta2"
 	cert_controller "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/pkg/certificates"
@@ -940,6 +939,12 @@ func generateMetricsListCM(client client.Client) (*corev1.ConfigMap, *corev1.Con
 	return metricsAllowlistCM, ocp311AllowlistCM, nil
 }
 
+// getObservabilityAddon gets the ObservabilityAddon in the spoke namespace in the hub cluster.
+// This is then synced to the actual spoke, by injecting it into the manifestwork.
+// We assume that an existing addon will always be found here as we create it initially.
+// If the addon is found with the mco source annotation, it will update the existing addon with the new values from MCO
+// If the addon is found with the override source annotation, it will not update the existing addon but it will use the existing values.
+// If the addon is found without any source annotation, it will add the mco source annotation and use the MCO values (upgrade case from ACM 2.12.2).
 func getObservabilityAddon(c client.Client, namespace string,
 	mco *mcov1beta2.MultiClusterObservability) (*mcov1beta1.ObservabilityAddon, error) {
 	if namespace == config.GetDefaultNamespace() {
@@ -952,30 +957,47 @@ func getObservabilityAddon(c client.Client, namespace string,
 	}
 	err := c.Get(context.TODO(), namespacedName, found)
 	if err != nil {
-		if k8serrors.IsNotFound(err) {
-			return nil, nil
+		if !k8serrors.IsNotFound(err) {
+			log.Error(err, "Failed to check observabilityAddon")
+			return nil, err
 		}
-		log.Error(err, "Failed to check observabilityAddon")
-		return nil, err
 	}
 	if found.ObjectMeta.DeletionTimestamp != nil {
 		return nil, nil
 	}
-	return &mcov1beta1.ObservabilityAddon{
+
+	addon := &mcov1beta1.ObservabilityAddon{
 		TypeMeta: metav1.TypeMeta{
 			APIVersion: "observability.open-cluster-management.io/v1beta1",
 			Kind:       "ObservabilityAddon",
 		},
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      obsAddonName,
-			Namespace: spokeNameSpace,
-		},
-		Spec: mcoshared.ObservabilityAddonSpec{
-			EnableMetrics: mco.Spec.ObservabilityAddonSpec.EnableMetrics,
-			Interval:      mco.Spec.ObservabilityAddonSpec.Interval,
-			Resources:     config.GetOBAResources(mco.Spec.ObservabilityAddonSpec, mco.Spec.InstanceSize),
+			Name:        obsAddonName,
+			Namespace:   spokeNameSpace,
+			Annotations: make(map[string]string),
 		},
-	}, nil
+	}
+
+	// Handle cases where the addon doesn't have the annotation
+	if found.Annotations == nil {
+		found.Annotations = make(map[string]string)
+	}
+
+	if _, ok := found.Annotations[addonSourceAnnotation]; !ok {
+		found.Annotations[addonSourceAnnotation] = addonSourceMCO
+	}
+
+	addon.Annotations = found.Annotations
+
+	if found.Annotations[addonSourceAnnotation] == addonSourceMCO {
+		setObservabilityAddonSpec(addon, mco.Spec.ObservabilityAddonSpec, config.GetOBAResources(mco.Spec.ObservabilityAddonSpec, mco.Spec.InstanceSize))
+	}
+
+	if found.Annotations[addonSourceAnnotation] == addonSourceOverride {
+		setObservabilityAddonSpec(addon, &found.Spec, found.Spec.Resources)
+	}
+
+	return addon, nil
 }
 
 func removeObservabilityAddon(client client.Client, namespace string) error {

operators/multiclusterobservability/controllers/placementrule/manifestwork_test.go

@@ -33,7 +33,7 @@ import (
 
 const (
 	pullSecretName = "test-pull-secret"
-	workSize       = 13
+	workSize       = 14
 )
 
 func init() {

operators/multiclusterobservability/controllers/placementrule/obsaddon.go

@@ -6,23 +6,31 @@ package placementrule
 
 import (
 	"context"
+	"fmt"
 	"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/equality"
 	"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"
 )
 
 const (
-	obsAddonName      = "observability-addon"
-	obsAddonFinalizer = "observability.open-cluster-management.io/addon-cleanup"
+	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 {
@@ -58,7 +66,11 @@ func deleteObsAddon(c client.Client, namespace string) error {
 	return nil
 }
 
-func createObsAddon(c client.Client, namespace string) error {
+// createObsAddon creates the default ObservabilityAddon in the spoke namespace in the hub cluster.
+// It will initially mirror values from the MultiClusterObservability CR with the mco source annotation.
+// If an existing addon is found with the mco source annotation it will update the existing addon with the new values.
+// If the existing addon is created by the user with the override source annotation, it will not update the existing addon.
+func createObsAddon(mco *mcov1beta2.MultiClusterObservability, c client.Client, namespace string) error {
 	if namespace == config.GetDefaultNamespace() {
 		return nil
 	}
@@ -70,11 +82,19 @@ func createObsAddon(c client.Client, namespace string) error {
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      obsAddonName,
 			Namespace: namespace,
+			Annotations: map[string]string{
+				addonSourceAnnotation: addonSourceMCO,
+			},
 			Labels: map[string]string{
 				ownerLabelKey: ownerLabelValue,
 			},
 		},
 	}
+
+	if mco.Spec.ObservabilityAddonSpec != nil {
+		setObservabilityAddonSpec(ec, mco.Spec.ObservabilityAddonSpec, config.GetOBAResources(mco.Spec.ObservabilityAddonSpec, mco.Spec.InstanceSize))
+	}
+
 	found := &obsv1beta1.ObservabilityAddon{}
 	err := c.Get(context.TODO(), types.NamespacedName{Name: obsAddonName, Namespace: namespace}, found)
 	if err != nil && errors.IsNotFound(err) || err == nil && found.GetDeletionTimestamp() != nil {
@@ -96,6 +116,20 @@ func createObsAddon(c client.Client, namespace string) error {
 		return err
 	}
 
+	// Check if existing addon was created by MCO
+	if found.Annotations != nil && found.Annotations[addonSourceAnnotation] == addonSourceMCO {
+		// Only update if specs are different
+		if !equality.Semantic.DeepEqual(found.Spec, ec.Spec) {
+			found.Spec = ec.Spec
+			err = c.Update(context.TODO(), found)
+			if err != nil {
+				return fmt.Errorf("failed to update observabilityaddon cr: %w", err)
+			}
+			log.Info("observabilityaddon updated", "namespace", namespace)
+			return nil
+		}
+	}
+
 	log.Info("observabilityaddon already existed/unchanged", "namespace", namespace)
 	return nil
 }
@@ -145,3 +179,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
+	}
+}