CM-1033: Add performance tuning parameters to the supported args whitelist#439
CM-1033: Add performance tuning parameters to the supported args whitelist#439chiragkyal wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@chiragkyal: This pull request references CM-1033 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 "5.0.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. |
WalkthroughAdds four flag-name constants for controller performance tuning ( ChangesBurst/QPS Validation for Controller Override Args
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chiragkyal 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 |
Add --concurrent-workers, --kube-api-qps, --kube-api-burst, and --max-concurrent-challenges to the controller's supported overrideArgs, allowing to tune cert-manager throughput via the CertManager CR. Signed-off-by: chiragkyal <ckyal@redhat.com>
openshift-ci
Bot
removed
the
needs-rebase
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/controller/certmanager/deployment_overrides_validation.go`:
- Around line 146-148: The validateBurstQPS function silently ignores parsing
errors by returning nil when errQ or errB are not nil, which allows malformed
numeric inputs to bypass validation. Instead of returning nil when either error
is non-nil, the function should return an error describing what failed to parse,
so that invalid QPS or burst values are properly rejected rather than silently
accepted. This ensures that parsing failures are not ignored and callers can
detect when the values are invalid.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 40911c5e-5f7a-4b04-afff-b02d6dba7f1d
📒 Files selected for processing (3)
pkg/controller/certmanager/deployment_overrides_validation.gopkg/controller/certmanager/deployment_overrides_validation_test.gotest/e2e/overrides_test.go
| if errQ != nil || errB != nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Do not silently accept unparsable kube-api QPS/burst values.
validateBurstQPS currently returns nil when parsing fails, so malformed numeric inputs bypass validation and can still be applied.
Suggested fix
qps, errQ := strconv.ParseFloat(qpsStr, 64)
burst, errB := strconv.ParseFloat(burstStr, 64)
- if errQ != nil || errB != nil {
- return nil
- }
+ if errQ != nil {
+ return fmt.Errorf("validation failed: %s must be numeric, got %q: %w", argKubeAPIQPS, qpsStr, errQ)
+ }
+ if errB != nil {
+ return fmt.Errorf("validation failed: %s must be numeric, got %q: %w", argKubeAPIBurst, burstStr, errB)
+ }As per coding guidelines, Go security (prodsec-skills): “Never ignore error returns”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if errQ != nil || errB != nil { | |
| return nil | |
| } | |
| if errQ != nil { | |
| return fmt.Errorf("validation failed: %s must be numeric, got %q: %w", argKubeAPIQPS, qpsStr, errQ) | |
| } | |
| if errB != nil { | |
| return fmt.Errorf("validation failed: %s must be numeric, got %q: %w", argKubeAPIBurst, burstStr, errB) | |
| } |
🤖 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 `@pkg/controller/certmanager/deployment_overrides_validation.go` around lines
146 - 148, The validateBurstQPS function silently ignores parsing errors by
returning nil when errQ or errB are not nil, which allows malformed numeric
inputs to bypass validation. Instead of returning nil when either error is
non-nil, the function should return an error describing what failed to parse, so
that invalid QPS or burst values are properly rejected rather than silently
accepted. This ensures that parsing failures are not ignored and callers can
detect when the values are invalid.
Source: Coding guidelines
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/controller/certmanager/deployment_overrides_validation.go (1)
147-149:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDo not silently accept unparsable kube-api QPS/burst values.
validateBurstQPScurrently returnsnilwhen parsing fails, so malformed numeric inputs bypass validation and can still be applied. As per coding guidelines, Go security (prodsec-skills): "Never ignore error returns".🐛 Proposed fix
qps, errQ := strconv.ParseFloat(qpsStr, 64) burst, errB := strconv.ParseFloat(burstStr, 64) - if errQ != nil || errB != nil { - return nil - } + if errQ != nil { + return fmt.Errorf("validation failed: %s must be numeric, got %q: %w", argKubeAPIQPS, qpsStr, errQ) + } + if errB != nil { + return fmt.Errorf("validation failed: %s must be numeric, got %q: %w", argKubeAPIBurst, burstStr, errB) + }🤖 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 `@pkg/controller/certmanager/deployment_overrides_validation.go` around lines 147 - 149, The validateBurstQPS function silently accepts unparsable QPS/burst values by returning nil when parsing errors occur. When errQ or errB are not nil (indicating parsing failures for QPS or burst values), the current code returns nil instead of propagating the error. Fix this by returning an error when either errQ or errB is not nil, ensuring that malformed numeric inputs are properly rejected during validation rather than being silently accepted.Sources: Coding guidelines, Linters/SAST tools
🤖 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.
Duplicate comments:
In `@pkg/controller/certmanager/deployment_overrides_validation.go`:
- Around line 147-149: The validateBurstQPS function silently accepts unparsable
QPS/burst values by returning nil when parsing errors occur. When errQ or errB
are not nil (indicating parsing failures for QPS or burst values), the current
code returns nil instead of propagating the error. Fix this by returning an
error when either errQ or errB is not nil, ensuring that malformed numeric
inputs are properly rejected during validation rather than being silently
accepted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cc8a0c42-90b9-41de-9c77-e269dce0d78b
📒 Files selected for processing (3)
pkg/controller/certmanager/deployment_overrides_validation.gopkg/controller/certmanager/deployment_overrides_validation_test.gotest/e2e/overrides_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controller/certmanager/deployment_overrides_validation_test.go
- test/e2e/overrides_test.go
|
|
||
| // validateBurstQPS ensures that if both --kube-api-burst and --kube-api-qps are | ||
| // specified, burst is not less than qps (cert-manager crashes if burst < qps). | ||
| func validateBurstQPS(argMap map[string]string) error { |
There was a problem hiding this comment.
This validation is present on the operand side
So, this validation is optional here
There was a problem hiding this comment.
Still good to have, I think.
|
@chiragkyal: 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. |
bharath-b-rh
left a comment
bharath-b-rh
left a comment
There was a problem hiding this comment.
LGTM, except for few suggestions.
| ) | ||
|
|
||
| const ( | ||
| argConcurrentWorkers = "--concurrent-workers" |
There was a problem hiding this comment.
Needs gofmt fix here.
| if errQ != nil || errB != nil { | ||
| return nil | ||
| } |
|
|
||
| const ( | ||
| argConcurrentWorkers = "--concurrent-workers" | ||
| argKubeAPIQPS = "--kube-api-qps" |
There was a problem hiding this comment.
Are these constants necessary? They are not made use of in uts or e2es.
| argConcurrentWorkers, | ||
| // Maximum queries-per-second to the Kubernetes API server. (default 20) | ||
| argKubeAPIQPS, | ||
| // Maximum burst queries-per-second to the Kubernetes API server. Must be >= kube-api-qps. (default 50) | ||
| argKubeAPIBurst, | ||
| // Maximum number of challenges that can be scheduled as 'processing' at once. (default 60) | ||
| argMaxConcurrentChallenges, | ||
| } |
There was a problem hiding this comment.
nit: It would be good to have some sanity checks for the args. It's not there for others, but good to start from these. The operator doesn't monitor the pod status once after deployment resource creation, and if any of these args have invalid values and causes CrashLoopBackOff error stays in that state until someone notices and fixes it. There is liveness probe in-place, so no downtime is expected though.
|
|
||
| // validateBurstQPS ensures that if both --kube-api-burst and --kube-api-qps are | ||
| // specified, burst is not less than qps (cert-manager crashes if burst < qps). | ||
| func validateBurstQPS(argMap map[string]string) error { |
There was a problem hiding this comment.
Still good to have, I think.
| }) | ||
| }) | ||
|
|
||
| Context("When adding kube-api-burst less than kube-api-qps", func() { |
There was a problem hiding this comment.
Could we add a happy path test case too?
3 participants
Description
Adds
--concurrent-workers,--kube-api-qps,--kube-api-burst, and--max-concurrent-challengesto the cert-manager controller's supported overrideArgs whitelist, enabling users to tune cert-manager performance via the CertManager CR without relying on unsupportedConfigOverrides.Changes
validateBurstQPS()that checksburst >= qpswhen both are specifiedSummary by CodeRabbit