Fix tls-scanner-run cleanup crash and Failed pod handling - #82522
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jzding 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 |
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe TLS scanner now snapshots cleanup variables before installing the EXIT trap and evaluates pod phases, termination codes, and JUnit artifacts before deciding whether to describe, wait, or fail. ChangesTLS scanner execution
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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-commands.sh`:
- Around line 103-110: Update run_tls_scan and cleanup so NAMESPACE and
OWNS_NAMESPACE are copied into durable snapshot variables before registering the
EXIT trap. Have cleanup use those snapshots rather than defaulting the locals
after they go out of scope, while retaining defaults only when initialization
never occurred; preserve deletion of the owned namespace versus the targeted
scanner pod based on the captured ownership state.
🪄 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: Pro Plus
Run ID: 05c76cc4-32ec-4e29-8371-170a1893ac6f
📒 Files selected for processing (1)
ci-operator/step-registry/tls/scanner/run/tls-scanner-run-commands.sh
08c765b to
f3e399f
Compare
jzding
changed the title
f3e399f to
ae7709d
Compare
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-commands.sh`:
- Around line 264-271: Update the Failed pod branch in the scanner command flow
to return a non-zero status after logging and artifact handling. Use the numeric
pod_exit_code when it is available and non-zero; otherwise return 1, ensuring
scanner findings and crashes cannot result in successful completion.
🪄 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: Pro Plus
Run ID: 284641f1-dfc3-4e6f-9830-90826ad7f24f
📒 Files selected for processing (1)
ci-operator/step-registry/tls/scanner/run/tls-scanner-run-commands.sh
2428419 to
d70f6cb
Compare
|
@jzding: |
1 similar comment
|
@jzding: |
d70f6cb to
7d1e850
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ci-operator/step-registry/tls/scanner/run/tls-scanner-run-commands.sh (1)
257-273:⚠️ Potential issue | 🟠 MajorPropagate the scanner’s non-zero status.
This previous finding remains unresolved: the
Failedbranch only logspod_exit_codeand then falls through to the success epilogue. TLS findings and scanner crashes can therefore be reported as a successfulrun_tls_scan. Preserve artifact handling, then return the numeric non-zero code when valid, otherwise1, while preserving cleanup behavior.🤖 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 `@ci-operator/step-registry/tls/scanner/run/tls-scanner-run-commands.sh` around lines 257 - 273, Update the Failed branch in run_tls_scan to preserve the existing artifact handling, then return the numeric pod_exit_code when it is a valid non-zero value; otherwise return 1. Ensure this return occurs before the success epilogue while retaining the existing cleanup behavior.
🤖 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-commands.sh`:
- Around line 274-280: Update the pod-wait logic in the tls scanner command flow
to poll until the pod reaches either Succeeded or Failed, rather than waiting
only for Succeeded. After polling, route a Failed phase through the existing
artifact-aware failure handling, while preserving the current success and
timeout behavior.
---
Duplicate comments:
In `@ci-operator/step-registry/tls/scanner/run/tls-scanner-run-commands.sh`:
- Around line 257-273: Update the Failed branch in run_tls_scan to preserve the
existing artifact handling, then return the numeric pod_exit_code when it is a
valid non-zero value; otherwise return 1. Ensure this return occurs before the
success epilogue while retaining the existing cleanup behavior.
🪄 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: Pro Plus
Run ID: 807ae90d-6643-4cca-a4fc-48d851e8dc9e
📒 Files selected for processing (1)
ci-operator/step-registry/tls/scanner/run/tls-scanner-run-commands.sh
7d1e850 to
c53a1f5
Compare
Four issues in tls-scanner-run-commands.sh: 1. OWNS_NAMESPACE unbound variable: The cleanup trap references NAMESPACE and OWNS_NAMESPACE which are local variables inside run_tls_scan(). When the trap fires after the function returns, locals are out of scope. Fix: snapshot both locals into global variables (_CLEANUP_NAMESPACE, _CLEANUP_OWNS_NAMESPACE) before registering the trap. 2. Scanner exit code 1 treated as infrastructure failure: When the scanner finds TLS compliance issues, it exits with code 1 and the pod phase is "Failed". The script called exit 1 immediately, wasting the 10-minute oc-wait timeout. Replace with artifact-aware check: if JUnit exists, log findings; if not, describe the pod. 3. Failed pod branch did not exit non-zero: After logging, the Failed branch fell through silently. Add exit with the pod's exit code. 4. Fallback oc-wait only waited for Succeeded: If the pod reached Failed during the wait, it would timeout for 10 minutes. Replace oc-wait with a polling loop that terminates on either Succeeded or Failed, then route Failed through the same artifact-aware handler. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Jack Ding <jackding@gmail.com>
c53a1f5 to
3603d42
Compare
|
@jzding: |
|
[REHEARSALNOTIFIER]
A total of 75 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: |
openshift-ci
Bot
added
the
do-not-merge/invalid-owners-file
|
/verify-owners |
openshift-ci
Bot
removed
the
do-not-merge/invalid-owners-file
1 participant
Bugs
Two issues in
tls-scanner-run-commands.sh:1.
OWNS_NAMESPACE: unbound variablein cleanup trapOWNS_NAMESPACEandNAMESPACEare local variables insiderun_tls_scan(). The cleanup trap references them, but when the trap fires after the function returns, locals are out of scope:Fix: Use
${OWNS_NAMESPACE:-true}and${NAMESPACE:-tls-scanner}in the cleanup function.2. Scanner exit code 1 causes 10-minute timeout waste
When the scanner finds TLS compliance issues, it exits with code 1 and the pod phase is
Failed. The script (line 255-258) catches this and callsexit 1, triggering the cleanup trap crash from bug #1. Even without the crash, the subsequentoc wait --for=Succeeded(line 261) would timeout for 10 minutes since the pod is alreadyFailed, notSucceeded.The scan actually completed successfully — artifacts were collected via the
scan.donepolling loop before this code runs:Fix: After artifacts are collected, check the pod phase. If
Failedwith artifacts present, log that findings were detected (not an infrastructure failure). Only fall through tooc waitfor unexpected phases (e.g., pod stillRunningwithoutscan.done).Reproduction
Run
tls-scanner-runagainst any namespace where the scanner finds TLS compliance issues (exit code 1). Observed in PTP operator periodic CI (tls-scan-periodicon release-5.0)./cc @richardsonnick @rhmdnd @smith-xyz
🤖 Generated with Claude Code
Summary by CodeRabbit
tls-scanner-runcleanupEXITtrap inci-operator/step-registry/tls/scanner/run/tls-scanner-run-commands.shso it doesn’t break underset -o nounsetwhen the TLS scanner pod exits non-zero. The script snapshotsNAMESPACEandOWNS_NAMESPACEinto global_CLEANUP_*variables before registering the trap, and the trap uses those snapshots for namespace/pod deletion.phase=Failed, the script now checks whetherjunit_tls_scan.xmlwas collected. If artifacts exist, it treats the failure as expected TLS findings (and skips describing the pod as an infrastructure failure); it only describes the pod (and fails) when artifacts are missing. For other unexpected phases, it waits up to 10 minutes forSucceededand only fails on timeout—avoiding unnecessaryoc wait --for=Succeededtimeouts for TLS finding outcomes.