From 7a32bb261f0807cc181f1eb5d28e05a7dab5b4f8 Mon Sep 17 00:00:00 2001 From: Saswata Mukherjee Date: Tue, 10 Dec 2024 10:12:31 +0000 Subject: [PATCH 01/14] [ACM-16286]: Ensure MCO spec addon options are propagated correctly Signed-off-by: Saswata Mukherjee --- .../controllers/placementrule/manifestwork.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/operators/multiclusterobservability/controllers/placementrule/manifestwork.go b/operators/multiclusterobservability/controllers/placementrule/manifestwork.go index eb4ecadee..843ecc431 100644 --- a/operators/multiclusterobservability/controllers/placementrule/manifestwork.go +++ b/operators/multiclusterobservability/controllers/placementrule/manifestwork.go @@ -971,9 +971,11 @@ func getObservabilityAddon(c client.Client, namespace string, Namespace: spokeNameSpace, }, Spec: mcoshared.ObservabilityAddonSpec{ - EnableMetrics: mco.Spec.ObservabilityAddonSpec.EnableMetrics, - Interval: mco.Spec.ObservabilityAddonSpec.Interval, - Resources: config.GetOBAResources(mco.Spec.ObservabilityAddonSpec, mco.Spec.InstanceSize), + EnableMetrics: mco.Spec.ObservabilityAddonSpec.EnableMetrics, + Interval: mco.Spec.ObservabilityAddonSpec.Interval, + ScrapeSizeLimitBytes: mco.Spec.ObservabilityAddonSpec.ScrapeSizeLimitBytes, + Workers: mco.Spec.ObservabilityAddonSpec.Workers, + Resources: config.GetOBAResources(mco.Spec.ObservabilityAddonSpec, mco.Spec.InstanceSize), }, }, nil } From 607bb06773c137add233864e7c397f3beed3fee3 Mon Sep 17 00:00:00 2001 From: Saswata Mukherjee Date: Tue, 10 Dec 2024 11:05:07 +0000 Subject: [PATCH 02/14] [ACM-16286]: Update all observabilityaddon manifests Signed-off-by: Saswata Mukherjee --- ...ter-management.io_observabilityaddons.yaml | 99 ++++++++++++++++--- ...-management.io_observabilityaddon_crd.yaml | 79 ++++++++++++++- 2 files changed, 162 insertions(+), 16 deletions(-) diff --git a/operators/endpointmetrics/config/crd/bases/observability.open-cluster-management.io_observabilityaddons.yaml b/operators/endpointmetrics/config/crd/bases/observability.open-cluster-management.io_observabilityaddons.yaml index 047213f1d..cd52f4a9f 100644 --- a/operators/endpointmetrics/config/crd/bases/observability.open-cluster-management.io_observabilityaddons.yaml +++ b/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,19 +22,24 @@ 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 @@ -43,13 +47,84 @@ spec: 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 diff --git a/operators/multiclusterobservability/manifests/endpoint-observability/observability.open-cluster-management.io_observabilityaddon_crd.yaml b/operators/multiclusterobservability/manifests/endpoint-observability/observability.open-cluster-management.io_observabilityaddon_crd.yaml index a5e99c23a..cd52f4a9f 100644 --- a/operators/multiclusterobservability/manifests/endpoint-observability/observability.open-cluster-management.io_observabilityaddon_crd.yaml +++ b/operators/multiclusterobservability/manifests/endpoint-observability/observability.open-cluster-management.io_observabilityaddon_crd.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: @@ -20,26 +19,71 @@ spec: - name: v1beta1 schema: openAPIV3Schema: + 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 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 type: string metadata: type: object spec: + 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: 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: @@ -47,6 +91,9 @@ spec: - 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: @@ -55,13 +102,37 @@ spec: - 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 properties: conditions: items: + description: StatusCondition contains condition information for + an observability addon properties: lastTransitionTime: format: date-time @@ -94,5 +165,5 @@ status: acceptedNames: kind: "" plural: "" - conditions: [] - storedVersions: [] + conditions: null + storedVersions: null From 5ed10c55bebcc5eb251d71dad2b3500481f1996f Mon Sep 17 00:00:00 2001 From: Saswata Mukherjee Date: Tue, 10 Dec 2024 12:08:59 +0000 Subject: [PATCH 03/14] [ACM-16286]: Ensure hub and spoke obsaddon are synced based on spec Signed-off-by: Saswata Mukherjee --- .../controllers/placementrule/placementrule_controller.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go b/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go index ce031fb5b..8c614a6f1 100644 --- a/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go +++ b/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go @@ -653,7 +653,9 @@ func (r *PlacementRuleReconciler) SetupWithManager(mgr ctrl.Manager) error { e.ObjectNew.GetLabels()[ownerLabelKey] == ownerLabelValue && e.ObjectNew.GetNamespace() != localClusterName && !reflect.DeepEqual(e.ObjectNew.(*mcov1beta1.ObservabilityAddon).Status.Conditions, - e.ObjectOld.(*mcov1beta1.ObservabilityAddon).Status.Conditions) { + e.ObjectOld.(*mcov1beta1.ObservabilityAddon).Status.Conditions) && + !reflect.DeepEqual(e.ObjectNew.(*mcov1beta1.ObservabilityAddon).Spec, + e.ObjectOld.(*mcov1beta1.ObservabilityAddon).Spec) { return true } return false From 605534c85bc2bcc769f2215911c7489f0ef57dac Mon Sep 17 00:00:00 2001 From: Saswata Mukherjee Date: Tue, 10 Dec 2024 14:01:49 +0000 Subject: [PATCH 04/14] [ACM-16286]: Allow customizing ObsAddon in spoke ns Signed-off-by: Saswata Mukherjee --- .../controllers/placementrule/manifestwork.go | 27 +++++++++++++++++-- .../controllers/placementrule/obsaddon.go | 11 +++++++- .../placementrule/placementrule_controller.go | 2 +- 3 files changed, 36 insertions(+), 4 deletions(-) diff --git a/operators/multiclusterobservability/controllers/placementrule/manifestwork.go b/operators/multiclusterobservability/controllers/placementrule/manifestwork.go index 843ecc431..20f003ced 100644 --- a/operators/multiclusterobservability/controllers/placementrule/manifestwork.go +++ b/operators/multiclusterobservability/controllers/placementrule/manifestwork.go @@ -961,7 +961,8 @@ func getObservabilityAddon(c client.Client, namespace string, 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", @@ -977,7 +978,29 @@ func getObservabilityAddon(c client.Client, namespace string, Workers: mco.Spec.ObservabilityAddonSpec.Workers, Resources: config.GetOBAResources(mco.Spec.ObservabilityAddonSpec, mco.Spec.InstanceSize), }, - }, nil + } + + if !found.Spec.EnableMetrics { + addon.Spec.EnableMetrics = false + } + + if found.Spec.Interval != 300 { + addon.Spec.Interval = found.Spec.Interval + } + + if found.Spec.ScrapeSizeLimitBytes != 1073741824 { + addon.Spec.ScrapeSizeLimitBytes = found.Spec.ScrapeSizeLimitBytes + } + + if found.Spec.Workers != 1 { + addon.Spec.Workers = found.Spec.Workers + } + + if found.Spec.Resources != nil { + addon.Spec.Resources = found.Spec.Resources + } + + return addon, nil } func removeObservabilityAddon(client client.Client, namespace string) error { diff --git a/operators/multiclusterobservability/controllers/placementrule/obsaddon.go b/operators/multiclusterobservability/controllers/placementrule/obsaddon.go index 0ec0ab720..e0df4d3cf 100644 --- a/operators/multiclusterobservability/controllers/placementrule/obsaddon.go +++ b/operators/multiclusterobservability/controllers/placementrule/obsaddon.go @@ -17,6 +17,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" 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" ) @@ -58,7 +59,7 @@ func deleteObsAddon(c client.Client, namespace string) error { return nil } -func createObsAddon(c client.Client, namespace string) error { +func createObsAddon(mco *mcov1beta2.MultiClusterObservability, c client.Client, namespace string) error { if namespace == config.GetDefaultNamespace() { return nil } @@ -75,6 +76,14 @@ func createObsAddon(c client.Client, namespace string) error { }, }, } + + 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 + } + 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 { diff --git a/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go b/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go index 8c614a6f1..62440db99 100644 --- a/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go +++ b/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go @@ -508,7 +508,7 @@ func createManagedClusterRes( hubInfo *corev1.Secret, installProm bool, ) error { - err := createObsAddon(c, namespace) + err := createObsAddon(mco, c, namespace) if err != nil { log.Error(err, "Failed to create observabilityaddon") return err From 24423dfd0b1aa948d2d147425e3ab28939f352c8 Mon Sep 17 00:00:00 2001 From: Saswata Mukherjee Date: Tue, 10 Dec 2024 14:34:00 +0000 Subject: [PATCH 05/14] [ACM-16286]: Comments Signed-off-by: Saswata Mukherjee --- .../controllers/placementrule/manifestwork.go | 11 +++++++---- .../controllers/placementrule/obsaddon.go | 3 +++ .../controllers/placementrule/obsaddon_test.go | 6 +++--- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/operators/multiclusterobservability/controllers/placementrule/manifestwork.go b/operators/multiclusterobservability/controllers/placementrule/manifestwork.go index 20f003ced..5fd021238 100644 --- a/operators/multiclusterobservability/controllers/placementrule/manifestwork.go +++ b/operators/multiclusterobservability/controllers/placementrule/manifestwork.go @@ -940,6 +940,10 @@ 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. +// It will set the MCO spec by default, but if the existing spoke namespace ObservabilityAddon exists, +// it will use that. func getObservabilityAddon(c client.Client, namespace string, mco *mcov1beta2.MultiClusterObservability) (*mcov1beta1.ObservabilityAddon, error) { if namespace == config.GetDefaultNamespace() { @@ -952,11 +956,10 @@ 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 diff --git a/operators/multiclusterobservability/controllers/placementrule/obsaddon.go b/operators/multiclusterobservability/controllers/placementrule/obsaddon.go index e0df4d3cf..d85681e92 100644 --- a/operators/multiclusterobservability/controllers/placementrule/obsaddon.go +++ b/operators/multiclusterobservability/controllers/placementrule/obsaddon.go @@ -59,6 +59,9 @@ func deleteObsAddon(c client.Client, namespace string) error { return nil } +// createObsAddon creates the default ObservabilityAddon in the spoke namespace in the hub cluster. +// It will initially mirror values from the MultiClusterObservability CR. +// But if changed, it will use the new values, until you delete it. func createObsAddon(mco *mcov1beta2.MultiClusterObservability, c client.Client, namespace string) error { if namespace == config.GetDefaultNamespace() { return nil diff --git a/operators/multiclusterobservability/controllers/placementrule/obsaddon_test.go b/operators/multiclusterobservability/controllers/placementrule/obsaddon_test.go index a80694d48..72b3a9b77 100644 --- a/operators/multiclusterobservability/controllers/placementrule/obsaddon_test.go +++ b/operators/multiclusterobservability/controllers/placementrule/obsaddon_test.go @@ -31,7 +31,7 @@ func TestObsAddonCR(t *testing.T) { ). Build() - err := createObsAddon(c, namespace) + err := createObsAddon(&mcov1beta2.MultiClusterObservability{}, c, namespace) if err != nil { t.Fatalf("Failed to create observabilityaddon: (%v)", err) } @@ -41,7 +41,7 @@ func TestObsAddonCR(t *testing.T) { t.Fatalf("Failed to get observabilityaddon: (%v)", err) } - err = createObsAddon(c, namespace) + err = createObsAddon(&mcov1beta2.MultiClusterObservability{}, c, namespace) if err != nil { t.Fatalf("Failed to create observabilityaddon: (%v)", err) } @@ -88,7 +88,7 @@ func TestStaleObsAddonCR(t *testing.T) { objs := []runtime.Object{newTestObsApiRoute()} c := fake.NewClientBuilder().WithRuntimeObjects(objs...).Build() - err := createObsAddon(c, namespace) + err := createObsAddon(&mcov1beta2.MultiClusterObservability{}, c, namespace) if err != nil { t.Fatalf("Failed to create observabilityaddon: (%v)", err) } From d62f3c1369393c03049cdad56bfd5c48d3630c5b Mon Sep 17 00:00:00 2001 From: Saswata Mukherjee Date: Wed, 11 Dec 2024 10:07:09 +0000 Subject: [PATCH 06/14] Add addon source annotation Signed-off-by: Saswata Mukherjee --- .../controllers/placementrule/manifestwork.go | 34 ++++++++----------- .../controllers/placementrule/obsaddon.go | 3 ++ 2 files changed, 17 insertions(+), 20 deletions(-) diff --git a/operators/multiclusterobservability/controllers/placementrule/manifestwork.go b/operators/multiclusterobservability/controllers/placementrule/manifestwork.go index 5fd021238..fd9021ff0 100644 --- a/operators/multiclusterobservability/controllers/placementrule/manifestwork.go +++ b/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" @@ -944,6 +943,8 @@ func generateMetricsListCM(client client.Client) (*corev1.ConfigMap, *corev1.Con // This is then synced to the actual spoke, by injecting it into the manifestwork. // It will set the MCO spec by default, but if the existing spoke namespace ObservabilityAddon exists, // it will use that. +// If the found ObservabilityAddon exists with the default values, it will always prefer the MCO spec. +// However, if the found ObservabilityAddon exists with the non-default values, it will use the found values, even if the MCO spec is different. func getObservabilityAddon(c client.Client, namespace string, mco *mcov1beta2.MultiClusterObservability) (*mcov1beta1.ObservabilityAddon, error) { if namespace == config.GetDefaultNamespace() { @@ -974,33 +975,26 @@ func getObservabilityAddon(c client.Client, namespace string, Name: obsAddonName, Namespace: spokeNameSpace, }, - Spec: mcoshared.ObservabilityAddonSpec{ - EnableMetrics: mco.Spec.ObservabilityAddonSpec.EnableMetrics, - Interval: mco.Spec.ObservabilityAddonSpec.Interval, - ScrapeSizeLimitBytes: mco.Spec.ObservabilityAddonSpec.ScrapeSizeLimitBytes, - Workers: mco.Spec.ObservabilityAddonSpec.Workers, - Resources: config.GetOBAResources(mco.Spec.ObservabilityAddonSpec, mco.Spec.InstanceSize), - }, } - if !found.Spec.EnableMetrics { - addon.Spec.EnableMetrics = false - } + if found.Annotations["observability.open-cluster-management.io/addon-source"] == "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) - if found.Spec.Interval != 300 { - addon.Spec.Interval = found.Spec.Interval + addon.Annotations["observability.open-cluster-management.io/addon-source"] = "mco" } - if found.Spec.ScrapeSizeLimitBytes != 1073741824 { + if found.Annotations["observability.open-cluster-management.io/addon-source"] == "override" { + addon.Spec.EnableMetrics = found.Spec.EnableMetrics + addon.Spec.Interval = found.Spec.Interval addon.Spec.ScrapeSizeLimitBytes = found.Spec.ScrapeSizeLimitBytes - } - - if found.Spec.Workers != 1 { addon.Spec.Workers = found.Spec.Workers - } - - if found.Spec.Resources != nil { addon.Spec.Resources = found.Spec.Resources + + addon.Annotations["observability.open-cluster-management.io/addon-source"] = "override" } return addon, nil diff --git a/operators/multiclusterobservability/controllers/placementrule/obsaddon.go b/operators/multiclusterobservability/controllers/placementrule/obsaddon.go index d85681e92..98d231486 100644 --- a/operators/multiclusterobservability/controllers/placementrule/obsaddon.go +++ b/operators/multiclusterobservability/controllers/placementrule/obsaddon.go @@ -74,6 +74,9 @@ func createObsAddon(mco *mcov1beta2.MultiClusterObservability, c client.Client, ObjectMeta: metav1.ObjectMeta{ Name: obsAddonName, Namespace: namespace, + Annotations: map[string]string{ + "observability.open-cluster-management.io/addon-source": "mco", + }, Labels: map[string]string{ ownerLabelKey: ownerLabelValue, }, From 242cfec990606ce23322bdd698bbb5bb5f423fc5 Mon Sep 17 00:00:00 2001 From: Saswata Mukherjee Date: Wed, 11 Dec 2024 11:16:56 +0000 Subject: [PATCH 07/14] Add handling for empty map Signed-off-by: Saswata Mukherjee --- .../controllers/placementrule/manifestwork.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/operators/multiclusterobservability/controllers/placementrule/manifestwork.go b/operators/multiclusterobservability/controllers/placementrule/manifestwork.go index fd9021ff0..f2ef3235a 100644 --- a/operators/multiclusterobservability/controllers/placementrule/manifestwork.go +++ b/operators/multiclusterobservability/controllers/placementrule/manifestwork.go @@ -977,6 +977,11 @@ func getObservabilityAddon(c client.Client, namespace string, }, } + // Handle cases where the addon doesn't have the annotation + if _, ok := found.Annotations["observability.open-cluster-management.io/addon-source"]; !ok { + found.Annotations["observability.open-cluster-management.io/addon-source"] = "mco" + } + if found.Annotations["observability.open-cluster-management.io/addon-source"] == "mco" { addon.Spec.EnableMetrics = mco.Spec.ObservabilityAddonSpec.EnableMetrics addon.Spec.Interval = mco.Spec.ObservabilityAddonSpec.Interval From a9665f9fbd74b6f355e225c7500c9054e8f97d69 Mon Sep 17 00:00:00 2001 From: Saswata Mukherjee Date: Wed, 11 Dec 2024 11:29:58 +0000 Subject: [PATCH 08/14] Add handling for nil maps Signed-off-by: Saswata Mukherjee --- .../controllers/placementrule/manifestwork.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/operators/multiclusterobservability/controllers/placementrule/manifestwork.go b/operators/multiclusterobservability/controllers/placementrule/manifestwork.go index f2ef3235a..434a610e7 100644 --- a/operators/multiclusterobservability/controllers/placementrule/manifestwork.go +++ b/operators/multiclusterobservability/controllers/placementrule/manifestwork.go @@ -972,12 +972,17 @@ func getObservabilityAddon(c client.Client, namespace string, Kind: "ObservabilityAddon", }, ObjectMeta: metav1.ObjectMeta{ - Name: obsAddonName, - Namespace: spokeNameSpace, + Name: obsAddonName, + Namespace: spokeNameSpace, + Annotations: make(map[string]string), }, } // Handle cases where the addon doesn't have the annotation + if found.Annotations == nil { + found.Annotations = make(map[string]string) + } + if _, ok := found.Annotations["observability.open-cluster-management.io/addon-source"]; !ok { found.Annotations["observability.open-cluster-management.io/addon-source"] = "mco" } From 1e32f015c46923027f3f4a10d43bbaf661a03011 Mon Sep 17 00:00:00 2001 From: Saswata Mukherjee Date: Wed, 11 Dec 2024 14:09:50 +0000 Subject: [PATCH 09/14] Add annotation predicate, update in mco mode Signed-off-by: Saswata Mukherjee --- .../controllers/placementrule/obsaddon.go | 15 +++++++++++++++ .../placementrule/placementrule_controller.go | 4 +++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/operators/multiclusterobservability/controllers/placementrule/obsaddon.go b/operators/multiclusterobservability/controllers/placementrule/obsaddon.go index 98d231486..2b5a1d299 100644 --- a/operators/multiclusterobservability/controllers/placementrule/obsaddon.go +++ b/operators/multiclusterobservability/controllers/placementrule/obsaddon.go @@ -111,6 +111,21 @@ func createObsAddon(mco *mcov1beta2.MultiClusterObservability, c client.Client, return err } + // Check if existing addon was created by MCO + if found.Annotations != nil && found.Annotations["observability.open-cluster-management.io/addon-source"] == "mco" { + // Only update if specs are different + if found.Spec != ec.Spec { + found.Spec = ec.Spec + err = c.Update(context.TODO(), found) + if err != nil { + log.Error(err, "Failed to update observabilityaddon cr") + return err + } + log.Info("observabilityaddon updated", "namespace", namespace) + return nil + } + } + log.Info("observabilityaddon already existed/unchanged", "namespace", namespace) return nil } diff --git a/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go b/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go index 62440db99..70a840489 100644 --- a/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go +++ b/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go @@ -655,7 +655,9 @@ func (r *PlacementRuleReconciler) SetupWithManager(mgr ctrl.Manager) error { !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) { + e.ObjectOld.(*mcov1beta1.ObservabilityAddon).Spec) && + !reflect.DeepEqual(e.ObjectNew.(*mcov1beta1.ObservabilityAddon).Annotations, + e.ObjectOld.(*mcov1beta1.ObservabilityAddon).Annotations) { return true } return false From c9b609ec70fe63c5314a2122e8247220a78867e7 Mon Sep 17 00:00:00 2001 From: Saswata Mukherjee Date: Thu, 12 Dec 2024 10:28:41 +0000 Subject: [PATCH 10/14] Fix unit test, make addon annotation constant Signed-off-by: Saswata Mukherjee --- .../controllers/placementrule/manifestwork.go | 12 ++++++------ .../controllers/placementrule/manifestwork_test.go | 2 +- .../controllers/placementrule/obsaddon.go | 9 +++++---- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/operators/multiclusterobservability/controllers/placementrule/manifestwork.go b/operators/multiclusterobservability/controllers/placementrule/manifestwork.go index 434a610e7..51040e785 100644 --- a/operators/multiclusterobservability/controllers/placementrule/manifestwork.go +++ b/operators/multiclusterobservability/controllers/placementrule/manifestwork.go @@ -983,28 +983,28 @@ func getObservabilityAddon(c client.Client, namespace string, found.Annotations = make(map[string]string) } - if _, ok := found.Annotations["observability.open-cluster-management.io/addon-source"]; !ok { - found.Annotations["observability.open-cluster-management.io/addon-source"] = "mco" + if _, ok := found.Annotations[addonSourceAnnotation]; !ok { + found.Annotations[addonSourceAnnotation] = "mco" } - if found.Annotations["observability.open-cluster-management.io/addon-source"] == "mco" { + 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["observability.open-cluster-management.io/addon-source"] = "mco" + addon.Annotations[addonSourceAnnotation] = "mco" } - if found.Annotations["observability.open-cluster-management.io/addon-source"] == "override" { + 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["observability.open-cluster-management.io/addon-source"] = "override" + addon.Annotations[addonSourceAnnotation] = "override" } return addon, nil diff --git a/operators/multiclusterobservability/controllers/placementrule/manifestwork_test.go b/operators/multiclusterobservability/controllers/placementrule/manifestwork_test.go index eb419e448..1ec14a578 100644 --- a/operators/multiclusterobservability/controllers/placementrule/manifestwork_test.go +++ b/operators/multiclusterobservability/controllers/placementrule/manifestwork_test.go @@ -33,7 +33,7 @@ import ( const ( pullSecretName = "test-pull-secret" - workSize = 13 + workSize = 14 ) func init() { diff --git a/operators/multiclusterobservability/controllers/placementrule/obsaddon.go b/operators/multiclusterobservability/controllers/placementrule/obsaddon.go index 2b5a1d299..0af345ca8 100644 --- a/operators/multiclusterobservability/controllers/placementrule/obsaddon.go +++ b/operators/multiclusterobservability/controllers/placementrule/obsaddon.go @@ -22,8 +22,9 @@ import ( ) 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" ) func deleteObsAddon(c client.Client, namespace string) error { @@ -75,7 +76,7 @@ func createObsAddon(mco *mcov1beta2.MultiClusterObservability, c client.Client, Name: obsAddonName, Namespace: namespace, Annotations: map[string]string{ - "observability.open-cluster-management.io/addon-source": "mco", + addonSourceAnnotation: "mco", }, Labels: map[string]string{ ownerLabelKey: ownerLabelValue, @@ -112,7 +113,7 @@ func createObsAddon(mco *mcov1beta2.MultiClusterObservability, c client.Client, } // Check if existing addon was created by MCO - if found.Annotations != nil && found.Annotations["observability.open-cluster-management.io/addon-source"] == "mco" { + if found.Annotations != nil && found.Annotations[addonSourceAnnotation] == "mco" { // Only update if specs are different if found.Spec != ec.Spec { found.Spec = ec.Spec From 1ac83e425d9697ee2dc7b0589f9c39a9a59f285c Mon Sep 17 00:00:00 2001 From: Saswata Mukherjee Date: Thu, 12 Dec 2024 10:45:29 +0000 Subject: [PATCH 11/14] Update comment Signed-off-by: Saswata Mukherjee --- .../controllers/placementrule/manifestwork.go | 8 ++++---- .../controllers/placementrule/obsaddon.go | 5 +++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/operators/multiclusterobservability/controllers/placementrule/manifestwork.go b/operators/multiclusterobservability/controllers/placementrule/manifestwork.go index 51040e785..6af476d28 100644 --- a/operators/multiclusterobservability/controllers/placementrule/manifestwork.go +++ b/operators/multiclusterobservability/controllers/placementrule/manifestwork.go @@ -941,10 +941,10 @@ func generateMetricsListCM(client client.Client) (*corev1.ConfigMap, *corev1.Con // 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. -// It will set the MCO spec by default, but if the existing spoke namespace ObservabilityAddon exists, -// it will use that. -// If the found ObservabilityAddon exists with the default values, it will always prefer the MCO spec. -// However, if the found ObservabilityAddon exists with the non-default values, it will use the found values, even if the MCO spec is different. +// 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() { diff --git a/operators/multiclusterobservability/controllers/placementrule/obsaddon.go b/operators/multiclusterobservability/controllers/placementrule/obsaddon.go index 0af345ca8..bbe3d00f2 100644 --- a/operators/multiclusterobservability/controllers/placementrule/obsaddon.go +++ b/operators/multiclusterobservability/controllers/placementrule/obsaddon.go @@ -61,8 +61,9 @@ func deleteObsAddon(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. -// But if changed, it will use the new values, until you delete it. +// 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 From fe159737441c9c0a6637d73b4dfca7f39fd7a093 Mon Sep 17 00:00:00 2001 From: Saswata Mukherjee Date: Sun, 15 Dec 2024 11:24:52 +0000 Subject: [PATCH 12/14] Address comments Signed-off-by: Saswata Mukherjee --- .../controllers/placementrule/manifestwork.go | 22 ++++--------- .../controllers/placementrule/obsaddon.go | 32 +++++++++++++------ .../placementrule/placementrule_controller.go | 14 ++++---- 3 files changed, 36 insertions(+), 32 deletions(-) diff --git a/operators/multiclusterobservability/controllers/placementrule/manifestwork.go b/operators/multiclusterobservability/controllers/placementrule/manifestwork.go index 6af476d28..3d8f8bad0 100644 --- 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 index bbe3d00f2..63fb0f554 100644 --- 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 index 70a840489..520a357ec 100644 --- 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 From 20a306fd004a12b94b2fd3f288c79c6024da122d Mon Sep 17 00:00:00 2001 From: Saswata Mukherjee Date: Mon, 16 Dec 2024 11:17:45 +0000 Subject: [PATCH 13/14] Use semantic deepequal, add test for setter Signed-off-by: Saswata Mukherjee --- .../placementrule/obsaddon_test.go | 108 ++++++++++++++++++ .../placementrule/placementrule_controller.go | 8 +- 2 files changed, 112 insertions(+), 4 deletions(-) diff --git a/operators/multiclusterobservability/controllers/placementrule/obsaddon_test.go b/operators/multiclusterobservability/controllers/placementrule/obsaddon_test.go index 72b3a9b77..9e2a49bf2 100644 --- a/operators/multiclusterobservability/controllers/placementrule/obsaddon_test.go +++ b/operators/multiclusterobservability/controllers/placementrule/obsaddon_test.go @@ -8,12 +8,17 @@ import ( "context" "testing" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" addonv1alpha1 "open-cluster-management.io/api/addon/v1alpha1" "sigs.k8s.io/controller-runtime/pkg/client/fake" + obshared "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" ) @@ -109,3 +114,106 @@ func TestStaleObsAddonCR(t *testing.T) { t.Fatalf("Failed to remove stale observabilityaddon: (%v)", err) } } + +func TestSetObservabilityAddonSpec(t *testing.T) { + tests := []struct { + name string + desiredSpec *obshared.ObservabilityAddonSpec + resources *corev1.ResourceRequirements + want *mcov1beta1.ObservabilityAddon + }{ + { + name: "with desired spec", + desiredSpec: &obshared.ObservabilityAddonSpec{ + EnableMetrics: true, + Interval: 60, + ScrapeSizeLimitBytes: 1024, + Workers: 2, + }, + resources: &corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("200m"), + corev1.ResourceMemory: resource.MustParse("256Mi"), + }, + }, + want: &mcov1beta1.ObservabilityAddon{ + ObjectMeta: metav1.ObjectMeta{ + Name: obsAddonName, + Namespace: namespace, + Annotations: map[string]string{ + "test": "test", + }, + }, + Spec: obshared.ObservabilityAddonSpec{ + EnableMetrics: true, + Interval: 60, + ScrapeSizeLimitBytes: 1024, + Workers: 2, + Resources: &corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("200m"), + corev1.ResourceMemory: resource.MustParse("256Mi"), + }, + }, + }, + }, + }, + { + name: "with nil desired spec", + desiredSpec: nil, + resources: nil, + want: &mcov1beta1.ObservabilityAddon{ + ObjectMeta: metav1.ObjectMeta{ + Name: obsAddonName, + Namespace: namespace, + Annotations: map[string]string{ + "test": "test", + }, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := &mcov1beta1.ObservabilityAddon{ + ObjectMeta: metav1.ObjectMeta{ + Name: obsAddonName, + Namespace: namespace, + Annotations: map[string]string{ + "test": "test", + }, + }, + } + + setObservabilityAddonSpec(got, tt.desiredSpec, tt.resources) + + if !equality.Semantic.DeepEqual(got, tt.want) { + t.Errorf("setObservabilityAddonSpec() = %v, want %v", got, tt.want) + } + + override := &mcov1beta1.ObservabilityAddon{ + ObjectMeta: metav1.ObjectMeta{ + Name: obsAddonName, + Namespace: namespace, + Annotations: map[string]string{ + "test": "test", + }, + }, + Spec: obshared.ObservabilityAddonSpec{ + EnableMetrics: true, + Interval: 50, + ScrapeSizeLimitBytes: 24, + Workers: 1, + }, + } + setObservabilityAddonSpec(override, tt.desiredSpec, tt.resources) + + if tt.name != "with nil desired spec" { + if !equality.Semantic.DeepEqual(override, tt.want) { + t.Errorf("setObservabilityAddonSpec() = %v, want %v", override, tt.want) + } + } + }) + } +} diff --git a/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go b/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go index 520a357ec..269a2be25 100644 --- a/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go +++ b/operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go @@ -7,7 +7,6 @@ package placementrule import ( "context" "errors" - "reflect" "strings" "sync" "time" @@ -18,6 +17,7 @@ import ( "golang.org/x/exp/slices" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -649,11 +649,11 @@ 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, + equalStatus := equality.Semantic.DeepEqual(e.ObjectNew.(*mcov1beta1.ObservabilityAddon).Status.Conditions, e.ObjectOld.(*mcov1beta1.ObservabilityAddon).Status.Conditions) - equalSpec := reflect.DeepEqual(e.ObjectNew.(*mcov1beta1.ObservabilityAddon).Spec, + equalSpec := equality.Semantic.DeepEqual(e.ObjectNew.(*mcov1beta1.ObservabilityAddon).Spec, e.ObjectOld.(*mcov1beta1.ObservabilityAddon).Spec) - equalAnnotations := reflect.DeepEqual(e.ObjectNew.(*mcov1beta1.ObservabilityAddon).Annotations, + equalAnnotations := equality.Semantic.DeepEqual(e.ObjectNew.(*mcov1beta1.ObservabilityAddon).Annotations, e.ObjectOld.(*mcov1beta1.ObservabilityAddon).Annotations) if e.ObjectNew.GetName() == obsAddonName && From 54401ecad3d92b6da82ed387faf089d06fd25b72 Mon Sep 17 00:00:00 2001 From: Saswata Mukherjee Date: Mon, 16 Dec 2024 12:45:10 +0000 Subject: [PATCH 14/14] Set spoke addon correctly after comparison Signed-off-by: Saswata Mukherjee --- .../controllers/placementrule/obsaddon.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/operators/multiclusterobservability/controllers/placementrule/obsaddon.go b/operators/multiclusterobservability/controllers/placementrule/obsaddon.go index 63fb0f554..4df4acb7d 100644 --- a/operators/multiclusterobservability/controllers/placementrule/obsaddon.go +++ b/operators/multiclusterobservability/controllers/placementrule/obsaddon.go @@ -7,13 +7,13 @@ 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/equality" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -119,7 +119,8 @@ func createObsAddon(mco *mcov1beta2.MultiClusterObservability, c client.Client, // Check if existing addon was created by MCO if found.Annotations != nil && found.Annotations[addonSourceAnnotation] == addonSourceMCO { // Only update if specs are different - if !reflect.DeepEqual(found.Spec, ec.Spec) { + 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)