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. |
📝 WalkthroughWalkthroughAdds a new OpenShiftStateMetricsConfig type and field to ClusterMonitoringSpec (v1alpha1) with NodeSelector, Resources, Tolerations, and TopologySpreadConstraints, and applies MinProperties=1. Generates deepcopy and SwaggerDoc functions for the new type. Updates the CRD manifest to expose openShiftStateMetricsConfig with detailed schema validation for nested fields, quantities, and array sizes (field appears duplicated in the diff). Expands ClusterMonitoringConfig.yaml tests with multiple valid and rejection cases for OpenShiftStateMetricsConfig. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
![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
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
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Changes look good to me - great work!
/lgtm
|
Scheduling tests matching the |
openshift-ci
bot
added
the
approved
|
/retest-required |
3 similar comments
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/verified by ci |
openshift-ci-robot
added
the
verified
|
@danielmellado: This PR has been marked as verified by 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. |
openshift-merge-robot
added
the
needs-rebase
danielmellado
force-pushed
the
openshift-metrics-config
branch
from
bd28c7e to
ccf008e
Compare
openshift-ci-robot
removed
the
verified
openshift-merge-robot
removed
the
needs-rebase
|
@everettraven I ahd to rebase this in the middle of the merge xD, mind approving it again? Thanks! |
|
/lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven 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 |
|
/retest-required |
|
/verified by tests |
openshift-ci-robot
added
the
verified
|
@danielmellado: This PR has been marked as verified by 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. |
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
ccf008e to
4cf5fe1
Compare
openshift-ci-robot
removed
the
verified
|
New changes are detected. LGTM label has been removed. |
|
@everettraven had to rebase yet another time, let's see if we can finally merge this, thanks! xD |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml (1)
580-817: Add standalonenodeSelector/tolerationscoverage.
nodeSelectorandtolerationsare only covered through the "all fields" fixture right now. That leaves a small gap where either field could regress when used on its own without this suite noticing. Please add at least one single-field happy-path case for each, plus focused rejects fornodeSelector: {}andtolerations: [].🤖 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 580 - 817, Add standalone happy-path test cases and focused reject cases for nodeSelector and tolerations inside the ClusterMonitoring openShiftStateMetricsConfig tests: add a "Should be able to create OpenShiftStateMetricsConfig with only nodeSelector" test that sets spec.openShiftStateMetricsConfig.nodeSelector (e.g., kubernetes.io/os: linux) with an identical expected document, a "Should be able to create OpenShiftStateMetricsConfig with only tolerations" test that sets spec.openShiftStateMetricsConfig.tolerations (e.g., one toleration with key/operator/effect) with identical expected, plus negative tests "Should reject OpenShiftStateMetricsConfig with empty nodeSelector" where initial uses openShiftStateMetricsConfig.nodeSelector: {} and expectedError indicates missing properties, and "Should reject OpenShiftStateMetricsConfig with empty tolerations array" where initial uses tolerations: [] and expectedError matches the existing pattern for empty arrays; ensure you use the existing symbols ClusterMonitoring, openShiftStateMetricsConfig, nodeSelector, and tolerations so the new fixtures mirror the style and validation messages of the other test cases.
🤖 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 580-817: Add standalone happy-path test cases and focused reject
cases for nodeSelector and tolerations inside the ClusterMonitoring
openShiftStateMetricsConfig tests: add a "Should be able to create
OpenShiftStateMetricsConfig with only nodeSelector" test that sets
spec.openShiftStateMetricsConfig.nodeSelector (e.g., kubernetes.io/os: linux)
with an identical expected document, a "Should be able to create
OpenShiftStateMetricsConfig with only tolerations" test that sets
spec.openShiftStateMetricsConfig.tolerations (e.g., one toleration with
key/operator/effect) with identical expected, plus negative tests "Should reject
OpenShiftStateMetricsConfig with empty nodeSelector" where initial uses
openShiftStateMetricsConfig.nodeSelector: {} and expectedError indicates missing
properties, and "Should reject OpenShiftStateMetricsConfig with empty
tolerations array" where initial uses tolerations: [] and expectedError matches
the existing pattern for empty arrays; ensure you use the existing symbols
ClusterMonitoring, openShiftStateMetricsConfig, nodeSelector, and tolerations so
the new fixtures mirror the style and validation messages of the other test
cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f0205f9d-9f1d-49f5-97a4-9d76d5179cde
⛔ Files ignored due to path filters (6)
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1alpha1/zz_generated.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (3)
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlconfig/v1alpha1/types_cluster_monitoring.gopayload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml
|
@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. |
4 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