From 091fb2c80d1b1e3d8c7aedc11f35a2bb631844a9 Mon Sep 17 00:00:00 2001 From: "Jonathan A. Sternberg" Date: Tue, 26 Sep 2023 16:46:49 -0500 Subject: [PATCH] solver: use toSelectors to filter root paths instead of custom logic This updates #4270 to add an integration test and also merge some of the logic for how the selectors are created. Now, `toSelectors` will perform the root path detection instead of some custom logic in `getMountDeps`. `dedupePaths` has also been updated to check if the number of paths is 1 or less so it can avoid an allocation when the function is a no-op. Signed-off-by: Jonathan A. Sternberg --- frontend/dockerfile/dockerfile_mount_test.go | 54 +++++++++++ solver/llbsolver/ops/exec.go | 38 ++++---- solver/llbsolver/ops/exec_test.go | 99 +++----------------- 3 files changed, 91 insertions(+), 100 deletions(-) diff --git a/frontend/dockerfile/dockerfile_mount_test.go b/frontend/dockerfile/dockerfile_mount_test.go index f146ea7aa806..1d48312b2805 100644 --- a/frontend/dockerfile/dockerfile_mount_test.go +++ b/frontend/dockerfile/dockerfile_mount_test.go @@ -1,6 +1,7 @@ package dockerfile import ( + "fmt" "os" "path/filepath" "testing" @@ -24,6 +25,7 @@ var mountTests = integration.TestFuncs( testMountFromError, testMountInvalid, testMountTmpfsSize, + testMountDuplicate, testCacheMountUser, ) @@ -468,3 +470,55 @@ COPY --from=base /tmpfssize / require.NoError(t, err) require.Contains(t, string(dt), `size=131072k`) } + +// moby/buildkit#4123 +func testMountDuplicate(t *testing.T, sb integration.Sandbox) { + f := getFrontend(t, sb) + + dockerfile := []byte(` +FROM busybox AS base +RUN --mount=source=.,target=/tmp/test \ + --mount=source=b.txt,target=/tmp/b.txt \ + cat /tmp/test/a.txt /tmp/b.txt > /combined.txt +FROM scratch +COPY --from=base /combined.txt / +`) + + c, err := client.New(sb.Context(), sb.Address()) + require.NoError(t, err) + defer c.Close() + + destDir := t.TempDir() + + // Run this dockerfile a few times. It should update the context + // for a.txt properly and update the output. + test := func(text string) { + dir := integration.Tmpdir( + t, + fstest.CreateFile("Dockerfile", dockerfile, 0600), + fstest.CreateFile("a.txt", []byte(text), 0600), + fstest.CreateFile("b.txt", []byte("bar\n"), 0600), + ) + + _, err = f.Solve(sb.Context(), c, client.SolveOpt{ + Exports: []client.ExportEntry{ + { + Type: client.ExporterLocal, + OutputDir: destDir, + }, + }, + LocalDirs: map[string]string{ + dockerui.DefaultLocalNameDockerfile: dir, + dockerui.DefaultLocalNameContext: dir, + }, + }, nil) + require.NoError(t, err) + + dt, err := os.ReadFile(filepath.Join(destDir, "combined.txt")) + require.NoError(t, err) + require.Equal(t, fmt.Sprintf("%sbar\n", text), string(dt)) + } + + test("foo\n") + test("updated\n") +} diff --git a/solver/llbsolver/ops/exec.go b/solver/llbsolver/ops/exec.go index c5b8c35b9753..19ce5e727976 100644 --- a/solver/llbsolver/ops/exec.go +++ b/solver/llbsolver/ops/exec.go @@ -187,6 +187,12 @@ func (e *ExecOp) CacheMap(ctx context.Context, g session.Group, index int) (*sol } func dedupePaths(inp []string) []string { + // If there's one or fewer inputs, then dedupe won't do anything. + // Skip the allocations and logic of this function in that case. + if len(inp) <= 1 { + return inp + } + old := make(map[string]struct{}, len(inp)) for _, p := range inp { old[p] = struct{}{} @@ -195,7 +201,10 @@ func dedupePaths(inp []string) []string { for p1 := range old { var skip bool for p2 := range old { - if p1 != p2 && strings.HasPrefix(p1, p2+"/") { + // Check if p2 is a prefix of p1. Ensure that p2 ends in a slash + // so that we know p2 is a parent directory of p1. We don't want + // /foo to be a parent of /foobar. + if p1 != p2 && strings.HasPrefix(p1, forceTrailingSlash(p2)) { skip = true break } @@ -210,9 +219,21 @@ func dedupePaths(inp []string) []string { return paths } +// forceTrailingSlash ensures that the path always ends with a path separator. +// If the path already ends with a /, this method returns the same string. +func forceTrailingSlash(s string) string { + if strings.HasSuffix(s, "/") { + return s + } + return s + "/" +} + func toSelectors(p []string) []opsutils.Selector { sel := make([]opsutils.Selector, 0, len(p)) for _, p := range p { + if p == "" || p == "/" { + return nil + } sel = append(sel, opsutils.Selector{Path: p, FollowLinks: true}) } return sel @@ -233,9 +254,6 @@ func (e *ExecOp) getMountDeps() ([]dep, error) { return nil, errors.Errorf("invalid mountinput %v", m) } - // Mark the selector path as used. In this section, we need to - // record root selectors so the selection criteria isn't narrowed - // erroneously. sel := path.Join("/", m.Selector) deps[m.Input].Selectors = append(deps[m.Input].Selectors, sel) @@ -243,18 +261,6 @@ func (e *ExecOp) getMountDeps() ([]dep, error) { deps[m.Input].NoContentBasedHash = true } } - - // Remove extraneous selectors that may have been generated from above. - for i, dep := range deps { - for _, sel := range dep.Selectors { - // If the root path is included in the list of selectors, - // this is the same as if no selector was used. Zero out this field. - if sel == "/" { - deps[i].Selectors = nil - break - } - } - } return deps, nil } diff --git a/solver/llbsolver/ops/exec_test.go b/solver/llbsolver/ops/exec_test.go index bd40b81e74e2..5e21b2857980 100644 --- a/solver/llbsolver/ops/exec_test.go +++ b/solver/llbsolver/ops/exec_test.go @@ -3,97 +3,28 @@ package ops import ( "testing" - "github.com/moby/buildkit/solver" - "github.com/moby/buildkit/solver/pb" - digest "github.com/opencontainers/go-digest" "github.com/stretchr/testify/require" ) -func TestDedupPaths(t *testing.T) { - res := dedupePaths([]string{"Gemfile", "Gemfile/foo"}) - require.Equal(t, []string{"Gemfile"}, res) +func TestDedupePaths(t *testing.T) { + res := dedupePaths([]string{"/Gemfile", "/Gemfile/foo"}) + require.Equal(t, []string{"/Gemfile"}, res) - res = dedupePaths([]string{"Gemfile/bar", "Gemfile/foo"}) - require.Equal(t, []string{"Gemfile/bar", "Gemfile/foo"}, res) + res = dedupePaths([]string{"/Gemfile/bar", "/Gemfile/foo"}) + require.Equal(t, []string{"/Gemfile/bar", "/Gemfile/foo"}, res) - res = dedupePaths([]string{"Gemfile", "Gemfile.lock"}) - require.Equal(t, []string{"Gemfile", "Gemfile.lock"}, res) + res = dedupePaths([]string{"/Gemfile", "/Gemfile.lock"}) + require.Equal(t, []string{"/Gemfile", "/Gemfile.lock"}, res) - res = dedupePaths([]string{"Gemfile.lock", "Gemfile"}) - require.Equal(t, []string{"Gemfile", "Gemfile.lock"}, res) + res = dedupePaths([]string{"/Gemfile.lock", "/Gemfile"}) + require.Equal(t, []string{"/Gemfile", "/Gemfile.lock"}, res) - res = dedupePaths([]string{"foo", "Gemfile", "Gemfile/foo"}) - require.Equal(t, []string{"Gemfile", "foo"}, res) + res = dedupePaths([]string{"/foo", "/Gemfile", "/Gemfile/foo"}) + require.Equal(t, []string{"/Gemfile", "/foo"}, res) - res = dedupePaths([]string{"foo/bar/baz", "foo/bara", "foo/bar/bax", "foo/bar"}) - require.Equal(t, []string{"foo/bar", "foo/bara"}, res) -} - -func TestExecOp_getMountDeps(t *testing.T) { - v1 := &vertex{name: "local://context"} - v2 := &vertex{ - name: "foo", - inputs: []solver.Edge{ - {Vertex: v1, Index: 0}, - }, - } - op2, err := NewExecOp(v2, &pb.Op_Exec{ - Exec: &pb.ExecOp{ - Meta: &pb.Meta{ - Args: []string{"/bin/bash", "-l"}, - }, - Mounts: []*pb.Mount{ - { - Input: pb.Empty, - Dest: "/", - Readonly: true, - Output: pb.SkipOutput, - }, - { - Input: pb.InputIndex(0), - Selector: "b.txt", - Dest: "/test/b.txt", - Output: pb.SkipOutput, - }, - { - Input: pb.InputIndex(0), - Dest: "/test/data", - Output: pb.SkipOutput, - }, - }, - }, - }, nil, nil, nil, nil, nil, nil) - require.NoError(t, err) - - deps, err := op2.getMountDeps() - require.NoError(t, err) - - require.Len(t, deps, 1) - require.Len(t, deps[0].Selectors, 0) - require.False(t, deps[0].NoContentBasedHash) -} - -type vertex struct { - name string - inputs []solver.Edge -} - -func (v *vertex) Digest() digest.Digest { - return digest.FromString(v.name) -} - -func (v *vertex) Sys() interface{} { - return v -} - -func (v *vertex) Options() solver.VertexOptions { - return solver.VertexOptions{} -} - -func (v *vertex) Inputs() []solver.Edge { - return v.inputs -} + res = dedupePaths([]string{"/foo/bar/baz", "/foo/bara", "/foo/bar/bax", "/foo/bar"}) + require.Equal(t, []string{"/foo/bar", "/foo/bara"}, res) -func (v *vertex) Name() string { - return v.name + res = dedupePaths([]string{"/", "/foo"}) + require.Equal(t, []string{"/"}, res) }