Merge pull request #1382 from tonistiigi/cache-loop

solver: avoid recursive loop on cache-export
moby · Mar 3, 2020 · 09900f3 · 09900f3
2 parents f1ecc78 + 09e8a06
commit 09900f3
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 3 deletions.
diff --git a/client/client_test.go b/client/client_test.go
@@ -61,6 +61,7 @@ func TestIntegration(t *testing.T) {
 	mirrors := integration.WithMirroredImages(integration.OfficialImages("busybox:latest", "alpine:latest"))
 
 	integration.Run(t, []integration.Test{
+		testCacheExportCacheKeyLoop,
 		testRelativeWorkDir,
 		testFileOpMkdirMkfile,
 		testFileOpCopyRm,
@@ -135,6 +136,51 @@ func newContainerd(cdAddress string) (*containerd.Client, error) {
 	return containerd.New(cdAddress, containerd.WithTimeout(60*time.Second), containerd.WithDefaultRuntime("io.containerd.runtime.v1.linux"))
 }
 
+// moby/buildkit#1336
+func testCacheExportCacheKeyLoop(t *testing.T, sb integration.Sandbox) {
+	c, err := New(context.TODO(), sb.Address())
+	require.NoError(t, err)
+	defer c.Close()
+
+	tmpdir, err := ioutil.TempDir("", "buildkit-buildctl")
+	require.NoError(t, err)
+	defer os.RemoveAll(tmpdir)
+
+	err = ioutil.WriteFile(filepath.Join(tmpdir, "foo"), []byte("foodata"), 0600)
+	require.NoError(t, err)
+
+	for _, mode := range []bool{false, true} {
+		func(mode bool) {
+			t.Run(fmt.Sprintf("mode=%v", mode), func(t *testing.T) {
+				buildbase := llb.Image("alpine:latest").File(llb.Copy(llb.Local("mylocal"), "foo", "foo"))
+				if mode { // same cache keys with a separating node go to different code-path
+					buildbase = buildbase.Run(llb.Shlex("true")).Root()
+				}
+				intermed := llb.Image("alpine:latest").File(llb.Copy(buildbase, "foo", "foo"))
+				final := llb.Scratch().File(llb.Copy(intermed, "foo", "foooooo"))
+
+				def, err := final.Marshal()
+				require.NoError(t, err)
+
+				_, err = c.Solve(context.TODO(), def, SolveOpt{
+					CacheExports: []CacheOptionsEntry{
+						{
+							Type: "local",
+							Attrs: map[string]string{
+								"dest": filepath.Join(tmpdir, "cache"),
+							},
+						},
+					},
+					LocalDirs: map[string]string{
+						"mylocal": tmpdir,
+					},
+				}, nil)
+				require.NoError(t, err)
+			})
+		}(mode)
+	}
+}
+
 func testBridgeNetworking(t *testing.T, sb integration.Sandbox) {
 	if os.Getenv("BUILDKIT_RUN_NETWORK_INTEGRATION_TESTS") == "" {
 		t.SkipNow()

diff --git a/solver/exporter.go b/solver/exporter.go
@@ -20,11 +20,12 @@ func addBacklinks(t CacheExporterTarget, rec CacheExporterRecord, cm *cacheManag
 	if rec == nil {
 		var ok bool
 		rec, ok = bkm[id]
-		if ok {
+		if ok && rec != nil {
 			return rec, nil
 		}
 		_ = ok
 	}
+	bkm[id] = nil
 	if err := cm.backend.WalkBacklinks(id, func(id string, link CacheInfoLink) error {
 		if rec == nil {
 			rec = t.Add(link.Digest)
@@ -37,7 +38,9 @@ func addBacklinks(t CacheExporterTarget, rec CacheExporterRecord, cm *cacheManag
 				return err
 			}
 		}
-		rec.LinkFrom(r, int(link.Input), link.Selector.String())
+		if r != nil {
+			rec.LinkFrom(r, int(link.Input), link.Selector.String())
+		}
 		return nil
 	}); err != nil {
 		return nil, err
@@ -66,6 +69,7 @@ func (e *exporter) ExportTo(ctx context.Context, t CacheExporterTarget, opt Cach
 	if t.Visited(e) {
 		return e.res, nil
 	}
+	t.Visit(e)
 
 	deps := e.k.Deps()
 
@@ -177,7 +181,6 @@ func (e *exporter) ExportTo(ctx context.Context, t CacheExporterTarget, opt Cach
 	}
 
 	e.res = allRec
-	t.Visit(e)
 
 	return e.res, nil
 }