Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions .github/workflows/e2e.yml
Original file line number Diff line number Diff line change
Expand Up @@ -168,3 +168,36 @@ jobs:
DRIVER_OPT: ${{ matrix.driver-opt }}
ENDPOINT: ${{ matrix.endpoint }}
PLATFORMS: ${{ matrix.platforms }}

cases:
runs-on: ubuntu-20.04
needs:
- build
strategy:
fail-fast: false
matrix:
case:
- bake-sync-output
steps:
-
name: Checkout
uses: actions/checkout@v3
-
name: Install buildx
uses: actions/download-artifact@v3
with:
name: binary
path: /home/runner/.docker/cli-plugins
-
name: Fix perms and check
run: |
chmod +x /home/runner/.docker/cli-plugins/docker-buildx
docker buildx version
-
name: Set up Docker Buildx
uses: docker/setup-buildx-action@v2
-
name: Test
working-directory: ./test/${{ matrix.case }}
run: |
./test.sh
40 changes: 37 additions & 3 deletions build/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"strconv"
"strings"
"sync"
"sync/atomic"
"syscall"
"time"

Expand Down Expand Up @@ -782,11 +783,11 @@ func Invoke(ctx context.Context, cfg ContainerConfig) error {
return err
}

func Build(ctx context.Context, drivers []DriverInfo, opt map[string]Options, docker DockerAPI, configDir string, w progress.Writer) (resp map[string]*client.SolveResponse, err error) {
return BuildWithResultHandler(ctx, drivers, opt, docker, configDir, w, nil, false)
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) {
Comment on lines +786 to +790
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@jedevc jedevc Sep 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if len(drivers) == 0 {
return nil, errors.Errorf("driver required for build")
}
Expand Down Expand Up @@ -854,9 +855,11 @@ func BuildWithResultHandler(ctx context.Context, drivers []DriverInfo, opt map[s
}
}

msize := 0
for k, opt := range opt {
multiDriver := len(m[k]) > 1
hasMobyDriver := false
msize += len(m[k])
for i, dp := range m[k] {
di := drivers[dp.driverIndex]
if di.Driver.IsMobyDriver() {
Expand Down Expand Up @@ -928,6 +931,10 @@ func BuildWithResultHandler(ctx context.Context, drivers []DriverInfo, opt map[s

multiTarget := len(opt) > 1

buildGrp := &sync.WaitGroup{}
buildGrp.Add(msize)
errCount := int64(0)

for k, opt := range opt {
err := func(k string) error {
opt := opt
Expand Down Expand Up @@ -1147,6 +1154,8 @@ func BuildWithResultHandler(ctx context.Context, drivers []DriverInfo, opt map[s
res, err := c.Solve(ctx, req)
if err != nil {
if origErr != nil {
atomic.AddInt64(&errCount, 1)
buildGrp.Done()
return nil, err
}
var reqErr *errdefs.UnsupportedSubrequestError
Expand All @@ -1158,6 +1167,8 @@ func BuildWithResultHandler(ctx context.Context, drivers []DriverInfo, opt map[s
origErr = err
continue
}
atomic.AddInt64(&errCount, 1)
buildGrp.Done()
return nil, err
}
// buildkit v0.8 vendored in Docker 20.10 does not support typed errors
Expand All @@ -1167,6 +1178,8 @@ func BuildWithResultHandler(ctx context.Context, drivers []DriverInfo, opt map[s
continue
}
}
atomic.AddInt64(&errCount, 1)
buildGrp.Done()
return nil, err
}
if opt.PrintFunc != nil {
Expand All @@ -1176,6 +1189,27 @@ func BuildWithResultHandler(ctx context.Context, drivers []DriverInfo, opt map[s
if resultHandleFunc != nil {
resultHandleFunc(dp.driverIndex, &ResultContext{cc, res})
}

if syncOutputs {
err := res.EachRef(func(ref gateway.Reference) error {
return ref.Evaluate(ctx)
})
if err != nil {
atomic.AddInt64(&errCount, 1)
buildGrp.Done()
return nil, err
}
}
buildGrp.Done()
if syncOutputs {
buildGrp.Wait()
if atomic.LoadInt64(&errCount) > 0 {
// wait until cancelled
<-ctx.Done()
return nil, ctx.Err()
}
}

return res, nil
}
}, ch)
Expand Down
10 changes: 6 additions & 4 deletions commands/bake.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,10 @@ import (
)

type bakeOptions struct {
files []string
overrides []string
printOnly bool
files []string
overrides []string
printOnly bool
syncOutput bool
commonOptions
}

Expand Down Expand Up @@ -146,7 +147,7 @@ func runBake(dockerCli command.Cli, targets []string, in bakeOptions) (err error
return nil
}

resp, err := build.Build(ctx, dis, bo, dockerAPI(dockerCli), confutil.ConfigDir(dockerCli), printer)
resp, err := build.Build(ctx, dis, bo, dockerAPI(dockerCli), confutil.ConfigDir(dockerCli), printer, in.syncOutput)
if err != nil {
return wrapBuildError(err, true)
}
Expand Down Expand Up @@ -190,6 +191,7 @@ func bakeCmd(dockerCli command.Cli, rootOpts *rootOptions) *cobra.Command {
flags.BoolVar(&options.exportLoad, "load", false, `Shorthand for "--set=*.output=type=docker"`)
flags.BoolVar(&options.printOnly, "print", false, "Print the options without building")
flags.BoolVar(&options.exportPush, "push", false, `Shorthand for "--set=*.output=type=registry"`)
flags.BoolVar(&options.syncOutput, "sync-output", false, "Ensure all builds complete before beginning output")
flags.StringArrayVar(&options.overrides, "set", nil, `Override target value (e.g., "targetpattern.key=value")`)

commonBuildFlags(&options.commonOptions, flags)
Expand Down
2 changes: 1 addition & 1 deletion commands/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ func buildTargets(ctx context.Context, dockerCli command.Cli, opts map[string]bu
if res == nil || driverIndex < idx {
idx, res = driverIndex, gotRes
}
}, allowNoOutput)
}, false, allowNoOutput)
err1 := printer.Wait()
if err == nil {
err = err1
Expand Down
1 change: 1 addition & 0 deletions docs/reference/buildx_bake.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



<!---MARKER_GEN_END-->
Expand Down
1 change: 1 addition & 0 deletions test/bake-sync-output/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
/out
5 changes: 5 additions & 0 deletions test/bake-sync-output/bar.Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
FROM busybox
RUN echo bar > /bar

FROM scratch
COPY --from=0 /bar /bar
13 changes: 13 additions & 0 deletions test/bake-sync-output/docker-bake.hcl
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
group "default" {
targets = ["foo", "bar"]
}

target "foo" {
dockerfile = "foo.Dockerfile"
output = ["out"]
}

target "bar" {
dockerfile = "bar.Dockerfile"
output = ["out"]
}
5 changes: 5 additions & 0 deletions test/bake-sync-output/foo.Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
FROM busybox
RUN echo foo > /foo

FROM scratch
COPY --from=0 /foo /foo
13 changes: 13 additions & 0 deletions test/bake-sync-output/test.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#!/usr/bin/env bash

set -ex

rm -rf ./out

docker buildx bake --print
docker buildx bake --sync-output

if [[ ! -f ./out/foo || ! -f ./out/bar ]]; then
echo >&2 "error: missing output files"
exit 1
fi