MON-4608: add interrupts to NodeExporterCollectorConfig CRD types - #2955
MON-4608: add interrupts to NodeExporterCollectorConfig CRD types#2955midu16 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
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. |
|
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 ignored due to path filters (7)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds the disabled-by-default 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 |
PR Summary by QodoAdd interrupts collector config to ClusterMonitoring NodeExporter CRD
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
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
Code Review by Qodo
1.
|
everettraven
left a comment
There was a problem hiding this comment.
Overall this looks pretty good. A few comments.
2aa5ff4 to
8021c22
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml (1)
2157-2198: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd test coverage for the new
interruptscollector.This file updates the
netClassandsystemdexpected error messages, but it adds no test cases for the newinterruptscollector. Theinterruptscollector introduces its owncollectionPolicy/collectgate, aminProperties: 1guard oncollect, and anincludelist withminItems,maxItems,maxLength,minLength, and apatternregex constraint. None of these constraints have accept or reject test cases here.Add test cases similar to the existing
netClass/systemdtests, for example:
- Accept
interruptswithcollectionPolicy: Collectandcollect.includeset.- Reject
interruptswithcollectset whencollectionPolicy: DoNotCollect.- Accept
interruptswithcollectionPolicy: DoNotCollectand nocollect.- Reject
interruptswith an emptycollect: {}object.- Reject an
includeentry that violates the^[\x20-\x7E]+$pattern or exceedsmaxLength.🤖 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/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml` around lines 2157 - 2198, Add validation test cases for the new interrupts collector alongside the existing netClass and systemd cases: accept Collect with a populated collect.include list, reject collect under DoNotCollect, accept DoNotCollect without collect, reject an empty collect object, and reject include entries violating the printable-ASCII pattern or maxLength constraint. Use the existing ClusterMonitoring test structure and expected error conventions.
🤖 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.
Nitpick comments:
In
`@config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml`:
- Around line 2157-2198: Add validation test cases for the new interrupts
collector alongside the existing netClass and systemd cases: accept Collect with
a populated collect.include list, reject collect under DoNotCollect, accept
DoNotCollect without collect, reject an empty collect object, and reject include
entries violating the printable-ASCII pattern or maxLength constraint. Use the
existing ClusterMonitoring test structure and expected error conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 200ee26b-ff88-4769-af0f-cc28c5aa6040
⛔ 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 (3)
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlconfig/v1alpha1/types_cluster_monitoring.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/types_cluster_monitoring.go
8021c22 to
0f3afe1
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Scheduling tests matching the |
0f3afe1 to
595800a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
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 |
|
@simonpasquier any feedback from your side ? Thank you! |
|
@midu16: 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. |
|
@everettraven it might require an overwrite of ci/prow/verify-hypershift-integration can you help me with this ? |
|
/hold we need to agree on openshift/cluster-monitoring-operator#2888 first |
@simonpasquier would appreciate your feedback for PR#2888 . |
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 ✅