fix(docker): bake prisma offline in the componentized migrations image - #35485
Conversation
|
|
Greptile SummaryThe PR makes the standalone migrations image self-contained for offline execution and adds image-level CI coverage
Confidence Score: 5/5The PR appears safe to merge No blocking failure remains
|
| Filename | Overview |
|---|---|
| migrations/Dockerfile | Moves baked Prisma artifacts to /opt/prisma, adds runtime Node.js, and configures offline migration execution; the previously reported comment issue is no longer present |
| .github/workflows/image-scan.yml | Adds a dedicated job that builds and exercises the migrations image in the existing offline migration test |
| tests/proxy_migration_tests/test_offline_image_migration.py | Makes the migration command configurable so the same image-level regression test covers both runtime image layouts |
Reviews (2): Last reviewed commit: "fix(docker): bake prisma offline in the ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| # Pre-warm the Prisma binary cache so the Job pod doesn't reach the | ||
| # internet on first start. This matches what the backend Dockerfile does: | ||
| # `prisma generate` runs nodeenv (downloads Node), installs the prisma npm | ||
| # CLI, downloads the engine binaries for each `binaryTarget` in | ||
| # schema.prisma, AND emits the generated Python client. We don't need the | ||
| # client at runtime — the migration job invokes `prisma migrate deploy` | ||
| # via subprocess — but having it cached is harmless and the alternative | ||
| # (`prisma py fetch`) doesn't reliably trigger engine downloads. | ||
| RUN mkdir -p /home/nonroot && \ | ||
| HOME=/home/nonroot prisma generate --schema=./schema.prisma && \ | ||
| chown -R nonroot:nonroot /home/nonroot/.cache | ||
| # Bake the Prisma CLI and engines so the Job pod doesn't reach the internet | ||
| # on first start. `prisma generate` installs the prisma npm CLI, downloads | ||
| # the engine binaries for each `binaryTarget` in schema.prisma, AND emits the | ||
| # generated Python client. We don't need the client at runtime — the | ||
| # migration job invokes `prisma migrate deploy` via subprocess — but having | ||
| # it cached is harmless and the alternative (`prisma py fetch`) doesn't | ||
| # reliably trigger engine downloads. | ||
| # | ||
| # The bake lands under /opt/prisma, a fixed path every runtime uid can read |
There was a problem hiding this comment.
The newly added explanatory prose conflicts with the repository rule prohibiting new code comments unless explicitly requested, and it adds documentation that must remain synchronized across the Dockerfile, workflow, and test harness.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
The migrations image ran `prisma migrate deploy` against a bake anchored in $HOME with no node in the runtime stage, so prisma-client-py fell through to nodeenv and tried to download a Node runtime on first start. In an egress-restricted cluster that fails outright, and under an arbitrary uid the uid-specific cache path is unreadable, so the job never applies a migration. Move the bake to /opt/prisma with world-readable modes, install node in the runtime stage, and pin PRISMA_BINARY_CACHE_DIR / PRISMA_CLI_PATH / PRISMA_OFFLINE_MODE so the migration entrypoint runs the cached CLI directly. This is the same treatment the root, non_root and database images already carry. Resolves LIT-4727
d33e758 to
355aaf5
Compare
|
@greptileai please review the current head You were right about the comments; the repository guide forbids adding new ones and I had added three. All of them are gone, and the diff against the base now contains zero added comment lines ( Nothing else changed, so I rebuilt the image from the amended commit and re-ran the regression to confirm the discriminator survived: it passes on the new image and still fails on the base image, which is the behaviour the new On the concern the comments were carrying: the guard is the test, not the prose. If a future edit drops |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
4 participants
TLDR
Problem this solves:
prisma migrate deployhangs in egress-restricted clusters$HOMEpathHow it solves it:
/opt/prisma, world-readablenodejsin the migrations runtime stagePRISMA_CLI_PATH/PRISMA_OFFLINE_MODEso no download is attemptedRelevant issues
Same failure class as #33167 and #24554, which were fixed for the root,
Dockerfile.databaseandDockerfile.non_rootimages. The componentizedmigrations/Dockerfilewas never brought in line; this does thatLinear ticket
Resolves LIT-4727
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
The end-user surface here is the container, so the proof is the image itself running the way an air-gapped cluster runs it: a
--internaldocker network (no egress at all) with a reachable Postgres on it. Before captured at base0a42f28850, after atd33e75853a, then re-run at355aaf50c1after a comment-only amendBefore, base image, no egress, arbitrary non-root uid
Run as the image's own default uid, the same base image shows the download attempt directly:
After, same network, same flags
After, default uid, read-only root filesystem
This is the constraint the report came in under, so it gets its own run
The regression test discriminates
tests/proxy_migration_tests/test_offline_image_migration.pyalready encodes this contract for the single image; the change makes the migration invocation configurable so the same suite covers this image, and a newmigrations-imagejob inimage-scan.ymlruns itRe-run after the comment-only amend, rebuilding the image from
355aaf50c1, to confirm the discriminator survived itType
🐛 Bug Fix
Changes
migrations/Dockerfilenow bakes the prisma CLI and engines under/opt/prismawitha+rXmodes instead of anchoring them in$HOME, installsnodejsin the runtime stage, and pinsPRISMA_BINARY_CACHE_DIR,PRISMA_CLI_PATH,PRISMA_CLI_QUERY_ENGINE_TYPEandPRISMA_OFFLINE_MODE. Those last two env vars are whatlitellm_proxy_extras.utils._get_prisma_commandand_get_prisma_envread to run the cached CLI directly rather than falling through the python wrapper, which is the path that reaches for nodeenv. The build now asserts the two baked artifacts exist, so a future edit that breaks the bake fails the build instead of the clusterThere are two distinct symptoms behind one root cause. Without node in the runtime stage prisma-client-py resolves its node target through nodeenv and downloads a runtime; with the bake behind
/home/nonroot, an arbitrary uid cannot read it. Moving to/opt/prismaplus a runtime node closes both, which is exactly what the root, non_root and database images already doWorth calling out why this survived the earlier round of fixes:
grep -rn 'migrations/Dockerfile\|gateway/Dockerfile\|backend/Dockerfile' .github/ ci_cd/ Makefile .circleci/config.ymlcame back empty on the base branch, so no job in this repo built any componentized image and nothing would have caught it. Themigrations-imagejob here is the firstNo grype step was added for this image. Its runtime apk set is a subset of
Dockerfile.non_root, which the existing scan job covers, andnodejswas already in that image's runtime layer, so this introduces no unscanned package surfaceScope note: the gateway and backend images also lack node in their runtime stage, but neither runs a prisma schema update. That path lives in
proxy_cli.py, which the componentized entrypoints bypass by importing the ASGI app directly, so they are unaffectedFinal Attestation