KONFLUX-14156(#945): add curl retry to OTP server calls - #946
KONFLUX-14156(#945): add curl retry to OTP server calls#946fullsend-ai-coder[bot] wants to merge 2 commits into
Conversation
Under high concurrency (200 concurrent RPM builds), the OTP server DNS name intermittently fails to resolve, causing curl to exit with code 6 and the build step to fail. This was observed as a 1% failure rate (2/200) on stone-stg-rh01. Add --retry 3 --retry-all-errors --retry-delay 1 to the curl calls that POST SSH keys to the OTP server /store-key endpoint in all three provisioning scripts: - deploy/operator/provision-shared-host.sh - deploy/operator/provision-host-windows.sh - deploy/operator/provision-host-macos.sh This matches the existing retry pattern already used for the otelcol-contrib download in provision-shared-host.sh. Closes #945 Authored-By: Claude Code <noreply@anthropic.com>
|
🤖 Finished Review · ✅ Success · Started 9:11 AM UTC · Completed 9:20 AM UTC |
ReviewFindingsLow
Previous runReviewFindingsHigh
Medium
Low
Labels: PR addresses a bug (OTP DNS resolution failure under concurrency) in provisioning scripts Previous run (2)Looks good to me ReviewFindingsLow
Info
Previous run (3)Looks good to me |
| echo "{message: \"Creating secret file using TLS certificate...\", level: \"INFO\"}" | ||
| KEY=$(cat id_rsa) | ||
| if ! otp_raw=$(curl --fail --cacert /tls/tls.crt -XPOST -d "$KEY" https://multi-platform-otp-server.multi-platform-controller.svc.cluster.local/store-key); then | ||
| if ! otp_raw=$(curl --fail --retry 3 --retry-all-errors --retry-delay 1 --cacert /tls/tls.crt -XPOST -d "$KEY" https://multi-platform-otp-server.multi-platform-controller.svc.cluster.local/store-key); then |
There was a problem hiding this comment.
[low] edge-case
The /store-key endpoint is not idempotent: each POST generates a new OTP and stores the SSH key under it. If a retry occurs after the server successfully processed a request but the client did not receive the response, an orphaned entry will remain in the in-memory globalMap. Under high concurrency this could leave a small number of dangling SSH keys in memory. The leaked entries are bounded and the map is freed on pod restart.
| echo "{message: \"Creating secret file using TLS certificate...\", level: \"INFO\"}" | ||
| KEY=$(cat id_rsa) | ||
| if ! otp_raw=$(curl --fail --cacert /tls/tls.crt -XPOST -d "$KEY" https://multi-platform-otp-server.multi-platform-controller.svc.cluster.local/store-key); then | ||
| if ! otp_raw=$(curl --fail --retry 3 --retry-all-errors --retry-delay 1 --cacert /tls/tls.crt -XPOST -d "$KEY" https://multi-platform-otp-server.multi-platform-controller.svc.cluster.local/store-key); then |
There was a problem hiding this comment.
[low] pattern-inconsistency
The curl retry flags added by this PR do not include --connect-timeout or --max-time timeout controls. The existing curl call in provision-shared-host.sh line 46 (downloading otelcol-contrib from GitHub) includes these flags, though that is a different use case (large external download vs. small in-cluster POST). Consider whether explicit timeout flags are desirable for these cluster-local POST calls as a defensive measure.
Suggested fix: Consider adding --connect-timeout and --max-time flags if hang protection is desired for the in-cluster OTP server calls.
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 11:19 AM UTC · Completed 12:04 PM UTC |
🔧 Fix agent — iteration 1 (human-triggered)Added --connect-timeout 5 --max-time 30 to all three OTP curl calls addressing the pattern-inconsistency finding. The edge-case finding about non-idempotent retries is acknowledged but out of scope (server-side concern, bounded impact). Fixed (1):
Disagreed (1):
Tests: passed Updated by fullsend fix agent |
|
🤖 Finished Review · ✅ Success · Started 12:07 PM UTC · Completed 12:15 PM UTC |
| echo "{message: \"Creating secret file using TLS certificate...\", level: \"INFO\"}" | ||
| KEY=$(cat id_rsa) | ||
| if ! otp_raw=$(curl --fail --cacert /tls/tls.crt -XPOST -d "$KEY" https://multi-platform-otp-server.multi-platform-controller.svc.cluster.local/store-key); then | ||
| if ! otp_raw=$(curl --fail --connect-timeout 5 --max-time 30 --retry 3 --retry-all-errors --retry-delay 1 --cacert /tls/tls.crt -XPOST -d "$KEY" https://multi-platform-otp-server.multi-platform-controller.svc.cluster.local/store-key); then |
There was a problem hiding this comment.
[low] edge-case
The --retry-all-errors flag causes curl to retry on ALL failures, including non-transient HTTP errors like 400 Bad Request or 403 Forbidden. If the OTP server returns a 4xx error, the script will retry 3 times with 1-second delays before failing, adding unnecessary delay.
| echo "{message: \"Creating secret file using TLS certificate...\", level: \"INFO\"}" | ||
| KEY=$(cat id_rsa) | ||
| if ! otp_raw=$(curl --fail --cacert /tls/tls.crt -XPOST -d "$KEY" https://multi-platform-otp-server.multi-platform-controller.svc.cluster.local/store-key); then | ||
| if ! otp_raw=$(curl --fail --connect-timeout 5 --max-time 30 --retry 3 --retry-all-errors --retry-delay 1 --cacert /tls/tls.crt -XPOST -d "$KEY" https://multi-platform-otp-server.multi-platform-controller.svc.cluster.local/store-key); then |
There was a problem hiding this comment.
[low] parameter-inconsistency
The curl timeout parameters (--connect-timeout 5 --max-time 30) differ from the existing curl pattern at provision-shared-host.sh:46 (--connect-timeout 10 --max-time 120). The difference is reasonable (internal cluster service vs external GitHub download) but undocumented.
| echo "{message: \"Creating secret file using TLS certificate...\", level: \"INFO\"}" | ||
| KEY=$(cat id_rsa) | ||
| if ! otp_raw=$(curl --fail --cacert /tls/tls.crt -XPOST -d "$KEY" https://multi-platform-otp-server.multi-platform-controller.svc.cluster.local/store-key); then | ||
| if ! otp_raw=$(curl --fail --connect-timeout 5 --max-time 30 --retry 3 --retry-all-errors --retry-delay 1 --cacert /tls/tls.crt -XPOST -d "$KEY" https://multi-platform-otp-server.multi-platform-controller.svc.cluster.local/store-key); then |
There was a problem hiding this comment.
[info] consistency
The retry parameters (--retry 3 --retry-all-errors --retry-delay 1) are consistently applied across all three modified files.
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 8:49 PM UTC · Completed 8:59 PM UTC |
cfdfb3e to
b655611
Compare
|
🤖 Finished Review · ✅ Success · Started 9:08 PM UTC · Completed 9:17 PM UTC |
| echo "{message: \"Creating secret file using TLS certificate...\", level: \"INFO\"}" | ||
| KEY=$(cat id_rsa) | ||
| if ! otp_raw=$(curl --fail --cacert /tls/tls.crt -XPOST -d "$KEY" https://multi-platform-otp-server.multi-platform-controller.svc.cluster.local/store-key); then | ||
| if ! otp_raw=$(curl --fail --connect-timeout 5 --max-time 30 --retry 3 --retry-all-errors --retry-delay 1 --cacert /tls/tls.crt -XPOST -d "$KEY" https://multi-platform-otp-server.multi-platform-controller.svc.cluster.local/store-key); then |
There was a problem hiding this comment.
[low] inconsistent-retry-pattern
The curl retry flags for the OTP server call (--connect-timeout 5 --max-time 30) differ from the existing curl retry pattern at line 46 (--connect-timeout 10 --max-time 120). The difference is justified (in-cluster OTP POST vs. external GitHub download), but a brief inline comment explaining the rationale would aid future maintainers.
Suggested fix: Consider adding a brief comment explaining why the OTP server curl uses shorter timeouts than the GitHub download curl, to aid future maintainers.
|
/retest |
returning PR to inital state to show fullsend unwanted behavior
Under high concurrency (200 concurrent RPM builds), the OTP server DNS name intermittently fails to resolve, causing curl to exit with code 6 and the build step to fail. This was observed as a 1% failure rate (2/200) on stone-stg-rh01.
Add --retry 3 --retry-all-errors --retry-delay 1 to the curl calls that POST SSH keys to the OTP server /store-key endpoint in all three provisioning scripts:
This matches the existing retry pattern already used for the otelcol-contrib download in provision-shared-host.sh.
Authored-By: Claude Code noreply@anthropic.com
Closes #945
Post-script verification
agent/945-otp-curl-retry)abd7daf409b9db1bb5ffbc9103153a9667710e46..HEAD)