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

saswatamcode · 2024-12-10T10:13:05Z

Turns out there are a few issues with the behavior of the ObservabilityAddon CRD.

The CRD should operationally be doing the following,

Have a sane default for the options, all of which it will inherit from the MCO obsAddon spec at the start
Be per-cluster configurable, so that the user can modify the ObservabilityAddon CR in a specific spoke cluster namespace on the hub
Get copied over onto the spoke cluster and configure endpoint-operator
Allow user to switch back to MCO-based ObsservabilityAddon config if they wish to
All these changes are in sync in three places: ManifestWork on hub, spoke ns ObservabilityAddon CR on hub, ObservabilityAddon CR in the actual spoke cluster.

However it was not doing this correctly which led to a few issues and fields like the scrape limit or workers for metrics-collector just wouldn't work as well as couldn't be customized. So this PR addresses those issues. This PR,

Updates all copies of the relevant ObservabilityAddon CRD in the repo (in both mco and endpoint-operator), to include all of the options that have been added to it
Make sure Placementrule Controller reconciles the ObservabilityAddon CR between spoke ns and ManifestWork correctly on changes to fields and annotations as well as status changes.
Add an annotation to the ObservabilityAddon CR observability.open-cluster-management.io/addon-source during the creation of the ObservabilityAddon CR and maintain it for the lifecycle. It will have two accepted values "mco" and "override". When set to "mco" it will always reflect spec from MCO CR. If you try to change it, it will reconcile back to those values. When set to "override", we will not update the ObservabilityAddon CRD and will only use found values. This basically means user can set it to whatever they want, for a particular cluster
If user deletes ObservabilityAddon CR, it will revert back to default with "mco" addon-source again

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

philipgough

/lgtm

saswatamcode · 2024-12-10T10:41:49Z

/hold

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

saswatamcode · 2024-12-10T11:06:45Z

/retest

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

saswatamcode · 2024-12-12T10:32:04Z

/retest

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

operators/multiclusterobservability/controllers/placementrule/manifestwork.go

philipgough · 2024-12-12T11:14:16Z

operators/multiclusterobservability/controllers/placementrule/manifestwork.go

+		addon.Spec.Workers = found.Spec.Workers
+		addon.Spec.Resources = found.Spec.Resources
+
+		addon.Annotations[addonSourceAnnotation] = "override"


same comment as above? we wouldnt fall in here unless this k,v was already set?

This is a new copy, but yeah we can just copy over from found.

operators/multiclusterobservability/controllers/placementrule/obsaddon.go

operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go

thibaultmg

Looks good overall! It always surprises me that we discover those kind of bugs so late... Waiting for your replies

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

saswatamcode · 2024-12-15T12:21:30Z

/retest

saswatamcode · 2024-12-16T07:52:21Z

/retest

thibaultmg

/lgtm

saswatamcode · 2024-12-16T09:06:26Z

/retest

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

saswatamcode · 2024-12-16T12:40:05Z

/test test-e2e

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

sonarqubecloud · 2024-12-16T12:55:18Z

Quality Gate failed

Failed conditions
67.4% Coverage on New Code (required ≥ 70%)

See analysis details on SonarQube Cloud

openshift-ci · 2024-12-16T12:55:24Z

@saswatamcode: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/sonarcloud	`54401ec`	link	false	`/test sonarcloud`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

saswatamcode · 2024-12-16T13:57:42Z

/hold cancel

coleenquadros · 2024-12-16T14:00:28Z

/lgtm

openshift-ci · 2024-12-16T14:00:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: coleenquadros, philipgough, saswatamcode, 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~~ [coleenquadros,philipgough,saswatamcode,thibaultmg]

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

…tolostron#1718) * [ACM-16286]: Ensure MCO spec addon options are propagated correctly Signed-off-by: Saswata Mukherjee <[email protected]> * [ACM-16286]: Update all observabilityaddon manifests Signed-off-by: Saswata Mukherjee <[email protected]> * [ACM-16286]: Ensure hub and spoke obsaddon are synced based on spec Signed-off-by: Saswata Mukherjee <[email protected]> * [ACM-16286]: Allow customizing ObsAddon in spoke ns Signed-off-by: Saswata Mukherjee <[email protected]> * [ACM-16286]: Comments Signed-off-by: Saswata Mukherjee <[email protected]> * Add addon source annotation Signed-off-by: Saswata Mukherjee <[email protected]> * Add handling for empty map Signed-off-by: Saswata Mukherjee <[email protected]> * Add handling for nil maps Signed-off-by: Saswata Mukherjee <[email protected]> * Add annotation predicate, update in mco mode Signed-off-by: Saswata Mukherjee <[email protected]> * Fix unit test, make addon annotation constant Signed-off-by: Saswata Mukherjee <[email protected]> * Update comment Signed-off-by: Saswata Mukherjee <[email protected]> * Address comments Signed-off-by: Saswata Mukherjee <[email protected]> * Use semantic deepequal, add test for setter Signed-off-by: Saswata Mukherjee <[email protected]> * Set spoke addon correctly after comparison Signed-off-by: Saswata Mukherjee <[email protected]> --------- Signed-off-by: Saswata Mukherjee <[email protected]>

