diff --git a/builder/node.go b/builder/node.go index b1bccbf77b83..abffe0ce10d8 100644 --- a/builder/node.go +++ b/builder/node.go @@ -47,9 +47,10 @@ func (b *Builder) Nodes() []Node { type LoadNodesOption func(*loadNodesOptions) type loadNodesOptions struct { - data bool - dialMeta map[string][]string - clientOpt []client.ClientOpt + data bool + skipImageOpt bool + dialMeta map[string][]string + clientOpt []client.ClientOpt } func WithData() LoadNodesOption { @@ -58,6 +59,12 @@ func WithData() LoadNodesOption { } } +func WithSkippedImageOpt() LoadNodesOption { + return func(o *loadNodesOptions) { + o.skipImageOpt = true + } +} + func WithDialMeta(dialMeta map[string][]string) LoadNodesOption { return func(o *loadNodesOptions) { o.dialMeta = dialMeta @@ -94,9 +101,12 @@ func (b *Builder) LoadNodes(ctx context.Context, opts ...LoadNodesOption) (_ []N return nil, err } - imageopt, err := b.ImageOpt() - if err != nil { - return nil, err + var imageopt imagetools.Opt + if !lno.skipImageOpt { + imageopt, err = b.ImageOpt() + if err != nil { + return nil, err + } } for i, n := range b.NodeGroup.Nodes { diff --git a/commands/rm.go b/commands/rm.go index 44151121bcc6..3e61704fec3f 100644 --- a/commands/rm.go +++ b/commands/rm.go @@ -2,6 +2,7 @@ package commands import ( "context" + stderrors "errors" "fmt" "time" @@ -78,12 +79,19 @@ func runRm(ctx context.Context, dockerCli command.Cli, in rmOptions) error { return errors.Errorf("context builder cannot be removed, run `docker context rm %s` to remove this context", b.Name) } - nodes, err := b.LoadNodes(timeoutCtx) + if in.keepDaemon { + return txn.Remove(b.Name) + } + + nodes, err := b.LoadNodes(timeoutCtx, builder.WithSkippedImageOpt()) if err != nil { + if err1 := txn.Remove(b.Name); err1 != nil { + return err1 + } return err } - err1 := rm(ctx, nodes, in) + err1 := rm(timeoutCtx, nodes, in) if err := txn.Remove(b.Name); err != nil { return err } @@ -133,24 +141,36 @@ func rmCmd(dockerCli command.Cli, rootOpts *rootOptions) *cobra.Command { } func rm(ctx context.Context, nodes []builder.Node, in rmOptions) (err error) { + errCh := make(chan error, len(nodes)*3) + var eg errgroup.Group for _, node := range nodes { - if node.Driver == nil { - continue - } - // Do not stop the buildkitd daemon when --keep-daemon is provided - if !in.keepDaemon { - if err := node.Driver.Stop(ctx, true); err != nil { - return err + eg.Go(func() error { + if node.Err != nil { + errCh <- errors.Wrapf(node.Err, "failed to load node %s", node.Name) } - } - if err := node.Driver.Rm(ctx, true, !in.keepState, !in.keepDaemon); err != nil { - return err - } - if node.Err != nil { - err = node.Err - } + if node.Driver == nil { + return nil + } + // Do not stop the buildkitd daemon when --keep-daemon is provided + if !in.keepDaemon { + if err := node.Driver.Stop(ctx, true); err != nil { + errCh <- errors.Wrapf(err, "failed to stop node %s", node.Name) + } + } + if err := node.Driver.Rm(ctx, true, !in.keepState, !in.keepDaemon); err != nil { + errCh <- errors.Wrapf(err, "failed to remove node %s", node.Name) + } + return nil + }) } - return err + _ = eg.Wait() + close(errCh) + + var errs []error + for err := range errCh { + errs = append(errs, err) + } + return stderrors.Join(errs...) } func rmAllInactive(ctx context.Context, txn *store.Txn, dockerCli command.Cli, in rmOptions) error { @@ -169,7 +189,10 @@ func rmAllInactive(ctx context.Context, txn *store.Txn, dockerCli command.Cli, i for _, b := range builders { func(b *builder.Builder) { eg.Go(func() error { - nodes, err := b.LoadNodes(timeoutCtx, builder.WithData()) + if b.DockerContext { + return nil + } + nodes, err := b.LoadNodes(timeoutCtx, builder.WithData(), builder.WithSkippedImageOpt()) if err != nil { return errors.Wrapf(err, "cannot load %s", b.Name) } @@ -177,7 +200,7 @@ func rmAllInactive(ctx context.Context, txn *store.Txn, dockerCli command.Cli, i return nil } if b.Inactive() { - rmerr := rm(ctx, nodes, in) + rmerr := rm(timeoutCtx, nodes, in) if err := txn.Remove(b.Name); err != nil { return err } diff --git a/tests/rm.go b/tests/rm.go index 1eb8c070e7dd..c67f8a07f70a 100644 --- a/tests/rm.go +++ b/tests/rm.go @@ -1,10 +1,16 @@ package tests import ( + "os" "strings" "testing" + "github.com/docker/buildx/driver" + "github.com/docker/buildx/store" + "github.com/docker/buildx/util/confutil" + "github.com/moby/buildkit/identity" "github.com/moby/buildkit/util/testutil/integration" + "github.com/pkg/errors" "github.com/stretchr/testify/require" ) @@ -18,6 +24,10 @@ func rmCmd(sb integration.Sandbox, opts ...cmdOpt) (string, error) { var rmTests = []func(t *testing.T, sb integration.Sandbox){ testRm, testRmMulti, + testRmInvalidBuildkitdConfig, + testRmAllInactiveInvalidBuildkitdConfig, + testRmUnreachableEndpoint, + testRmUnreachableRemoteEndpoint, } func testRm(t *testing.T, sb integration.Sandbox) { @@ -27,6 +37,7 @@ func testRm(t *testing.T, sb integration.Sandbox) { out, err := rmCmd(sb, withArgs("default")) require.Error(t, err, out) // can't remove a docker builder + require.Contains(t, out, "context builder cannot be removed") out, err = createCmd(sb, withArgs("--driver", "docker-container")) require.NoError(t, err, out) @@ -58,3 +69,190 @@ func testRmMulti(t *testing.T, sb integration.Sandbox) { out, err := rmCmd(sb, withArgs(builderNames...)) require.NoError(t, err, out) } + +func testRmInvalidBuildkitdConfig(t *testing.T, sb integration.Sandbox) { + if !isDockerContainerWorker(sb) { + t.Skip("only testing with docker-container worker") + } + + out, err := createCmd(sb, withArgs("--driver", "docker-container")) + require.NoError(t, err, out) + builderName := strings.TrimSpace(out) + + out, err = inspectCmd(sb, withArgs(builderName, "--bootstrap")) + require.NoError(t, err, out) + + var container string + t.Cleanup(func() { + if builderName != "" { + _, _ = rmCmd(sb, withArgs("--keep-daemon", builderName)) + } + if container != "" { + _ = dockerCmd(sb, withArgs("container", "rm", "-f", container)).Run() + } + }) + + updateStoredBuilder(t, sb, builderName, func(ng *store.NodeGroup) { + require.NotEmpty(t, ng.Nodes) + container = driver.BuilderName(ng.Nodes[0].Name) + + if ng.Nodes[0].Files == nil { + ng.Nodes[0].Files = map[string][]byte{} + } + ng.Nodes[0].Files["buildkitd.toml"] = []byte(` +[worker.oci] + gc = "maybe" +`) + }) + + out, err = rmCmd(sb, withArgs(builderName)) + require.NoError(t, err, out) + require.Contains(t, out, builderName+" removed") + requireNoStoredBuilder(t, sb, builderName) + requireNoContainer(t, sb, container) + builderName = "" +} + +func testRmAllInactiveInvalidBuildkitdConfig(t *testing.T, sb integration.Sandbox) { + if !isDockerContainerWorker(sb) { + t.Skip("only testing with docker-container worker") + } + + out, err := createCmd(sb, withArgs("--driver", "docker-container")) + require.NoError(t, err, out) + builderName := strings.TrimSpace(out) + + out, err = inspectCmd(sb, withArgs(builderName, "--bootstrap")) + require.NoError(t, err, out) + + var container string + t.Cleanup(func() { + if builderName != "" { + _, _ = rmCmd(sb, withArgs("--keep-daemon", builderName)) + } + if container != "" { + _ = dockerCmd(sb, withArgs("container", "rm", "-f", container)).Run() + } + }) + + updateStoredBuilder(t, sb, builderName, func(ng *store.NodeGroup) { + require.NotEmpty(t, ng.Nodes) + container = driver.BuilderName(ng.Nodes[0].Name) + + if ng.Nodes[0].Files == nil { + ng.Nodes[0].Files = map[string][]byte{} + } + ng.Nodes[0].Files["buildkitd.toml"] = []byte(` +[worker.oci] + gc = "maybe" +`) + }) + + cmd := dockerCmd(sb, withArgs("container", "stop", container)) + require.NoError(t, cmd.Run()) + + out, err = rmCmd(sb, withArgs("--all-inactive", "--force")) + require.NoError(t, err, out) + require.Contains(t, out, builderName+" removed") + requireNoStoredBuilder(t, sb, builderName) + requireNoContainer(t, sb, container) + builderName = "" +} + +func testRmUnreachableEndpoint(t *testing.T, sb integration.Sandbox) { + if !isDockerContainerWorker(sb) { + t.Skip("only testing with docker-container worker") + } + + out, err := createCmd(sb, withArgs("--driver", "docker-container")) + require.NoError(t, err, out) + builderName := strings.TrimSpace(out) + + out, err = inspectCmd(sb, withArgs(builderName, "--bootstrap")) + require.NoError(t, err, out) + + t.Cleanup(func() { + if builderName == "" { + return + } + _, _ = rmCmd(sb, withArgs("--keep-daemon", builderName)) + }) + + var goodContainer string + updateStoredBuilder(t, sb, builderName, func(ng *store.NodeGroup) { + require.NotEmpty(t, ng.Nodes) + goodContainer = driver.BuilderName(ng.Nodes[0].Name) + badNode := ng.Nodes[0] + badNode.Name += "-unreachable" + badNode.Endpoint = "tcp://127.0.0.1:1" + ng.Nodes = append([]store.Node{badNode}, ng.Nodes...) + }) + + out, err = rmCmd(sb, withArgs("--timeout=2s", builderName)) + require.Error(t, err, out) + require.Contains(t, out, "failed to remove "+builderName) + requireNoStoredBuilder(t, sb, builderName) + requireNoContainer(t, sb, goodContainer) + builderName = "" +} + +func testRmUnreachableRemoteEndpoint(t *testing.T, sb integration.Sandbox) { + if !isRemoteWorker(sb) || isRemoteMultiNodeWorker(sb) { + t.Skip("only testing with remote worker") + } + + builderName := "remote-" + identity.NewID() + out, err := createCmd(sb, withArgs("--driver", "remote", "--name", builderName, "--timeout=2s", "tcp://127.0.0.1:1")) + require.NoError(t, err, out) + + t.Cleanup(func() { + if builderName != "" { + _, _ = rmCmd(sb, withArgs("--timeout=2s", builderName)) + } + }) + + out, err = rmCmd(sb, withArgs("--timeout=2s", builderName)) + require.NoError(t, err, out) + require.Contains(t, out, builderName+" removed") + requireNoStoredBuilder(t, sb, builderName) + builderName = "" +} + +func updateStoredBuilder(t *testing.T, sb integration.Sandbox, name string, fn func(*store.NodeGroup)) { + t.Helper() + + st, err := store.New(confutil.NewConfig(nil, confutil.WithDir(buildxConfig(sb)))) + require.NoError(t, err) + + txn, release, err := st.Txn() + require.NoError(t, err) + defer release() + + ng, err := txn.NodeGroupByName(name) + require.NoError(t, err) + + fn(ng) + require.NoError(t, txn.Save(ng)) +} + +func requireNoStoredBuilder(t *testing.T, sb integration.Sandbox, name string) { + t.Helper() + + st, err := store.New(confutil.NewConfig(nil, confutil.WithDir(buildxConfig(sb)))) + require.NoError(t, err) + + txn, release, err := st.Txn() + require.NoError(t, err) + defer release() + + _, err = txn.NodeGroupByName(name) + require.Error(t, err) + require.True(t, os.IsNotExist(errors.Cause(err)), "expected builder %q to be removed: %v", name, err) +} + +func requireNoContainer(t *testing.T, sb integration.Sandbox, name string) { + t.Helper() + + cmd := dockerCmd(sb, withArgs("container", "inspect", name)) + require.Error(t, cmd.Run()) +}