Skip to content

MON-4607: add zoneinfo to NodeExporterCollectorConfig CRD types - #2948

Open
midu16 wants to merge 1 commit into
openshift:masterfrom
midu16:node-exporter-collector-zoneinfo
Open

MON-4607: add zoneinfo to NodeExporterCollectorConfig CRD types#2948
midu16 wants to merge 1 commit into
openshift:masterfrom
midu16:node-exporter-collector-zoneinfo

Conversation

@midu16

@midu16 midu16 commented Jul 23, 2026

Copy link
Copy Markdown

Add NodeExporterCollectorZoneinfoConfig with a collectionPolicy-only struct (matching the softirqs pattern) and the Zoneinfo field to NodeExporterCollectorConfig. Regenerated CRDs, deepcopy, and OpenAPI.

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 23, 2026
@openshift-ci-robot

openshift-ci-robot commented Jul 23, 2026

Copy link
Copy Markdown

@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.

Details

In response to this:

Add NodeExporterCollectorZoneinfoConfig with a collectionPolicy-only struct (matching the softirqs pattern) and the Zoneinfo field to NodeExporterCollectorConfig. Regenerated CRDs, deepcopy, and OpenAPI.

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.

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 4ddec9cf-94a6-4c8f-b0af-bae7c29f738c

📥 Commits

Reviewing files that changed from the base of the PR and between 10aafac and fe597b0.

