Skip to content

Commit b7b3bc4

Browse files
committed
Replace GetContainingDigests() with VirtualApply()
Right now we have completely different code paths for computing the containing digests for files and directories. For files we call LinkableLeaf.VirtualApply(), while for directories we call InitialContentsFetcher.GetContainingDigests(). The downside of the current approach is that generic virtual file system APIs are strongly coupled against REv2. By giving InitialContentsFetcher a VirtualApply(), we get rid of the coupling. In addition to that, it's now possible to call InitialChild.GetNode().VirtualApply() to obtain the set of containing digests in a file type oblivious way.
1 parent 4983b38 commit b7b3bc4

File tree

6 files changed

+59
-47
lines changed

6 files changed

+59
-47
lines changed

pkg/filesystem/virtual/cas_initial_contents_fetcher.go

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -127,23 +127,29 @@ func (icf *casInitialContentsFetcher) FetchContents(fileReadMonitorFactory FileR
127127
return children, nil
128128
}
129129

130-
func (icf *casInitialContentsFetcher) GetContainingDigests(ctx context.Context) (digest.Set, error) {
131-
gatherer := casContainingDigestsGatherer{
132-
context: ctx,
133-
digestFunction: icf.options.digestFunction,
134-
digests: digest.NewSetBuilder(),
135-
directoriesGathered: map[digest.Digest]struct{}{},
136-
}
137-
err := gatherer.traverse(icf.directoryWalker)
138-
if err != nil {
139-
return digest.EmptySet, err
130+
func (icf *casInitialContentsFetcher) VirtualApply(data any) bool {
131+
switch p := data.(type) {
132+
case *ApplyGetContainingDigests:
133+
gatherer := casContainingDigestsGatherer{
134+
context: p.Context,
135+
digestFunction: icf.options.digestFunction,
136+
digests: digest.NewSetBuilder(),
137+
directoriesGathered: map[digest.Digest]struct{}{},
138+
}
139+
if err := gatherer.traverse(icf.directoryWalker); err == nil {
140+
p.ContainingDigests = gatherer.digests.Build()
141+
} else {
142+
p.Err = err
143+
}
144+
default:
145+
return false
140146
}
141-
return gatherer.digests.Build(), nil
147+
return true
142148
}
143149

144150
// casContainingDigestsGatherer is used by casInitialContentsFetcher's
145-
// GetContainingDigests() to compute the transitive closure of digests
146-
// referenced by a hierarchy of Directory objects.
151+
// implementation of ApplyGetContainingDigests to compute the transitive
152+
// closure of digests referenced by a hierarchy of Directory objects.
147153
type casContainingDigestsGatherer struct {
148154
context context.Context
149155
digestFunction digest.Function

pkg/filesystem/virtual/cas_initial_contents_fetcher_test.go

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -257,8 +257,11 @@ func TestCASInitialContentsFetcherGetContainingDigests(t *testing.T) {
257257
Return(nil, status.Error(codes.Internal, "Server failure"))
258258
directoryWalker.EXPECT().GetDescription().Return("Root directory")
259259

260-
_, err := initialContentsFetcher.GetContainingDigests(ctx)
261-
testutil.RequireEqualStatus(t, status.Error(codes.Internal, "Root directory: Server failure"), err)
260+
p := virtual.ApplyGetContainingDigests{
261+
Context: ctx,
262+
}
263+
require.True(t, initialContentsFetcher.VirtualApply(&p))
264+
testutil.RequireEqualStatus(t, status.Error(codes.Internal, "Root directory: Server failure"), p.Err)
262265
})
263266

264267
t.Run("ChildDirectoryInvalidDigest", func(t *testing.T) {
@@ -277,8 +280,11 @@ func TestCASInitialContentsFetcherGetContainingDigests(t *testing.T) {
277280
}, nil)
278281
directoryWalker.EXPECT().GetDescription().Return("Root directory")
279282

280-
_, err := initialContentsFetcher.GetContainingDigests(ctx)
281-
testutil.RequireEqualStatus(t, status.Error(codes.InvalidArgument, "Root directory: Failed to obtain digest for directory \"hello\": Hash has length 18, while 32 characters were expected"), err)
283+
p := virtual.ApplyGetContainingDigests{
284+
Context: ctx,
285+
}
286+
require.True(t, initialContentsFetcher.VirtualApply(&p))
287+
testutil.RequireEqualStatus(t, status.Error(codes.InvalidArgument, "Root directory: Failed to obtain digest for directory \"hello\": Hash has length 18, while 32 characters were expected"), p.Err)
282288
})
283289

284290
t.Run("ChildFileInvalidDigest", func(t *testing.T) {
@@ -297,8 +303,11 @@ func TestCASInitialContentsFetcherGetContainingDigests(t *testing.T) {
297303
}, nil)
298304
directoryWalker.EXPECT().GetDescription().Return("Root directory")
299305

300-
_, err := initialContentsFetcher.GetContainingDigests(ctx)
301-
testutil.RequireEqualStatus(t, status.Error(codes.InvalidArgument, "Root directory: Failed to obtain digest for file \"hello\": Hash has length 18, while 32 characters were expected"), err)
306+
p := virtual.ApplyGetContainingDigests{
307+
Context: ctx,
308+
}
309+
require.True(t, initialContentsFetcher.VirtualApply(&p))
310+
testutil.RequireEqualStatus(t, status.Error(codes.InvalidArgument, "Root directory: Failed to obtain digest for file \"hello\": Hash has length 18, while 32 characters were expected"), p.Err)
302311
})
303312

