Skip to content

MON-4029: Add KubeStateMetricsConfig to ClusterMonitoring API#2778

Merged
openshift-merge-bot[bot] merged 1 commit into
openshift:masterfrom
danielmellado:mon_4029_kube_state_metrics
Jun 4, 2026
Merged

MON-4029: Add KubeStateMetricsConfig to ClusterMonitoring API#2778
openshift-merge-bot[bot] merged 1 commit into
openshift:masterfrom
danielmellado:mon_4029_kube_state_metrics

Conversation

@danielmellado

Copy link
Copy Markdown
Contributor

Adds a new KubeStateMetricsConfig struct to the ClusterMonitoring CRD,
allowing configuration of the kube-state-metrics agent (node selectors,
resources, tolerations, topology spread constraints). Includes
comprehensive integration tests for validation.

Signed-off-by: Daniel Mellado dmellado@redhat.com

@openshift-ci-robot

Copy link
Copy Markdown

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 Mar 24, 2026
@openshift-ci-robot

openshift-ci-robot commented Mar 24, 2026

Copy link
Copy Markdown

@danielmellado: This pull request references MON-4029 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 story to target the "4.22.0" version, but no target version was set.

Details

In response to this:

Adds a new KubeStateMetricsConfig struct to the ClusterMonitoring CRD,
allowing configuration of the kube-state-metrics agent (node selectors,
resources, tolerations, topology spread constraints). Includes
comprehensive integration tests for validation.

Signed-off-by: Daniel Mellado dmellado@redhat.com

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

openshift-ci Bot commented Mar 24, 2026

Copy link
Copy Markdown
Contributor

Hello @danielmellado! 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.

@openshift-ci openshift-ci Bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 24, 2026
@danielmellado

Copy link
Copy Markdown
Contributor Author

@everettraven this PR continues #2461 and rebases and adds your comments. Thanks!

@coderabbitai

coderabbitai Bot commented Mar 24, 2026

Copy link
Copy Markdown

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Added an optional kubeStateMetricsConfig field to ClusterMonitoringSpec and new exported types: KubeStateMetricsConfig, KubeStateMetricsResourceLabels, KubeStateMetricsResourceName, and KubeStateMetricsLabelName plus related constants. Generated deepcopy and Swagger doc methods for the new types. Extended the ClusterMonitoring CRD schema with validation for nodeSelector, resources, tolerations, topologySpreadConstraints, and additionalResourceLabels. Added extensive YAML tests covering valid and invalid spec.kubeStateMetricsConfig cases.

