MON-4607: add zoneinfo to NodeExporterCollectorConfig CRD types - #2948
MON-4607: add zoneinfo to NodeExporterCollectorConfig CRD types#2948midu16 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@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 optional 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 |
|
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)
|
openshift-ci
Bot
added
the
size/L
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 |
saschagrunert
left a comment
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 |
|
Please rebase, squash the commits, and update the code generation, and then this is ready to go |
62c7ac4 to
10aafac
Compare
|
@JoelSpeed I've rebased, squashed the commits, and updated the code generation, what to do next? |
openshift-ci
Bot
added
the
needs-rebase
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.
10aafac to
fe597b0
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/lgtm |
|
/override ci/prow/verify-hypershift-integration |
|
@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/verify-hypershift-integration 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 kubernetes-sigs/prow repository. |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed, saschagrunert, simonpasquier 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 |
openshift-ci
Bot
added
the
approved
|
@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. |
|
@JoelSpeed this is my testing report (if you want to cover other tests, i can cover them and add the verified label): Test Report: MON-4607 — Zoneinfo Collector in NodeExporterCollectorConfigDate: 2026-08-02 (updated) Test Environment
Cluster Nodes: Summary
Phase 1: Repository Integration Tests1a. Unit Tests (
|
| # | Test Name | Type | Result |
|---|---|---|---|
| 1 | Should accept zoneinfo collector with Collect | Positive | PASS |
| 2 | Should accept zoneinfo collector with DoNotCollect | Positive | PASS |
| 3 | Should reject zoneinfo collector with invalid collectionPolicy | Negative | PASS |
| 4 | Should reject zoneinfo collector with empty collectionPolicy | Negative | PASS |
| 5 | Should be able to create NodeExporterConfig with all fields (updated to include zoneinfo) | Positive | PASS |
Each test ran twice (against both the ungated and ClusterMonitoringConfig-gated CRD variants), for a total of 10 zoneinfo test executions.
Phase 2: Cluster Preparation
Enable TechPreviewNoUpgrade
$ oc patch featuregate cluster --type=merge -p '{"spec":{"featureSet":"TechPreviewNoUpgrade"}}'
featuregate.config.openshift.io/cluster patchedThe cluster-monitoring-operator restarted and registered the clustermonitorings.config.openshift.io CRD within ~30 seconds.
Patch CRD with Local Build
The stock CRD from OCP 4.22.5 does not include the zoneinfo field. Applied the locally-generated CRD:
$ oc replace -f config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings.crd.yaml
customresourcedefinition.apiextensions.k8s.io/clustermonitorings.config.openshift.io replacedNote: oc apply failed due to annotation size limit (CRD > 262144 bytes). oc replace was used instead.
Note: The cluster-monitoring-operator periodically reverts the CRD to the stock version from its payload. Negative tests used --dry-run=server to validate against the API server without requiring persistence, ensuring they ran against the patched CRD schema.
Verified zoneinfo schema was live:
{
"properties": {
"collectionPolicy": {
"enum": ["Collect", "DoNotCollect"],
"type": "string"
}
},
"required": ["collectionPolicy"],
"type": "object"
}Phase 3: Live Cluster Validation Tests
Test 4.1: Create CR with zoneinfo Collect
Result: PASS
$ oc apply -f - <<EOF
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
metadata:
name: cluster
spec:
nodeExporterConfig:
collectors:
zoneinfo:
collectionPolicy: Collect
EOF
clustermonitoring.config.openshift.io/cluster created
$ oc get clustermonitoring cluster -o jsonpath='{.spec.nodeExporterConfig.collectors.zoneinfo.collectionPolicy}'
CollectTest 4.2: Update to DoNotCollect
Result: PASS
$ oc patch clustermonitoring cluster --type=merge \
-p '{"spec":{"nodeExporterConfig":{"collectors":{"zoneinfo":{"collectionPolicy":"DoNotCollect"}}}}}'
clustermonitoring.config.openshift.io/cluster patched
$ oc get clustermonitoring cluster -o jsonpath='{.spec.nodeExporterConfig.collectors.zoneinfo.collectionPolicy}'
DoNotCollectTest 4.3: Reject invalid collectionPolicy
Result: PASS
$ oc apply --dry-run=server -f - <<EOF
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
metadata:
name: test-invalid
spec:
nodeExporterConfig:
collectors:
zoneinfo:
collectionPolicy: Invalid
EOF
The ClusterMonitoring "test-invalid" is invalid:
* spec.nodeExporterConfig.collectors.zoneinfo.collectionPolicy: Unsupported value: "Invalid": supported values: "Collect", "DoNotCollect"Test 4.4: Reject empty zoneinfo (missing required collectionPolicy)
Result: PASS
$ oc apply --dry-run=server -f - <<EOF
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
metadata:
name: test-empty
spec:
nodeExporterConfig:
collectors:
zoneinfo: {}
EOF
The ClusterMonitoring "test-empty" is invalid:
* spec.nodeExporterConfig.collectors.zoneinfo.collectionPolicy: Required valueTest 4.5: Combined collectors (zoneinfo + softirqs + cpuFreq)
Result: PASS
$ oc patch clustermonitoring cluster --type=merge -p '{
"spec": {
"nodeExporterConfig": {
"collectors": {
"zoneinfo": {"collectionPolicy": "Collect"},
"softirqs": {"collectionPolicy": "DoNotCollect"},
"cpuFreq": {"collectionPolicy": "Collect"}
}
}
}
}'
clustermonitoring.config.openshift.io/cluster patched
$ oc get clustermonitoring cluster -o jsonpath='{.spec.nodeExporterConfig.collectors}' | python3 -m json.tool
{
"cpuFreq": {"collectionPolicy": "Collect"},
"softirqs": {"collectionPolicy": "DoNotCollect"},
"zoneinfo": {"collectionPolicy": "Collect"}
}Test 4.6: Update zoneinfo to DoNotCollect on live CR
Result: PASS
$ oc patch clustermonitoring cluster --type=merge \
-p '{"spec":{"nodeExporterConfig":{"collectors":{"zoneinfo":{"collectionPolicy":"DoNotCollect"}}}}}'
clustermonitoring.config.openshift.io/cluster patched
$ oc get clustermonitoring cluster -o jsonpath='{.spec.nodeExporterConfig.collectors.zoneinfo.collectionPolicy}'
DoNotCollectPhase 4: Persistent CRD Patch with CVO Override (2026-08-02)
The initial Phase 3 tests used --dry-run=server for negative tests and observed that the CRD was reconciled back by the CVO/config-operator. This phase demonstrates a persistent patching approach.
Root Cause of Initial Failure
Applying the CR directly against the stock cluster CRD fails because:
- The cluster's CRD (OCP 4.22.5) does not include
nodeExporterConfig,monitoringPluginConfig, orkubeStateMetricsConfig— these are new fields from this branch. - Kubernetes prunes unknown fields from CRs (pruning).
- After pruning,
specbecomes{}, which violatesminProperties: 1onClusterMonitoringSpec.
$ oc apply -f - <<EOF
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
metadata:
name: cluster
spec:
nodeExporterConfig:
collectors:
zoneinfo:
collectionPolicy: Collect
EOF
The ClusterMonitoring "cluster" is invalid: spec: Invalid value: 0: spec in body should have at least 1 properties
Step 1: Build Updated CRD
$ PROTO_OPTIONAL=1 make update
# Generates payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml
# with all new fields including nodeExporterConfig.collectors.zoneinfoStep 2: Add CVO Override
Without this, the CVO reconciles the CRD back to its payload version within seconds.
$ oc patch clusterversion version --type=merge -p '{
"spec": {
"overrides": [
{
"kind": "CustomResourceDefinition",
"name": "clustermonitorings.config.openshift.io",
"namespace": "",
"group": "apiextensions.k8s.io",
"unmanaged": true
}
]
}
}'
clusterversion.config.openshift.io/version patchedNote: The group must be apiextensions.k8s.io (not apiextensions.k8s.io/v1).
Step 3: Scale Down Config Operator (optional, may help prevent races)
$ oc scale deployment openshift-config-operator -n openshift-config-operator --replicas=0
deployment.apps/openshift-config-operator scaledStep 4: Replace CRD
$ oc replace -f payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml
customresourcedefinition.apiextensions.k8s.io/clustermonitorings.config.openshift.io replacedVerified the new fields are present:
$ oc get crd clustermonitorings.config.openshift.io \
-o jsonpath='{.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties}' \
| python3 -c "import json,sys; d=json.load(sys.stdin); print(sorted(d.keys()))"
['alertmanagerConfig', 'kubeStateMetricsConfig', 'metricsServerConfig', 'monitoringPluginConfig',
'nodeExporterConfig', 'openShiftStateMetricsConfig', 'prometheusConfig',
'prometheusOperatorAdmissionWebhookConfig', 'prometheusOperatorConfig',
'telemeterClientConfig', 'thanosQuerierConfig', 'userDefined']
Step 5: Scale Config Operator Back Up
$ oc scale deployment openshift-config-operator -n openshift-config-operator --replicas=1Test 5.1: Create CR with zoneinfo Collect (persistent)
Result: PASS
$ oc apply -f - <<EOF
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
metadata:
name: cluster
spec:
nodeExporterConfig:
collectors:
zoneinfo:
collectionPolicy: Collect
EOF
clustermonitoring.config.openshift.io/cluster configured
$ oc get clustermonitoring cluster -o yaml
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
metadata:
name: cluster
generation: 9
spec:
nodeExporterConfig:
collectors:
zoneinfo:
collectionPolicy: CollectTest 5.2: Update to DoNotCollect (persistent)
Result: PASS
$ oc apply -f - <<EOF
...
zoneinfo:
collectionPolicy: DoNotCollect
EOF
clustermonitoring.config.openshift.io/cluster configured
$ oc get clustermonitoring cluster -o jsonpath='{.spec.nodeExporterConfig.collectors.zoneinfo.collectionPolicy}'
DoNotCollectTest 5.3: Reject invalid collectionPolicy (persistent)
Result: PASS
$ oc apply -f - <<EOF
...
zoneinfo:
collectionPolicy: InvalidValue
EOF
The ClusterMonitoring "cluster" is invalid:
* spec.nodeExporterConfig.collectors.zoneinfo.collectionPolicy: Unsupported value: "InvalidValue": supported values: "Collect", "DoNotCollect"
Test 5.4: Reject empty collectors (persistent)
Result: PASS
$ oc apply -f - <<EOF
...
collectors: {}
EOF
The ClusterMonitoring "cluster" is invalid: spec.nodeExporterConfig.collectors: Invalid value: 0:
spec.nodeExporterConfig.collectors in body should have at least 1 properties
Test 5.5: Reject empty spec (persistent)
Result: PASS
$ oc apply -f - <<EOF
...
spec: {}
EOF
The ClusterMonitoring "cluster" is invalid: spec: Invalid value: 0:
spec in body should have at least 1 properties
Test 5.6: Multiple collectors combined (persistent)
Result: PASS
$ oc apply -f - <<EOF
...
collectors:
zoneinfo:
collectionPolicy: Collect
cpuFreq:
collectionPolicy: Collect
buddyInfo:
collectionPolicy: Collect
EOF
clustermonitoring.config.openshift.io/cluster configured
$ oc get clustermonitoring cluster -o jsonpath='{.spec.nodeExporterConfig.collectors}' | python3 -m json.tool
{
"buddyInfo": {"collectionPolicy": "Collect"},
"cpuFreq": {"collectionPolicy": "Collect"},
"zoneinfo": {"collectionPolicy": "Collect"}
}CRD Persistence Verification
Verified the CRD was NOT reconciled back after 60+ seconds (with CVO override in place):
CRD resourceVersion before: 3278897
CRD resourceVersion after 60s: 3278897
PASS: CRD resourceVersion unchanged — no reconciliation occurred
CRD still has nodeExporterConfig: True
CR spec still intact: {"nodeExporterConfig":{"collectors":{"zoneinfo":{"collectionPolicy":"Collect"}}}}
Also verified persistence after scaling config-operator back up (the CVO override protects the CRD):
Config operator pod: Running (1/1)
CRD still has nodeExporterConfig: True
CR spec still intact: {"nodeExporterConfig":{"collectors":{"zoneinfo":{"collectionPolicy":"Collect"}}}}
Cleanup: Remove CVO Override
After testing, remove the override to restore normal cluster behavior:
$ oc patch clusterversion version --type=json -p '[{"op": "remove", "path": "/spec/overrides"}]'Observations
-
CRD operator reconciliation: Without the CVO override, both the CVO and config-operator reconcile the CRD back to the stock payload version within seconds. The CVO override with
unmanaged: trueprevents this. -
CRD size: The ClusterMonitoring CRD exceeds 262KB, which prevents
oc applyfrom working due to thelast-applied-configurationannotation size limit.oc replacemust be used. -
Schema consistency: The zoneinfo field follows the exact same pattern as other simple collectors (softirqs, buddyInfo, cpuFreq, etc.), ensuring consistency in the API surface.
-
CVO override group format: The
groupfield in CVO overrides must beapiextensions.k8s.io(without/v1suffix). Usingapiextensions.k8s.io/v1does not work.
Conclusions
ALL TESTS PASSED. The zoneinfo collector addition to NodeExporterCollectorConfig is working correctly:
- Go types compile and pass deepcopy/roundtrip tests
- CRD schema validation (enum, required) works in envtest and live cluster
- The field integrates correctly alongside other collectors
- Invalid values are properly rejected by the API server
- CRD and CR persist with CVO override in place
- Config-operator restart does not overwrite the patched CRD when CVO override is active
6 participants
Add NodeExporterCollectorZoneinfoConfig with a collectionPolicy-only struct (matching the softirqs pattern) and the Zoneinfo field to NodeExporterCollectorConfig. Regenerated CRDs, deepcopy, and OpenAPI.