add a smoke test to ensure server images start correctly - #1646
Open
tmckayus wants to merge 1 commit into
Open
Conversation
tmckayus
added
non-breaking
📝 WalkthroughWalkthroughChangesImage startup smoke testing
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 runs this for both variants after the multiarch manifests are published | ||
| (see `.github/workflows/test_images.yaml` job `smoke`). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the official GitHub product name.
Replace GITHUB with GitHub.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~42-~42: The official name of this software platform is spelled with a capital “H”.
Context: ... multiarch manifests are published (see .github/workflows/test_images.yaml job smoke...
(GITHUB)
🤖 Prompt for 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.
In `@ci/docker/README.md` at 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.
Source: Linters/SAST tools
Comment on lines
+40
to
+79
| cleanup() { | ||
| if [[ -n "${cid}" ]]; then | ||
| docker rm -f "${cid}" >/dev/null 2>&1 || true | ||
| fi | ||
| rm -f "${log}" | ||
| } | ||
| trap cleanup RETURN | ||
|
|
||
| info "Starting ${label} server from ${IMAGE}" | ||
| # Do not use --rm: a fast crash (e.g. missing libnccl.so.2) would delete the | ||
| # container before we can collect logs. | ||
| cid="$(docker run -d --name "${name}" "${GPU_ARGS[@]}" "$@" "${IMAGE}")" | ||
|
|
||
| for ((i = 1; i <= TIMEOUT_SECS; i++)); do | ||
| docker logs "${cid}" >"${log}" 2>&1 || true | ||
|
|
||
| if grep -qiE 'error while loading shared libraries|libnccl\.so|FATAL FIPS SELFTEST|OpenSSL internal error' "${log}"; then | ||
| echo "----- ${label} logs -----" | ||
| cat "${log}" | ||
| fail "${label}: loader/crypto failure while starting" | ||
| fi | ||
|
|
||
| if grep -qE "${expect_re}" "${log}"; then | ||
| pass "${label}: matched /${expect_re}/" | ||
| return 0 | ||
| fi | ||
|
|
||
| # Container exited before listen — dump logs and fail. | ||
| if ! docker inspect -f '{{.State.Running}}' "${cid}" 2>/dev/null | grep -qx true; then | ||
| echo "----- ${label} logs -----" | ||
| cat "${log}" | ||
| fail "${label}: container exited before becoming ready" | ||
| fi | ||
|
|
||
| sleep 1 | ||
| done | ||
|
|
||
| echo "----- ${label} logs -----" | ||
| cat "${log}" | ||
| fail "${label}: timed out after ${TIMEOUT_SECS}s waiting for /${expect_re}/" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Run cleanup when a smoke test fails.
trap cleanup RETURN runs only when smoke_one returns. The fail calls use exit 1, so Bash terminates before the function returns. A container that starts and then fails, and its temporary log, remain on the runner. The docker run command at Line 51 can cause the same result through set -e.
Return a nonzero status from smoke_one after each failure path, including a failed docker run, or use an EXIT cleanup design that safely retains the required container and log state.
🤖 Prompt for 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.
In `@ci/docker/smoke_image.sh` around lines 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.
CI Test Summary✅ All 31 test job(s) passed. |
| # 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: |
Collaborator
There was a problem hiding this comment.
Can't we add the test script as part of test_image.sh ?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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).