testrunner: use buildkit PassthroughOp for validation-only steps#1099
Draft
cpuguy83 wants to merge 2 commits into
Draft
testrunner: use buildkit PassthroughOp for validation-only steps#1099cpuguy83 wants to merge 2 commits into
cpuguy83 wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR integrates buildkit's new PassthroughOp (from v0.31.0) into the dalec test runner, replacing the previous no-op exec hack used to express "evaluate this state for validation only." The PR is stacked on #1097, which bumps the buildkit dependency to v0.31.0-rc2 and pins CI builders to this version.
Changes:
WithFinalStatenow usesllb.State.Requireswhen the daemon supportsPassthroughOp, falling back to the existing no-op exec on older daemons. Capability detection follows the samesync.Once+atomic.Boolpattern used forSupportsDiffMerge.- A new
DALEC_DISABLE_PASSTHROUGHbuild arg allows forcing the legacy fallback path, wired throughload.go,frontend/gateway.go, andfrontend/router.go. - New unit test validates both the passthrough and fallback paths;
testLinuxPackageTestsFailintegration test now exercises both code paths.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
helpers.go |
Adds passthroughOpSupported atomic.Bool with getter/setter, mirroring disableDiffMerge |
frontend/gateway.go |
Adds SupportsPassthroughOp with sync.Once-guarded capability check and DALEC_DISABLE_PASSTHROUGH override |
frontend/router.go |
Calls SupportsPassthroughOp at handler startup and records result via SetPassthroughOpSupported |
internal/testrunner/runner.go |
Updates WithFinalState to branch on capability; adds finalStateRequiresID constant |
internal/testrunner/runner_test.go |
New unit test verifying passthrough-vs-exec wiring in WithFinalState |
load.go |
Registers DALEC_DISABLE_PASSTHROUGH as a known build arg |
test/linux_target_test.go |
Wraps testLinuxPackageTestsFail to run both passthrough modes; refactors body into testLinuxPackageTestsFailMode |
go.mod |
Bumps buildkit to v0.31.0-rc2, Go to 1.25.9, and transitive dependencies |
go.sum |
Updated checksums for dependency bumps |
.github/workflows/ci.yml |
Pins CI builders to moby/buildkit:v0.31.0-rc2; creates buildx container builders for integration and e2e jobs; improves log collection |
Pin the CI builders to moby/buildkit:v0.31.0 and bump the buildkit Go module to match so both the integration (distro) matrix and the e2e suite build against the released buildkitd. Behavior changes: - Add a top-level BUILDKIT_TEST_IMAGE env as the single source of truth for the buildkit image under test. - The integration job now creates a buildx container builder pinned to the test image and uses it, instead of relying on the engine's embedded buildkit. The test harness (buildx dial-stdio) and the bake pre-builds all target this builder. Its failure log dump now also collects the buildx_buildkit_* container logs. - The e2e job creates a pinned container builder for both diff/merge matrix legs (USE_BUILDX=1 for both) rather than only the diff/merge-enabled leg falling back to the default docker driver for the other. - Bump github.com/moby/buildkit v0.30.0 -> v0.31.0 (with the transitive updates pulled in by go mod tidy). Trade-offs: - The integration job previously used the default docker driver, so bake pre-builds landed in the docker image store. With a container builder they warm the builder cache instead; tests consume the frontend as a buildkit frontend-input and reuse the same builder, so this is functionally equivalent but is the main thing to watch on the first run. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Package tests run validation steps purely for their side effect of failing the build when a check does not hold; their filesystem output is discarded. Previously the test runner forced this evaluation with a documented hack: it ran a no-op `true` command whose root mount was the state to validate, relying on the exec being evaluated to pull the validation steps into the build graph. buildkit v0.31 adds the PassthroughOp, exposed via llb.State.Requires, which expresses exactly this intent: return one state's output while requiring other states to be built. WithFinalState now uses it when the daemon advertises pb.CapPassthroughOp. Capability detection mirrors the existing SupportsDiffMerge path: the router probes the client's LLBCaps once per process and records the result on a process-global atomic toggle. A DALEC_DISABLE_PASSTHROUGH=1 build-arg forces the legacy path. Daemons that do not advertise the cap keep using the trueCmd hack, so behavior is unchanged for them and backwards compatibility is preserved. The testrunner integration test (testLinuxPackageTestsFail) now runs both with and without DALEC_DISABLE_PASSTHROUGH so the PassthroughOp and the fallback wiring are both exercised. Trade-offs: PassthroughOp is still experimental in buildkit, so it is gated behind capability detection rather than required outright. The detection flag is process-global (one buildkit daemon per frontend process), consistent with how diff/merge support is already tracked. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Brian Goff <cpuguy83@gmail.com>
cpuguy83
force-pushed
the
buildkit-passthrough-op
branch
from
78d2890 to
0e8961e
Compare
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.
Stacked on #1097. This branch is based on #1097's
test-buildkit-0.31.0-rc2,so the diff below also includes that PR's
go.mod/go.sumbump and CIchanges until #1097 lands on
main. The passthrough work is the finalcommit; review that one (
testrunner: use buildkit PassthroughOp for validation-only steps).What
Integrate buildkit's PassthroughOp (new in v0.31) to express "evaluate this
state for validation only" in the package test runner, replacing the
long-standing no-op
truecommand hack.WithFinalStateusesllb.State.Requireswhen the daemon advertisespb.CapPassthroughOp, otherwise falls back to the existingtrueCmd.WithOutputexec.SupportsDiffMerge: the router probes theclient's
LLBCapsonce per process and records the result on aprocess-global atomic toggle.
DALEC_DISABLE_PASSTHROUGH=1forces the legacy path.Backwards compatibility
Daemons that don't advertise the capability keep using the fallback, so
behavior is unchanged for them.
Testing
internal/testrunner/runner_test.go) locks in thepassthrough-vs-exec wiring.
testLinuxPackageTestsFailnow runs both with and withoutDALEC_DISABLE_PASSTHROUGH, exercising both the PassthroughOp and thefallback paths.