Merge pull request #35139 from simonferquel/fix-remaining-containers-on-fail

Test & Fix build with rm/force-rm matrix

builder/dockerfile/builder.go

@@ -322,7 +322,6 @@ func (b *Builder) dispatchDockerfileWithCancellation(parseResult []instructions.
 			if err := dispatch(dispatchRequest, cmd); err != nil {
 				return nil, err
 			}
-
 			dispatchRequest.state.updateRunConfig()
 			fmt.Fprintf(b.Stdout, " ---> %s\n", stringid.TruncateID(dispatchRequest.state.imageID))
 
@@ -335,9 +334,6 @@ func (b *Builder) dispatchDockerfileWithCancellation(parseResult []instructions.
 			return nil, err
 		}
 	}
-	if b.options.Remove {
-		b.containerManager.RemoveAll(b.Stdout)
-	}
 	buildArgs.WarnOnUnusedBuildArgs(b.Stdout)
 	return dispatchRequest.state, nil
 }

builder/dockerfile/evaluator.go

@@ -32,7 +32,7 @@ import (
 	"github.com/pkg/errors"
 )
 
-func dispatch(d dispatchRequest, cmd instructions.Command) error {
+func dispatch(d dispatchRequest, cmd instructions.Command) (err error) {
 	if c, ok := cmd.(instructions.PlatformSpecific); ok {
 		optionsOS := system.ParsePlatform(d.builder.options.Platform).OS
 		err := c.CheckPlatform(optionsOS)
@@ -52,10 +52,16 @@ func dispatch(d dispatchRequest, cmd instructions.Command) error {
 		}
 	}
 
+	defer func() {
 		if d.builder.options.ForceRemove {
-		defer d.builder.containerManager.RemoveAll(d.builder.Stdout)
+			d.builder.containerManager.RemoveAll(d.builder.Stdout)
+			return
 		}
-
+		if d.builder.options.Remove && err == nil {
+			d.builder.containerManager.RemoveAll(d.builder.Stdout)
+			return
+		}
+	}()
 	switch c := cmd.(type) {
 	case *instructions.EnvCommand:
 		return dispatchEnv(d, c)

integration/build/build_test.go

@@ -0,0 +1,130 @@
+package build
+
+import (
+	"archive/tar"
+	"bytes"
+	"context"
+	"encoding/json"
+	"io"
+	"strings"
+	"testing"
+
+	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/api/types/filters"
+	"github.com/docker/docker/integration/util/request"
+	"github.com/docker/docker/pkg/jsonmessage"
+	"github.com/stretchr/testify/require"
+)
+
+func TestBuildWithRemoveAndForceRemove(t *testing.T) {
+	t.Parallel()
+	cases := []struct {
+		name                           string
+		dockerfile                     string
+		numberOfIntermediateContainers int
+		rm                             bool
+		forceRm                        bool
+	}{
+		{
+			name: "successful build with no removal",
+			dockerfile: `FROM busybox
+			RUN exit 0
+			RUN exit 0`,
+			numberOfIntermediateContainers: 2,
+			rm:      false,
+			forceRm: false,
+		},
+		{
+			name: "successful build with remove",
+			dockerfile: `FROM busybox
+			RUN exit 0
+			RUN exit 0`,
+			numberOfIntermediateContainers: 0,
+			rm:      true,
+			forceRm: false,
+		},
+		{
+			name: "successful build with remove and force remove",
+			dockerfile: `FROM busybox
+			RUN exit 0
+			RUN exit 0`,
+			numberOfIntermediateContainers: 0,
+			rm:      true,
+			forceRm: true,
+		},
+		{
+			name: "failed build with no removal",
+			dockerfile: `FROM busybox
+			RUN exit 0
+			RUN exit 1`,
+			numberOfIntermediateContainers: 2,
+			rm:      false,
+			forceRm: false,
+		},
+		{
+			name: "failed build with remove",
+			dockerfile: `FROM busybox
+			RUN exit 0
+			RUN exit 1`,
+			numberOfIntermediateContainers: 1,
+			rm:      true,
+			forceRm: false,
+		},
+		{
+			name: "failed build with remove and force remove",
+			dockerfile: `FROM busybox
+			RUN exit 0
+			RUN exit 1`,
+			numberOfIntermediateContainers: 0,
+			rm:      true,
+			forceRm: true,
+		},
+	}
+
+	client := request.NewAPIClient(t)
+	ctx := context.Background()
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			t.Parallel()
+			dockerfile := []byte(c.dockerfile)
+
+			buff := bytes.NewBuffer(nil)
+			tw := tar.NewWriter(buff)
+			require.NoError(t, tw.WriteHeader(&tar.Header{
+				Name: "Dockerfile",
+				Size: int64(len(dockerfile)),
+			}))
+			_, err := tw.Write(dockerfile)
+			require.NoError(t, err)
+			require.NoError(t, tw.Close())
+			resp, err := client.ImageBuild(ctx, buff, types.ImageBuildOptions{Remove: c.rm, ForceRemove: c.forceRm, NoCache: true})
+			require.NoError(t, err)
+			defer resp.Body.Close()
+			filter, err := buildContainerIdsFilter(resp.Body)
+			require.NoError(t, err)
+			remainingContainers, err := client.ContainerList(ctx, types.ContainerListOptions{Filters: filter, All: true})
+			require.NoError(t, err)
+			require.Equal(t, c.numberOfIntermediateContainers, len(remainingContainers), "Expected %v remaining intermediate containers, got %v", c.numberOfIntermediateContainers, len(remainingContainers))
+		})
+	}
+}
+
+func buildContainerIdsFilter(buildOutput io.Reader) (filters.Args, error) {
+	const intermediateContainerPrefix = " ---> Running in "
+	filter := filters.NewArgs()
+
+	dec := json.NewDecoder(buildOutput)
+	for {
+		m := jsonmessage.JSONMessage{}
+		err := dec.Decode(&m)
+		if err == io.EOF {
+			return filter, nil
+		}
+		if err != nil {
+			return filter, err
+		}
+		if ix := strings.Index(m.Stream, intermediateContainerPrefix); ix != -1 {
+			filter.Add("id", strings.TrimSpace(m.Stream[ix+len(intermediateContainerPrefix):]))
+		}
+	}
+}

integration/build/main_test.go

@@ -0,0 +1,28 @@
+package build
+
+import (
+	"fmt"
+	"os"
+	"testing"
+
+	"github.com/docker/docker/internal/test/environment"
+)
+
+var testEnv *environment.Execution
+
+func TestMain(m *testing.M) {
+	var err error
+	testEnv, err = environment.New()
+	if err != nil {
+		fmt.Println(err)
+		os.Exit(1)
+	}
+
+	testEnv.Print()
+	os.Exit(m.Run())
+}
+
+func setupTest(t *testing.T) func() {
+	environment.ProtectAll(t, testEnv)
+	return func() { testEnv.Clean(t) }
+}