rm: handle broken builders during removal

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 {

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,15 +189,18 @@ 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)
 				}
 				if b.Dynamic {
 					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
 					}

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())
+}