🚥 Pre-merge checks | ✅ 11 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Test Structure And Quality ⚠️ Warning Ginkgo test framework in tests/generator.go lacks assertion messages on cluster operations. Lines 137, 178, 235, 270, 298, 332 have Expect calls without descriptive failure messages. Add meaningful failure messages to Expect assertions on k8sClient operations in generator.go. Change Expect(k8sClient.Get(...)) to Expect(k8sClient.Get(...), "description")
✅ Passed checks (11 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and specifically summarizes the main change—adding KubeStateMetricsConfig to the ClusterMonitoring API, which is the core focus of all file changes.
Description check ✅ Passed The description is directly related to the changeset, accurately describing the addition of KubeStateMetricsConfig struct and mentioning key features (node selectors, resources, tolerations, topology spread constraints) plus comprehensive tests.
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 All 165 test names in YAML test file are stable and deterministic. No dynamic content (pod names, timestamps, UUIDs, IPs) found. All names use clear descriptive verbs and are not test-run specific.
Microshift Test Compatibility ✅ Passed No Ginkgo e2e tests are added in this PR. Changes are limited to API type definitions, generated code, YAML validation tests, and CRD manifests. MicroShift compatibility check not applicable.
Single Node Openshift (Sno) Test Compatibility ✅ Passed No multi-node SNO incompatibilities found. The PR adds YAML-based CRD schema validation tests that run against single-node envtest environment. Tests validate field constraints only.
Topology-Aware Scheduling Compatibility ✅ Passed PR adds optional KubeStateMetricsConfig to openshift/api CRD. All scheduling fields are optional with platform defaults. No hardcoded topology-unaware constraints introduced.
Ote Binary Stdout Contract ✅ Passed No OTE Stdout Contract violations detected. Changes limited to type definitions, auto-generated code, CRD manifests, and YAML test cases. No stdout writes, klog, or init() functions found.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed The PR adds declarative CRD validation tests (YAML), not Ginkgo e2e tests. No IPv4 assumptions, hardcoded IPs, or external connectivity requirements detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml`:
- Around line 1117-1234: The CRD currently documents invariants but doesn't
enforce them; update the schema for the PodTopologySpread-like object to:
restrict whenUnsatisfiable to an enum of allowed values (e.g.,
"DoNotSchedule","ScheduleAnyway") on the whenUnsatisfiable property, add
minimum: 1 (and disallow 0) on maxSkew (format int32) and minimum: 1 on
minDomains (or use minimum: 1 when present), and add a validation rule that
requires labelSelector when matchLabelKeys is set (make matchLabelKeys mutually
exclusive with labelSelector and/or add a x-kubernetes-requirements or OpenAPI
dependency that enforces labelSelector presence when matchLabelKeys exists).
Target the properties named whenUnsatisfiable, maxSkew, minDomains,
matchLabelKeys and labelSelector in the CRD fragment to implement these
constraints.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: a91c322b-92fd-43a0-a96f-b2b4da86aa91

📥 Commits

Reviewing files that changed from the base of the PR and between 324a1bc and 02919b5.

⛔ Files ignored due to path filters (3)
  • 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.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
  • openapi/generated_openapi/zz_generated.openapi.go is excluded by !openapi/**
📒 Files selected for processing (5)
  • config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml
  • config/v1alpha1/types_cluster_monitoring.go
  • config/v1alpha1/zz_generated.deepcopy.go
  • config/v1alpha1/zz_generated.swagger_doc_generated.go
  • payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml

@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 28, 2026
@danielmellado
danielmellado force-pushed the mon_4029_kube_state_metrics branch from 02919b5 to 361dc49 Compare April 7, 2026 13:37
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2026
@danielmellado

Copy link
Copy Markdown
Contributor Author

/hold until openshift/cluster-monitoring-operator#2553 merges

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 7, 2026
@danielmellado

Copy link
Copy Markdown
Contributor Author

/cc @rexagod

@openshift-ci
openshift-ci Bot requested a review from rexagod April 7, 2026 13:38
@danielmellado
danielmellado force-pushed the mon_4029_kube_state_metrics branch from 361dc49 to d134762 Compare April 17, 2026 12:56
@danielmellado

Copy link
Copy Markdown
Contributor Author

/unhold

@openshift-ci openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml (1)

1175-1292: ⚠️ Potential issue | 🟠 Major

Enforce documented topology spread invariants in schema (still missing).

This block documents constraints, but schema-level enforcement is still incomplete (whenUnsatisfiable values, positive maxSkew/minDomains, and matchLabelKeys requiring labelSelector). Invalid CRs can pass admission and fail later.

🔧 Proposed schema validation patch
                         maxSkew:
@@
                           format: int32
+                          minimum: 1
                           type: integer
                         minDomains:
@@
                           format: int32
+                          minimum: 1
                           type: integer
@@
                         whenUnsatisfiable:
@@
+                          enum:
+                          - DoNotSchedule
+                          - ScheduleAnyway
                           type: string
                       required:
                       - maxSkew
                       - topologyKey
                       - whenUnsatisfiable
                       type: object
+                      x-kubernetes-validations:
+                      - message: minDomains can only be set when whenUnsatisfiable is DoNotSchedule
+                        rule: '!has(self.minDomains) || self.whenUnsatisfiable == ''DoNotSchedule'''
+                      - message: matchLabelKeys cannot be set when labelSelector is not set
+                        rule: '!has(self.matchLabelKeys) || has(self.labelSelector)'
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml`
around lines 1175 - 1292, The CRD currently documents invariants but doesn't
enforce them; update the CRD's openAPIV3Schema to (1) restrict whenUnsatisfiable
to the allowed enum values (e.g., "DoNotSchedule","ScheduleAnyway"), (2) add
minimum: 1 for maxSkew and minDomains (and type integer) so maxSkew cannot be
0/negative and minDomains > 0, (3) add a conditional validation that when
minDomains is set then whenUnsatisfiable must equal "DoNotSchedule", and (4) add
validations requiring that matchLabelKeys cannot be present unless labelSelector
is set and that keys in matchLabelKeys do not overlap with labelSelector keys;
implement the conditional/complex checks using CRD CEL
(x-kubernetes-validations) or equivalent CRD validation mechanisms referencing
the fields matchLabelKeys, labelSelector, maxSkew, minDomains, and
whenUnsatisfiable so invalid CRs are rejected at admission time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml`:
- Around line 2145-2158: The test "Should reject KubeStateMetricsConfig with
duplicate resource in additionalResourceLabels" uses an invalid resource value
"jobs" which triggers enum validation before duplicate detection; update the two
entries under spec.kubeStateMetricsConfig.additionalResourceLabels to use a
valid resource (for example "pods" or another accepted enum value) so the YAML
passes resource validation and the duplicate-resource check is exercised by
ClusterMonitoring validation.

---

Duplicate comments:
In
`@payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml`:
- Around line 1175-1292: The CRD currently documents invariants but doesn't
enforce them; update the CRD's openAPIV3Schema to (1) restrict whenUnsatisfiable
to the allowed enum values (e.g., "DoNotSchedule","ScheduleAnyway"), (2) add
minimum: 1 for maxSkew and minDomains (and type integer) so maxSkew cannot be
0/negative and minDomains > 0, (3) add a conditional validation that when
minDomains is set then whenUnsatisfiable must equal "DoNotSchedule", and (4) add
validations requiring that matchLabelKeys cannot be present unless labelSelector
is set and that keys in matchLabelKeys do not overlap with labelSelector keys;
implement the conditional/complex checks using CRD CEL
(x-kubernetes-validations) or equivalent CRD validation mechanisms referencing
the fields matchLabelKeys, labelSelector, maxSkew, minDomains, and
whenUnsatisfiable so invalid CRs are rejected at admission time.
🪄 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: Pro Plus

Run ID: 5847c428-e700-46b3-84f5-f167574373e9

📥 Commits

Reviewing files that changed from the base of the PR and between 361dc49 and d134762.

⛔ Files ignored due to path filters (6)
  • 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.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 (3)
  • config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml
  • config/v1alpha1/types_cluster_monitoring.go
  • payload-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

@rexagod

rexagod commented Apr 22, 2026

Copy link
Copy Markdown
Member

@danielmellado could you please rebase this? Thanks again for the patch! :)

@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2026
@danielmellado
danielmellado force-pushed the mon_4029_kube_state_metrics branch from d134762 to d1719c1 Compare April 23, 2026 10:21

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml (1)

2231-2506: Add explicit negative tests for nodeSelector and tolerations in kubeStateMetricsConfig.

nodeSelector and tolerations are only exercised in the all-fields happy path (Line 2237 and Line 2243). Unlike sibling configs in this file, there are no reject cases for empty/oversized values, so regressions in those validations could slip through.

🧪 Suggested test additions
+    - name: Should reject KubeStateMetricsConfig with empty nodeSelector
+      initial: |
+        apiVersion: config.openshift.io/v1alpha1
+        kind: ClusterMonitoring
+        spec:
+          kubeStateMetricsConfig:
+            nodeSelector: {}
+      expectedError: 'spec.kubeStateMetricsConfig.nodeSelector: Invalid value: 0: spec.kubeStateMetricsConfig.nodeSelector in body should have at least 1 properties'
+
+    - name: Should reject KubeStateMetricsConfig with more than 10 nodeSelector entries
+      initial: |
+        apiVersion: config.openshift.io/v1alpha1
+        kind: ClusterMonitoring
+        spec:
+          kubeStateMetricsConfig:
+            nodeSelector:
+              key1: val1
+              key2: val2
+              key3: val3
+              key4: val4
+              key5: val5
+              key6: val6
+              key7: val7
+              key8: val8
+              key9: val9
+              key10: val10
+              key11: val11
+      expectedError: 'spec.kubeStateMetricsConfig.nodeSelector: Too many: 11: must have at most 10 items'
+
+    - name: Should reject KubeStateMetricsConfig with empty tolerations array
+      initial: |
+        apiVersion: config.openshift.io/v1alpha1
+        kind: ClusterMonitoring
+        spec:
+          kubeStateMetricsConfig:
+            tolerations: []
+      expectedError: 'spec.kubeStateMetricsConfig.tolerations: Invalid value: 0: spec.kubeStateMetricsConfig.tolerations in body should have at least 1 items'
+
+    - name: Should reject KubeStateMetricsConfig with more than 10 tolerations
+      initial: |
+        apiVersion: config.openshift.io/v1alpha1
+        kind: ClusterMonitoring
+        spec:
+          kubeStateMetricsConfig:
+            tolerations:
+              - key: "key1"
+                operator: "Exists"
+              - key: "key2"
+                operator: "Exists"
+              - key: "key3"
+                operator: "Exists"
+              - key: "key4"
+                operator: "Exists"
+              - key: "key5"
+                operator: "Exists"
+              - key: "key6"
+                operator: "Exists"
+              - key: "key7"
+                operator: "Exists"
+              - key: "key8"
+                operator: "Exists"
+              - key: "key9"
+                operator: "Exists"
+              - key: "key10"
+                operator: "Exists"
+              - key: "key11"
+                operator: "Exists"
+      expectedError: 'spec.kubeStateMetricsConfig.tolerations: Too many: 11: must have at most 10 items'
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml`
around lines 2231 - 2506, The test suite lacks negative cases for
kubeStateMetricsConfig.nodeSelector and kubeStateMetricsConfig.tolerations; add
reject tests that mirror existing patterns: (1) a case where
kubeStateMetricsConfig.nodeSelector is {} and assert an expectedError about
requiring at least one property; (2) a case where
kubeStateMetricsConfig.nodeSelector has an empty key or invalid value (if schema
enforces non-empty strings); (3) a case where kubeStateMetricsConfig.tolerations
is [] and assert an expectedError about requiring at least one item; and (4) a
case with too many tolerations (or duplicate tolerations) to assert the "Too
many" or "Duplicate value" errors; reference the kubeStateMetricsConfig,
nodeSelector, and tolerations keys when adding these new test entries consistent
with the surrounding test format.
config/v1alpha1/types_cluster_monitoring.go (1)

2502-2568: Consider extracting the shared scheduling/resources shape.

This block re-declares the same nodeSelector / resources / tolerations / topologySpreadConstraints shape already present in OpenShiftStateMetricsConfig. Pulling the common part into a shared struct would reduce drift in docs and validation.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@config/v1alpha1/types_cluster_monitoring.go` around lines 2502 - 2568,
KubeStateMetricsConfig duplicates the scheduling/resources fields from
OpenShiftStateMetricsConfig; extract a shared struct (e.g.,
PodSchedulingAndResources) containing NodeSelector, Resources, Tolerations, and
TopologySpreadConstraints and replace those fields in both
KubeStateMetricsConfig and OpenShiftStateMetricsConfig with a single
embedded/shared field of that new struct; update kubebuilder tags and JSON names
on the new struct to preserve validation (Min/MaxItems, listType/listMapKey,
optional, etc.) and adjust any references to the original fields in
code/comments to use the new struct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@config/v1alpha1/types_cluster_monitoring.go`:
- Around line 2601-2627: KubeStateMetricsLabelName currently only enforces
length; add CEL validation and tighten the max length to enforce valid
Kubernetes label keys or the wildcard "*". Update the type
KubeStateMetricsLabelName to change the MaxLength from 256 to 253 and add a
+kubebuilder:validation:XValidation CEL rule that allows either self == "*" or a
DNS1123 subdomain prefix + "/" + qualified name (or no prefix) using split('/')
and format.dns1123Subdomain.validate for the prefix and a regex for the name
(^[A-Za-z0-9]([-A-Za-z0-9_.]*[A-Za-z0-9])?$), with an appropriate message like
"must be a valid Kubernetes label key or *"; keep the existing MinLength and
ensure the new validation applies to KubeStateMetricsLabelName so Labels
[]KubeStateMetricsLabelName inherits it.

---

Nitpick comments:
In
`@config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml`:
- Around line 2231-2506: The test suite lacks negative cases for
kubeStateMetricsConfig.nodeSelector and kubeStateMetricsConfig.tolerations; add
reject tests that mirror existing patterns: (1) a case where
kubeStateMetricsConfig.nodeSelector is {} and assert an expectedError about
requiring at least one property; (2) a case where
kubeStateMetricsConfig.nodeSelector has an empty key or invalid value (if schema
enforces non-empty strings); (3) a case where kubeStateMetricsConfig.tolerations
is [] and assert an expectedError about requiring at least one item; and (4) a
case with too many tolerations (or duplicate tolerations) to assert the "Too
many" or "Duplicate value" errors; reference the kubeStateMetricsConfig,
nodeSelector, and tolerations keys when adding these new test entries consistent
with the surrounding test format.

In `@config/v1alpha1/types_cluster_monitoring.go`:
- Around line 2502-2568: KubeStateMetricsConfig duplicates the
scheduling/resources fields from OpenShiftStateMetricsConfig; extract a shared
struct (e.g., PodSchedulingAndResources) containing NodeSelector, Resources,
Tolerations, and TopologySpreadConstraints and replace those fields in both
KubeStateMetricsConfig and OpenShiftStateMetricsConfig with a single
embedded/shared field of that new struct; update kubebuilder tags and JSON names
on the new struct to preserve validation (Min/MaxItems, listType/listMapKey,
optional, etc.) and adjust any references to the original fields in
code/comments to use the new struct.
🪄 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: Pro Plus

Run ID: d4a84268-dde5-4503-a8ce-f4b5c2c51857

📥 Commits

Reviewing files that changed from the base of the PR and between d134762 and d1719c1.

⛔ Files ignored due to path filters (6)
  • 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.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 (3)
  • config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml
  • config/v1alpha1/types_cluster_monitoring.go
  • payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml

Comment thread config/v1alpha1/types_cluster_monitoring.go
Comment thread config/v1alpha1/types_cluster_monitoring.go Outdated
@danielmellado
danielmellado force-pushed the mon_4029_kube_state_metrics branch from d1719c1 to 57a8c79 Compare May 6, 2026 11:40
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 6, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
config/v1alpha1/types_cluster_monitoring.go (1)

2623-2634: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use Kubernetes label-key validation here instead of a broad regex.

The current rule still accepts invalid label keys like team/a/b or prefixes that are not valid DNS subdomains, so the CRD can persist additionalResourceLabels values that kube-state-metrics should reject downstream. Please validate the optional prefix and name segment separately (single /, DNS1123 prefix, qualified-name suffix) rather than allowing any mix of ._/-.

For Kubernetes CRD / kubebuilder validation, what CEL rule correctly validates a Kubernetes label key (optional DNS1123 prefix + "/" + qualified name) while also allowing the wildcard "*"?
🤖 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 2623 - 2634,
Replace the broad XValidation on type KubeStateMetricsLabelName with a CEL rule
that allows "*" or enforces an optional single "/" prefix validated as a DNS1123
subdomain and a name segment validated as a Kubernetes qualified name; for
example use a rule like: self == '*' || (self.split('/').size() == 1 &&
self.matches('^[A-Za-z0-9]([A-Za-z0-9_.-]*[A-Za-z0-9])?$')) ||
(self.split('/').size() == 2 &&
self.split('/')[0].matches('^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$')
&& self.split('/')[1].matches('^[A-Za-z0-9]([A-Za-z0-9_.-]*[A-Za-z0-9])?$'));
update the +kubebuilder:validation:XValidation line on the
KubeStateMetricsLabelName type accordingly.
🤖 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
`@payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml`:
- Around line 933-949: The current x-kubernetes-validations regex for
KubeStateMetricsLabelName (used by additionalResourceLabels.labels) allows
multiple slashes and mixed-case DNS prefixes; replace it with a stricter
validation that enforces at most one optional DNS subdomain prefix + '/' and a
single name segment that matches Kubernetes label-key syntax (DNS subdomain must
be lower-case DNS-1123 subdomain, name segment max 63 chars, begins/ends with
alphanumeric, and only contains alphanumerics, -, _, . in between). Update the
validation rule on the x-kubernetes-validations entry for
KubeStateMetricsLabelName so it rejects strings like "a/b/c" or
"Example.COM/key" by requiring a single optional lowercase DNS prefix and a
single valid name segment.

---

Duplicate comments:
In `@config/v1alpha1/types_cluster_monitoring.go`:
- Around line 2623-2634: Replace the broad XValidation on type
KubeStateMetricsLabelName with a CEL rule that allows "*" or enforces an
optional single "/" prefix validated as a DNS1123 subdomain and a name segment
validated as a Kubernetes qualified name; for example use a rule like: self ==
'*' || (self.split('/').size() == 1 &&
self.matches('^[A-Za-z0-9]([A-Za-z0-9_.-]*[A-Za-z0-9])?$')) ||
(self.split('/').size() == 2 &&
self.split('/')[0].matches('^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$')
&& self.split('/')[1].matches('^[A-Za-z0-9]([A-Za-z0-9_.-]*[A-Za-z0-9])?$'));
update the +kubebuilder:validation:XValidation line on the
KubeStateMetricsLabelName type accordingly.
🪄 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: 498ad17f-d61b-43f6-aa2f-a3df6fb77a07

📥 Commits

Reviewing files that changed from the base of the PR and between d1719c1 and 57a8c79.

⛔ Files ignored due to path filters (6)
  • 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.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 (3)
  • config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml
  • config/v1alpha1/types_cluster_monitoring.go
  • payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml

Comment thread payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml Outdated
@simonpasquier

Copy link
Copy Markdown

I'll wait on the replies on the Job/CronJob

I'm fine with @saschagrunert recommendations, it totally makes sense to follow Kubernetes conventions.

@danielmellado
danielmellado force-pushed the mon_4029_kube_state_metrics branch from 36cf1a4 to acf2876 Compare May 27, 2026 10:35

@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

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label May 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

@simonpasquier

Copy link
Copy Markdown

/test e2e-aws-ovn-hypershift-conformance

@simonpasquier simonpasquier left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

/lgtm

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

Overall the structure here looks good. Just a couple questions.

Comment thread config/v1alpha1/types_cluster_monitoring.go
Comment thread config/v1alpha1/types_cluster_monitoring.go
// Each entry specifies a resource name and a list of Kubernetes label names to expose.
// Use "*" in the labels list to expose all labels for a given resource.
// additionalResourceLabels is optional.
// When omitted, only the default set of resource labels is exposed.

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.

What is the "default set of resource labels"?

How would I as a user know if I need to configure this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

By "default set of resource labels", this refers to the labels kube-state-metrics exposes out of the box, which is effectively none for Jobs/CronJobs. kube-state-metrics does not expose Kubernetes object labels as metric labels by default; it only does so when explicitly configured via --metric-labels-allowlist.

I agree the wording is misleading. I'll rephrase to something like:

"When omitted, no additional Kubernetes object labels are exposed as metrics by kube-state-metrics beyond its built-in metric labels (e.g. namespace, job_name). Use this field to opt in to exposing specific Kubernetes labels as metric labels for the supported resource types."

Comment thread config/v1alpha1/types_cluster_monitoring.go
@danielmellado

Copy link
Copy Markdown
Contributor Author

Correct. kube-state-metrics always collects its built-in metrics for all resources it watches (Jobs, CronJobs, Pods, etc.) -- that part is not affected by this config. What additionalResourceLabels controls is whether Kubernetes object labels (the user-defined labels on the resource metadata, like app=frontend) are promoted into metric labels.

If you don't add an entry for, say, CronJob in additionalResourceLabels, kube-state-metrics still collects CronJob metrics, but none of the CronJob's Kubernetes labels appear as metric labels. Adding an entry lets you opt in to that per resource.

Adds a new KubeStateMetricsConfig struct to the ClusterMonitoring CRD,
allowing configuration of the kube-state-metrics agent (node selectors,
resources, tolerations, topology spread constraints). Includes
comprehensive integration tests for validation.

Signed-off-by: Daniel Mellado <dmellado@fedoraproject.org>
@danielmellado
danielmellado force-pushed the mon_4029_kube_state_metrics branch from acf2876 to 1a302b0 Compare June 2, 2026 10:04
@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 2, 2026

@everettraven everettraven 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 Jun 2, 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 Jun 2, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: everettraven, saschagrunert, simonpasquier

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

The pull request process is described 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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 2, 2026
@danielmellado

Copy link
Copy Markdown
Contributor Author

/retest-required

4 similar comments
@danielmellado

Copy link
Copy Markdown
Contributor Author

/retest-required

@danielmellado

Copy link
Copy Markdown
Contributor Author

/retest-required

@danielmellado

Copy link
Copy Markdown
Contributor Author

/retest-required

@danielmellado

Copy link
Copy Markdown
Contributor Author

/retest-required

@danielmellado

Copy link
Copy Markdown
Contributor Author

/verified by tests

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Jun 4, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@danielmellado: This PR has been marked as verified by tests.

Details

In response to this:

/verified by tests

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

openshift-ci Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

@danielmellado: all tests passed!

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.

@openshift-merge-bot
openshift-merge-bot Bot merged commit 332af6b into openshift:master Jun 4, 2026
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants