diff --git a/build/build.go b/build/build.go index f0badc95982d..04665f5207db 100644 --- a/build/build.go +++ b/build/build.go @@ -432,6 +432,41 @@ func toRepoOnly(in string) (string, error) { return strings.Join(out, ","), nil } +func prepareMultiDriverExports(so *client.SolveOpt, pushNames *string, insecurePush *bool) error { + var pushPrepared bool + for i := range so.Exports { + e := &so.Exports[i] + switch e.Type { + case "oci", "tar": + return errors.Errorf("%s for multi-node builds currently not supported", e.Type) + case "image": + if pushPrepared { + continue + } + if ok, _ := strconv.ParseBool(e.Attrs["push"]); !ok { + continue + } + if *pushNames == "" { + *pushNames = e.Attrs["name"] + if *pushNames == "" { + return errors.Errorf("tag is needed when pushing to registry") + } + if ok, _ := strconv.ParseBool(e.Attrs["registry.insecure"]); ok { + *insecurePush = true + } + } + names, err := toRepoOnly(e.Attrs["name"]) + if err != nil { + return err + } + e.Attrs["name"] = names + e.Attrs["push-by-digest"] = "true" + pushPrepared = true + } + } + return nil +} + type ( EvaluateFunc func(ctx context.Context, name string, c gateway.Client, res *gateway.Result, opt Options) error Handler struct { @@ -566,30 +601,8 @@ func BuildWithResultHandler(ctx context.Context, nodes []builder.Node, opts map[ node := dp.Node() so := reqForNodes[k][i].so if multiDriver { - for i, e := range so.Exports { - switch e.Type { - case "oci", "tar": - return errors.Errorf("%s for multi-node builds currently not supported", e.Type) - case "image": - if pushNames == "" && e.Attrs["push"] != "" { - if ok, _ := strconv.ParseBool(e.Attrs["push"]); ok { - pushNames = e.Attrs["name"] - if pushNames == "" { - return errors.Errorf("tag is needed when pushing to registry") - } - names, err := toRepoOnly(e.Attrs["name"]) - if err != nil { - return err - } - if ok, _ := strconv.ParseBool(e.Attrs["registry.insecure"]); ok { - insecurePush = true - } - e.Attrs["name"] = names - e.Attrs["push-by-digest"] = "true" - so.Exports[i].Attrs = e.Attrs - } - } - } + if err := prepareMultiDriverExports(so, &pushNames, &insecurePush); err != nil { + return err } } @@ -602,7 +615,8 @@ func BuildWithResultHandler(ctx context.Context, nodes []builder.Node, opts map[ // shared solver vertices land on the same daemon instance c = linkedClients[node.Name] if c == nil { - c, err = dp.Client(ctx) + // The shared client must outlive this target's errgroup. + c, err = dp.Client(baseCtx) if err == nil { linkedClients[node.Name] = c } diff --git a/build/build_test.go b/build/build_test.go index d7f65fb4e092..30450584782f 100644 --- a/build/build_test.go +++ b/build/build_test.go @@ -35,6 +35,48 @@ func (d warnOutputDriver) IsMobyDriver() bool { return d.moby } +func TestPrepareMultiDriverExportsForEveryNode(t *testing.T) { + const taggedName = "registry.example.com/user/app:latest" + const secondaryName = "registry.example.com/user/secondary:latest" + newSolveOpt := func() *client.SolveOpt { + return &client.SolveOpt{ + Exports: []client.ExportEntry{ + { + Type: "image", + Attrs: map[string]string{ + "name": taggedName, + "push": "true", + "registry.insecure": "true", + }, + }, + { + Type: "image", + Attrs: map[string]string{ + "name": secondaryName, + "push": "true", + }, + }, + }, + } + } + solveOpts := []*client.SolveOpt{newSolveOpt(), newSolveOpt()} + + var pushNames string + var insecurePush bool + for _, so := range solveOpts { + require.NoError(t, prepareMultiDriverExports(so, &pushNames, &insecurePush)) + } + + require.Equal(t, taggedName, pushNames) + require.True(t, insecurePush) + for _, so := range solveOpts { + require.Equal(t, "registry.example.com/user/app", so.Exports[0].Attrs["name"]) + require.Equal(t, "true", so.Exports[0].Attrs["push-by-digest"]) + require.Equal(t, secondaryName, so.Exports[1].Attrs["name"]) + require.NotContains(t, so.Exports[1].Attrs, "push-by-digest") + } +} + func TestWarnOnNoOutput(t *testing.T) { cloudNodes := []builder.Node{{Driver: newWarnOutputDriver("cloud", false)}} mobyNodes := []builder.Node{{Driver: newWarnOutputDriver("docker", true)}} diff --git a/commands/rm.go b/commands/rm.go index 3e61704fec3f..0e74f5147253 100644 --- a/commands/rm.go +++ b/commands/rm.go @@ -91,7 +91,7 @@ func runRm(ctx context.Context, dockerCli command.Cli, in rmOptions) error { return err } - err1 := rm(timeoutCtx, nodes, in) + err1 := rm(ctx, nodes, in) if err := txn.Remove(b.Name); err != nil { return err } @@ -200,7 +200,7 @@ func rmAllInactive(ctx context.Context, txn *store.Txn, dockerCli command.Cli, i return nil } if b.Inactive() { - rmerr := rm(timeoutCtx, nodes, in) + rmerr := rm(ctx, nodes, in) if err := txn.Remove(b.Name); err != nil { return err } diff --git a/driver/docker-container/driver.go b/driver/docker-container/driver.go index b9f95c8a9103..5800311f40d5 100644 --- a/driver/docker-container/driver.go +++ b/driver/docker-container/driver.go @@ -35,9 +35,10 @@ import ( ) const ( - volumeStateSuffix = "_state" - buildkitdConfigFile = "buildkitd.toml" - buildkitdStartupTimeout = 20 * time.Second + volumeStateSuffix = "_state" + buildkitdConfigFile = "buildkitd.toml" + buildkitdStartupWindow = 20 * time.Second + buildkitdReadyTimeout = 20 * time.Second ) type Driver struct { @@ -525,7 +526,7 @@ func (d *Driver) Client(ctx context.Context, opts ...client.ClientOpt) (*client. } return nil, errors.WithStack(err) } - waitDeadline, err := clientWaitDeadline(res.Container.State, time.Now()) + waitReady, err := clientWaitReady(res.Container.State, time.Now()) if err != nil { return nil, err } @@ -549,11 +550,11 @@ func (d *Driver) Client(ctx context.Context, opts ...client.ClientOpt) (*client. _ = conn.Close() return nil, err } - if waitDeadline.IsZero() { + if !waitReady { return c, nil } - waitCtx, cancel := context.WithDeadlineCause(ctx, waitDeadline, errors.WithStack(context.DeadlineExceeded)) + waitCtx, cancel := context.WithTimeoutCause(ctx, buildkitdReadyTimeout, errors.WithStack(context.DeadlineExceeded)) defer cancel() if err := c.Wait(waitCtx); err != nil { _ = c.Close() @@ -562,22 +563,18 @@ func (d *Driver) Client(ctx context.Context, opts ...client.ClientOpt) (*client. return c, nil } -func clientWaitDeadline(state *container.State, now time.Time) (time.Time, error) { +func clientWaitReady(state *container.State, now time.Time) (bool, error) { if state == nil || !state.Running { - return time.Time{}, driver.ErrNotRunning{} + return false, driver.ErrNotRunning{} } // Docker reports a container as running before buildkitd has bound its - // socket. Wait only during that startup window so an established but broken - // builder still returns its connection error promptly. + // socket. Use the startup window only to decide whether to wait so an + // established but broken builder still returns its connection error promptly. startedAt, err := time.Parse(time.RFC3339Nano, state.StartedAt) if err != nil { - return time.Time{}, nil + return false, nil } - deadline := startedAt.Add(buildkitdStartupTimeout) - if !now.Before(deadline) { - return time.Time{}, nil - } - return deadline, nil + return now.Before(startedAt.Add(buildkitdStartupWindow)), nil } func (d *Driver) Factory() driver.Factory { diff --git a/driver/docker-container/driver_test.go b/driver/docker-container/driver_test.go index 9a47579c426a..61d60ac6e9b0 100644 --- a/driver/docker-container/driver_test.go +++ b/driver/docker-container/driver_test.go @@ -9,40 +9,35 @@ import ( "github.com/stretchr/testify/require" ) -func TestClientWaitDeadline(t *testing.T) { +func TestClientWaitReady(t *testing.T) { now := time.Now() + running := func(startedAt string) *container.State { + return &container.State{Running: true, StartedAt: startedAt} + } + tests := []struct { + name string + state *container.State + wantWait bool + wantErr error + }{ + {name: "missing-state-fails-fast", wantErr: driver.ErrNotRunning{}}, + {name: "stopped-builder-fails-fast", state: &container.State{}, wantErr: driver.ErrNotRunning{}}, + {name: "established-builder-skips-wait", state: running(now.Add(-2 * buildkitdStartupWindow).Format(time.RFC3339Nano))}, + {name: "recent-start-builder-waits", state: running(now.Add(-time.Second).Format(time.RFC3339Nano)), wantWait: true}, + {name: "nearly-expired-startup-window-waits", state: running(now.Add(-buildkitdStartupWindow + time.Nanosecond).Format(time.RFC3339Nano)), wantWait: true}, + {name: "expired-startup-window-skips-wait", state: running(now.Add(-buildkitdStartupWindow).Format(time.RFC3339Nano))}, + {name: "invalid-start-time-skips-wait", state: running("invalid")}, + } - t.Run("stopped-builder-fails-fast", func(t *testing.T) { - deadline, err := clientWaitDeadline(&container.State{}, now) - require.ErrorIs(t, err, driver.ErrNotRunning{}) - require.True(t, deadline.IsZero()) - }) - - t.Run("established-builder-skips-wait", func(t *testing.T) { - deadline, err := clientWaitDeadline(&container.State{ - Running: true, - StartedAt: now.Add(-2 * buildkitdStartupTimeout).Format(time.RFC3339Nano), - }, now) - require.NoError(t, err) - require.True(t, deadline.IsZero()) - }) - - t.Run("recent-start-builder-waits", func(t *testing.T) { - startedAt := now.Add(-time.Second) - deadline, err := clientWaitDeadline(&container.State{ - Running: true, - StartedAt: startedAt.Format(time.RFC3339Nano), - }, now) - require.NoError(t, err) - require.True(t, deadline.Equal(startedAt.Add(buildkitdStartupTimeout))) - }) - - t.Run("invalid-start-time-skips-wait", func(t *testing.T) { - deadline, err := clientWaitDeadline(&container.State{ - Running: true, - StartedAt: "invalid", - }, now) - require.NoError(t, err) - require.True(t, deadline.IsZero()) - }) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + wait, err := clientWaitReady(tt.state, now) + if tt.wantErr != nil { + require.ErrorIs(t, err, tt.wantErr) + } else { + require.NoError(t, err) + } + require.Equal(t, tt.wantWait, wait) + }) + } } diff --git a/tests/build.go b/tests/build.go index 28b2e34c1210..e7d6b8a9b523 100644 --- a/tests/build.go +++ b/tests/build.go @@ -67,6 +67,7 @@ var buildTests = []func(t *testing.T, sb integration.Sandbox){ testBuildRegistryExport, testBuildRegistryExportAttestations, testBuildRegistryExportNoDefaultOCIArtifact, + testBuildMultiNodePushFailureDoesNotPublishTag, testBuildTarExport, testBuildMobyFromLocalImage, testBuildDetailsLink, @@ -689,6 +690,34 @@ func requireLegacyAttestationStorage(t *testing.T, sb integration.Sandbox, ref s require.NotEmpty(t, mfst.Layers) } +func testBuildMultiNodePushFailureDoesNotPublishTag(t *testing.T, sb integration.Sandbox) { + if !isRemoteMultiNodeWorker(sb) { + t.Skip("only testing with remote multi-node worker") + } + + dir := createTestProject(t) + err := os.WriteFile(filepath.Join(dir, "Dockerfile"), []byte(`FROM alpine +ARG TARGETARCH +RUN if [ "$TARGETARCH" = arm64 ]; then exit 1; fi +RUN echo "$TARGETARCH" > /platform +`), 0644) + require.NoError(t, err) + + registry, err := sb.NewRegistry() + if errors.Is(err, integration.ErrRequirements) { + t.Skip(err.Error()) + } + require.NoError(t, err) + + target := registry + "/buildx/partial-push:" + identity.NewID() + + out, err := buildCmd(sb, withArgs("-t", target, "--push", "--platform=linux/amd64,linux/arm64", "--provenance=false", dir)) + require.Error(t, err, string(out)) + + _, _, err = contentutil.ProviderFromRef(sb.Context(), target) + require.Error(t, err) +} + func testImageIDOutput(t *testing.T, sb integration.Sandbox) { dockerfile := []byte(`FROM busybox:latest`)