add a smoke test to ensure server images start correctly - #1646
Conversation
tmckayus
added
non-breaking
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe pull request adds a Docker smoke-test script for REST and gRPC image startup. It integrates Ubuntu and conditional CUDA 13 UBI10 checks into the image workflow and documents the commands. ChangesImage startup smoke testing
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change adds container startup smoke checks and related CI documentation for REST and gRPC images. No current merge-readiness risk has been identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 1 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 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/docker/README.md`:
- Line 42: Update the reference in the smoke job note to use the official
product name “GitHub” instead of “GITHUB,” without changing the referenced
workflow or job identifiers.
In `@ci/docker/smoke_image.sh`:
- Around line 40-79: Update smoke_one’s cleanup flow so every failure path
performs cleanup before terminating: handle a failed docker run, replace
fail/exit-based paths with nonzero returns followed by cleanup, or use an EXIT
trap that safely removes the container and temporary log. Preserve log output
and failure status for loader errors, early exits, and timeouts.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 521d87c9-d77d-49a0-8e5f-2bc77bf28770
📒 Files selected for processing (3)
.github/workflows/test_images.yamlci/docker/README.mdci/docker/smoke_image.sh
CI Test Summary⏭️ All 5 test job(s) skipped. |
| # CUOPT_SERVER_TYPE=grpc. The jobs above run *inside* the image as a GHA | ||
| # container and never launch the servers, so they cannot catch packaging | ||
| # gaps such as UBI10's RHEL lib/lib64 NCCL path miss. | ||
| smoke: |
There was a problem hiding this comment.
Can't we add the test script as part of test_image.sh ?
There was a problem hiding this comment.
We can't put this in test_image.sh as wired today — that job runs inside the image as a GHA container, so it never exercises ENTRYPOINT/CMD, and it already patches LD_LIBRARY_PATH for the nvidia wheels (which is what masked the UBI10 NCCL packaging bug). The smoke test has to be host-side docker run.
The bug we hit was invoking the ubi10 image with just a docker command, as a user would.
|
🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
3 similar comments
|
🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
|
🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
|
🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
fbcf688 to
6ccc937
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/ok to test 6ccc937 |
2 participants
This adds a simple smoke test after image builds to make sure the server instances start. The goal is to detect defects in the Dockerfile used to build the image (like faulty configuration of shared library paths, etc).