304313
t.Run("Success", func(t *testing.T) {
@@ -357,8 +366,11 @@ func TestCASInitialContentsFetcherGetContainingDigests(t *testing.T) {
357366
},
358367
}, nil)
359368

360-
digests, err := initialContentsFetcher.GetContainingDigests(ctx)
361-
require.NoError(t, err)
369+
p := virtual.ApplyGetContainingDigests{
370+
Context: ctx,
371+
}
372+
require.True(t, initialContentsFetcher.VirtualApply(&p))
373+
require.NoError(t, p.Err)
362374
require.Equal(
363375
t,
364376
digest.NewSetBuilder().
@@ -367,6 +379,6 @@ func TestCASInitialContentsFetcherGetContainingDigests(t *testing.T) {
367379
Add(digest.MustNewDigest("hello", remoteexecution.DigestFunction_MD5, "c0607941dd5b3ca8e175a1bfbfd1c0ea", 789)).
368380
Add(digest.MustNewDigest("hello", remoteexecution.DigestFunction_MD5, "19dc69325bd8dfcd75cefbb6144ea3bb", 42)).
369381
Build(),
370-
digests)
382+
p.ContainingDigests)
371383
})
372384
}

pkg/filesystem/virtual/empty_initial_contents_fetcher.go

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,6 @@
11
package virtual
22

33
import (
4-
"context"
5-
6-
"github.com/buildbarn/bb-storage/pkg/digest"
74
"github.com/buildbarn/bb-storage/pkg/filesystem/path"
85
)
96

@@ -13,8 +10,8 @@ func (f emptyInitialContentsFetcher) FetchContents(fileReadMonitorFactory FileRe
1310
return map[path.Component]InitialChild{}, nil
1411
}
1512

16-
func (f emptyInitialContentsFetcher) GetContainingDigests(ctx context.Context) (digest.Set, error) {
17-
return digest.EmptySet, nil
13+
func (f emptyInitialContentsFetcher) VirtualApply(data any) bool {
14+
return false
1815
}
1916

2017
// EmptyInitialContentsFetcher is an instance of InitialContentsFetcher
Lines changed: 12 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,22 @@
11
package virtual
22

33
import (
4-
"context"
5-
6-
"github.com/buildbarn/bb-storage/pkg/digest"
74
"github.com/buildbarn/bb-storage/pkg/filesystem/path"
85
)
96

7+
// InitialNode contains the common set of operations that can be applied
8+
// against files and directories returned by
9+
// InitialContentsFetcher.FetchContents().
10+
type InitialNode interface {
11+
// VirtualApply can be used to perform implementation defined
12+
// operations against files and directories.
13+
VirtualApply(data any) bool
14+
}
15+
1016
// InitialChild is the value type of the map of directory entries
1117
// returned by InitialContentsFetcher.FetchContents(). Either Directory
1218
// or Leaf is set, but not both.
13-
type InitialChild = Child[InitialContentsFetcher, LinkableLeaf, any]
19+
type InitialChild = Child[InitialContentsFetcher, LinkableLeaf, InitialNode]
1420

1521
// FileReadMonitor is used by the regular files created through the
1622
// InitialContentsFetcher to indicate that one or more calls against
@@ -35,21 +41,7 @@ type FileReadMonitorFactory func(name path.Component) FileReadMonitor
3541
// may be possible FetchContents() is never called. This may happen if
3642
// the directory in question is never accessed.
3743
type InitialContentsFetcher interface {
38-
FetchContents(fileReadMonitorFactory FileReadMonitorFactory) (map[path.Component]InitialChild, error)
44+
InitialNode
3945

40-
// GetContainingDigests() returns a set of digests of objects in
41-
// the Content Addressable Storage that back the directories and
42-
// leaf nodes yielded by this InitialContentsFetcher.
43-
//
44-
// The set returned by this function may be passed to
45-
// ContentAddressableStorage.FindMissingBlobs() to check whether
46-
// the all files underneath this directory still exist, and to
47-
// prevent them from being removed in the nearby future.
48-
//
49-
// This API assumes that the resulting set is small enough to
50-
// fit in memory. For hierarchies backed by Tree objects, this
51-
// will generally hold. It may not be safe to call this method
52-
// on InitialContentsFetchers that expand to infinitely big
53-
// hierarchies.
54-
GetContainingDigests(ctx context.Context) (digest.Set, error)
46+
FetchContents(fileReadMonitorFactory FileReadMonitorFactory) (map[path.Component]InitialChild, error)
5547
}

pkg/filesystem/virtual/linkable_leaf.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ package virtual
44
// PrepopulatedDirectory.
55
type LinkableLeaf interface {
66
Leaf
7+
InitialNode
78

89
// Operations called into by implementations of
910
// PrepopulatedDirectory. The Link() operation may fail, for the

pkg/filesystem/virtual/node.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,8 +68,12 @@ type ApplyUploadFile struct {
6868
// the file still exists in its entirety, and to prevent that
6969
// the file is removed in the nearby future.
7070
type ApplyGetContainingDigests struct {
71+
// Inputs.
72+
Context context.Context
73+
7174
// Outputs.
7275
ContainingDigests digest.Set
76+
Err error
7377
}
7478

7579
// ApplyGetBazelOutputServiceStat is an operation for VirtualApply that

0 commit comments

Comments
 (0)