MON-4037: Add MonitoringPluginConfig to ClusterMonitoring API#2768
MON-4037: Add MonitoringPluginConfig to ClusterMonitoring API#2768danielmellado wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@danielmellado: This pull request references MON-4037 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.22." or "openshift-4.22.", but it targets "openshift-5.0" instead. DetailsIn response to this: 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. |
|
Hello @danielmellado! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XXL
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a new optional ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/v1alpha1/types_cluster_monitoring.go`:
- Around line 243-247: The Resources slice on the monitoring plugin config
currently only enforces MaxItems=10 (defined as the Resources
[]ContainerResource field) which allows an empty list; add a kubebuilder
validation tag to require at least one entry by adding
+kubebuilder:validation:MinItems=1 to the Resources field declaration so
monitoringPluginConfig.resources cannot be an empty array (update the Resources
[]ContainerResource json:"resources,omitempty,omitzero" field in
types_cluster_monitoring.go).
In
`@payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml`:
- Around line 1293-1294: The CRD currently allows an empty array for the
"resources" property even though the object requires non-empty content; update
the schema for the "resources" array(s) to include minItems: 1 (in addition to
the existing minProperties rule) so that resources: [] is rejected, and apply
the same change to all analogous "resources" schema blocks in this CRD to ensure
at least one item is required when the array is present.
- Around line 1614-1617: The CRD shows topologySpreadConstraints for
MonitoringPluginConfig using atomic list semantics; change the source markers
for MonitoringPluginConfig.TopologySpreadConstraints to use map-list semantics
by adding the kubebuilder markers +listType=map and two +listMapKey entries
(+listMapKey=topologyKey and +listMapKey=whenUnsatisfiable) so the generated CRD
replaces x-kubernetes-list-type: atomic with a keyed map list like its sibling
monitoring configs. Locate the MonitoringPluginConfig struct and update the
field tags/markers for TopologySpreadConstraints accordingly, then regenerate
the CRD.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 4854f825-5c11-4ca8-bb89-52abfc00b13e
⛔ Files ignored due to path filters (3)
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1alpha1/zz_generated.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**
📒 Files selected for processing (5)
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlconfig/v1alpha1/types_cluster_monitoring.goconfig/v1alpha1/zz_generated.deepcopy.goconfig/v1alpha1/zz_generated.swagger_doc_generated.gopayload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml
payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml
Show resolved
Hide resolved
payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml
Outdated
Show resolved
Hide resolved
danielmellado
force-pushed
the
mon_4037_monitoring_plugin_config
branch
from
7590acb to
30b1be2
Compare
|
@everettraven coming ere from #2753, I addressed your comments, as there was an issue wit GH and the branch and the PR was closed, thanks! |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml (1)
1619-1622:⚠️ Potential issue | 🟠 Major
topologySpreadConstraintsstill needs map-list semantics.Line 1622 keeps this list atomic, so the uniqueness promise in Lines 1446-1447 is not enforced and server-side apply behavior diverges from the sibling monitoring configs. Please add
+listType=map,+listMapKey=topologyKey, and+listMapKey=whenUnsatisfiableonMonitoringPluginConfig.TopologySpreadConstraints, then regenerate the CRD.Expected generated shape
maxItems: 10 minItems: 1 type: array - x-kubernetes-list-type: atomic + x-kubernetes-list-map-keys: + - topologyKey + - whenUnsatisfiable + x-kubernetes-list-type: mapVerify that
metricsServerConfig.topologySpreadConstraintsremains a keyed map list,monitoringPluginConfig.topologySpreadConstraintsstops generating as atomic, and the source struct carries the matching kubebuilder markers.#!/bin/bash set -euo pipefail crd='payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml' types_file="$(fd 'types_cluster_monitoring\.go$' | head -n1)" test -n "$types_file" echo '== Generated CRD list semantics: metricsServerConfig vs monitoringPluginConfig ==' python - <<'PY' from pathlib import Path lines = Path("payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml").read_text().splitlines() def show(section, next_section): start = next(i for i, l in enumerate(lines) if l.strip() == f"{section}:") end = next(i for i, l in enumerate(lines[start + 1:], start + 1) if l.strip() == f"{next_section}:") inside = False print(f"-- {section} --") for i in range(start, end): s = lines[i].strip() if s == "topologySpreadConstraints:": inside = True if not inside: continue if s in { "topologySpreadConstraints:", "maxItems: 10", "minItems: 1", "type: array", "x-kubernetes-list-map-keys:", "- topologyKey", "- whenUnsatisfiable", "x-kubernetes-list-type: atomic", "x-kubernetes-list-type: map", }: print(f"{i+1}:{lines[i]}") if s in {"x-kubernetes-list-type: atomic", "x-kubernetes-list-type: map"}: break show("metricsServerConfig", "monitoringPluginConfig") show("monitoringPluginConfig", "openShiftStateMetricsConfig") PY echo echo '== Source markers for MonitoringPluginConfig ==' awk ' /^type MonitoringPluginConfig struct/ {inblock=1} inblock {print NR ":" $0} inblock && /^}/ {exit} ' "$types_file"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml` around lines 1619 - 1622, The CRD shows MonitoringPluginConfig.topologySpreadConstraints as an atomic list; update the source struct (MonitoringPluginConfig) field TopologySpreadConstraints to carry the kubebuilder markers +listType=map and two +listMapKey entries (+listMapKey=topologyKey and +listMapKey=whenUnsatisfiable), then regenerate the CRD so monitoringPluginConfig.topologySpreadConstraints emits x-kubernetes-list-type: map and x-kubernetes-list-map-keys matching metricsServerConfig.topologySpreadConstraints.
🧹 Nitpick comments (1)
config/v1alpha1/types_cluster_monitoring.go (1)
274-277: ChangeMonitoringPluginConfig.TopologySpreadConstraintsto map-style list semantics for API consistency.
MonitoringPluginConfig.TopologySpreadConstraints(lines 274–277) uses+listType=atomic, while all otherTopologySpreadConstraintsfields in this file (OpenShiftStateMetricsConfig,AlertmanagerConfig,MetricsServerConfig,PrometheusOperatorConfig,PrometheusOperatorAdmissionWebhookConfig,PrometheusConfig) use+listType=mapkeyed bytopologyKeyandwhenUnsatisfiable. Aligning this field with map semantics would improve consistency across the API surface and provide better duplicate handling based on the naturally identifying fields ofTopologySpreadConstraint.Suggested change
- // +listType=atomic + // +listType=map + // +listMapKey=topologyKey + // +listMapKey=whenUnsatisfiable // +kubebuilder:validation:MinItems=1 // +kubebuilder:validation:MaxItems=10 TopologySpreadConstraints []v1.TopologySpreadConstraint `json:"topologySpreadConstraints,omitempty"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1alpha1/types_cluster_monitoring.go` around lines 274 - 277, Change the list semantics for MonitoringPluginConfig.TopologySpreadConstraints from atomic to map to match the other types: replace the existing "+listType=atomic" annotation with "+listType=map" and add a map key annotation listing the natural identity fields (e.g. "+listMapKey=topologyKey,whenUnsatisfiable") so duplicates are detected by those keys; update the comment block above the TopologySpreadConstraints field in MonitoringPluginConfig to mirror the annotations used on OpenShiftStateMetricsConfig/AlertmanagerConfig/MetricsServerConfig/PrometheusOperatorConfig/PrometheusOperatorAdmissionWebhookConfig/PrometheusConfig.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml`:
- Around line 1619-1622: The CRD shows
MonitoringPluginConfig.topologySpreadConstraints as an atomic list; update the
source struct (MonitoringPluginConfig) field TopologySpreadConstraints to carry
the kubebuilder markers +listType=map and two +listMapKey entries
(+listMapKey=topologyKey and +listMapKey=whenUnsatisfiable), then regenerate the
CRD so monitoringPluginConfig.topologySpreadConstraints emits
x-kubernetes-list-type: map and x-kubernetes-list-map-keys matching
metricsServerConfig.topologySpreadConstraints.
---
Nitpick comments:
In `@config/v1alpha1/types_cluster_monitoring.go`:
- Around line 274-277: Change the list semantics for
MonitoringPluginConfig.TopologySpreadConstraints from atomic to map to match the
other types: replace the existing "+listType=atomic" annotation with
"+listType=map" and add a map key annotation listing the natural identity fields
(e.g. "+listMapKey=topologyKey,whenUnsatisfiable") so duplicates are detected by
those keys; update the comment block above the TopologySpreadConstraints field
in MonitoringPluginConfig to mirror the annotations used on
OpenShiftStateMetricsConfig/AlertmanagerConfig/MetricsServerConfig/PrometheusOperatorConfig/PrometheusOperatorAdmissionWebhookConfig/PrometheusConfig.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d7a946f8-66f3-4667-8902-9c89aca1b367
⛔ Files ignored due to path filters (3)
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1alpha1/zz_generated.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**
📒 Files selected for processing (5)
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlconfig/v1alpha1/types_cluster_monitoring.goconfig/v1alpha1/zz_generated.deepcopy.goconfig/v1alpha1/zz_generated.swagger_doc_generated.gopayload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml
- config/v1alpha1/zz_generated.deepcopy.go
|
@everettraven could you PTAL when available? Thanks! |
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
This generally seems fine aside from merge conflicts.
I suspect this needs some updating in relation to recent container resources CEL changes?
danielmellado
force-pushed
the
mon_4037_monitoring_plugin_config
branch
from
30b1be2 to
860ad02
Compare
|
Hey, thanks for the review! Rebased onto latest master and fixed everything up. I also |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml`:
- Around line 1403-1428: The CRD schema lacks enforcement for constrained enum
and dependent/threshold fields; update
config/v1alpha1/types_cluster_monitoring.go to add validated wrapper types or
XValidation rules for the ClusterMonitoring spec fields (validate enums for
effect, operator, whenUnsatisfiable, nodeAffinityPolicy, nodeTaintsPolicy;
enforce maxSkew>0 and minDomains>0; require matchLabelKeys only when
labelSelector is present), implement validation functions (or kubebuilder
annotations) on the relevant types/fields and then regenerate the CRD so the
payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml
includes the XValidation blocks and stricter schemas reflecting those
constraints.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: dcee3600-f546-4282-a07b-f73def79049e
⛔ Files ignored due to path filters (3)
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1alpha1/zz_generated.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**
📒 Files selected for processing (5)
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlconfig/v1alpha1/types_cluster_monitoring.goconfig/v1alpha1/zz_generated.deepcopy.goconfig/v1alpha1/zz_generated.swagger_doc_generated.gopayload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml
✅ Files skipped from review due to trivial changes (2)
- config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml
- config/v1alpha1/types_cluster_monitoring.go
🚧 Files skipped from review as they are similar to previous changes (1)
- config/v1alpha1/zz_generated.swagger_doc_generated.go
| effect: | ||
| description: |- | ||
| Effect indicates the taint effect to match. Empty means match all taint effects. | ||
| When specified, allowed values are NoSchedule, PreferNoSchedule and NoExecute. | ||
| type: string | ||
| key: | ||
| description: |- | ||
| Key is the taint key that the toleration applies to. Empty means match all taint keys. | ||
| If the key is empty, operator must be Exists; this combination means to match all values and all keys. | ||
| type: string | ||
| operator: | ||
| description: |- | ||
| Operator represents a key's relationship to the value. | ||
| Valid operators are Exists, Equal, Lt, and Gt. Defaults to Equal. | ||
| Exists is equivalent to wildcard for value, so that a pod can | ||
| tolerate all taints of a particular category. | ||
| Lt and Gt perform numeric comparisons (requires feature gate TaintTolerationComparisonOperators). | ||
| type: string | ||
| tolerationSeconds: | ||
| description: |- | ||
| TolerationSeconds represents the period of time the toleration (which must be | ||
| of effect NoExecute, otherwise this field is ignored) tolerates the taint. By default, | ||
| it is not set, which means tolerate the taint forever (do not evict). Zero and | ||
| negative values will be treated as 0 (evict immediately) by the system. | ||
| format: int64 | ||
| type: integer |
There was a problem hiding this comment.
Documented scheduling constraints are not enforced here.
This schema says effect/operator/whenUnsatisfiable/nodeAffinityPolicy/nodeTaintsPolicy have restricted values, maxSkew and minDomains must be positive, and matchLabelKeys depends on labelSelector, but the generated shape only types those fields. Examples like effect: Foo, maxSkew: 0, minDomains: 0, or matchLabelKeys without labelSelector would still validate here. Please add validated wrapper types or XValidations in config/v1alpha1/types_cluster_monitoring.go and regenerate the CRD.
Also applies to: 1504-1616
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml`
around lines 1403 - 1428, The CRD schema lacks enforcement for constrained enum
and dependent/threshold fields; update
config/v1alpha1/types_cluster_monitoring.go to add validated wrapper types or
XValidation rules for the ClusterMonitoring spec fields (validate enums for
effect, operator, whenUnsatisfiable, nodeAffinityPolicy, nodeTaintsPolicy;
enforce maxSkew>0 and minDomains>0; require matchLabelKeys only when
labelSelector is present), implement validation functions (or kubebuilder
annotations) on the relevant types/fields and then regenerate the CRD so the
payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml
includes the XValidation blocks and stricter schemas reflecting those
constraints.
There was a problem hiding this comment.
This is generally the approach we would recommend from an API review perspective, but there was already an separate agreement that because these values are being plumbed directly to a Pod Spec that it is OK to re-use the upstream types that don't implement the same markers used to generate the CRD schema with appropriate constraints.
There was a problem hiding this comment.
@everettraven Acknowledged — thanks for the context. Given the prior agreement to reuse upstream PodSpec types for these scheduling fields, I won’t block on adding CRD-level XValidations here.
Non-blocking suggestions to reduce user surprise:
- Add a one-liner to the field docs noting these are pass-throughs to PodSpec; enum/dependency checks are enforced by Kubernetes, not this CRD.
- If you later want minimal guardrails without diverging much, consider cheap CEL rules like “maxSkew ≥ 1”, “minDomains ≥ 1 when set”, and “matchLabelKeys requires labelSelector” (subject to CEL cost budget).
No change requested from me otherwise.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
danielmellado
force-pushed
the
mon_4037_monitoring_plugin_config
branch
from
860ad02 to
d980b5e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml (1)
1403-1428:⚠️ Potential issue | 🟠 MajorDocumented constraints are still not enforced in schema.
The descriptions declare constrained values/relationships (for example
effect/operator/whenUnsatisfiable, positivemaxSkew/minDomains, andmatchLabelKeysdependency onlabelSelector), but this block still only types those fields. Invalid values and invalid combinations can pass CRD validation despite the documented contract.Please add explicit enum/range/dependency validations in the source API type (
config/v1alpha1/types_cluster_monitoring.go) and regenerate this CRD.Also applies to: 1504-1616
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml` around lines 1403 - 1428, The CRD allows invalid values/combinations because the API types lack validation; update the source API (config/v1alpha1/types_cluster_monitoring.go) to add kubebuilder validation markers: enum tags for fields like Effect, Operator, WhenUnsatisfiable; minimum (>0) or maximum tags for numeric fields like MaxSkew and MinDomains; pattern/enum or required markers for matchLabelKeys and add an XValidations or validation rule to enforce that matchLabelKeys is only allowed when LabelSelector is present; then regenerate the CRD so the payload-manifests/crds/..._clustermonitorings.crd.yaml includes these enums/ranges/dependencies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml`:
- Around line 1403-1428: The CRD allows invalid values/combinations because the
API types lack validation; update the source API
(config/v1alpha1/types_cluster_monitoring.go) to add kubebuilder validation
markers: enum tags for fields like Effect, Operator, WhenUnsatisfiable; minimum
(>0) or maximum tags for numeric fields like MaxSkew and MinDomains;
pattern/enum or required markers for matchLabelKeys and add an XValidations or
validation rule to enforce that matchLabelKeys is only allowed when
LabelSelector is present; then regenerate the CRD so the
payload-manifests/crds/..._clustermonitorings.crd.yaml includes these
enums/ranges/dependencies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 957d412b-78a5-4e54-aa44-56532f314fe4
⛔ Files ignored due to path filters (3)
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1alpha1/zz_generated.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**
📒 Files selected for processing (4)
config/v1alpha1/types_cluster_monitoring.goconfig/v1alpha1/zz_generated.deepcopy.goconfig/v1alpha1/zz_generated.swagger_doc_generated.gopayload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- config/v1alpha1/zz_generated.deepcopy.go
Signed-off-by: Daniel Mellado <dmellado@fedoraproject.org>
danielmellado
force-pushed
the
mon_4037_monitoring_plugin_config
branch
from
d980b5e to
5257bc5
Compare
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@danielmellado: The following tests 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. |
openshift-ci
bot
added
the
needs-rebase
|
PR needs rebase. 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. |
4 participants
No description provided.