MON-4609 - Add configurationSource selector to ClusterMonitoring API. - #2938
MON-4609 - Add configurationSource selector to ClusterMonitoring API.#2938marioferh wants to merge 2 commits into
Conversation
Allow choosing between the cluster-monitoring-config ConfigMap and the ClusterMonitoring CRD as the configuration source, defaulting to CRD. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Hello @marioferh! Some important instructions when contributing to openshift/api: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds an optional Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/test api-review |
|
PR-Agent: could not find a component named |
|
I've reviewed all the changes in this diff. Let me verify my analysis is complete by checking the new field and type against all 7 rules: New
New No issues found. The new
|
openshift-api-review-bot
Bot
added
ai/approved
| // 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"` |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Cluster admins create it (opt-in). if missing, it falls back to ConfigMap defaults.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.yamlwould 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?
|
/test verify-hypershift-integration |
|
PR-Agent: could not find a component named |
|
@marioferh: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
5 participants
Allow choosing between the cluster-monitoring-config ConfigMap and the ClusterMonitoring CRD as the configuration source, defaulting to CRD.