MON-4343: Cleanup deprecate pa config #2651

jan--f · 2025-08-26T13:54:43Z

I added CHANGELOG entry for this change.
No user facing changes, so no entry in CHANGELOG was needed.

jan--f · 2025-08-26T13:55:01Z

The final bit of #2648

machine424

thanks, some suggestions

machine424 · 2025-08-26T14:13:27Z

pkg/manifests/config.go

+
+	var warning *InvalidConfigWarning
+	wCmc := defaultClusterMonitoringConfiguration()
+	wErr := UnmarshalStrict(content, &wCmc)


no need to propagate warnings for this.
we already capture the error below in err := UnmarshalStrict(content, &cmc)

hmm I see, I suppose I didn't understand #2592 fully.

in #2592, we had 2 different unmarshallers, we propagated the failures of the newer/stricter one as warnings only to block upgrades so the configs are fixed.

now the newer/stricter one is the default/only one and we can (should) propagate its failures as errors.

machine424 · 2025-08-26T14:14:11Z

pkg/manifests/config.go

+
+	var warning *InvalidConfigWarning
+	wU := &UserWorkloadConfiguration{}
+	wErr := UnmarshalStrict([]byte(content), &wU)


same here, having err := UnmarshalStrict([]byte(content), &u) below is enough, no need to have it as a warning as well.

machine424 · 2025-08-26T14:15:00Z

pkg/manifests/config_test.go

