Proposal: add new --sync-output flag to bake #1197

jedevc · 2022-07-05T14:39:41Z

Potentially fixes #1089. ~~Depends on a server that includes that patch here: moby/buildkit#2947~~

This proposal patch introduces a new syncable output option, which ensures that all builds finish simultaneously in the solver, so that no outputs are completed independently of each other. This allows bake to easily express the notion that either all builds should succeed and output or none of them should.

Usage:

$ docker buildx bake --sync-output

Comments and thoughts appreciated 🎉

ciaranmcnulty · 2022-07-05T14:54:06Z

Yes that's exactly what I had in mind 👍

crazy-max

I pushed an extra commit as simple test case but we should look at implementing integration tests (pretty much like BuildKit with testutil).

crazy-max · 2022-09-26T14:42:13Z

build/build.go

+func Build(ctx context.Context, drivers []DriverInfo, opt map[string]Options, docker DockerAPI, configDir string, w progress.Writer, syncOutputs bool) (resp map[string]*client.SolveResponse, err error) {
+	return BuildWithResultHandler(ctx, drivers, opt, docker, configDir, w, nil, syncOutputs, false)
 }

-func BuildWithResultHandler(ctx context.Context, drivers []DriverInfo, opt map[string]Options, docker DockerAPI, configDir string, w progress.Writer, resultHandleFunc func(driverIndex int, rCtx *ResultContext), allowNoOutput bool) (resp map[string]*client.SolveResponse, err error) {
+func BuildWithResultHandler(ctx context.Context, drivers []DriverInfo, opt map[string]Options, docker DockerAPI, configDir string, w progress.Writer, resultHandleFunc func(driverIndex int, rCtx *ResultContext), syncOutputs bool, allowNoOutput bool) (resp map[string]*client.SolveResponse, err error) {


We should look at refactoring these funcs in a follow-up

Yup, have avoided making too many changes since #1296 refactors around this as well.

crazy-max · 2022-09-26T14:48:02Z

docs/reference/buildx_bake.md

@@ -25,6 +25,7 @@ Build from a file
 | [`--pull`](#pull) |  |  | Always attempt to pull all referenced images |
 | `--push` |  |  | Shorthand for `--set=*.output=type=registry` |
 | [`--set`](#set) | `stringArray` |  | Override target value (e.g., `targetpattern.key=value`) |
+| `--sync-output` |  |  | Ensure all builds complete before beginning output |


Maybe --sync would be enough? There are some cases where we might not need to output anything but keep build result synced (type=cacheonly)?

Suggested change

| `--sync-output` | | | Ensure all builds complete before beginning output |

| `--sync` | | | Ensure all builds complete before returning result |

Let's also add a simple example here for this flag.

I think --sync isn't enough, you want to hint it is about synching the 'stuff at the end'

jedevc · 2022-10-17T09:13:30Z

Have updated with the new Evaluate API introduced in moby/buildkit#3137.

Still not sure about the flag name, but the functionality should be correct now (have tested with buildkit before+after the evaluate API merged to check the StatFile fallback works correctly).

jedevc · 2022-11-30T16:21:32Z

Adding to the v0.10 milestone, think this would be good to get in there.

Revisiting this after a while, I wonder if maybe we might want to make this the default behavior? Or some other way that doesn't involve needing to add lots more flags, IMO not needing a lot of extra flags on the cli is one of the selling points of bake 🤔

This patch introduces a new syncable output option, which ensures that all builds finish simultaneously in the solver, so that no outputs are completed independently of each other. This allows bake to easily express the notion that either all builds should succeed and output or none of them should. Signed-off-by: Justin Chadwell <me@jedevc.com>

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

jedevc · 2023-03-02T22:14:05Z

Ping @tonistiigi @crazy-max any thoughts on the overall approach here? I'm a bit hesitant around adding new flags to the CLI though 🤔

Possible approaches:

Just add the new flag (clutters the command line)
Make the synced outputs default (slows down quite a few builds)
Maybe add a sync key or similar to each group in bake, all targets part of that group would be synced? We could allow setting it at the command line using --set (though this doesn't work very well if you don't use groups, and just specify multiple targets).

Nithos · 2023-03-03T02:42:37Z

Does the current action support group entries currently into the targets input? If so adding a sync to a group definition is a fairly elegant solution that avoids the flags. I am assuming that the group would get decomposed and automatically parallelize the process rather than having to manually deconstruct the list of targets present within the group and run through the GH matrix.

crazy-max · 2023-03-03T08:02:17Z

Having a sync field for group is a good idea. Hence my suggestion to have a less opinionated flag named --sync instead of --sync-output.

I'm thinking about another use case now. Should we have a fail-fast strategy that cancels all in-progress targets in the group if any target fails? I think this is the current behavior.

jedevc · 2023-03-03T08:46:56Z

Yeah, if we have a sync field, having the ability to sync at different points would be good:

fail-fast is the current behavior
output syncs at the output stage, like this patch
fail-slow (naming is hard) is the opposite of fail-fast, all builds are run to completion (though we might also want to have variants of this that sync at output or after).

jedevc changed the title ~~bake: add new --sync-output flag~~ Proposal: add new --sync-output flag to bake Jul 5, 2022

jedevc mentioned this pull request Jul 18, 2022

Always respect value of evaluate field in the gateway api moby/buildkit#2947

Closed

jedevc requested review from tonistiigi and crazy-max August 4, 2022 11:44

crazy-max added the area/bake label Sep 10, 2022

jedevc force-pushed the bake-syncable-output branch from 449a668 to 0857015 Compare September 26, 2022 10:21

jedevc marked this pull request as ready for review September 26, 2022 10:23

crazy-max reviewed Sep 26, 2022

View reviewed changes

jedevc mentioned this pull request Sep 29, 2022

Add Evaluate(Ref) Gateway API moby/buildkit#3137

Merged

jedevc force-pushed the bake-syncable-output branch from a73a4f9 to 0ac4310 Compare October 17, 2022 09:06

jedevc added this to the v0.10 milestone Nov 30, 2022

jedevc and others added 2 commits November 30, 2022 16:22

bake(e2e): simple test for sync-output flag

048e6ac

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

jedevc force-pushed the bake-syncable-output branch from ec88287 to 048e6ac Compare November 30, 2022 16:22

jedevc removed this from the v0.10.0 milestone Jan 6, 2023

jedevc mentioned this pull request Mar 2, 2023

feature: perform push iff all targets succeed docker/bake-action#112

Closed

crazy-max mentioned this pull request Oct 9, 2023

docker buildx bake does not support saving multiple targets to a single tarball #1668

Open

3 tasks

jedevc mentioned this pull request Jan 10, 2024

solver: ensure each ref in the result map is evaluated moby/buildkit#4544

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal: add new --sync-output flag to bake #1197

Proposal: add new --sync-output flag to bake #1197

jedevc commented Jul 5, 2022 •

edited

Loading

ciaranmcnulty commented Jul 5, 2022

crazy-max left a comment

crazy-max Sep 26, 2022

jedevc Sep 26, 2022 •

edited

Loading

crazy-max Sep 26, 2022

ciaranmcnulty Sep 26, 2022

jedevc commented Oct 17, 2022

jedevc commented Nov 30, 2022

jedevc commented Mar 2, 2023 •

edited

Loading

Nithos commented Mar 3, 2023

crazy-max commented Mar 3, 2023

jedevc commented Mar 3, 2023

	\| `--sync-output` \| \| \| Ensure all builds complete before beginning output \|
	\| `--sync` \| \| \| Ensure all builds complete before returning result \|

Proposal: add new --sync-output flag to bake #1197

Are you sure you want to change the base?

Proposal: add new --sync-output flag to bake #1197

Conversation

jedevc commented Jul 5, 2022 • edited Loading

ciaranmcnulty commented Jul 5, 2022

crazy-max left a comment

Choose a reason for hiding this comment

crazy-max Sep 26, 2022

Choose a reason for hiding this comment

jedevc Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

crazy-max Sep 26, 2022

Choose a reason for hiding this comment

ciaranmcnulty Sep 26, 2022

Choose a reason for hiding this comment

jedevc commented Oct 17, 2022

jedevc commented Nov 30, 2022

jedevc commented Mar 2, 2023 • edited Loading

Nithos commented Mar 3, 2023

crazy-max commented Mar 3, 2023

jedevc commented Mar 3, 2023

jedevc commented Jul 5, 2022 •

edited

Loading

jedevc Sep 26, 2022 •

edited

Loading

jedevc commented Mar 2, 2023 •

edited

Loading