…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]>

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

7a32bb2

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

saswatamcode requested a review from coleenquadros December 10, 2024 10:13

openshift-ci bot added dco-signoff: yes approved labels Dec 10, 2024

philipgough approved these changes Dec 10, 2024

View reviewed changes

openshift-ci bot assigned philipgough Dec 10, 2024

openshift-ci bot added the lgtm label Dec 10, 2024

openshift-ci bot added the do-not-merge/hold label Dec 10, 2024

[ACM-16286]: Update all observabilityaddon manifests

607bb06

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

openshift-ci bot removed the lgtm label Dec 10, 2024

saswatamcode added 8 commits December 10, 2024 12:08

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

5ed10c5

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

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

605534c

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

[ACM-16286]: Comments

24423df

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

Add addon source annotation

d62f3c1

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

Add handling for empty map

242cfec

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

Add handling for nil maps

a9665f9

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

Add annotation predicate, update in mco mode

1e32f01

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

Fix unit test, make addon annotation constant

c9b609e

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

Update comment

1ac83e4

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

coleenquadros requested a review from subbarao-meduri December 12, 2024 11:09

philipgough reviewed Dec 12, 2024

View reviewed changes

operators/multiclusterobservability/controllers/placementrule/manifestwork.go Outdated Show resolved Hide resolved

philipgough reviewed Dec 12, 2024

View reviewed changes

operators/multiclusterobservability/controllers/placementrule/manifestwork.go Outdated Show resolved Hide resolved

philipgough reviewed Dec 12, 2024

View reviewed changes

operators/multiclusterobservability/controllers/placementrule/obsaddon.go Outdated Show resolved Hide resolved

thibaultmg reviewed Dec 13, 2024

View reviewed changes

operators/multiclusterobservability/controllers/placementrule/obsaddon.go Outdated Show resolved Hide resolved

thibaultmg reviewed Dec 13, 2024

View reviewed changes

operators/multiclusterobservability/controllers/placementrule/obsaddon.go Outdated Show resolved Hide resolved

thibaultmg reviewed Dec 13, 2024

View reviewed changes

operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go Outdated Show resolved Hide resolved

thibaultmg reviewed Dec 13, 2024

View reviewed changes

Address comments

fe15973

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

thibaultmg approved these changes Dec 16, 2024

View reviewed changes

openshift-ci bot assigned thibaultmg Dec 16, 2024

openshift-ci bot added the lgtm label Dec 16, 2024

Use semantic deepequal, add test for setter

20a306f

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

openshift-ci bot removed the lgtm label Dec 16, 2024

Set spoke addon correctly after comparison

54401ec

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

openshift-ci bot removed the do-not-merge/hold label Dec 16, 2024

coleenquadros approved these changes Dec 16, 2024

View reviewed changes

openshift-ci bot assigned coleenquadros Dec 16, 2024

openshift-ci bot added the lgtm label Dec 16, 2024

openshift-merge-bot bot merged commit 8755dbd into stolostron:main Dec 16, 2024
15 of 16 checks passed

saswatamcode mentioned this pull request Dec 16, 2024

[ACM-16286/backport 2.12]: Ensure MCO spec addon options are propagated correctly #1725

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

saswatamcode commented Dec 10, 2024 •

edited

Loading

philipgough left a comment

saswatamcode commented Dec 10, 2024

saswatamcode commented Dec 10, 2024

saswatamcode commented Dec 12, 2024

philipgough Dec 12, 2024

saswatamcode Dec 15, 2024

thibaultmg left a comment

saswatamcode commented Dec 15, 2024

saswatamcode commented Dec 16, 2024

thibaultmg left a comment

saswatamcode commented Dec 16, 2024

saswatamcode commented Dec 16, 2024

sonarqubecloud bot commented Dec 16, 2024

openshift-ci bot commented Dec 16, 2024 •

edited

Loading

saswatamcode commented Dec 16, 2024

coleenquadros commented Dec 16, 2024

openshift-ci bot commented Dec 16, 2024

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

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

Conversation

saswatamcode commented Dec 10, 2024 • edited Loading

philipgough left a comment

Choose a reason for hiding this comment

saswatamcode commented Dec 10, 2024

saswatamcode commented Dec 10, 2024

saswatamcode commented Dec 12, 2024

philipgough Dec 12, 2024

Choose a reason for hiding this comment

saswatamcode Dec 15, 2024

Choose a reason for hiding this comment

thibaultmg left a comment

Choose a reason for hiding this comment

saswatamcode commented Dec 15, 2024

saswatamcode commented Dec 16, 2024

thibaultmg left a comment

Choose a reason for hiding this comment

saswatamcode commented Dec 16, 2024

saswatamcode commented Dec 16, 2024

sonarqubecloud bot commented Dec 16, 2024

Quality Gate failed

openshift-ci bot commented Dec 16, 2024 • edited Loading

saswatamcode commented Dec 16, 2024

coleenquadros commented Dec 16, 2024

openshift-ci bot commented Dec 16, 2024

saswatamcode commented Dec 10, 2024 •

edited

Loading

openshift-ci bot commented Dec 16, 2024 •

edited

Loading