MON-4607: add zoneinfo to NodeExporterCollectorConfig CRD types#2948
MON-4607: add zoneinfo to NodeExporterCollectorConfig CRD types#2948midu16 wants to merge 2 commits into
Conversation
Add NodeExporterCollectorZoneinfoConfig with a collectionPolicy-only struct (matching the softirqs pattern) and the Zoneinfo field to NodeExporterCollectorConfig. Regenerated CRDs, deepcopy, and OpenAPI.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@midu16: This pull request references MON-4607 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 (2)
📝 WalkthroughWalkthroughThe node-exporter collector configuration API adds a 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 |
|
Hello @midu16! Some important instructions when contributing to openshift/api: |
PR Summary by QodoAdd zoneinfo collector config to ClusterMonitoringConfig node-exporter types
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
Code Review by Qodo
1. No zoneinfo CRD tests
|
Add optional nodeExporter.collectors.zoneinfo toggle (disabled by default). Wire --collector.zoneinfo/--no-collector.zoneinfo in updateNodeExporterArgs, extend unit/e2e tests, and add zoneinfo to telemeter collector success matcher. CRD merge (config_merge.go) will be added once openshift/api#2948 merges and the vendor is updated with the NodeExporterCollectorZoneinfoConfig type.
Add optional nodeExporter.collectors.zoneinfo toggle (disabled by default). Wire --collector.zoneinfo/--no-collector.zoneinfo in updateNodeExporterArgs, extend unit/e2e tests, and add zoneinfo to telemeter collector success matcher. Update jsonnet/versions.yaml to match upstream component versions. CRD merge (config_merge.go) will be added once openshift/api#2948 merges and the vendor is updated with the NodeExporterCollectorZoneinfoConfig type.
|
/test verify-hypershift-integration |
|
PR-Agent: could not find a component named |
|
/retest-required |
|
@everettraven @JoelSpeed could you help me understand these failures ? As far as i can see, the issues are environmental not PR/code change wise. Thank you |
Add optional nodeExporter.collectors.zoneinfo toggle (disabled by default). Wire --collector.zoneinfo/--no-collector.zoneinfo in updateNodeExporterArgs, extend unit/e2e tests, and add zoneinfo to telemeter collector success matcher. Update jsonnet/versions.yaml to match upstream component versions. CRD merge (config_merge.go) will be added once openshift/api#2948 merges and the vendor is updated with the NodeExporterCollectorZoneinfoConfig type.
|
The hypershift integration is broken pending a rebase of the HyperShift dependencies to 1.36, nothing to do with your PR presently |
|
@JoelSpeed thank you for your answer, may i get a feedback/review from your side on the PR ? |
|
Just wanted to mention that this has been assigned to @saschagrunert to review. I believe he mentioned that he will take a look at this on Monday. |
Thank you @everettraven |
|
Scheduling tests matching the |
|
/retest-required |
|
@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. |
saschagrunert
left a comment
There was a problem hiding this comment.
LGTM, the change seems to comply to the API guidelines and looks correct.
|
/lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: saschagrunert, simonpasquier 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 |
| // watermarks, and protection thresholds from /proc/zoneinfo. | ||
| // zoneinfo is optional. | ||
| // When omitted, this means no opinion and the platform is left to choose a reasonable default, | ||
| // which is subject to change over time. The current default is disabled. |
There was a problem hiding this comment.
Disabled isn't actually an option here, so I think this godoc is out of date
| // which is subject to change over time. The current default is disabled. | ||
| // Enable when you need visibility into kernel memory zone allocation and pressure. | ||
| // +optional | ||
| Zoneinfo NodeExporterCollectorZoneinfoConfig `json:"zoneinfo,omitempty,omitzero"` |
There was a problem hiding this comment.
Nit, omitempty is superfluous
| Zoneinfo NodeExporterCollectorZoneinfoConfig `json:"zoneinfo,omitempty,omitzero"` | |
| Zoneinfo NodeExporterCollectorZoneinfoConfig `json:"zoneinfo,omitzero"` |
Add NodeExporterCollectorZoneinfoConfig with a collectionPolicy-only struct (matching the softirqs pattern) and the Zoneinfo field to NodeExporterCollectorConfig. Regenerated CRDs, deepcopy, and OpenAPI.