Delegate sticky BuildKit to setup-buildx#45
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d246a920c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| container, err := inspectBuildkitContainer() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| deadline := time.Now().Add(buildkitStopWait) | ||
| for time.Now().Before(deadline) { | ||
| if exec.Command("pgrep", "-x", "buildkitd").Run() != nil { | ||
| action.Infof("buildkitd stopped.") | ||
| return nil | ||
| if !containerUsesBuildkitVolume(container) { |
There was a problem hiding this comment.
For workflows that pass append: to docker/setup-buildx-action, the primary buildx_buildkit_runs-on0 container still exists and mounts the sticky volume, so this single-container check succeeds even though additional BuildKit nodes are part of the same builder and use non-sticky state. That contradicts the documented contract that an appended node causes a clear failure and lets multi-platform builds silently run on uncached nodes; inspect the builder/node list and fail unless it contains exactly the one sticky node.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f763d0ca1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !dockerVolumeMatches(volume, stateRoot) { | ||
| return fmt.Errorf("Docker volume %s already exists but is not backed by %s; run runs-on/action before docker/setup-buildx-action", buildkitStateVolumeName, stateRoot) |
There was a problem hiding this comment.
On reused runners, if a previous sticky BuildKit job is cancelled or otherwise misses this action's post cleanup, the labelled Docker volume can be left behind pointing at that job's old sticky-disk path. The next job gets a different stateRoot, so this unconditional mismatch error prevents all sticky BuildKit setup even though the stale volume was created by this action and could be safely removed/recreated; use the runs-on.stickydisk=buildkit label (or a missing device path) to distinguish stale owned volumes from a real reversed setup-buildx ordering problem.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 515da5fa39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if parsed.Scheme != "https" || parsed.Host == "" || parsed.User != nil || (parsed.Path != "" && parsed.Path != "/") || parsed.RawQuery != "" || parsed.Fragment != "" { | ||
| return "", fmt.Errorf("invalid ECR registry %q: expected an HTTPS registry host without credentials, path, query, or fragment", value) | ||
| } |
There was a problem hiding this comment.
When RUNS_ON_ECR_PULL_THROUGH_CACHE includes the Docker Hub pull-through namespace that ECR uses (for example <registry>/docker-hub), this validation rejects the value and SetBuildkitOutputs clears buildkit-inline-config. In that RunsOn ecr-pull-through environment the new setup-buildx path silently loses the promised Docker Hub mirror and builds fall back to Docker Hub; handle the repository prefix (or route through a proxy/rewrite that can) instead of rejecting it as an invalid registry.
Useful? React with 👍 / 👎.
| pathInput := action.GetInput("path") | ||
| if pathInput != "" { | ||
| for _, entry := range strings.Split(pathInput, "\n") { | ||
| stickyCacheInput := action.GetInput("sticky_cache") |
There was a problem hiding this comment.
For workflows still using the existing v2 cache:/path: inputs, this now leaves cfg.StickyCache empty, so HasStickyDiskCache returns false and the action succeeds without setting up any sticky mounts. Since the README/examples still target runs-on/action@v2, users upgrading within v2 will silently lose all sticky caching; either map the legacy inputs to the new record format or fail loudly.
Useful? React with 👍 / 👎.
| // volume, then owns final shutdown because the sticky disk must not be | ||
| // snapshotted while BuildKit is still writing to it. | ||
| func cleanupBuildkit(action *githubactions.Action) error { | ||
| stateRootBytes, err := os.ReadFile(buildkitPreparedStateFile) |
There was a problem hiding this comment.
On reused runners, if a previous job was cancelled after setupBuildkit wrote /tmp/runs-on-buildkit-volume but before setup-buildx created the container, a later job with optional sticky_cache: buildkit and no sticky disk returns from Configure without rewriting this marker. The post step then reads the stale marker here, tries to inspect a container from the old job, and fails even though missing optional sticky cache is supposed to warn and continue; clear the marker whenever BuildKit setup is skipped or namespace it to the current job.
Useful? React with 👍 / 👎.
1 participant
Summary
docker/setup-buildx-actioncache,path,wait_timeout, andfail_on_missinginputs with structuredsticky_cacherecords andsticky_wait_timeoutcustom,path=...records, aliases, strict validation, and complete cache-hit accountingsnap=tosticky=with no compatibility aliasWhy
The original sticky BuildKit mode downloaded and managed a separate
buildkitd, so setup-buildx configuration and the RunsOn ECR pull-through mirror did not carry over. Letting Docker's official action own the builder preserves its normal configuration surface while RunsOn owns only persistent storage and final snapshot-safe cleanup.Buildx does not expose a public state-volume override for the docker-container driver. This implementation deliberately follows the pinned single-node volume contract (
buildx_buildkit_runs-on0_state) and verifies the actual container mount during the post step so an upstream naming change fails visibly instead of silently using ephemeral storage.Disk availability is global to all caches on a sticky volume, so the API does not expose a per-cache missing-disk policy. Declaring
sticky_cacheis itself the persistence contract:Impact
This is intentionally breaking: the old input names and comma/slash parsing are removed without aliases. Sticky BuildKit users must run
runs-on/actionbeforedocker/setup-buildx-action, use the emitted builder name and inline config, select thedocker-containerdriver, and setcleanup: false. Non-sticky users can consume the inline config to make ordinary BuildKit builders honor the RunsOn ECR Docker Hub pull-through mirror.This is a stacked follow-up to #43 and targets
feature/sticky.Validation
mise exec -- go test ./...go vetfor the root, config, and sticky-disk packagesmise exec -- make buildaction.ymland the workflow as YAMLcache-hit=true) and the BuildKit layer; all setup and post-job hooks passsticky_cachewithoutstickyfails as expected; positive sticky jobs are deploy-time/manual because theruns-onorg is not attached to the dev Fleet enterprise/var/lib/buildkitand received BuildKit statego vet ./...still reports the pre-existing unreachable statement ininternal/monitoring/agent.go:186