ci: remove the Podman checkpoint/restore job - #123
Conversation
Drops test-integration-podman and the .github/scripts/podman-cr.sh helper it docker-ran. The job was the only consumer of that script, so .github/scripts/ goes with it. This removes the Podman CI surface only; runtime/podman and the gated tests it built (./runtime/podman, TestPodman* under test/integration) stay in the tree and still compile under `go test ./...` — they are simply no longer exercised against a live Podman socket with CRIU. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
On-demand reviews are free for the next 23 days. After that, they cost $0.25 per reviewed file. Or wait 4 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Your 62 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
There was a problem hiding this comment.
Code Review — head c9958ad
Reviewed the one-commit, two-file PR range at head c9958ad (83 deletions): the Podman workflow job and its helper script. Read the applicable engineering directives at base 29ce1d9, CONTRIBUTING.md, the Podman/checkpoint design records, the remaining CI workflow, and all Podman integration-test consumers. The deleted job was the only path that supplied a live Podman+CRIU environment; the remaining Docker integration shards skip Podman tests because PODMAN_SOCKET is unset. Recorded the R8 finding for removing that required real-backend gate. No other concrete defect was established in the deletion itself. Coverage gaps: PRD.md referenced by the directives is absent at the directives base; legacy branch-protection status checks could not be queried (GitHub returned 403), though the accessible Trunk ruleset declares no required checks. I did not execute tests or the system under review. Replies typed into inline comment threads will not reach this review; answer in the pull-request conversation instead.
Commented inline
- [MEDIUM] [R8] Removing the only live Podman gate leaves checkpoint/restore regressions invisible to CI —
.github/workflows/ci.yml:117
Verdict
ADVISORY — findings worth reading, none of them blocking.
| test-integration-podman: | ||
| runs-on: ubuntu-latest | ||
| needs: [lint, test-linux] | ||
| steps: | ||
| - uses: actions/checkout@v6 | ||
| - uses: actions/setup-go@v6 | ||
| with: | ||
| go-version: "1.25" | ||
| cache: true | ||
| - name: Build gated test binaries (static; compile coverage + run in container) | ||
| env: | ||
| CGO_ENABLED: "0" | ||
| run: | | ||
| go test -tags=integration -c ./test/integration -o ./int.test | ||
| go test -c ./runtime/podman -o ./podman.test |
There was a problem hiding this comment.
[MEDIUM] [R8] Removing the only live Podman gate leaves checkpoint/restore regressions invisible to CI
- Witness: Command:
git show 29ce1d948b03c493b59b9811bb1d34059b0989c7:design/podman-backend.md | nl -ba | grep -i -A6 -B4 -E 'integration|test' | head -100\nRaw output:281\t### Phase 2 — the Podman backend...288\t- Integration tests behind a real-podman gate (mirror the existing/289\t real-docker compose integration-test gate).and318\t- **Integration (real podman gate):** bring a container up via the Podman/319\t backend, checkpoint→remove→restore, assert memory + networking resume/320\t — the bench test, codified.\n\nThe deleted job was the only CI path that compiled and ran the live tests (go test -c ./runtime/podman, then/w/podman.test -test.run TestIntegrationand/w/int.test -test.run '^TestPodman'). The remaining workflow only runsgo test -tags=integration ./test/integration/...against Docker and does not setPODMAN_SOCKET; the Podman tests explicitly callt.Skipwhen that variable is absent (runtime/podman/integration_test.go:27-30,test/integration/podman_checkpoint_restore_test.go:36-38). Thus a broken libpod checkpoint/restore or build endpoint can pass every remaining check, directly contradicting the backend design's required real-Podman gate. - Consumer:
runtime/podman/integration_test.go:27-30andtest/integration/podman_checkpoint_restore_test.go:36-38readPODMAN_SOCKETand skip without it;.github/workflows/ci.yml:74-86runs the remaining integration suite without providing that socket, so no remaining CI consumer exercises the Podman backend against a daemon. - Fix: Keep a real-Podman integration gate, or replace it with an equivalent scheduled/hosted or self-hosted job that provisions Podman + CRIU, sets
PODMAN_SOCKET, and runs the runtime-level and engine/project checkpoint tests. If intentionally abandoning this guarantee, updatedesign/podman-backend.mdand the backend's support/validation contract rather than silently deleting the gate.
There was a problem hiding this comment.
Dismissed — the reviewer retracted this finding.
Reason: After re-reading the repository standard and its cited evidence, I no longer stand behind this as a defect in this PR. The factual coverage loss remains real, but R8 explicitly allows a backend-specific path to be untested when the change makes an explicit statement of which backend is untested; the PR description does exactly that. The referenced podman-backend design is marked “Draft for review,” and design/README.md:10-15 says such records are not kept in sync and that code is authoritative, so my original “required gate/design contradiction” framing was not valid. The preceding #121/#122 removal sequence also establishes that this retirement work intentionally removes backend coverage without rewriting design records. The deleted job was best-effort (it intentionally exited green when CRIU was unavailable), and no evidence shows a current supported Podman consumer that this CI deletion breaks.
The review comment above records what was claimed at the time, and is left unedited.
|
Thanks — taking the R8 finding on its merits. The factual core is correct and I'm not disputing it: after this PR no CI path exercises the Podman backend against a live daemon. Where I disagree is the framing — that this abandons a guarantee, and that the design record must be amended in the same commit. 1. The design record is not a validation contract.
2. There is direct precedent from the preceding step of this same removal. #122 — the immediately prior merged PR, co-authored by the repo owner — closes with:
#121 removed the Apple Containers CI jobs on the same basis. Amending 3. Nothing is being deleted silently. This is step 3 of retiring the non-Docker backends, stated in the PR description with the two prior PRs named. 4. The gate was weaker than the finding implies. The job smoke-tested a checkpoint first and skipped green — with the real reason — on any runner whose kernel couldn't drive a nested CRIU checkpoint ( Standing up a self-hosted or scheduled Podman+CRIU runner, as the fix suggests, would mean provisioning infrastructure for a backend being deleted in the next PR. I'd rather leave this one standing as advisory than paper over it, unless you see something in the above that's wrong. One point from your coverage gaps is genuinely useful and I've passed it on: you found the accessible Trunk ruleset declares no required checks, which answers most of the branch-protection question I flagged in the description — legacy protection is still unverified (403 for both of us). |
Third step in retiring the non-Docker backends, after #121 (Apple Containers CI) and #122 (README). This one is CI-only.
What goes
test-integration-podmanin.github/workflows/ci.yml— the job that built the gated tests statically on the runner, then docker-ran them inside aquay.io/podman/stableprivileged container (podman 5.x + crun + criu,--cgroupns=host) so CRIU could drive the runner's kernel..github/scripts/podman-cr.sh— the script that job invoked. It was the only file in.github/scripts/, and the job was its only consumer, so the directory goes with it.83 deletions, no Go changes.
What stays
runtime/podmanand the tests the job compiled —./runtime/podmanandTestPodman*undertest/integration— are untouched and still build undergo test ./.... They are simply no longer exercised against a live Podman socket with CRIU. Coverage loss is limited to that gated path: the job only ever ran for real on a runner whose kernel permitted a nested checkpoint, and skipped green with the reason otherwise.The cross-node test (
TestPodmanXNode_*) already skipped in CI — it needs two hosts and noDCCKPT_XNODE_DIRwas set — so nothing changes for it.Remaining jobs
lint,test-linux,test-integration-linux. Worth a check on your side: iftest-integration-podmanis listed as a required status check onmain, it needs dropping there too or PRs will block waiting on a job that no longer reports. I can't read the branch protection settings from here (403).Not in this PR
The backend code itself —
runtime/podman/(875 LOC),runtime/applecontainer/(3,660),applecontainer-bridge/(2,228), the integration tests, the--runtime applecontainerdispatch incmd/devcontainer/root.go, the Swift bridge targets in theMakefile, and the design docs. One open question to settle before that PR:runtime/podmanis the only implementation ofruntime.CheckpointRuntime—runtime/dockerhas noCheckpoint/Restore— so removing it orphans the whole checkpoint/restore feature (the interface,checkpoint.go/checkpoint_project.go,Capabilities().Checkpoint).