Merge pull request #5306 from tonistiigi/cache-mount-mode-prune

exec: fix pruning cache mounts with parent ref on no-cache

cache/metadata.go

@@ -44,7 +44,7 @@ const blobchainIndex = "blobchainid:"
 const chainIndex = "chainid:"
 
 type MetadataStore interface {
-	Search(context.Context, string) ([]RefMetadata, error)
+	Search(context.Context, string, bool) ([]RefMetadata, error)
 }
 
 type RefMetadata interface {
@@ -71,6 +71,7 @@ type RefMetadata interface {
 
 	// generic getters/setters for external packages
 	GetString(string) string
+	Get(string) *metadata.Value
 	SetString(key, val, index string) error
 
 	GetExternal(string) ([]byte, error)
@@ -79,15 +80,15 @@ type RefMetadata interface {
 	ClearValueAndIndex(string, string) error
 }
 
-func (cm *cacheManager) Search(ctx context.Context, idx string) ([]RefMetadata, error) {
+func (cm *cacheManager) Search(ctx context.Context, idx string, prefixOnly bool) ([]RefMetadata, error) {
 	cm.mu.Lock()
 	defer cm.mu.Unlock()
-	return cm.search(ctx, idx)
+	return cm.search(ctx, idx, prefixOnly)
 }
 
 // callers must hold cm.mu lock
-func (cm *cacheManager) search(ctx context.Context, idx string) ([]RefMetadata, error) {
-	sis, err := cm.MetadataStore.Search(ctx, idx)
+func (cm *cacheManager) search(ctx context.Context, idx string, prefixOnly bool) ([]RefMetadata, error) {
+	sis, err := cm.MetadataStore.Search(ctx, idx, prefixOnly)
 	if err != nil {
 		return nil, err
 	}
@@ -119,12 +120,12 @@ func (cm *cacheManager) getMetadata(id string) (*cacheMetadata, bool) {
 
 // callers must hold cm.mu lock
 func (cm *cacheManager) searchBlobchain(ctx context.Context, id digest.Digest) ([]RefMetadata, error) {
-	return cm.search(ctx, blobchainIndex+id.String())
+	return cm.search(ctx, blobchainIndex+id.String(), false)
 }
 
 // callers must hold cm.mu lock
 func (cm *cacheManager) searchChain(ctx context.Context, id digest.Digest) ([]RefMetadata, error) {
-	return cm.search(ctx, chainIndex+id.String())
+	return cm.search(ctx, chainIndex+id.String(), false)
 }
 
 type cacheMetadata struct {
@@ -486,6 +487,10 @@ func (md *cacheMetadata) GetString(key string) string {
 	return str
 }
 
+func (md *cacheMetadata) Get(key string) *metadata.Value {
+	return md.si.Get(key)
+}
+
 func (md *cacheMetadata) GetStringSlice(key string) []string {
 	v := md.si.Get(key)
 	if v == nil {

cache/metadata/metadata.go

@@ -83,7 +83,7 @@ func (s *Store) Probe(index string) (bool, error) {
 	return exists, errors.WithStack(err)
 }
 
-func (s *Store) Search(ctx context.Context, index string) ([]*StorageItem, error) {
+func (s *Store) Search(ctx context.Context, index string, prefix bool) ([]*StorageItem, error) {
 	var out []*StorageItem
 	err := s.db.View(func(tx *bolt.Tx) error {
 		b := tx.Bucket([]byte(indexBucket))
@@ -94,12 +94,18 @@ func (s *Store) Search(ctx context.Context, index string) ([]*StorageItem, error
 		if main == nil {
 			return nil
 		}
-		index = indexKey(index, "")
+		if !prefix {
+			index = indexKey(index, "")
+		}
 		c := b.Cursor()
 		k, _ := c.Seek([]byte(index))
 		for {
 			if k != nil && strings.HasPrefix(string(k), index) {
-				itemID := strings.TrimPrefix(string(k), index)
+				idx := strings.LastIndex(string(k), "::")
+				if idx == -1 {
+					continue
+				}
+				itemID := string(k[idx+2:])
 				k, _ = c.Next()
 				b := main.Bucket([]byte(itemID))
 				if b == nil {

cache/metadata/metadata_test.go

@@ -142,14 +142,14 @@ func TestIndexes(t *testing.T) {
 	}
 
 	ctx := context.Background()
-	sis, err := s.Search(ctx, "tag:baz")
+	sis, err := s.Search(ctx, "tag:baz", false)
 	require.NoError(t, err)
 	require.Equal(t, 2, len(sis))
 
 	require.Equal(t, "foo1", sis[0].ID())
 	require.Equal(t, "foo3", sis[1].ID())
 
-	sis, err = s.Search(ctx, "tag:bax")
+	sis, err = s.Search(ctx, "tag:bax", false)
 	require.NoError(t, err)
 	require.Equal(t, 1, len(sis))
 
@@ -158,7 +158,7 @@ func TestIndexes(t *testing.T) {
 	err = s.Clear("foo1")
 	require.NoError(t, err)
 
-	sis, err = s.Search(ctx, "tag:baz")
+	sis, err = s.Search(ctx, "tag:baz", false)
 	require.NoError(t, err)
 	require.Equal(t, 1, len(sis))
 

frontend/dockerfile/dockerfile2llb/convert_runmount.go

@@ -57,7 +57,7 @@ func setCacheUIDGID(m *instructions.Mount, st llb.State) llb.State {
 	if m.Mode != nil {
 		mode = os.FileMode(*m.Mode)
 	}
-	return st.File(llb.Mkdir("/cache", mode, llb.WithUIDGID(uid, gid)), llb.WithCustomName("[internal] settings cache mount permissions"))
+	return st.File(llb.Mkdir("/cache", mode, llb.WithUIDGID(uid, gid)), llb.WithCustomName("[internal] setting cache mount permissions"))
 }
 
 func dispatchRunMounts(d *dispatchState, c *instructions.RunCommand, sources []*dispatchState, opt dispatchOpt) ([]llb.RunOption, error) {

frontend/dockerfile/dockerfile_test.go

@@ -100,6 +100,7 @@ var allTests = integration.TestFuncs(
 	testReproducibleIDs,
 	testImportExportReproducibleIDs,
 	testNoCache,
+	testCacheMountModeNoCache,
 	testDockerfileFromHTTP,
 	testBuiltinArgs,
 	testPullScratch,
@@ -5065,6 +5066,74 @@ COPY --from=s1 unique2 /
 	require.NotEqual(t, string(unique2Dir1), string(unique2Dir3))
 }
 
+// moby/buildkit#5305
+func testCacheMountModeNoCache(t *testing.T, sb integration.Sandbox) {
+	integration.SkipOnPlatform(t, "windows")
+	f := getFrontend(t, sb)
+
+	dockerfile := []byte(`
+FROM busybox AS base
+ARG FOO=abc
+RUN --mount=type=cache,target=/cache,mode=0773 touch /cache/$FOO && ls -l /cache | wc -l > /out
+
+FROM scratch
+COPY --from=base /out /
+`)
+	dir := integration.Tmpdir(
+		t,
+		fstest.CreateFile("Dockerfile", dockerfile, 0600),
+	)
+
+	c, err := client.New(sb.Context(), sb.Address())
+	require.NoError(t, err)
+	defer c.Close()
+
+	destDir := t.TempDir()
+
+	opt := client.SolveOpt{
+		FrontendAttrs: map[string]string{},
+		Exports: []client.ExportEntry{
+			{
+				Type:      client.ExporterLocal,
+				OutputDir: destDir,
+			},
+		},
+		LocalMounts: map[string]fsutil.FS{
+			dockerui.DefaultLocalNameDockerfile: dir,
+			dockerui.DefaultLocalNameContext:    dir,
+		},
+	}
+
+	_, err = f.Solve(sb.Context(), c, opt, nil)
+	require.NoError(t, err)
+
+	opt.FrontendAttrs["no-cache"] = ""
+
+	dt, err := os.ReadFile(filepath.Join(destDir, "out"))
+	require.NoError(t, err)
+	require.Equal(t, "2\n", string(dt))
+
+	opt.FrontendAttrs["build-arg:FOO"] = "def"
+
+	_, err = f.Solve(sb.Context(), c, opt, nil)
+	require.NoError(t, err)
+
+	dt, err = os.ReadFile(filepath.Join(destDir, "out"))
+	require.NoError(t, err)
+	require.Equal(t, "2\n", string(dt))
+
+	// safety check without no-cache
+	delete(opt.FrontendAttrs, "no-cache")
+	opt.FrontendAttrs["build-arg:FOO"] = "ghi"
+
+	_, err = f.Solve(sb.Context(), c, opt, nil)
+	require.NoError(t, err)
+
+	dt, err = os.ReadFile(filepath.Join(destDir, "out"))
+	require.NoError(t, err)
+	require.Equal(t, "3\n", string(dt))
+}
+
 func testPlatformArgsImplicit(t *testing.T, sb integration.Sandbox) {
 	integration.SkipOnPlatform(t, "windows")
 	f := getFrontend(t, sb)

solver/llbsolver/bridge.go

@@ -144,12 +144,8 @@ func (b *llbBridge) loadResult(ctx context.Context, def *pb.Definition, cacheImp
 	}
 
 	if len(dpc.ids) > 0 {
-		ids := make([]string, 0, len(dpc.ids))
-		for id := range dpc.ids {
-			ids = append(ids, id)
-		}
 		if err := b.eachWorker(func(w worker.Worker) error {
-			return w.PruneCacheMounts(ctx, ids)
+			return w.PruneCacheMounts(ctx, dpc.ids)
 		}); err != nil {
 			return nil, err
 		}

solver/llbsolver/mounts/mount.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 	"sync"
 	"time"
 
@@ -116,7 +117,7 @@ func (g *cacheRefGetter) getRefCacheDirNoCache(ctx context.Context, key string,
 	cacheRefsLocker.Lock(key)
 	defer cacheRefsLocker.Unlock(key)
 	for {
-		sis, err := SearchCacheDir(ctx, g.cm, key)
+		sis, err := SearchCacheDir(ctx, g.cm, key, false)
 		if err != nil {
 			return nil, err
 		}
@@ -542,13 +543,24 @@ func (r *cacheRef) Release(ctx context.Context) error {
 const keyCacheDir = "cache-dir"
 const cacheDirIndex = keyCacheDir + ":"
 
-func SearchCacheDir(ctx context.Context, store cache.MetadataStore, id string) ([]CacheRefMetadata, error) {
+func SearchCacheDir(ctx context.Context, store cache.MetadataStore, id string, withNested bool) ([]CacheRefMetadata, error) {
 	var results []CacheRefMetadata
-	mds, err := store.Search(ctx, cacheDirIndex+id)
+	key := cacheDirIndex + id
+	if withNested {
+		key += ":"
+	}
+	mds, err := store.Search(ctx, key, withNested)
 	if err != nil {
 		return nil, err
 	}
 	for _, md := range mds {
+		if withNested {
+			v := md.Get(keyCacheDir)
+			// skip partial ids but allow id without ref ID
+			if v == nil || v.Index != key && !strings.HasPrefix(v.Index, key) {
+				continue
+			}
+		}
 		results = append(results, CacheRefMetadata{md})
 	}
 	return results, nil

solver/llbsolver/vertex.go

@@ -125,7 +125,7 @@ func ValidateEntitlements(ent entitlements.Set) LoadOpt {
 }
 
 type detectPrunedCacheID struct {
-	ids map[string]struct{}
+	ids map[string]bool
 }
 
 func (dpc *detectPrunedCacheID) Load(op *pb.Op, md *pb.OpMetadata, opt *solver.VertexOptions) error {
@@ -142,9 +142,10 @@ func (dpc *detectPrunedCacheID) Load(op *pb.Op, md *pb.OpMetadata, opt *solver.V
 						id = m.Dest
 					}
 					if dpc.ids == nil {
-						dpc.ids = map[string]struct{}{}
+						dpc.ids = map[string]bool{}
 					}
-					dpc.ids[id] = struct{}{}
+					// value shows in mount is on top of a ref
+					dpc.ids[id] = m.Input != -1
 				}
 			}
 		}

source/git/source.go

@@ -746,7 +746,7 @@ const gitSnapshotIndex = keyGitSnapshot + "::"
 
 func search(ctx context.Context, store cache.MetadataStore, key string, idx string) ([]cacheRefMetadata, error) {
 	var results []cacheRefMetadata
-	mds, err := store.Search(ctx, idx+key)
+	mds, err := store.Search(ctx, idx+key, false)
 	if err != nil {
 		return nil, err
 	}

source/http/source.go

@@ -480,7 +480,7 @@ func getFileName(urlStr, manualFilename string, resp *http.Response) string {
 
 func searchHTTPURLDigest(ctx context.Context, store cache.MetadataStore, dgst digest.Digest) ([]cacheRefMetadata, error) {
 	var results []cacheRefMetadata
-	mds, err := store.Search(ctx, string(dgst))
+	mds, err := store.Search(ctx, string(dgst), false)
 	if err != nil {
 		return nil, err
 	}

source/local/source.go

@@ -334,7 +334,7 @@ const sharedKeyIndex = keySharedKey + ":"
 
 func searchSharedKey(ctx context.Context, store cache.MetadataStore, k string) ([]cacheRefMetadata, error) {
 	var results []cacheRefMetadata
-	mds, err := store.Search(ctx, sharedKeyIndex+k)
+	mds, err := store.Search(ctx, sharedKeyIndex+k, false)
 	if err != nil {
 		return nil, err
 	}

worker/base/worker.go

@@ -341,13 +341,13 @@ func (w *Worker) ResolveOp(v solver.Vertex, s frontend.FrontendLLBBridge, sm *se
 	return nil, errors.Errorf("could not resolve %v", v)
 }
 
-func (w *Worker) PruneCacheMounts(ctx context.Context, ids []string) error {
+func (w *Worker) PruneCacheMounts(ctx context.Context, ids map[string]bool) error {
 	mu := mounts.CacheMountsLocker()
 	mu.Lock()
 	defer mu.Unlock()
 
-	for _, id := range ids {
-		mds, err := mounts.SearchCacheDir(ctx, w.CacheMgr, id)
+	for id, nested := range ids {
+		mds, err := mounts.SearchCacheDir(ctx, w.CacheMgr, id, nested)
 		if err != nil {
 			return err
 		}

worker/worker.go

@@ -35,7 +35,7 @@ type Worker interface {
 	Exporter(name string, sm *session.Manager) (exporter.Exporter, error)
 	Prune(ctx context.Context, ch chan client.UsageInfo, opt ...client.PruneInfo) error
 	FromRemote(ctx context.Context, remote *solver.Remote) (cache.ImmutableRef, error)
-	PruneCacheMounts(ctx context.Context, ids []string) error
+	PruneCacheMounts(ctx context.Context, ids map[string]bool) error
 	ContentStore() *containerdsnapshot.Store
 	Executor() executor.Executor
 	CacheManager() cache.Manager