MON-4035: Add ThanosQuerierConfig to ClusterMonitoring API#2769
MON-4035: Add ThanosQuerierConfig to ClusterMonitoring API#2769danielmellado wants to merge 1 commit intoopenshift:masterfrom
Conversation
Migrate the thanos-querier configmap settings to a CRD field within ClusterMonitoringSpec in config/v1alpha1. The new ThanosQuerierConfig struct supports: - nodeSelector: pod scheduling to specific nodes - resources: compute resource requests and limits - tolerations: pod tolerations for scheduling - topologySpreadConstraints: pod distribution across topology domains Also removes per-field positive-quantity CEL checks from ContainerResource request/limit fields to stay within the Kubernetes CRD CEL validation cost budget (StaticEstimatedCRDCostLimit) with 7 component configs at MaxItems=10. Signed-off-by: Daniel Mellado <dmellado@fedoraproject.org>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@danielmellado: This pull request references MON-4035 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 either version "4.22." or "openshift-4.22.", but it targets "openshift-5.0" instead. 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. |
|
Hello @danielmellado! Some important instructions when contributing to openshift/api: |
📝 WalkthroughWalkthroughThe changes add a new ThanosQuerierConfig field to ClusterMonitoringSpec to enable configuration of a Thanos Querier component in openshift-monitoring. The new type includes nodeSelector, resources, tolerations, and topologySpreadConstraints fields with corresponding validation rules. Positive-quantity validation constraints previously enforced on ContainerResource request and limit fields were removed. Supporting changes include generated deep copy methods, swagger documentation, and comprehensive test scenarios validating the new configuration options and rejection of invalid inputs. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.11.3)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
|
[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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml (1)
790-971: Add one explicit regression case for the relaxed quantity checks.If the non-positive
ContainerResourcerelaxation is intentional, please add one representativethanosQuerierConfig.resourcescreate case for it. The new coverage only exercises list semantics andlimit >= request, so that contract change is still implicit.🤖 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 790 - 971, Add a regression test entry for the relaxed non-positive ContainerResource check by inserting a new test case in the same YAML list that validates thanosQuerierConfig.resources accepts a non-positive quantity (e.g., request: "0" and limit: "0"); the test should include a name like "Should be able to create ThanosQuerierConfig with non-positive resources" and provide identical initial and expected manifests under spec.thanosQuerierConfig.resources to assert create succeeds with request "0" and limit "0". Ensure the added case follows the same structure as existing tests (name, initial, expected) and targets thanosQuerierConfig.resources so the change in semantics is explicitly covered.payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml (1)
4656-4702: Consider disallowing negative quantities in the regex to match the documented contract.
limit/requestdescriptions state values must be> 0, but the current pattern accepts a leading-. Tightening the regex keeps validation cheap while aligning behavior with docs.Suggested diff
- pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ + pattern: ^(\+)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ ... - pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ + pattern: ^(\+)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$🤖 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 4656 - 4702, The regex for resource quantities in the limit and request fields currently allows a leading '-' which contradicts the documented "must be greater than 0" constraint; update the pattern in both the limit and request definitions (the pattern under the limit and request properties) to remove the optional negative sign (e.g., change ^(\+|-)?... to ^(\+)?... or simply drop the sign group) so negative values are rejected while preserving the rest of the quantity syntax and x-kubernetes-int-or-string behavior.
🤖 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 790-971: Add a regression test entry for the relaxed non-positive
ContainerResource check by inserting a new test case in the same YAML list that
validates thanosQuerierConfig.resources accepts a non-positive quantity (e.g.,
request: "0" and limit: "0"); the test should include a name like "Should be
able to create ThanosQuerierConfig with non-positive resources" and provide
identical initial and expected manifests under
spec.thanosQuerierConfig.resources to assert create succeeds with request "0"
and limit "0". Ensure the added case follows the same structure as existing
tests (name, initial, expected) and targets thanosQuerierConfig.resources so the
change in semantics is explicitly covered.
In
`@payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml`:
- Around line 4656-4702: The regex for resource quantities in the limit and
request fields currently allows a leading '-' which contradicts the documented
"must be greater than 0" constraint; update the pattern in both the limit and
request definitions (the pattern under the limit and request properties) to
remove the optional negative sign (e.g., change ^(\+|-)?... to ^(\+)?... or
simply drop the sign group) so negative values are rejected while preserving the
rest of the quantity syntax and x-kubernetes-int-or-string behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: af668436-5d4c-40ff-9589-2615607272cd
⛔ Files ignored due to path filters (4)
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/**openapi/openapi.jsonis 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: 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. |
Migrate the thanos-querier configmap settings to a CRD field within
ClusterMonitoringSpec in config/v1alpha1. The new ThanosQuerierConfig
struct supports:
Also removes per-field positive-quantity CEL checks from
ContainerResource request/limit fields to stay within the Kubernetes
CRD CEL validation cost budget (StaticEstimatedCRDCostLimit) with
7 component configs at MaxItems=10.
Signed-off-by: Daniel Mellado dmellado@fedoraproject.org