From 610affa5fd10420d09e9bdc231b5531afc35e04a Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Thu, 5 Sep 2024 17:46:03 -0700 Subject: [PATCH] exec: fix pruning cache mounts with parent ref on no-cache On a build with no-cache, cache mounts were not pruned correctly if the mount was on top of another ref. This also appeared in Dockerfile when mode/uid/gid was set because implicit parent ref is created in these cases in order to change the permissions of a subdir that is used as a cache mount base. Because it is not possible to know ahead of time what ref will become the parent of cache mount during build, all cache mounts matching the ID that have a parent will be pruned. Signed-off-by: Tonis Tiigi --- cache/metadata.go | 19 +++-- cache/metadata/metadata.go | 12 +++- cache/metadata/metadata_test.go | 6 +- .../dockerfile2llb/convert_runmount.go | 2 +- frontend/dockerfile/dockerfile_test.go | 69 +++++++++++++++++++ solver/llbsolver/bridge.go | 6 +- solver/llbsolver/mounts/mount.go | 18 ++++- solver/llbsolver/vertex.go | 7 +- source/git/source.go | 2 +- source/http/source.go | 2 +- source/local/source.go | 2 +- worker/base/worker.go | 6 +- worker/worker.go | 2 +- 13 files changed, 121 insertions(+), 32 deletions(-) diff --git a/cache/metadata.go b/cache/metadata.go index b223024dcae3..8de8efd9bcef 100644 --- a/cache/metadata.go +++ b/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 { diff --git a/cache/metadata/metadata.go b/cache/metadata/metadata.go index 72d66800b353..47a08b69d4a0 100644 --- a/cache/metadata/metadata.go +++ b/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 { diff --git a/cache/metadata/metadata_test.go b/cache/metadata/metadata_test.go index 1038b578225c..c44cda1bc076 100644 --- a/cache/metadata/metadata_test.go +++ b/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)) diff --git a/frontend/dockerfile/dockerfile2llb/convert_runmount.go b/frontend/dockerfile/dockerfile2llb/convert_runmount.go index c222b522f759..cc9d858641d2 100644 --- a/frontend/dockerfile/dockerfile2llb/convert_runmount.go +++ b/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) { diff --git a/frontend/dockerfile/dockerfile_test.go b/frontend/dockerfile/dockerfile_test.go index e12c0dd88286..42cf2a1bd08c 100644 --- a/frontend/dockerfile/dockerfile_test.go +++ b/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) diff --git a/solver/llbsolver/bridge.go b/solver/llbsolver/bridge.go index 9c4382606354..fa00b65e127d 100644 --- a/solver/llbsolver/bridge.go +++ b/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 } diff --git a/solver/llbsolver/mounts/mount.go b/solver/llbsolver/mounts/mount.go index 85bc4ffb8632..509c7e44add1 100644 --- a/solver/llbsolver/mounts/mount.go +++ b/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 diff --git a/solver/llbsolver/vertex.go b/solver/llbsolver/vertex.go index fdefa06e8447..b828e7336993 100644 --- a/solver/llbsolver/vertex.go +++ b/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 } } } diff --git a/source/git/source.go b/source/git/source.go index 1b757500d7a3..be14d3fb55f7 100644 --- a/source/git/source.go +++ b/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 } diff --git a/source/http/source.go b/source/http/source.go index 5369eac3a496..d6c6050199fb 100644 --- a/source/http/source.go +++ b/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 } diff --git a/source/local/source.go b/source/local/source.go index db61b7a4735d..7c17117886ce 100644 --- a/source/local/source.go +++ b/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 } diff --git a/worker/base/worker.go b/worker/base/worker.go index 8121c74c6530..d57cf457618f 100644 --- a/worker/base/worker.go +++ b/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 } diff --git a/worker/worker.go b/worker/worker.go index dd336917a4c9..e5d6ef564c5d 100644 --- a/worker/worker.go +++ b/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