⛔ Files ignored due to path filters (7)
  • config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings.crd.yaml is excluded by !**/zz_generated.crd-manifests/*
  • config/v1alpha1/zz_generated.deepcopy.go is excluded by !**/zz_generated*
  • config/v1alpha1/zz_generated.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
  • config/v1alpha1/zz_generated.model_name.go is excluded by !**/zz_generated*
  • config/v1alpha1/zz_generated.swagger_doc_generated.go is excluded by !**/zz_generated*
  • openapi/generated_openapi/zz_generated.openapi.go is excluded by !openapi/**, !**/zz_generated*
  • openapi/openapi.json is excluded by !openapi/**
📒 Files selected for processing (2)
  • config/v1alpha1/types_cluster_monitoring.go
  • payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml

📝 Walkthrough

Walkthrough

The node-exporter collector configuration API adds optional deviceMapperMultipath and zoneinfo collectors, each with a collectionPolicy setting. The ClusterMonitoring CRD schema exposes these collectors and restricts their policies to Collect or DoNotCollect.

Possibly related PRs

  • openshift/api#2956: Adds overlapping device mapper multipath collector API and CRD fields.
  • openshift/api#2960: Adds another node-exporter collector using the same collectionPolicy configuration pattern.

Suggested reviewers: joelspeed, everettraven, saschagrunert, marioferh

🚥 Pre-merge checks | ✅ 15
✅ Passed checks (15 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and clearly covers the main zoneinfo CRD type change, though it omits the additional multipath collector.
Description check ✅ Passed The description is directly related to the change set and accurately describes the new zoneinfo config and regenerated artifacts.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed No Ginkgo test titles were added or modified; the diff only touches config, CRD, and generated schema files.
Test Structure And Quality ✅ Passed No Ginkgo test files or test logic were changed in this PR; only config types and generated manifests were updated.
Microshift Test Compatibility ✅ Passed No new Ginkgo/e2e test files were added; the patch only changes config types and generated schema/docs.
Single Node Openshift (Sno) Test Compatibility ✅ Passed No new Ginkgo/e2e tests were added; only config/CRD/OpenAPI code-gen changes, so no SNO assumptions to review.
Topology-Aware Scheduling Compatibility ✅ Passed Only node-exporter collector API/CRD schema was added; no deployment, controller, affinity, toleration, replica, or nodeSelector changes.
Ote Binary Stdout Contract ✅ Passed The PR only adds config/CRD schema types; no main/init/TestMain/suite setup code or stdout-writing APIs appear in the changed files.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed No new Ginkgo e2e tests were added; changes are schema/codegen only, with no IPv4-only or external connectivity assumptions in changed files.
No-Weak-Crypto ✅ Passed Touched files only add monitoring collector config/schema; no weak-crypto terms, custom crypto, or non-constant-time secret compares were found.
Container-Privileges ✅ Passed Changed files only add collector schema/CRD definitions; no privileged, hostPID/network/IPC, SYS_ADMIN, or allowPrivilegeEscalation settings were added.
No-Sensitive-Data-In-Logs ✅ Passed Only schema/type additions and regenerated manifests were changed; precise search found no logging calls in the touched files.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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
The command is terminated due to an error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented


Comment @coderabbitai help to get the list of available commands.

@openshift-ci

openshift-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Hello @midu16! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

@qodo-for-rh-openshift

Copy link
Copy Markdown

PR Summary by Qodo

Add zoneinfo collector config to ClusterMonitoringConfig node-exporter types

✨ Enhancement ⚙️ Configuration changes 🕐 20-40 Minutes

Grey Divider

AI Description

• Add a Zoneinfo collector configuration block to NodeExporterCollectorConfig.
• Introduce NodeExporterCollectorZoneinfoConfig with collectionPolicy semantics matching other
 collectors.
• Regenerate CRD manifests, deepcopy methods, and OpenAPI/Swagger artifacts to expose the new field.
Diagram

graph TD
  A["ClusterMonitoringConfig CR"] --> B["NodeExporterCollectorConfig"] --> C["Zoneinfo config"]
  C --> D["CRD manifests"]
  C --> E["Go codegen"] --> F["OpenAPI JSON"]
Loading
High-Level Assessment

The approach (dedicated Zoneinfo config struct with collectionPolicy, mirroring existing collector patterns like softirqs) is consistent with the API style, keeps the CRD schema explicit, and fits existing codegen pipelines. A more generic shared collector-config type could reduce duplication, but would be a broader API refactor with higher compatibility risk than warranted for a single new collector.

Files changed (9) +176 / -1

Enhancement (1) +22 / -0
types_cluster_monitoring.goAdd Zoneinfo collector field and config type to node-exporter API +22/-0

Add Zoneinfo collector field and config type to node-exporter API

• Adds a new Zoneinfo field to NodeExporterCollectorConfig with descriptive API docs. Introduces NodeExporterCollectorZoneinfoConfig containing only collectionPolicy, matching the established collector config pattern.

config/v1alpha1/types_cluster_monitoring.go

Documentation (1) +10 / -0
zz_generated.swagger_doc_generated.goAdd Swagger docs for zoneinfo field and Zoneinfo config type +10/-0

Add Swagger docs for zoneinfo field and Zoneinfo config type

• Extends the NodeExporterCollectorConfig SwaggerDoc map with the zoneinfo field description. Adds a SwaggerDoc map/function for NodeExporterCollectorZoneinfoConfig and its collectionPolicy field.

config/v1alpha1/zz_generated.swagger_doc_generated.go

Other (7) +144 / -1
0000_10_config-operator_01_clustermonitorings.crd.yamlExpose zoneinfo under node-exporter collectors in generated CRD schema +23/-0

Expose zoneinfo under node-exporter collectors in generated CRD schema

• Regenerates the ClusterMonitoringConfig CRD manifest to include the zoneinfo property. Adds validation/enum for collectionPolicy and marks it required within the zoneinfo object.

config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings.crd.yaml

zz_generated.deepcopy.goRegenerate deepcopy for Zoneinfo config and collector config struct +17/-0

Regenerate deepcopy for Zoneinfo config and collector config struct

• Updates NodeExporterCollectorConfig DeepCopyInto to copy the new Zoneinfo field. Adds autogenerated DeepCopy/DeepCopyInto methods for NodeExporterCollectorZoneinfoConfig.

config/v1alpha1/zz_generated.deepcopy.go

ClusterMonitoringConfig.yamlRegenerate feature-gated CRD schema with zoneinfo collector config +23/-0

Regenerate feature-gated CRD schema with zoneinfo collector config

• Updates the feature-gated CRD manifest to include the zoneinfo property and its collectionPolicy enum/required constraint, mirroring the main CRD schema.

config/v1alpha1/zz_generated.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml

zz_generated.model_name.goRegister OpenAPI model name for Zoneinfo config type +5/-0

Register OpenAPI model name for Zoneinfo config type

• Adds OpenAPIModelName() for NodeExporterCollectorZoneinfoConfig so it can be referenced in generated OpenAPI definitions.

config/v1alpha1/zz_generated.model_name.go

zz_generated.openapi.goRegenerate OpenAPI Go definitions to include zoneinfo schema +31/-1

Regenerate OpenAPI Go definitions to include zoneinfo schema

• Registers NodeExporterCollectorZoneinfoConfig in GetOpenAPIDefinitions and adds it as a dependency of NodeExporterCollectorConfig. Introduces the schema generator function for the new type and adds the zoneinfo property to the collector config schema.

openapi/generated_openapi/zz_generated.openapi.go

openapi.jsonRegenerate OpenAPI JSON with zoneinfo property and definition +22/-0

Regenerate OpenAPI JSON with zoneinfo property and definition

• Adds the zoneinfo field to the NodeExporterCollectorConfig definition with a reference to the new type. Introduces the NodeExporterCollectorZoneinfoConfig schema definition requiring collectionPolicy with the Collect/DoNotCollect enum.

openapi/openapi.json

0000_10_config-operator_01_clustermonitorings.crd.yamlUpdate payload CRD manifest to include zoneinfo collector config +23/-0

Update payload CRD manifest to include zoneinfo collector config

• Regenerates the shipped CRD payload manifest to include the zoneinfo property and its validation, keeping payload CRDs aligned with the API type changes.

payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml

@openshift-ci openshift-ci Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 23, 2026
@qodo-for-rh-openshift

qodo-for-rh-openshift Bot commented Jul 23, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 29 rules
✅ Skills: api-review

Grey Divider


Informational

1. No zoneinfo CRD tests 🐞 Bug ⚙ Maintainability
Description
The CRD validation tests for node-exporter collectors don’t exercise the new
spec.nodeExporterConfig.collectors.zoneinfo field, so regressions in its required/enum validation
(e.g., missing collectionPolicy) could ship without test coverage. This is a
maintainability/regression-risk gap rather than a current schema defect.
Code

config/v1alpha1/types_cluster_monitoring.go[R469-476]

+	// zoneinfo configures the zoneinfo collector, which exposes per-zone memory page counts,
+	// 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.
+	// Enable when you need visibility into kernel memory zone allocation and pressure.
+	// +optional
+	Zoneinfo NodeExporterCollectorZoneinfoConfig `json:"zoneinfo,omitempty,omitzero"`
Relevance

● Weak

Similar requests to add extra v1alpha1 CRD integration test cases were rejected previously in this
repo.

PR-#2855

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The API adds the new collectors.zoneinfo field, but the existing collector test case enumerates
several collectors without including zoneinfo, so it won’t catch schema regressions specific to
that field.

config/v1alpha1/types_cluster_monitoring.go[461-476]
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml[1976-2024]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The collector validation tests cover several existing collectors but do not include the newly added `collectors.zoneinfo` field. This reduces confidence that the new schema (required `collectionPolicy`, enum values) will remain correct over time.

### Issue Context
`Zoneinfo` was added to `NodeExporterCollectorConfig` and a new `NodeExporterCollectorZoneinfoConfig` type was introduced.

### Fix Focus Areas
- config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml[1976-2024]
- config/v1alpha1/types_cluster_monitoring.go[461-476]

### What to change
- Extend the existing “Should be able to create NodeExporterConfig with collectors” test to include:
 - `zoneinfo:
     collectionPolicy: Collect`
- Add a negative test to reject `zoneinfo: {}` (missing required `collectionPolicy`).
- (Optional) Add a negative test for an invalid enum value under `zoneinfo.collectionPolicy`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

@openshift-ci
openshift-ci Bot requested review from JoelSpeed and everettraven July 23, 2026 16:30
midu16 added a commit to midu16/cluster-monitoring-operator that referenced this pull request Jul 23, 2026
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.
midu16 added a commit to midu16/cluster-monitoring-operator that referenced this pull request Jul 23, 2026
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.
@midu16

midu16 commented Jul 24, 2026

Copy link
Copy Markdown
Author

/test verify-hypershift-integration

@qodo-for-rh-openshift

qodo-for-rh-openshift Bot commented Jul 24, 2026

Copy link
Copy Markdown

PR-Agent: could not find a component named verify-hypershift-integration in a supported language in this PR.

@midu16

midu16 commented Jul 24, 2026

Copy link
Copy Markdown
Author

/retest-required

@midu16

midu16 commented Jul 24, 2026

Copy link
Copy Markdown
Author

@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

midu16 added a commit to midu16/cluster-monitoring-operator that referenced this pull request Jul 24, 2026
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.
@JoelSpeed

Copy link
Copy Markdown
Contributor

The hypershift integration is broken pending a rebase of the HyperShift dependencies to 1.36, nothing to do with your PR presently

@midu16

midu16 commented Jul 24, 2026

Copy link
Copy Markdown
Author

@JoelSpeed thank you for your answer, may i get a feedback/review from your side on the PR ?

@everettraven

Copy link
Copy Markdown
Contributor

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.

@midu16

midu16 commented Jul 24, 2026

Copy link
Copy Markdown
Author

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

@simonpasquier simonpasquier left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jul 24, 2026
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e-aws-ovn
/test e2e-aws-ovn-hypershift
/test e2e-aws-ovn-hypershift-conformance
/test e2e-aws-ovn-techpreview
/test e2e-aws-serial-1of2
/test e2e-aws-serial-2of2
/test e2e-aws-serial-techpreview-1of2
/test e2e-aws-serial-techpreview-2of2
/test e2e-azure
/test e2e-gcp
/test e2e-upgrade
/test e2e-upgrade-out-of-change
/test minor-e2e-upgrade-minor

@midu16

midu16 commented Jul 25, 2026

Copy link
Copy Markdown
Author

/retest-required

@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2026

@saschagrunert saschagrunert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, the change seems to comply to the API guidelines and looks correct.

@simonpasquier

Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2026
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e-aws-ovn
/test e2e-aws-ovn-hypershift
/test e2e-aws-ovn-hypershift-conformance
/test e2e-aws-ovn-techpreview
/test e2e-aws-serial-1of2
/test e2e-aws-serial-2of2
/test e2e-aws-serial-techpreview-1of2
/test e2e-aws-serial-techpreview-2of2
/test e2e-azure
/test e2e-gcp
/test e2e-upgrade
/test e2e-upgrade-out-of-change
/test minor-e2e-upgrade-minor

@openshift-ci

openshift-ci Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: saschagrunert, simonpasquier
Once this PR has been reviewed and has the lgtm label, please assign joelspeed for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Comment thread config/v1alpha1/types_cluster_monitoring.go Outdated
Comment thread config/v1alpha1/types_cluster_monitoring.go Outdated
@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 28, 2026
@openshift-ci

openshift-ci Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

New changes are detected. LGTM label has been removed.

@JoelSpeed

Copy link
Copy Markdown
Contributor

Please rebase, squash the commits, and update the code generation, and then this is ready to go

@midu16
midu16 force-pushed the node-exporter-collector-zoneinfo branch from 62c7ac4 to 10aafac Compare July 28, 2026 11:56
@midu16

midu16 commented Jul 29, 2026

Copy link
Copy Markdown
Author

@JoelSpeed I've rebased, squashed the commits, and updated the code generation, what to do next?

@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2026
Add NodeExporterCollectorZoneinfoConfig with CollectionPolicy enum
to expose per-zone memory page counts, watermarks, and protection
thresholds from /proc/zoneinfo. Use terminology consistent with
Collect/DoNotCollect enum values.
@midu16
midu16 force-pushed the node-exporter-collector-zoneinfo branch from 10aafac to fe597b0 Compare July 30, 2026 08:59
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 30, 2026
@openshift-ci

openshift-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

@midu16: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-ovn-techpreview 25978bc link true /test e2e-aws-ovn-techpreview
ci/prow/e2e-gcp 25978bc link true /test e2e-gcp
ci/prow/verify-hypershift-integration fe597b0 link true /test verify-hypershift-integration

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants