ACM-14962: init MCOA metrics #1659

thibaultmg · 2024-11-07T13:12:16Z

This PR initialises metrics collection for MCOA. More precisely, it:

Updates MCO CRD to support the metrics capability for both platform and user workload.
Adds customised variables to the addon configuration.
Adds permissions to MCOA for handling metrics resources.
Generates only the configs for the enabled signals to avoid having to update it for getting a valid addon status

Relates to the MCOA PR: stolostron/multicluster-observability-addon#77

Notes:

Referenced default configurations for metrics are currently generated by the addon manager. It might make for sense to migrate it in MCO in a subsequent PR.
The addon status is currently unhealthy because of the current HealthCHeck implementation on the addon manager which will soon get resolved (cf feat: update health probe to make use of healthChecker multicluster-observability-addon#94)

openshift-ci · 2024-11-07T13:12:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thibaultmg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [thibaultmg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Thibault Mange <[email protected]>

thibaultmg · 2024-11-27T17:04:44Z

/retest

Signed-off-by: Thibault Mange <[email protected]>

JoaoBraveCoding

Some thoughts

JoaoBraveCoding · 2024-11-29T10:38:37Z

operators/multiclusterobservability/pkg/rendering/renderer_mcoa.go

+				{
+					ConfigGroupResource: addonapiv1alpha1.ConfigGroupResource{
+						Group:    prometheusalpha1.SchemeGroupVersion.Group,
+						Resource: prometheusalpha1.ScrapeConfigName,
+					},
+					ConfigReferent: addonapiv1alpha1.ConfigReferent{
+						Name:      "platform-metrics-default",
+						Namespace: mcoconfig.GetDefaultNamespace(),
+					},
+				},
+				{
+					ConfigGroupResource: addonapiv1alpha1.ConfigGroupResource{
+						Group:    prometheusv1.SchemeGroupVersion.Group,
+						Resource: prometheusv1.PrometheusRuleName,
+					},
+					ConfigReferent: addonapiv1alpha1.ConfigReferent{
+						Name:      "platform-rules-default",
+						Namespace: mcoconfig.GetDefaultNamespace(),
+					},
+				},


Do you really want to reference these ones out of the get go?

My aim is to have it working out of the box for the base case. It lists here the metrics we need to collect for having the platform dashboards working. What are you thinking about?

I was thinking of not listing them here since they are defaults, here I feel it would only make sense to list extra metrics/rules the user would like to collect/have.

The issue is that we must let the user the ability to tune this list himself. As of now, he must be able to remove metrics from this list. However, the need for most or our users is to collect more metrics.
This is something I must discuss again with our PM. Maybe this will change later on.

I could probably remove the rules more easily however. I'll think about it

If there I a need I'm not opposed to having them here I was just double checking 👍

I'm leaving it like this for now. I might remove some of them after reviewing the list of needed metrics, in another PR.

operators/multiclusterobservability/pkg/rendering/renderer_mcoa.go

JoaoBraveCoding · 2024-11-29T11:36:17Z

operators/multiclusterobservability/api/v1beta2/multiclusterobservability_types.go

+// PlatformMetricsCollectionSpec defines the spec for the addon to collect and forward metrics
+// from fleet managed clusters.
+type PlatformMetricsCollectionSpec struct {
+	// Enabled defines a flag to enable/disable the platform metrics collection.
+	//
+	// +optional
+	// +kubebuilder:validation:Optional
+	Enabled bool `json:"enabled,omitempty"`
+}


In this initial version since metrics will be taking a managed approach to metric collection I wonder if these values are the best ones to use since the equivalent for logs & traces currently correlates to an unmanaged approach where the user fully controls the root resources. I would be in favor of deliberating a bit how we want these different scenarios to be exposed to users. This correlates with stolostron/multicluster-observability-addon#88 (comment) maybe a discussion for the nexus syncs

You mean that you expect different fields in the MCO CRD to have either managed or unmanaged deployments?
It would be very interesting to discuss those concepts in the next nexus sync.
On the metrics side, I am not sure there is demand for unmanaged type of deployments 🤷 . The expressed need is to replicate what we have now. We need something in between with some managed fields and some that we can leave it to the user for fine tuning.
Let's discuss all that in the nexus sync 👍

100% let's discuss on nexus sync 👍 just writing these down so we can go back to them:

if a signal is providing a turnkey solution then it will most likely be managed since that's the only way to reasonably offer support the storage side

maybe some use-cases only make sense to be managed (storage for instance, if you enable storage you are opting into the managed use case)

otherwise if we want to provide the unmanaged use-case for storage then we have to think how will it look like on the MCO CR

whatever we decide we should make sure that the CR feels similar across signals

Signed-off-by: Thibault Mange <[email protected]>

JoaoBraveCoding

Disregarding the discussion how the capabilities section will look like, I still have to manually test it but apart from that I would put it in lgtm territory

operators/multiclusterobservability/pkg/rendering/renderer_mcoa_test.go

JoaoBraveCoding · 2024-12-05T17:45:32Z

operators/multiclusterobservability/pkg/rendering/renderer_mcoa_test.go

+			baseCMA := &addonv1alpha1.ClusterManagementAddOn{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{
+						"k1": "v1",
+					},
+				},
+				Spec: addonv1alpha1.ClusterManagementAddOnSpec{
+					InstallStrategy: addonv1alpha1.InstallStrategy{
+						Placements: []addonv1alpha1.PlacementStrategy{
+							{
+								Configs: []addonv1alpha1.AddOnConfig{},
+							},
+						},
+					},
+				},
+			}
+			// to kustomize resource
+			baseCMAUnstructured, err := runtime.DefaultUnstructuredConverter.ToUnstructured(baseCMA)
+			assert.NoError(t, err)
+			kres := &kustomizeres.Resource{}
+			err = runtime.DefaultUnstructuredConverter.FromUnstructured(baseCMAUnstructured, kres)
+			assert.NoError(t, err)
+


Why do it this way VS the way we did it for TestRenderAddonDeploymentConfig?

What do you mean? I'm testing the ClusterManagementAddOn, not the AddOnDeploymentConfig.

JoaoBraveCoding · 2024-12-05T17:48:37Z

operators/multiclusterobservability/pkg/rendering/renderer_mcoa_test.go

+			dups := make(map[string]struct{})
+			for _, cfg := range cma.Spec.SupportedConfigs {
+				key := cfg.ConfigGroupResource.Group + "/" + cfg.ConfigGroupResource.Resource
+				if _, ok := dups[key]; ok {
+					t.Errorf("duplicated supportedConfigs %s", key)
+				}
+				dups[key] = struct{}{}
+			}


Why are we checking for duplicates? This is actually something that is supported by the addon-framework

The addon framework will fail at runtime, I'm just checking it earlier at unit-test time.

Signed-off-by: Thibault Mange <[email protected]>

sonarcloud · 2024-12-16T13:34:44Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
95.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

openshift-ci bot added the dco-signoff: yes label Nov 7, 2024

openshift-ci bot added the approved label Nov 7, 2024

thibaultmg added 12 commits November 22, 2024 14:08

update mco api

4b3763c

Signed-off-by: Thibault Mange <[email protected]>

add supported and default configs

7f72d35

Signed-off-by: Thibault Mange <[email protected]>

add customized variables

f36a552

Signed-off-by: Thibault Mange <[email protected]>

fix test

9e0f78f

Signed-off-by: Thibault Mange <[email protected]>

add metrics in mcoa enabled

b75d7ac

Signed-off-by: Thibault Mange <[email protected]>

fix crd name

bcb0d7b

Signed-off-by: Thibault Mange <[email protected]>

update mcoa role

4f10a31

Signed-off-by: Thibault Mange <[email protected]>

fix role

4ce02a2

Signed-off-by: Thibault Mange <[email protected]>

fix role typo

a579855

Signed-off-by: Thibault Mange <[email protected]>

add write perms for cfg resources

18bb470

Signed-off-by: Thibault Mange <[email protected]>

fix tests

aefd504

Signed-off-by: Thibault Mange <[email protected]>

remove default configs

413717f

Signed-off-by: Thibault Mange <[email protected]>

thibaultmg force-pushed the ACM_14962_init_mocoa_metrics branch from 553efbb to 413717f Compare November 22, 2024 13:09

thibaultmg added 6 commits November 25, 2024 11:50

dynamic supported configs

c1e846f

Signed-off-by: Thibault Mange <[email protected]>

fix

ed6bc29

Signed-off-by: Thibault Mange <[email protected]>

fix

037b48e

Signed-off-by: Thibault Mange <[email protected]>

fix

b3545a0

Signed-off-by: Thibault Mange <[email protected]>

fix supported configs

275a036

Signed-off-by: Thibault Mange <[email protected]>

revert dynamic supportedConfigs

217a160

Signed-off-by: Thibault Mange <[email protected]>

thibaultmg added 2 commits November 28, 2024 16:08

fix

0de6631

Signed-off-by: Thibault Mange <[email protected]>

only enabled configs

57e2104

Signed-off-by: Thibault Mange <[email protected]>

JoaoBraveCoding reviewed Nov 29, 2024

View reviewed changes

thibaultmg added 3 commits December 2, 2024 14:29

change metricsHub var

f84c657

Signed-off-by: Thibault Mange <[email protected]>

change MetricsHubHostname add logic

48cdd60

Signed-off-by: Thibault Mange <[email protected]>

fix MetricsHubHostname generation

c2c06b1

Signed-off-by: Thibault Mange <[email protected]>

JoaoBraveCoding reviewed Dec 5, 2024

View reviewed changes

simplify mcoa test

fd6f508

Signed-off-by: Thibault Mange <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ACM-14962: init MCOA metrics #1659

ACM-14962: init MCOA metrics #1659

thibaultmg commented Nov 7, 2024 •

edited

Loading

openshift-ci bot commented Nov 7, 2024

thibaultmg commented Nov 27, 2024

JoaoBraveCoding left a comment

JoaoBraveCoding Nov 29, 2024

thibaultmg Dec 2, 2024

JoaoBraveCoding Dec 2, 2024

thibaultmg Dec 2, 2024

thibaultmg Dec 2, 2024

JoaoBraveCoding Dec 2, 2024

thibaultmg Dec 2, 2024

JoaoBraveCoding Nov 29, 2024

thibaultmg Dec 2, 2024

JoaoBraveCoding Dec 2, 2024

JoaoBraveCoding left a comment

JoaoBraveCoding Dec 5, 2024

thibaultmg Dec 16, 2024

JoaoBraveCoding Dec 5, 2024

thibaultmg Dec 16, 2024

sonarcloud bot commented Dec 16, 2024

ACM-14962: init MCOA metrics #1659

Are you sure you want to change the base?

ACM-14962: init MCOA metrics #1659

Conversation

thibaultmg commented Nov 7, 2024 • edited Loading

openshift-ci bot commented Nov 7, 2024

thibaultmg commented Nov 27, 2024

JoaoBraveCoding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoaoBraveCoding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Dec 16, 2024

Quality Gate passed

thibaultmg commented Nov 7, 2024 •

edited

Loading