MON-4608: add interrupts to NodeExporterCollectorConfig CRD types#2955
MON-4608: add interrupts to NodeExporterCollectorConfig CRD types#2955midu16 wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
Hello @midu16! Some important instructions when contributing to openshift/api: |
|
@midu16: This pull request references MON-4608 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 sub-task to target the "5.0.0" version, but no target version was set. 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. |
openshift-ci
Bot
added
the
size/L
📝 WalkthroughWalkthroughAdds support for configuring the node-exporter Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.12.2)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
|
[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 |
PR Summary by QodoAdd interrupts collector config to ClusterMonitoring NodeExporter CRD
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/v1alpha1/types_cluster_monitoring.go`:
- Around line 741-750: Allow an empty Include list as documented by removing the
MinItems=1 validation marker from NodeExporterInterruptsIncludePattern in
config/v1alpha1/types_cluster_monitoring.go:741-750. Regenerate
payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml:2215-2232
so the CRD no longer enforces a minimum item count.
🪄 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), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ca85e130-9168-465d-a7ab-9487ed4c4c72
⛔ Files ignored due to path filters (7)
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1alpha1/zz_generated.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1alpha1/zz_generated.model_name.gois excluded by!**/zz_generated*config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (2)
config/v1alpha1/types_cluster_monitoring.gopayload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml
| // An empty list or omitting include collects all interrupt lines. | ||
| // Each entry must be at least 1 character and at most 1024 characters. | ||
| // Maximum length for this list is 50. | ||
| // Minimum length for this list is 1. | ||
| // Entries in this list must be unique. | ||
| // +kubebuilder:validation:MaxItems=50 | ||
| // +kubebuilder:validation:MinItems=1 | ||
| // +listType=set | ||
| // +optional | ||
| Include []NodeExporterInterruptsIncludePattern `json:"include,omitempty"` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Allow the documented empty include list.
Line 741 says an empty list collects all interrupt lines, but MinItems=1 rejects include: []. Permit empty lists (remove MinItems) or change the documented API behavior.
config/v1alpha1/types_cluster_monitoring.go#L741-L750: remove theMinItems=1validation marker if empty lists are intended to collect all lines.payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml#L2215-L2232: regenerate the CRD so it no longer requires oneincludeitem.
📍 Affects 2 files
config/v1alpha1/types_cluster_monitoring.go#L741-L750(this comment)payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml#L2215-L2232
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/v1alpha1/types_cluster_monitoring.go` around lines 741 - 750, Allow an
empty Include list as documented by removing the MinItems=1 validation marker
from NodeExporterInterruptsIncludePattern in
config/v1alpha1/types_cluster_monitoring.go:741-750. Regenerate
payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml:2215-2232
so the CRD no longer enforces a minimum item count.
Code Review by Qodo
1. collect omits omit behavior
|
![qodo-for-rh-openshift[bot]](https://avatars.githubusercontent.com/u/792337?s=60&v=4){kind=small}
| // collect contains configuration options that apply only when the interrupts collector is actively collecting metrics | ||
| // (i.e. when collectionPolicy is Collect). | ||
| // collect is optional and may be omitted even when collectionPolicy is Collect. | ||
| // collect may only be set when collectionPolicy is Collect. | ||
| // When set, at least one field must be specified within collect. | ||
| // +unionMember | ||
| // +optional | ||
| Collect NodeExporterCollectorInterruptsCollectConfig `json:"collect,omitzero,omitempty"` |
There was a problem hiding this comment.
2. collect omits omit behavior 📜 Skill insight ✧ Quality
The new optional InterruptsConfig.Collect field is documented as optional, but does not explain what happens when it is omitted (defaults/behavior). This violates the requirement that new optional fields document omit behavior.
Agent Prompt
## Issue description
`Collect` is marked `+optional` but its comment does not describe the behavior when the field is omitted (e.g., what defaults apply / what is collected).
## Issue Context
Optional-field documentation must clearly state omit behavior ("When omitted...", "Defaults to..."), especially for API/CRD fields.
## Fix Focus Areas
- config/v1alpha1/types_cluster_monitoring.go[721-728]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
@midu16: 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. |
2 participants
Summary
Add
NodeExporterCollectorInterruptsConfigto theNodeExporterCollectorConfigCRD type, enabling users to configure the node-exporterinterruptscollector via theClusterMonitoringCR.
Unlike simpler collectors (softirqs, zoneinfo) that use a plain
collectionPolicyon/off switch, the interrupts collector follows the systemd pattern — a union withcollectionPolicyas the discriminator and an optional
collectsub-struct containing anincludelist of regex patterns. This design matches the CMO implementation on thenode-exporter-collector-interruptsbranch, which passes the include patterns to--collector.interrupts.name-include.Changes
Interruptsfield toNodeExporterCollectorConfigNodeExporterCollectorInterruptsConfig(union:collectionPolicy+collect)NodeExporterCollectorInterruptsCollectConfigwithincludelist (+listType=set, max 50 items)NodeExporterInterruptsIncludePatternvalidated string type (1–1024 chars)collectis forbidden whencollectionPolicyis notCollectCross-reference
node-exporter-collector-interrupts— usesInterrupts.Include []stringinternally;config_merge.gowill translate from this CRD'scollectionPolicy/collect.includeto the internal representation when vendoring.
node-exporter-collector-zoneinfo), extended with the union pattern from the systemd collector.Verification
make update(full regeneration) ✅make build✅make -C config/v1alpha1 test— 262 tests passed ✅make lint— 0 issues ✅