NO_ISSUE: Fix refresh script race conditions causing e2e-vmaas flakes - #122
Conversation
WalkthroughThis PR adds a bash retry_command helper and uses it to retry transient cluster operations in two scripts. prepare-fulfillment-service.sh now queries routes with a quoted namespace and wraps osac login, hub deletion, and hub creation in retry_command. refresh-after-snapshot.sh now retries oc patch for route hosts and waits for cert-manager's trust-manager rollout (logging pods/endpoints) before applying overlays and deleting jobs. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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 |
omer-vishlitzky
changed the title
a237b4b to
4531c5b
Compare
![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 `@scripts/lib.sh`:
- Around line 76-80: The if block is capturing $? too late so local rc gets 0;
run the command and capture its exit code immediately into rc before testing it:
execute "$@" then set rc=$? and use that rc in the conditional, the error log,
and the final return. Update the logic around the existing if "$@"; then ... fi
(and the local rc assignment) so all references use the captured rc variable
(from the immediate execution) instead of relying on `$?` after the if; ensure
the success branch still prints the retry message and the failure branch logs
"exit code $rc" and returns $rc.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 2755a0d7-089c-4232-9208-70d4454ecb61
📒 Files selected for processing (3)
scripts/lib.shscripts/prepare-fulfillment-service.shscripts/refresh-after-snapshot.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/prepare-fulfillment-service.sh
- scripts/refresh-after-snapshot.sh
Add retry/wait logic at three failure points in the refresh script: - Route patching: retry on API admission webhook connection refused - Kustomize apply: wait for trust-manager deployment before applying Bundle CRs - Fulfillment service: retry osac CLI gRPC calls until server is reachable
4531c5b to
9e5ba91
Compare
|
/retest |
Replace scattered per-step retries with a single parallel stabilization gate that waits for trust-manager, keycloak-tls certificate, and route patching concurrently. Total wait = max of all three (~2-3 min typical). Worst-case retry budget drops from 50min to 20min, well under the 50-min SSH timeout. Fix (( attempt++ )) which returns exit code 1 when attempt is 0.
Replace bare wait with per-PID wait and failure tracking for both fulfillment rollout steps. Bare wait under errexit does not propagate background job failures.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akshaynadkarni, omer-vishlitzky 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 |
2 participants
Problem
The e2e-vmaas CI job has a high failure rate, with the vast majority of failures caused by race conditions in the refresh script — not actual test failures.
Four root causes account for ~80% of all CI failures:
10.128.0.1:443refuses connections after snapshot bootkeycloak-tlsafter recert causes TLS handshake failuresosacCLI calls fail before server is reachableAll four are the same fundamental bug: the script doesn't wait for what it's about to use. The cluster passes the operator readiness gate (34/34), but individual services need additional time after snapshot boot + recert.
Fix
Parallel stability gate (before any steps)
A single parallel gate waits for all pre-kustomize dependencies concurrently:
oc rollout status deploy/trust-manager— trust-manager is not a cluster operator, so 34/34 misses itoc wait --for=condition=Ready certificate/keycloak-tls— cert-manager reissues TLS certs after recertretry_command— API admission webhook needs time after recertAll three run in parallel. Total wall time = max(all three) ≈ 2-3 min typical.
Post-kustomize retries
osac login,osac delete hub,osac create hubwrapped inretry_command(5min timeout each) — handles gRPC server not being ready after deployment rollout.Other fixes
retry_commandhelper inlib.shwith correct exit code handling (local rc=0; "$@" || rc=$?instead of the bash-brokenlocal rc=$?)waitcalls that silently swallowed rollout failures underset -o errexitTimeout budget
Well under the 50-min SSH timeout.