KONFLUX-14156 add curl retry and timeouts to OTP server calls - #951
Conversation
PR Summary by QodoAdd curl retries/timeouts for OTP /store-key provisioning calls Description
Diagram
High-Level Assessment
Files changed (3)
|
|
🤖 Finished Review · ✅ Success · Started 6:57 PM UTC · Completed 7:06 PM UTC |
Code Review by Qodo
Context used 1. Retry leaks OTP entries
|
ReviewFindingsMedium
Low
Previous runReviewFindingsCritical
Low
Previous run (2)Looks good to me Low
Labels: Bug fix adding retry logic to provisioning shell scripts — low-risk, patch-level change Previous run (3)ReviewFindingsLow
Previous run (4)Looks good to me Labels: Bug fix adding curl retry/timeout resilience to provisioning scripts. |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
fullsend-ai-review
Bot
added
ready-for-merge
|
🤖 Review · ❌ Terminated · Started 7:59 PM UTC · Ended 8:09 PM UTC |
|
Qodo and fullsend are wrong - ServeHTTP() shows different, we do clean the key. |
You’re right that The finding isn’t about that path, though — it’s about the retry around So the leak is not from If you want, I can help rephrase the finding more precisely or check whether the retry is actually safe with the current server behavior. |
fullsend-ai-review
Bot
added
ready-for-merge
|
🤖 Finished Review · ✅ Success · Started 7:59 PM UTC · Completed 8:09 PM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #951 +/- ##
==========================================
- Coverage 76.92% 76.85% -0.08%
==========================================
Files 26 26
Lines 2817 2817
==========================================
- Hits 2167 2165 -2
- Misses 452 453 +1
- Partials 198 199 +1
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
🤖 Finished Review · ✅ Success · Started 8:22 PM UTC · Completed 8:33 PM UTC |
fullsend-ai-review
Bot
added
ready-for-merge
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
meyrevived
left a comment
meyrevived
left a comment
There was a problem hiding this comment.
It's better and more detailed than what fullsend created, but there are a couple of things here
|
🤖 Review · |
|
🤖 Finished Review · ✅ Success · Started 8:14 AM UTC · Completed 8:27 AM UTC |
fullsend-ai-review
Bot
removed
the
ready-for-merge
…calls Under high concurrency (200 concurrent RPM builds), the OTP server DNS name intermittently fails to resolve, causing curl to exit with code 6 (CURLE_COULDNT_RESOLVE_HOST) and the build step to fail. Observed as a 1% failure rate (2/200) on stone-stg-rh01. Add a shared otp-utils.sh with curl_otp_store_key() that wraps curl in a retry loop, retrying up to 3 times on transient errors only (curl exit codes 6, 7, 22, 28, 35, 56). All other errors fail immediately. The shared script is added to each provisioning ConfigMap via kustomization.yaml and sourced at script start. The empty-token check is also moved into the shared function to reduce duplication across the provisioning scripts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Gal Levi <glevi@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 12:17 PM UTC · Completed 12:28 PM UTC |
fullsend-ai-review
Bot
added
the
requires-manual-review
|
🤖 Finished Retro · ❌ Failure · Started 1:17 PM UTC · Completed 1:27 PM UTC |
3 participants
Summary
Add a shell retry loop to OTP
/store-keycurl calls to handle transient failures under high concurrency (1% failure rate at 200 concurrent RPM builds on stone-stg-rh01).Resolves: KONFLUX-14156
Based on #946 — refined to use a shell retry loop instead of curl's
--retry-all-errorsfor precise control over which errors trigger a retry.Changes
deploy/operator/otp-utils.sh— sharedcurl_otp_store_key()function with retry loop and empty-token validationdeploy/operator/kustomization.yaml— addotp-utils.shto all three provisioning ConfigMapsdeploy/operator/provision-shared-host.sh— sourceotp-utils.sh, replace inline curl + empty-token check withcurl_otp_store_key()deploy/operator/provision-host-macos.sh— samedeploy/operator/provision-host-windows.sh— sameThe retry loop retries only on transient curl exit codes:
--fail(e.g. 503 during startup)--connect-timeout 5, this is a connect-phase timeoutAll other errors fail immediately without retry.
Testing
make fmtpassesmake lintpassesmake testpasses (46/46 specs, 78.4% composite coverage)shellcheckpasses (zero warnings onotp-utils.sh)kustomize buildrendersotp-utils.shin all three provisioning ConfigMapsCoverage
Shell-only changes — no Go code modified, no coverage impact.
Notes
--retry-connrefusedonly covers exit code 7, not DNS failures (exit code 6).--retry-all-errorsretries more broadly than needed. A shell loop gives precise control.otp-utils.shis a sourced library — it inheritsset -eu,pipefail, and the ERR trap from the parent script.Closes #945