Repository navigation
feat(docker): build the data tier's omnigraph CLI with the inline S3 root patch - #569
Merged
Merged
Conversation
…root patch The omnigraph 0.14 CLI only addresses a local root inline, so with the server stopped it cannot reach the S3 cluster root at all: boot converge, the maintenance window and force-unlock all lose their path. Upstream PR #1189 admits an inline s3:// root for exactly those commands. This vendors that PR's crates/ diff, regenerated against v0.14.0 (it applies as is), and adds a cli-patched stage that builds `omnigraph` from the tag's commit plus the patch. OMNIGRAPH_CLI_SOURCE picks the stage and defaults to release, so the image is unchanged at today's 0.13 pin. `just check-omnigraph-pins` now also runs bin/check_omnigraph_cli_patch.py, which requires `patched` exactly when the release tag is the patch's base tag, so the 0.14 bump cannot land without it and the patched CLI cannot ship beside a server from another release. omnigraph-server, the witan image and `witan setup` keep the stock release digests. The new omnigraph-cli-patch workflow builds the image with the patched CLI and runs bin/qualify_omnigraph_cli_s3.py through it against RustFS: a seeded local root is copied to S3, then apply (adding a graph), status and replay by deployment id, force-unlock, optimize and cleanup run against the copy, and the root is copied back to check every row survived. ModernRelay/omnigraph#1189 Claude-Session: https://claude.ai/code/session_01FGZebE3YVtnu2oBXH1mKNR
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Unsupported --as arguments cause the pinned v0.14.0 CLI to reject cleanup and optimize calls, blocking qualification before maintenance can be exercised.
No open issues 🎉
What changed in this PR
Prepares the omnigraph data-tier image to use a patched CLI for stopped-server S3 operations when the release pin moves to v0.14.0, while keeping the current v0.13 default unchanged.
Changes:
- Adds a pinned source-build stage and upstream inline-S3-root patch.
- Adds CLI selection checks to the existing release-pin validation.
- Adds image-based S3 qualification against RustFS and protects patch files from whitespace rewriting.
| File | Description |
|---|---|
prek.toml |
Excludes vendored patches from whitespace fixers. |
justfile |
Adds patched-CLI validation to the pin check. |
docker/README.md |
Documents patched CLI selection and qualification. |
docker/patches/omnigraph-cli-inline-s3-root.patch |
Vendors upstream S3-root support and tests. |
docker/omnigraph-server.Dockerfile |
Adds selectable patched CLI builds and a source label. |
bin/qualify_omnigraph_cli_s3.py |
Exercises deployment, maintenance, unlocking, and data preservation on S3. |
bin/check_omnigraph_cli_patch.py |
Checks patch inputs and runtime CLI selection against release pins. |
.github/workflows/witan-tests.yml |
Installs uv for the expanded pin check. |
.github/workflows/omnigraph-cli-patch.yml |
Builds the patched image and runs RustFS qualification. |
Review effort: Balanced
The patched CLI refuses --as on optimize and cleanup, so the qualification failed at its first cleanup against the S3 copy. Upstream's own e2e runs both without an actor. Claude-Session: https://claude.ai/code/session_01FGZebE3YVtnu2oBXH1mKNR
2 tasks
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
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.
What are the relevant tickets?
witan task
tk-build-omnigraph-v0-14-0-plus-the-scope-rs-s3-pat-ec78ba(decision: option (e) oftk-decide-how-optimize-cleanup-force-unlock-and-boo-58bab2). Upstream: ModernRelay/omnigraph#1189Description (What does it do?)
Since 0.14 an inline
--targetis local-only, so the stock CLI cannot address the data tier's S3 cluster root. With the server stopped there is then no way to converge the catalog on boot, runoptimize/cleanup, or force-unlock a retained lock. Upstream #1189 (draft) admits an inlines3://root forcluster plan/apply/status/force-unlock/upgrade-ledgerand foroptimize/cleanup/repair/rebuild-full-text-indexeswith--graph.This builds the omnigraph-server image's CLI from v0.14.0 plus that change:
docker/patches/omnigraph-cli-inline-s3-root.patchis #1189'scrates/diff (commit 32722615) regenerated against the v0.14.0 commit (60bfd19). The PR is based on upstream main, but everycrates/hunk applies to the tag as is (two at line offsets), so nothing needed backporting. Its docs/skills/changelog hunks are left out because none of them touch the build and one targets a page v0.14.0 does not have.cli-patchedstage fetches that commit by id, applies the patch and runscargo build --release --locked -p omnigraph-clionrust:1.97.1-slim-trixie(the toolchain upstream'srust-toolchain.tomlpins, and the runtime's glibc).FROM cli-${OMNIGRAPH_CLI_SOURCE} AS clipicks between it and the release tarball's binary, and the runtime copiesomnigraphfromcli. The image gets anedu.mit.ol.omnigraph.cli-sourcelabel.omnigraph-serverstill comes from the tarball, and the witan image andwitan setupare untouched, so they all keep the stock digests.just check-omnigraph-pinsnow also runsbin/check_omnigraph_cli_patch.py. It requiresOMNIGRAPH_CLI_SOURCE=patchedexactly whenOMNIGRAPH_RELEASE_TAGequals the patch's base tag, a full base commit that the stage actually fetches, and exactly one install of/usr/local/bin/omnigraphin the runtime stage, fromcli. Theomnigraph-pinsjob gainssetup-uvfor it.omnigraph patched CLIworkflow builds the image with the patched CLI and runsbin/qualify_omnigraph_cli_s3.pythrough the image's own binary against RustFS.prek.tomlkeeps the whitespace fixers offdocker/patches/, because stripping a diff's blank context lines stopsgit applymatching.I gated it on the pin rather than landing it live:
OMNIGRAPH_CLI_SOURCEdefaults torelease, so at today's 0.13 pin the image is the same as before apart from the new label (cli-source=release), and a default build never touches the Rust stage (checked locally: the plain build log never mentionscli-patchedor the rust image, and the image'somnigraph --versionis 0.13.0). Once the 0.14 pin bump setsOMNIGRAPH_RELEASE_TAG=v0.14.0, the pin check fails until that same change setsOMNIGRAPH_CLI_SOURCE=patched. The patched CLI therefore ships with the bump and cannot ship next to a server from a different release. If this merges after the bump, the check makes this PR flip the default itself.The qualification is meant to cover what upstream's e2e did not. It deploys a small cluster to a local root, loads four batches (one fragment and version each), and copies the root to S3 with
aws s3 cp. Then, against the copy, with an inlines3://target:cluster planmust see the copy as convergedcluster applyof a config that adds a graph must complete converged, as a real writecluster status --deployment-idandapply --deployment-id --writers-stoppedmust replay it as lookup-onlyforce-unlockreleases each retained lock by its exact idquerymust still refusecleanupwithout--yesmust refuseoptimizemust write objects andcleanup --keep 1 --confirm --yesmust remove them, with a force-unlock after eachFinally the root is copied back and queried locally to check that every loaded row survived.
Drop the patch, the three
OMNIGRAPH_CLI_*args, the stage and the workflow once a release or an immutable edge build carries #1189, or once served maintenance (upstream RFC #1186 / #1177) replaces the direct CLI. The pin check passes on a Dockerfile with none of them.This PR does not touch
omnigraph_server_entrypoint.py,omnigraph_maintenance.pyoromnigraph_plan_gate.py. They still address the CLI the 0.13 way, and moving them to an inlines3://target istk-rework-the-omnigraph-server-entrypoint-maintenan-4311a3.How can this be tested?
Locally, on a 12-core WSL2 host that was also running other builds (load average 25 to 35 the whole time, so treat the times as an upper bound rather than what CI or Concourse will see):
git apply --checkof #1189's full patch against v0.14.0: everycrates/hunk applies (planes.rsandtargets.rsat line offsets), and onlydocs/user/operations/upgrade-0.14.mdis missing.docker build --build-arg OMNIGRAPH_CLI_SOURCE=patchedsucceeded. The coldcargo build --release --locked -p omnigraph-clistep took 2551s (about 42 minutes), on an empty cargo cache. The same build on the host took 56 minutes, but it ran alongside the docker build. Fetching,git applyand the version check take seconds.omnigraphis 266,217,704 bytes and the stock v0.14.0 one is 266,341,208, and none of the Rust stage reaches the runtime image. The image is 637 MB, vs. 615 MB for today's 0.13 image, and that difference is the 0.13 to 0.14 binaries, not the patch.omnigraph --versionprintsomnigraph 0.14.0and the label isedu.mit.ol.omnigraph.cli-source=patched.bin/qualify_omnigraph_cli_s3.pypassed against RustFS (the samerustfs/rustfs:1.0.0-beta.12digest upstream's CI pins), both with the host-built binary and through the image's own binary via the samedocker runwrapper the workflow writes. The S3 copy had 48 objects beforeoptimize, 63 after it and 45 aftercleanup --keep 1, and all 100 loaded rows read back after the round trip. Every retained lock (apply, optimize, cleanup) was released by its exact id, and the root ended unlocked.inline access supports local storage only.just check-omnigraph-pinspasses (omnigraph-server CLI: release (patch base v0.14.0, server v0.13.0)). I ranbin/check_omnigraph_cli_patch.py'sproblems()against edited copies of the Dockerfile, and it fails each of these: the bump without the flip, the flip without the bump, an unknown selector, a short commit, a fetch ofmain, the runtime copying fromfetch(alone or as well as fromcli), a missing runtimeARG, partial pins, a missing patch file, and the pins dropped with the selector left in. It passes a Dockerfile with all of it removed.A fresh-context
code-reviewpass found thatoptimizecould pass without doing anything and thatapplyon S3 was only ever a no-op. It also found three ways to get the stock CLI or the wrong commit past the pin check. All of those are fixed in the first commit. The second commit drops--asfromoptimize/cleanup, which the CLI refuses on those commands. The local run caught that.In CI the
omnigraph patched CLIjob passed on 3c58a0d (https://github.com/mitodl/agent-kit/actions/runs/40769678496). The same counts came out (48, 63, 45 objects, 100 rows), and the qualification step took 13s. The cold image build took 30 to 37 minutes onubuntu-latest(two runs overlapped, both with an empty gha cache). The earlier run on 4693716 failed only on the--asrefusal that the second commit fixes.Not verified:
repair/rebuild-full-text-indexeson S3.https://claude.ai/code/session_01FGZebE3YVtnu2oBXH1mKNR