step-registry/tls-scanner: split CPU/memory into request and limit - #82865
Conversation
WalkthroughThe TLS scanner now supports separate CPU and memory requests and limits. Legacy combined variables remain as deprecated fallbacks. HyperShift guest scans support guest-specific resource overrides, and generated Pods use independent values. ChangesTLS scanner resource configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| documentation: "Deprecated: use SCANNER_MEM_GUEST_REQUEST and SCANNER_MEM_GUEST_LIMIT. Kept as fallback." | ||
| - name: SCANNER_CPU_GUEST_REQUEST | ||
| default: "1" | ||
| documentation: "CPU request for the scanner pod on the guest cluster. Falls back to SCANNER_CPU_GUEST if unset." |
There was a problem hiding this comment.
No need to add the Falls back to SCANNER_CPU_GUEST if unset. at the end.
There was a problem hiding this comment.
Done — removed the "Falls back to..." text from all split var docs (both management and guest).
AI-generated. Review for accuracy.
| scanner_cpu_request="${SCANNER_CPU_GUEST_REQUEST:-${SCANNER_CPU_GUEST:-1}}" | ||
| scanner_cpu_limit="${SCANNER_CPU_GUEST_LIMIT:-${SCANNER_CPU_GUEST:-1}}" | ||
| scanner_mem_request="${SCANNER_MEM_GUEST_REQUEST:-${SCANNER_MEMORY_GUEST:-2Gi}}" | ||
| scanner_mem_limit="${SCANNER_MEM_GUEST_LIMIT:-${SCANNER_MEMORY_GUEST:-2Gi}}" |
There was a problem hiding this comment.
the defaults are already defined in ci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-ref.yaml
no need to have things like :-1 :-2Gi
There was a problem hiding this comment.
Done — removed hardcoded :-1 / :-2Gi defaults. The ref yaml already sets them.
AI-generated. Review for accuracy.
![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 `@ci-operator/step-registry/tls/scanner/run/tls-scanner-run-ref.yaml`:
- Around line 22-33: Remove the non-empty defaults from the split scanner
resource declarations so legacy override variables remain effective through the
existing fallback expressions. Update all affected declarations in
ci-operator/step-registry/tls/scanner/run/tls-scanner-run-ref.yaml lines 22-33,
ci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-ref.yaml
lines 27-38, and
ci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-ref.yaml
lines 45-56; preserve the documentation and fallback behavior for SCANNER_CPU,
SCANNER_MEMORY, and the guest equivalents.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c069b845-57ca-465c-91ff-c7da64c30a4b
📒 Files selected for processing (3)
ci-operator/step-registry/tls/scanner/hypershift-run/tls-scanner-hypershift-run-ref.yamlci-operator/step-registry/tls/scanner/run/tls-scanner-run-commands.shci-operator/step-registry/tls/scanner/run/tls-scanner-run-ref.yaml
83f348c to
4b8a7c4
Compare
|
/pj-rehearse periodic-ci-openshift-tls-scanner-main-periodic-tls13-adherence AI-generated. Review for accuracy. |
|
/pj-rehearse periodic-ci-openshift-tls-scanner-main-periodic-default-tls AI-generated. Review for accuracy. |
|
/pj-rehearse periodic-ci-openshift-tls-scanner-main-periodic-pqc-readiness AI-generated. Review for accuracy. |
|
/pj-rehearse periodic-ci-openshift-tls-scanner-main-periodic-pqc-readiness-hypershift AI-generated. Review for accuracy. |
|
@redhat-chai-bot: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
3 similar comments
|
@redhat-chai-bot: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@redhat-chai-bot: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@redhat-chai-bot: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@machine424: The following tests failed, say
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. |
|
/cc @richardsonnick |
Align the step-registry env vars with the upstream tls-scanner change (openshift/tls-scanner#87) that splits SCANNER_CPU / SCANNER_MEMORY into separate request and limit variables. New variables (both run and hypershift-run refs): SCANNER_CPU_REQUEST / SCANNER_CPU_LIMIT (default 4 / 4) SCANNER_MEM_REQUEST / SCANNER_MEM_LIMIT (default 4Gi / 4Gi) New guest-cluster variables (hypershift-run ref only): SCANNER_CPU_GUEST_REQUEST / SCANNER_CPU_GUEST_LIMIT (default 1 / 1) SCANNER_MEM_GUEST_REQUEST / SCANNER_MEM_GUEST_LIMIT (default 2Gi / 2Gi) The original SCANNER_CPU, SCANNER_MEMORY, SCANNER_CPU_GUEST, and SCANNER_MEMORY_GUEST are preserved as deprecated fallbacks so that existing CI configs continue to work without changes. This unblocks openshift#82553 (add tls-scanner to cluster-monitoring-operator e2e) which uses the new split vars to set a lower CPU request for scheduling while keeping a higher burst limit.
4b8a7c4 to
9b231b8
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
[REHEARSALNOTIFIER]
A total of 119 jobs have been affected by this change. The above listing is non-exhaustive and limited to 25 jobs. A full list of affected jobs can be found here Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: machine424, richardsonnick 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 |
openshift-ci
Bot
added
the
approved
|
/pj-rehearse ack |
|
@machine424: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift-merge-bot
Bot
added
the
rehearsals-ack
|
Our tests which use the old names are now failing. Apparently the fallback doesn't work since our pods which should have 0.5 core and 1GB now have requests of 4 cores and 4 GB and can't schedule. |
That highlights the importance of decoupling requests and limits the hard way. Hopefully, #83145 will resolve the issue for the others. |
4 participants
Align the step-registry env vars with the upstream tls-scanner change
(openshift/tls-scanner#87) that splits SCANNER_CPU / SCANNER_MEMORY
into separate request and limit variables.
New variables (both run and hypershift-run refs):
SCANNER_CPU_REQUEST / SCANNER_CPU_LIMIT (default 4 / 4)
SCANNER_MEM_REQUEST / SCANNER_MEM_LIMIT (default 4Gi / 4Gi)
New guest-cluster variables (hypershift-run ref only):
SCANNER_CPU_GUEST_REQUEST / SCANNER_CPU_GUEST_LIMIT (default 1 / 1)
SCANNER_MEM_GUEST_REQUEST / SCANNER_MEM_GUEST_LIMIT (default 2Gi / 2Gi)
The original SCANNER_CPU, SCANNER_MEMORY, SCANNER_CPU_GUEST, and
SCANNER_MEMORY_GUEST are preserved as deprecated fallbacks so that
existing CI configs continue to work without changes.
This unblocks #82553 (add tls-scanner to
cluster-monitoring-operator e2e) which uses the new split vars to set
a lower CPU request for scheduling while keeping a higher burst limit.
Summary by CodeRabbit
The TLS scanner CI steps now support separate CPU and memory requests and limits for scanner and HyperShift guest-cluster pods. This enables lower resource requests with higher limits for cluster-monitoring-operator e2e integration.
Existing
SCANNER_CPU,SCANNER_MEMORY,SCANNER_CPU_GUEST, andSCANNER_MEMORY_GUESTvariables remain available as deprecated fallbacks. Defaults preserve current scanner resources and define guest-cluster resources.