Skip to content

MON-4609 - Add configurationSource selector to ClusterMonitoring API. #2938

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
marioferh wants to merge 2 commits into
base: master
Choose a base branch
from
Open

MON-4609 - Add configurationSource selector to ClusterMonitoring API. #2938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
96752c7
Add configurationSource selector to ClusterMonitoring API.
marioferh Jul 17, 2026
37cb690
fix lint
marioferh Jul 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,35 @@ tests:
spec:
userDefined:
mode: "Disabled"
- name: Should accept configurationSource ConfigMap
initial: |
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
spec:
configurationSource: ConfigMap
expected: |
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
spec:
configurationSource: ConfigMap
- name: Should accept configurationSource CRD
initial: |
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
spec:
configurationSource: CRD
expected: |
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
spec:
configurationSource: CRD
- name: Should reject invalid configurationSource
initial: |
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
spec:
configurationSource: Invalid
expectedError: 'spec.configurationSource: Unsupported value: "Invalid": supported values: "ConfigMap", "CRD"'
- name: Should accept userAlertmanagerConfigSelection on alertmanagerConfig customConfig
initial: |
apiVersion: config.openshift.io/v1alpha1
Expand Down
25 changes: 25 additions & 0 deletions config/v1alpha1/types_cluster_monitoring.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,17 @@ type ClusterMonitoringList struct {
// ClusterMonitoringSpec defines the desired state of Cluster Monitoring Operator
// +kubebuilder:validation:MinProperties=1
type ClusterMonitoringSpec struct {
// configurationSource is an optional field that selects whether the Cluster Monitoring Operator
// reads its configuration from the cluster-monitoring-config ConfigMap or from this ClusterMonitoring CRD.
// Valid values are "ConfigMap" and "CRD".
// When set to ConfigMap, the operator uses the cluster-monitoring-config ConfigMap in the
// openshift-monitoring namespace as the configuration source.
// When set to CRD, the operator uses this ClusterMonitoring custom resource as the configuration source.
// When omitted, this means no opinion and the platform is left to choose a reasonable default,
// which is subject to change over time.
// The current default value is CRD.
// +optional
ConfigurationSource ConfigurationSource `json:"configurationSource,omitempty"`
Comment on lines +79 to +89

@everettraven everettraven Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a little while since I've reviewed something on this API - could you remind me who is intended to create this resource initially?

Is it an end-user that creates an instance of this resource or will this follow a similar pattern to other config resources where the platform ships with a default instance of this resource named cluster?

@marioferh marioferh Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cluster admins create it (opt-in). if missing, it falls back to ConfigMap defaults.

@everettraven everettraven Jul 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, what value does adding this field provide?

Could we just use the existence of the CR having been created by an admin as a signal that they would like to use the CRD over the ConfigMap?

@danielmellado danielmellado Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, and while I understand your comment we'd need to have a boolean-like value to check this in the inplementation. If you check the proposal you'll see that in the final phase this will be tombstoned as well. Currently it wouldn't be straightforward to follow your advice, but thanks!

@everettraven everettraven Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check in the implementation would essentially be:

  • IF CR exists, do CRD-based logic
  • ELSE do ConfigMap-based logic

I'm not sure I understand why we need a knob on the CRD to use the configmap or the CRD when we can use existence as the "preference" knob.

I'd prefer we don't add an API field that we intend to remove in the future as that is a breaking API change that should only be done if absolutely necessary.

Currently it wouldn't be straightforward to follow your advice, but thanks!

What makes this not straightforward to follow?

@danielmellado danielmellado Aug 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, and sorry for maybe being vague earlier. Let me try to be more concrete.

The main reason we can't just rely on CR existence is rollback safety during the transition. Think of a cluster admin who's been building out their monitoring config in the CRD - prometheus retention, alertmanager, remote write, the works.

If something goes sideways, with existence-based detection their only option is to nuke the entire CR to fall back to ConfigMap, losing all that config. With configurationSource, they just flip it to ConfigMap and they're back on the old path while their CRD config stays put for when they're ready to try again. Given how many knobs ClusterMonitoring has, that matters.

There's also a practical wrinkle: the spec has MinProperties=1, so you can't have a CR with an empty spec. If someone just wants to say "use CRD mode" without tweaking anything else, they'd need to set some arbitrary field. configurationSource gives them a clean way to do that.

FWIW, this is what Phase 2 of the enhancement lays out "Allow users to choose the source of configuration" - and it was agreed upon with the api-approvers at the time.

On the removal concern, yeah! fair enough, we don't want to ship something we'll yank later. I think the cleanest option is to just keep the field once ConfigMap is deprecated. It'd only accept CRD at that point (or become a no-op). No breaking change needed. And since we're still in v1alpha1 behind ClusterMonitoringConfig FeatureGate, we have room to iterate before GA regardless.

What do you think? Thanks!

@simonpasquier simonpasquier Aug 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll throw another option into the discussion: what about an annotation on the configmap which would let a cluster admin control which takes precedence?

Given that almost all clusters have an existing CMO configmap, we can bet that despite all the testing, there will a few cases where the migration won't be smooth. Which is why we want an escape hatch in case things go wrong.

@everettraven everettraven Aug 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason we can't just rely on CR existence is rollback safety during the transition. Think of a cluster admin who's been building out their monitoring config in the CRD - prometheus retention, alertmanager, remote write, the works.

If something goes sideways, with existence-based detection their only option is to nuke the entire CR to fall back to ConfigMap, losing all that config. With configurationSource, they just flip it to ConfigMap and they're back on the old path while their CRD config stays put for when they're ready to try again. Given how many knobs ClusterMonitoring has, that matters.

Rollback safety here is to save a backup of your configuration before you delete the CR no? Something like:

kubectl get clustermonitoring/cluster -o yaml > clustermonitoring-backup.yaml

would suffice for a temporary backup I would think.

There's also a practical wrinkle: the spec has MinProperties=1, so you can't have a CR with an empty spec. If someone just wants to say "use CRD mode" without tweaking anything else, they'd need to set some arbitrary field. configurationSource gives them a clean way to do that

What does "use CRD mode" mean if they don't specify any configuration? Is that any different to them not creating a CR and not creating a ConfigMap?

FWIW, this is what Phase 2 of the enhancement lays out "Allow users to choose the source of configuration" - and it was agreed upon with the api-approvers at the time.

That section of the enhancement, at least to me, isn't prescriptive as to the approach. I think that the approach I'm suggesting is still within the spirit of that phase of the enhancement. Users are able to choose whether they want to use the configmap or the CRD-based configuration by deciding to create (or not) the CR.

I'll throw another option into the discussion: what about an annotation on the configmap which would let a cluster admin control which takes precedence?

Given that almost all clusters have an existing CMO configmap, we can bet that despite all the testing, there will a few cases where the migration won't be smooth. Which is why we want an escape hatch in case things go wrong.

Don't use annotations as an API. I'm still not sure I'm following why the precedence behavior and backup + deletion of the CR isn't a sufficient escape hatch here?

// userDefined set the deployment mode for user-defined monitoring in addition to the default platform monitoring.
// userDefined is optional.
// When omitted, this means no opinion and the platform is left to choose a reasonable default, which is subject to change over time.
Expand Down Expand Up @@ -757,6 +768,20 @@ type MonitoringPluginConfig struct {
TopologySpreadConstraints []v1.TopologySpreadConstraint `json:"topologySpreadConstraints,omitempty"`
}

// ConfigurationSource selects the configuration source for the Cluster Monitoring Operator.
// +kubebuilder:validation:Enum=ConfigMap;CRD
// +enum
type ConfigurationSource string

const (
// ConfigurationSourceConfigMap means the operator reads configuration from the
// cluster-monitoring-config ConfigMap in the openshift-monitoring namespace.
ConfigurationSourceConfigMap ConfigurationSource = "ConfigMap"
// ConfigurationSourceCRD means the operator reads configuration from this
// ClusterMonitoring custom resource.
ConfigurationSourceCRD ConfigurationSource = "CRD"
)

// UserDefinedMonitoring config for user-defined projects.
type UserDefinedMonitoring struct {
// mode defines the different configurations of UserDefinedMonitoring
Expand Down
15 changes: 15 additions & 0 deletions ...1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -915,6 +915,21 @@ spec:
and forbidden otherwise
rule: 'self.deploymentMode == ''CustomConfig'' ? has(self.customConfig)
: !has(self.customConfig)'
configurationSource:
description: |-
configurationSource is an optional field that selects whether the Cluster Monitoring Operator
reads its configuration from the cluster-monitoring-config ConfigMap or from this ClusterMonitoring CRD.
Valid values are "ConfigMap" and "CRD".
When set to ConfigMap, the operator uses the cluster-monitoring-config ConfigMap in the
openshift-monitoring namespace as the configuration source.
When set to CRD, the operator uses this ClusterMonitoring custom resource as the configuration source.
When omitted, this means no opinion and the platform is left to choose a reasonable default,
which is subject to change over time.
The current default value is CRD.
enum:
- ConfigMap
- CRD
type: string
kubeStateMetricsConfig:
description: |-
kubeStateMetricsConfig is an optional field that can be used to configure the kube-state-metrics
Expand Down
15 changes: 15 additions & 0 deletions ...regated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -915,6 +915,21 @@ spec:
and forbidden otherwise
rule: 'self.deploymentMode == ''CustomConfig'' ? has(self.customConfig)
: !has(self.customConfig)'
configurationSource:
description: |-
configurationSource is an optional field that selects whether the Cluster Monitoring Operator
reads its configuration from the cluster-monitoring-config ConfigMap or from this ClusterMonitoring CRD.
Valid values are "ConfigMap" and "CRD".
When set to ConfigMap, the operator uses the cluster-monitoring-config ConfigMap in the
openshift-monitoring namespace as the configuration source.
When set to CRD, the operator uses this ClusterMonitoring custom resource as the configuration source.
When omitted, this means no opinion and the platform is left to choose a reasonable default,
which is subject to change over time.
The current default value is CRD.
enum:
- ConfigMap
- CRD
type: string
kubeStateMetricsConfig:
description: |-
kubeStateMetricsConfig is an optional field that can be used to configure the kube-state-metrics
Expand Down
1 change: 1 addition & 0 deletions config/v1alpha1/zz_generated.swagger_doc_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions openapi/generated_openapi/zz_generated.openapi.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions openapi/openapi.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13110,6 +13110,14 @@
"default": {},
"$ref": "#/definitions/com.github.openshift.api.config.v1alpha1.AlertmanagerConfig"
},
"configurationSource": {
"description": "configurationSource is an optional field that selects whether the Cluster Monitoring Operator reads its configuration from the cluster-monitoring-config ConfigMap or from this ClusterMonitoring CRD. Valid values are \"ConfigMap\" and \"CRD\". When set to ConfigMap, the operator uses the cluster-monitoring-config ConfigMap in the openshift-monitoring namespace as the configuration source. When set to CRD, the operator uses this ClusterMonitoring custom resource as the configuration source. When omitted, this means no opinion and the platform is left to choose a reasonable default, which is subject to change over time. The current default value is CRD.\n\nPossible enum values:\n - `\"CRD\"` means the operator reads configuration from this ClusterMonitoring custom resource.\n - `\"ConfigMap\"` means the operator reads configuration from the cluster-monitoring-config ConfigMap in the openshift-monitoring namespace.",
"type": "string",
"enum": [
"CRD",
"ConfigMap"
]
},
"kubeStateMetricsConfig": {
"description": "kubeStateMetricsConfig is an optional field that can be used to configure the kube-state-metrics agent that runs in the openshift-monitoring namespace. kube-state-metrics generates metrics about the state of Kubernetes objects such as Deployments, Nodes, and Pods. When omitted, this means no opinion and the platform is left to choose a reasonable default, which is subject to change over time.",
"default": {},
Expand Down
15 changes: 15 additions & 0 deletions payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -915,6 +915,21 @@ spec:
and forbidden otherwise
rule: 'self.deploymentMode == ''CustomConfig'' ? has(self.customConfig)
: !has(self.customConfig)'
configurationSource:
description: |-
configurationSource is an optional field that selects whether the Cluster Monitoring Operator
reads its configuration from the cluster-monitoring-config ConfigMap or from this ClusterMonitoring CRD.
Valid values are "ConfigMap" and "CRD".
When set to ConfigMap, the operator uses the cluster-monitoring-config ConfigMap in the
openshift-monitoring namespace as the configuration source.
When set to CRD, the operator uses this ClusterMonitoring custom resource as the configuration source.
When omitted, this means no opinion and the platform is left to choose a reasonable default,
which is subject to change over time.
The current default value is CRD.
enum:
- ConfigMap
- CRD
type: string
kubeStateMetricsConfig:
description: |-
kubeStateMetricsConfig is an optional field that can be used to configure the kube-state-metrics
Expand Down