MON-4033: Add OpenShiftMetricsConfig#2726
MON-4033: Add OpenShiftMetricsConfig#2726danielmellado wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@danielmellado: This pull request references MON-4033 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. 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. |
📝 WalkthroughWalkthroughThis pull request introduces OpenShiftStateMetricsConfig as a new configuration type within the ClusterMonitoring API (v1alpha1). The change adds the OpenShiftStateMetricsConfig field to ClusterMonitoringSpec with support for NodeSelector, Resources, Tolerations, and TopologySpreadConstraints. Corresponding deepcopy and swagger documentation methods were generated to support the new type. The CRD manifest was updated with detailed validation constraints for the new configuration field, including restrictions on resource quantities, array sizes, and nested properties. Comprehensive test cases were added to validate both successful configurations and validation failure scenarios. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
|
Hello @danielmellado! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XXL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml (1)
578-591: Make the duplicate topology spread assertion more specific.Line 591 uses a very broad substring (
"Duplicate value"), which can hide failures from the wrong field/path.🔧 Suggested test assertion tightening
- expectedError: "Duplicate value" + expectedError: 'spec.openShiftStateMetricsConfig.topologySpreadConstraints[1]: Duplicate value'🤖 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 578 - 591, The test "Should reject OpenShiftStateMetricsConfig with duplicate topologySpreadConstraints" is asserting a too-generic error ("Duplicate value"); update the expectedError to assert the precise validation message and path (for example include spec.openShiftStateMetricsConfig.topologySpreadConstraints and the duplicate topologyKey/value) so the test verifies the exact field/path that failed (target the openShiftStateMetricsConfig.topologySpreadConstraints duplicate topologyKey error string rather than the broad "Duplicate value").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml`:
- Around line 578-591: The test "Should reject OpenShiftStateMetricsConfig with
duplicate topologySpreadConstraints" is asserting a too-generic error
("Duplicate value"); update the expectedError to assert the precise validation
message and path (for example include
spec.openShiftStateMetricsConfig.topologySpreadConstraints and the duplicate
topologyKey/value) so the test verifies the exact field/path that failed (target
the openShiftStateMetricsConfig.topologySpreadConstraints duplicate topologyKey
error string rather than the broad "Duplicate value").
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (3)
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1alpha1/zz_generated.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**
📒 Files selected for processing (5)
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlconfig/v1alpha1/types_cluster_monitoring.goconfig/v1alpha1/zz_generated.deepcopy.goconfig/v1alpha1/zz_generated.swagger_doc_generated.gopayload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml
This commit adds configuration options for the openshift-state-metrics agent in config/v1alpha1 The new struct supports: - nodeSelector: node scheduling constraints - resources: compute resource requests and limits - tolerations: pod tolerations - topologySpreadConstraints: pod distribution across topology domains Signed-off-by: Daniel Mellado <dmellado@fedoraproject.org>
danielmellado
force-pushed
the
openshift-metrics-config
branch
from
8412fe0 to
bd28c7e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml (1)
642-687: Consider adding boundary tests fornodeSelectorandtolerations.You validate
resourcesandtopologySpreadConstraintsthoroughly, butnodeSelector(min/maxProperties) andtolerations(min/maxItems) constraints are not explicitly exercised yet.🤖 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 642 - 687, Add explicit boundary test cases for openShiftStateMetricsConfig.nodeSelector and openShiftStateMetricsConfig.tolerations: add tests that cover nodeSelector min/maxProperties (one case with zero properties if min>0, one with exactly the max allowed properties, and one exceeding max) and tolerations min/maxItems (one with zero items if min>0, one with exactly the max allowed items, and one exceeding max). Use clear test names (e.g., "Should accept OpenShiftStateMetricsConfig with nodeSelector at maxProperties", "Should reject OpenShiftStateMetricsConfig with nodeSelector exceeding maxProperties", "Should accept OpenShiftStateMetricsConfig with tolerations at maxItems", "Should reject OpenShiftStateMetricsConfig with tolerations exceeding maxItems") and populate the initial/expected YAML for openShiftStateMetricsConfig.nodeSelector and openShiftStateMetricsConfig.tolerations accordingly so validation exercises both min/max constraints.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml`:
- Around line 642-687: Add explicit boundary test cases for
openShiftStateMetricsConfig.nodeSelector and
openShiftStateMetricsConfig.tolerations: add tests that cover nodeSelector
min/maxProperties (one case with zero properties if min>0, one with exactly the
max allowed properties, and one exceeding max) and tolerations min/maxItems (one
with zero items if min>0, one with exactly the max allowed items, and one
exceeding max). Use clear test names (e.g., "Should accept
OpenShiftStateMetricsConfig with nodeSelector at maxProperties", "Should reject
OpenShiftStateMetricsConfig with nodeSelector exceeding maxProperties", "Should
accept OpenShiftStateMetricsConfig with tolerations at maxItems", "Should reject
OpenShiftStateMetricsConfig with tolerations exceeding maxItems") and populate
the initial/expected YAML for openShiftStateMetricsConfig.nodeSelector and
openShiftStateMetricsConfig.tolerations accordingly so validation exercises both
min/max constraints.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (3)
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1alpha1/zz_generated.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**
📒 Files selected for processing (5)
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlconfig/v1alpha1/types_cluster_monitoring.goconfig/v1alpha1/zz_generated.deepcopy.goconfig/v1alpha1/zz_generated.swagger_doc_generated.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/zz_generated.swagger_doc_generated.go
|
@danielmellado: all tests passed! 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. |
2 participants
This commit adds configuration options for the openshift-state-metrics
agent in config/v1alpha1
The new struct supports:
Signed-off-by: Daniel Mellado dmellado@fedoraproject.org