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
|
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
payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml
Show resolved
Hide resolved
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
danielmellado
force-pushed
the
mon_4037_monitoring_plugin_config
branch
from
d980b5e to
5257bc5
Compare
|
Scheduling tests matching the |
openshift-ci
bot
added
approved
danielmellado
force-pushed
the
mon_4037_monitoring_plugin_config
branch
from
5257bc5 to
042508c
Compare
openshift-ci
bot
removed
the
needs-rebase
|
@everettraven I had to rebase this one, mind approving again? thanks! |
Signed-off-by: Daniel Mellado <dmellado@fedoraproject.org>
danielmellado
force-pushed
the
mon_4037_monitoring_plugin_config
branch
from
042508c to
d1fc772
Compare
|
/lgtm |
|
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 |
|
/retest-required |
1 similar comment
|
/retest-required |
|
@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. |
4 participants
No description provided.