fix: add retry for curl downloads #2987
Conversation
Signed-off-by: Anant Sharma <anants@nvidia.com>
nv-anants
requested review from
a team,
alec-flowers,
ishandhanani,
nnshah1,
ptarasiewiczNV,
richardhuo-nv,
rmccorm4 and
tanmayv25
as code owners
WalkthroughAdded curl retry flags to Prometheus download steps in two Dockerfiles; other extraction and installation steps unchanged. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 0
🧹 Nitpick comments (3)
container/Dockerfile.vllm (2)
230-234: Harden download: timeouts, broader retry, no pipe-to-tar, optional checksum.
- Add timeouts to avoid long hangs.
- Broaden retry to cover conn resets; use --retry-all-errors when available, else --retry-connrefused.
- Avoid curl | tar to remove pipefail edge cases; download, verify, then extract.
- Optionally verify SHA256 from upstream to reduce supply-chain risk.
Apply within this hunk:
- curl -fsSL --retry 5 --retry-delay 5 "https://github.com/prometheus/prometheus/releases/download/v${PROM_VERSION}/prometheus-${PROM_VERSION}.${PLATFORM}.tar.gz" \ - | tar -xz -C /tmp && \ + curl -fsSL \ + --retry ${CURL_RETRY:-5} \ + --retry-delay ${CURL_RETRY_DELAY:-5} \ + --retry-connrefused \ + --connect-timeout ${CURL_CONNECT_TIMEOUT:-30} \ + --max-time ${CURL_MAX_TIME:-600} \ + -o "/tmp/prometheus-${PROM_VERSION}.${PLATFORM}.tar.gz" \ + "https://github.com/prometheus/prometheus/releases/download/v${PROM_VERSION}/prometheus-${PROM_VERSION}.${PLATFORM}.tar.gz" && \ + tar -xzf "/tmp/prometheus-${PROM_VERSION}.${PLATFORM}.tar.gz" -C /tmp && \ mv "/tmp/prometheus-${PROM_VERSION}.${PLATFORM}/prometheus" /usr/local/bin/ && \ chmod +x /usr/local/bin/prometheus && \ - rm -rf "/tmp/prometheus-${PROM_VERSION}.${PLATFORM}" + rm -rf "/tmp/prometheus-${PROM_VERSION}.${PLATFORM}" "/tmp/prometheus-${PROM_VERSION}.${PLATFORM}.tar.gz"Add build args once (outside this hunk) near the PROM_VERSION arg:
ARG CURL_RETRY=5 ARG CURL_RETRY_DELAY=5 ARG CURL_CONNECT_TIMEOUT=30 ARG CURL_MAX_TIME=600Optional checksum verification (insert between curl and tar):
curl -fsSL "https://github.com/prometheus/prometheus/releases/download/v${PROM_VERSION}/sha256sums.txt" -o /tmp/prom.sha256 && \ grep "prometheus-${PROM_VERSION}.${PLATFORM}.tar.gz" /tmp/prom.sha256 | sha256sum -c - && \
230-230: Consider DRYing curl retry args across Dockerfiles.Define the CURL_* ARGs once and reuse here and in Dockerfile.sglang to keep behavior consistent.
Happy to push a follow-up commit wiring these ARGs into both Dockerfiles.
container/Dockerfile.sglang (1)
167-171: Mirror hardening from vLLM Dockerfile: timeouts, broader retry, no pipe-to-tar, optional checksum.Keep both images aligned for reliability and security.
Apply within this hunk:
- curl -fsSL --retry 5 --retry-delay 5 "https://github.com/prometheus/prometheus/releases/download/v${PROM_VERSION}/prometheus-${PROM_VERSION}.${PLATFORM}.tar.gz" \ - | tar -xz -C /tmp && \ + curl -fsSL \ + --retry ${CURL_RETRY:-5} \ + --retry-delay ${CURL_RETRY_DELAY:-5} \ + --retry-connrefused \ + --connect-timeout ${CURL_CONNECT_TIMEOUT:-30} \ + --max-time ${CURL_MAX_TIME:-600} \ + -o "/tmp/prometheus-${PROM_VERSION}.${PLATFORM}.tar.gz" \ + "https://github.com/prometheus/prometheus/releases/download/v${PROM_VERSION}/prometheus-${PROM_VERSION}.${PLATFORM}.tar.gz" && \ + tar -xzf "/tmp/prometheus-${PROM_VERSION}.${PLATFORM}.tar.gz" -C /tmp && \ mv "/tmp/prometheus-${PROM_VERSION}.${PLATFORM}/prometheus" /usr/local/bin/ && \ chmod +x /usr/local/bin/prometheus && \ - rm -rf "/tmp/prometheus-${PROM_VERSION}.${PLATFORM}" + rm -rf "/tmp/prometheus-${PROM_VERSION}.${PLATFORM}" "/tmp/prometheus-${PROM_VERSION}.${PLATFORM}.tar.gz"Add (outside this hunk), near PROM_VERSION:
ARG CURL_RETRY=5 ARG CURL_RETRY_DELAY=5 ARG CURL_CONNECT_TIMEOUT=30 ARG CURL_MAX_TIME=600Optional checksum verification (same as vLLM comment).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
container/Dockerfile.sglang(1 hunks)container/Dockerfile.vllm(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (4)
container/Dockerfile.vllm (2)
230-230: LGTM: Added curl retries to Prometheus download.Good step toward reducing flaky builds.
230-230: Verify and update retry flag based on curl version
Please confirm that the runtime image’s curl version (in container/Dockerfile.vllm:230) is ≥ 7.71; if so, replace--retry-connrefusedwith--retry-all-errors.container/Dockerfile.sglang (2)
167-167: LGTM: Retry flags added to Prometheus curl.Matches vllm image; should reduce transient failures.
167-167: Add --retry-all-errors when supported.Same note as vLLM image; fall back to --retry-connrefused if older curl.
Use the same script adjusted for container/Dockerfile.sglang.
Signed-off-by: Anant Sharma <anants@nvidia.com> Signed-off-by: ayushag <ayushag@nvidia.com>
Signed-off-by: Anant Sharma <anants@nvidia.com> Signed-off-by: zhongdaor <zhongdaor@nvidia.com>
Overview:
avoid connection reset and other network failures in curl download - https://github.com/ai-dynamo/dynamo/actions/runs/17615998578/job/50049141692?pr=2967#step:7:17474
Summary by CodeRabbit