MON-4036: Add TelemeterClientConfig to ClusterMonitoring API#2755
MON-4036: Add TelemeterClientConfig to ClusterMonitoring API#2755danielmellado wants to merge 1 commit intoopenshift:masterfrom
Conversation
Migrate the telemeter-client configmap settings to a CRD field within ClusterMonitoringSpec in config/v1alpha1. The new TelemeterClientConfig 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 Signed-off-by: Daniel Mellado <dmellado@fedoraproject.org>
|
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-4036 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 Telemeter Client configuration support to the ClusterMonitoring API. A new TelemeterClientConfig type is introduced with fields for NodeSelector, Resources, Tolerations, and TopologySpreadConstraints. This type is added to ClusterMonitoringSpec. The CRD manifest is updated to include the telemeterClientConfig field with schema definitions including default values, constraints on resource names and limits, and validation rules. New test cases validate TelemeterClientConfig behavior including valid creation scenarios, error handling, and rejection of invalid configurations. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
openshift-ci
bot
added
the
size/XXL
Review Summary by QodoAdd TelemeterClientConfig to ClusterMonitoring API for pod scheduling and resources
WalkthroughsDescription• Add TelemeterClientConfig struct to ClusterMonitoringSpec API • Support pod scheduling via nodeSelector, tolerations, and topologySpreadConstraints • Enable resource management with resources field for CPU/memory constraints • Include comprehensive validation rules and test coverage for new configuration Diagramflowchart LR
A["ClusterMonitoringSpec"] -->|adds field| B["TelemeterClientConfig"]
B -->|supports| C["nodeSelector"]
B -->|supports| D["resources"]
B -->|supports| E["tolerations"]
B -->|supports| F["topologySpreadConstraints"]
C -->|controls| G["Pod Scheduling"]
D -->|manages| H["Resource Limits"]
E -->|handles| I["Taint Tolerance"]
F -->|distributes| J["Topology Domains"]
File Changes1. config/v1alpha1/types_cluster_monitoring.go
|
Code Review by Qodo
1. TelemeterClientConfig MinProperties undocumented
|
|
[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 |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| // TelemeterClientConfig provides configuration options for the Telemeter Client component | ||
| // that runs in the `openshift-monitoring` namespace. The Telemeter Client collects selected | ||
| // monitoring metrics and forwards them to Red Hat for telemetry purposes. | ||
| // Use this configuration to control pod scheduling and resource allocation. | ||
| // +kubebuilder:validation:MinProperties=1 | ||
| type TelemeterClientConfig struct { | ||
| // nodeSelector defines the nodes on which the Pods are scheduled. |
There was a problem hiding this comment.
1. telemeterclientconfig minproperties undocumented 📘 Rule violation ✓ Correctness
TelemeterClientConfig has +kubebuilder:validation:MinProperties=1, but the type comment does not document that the object must not be empty (i.e., at least one field must be set). This violates the requirement that validation markers be fully documented, potentially confusing API consumers about valid/invalid configurations.
Agent Prompt
## Issue description
`TelemeterClientConfig` includes the validation marker `+kubebuilder:validation:MinProperties=1`, but the type-level comment does not document the resulting constraint that the object must not be empty.
## Issue Context
Compliance requires that every validation marker applied to an API field/type be fully documented in comments so users understand constraints without inspecting generated schema.
## Fix Focus Areas
- config/v1alpha1/types_cluster_monitoring.go[575-581]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![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)
580-832: Add failure cases fornodeSelectorandtolerationsbounds.The new schema also enforces
min/maxPropertiesonnodeSelectorandmin/maxItemsontolerations, but this block only exercises negative paths forresourcesandtopologySpreadConstraints. A couple of explicit failures here would make regressions in those two branches much harder to miss.Example additions
+ - name: Should reject TelemeterClientConfig with empty nodeSelector + initial: | + apiVersion: config.openshift.io/v1alpha1 + kind: ClusterMonitoring + spec: + telemeterClientConfig: + nodeSelector: {} + expectedError: 'spec.telemeterClientConfig.nodeSelector: Invalid value: 0: spec.telemeterClientConfig.nodeSelector in body should have at least 1 properties' + + - name: Should reject TelemeterClientConfig with empty tolerations array + initial: | + apiVersion: config.openshift.io/v1alpha1 + kind: ClusterMonitoring + spec: + telemeterClientConfig: + tolerations: [] + expectedError: 'spec.telemeterClientConfig.tolerations: Invalid value: 0: spec.telemeterClientConfig.tolerations in body should have at least 1 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 580 - 832, Add negative test cases exercising nodeSelector and tolerations bounds: include tests named e.g. "Should reject TelemeterClientConfig with empty nodeSelector" and "Should reject TelemeterClientConfig with too many nodeSelector properties" that set spec.telemeterClientConfig.nodeSelector to {} and to an object exceeding the schema maxProperties respectively, and include tests "Should reject TelemeterClientConfig with empty tolerations array" (set tolerations: []) and "Should reject TelemeterClientConfig with too many tolerations" (provide more than the allowed maxItems). Ensure each test uses the same ClusterMonitoring YAML structure as other cases and sets expectedError strings matching the schema validation messages for min/maxProperties and min/maxItems on nodeSelector and tolerations so failures are asserted.
🤖 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-832: Add negative test cases exercising nodeSelector and
tolerations bounds: include tests named e.g. "Should reject
TelemeterClientConfig with empty nodeSelector" and "Should reject
TelemeterClientConfig with too many nodeSelector properties" that set
spec.telemeterClientConfig.nodeSelector to {} and to an object exceeding the
schema maxProperties respectively, and include tests "Should reject
TelemeterClientConfig with empty tolerations array" (set tolerations: []) and
"Should reject TelemeterClientConfig with too many tolerations" (provide more
than the allowed maxItems). Ensure each test uses the same ClusterMonitoring
YAML structure as other cases and sets expectedError strings matching the schema
validation messages for min/maxProperties and min/maxItems on nodeSelector and
tolerations so failures are asserted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 897b5361-c81f-468e-bb44-621465732eb1
⛔ Files ignored due to path filters (5)
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*
📒 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. |
2 participants
Migrate the telemeter-client configmap settings to a CRD field within
ClusterMonitoringSpec in config/v1alpha1. The new TelemeterClientConfig
struct supports:
Signed-off-by: Daniel Mellado dmellado@fedoraproject.org