fix: cache images with architecture and check architecture in startup scripts#2087
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCI jobs now provision workspace-backed Docker storage, use architecture-aware image caches, and validate cached image compatibility. Integration and E2E scripts compare host and backend image architectures, while integration failures include recent backend logs when OIDC startup times out. ChangesArchitecture-aware image handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/setup-e2e.sh (1)
72-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract duplicated host architecture normalization logic.
The logic to normalize the
host_archstrings is duplicated across multiple scripts. Consider abstracting it into a shared helper script (e.g.,scripts/utils.sh) to reduce code duplication and simplify future architecture edge-case handling.
scripts/setup-e2e.sh#L72-L81: Replace the inline logic by sourcing and invoking the extracted utility.scripts/ci/run_integration_suite.sh#L152-L161: Replace the inline logic with the extracted utility..github/workflows/test-ci.yml#L154-L163: Refactor to use the shared utility, or retain the inline logic if importing and evaluating an external script inside the CI YAML context proves too cumbersome.🤖 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 `@scripts/setup-e2e.sh` around lines 72 - 81, Extract the duplicated host architecture normalization into a shared helper, such as a utility function in scripts/utils.sh, preserving the existing x86_64, aarch64/arm64, and fallback behavior. Update scripts/setup-e2e.sh lines 72-81 and scripts/ci/run_integration_suite.sh lines 152-161 to source and invoke the helper; refactor .github/workflows/test-ci.yml lines 154-163 to use it if practical, otherwise explicitly retain its inline logic.
🤖 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 `@scripts/setup-e2e.sh`:
- Around line 83-91: Move the image architecture validation block in
scripts/setup-e2e.sh (lines 83-91) to immediately after the service startup
command so the image exists before docker inspect runs. In
scripts/ci/run_integration_suite.sh (lines 163-171), ensure the backend image is
pulled or built before the architecture check and reorder the check accordingly;
preserve the existing mismatch warning behavior.
---
Nitpick comments:
In `@scripts/setup-e2e.sh`:
- Around line 72-81: Extract the duplicated host architecture normalization into
a shared helper, such as a utility function in scripts/utils.sh, preserving the
existing x86_64, aarch64/arm64, and fallback behavior. Update
scripts/setup-e2e.sh lines 72-81 and scripts/ci/run_integration_suite.sh lines
152-161 to source and invoke the helper; refactor .github/workflows/test-ci.yml
lines 154-163 to use it if practical, otherwise explicitly retain its inline
logic.
🪄 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: Pro
Run ID: 81bb4c6a-f02a-4d56-b4d1-abbf737ea5a2
📒 Files selected for processing (3)
.github/workflows/test-ci.ymlscripts/ci/run_integration_suite.shscripts/setup-e2e.sh
| # Get image architecture | ||
| image_name="langflowai/openrag-backend:${OPENRAG_VERSION:-latest}" | ||
| image_arch=$(${CONTAINER_RUNTIME} inspect --format '{{.Architecture}}' "$image_name" 2>/dev/null || echo "") | ||
|
|
||
| if [[ -n "$image_arch" && "$image_arch" != "$host_arch_norm" ]]; then | ||
| echo "WARNING: Architecture mismatch detected!" | ||
| echo "Host architecture is '${host_arch}' (${host_arch_norm}), but container image architecture is '${image_arch}'." | ||
| echo "The backend will run under QEMU emulation, which is extremely slow and may cause timeouts." | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Ensure the image exists locally before inspecting its architecture.
If the target container image has not been pulled or built locally prior to running docker inspect, the command will fail and evaluate to an empty string. This causes the architecture mismatch check (if [[ -n "$image_arch" ... ]]) to silently bypass the warning exactly when it is most needed, such as on a developer's first-time setup.
scripts/setup-e2e.sh#L83-L91: Move this validation block immediately below the service startup command (e.g., aftermake dev-cpuon line 100) so the image is guaranteed to be present.scripts/ci/run_integration_suite.sh#L163-L171: Verify that the backend image is pulled or built before this check is executed, and reorder it to occur after the pull/build step if necessary.
📍 Affects 2 files
scripts/setup-e2e.sh#L83-L91(this comment)scripts/ci/run_integration_suite.sh#L163-L171
🤖 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 `@scripts/setup-e2e.sh` around lines 83 - 91, Move the image architecture
validation block in scripts/setup-e2e.sh (lines 83-91) to immediately after the
service startup command so the image exists before docker inspect runs. In
scripts/ci/run_integration_suite.sh (lines 163-171), ensure the backend image is
pulled or built before the architecture check and reorder the check accordingly;
preserve the existing mismatch warning behavior.
This pull request improves the handling of container image architecture compatibility in CI and E2E scripts, ensuring that cached Docker images match the host architecture. It adds architecture checks and warnings to help avoid performance issues due to emulation, and enhances debugging information for backend startup failures.
CI/CD workflow improvements:
.github/workflows/test-ci.ymlto include${{ runner.arch }}, ensuring that cached Docker images are architecture-specific and preventing cross-architecture cache issues..github/workflows/test-ci.ymlto inspect the architecture of cached images, only using them if they match the host, and discarding incompatible caches.Architecture compatibility checks:
scripts/ci/run_integration_suite.shandscripts/setup-e2e.sh, with warnings if a mismatch is detected (indicating the backend will run under QEMU emulation, which may cause slowness and timeouts). [1] [2]Debugging enhancements:
scripts/ci/run_integration_suite.shby printing the last 50 lines of backend logs if the backend fails to start, aiding in diagnosing startup issues.Summary by CodeRabbit
Bug Fixes
CI Improvements