@@ -265,7 +296,12 @@ thanosRuler:

 	for _, tc := range tcs {
 		t.Run(tc.name, func(t *testing.T) {
-			c, err := NewUserConfigFromString(tc.configString())
+			c, warning, err := NewUserConfigFromString(tc.configString())


I think that warnings in all tests that don't have a promadapter field set should nil

machine424 · 2025-08-26T14:16:11Z

test/e2e/config_test.go

 	f.AssertOperatorConditionReason(configv1.OperatorDegraded, "InvalidConfiguration")
 	f.AssertOperatorConditionReason(configv1.OperatorAvailable, "InvalidConfiguration")
 	// Check that the previous setup hasn't been reverted
 	f.AssertStatefulsetExists("prometheus-user-workload", f.UserWorkloadMonitoringNs)(t)

+	t.Log("invalid configuration with field of the wrong case")


no need have those as we don't want to propagate the same unmarshalling errors as warnings, see above.

machine424 · 2025-08-26T14:20:50Z

test/e2e/config_test.go

 	// Restore the first configuration.
 	f.MustCreateOrUpdateConfigMap(t, getUserWorkloadEnabledConfigMap(t, f))
 	t.Log("asserting that CMO goes back healthy after the configuration is fixed")
 	f.AssertOperatorCondition(configv1.OperatorDegraded, configv1.ConditionFalse)(t)
 	f.AssertOperatorCondition(configv1.OperatorAvailable, configv1.ConditionTrue)(t)
+	// Once the config is adjusted, the operator becomes Upgradeable.


we could keep this and replace it with a config with promAdapter set; check it goes upgreadable=false when set and true when removed.

I see you added those checks in TestClusterMonitoringDeprecatedConfig

machine424 · 2025-08-26T14:23:02Z

test/e2e/user_workload_monitoring_test.go

 	f.AssertOperatorCondition(configv1.OperatorDegraded, configv1.ConditionTrue)(t)
 	f.AssertOperatorCondition(configv1.OperatorAvailable, configv1.ConditionFalse)(t)
 	f.AssertOperatorConditionReason(configv1.OperatorDegraded, "UserWorkloadInvalidConfiguration")
 	f.AssertOperatorConditionReason(configv1.OperatorAvailable, "UserWorkloadInvalidConfiguration")
+	// Even when an invalid configuration is caught by both unmarshallers, the operator is still set


no longer needed, we just have one unmarshaller now and it only yields errors, no need to have warnings for it. see above.

machine424 · 2025-08-28T08:50:44Z

pkg/manifests/config.go

 	if c.ClusterMonitoringConfiguration.PrometheusK8sConfig.CollectionProfile != FullCollectionProfile && !c.CollectionProfilesFeatureGateEnabled {
-		return fmt.Errorf("%w: collectionProfiles is currently a TechPreview feature behind the \"MetricsCollectionProfiles\" feature-gate, to be able to use a profile different from the default (\"full\") please enable it first", ErrConfigValidation)
+		return nil, fmt.Errorf("%w: collectionProfiles is currently a TechPreview feature behind the \"MetricsCollectionProfiles\" feature-gate, to be able to use a profile different from the default (\"full\") please enable it first", ErrConfigValidation)


not related to the PR: I think collectionProfile is GA in 4.19 and I think this logic can be cleaned now
cc @rexagod (I can create a ticket if needed)

machine424 · 2025-08-28T08:52:49Z

pkg/manifests/config_test.go

@@ -791,7 +798,7 @@ func TestCollectionProfilePreCheck(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			c, err := NewConfigFromString(tc.config, true)
 			require.NoError(t, err)
-			err = c.Precheck()
+			_, err = c.Precheck()


nit: we can set the warning and ensure it's nil, an easy extra check is always welcome.

machine424 · 2025-08-28T08:57:32Z

test/e2e/config_test.go

@@ -71,6 +71,9 @@ func TestClusterMonitoringOperatorConfiguration(t *testing.T) {
 	t.Log("asserting that CMO goes degraded after an invalid configuration is pushed")
 	f.AssertOperatorCondition(configv1.OperatorDegraded, configv1.ConditionTrue)(t)
 	f.AssertOperatorCondition(configv1.OperatorAvailable, configv1.ConditionFalse)(t)
+	// Even when an invalid configuration is caught by both unmarshallers, the operator is still set


leftover? also I don't think we do/need to make Upgradeable=false, no for such case.

Yeah I see what you mean...thanks for your patience 😓

machine424 · 2025-08-28T08:59:34Z

test/e2e/user_workload_monitoring_test.go

+	t.Log("restoring the initial configurations")
+	uwmCM.Data["config.yaml"] = ``
+	f.MustCreateOrUpdateConfigMap(t, uwmCM)
+	f.MustCreateOrUpdateConfigMap(t, getUserWorkloadEnabledConfigMap(t, f))


nit: do we need this f.MustCreateOrUpdateConfigMap(t, getUserWorkloadEnabledConfigMap(t, f)) line?

machine424 · 2025-08-28T09:00:50Z

test/e2e/user_workload_monitoring_test.go

+	f.MustCreateOrUpdateConfigMap(t, getUserWorkloadEnabledConfigMap(t, f))
+	f.AssertOperatorCondition(configv1.OperatorDegraded, configv1.ConditionFalse)(t)
+	f.AssertOperatorCondition(configv1.OperatorAvailable, configv1.ConditionTrue)(t)
+	// Once the config is adjusted, the operator becomes Upgradeable.


nit: I think the operator should always stay upgradeable for cannot be deserialized

jan--f · 2025-08-29T06:36:31Z

/retest

machine424 · 2025-08-29T11:56:53Z

/lgtm
thanks!

… monitoring and set CMO to Upgradeable=false if errors are found The strict unmarshaller is currently in advisory mode. Setting CMO to Upgradeable=false ensures that configurations meet the new unmarshaller checks in 4.18 before it is set to be the default in 4.19. (cherry picked from commit 8e7bb77)

…config Signed-off-by: Jan Fajerski <[email protected]>

Signed-off-by: Jan Fajerski <[email protected]>

jan--f · 2025-08-29T12:25:22Z

rebased. @machine424 requires your lgtm again.

machine424 · 2025-08-29T12:51:10Z

/lgtm

openshift-ci · 2025-08-29T12:51:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jan--f, machine424

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~~ [jan--f,machine424]

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

jan--f · 2025-09-01T08:05:38Z

/label acknowledge-critical-fixes-only

jan--f · 2025-09-01T08:32:30Z

/retitle MON-4343: Cleanup deprecate pa config

openshift-ci-robot · 2025-09-01T08:32:46Z

@jan--f: This pull request references MON-4343 which is a valid jira issue.

In response to this:

I added CHANGELOG entry for this change.

No user facing changes, so no entry in CHANGELOG was needed.

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2025-09-01T11:42:41Z

@jan--f: The following tests 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/okd-scos-e2e-aws-ovn	`4370de4`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/versions	`4370de4`	link	false	`/test versions`

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.

openshift-ci-robot · 2025-09-01T11:45:59Z

/retest-required

Remaining retests: 0 against base HEAD a5a091b and 2 for PR HEAD 4370de4 in total

dgoodwin · 2025-09-02T14:21:13Z

Apologies but this took out payloads due to ROSA use of the deprecated config: https://issues.redhat.com/browse/OCPBUGS-61135

Per org policy I need to pursue a revert.

jan--f requested a review from machine424 August 26, 2025 13:55

openshift-ci bot requested review from danielmellado and rexagod August 26, 2025 13:56

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 26, 2025

machine424 reviewed Aug 26, 2025

View reviewed changes

jan--f force-pushed the cleanup-deprecate-pa-config branch 2 times, most recently from 2ddd33a to 07f5388 Compare August 28, 2025 07:56

machine424 reviewed Aug 28, 2025

View reviewed changes

jan--f force-pushed the cleanup-deprecate-pa-config branch from 07f5388 to ce0b4a1 Compare August 28, 2025 13:32

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2025

openshift-ci bot assigned machine424 Aug 29, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 29, 2025

machine424 and others added 3 commits August 29, 2025 14:14

chore: set upgradeable=false if prometheusAdapter config apprears in …

d7db970

…config Signed-off-by: Jan Fajerski <[email protected]>

test: assert that operator is not upgradeable with deprecated config

4370de4

Signed-off-by: Jan Fajerski <[email protected]>

jan--f force-pushed the cleanup-deprecate-pa-config branch from ce0b4a1 to 4370de4 Compare August 29, 2025 12:25

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 29, 2025

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 29, 2025

openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label Sep 1, 2025

openshift-ci bot changed the title ~~Cleanup deprecate pa config~~ MON-4343: Cleanup deprecate pa config Sep 1, 2025

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Sep 1, 2025

openshift-merge-bot bot merged commit 92f7f83 into openshift:main Sep 1, 2025
19 of 21 checks passed

dgoodwin mentioned this pull request Sep 2, 2025

OCPBUGS-61135: Revert "MON-4343: Cleanup deprecate pa config" #2658

Merged

jan--f mentioned this pull request Sep 4, 2025

Remove Prometheus adapter #2601

Open

2 tasks

MON-4343: Cleanup deprecate pa config #2651

MON-4343: Cleanup deprecate pa config #2651

Uh oh!

Conversation

jan--f commented Aug 26, 2025

Uh oh!

jan--f commented Aug 26, 2025

Uh oh!

machine424 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

machine424 Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jan--f commented Aug 29, 2025

Uh oh!

machine424 commented Aug 29, 2025

Uh oh!

jan--f commented Aug 29, 2025

Uh oh!

machine424 commented Aug 29, 2025

Uh oh!

openshift-ci bot commented Aug 29, 2025

Uh oh!

jan--f commented Sep 1, 2025

Uh oh!

jan--f commented Sep 1, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Sep 1, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Sep 1, 2025

Uh oh!

Uh oh!

dgoodwin commented Sep 2, 2025

Uh oh!

Uh oh!

machine424 Aug 27, 2025 •

edited

Loading

jan--f commented Sep 1, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Sep 1, 2025 •

edited by openshift-ci bot

Loading

openshift-ci bot commented Sep 1, 2025 •

edited

Loading