diff --git a/commands/rm.go b/commands/rm.go index ae94c7e6b..3e61704fe 100644 --- a/commands/rm.go +++ b/commands/rm.go @@ -2,6 +2,7 @@ package commands import ( "context" + stderrors "errors" "fmt" "time" @@ -90,7 +91,7 @@ func runRm(ctx context.Context, dockerCli command.Cli, in rmOptions) error { return err } - err1 := rm(ctx, nodes, in) + err1 := rm(timeoutCtx, nodes, in) if err := txn.Remove(b.Name); err != nil { return err } @@ -140,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 { @@ -187,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 cecd727f6..c67f8a07f 100644 --- a/tests/rm.go +++ b/tests/rm.go @@ -8,6 +8,7 @@ import ( "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" @@ -25,6 +26,8 @@ var rmTests = []func(t *testing.T, sb integration.Sandbox){ testRmMulti, testRmInvalidBuildkitdConfig, testRmAllInactiveInvalidBuildkitdConfig, + testRmUnreachableEndpoint, + testRmUnreachableRemoteEndpoint, } func testRm(t *testing.T, sb integration.Sandbox) { @@ -34,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) @@ -155,6 +159,65 @@ func testRmAllInactiveInvalidBuildkitdConfig(t *testing.T, sb integration.Sandbo 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()