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
|
[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 |
📝 WalkthroughWalkthroughAdds a new ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented 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
| maxItems: 10 | ||
| minItems: 1 | ||
| type: array | ||
| x-kubernetes-list-type: atomic |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/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)"
printf '== Generated topologySpreadConstraints list semantics across the CRD ==\n'
rg -n -C1 'topologySpreadConstraints:|x-kubernetes-list-map-keys:|x-kubernetes-list-type:' "$crd"
printf '\n== Source markers around MonitoringPluginConfig ==\n'
rg -n -C3 'type MonitoringPluginConfig|TopologySpreadConstraints|listType|listMapKey' "$types_file"Repository: openshift/api
Length of output: 24894
Use map-list semantics for topologySpreadConstraints in MonitoringPluginConfig.
Line 1617 has x-kubernetes-list-type: atomic, while all other topologySpreadConstraints fields in the CRD use keyed map lists with topologyKey and whenUnsatisfiable keys. This inconsistency changes patch/apply behavior for just this field.
Expected fix
maxItems: 10
minItems: 1
type: array
- x-kubernetes-list-type: atomic
+ x-kubernetes-list-map-keys:
+ - topologyKey
+ - whenUnsatisfiable
+ x-kubernetes-list-type: mapUpdate MonitoringPluginConfig.TopologySpreadConstraints markers in the source to use +listType=map with +listMapKey=topologyKey and +listMapKey=whenUnsatisfiable, matching all sibling monitoring component configs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| maxItems: 10 | |
| minItems: 1 | |
| type: array | |
| x-kubernetes-list-type: atomic | |
| maxItems: 10 | |
| minItems: 1 | |
| type: array | |
| x-kubernetes-list-map-keys: | |
| - topologyKey | |
| - whenUnsatisfiable | |
| x-kubernetes-list-type: map |
🤖 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 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.
Signed-off-by: Daniel Mellado <dmellado@fedoraproject.org>
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
|
@danielmellado: all tests passed! 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. |
3 participants
No description provided.