From 4f6f49dd81062335e09d5ae43ba0677d245c5568 Mon Sep 17 00:00:00 2001 From: CrazyMax <1951866+crazy-max@users.noreply.github.com> Date: Mon, 6 Jul 2026 11:12:24 +0200 Subject: [PATCH] rm: allow removing builders with invalid config Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com> --- builder/node.go | 22 +++++--- commands/rm.go | 14 ++++- tests/rm.go | 135 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 163 insertions(+), 8 deletions(-) diff --git a/builder/node.go b/builder/node.go index b1bccbf77..abffe0ce1 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 44151121b..ae94c7e6b 100644 --- a/commands/rm.go +++ b/commands/rm.go @@ -78,8 +78,15 @@ 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 } @@ -169,7 +176,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) } diff --git a/tests/rm.go b/tests/rm.go index 1eb8c070e..cecd727f6 100644 --- a/tests/rm.go +++ b/tests/rm.go @@ -1,10 +1,15 @@ 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/util/testutil/integration" + "github.com/pkg/errors" "github.com/stretchr/testify/require" ) @@ -18,6 +23,8 @@ func rmCmd(sb integration.Sandbox, opts ...cmdOpt) (string, error) { var rmTests = []func(t *testing.T, sb integration.Sandbox){ testRm, testRmMulti, + testRmInvalidBuildkitdConfig, + testRmAllInactiveInvalidBuildkitdConfig, } func testRm(t *testing.T, sb integration.Sandbox) { @@ -58,3 +65,131 @@ 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 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()) +}