Skip to content

Commit

Permalink
Replace GetContainingDigests() with VirtualApply()
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
EdSchouten committed Dec 9, 2024
1 parent 4983b38 commit b7b3bc4
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 47 deletions.
32 changes: 19 additions & 13 deletions pkg/filesystem/virtual/cas_initial_contents_fetcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,23 +127,29 @@ func (icf *casInitialContentsFetcher) FetchContents(fileReadMonitorFactory FileR
return children, nil
}

func (icf *casInitialContentsFetcher) GetContainingDigests(ctx context.Context) (digest.Set, error) {
gatherer := casContainingDigestsGatherer{
context: ctx,
digestFunction: icf.options.digestFunction,
digests: digest.NewSetBuilder(),
directoriesGathered: map[digest.Digest]struct{}{},
}
err := gatherer.traverse(icf.directoryWalker)
if err != nil {
return digest.EmptySet, err
func (icf *casInitialContentsFetcher) VirtualApply(data any) bool {
switch p := data.(type) {
case *ApplyGetContainingDigests:
gatherer := casContainingDigestsGatherer{
context: p.Context,
digestFunction: icf.options.digestFunction,
digests: digest.NewSetBuilder(),
directoriesGathered: map[digest.Digest]struct{}{},
}
if err := gatherer.traverse(icf.directoryWalker); err == nil {
p.ContainingDigests = gatherer.digests.Build()
} else {
p.Err = err
}
default:
return false
}
return gatherer.digests.Build(), nil
return true
}

// casContainingDigestsGatherer is used by casInitialContentsFetcher's
// GetContainingDigests() to compute the transitive closure of digests
// referenced by a hierarchy of Directory objects.
// implementation of ApplyGetContainingDigests to compute the transitive
// closure of digests referenced by a hierarchy of Directory objects.
type casContainingDigestsGatherer struct {
context context.Context
digestFunction digest.Function
Expand Down
30 changes: 21 additions & 9 deletions pkg/filesystem/virtual/cas_initial_contents_fetcher_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -257,8 +257,11 @@ func TestCASInitialContentsFetcherGetContainingDigests(t *testing.T) {
Return(nil, status.Error(codes.Internal, "Server failure"))
directoryWalker.EXPECT().GetDescription().Return("Root directory")

_, err := initialContentsFetcher.GetContainingDigests(ctx)
testutil.RequireEqualStatus(t, status.Error(codes.Internal, "Root directory: Server failure"), err)
p := virtual.ApplyGetContainingDigests{
Context: ctx,
}
require.True(t, initialContentsFetcher.VirtualApply(&p))
testutil.RequireEqualStatus(t, status.Error(codes.Internal, "Root directory: Server failure"), p.Err)
})

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

_, err := initialContentsFetcher.GetContainingDigests(ctx)
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)
p := virtual.ApplyGetContainingDigests{
Context: ctx,
}
require.True(t, initialContentsFetcher.VirtualApply(&p))
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)
})

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

_, err := initialContentsFetcher.GetContainingDigests(ctx)
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)
p := virtual.ApplyGetContainingDigests{
Context: ctx,
}
require.True(t, initialContentsFetcher.VirtualApply(&p))
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)
})

t.Run("Success", func(t *testing.T) {
Expand Down Expand Up @@ -357,8 +366,11 @@ func TestCASInitialContentsFetcherGetContainingDigests(t *testing.T) {
},
}, nil)

digests, err := initialContentsFetcher.GetContainingDigests(ctx)
require.NoError(t, err)
p := virtual.ApplyGetContainingDigests{
Context: ctx,
}
require.True(t, initialContentsFetcher.VirtualApply(&p))
require.NoError(t, p.Err)
require.Equal(
t,
digest.NewSetBuilder().
Expand All @@ -367,6 +379,6 @@ func TestCASInitialContentsFetcherGetContainingDigests(t *testing.T) {
Add(digest.MustNewDigest("hello", remoteexecution.DigestFunction_MD5, "c0607941dd5b3ca8e175a1bfbfd1c0ea", 789)).
Add(digest.MustNewDigest("hello", remoteexecution.DigestFunction_MD5, "19dc69325bd8dfcd75cefbb6144ea3bb", 42)).
Build(),
digests)
p.ContainingDigests)
})
}
7 changes: 2 additions & 5 deletions pkg/filesystem/virtual/empty_initial_contents_fetcher.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
package virtual

import (
"context"

"github.com/buildbarn/bb-storage/pkg/digest"
"github.com/buildbarn/bb-storage/pkg/filesystem/path"
)

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

func (f emptyInitialContentsFetcher) GetContainingDigests(ctx context.Context) (digest.Set, error) {
return digest.EmptySet, nil
func (f emptyInitialContentsFetcher) VirtualApply(data any) bool {
return false
}

// EmptyInitialContentsFetcher is an instance of InitialContentsFetcher
Expand Down
32 changes: 12 additions & 20 deletions pkg/filesystem/virtual/initial_contents_fetcher.go
Original file line number Diff line number Diff line change
@@ -1,16 +1,22 @@
package virtual

import (
"context"

"github.com/buildbarn/bb-storage/pkg/digest"
"github.com/buildbarn/bb-storage/pkg/filesystem/path"
)

// InitialNode contains the common set of operations that can be applied
// against files and directories returned by
// InitialContentsFetcher.FetchContents().
type InitialNode interface {
// VirtualApply can be used to perform implementation defined
// operations against files and directories.
VirtualApply(data any) bool
}

// InitialChild is the value type of the map of directory entries
// returned by InitialContentsFetcher.FetchContents(). Either Directory
// or Leaf is set, but not both.
type InitialChild = Child[InitialContentsFetcher, LinkableLeaf, any]
type InitialChild = Child[InitialContentsFetcher, LinkableLeaf, InitialNode]

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

// GetContainingDigests() returns a set of digests of objects in
// the Content Addressable Storage that back the directories and
// leaf nodes yielded by this InitialContentsFetcher.
//
// The set returned by this function may be passed to
// ContentAddressableStorage.FindMissingBlobs() to check whether
// the all files underneath this directory still exist, and to
// prevent them from being removed in the nearby future.
//
// This API assumes that the resulting set is small enough to
// fit in memory. For hierarchies backed by Tree objects, this
// will generally hold. It may not be safe to call this method
// on InitialContentsFetchers that expand to infinitely big
// hierarchies.
GetContainingDigests(ctx context.Context) (digest.Set, error)
FetchContents(fileReadMonitorFactory FileReadMonitorFactory) (map[path.Component]InitialChild, error)
}
1 change: 1 addition & 0 deletions pkg/filesystem/virtual/linkable_leaf.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package virtual
// PrepopulatedDirectory.
type LinkableLeaf interface {
Leaf
InitialNode

// Operations called into by implementations of
// PrepopulatedDirectory. The Link() operation may fail, for the
Expand Down
4 changes: 4 additions & 0 deletions pkg/filesystem/virtual/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,12 @@ type ApplyUploadFile struct {
// the file still exists in its entirety, and to prevent that
// the file is removed in the nearby future.
type ApplyGetContainingDigests struct {
// Inputs.
Context context.Context

// Outputs.
ContainingDigests digest.Set
Err error
}

// ApplyGetBazelOutputServiceStat is an operation for VirtualApply that
Expand Down

0 comments on commit b7b3bc4

Please sign in to comment.