Skip to content

Commit

Permalink
Rewrite Remote Output Service on top of Google's protocol
Browse files Browse the repository at this point in the history 
Google has merged a change for adding the Remote Output Service protocol
to their code base: bazelbuild/bazel#21140. They
did however make a couple of changes to it. For example:

- The protocol has been made REv2 agnostic. All explicit coupling to
  REv2 has been moved into a helper protocol.

- BatchCreate() has been renamed to StageArtifacts(). It can only be
  used to create files and directories. Not symlinks. It also doesn't
  provide options to clean directories. This is likely going to hurt
  runfiles directory creation, but we'll see whether that is actually a
  problem in practice.

- BatchStat() no longer provides follow_symlinks and
  include_file_digests. Symlinks are no longer followed, and file
  digests should always be included.

- There is a FinalizeArtifacts() function. This function can be used to
  reliably implement file modification tracking. As we don't implement
  that yet, we can simply let it be a stub for the time being.
  • Loading branch information
@EdSchouten
EdSchouten committed Mar 6, 2024
1 parent 9791c09 commit 94b3776
Show file tree
Hide file tree
Showing 20 changed files with 2,906 additions and 2,069 deletions.
2 changes: 1 addition & 1 deletion internal/mock/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -388,11 +388,11 @@ go_library(
"//pkg/filesystem",
"//pkg/filesystem/access",
"//pkg/filesystem/virtual",
"//pkg/proto/bazeloutputservice",
"//pkg/proto/buildqueuestate",
"//pkg/proto/cas",
"//pkg/proto/completedactionlogger",
"//pkg/proto/outputpathpersistency",
"//pkg/proto/remoteoutputservice",
"//pkg/proto/remoteworker",
"//pkg/proto/runner",
"//pkg/scheduler/initialsizeclass",
Expand Down
8 changes: 6 additions & 2 deletions pkg/filesystem/virtual/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,9 @@ go_library(
"//pkg/cas",
"//pkg/filesystem",
"//pkg/filesystem/access",
"//pkg/proto/bazeloutputservice",
"//pkg/proto/bazeloutputservice/rev2",
"//pkg/proto/outputpathpersistency",
"//pkg/proto/remoteoutputservice",
"//pkg/proto/tmp_installer",
"//pkg/sync",
"@com_github_bazelbuild_remote_apis//build/bazel/remote/execution/v2:execution",
Expand All @@ -64,6 +65,7 @@ go_library(
"@org_golang_google_grpc//codes",
"@org_golang_google_grpc//status",
"@org_golang_google_protobuf//encoding/protojson",
"@org_golang_google_protobuf//types/known/anypb",
"@org_golang_google_protobuf//types/known/emptypb",
],
)
Expand All @@ -87,8 +89,9 @@ go_test(
deps = [
":virtual",
"//internal/mock",
"//pkg/proto/bazeloutputservice",
"//pkg/proto/bazeloutputservice/rev2",
"//pkg/proto/outputpathpersistency",
"//pkg/proto/remoteoutputservice",
"//pkg/proto/tmp_installer",
"@com_github_bazelbuild_remote_apis//build/bazel/remote/execution/v2:execution",
"@com_github_buildbarn_bb_storage//pkg/auth",
Expand All @@ -103,6 +106,7 @@ go_test(
"@com_github_stretchr_testify//require",
"@org_golang_google_grpc//codes",
"@org_golang_google_grpc//status",
"@org_golang_google_protobuf//types/known/anypb",
"@org_golang_google_protobuf//types/known/structpb",
],
)
10 changes: 5 additions & 5 deletions pkg/filesystem/virtual/base_symlink_factory.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ import (
"unicode/utf8"

remoteexecution "github.com/bazelbuild/remote-apis/build/bazel/remote/execution/v2"
"github.com/buildbarn/bb-remote-execution/pkg/proto/bazeloutputservice"
"github.com/buildbarn/bb-remote-execution/pkg/proto/outputpathpersistency"
"github.com/buildbarn/bb-remote-execution/pkg/proto/remoteoutputservice"
"github.com/buildbarn/bb-storage/pkg/digest"
"github.com/buildbarn/bb-storage/pkg/filesystem"
"github.com/buildbarn/bb-storage/pkg/filesystem/path"
Expand Down Expand Up @@ -37,14 +37,14 @@ func (f symlink) Readlink() (string, error) {
return string(f.target), nil
}

func (f symlink) GetOutputServiceFileStatus(digestFunction *digest.Function) (*remoteoutputservice.FileStatus, error) {
func (f symlink) GetBazelOutputServiceStat(digestFunction *digest.Function) (*bazeloutputservice.BatchStatResponse_Stat, error) {
target, err := f.Readlink()
if err != nil {
return nil, err
}
return &remoteoutputservice.FileStatus{
FileType: &remoteoutputservice.FileStatus_Symlink_{
Symlink: &remoteoutputservice.FileStatus_Symlink{
return &bazeloutputservice.BatchStatResponse_Stat{
Type: &bazeloutputservice.BatchStatResponse_Stat_Symlink_{
Symlink: &bazeloutputservice.BatchStatResponse_Stat_Symlink{
Target: target,
},
},
Expand Down
33 changes: 21 additions & 12 deletions pkg/filesystem/virtual/blob_access_cas_file_factory.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,18 @@ import (
"syscall"

remoteexecution "github.com/bazelbuild/remote-apis/build/bazel/remote/execution/v2"
"github.com/buildbarn/bb-remote-execution/pkg/proto/bazeloutputservice"
bazeloutputservicerev2 "github.com/buildbarn/bb-remote-execution/pkg/proto/bazeloutputservice/rev2"
"github.com/buildbarn/bb-remote-execution/pkg/proto/outputpathpersistency"
"github.com/buildbarn/bb-remote-execution/pkg/proto/remoteoutputservice"
"github.com/buildbarn/bb-storage/pkg/blobstore"
"github.com/buildbarn/bb-storage/pkg/digest"
"github.com/buildbarn/bb-storage/pkg/filesystem"
"github.com/buildbarn/bb-storage/pkg/filesystem/path"
"github.com/buildbarn/bb-storage/pkg/util"

"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
"google.golang.org/protobuf/types/known/anypb"
)

type blobAccessCASFileFactory struct {
Expand Down Expand Up @@ -83,17 +88,21 @@ func (f *blobAccessCASFile) GetContainingDigests() digest.Set {
return f.digest.ToSingletonSet()
}

func (f *blobAccessCASFile) GetOutputServiceFileStatus(digestFunction *digest.Function) (*remoteoutputservice.FileStatus, error) {
fileStatusFile := remoteoutputservice.FileStatus_File{}
if digestFunction != nil {
// Assume that the file uses the same hash algorithm as
// the provided digest function. Incompatible files are
// removed from storage at the start of the build.
fileStatusFile.Digest = f.digest.GetProto()
func (f *blobAccessCASFile) GetBazelOutputServiceStat(digestFunction *digest.Function) (*bazeloutputservice.BatchStatResponse_Stat, error) {
// Assume that the file uses the same hash algorithm as
// the provided digest function. Incompatible files are
// removed from storage at the start of the build.
locator, err := anypb.New(&bazeloutputservicerev2.FileArtifactLocator{
Digest: f.digest.GetProto(),
})
if err != nil {
return nil, status.Error(codes.Internal, "Failed to marshal locator")
}
return &remoteoutputservice.FileStatus{
FileType: &remoteoutputservice.FileStatus_File_{
File: &fileStatusFile,
return &bazeloutputservice.BatchStatResponse_Stat{
Type: &bazeloutputservice.BatchStatResponse_Stat_File_{
File: &bazeloutputservice.BatchStatResponse_Stat_File{
Locator: locator,
},
},
}, nil
}
Expand Down Expand Up @@ -147,7 +156,7 @@ func (f *blobAccessCASFile) VirtualClose(shareAccess ShareMask) {}
func (f *blobAccessCASFile) virtualSetAttributesCommon(in *Attributes) Status {
// TODO: chmod() calls against CAS backed files should not be
// permitted. Unfortunately, we allowed it in the past. When
// using bb_clientd's Remote Output Service, we see Bazel
// using bb_clientd's Bazel Output Service, we see Bazel
// performing such calls, so we can't forbid it right now.
/*
if _, ok := in.GetPermissions(); ok {
Expand Down
41 changes: 19 additions & 22 deletions pkg/filesystem/virtual/blob_access_cas_file_factory_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,17 @@ import (
remoteexecution "github.com/bazelbuild/remote-apis/build/bazel/remote/execution/v2"
"github.com/buildbarn/bb-remote-execution/internal/mock"
"github.com/buildbarn/bb-remote-execution/pkg/filesystem/virtual"
"github.com/buildbarn/bb-remote-execution/pkg/proto/bazeloutputservice"
bazeloutputservicerev2 "github.com/buildbarn/bb-remote-execution/pkg/proto/bazeloutputservice/rev2"
"github.com/buildbarn/bb-remote-execution/pkg/proto/outputpathpersistency"
"github.com/buildbarn/bb-remote-execution/pkg/proto/remoteoutputservice"
"github.com/buildbarn/bb-storage/pkg/digest"
"github.com/buildbarn/bb-storage/pkg/filesystem"
"github.com/buildbarn/bb-storage/pkg/filesystem/path"
"github.com/buildbarn/bb-storage/pkg/testutil"
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/require"

"google.golang.org/protobuf/types/known/anypb"
)

const blobAccessCASFileFactoryAttributesMask = virtual.AttributesMaskChangeID |
Expand Down Expand Up @@ -98,7 +101,7 @@ func TestBlobAccessCASFileFactoryGetContainingDigests(t *testing.T) {
require.Equal(t, digest.ToSingletonSet(), f.GetContainingDigests())
}

func TestBlobAccessCASFileFactoryGetOutputServiceFileStatus(t *testing.T) {
func TestBlobAccessCASFileFactoryGetBazelOutputServiceStat(t *testing.T) {
ctrl, ctx := gomock.WithContext(context.Background(), t)

contentAddressableStorage := mock.NewMockBlobAccess(ctrl)
Expand All @@ -121,29 +124,23 @@ func TestBlobAccessCASFileFactoryGetOutputServiceFileStatus(t *testing.T) {
SetSizeBytes(123),
&out)

// When the provided digest.Function is nil, we should only
// report that this is a file.
fileStatus, err := f.GetOutputServiceFileStatus(nil)
// We should return the digest of the file as well. There is no
// need to perform any I/O, as the digest is already embedded in
// the file.
digestFunction := digest.GetDigestFunction()
fileStatus, err := f.GetBazelOutputServiceStat(&digestFunction)
require.NoError(t, err)
testutil.RequireEqualProto(t, &remoteoutputservice.FileStatus{
FileType: &remoteoutputservice.FileStatus_File_{
File: &remoteoutputservice.FileStatus_File{},
locator, err := anypb.New(&bazeloutputservicerev2.FileArtifactLocator{
Digest: &remoteexecution.Digest{
Hash: "8b1a9953c4611296a827abf8c47804d7",
SizeBytes: 123,
},
}, fileStatus)

// When the provided digest.Function is set, we should return
// the digest of the file as well. There is no need to perform
// any I/O, as the digest is already embedded in the file.
digestFunction := digest.GetDigestFunction()
fileStatus, err = f.GetOutputServiceFileStatus(&digestFunction)
})
require.NoError(t, err)
testutil.RequireEqualProto(t, &remoteoutputservice.FileStatus{
FileType: &remoteoutputservice.FileStatus_File_{
File: &remoteoutputservice.FileStatus_File{
Digest: &remoteexecution.Digest{
Hash: "8b1a9953c4611296a827abf8c47804d7",
SizeBytes: 123,
},
testutil.RequireEqualProto(t, &bazeloutputservice.BatchStatResponse_Stat{
Type: &bazeloutputservice.BatchStatResponse_Stat_File_{
File: &bazeloutputservice.BatchStatResponse_Stat_File{
Locator: locator,
},
},
}, fileStatus)
Expand Down
19 changes: 8 additions & 11 deletions pkg/filesystem/virtual/native_leaf.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ package virtual
import (
"context"

"github.com/buildbarn/bb-remote-execution/pkg/proto/bazeloutputservice"
"github.com/buildbarn/bb-remote-execution/pkg/proto/outputpathpersistency"
"github.com/buildbarn/bb-remote-execution/pkg/proto/remoteoutputservice"
"github.com/buildbarn/bb-storage/pkg/blobstore"
"github.com/buildbarn/bb-storage/pkg/digest"
"github.com/buildbarn/bb-storage/pkg/filesystem/path"
Expand Down Expand Up @@ -40,16 +40,13 @@ type NativeLeaf interface {
// the file still exists in its entirety, and to prevent that
// the file is removed in the nearby future.
GetContainingDigests() digest.Set
// GetOutputServiceFileStatus() returns the status of the leaf
// node in the form of a FileStatus message that is used by the
// Remote Output Service protocol.
//
// When digestFunction is not nil, a FileStatus responses for
// regular files should include the digest.
GetOutputServiceFileStatus(digestFunction *digest.Function) (*remoteoutputservice.FileStatus, error)
// GetOutputServiceFileStatus() appends a FileNode or
// SymlinkNode entry to a Directory message that is used to
// persist the state of a Remote Output Service output path to
// GetBazelOutputServiceStat() returns the status of the leaf
// node in the form of a Status message that is used by the
// Bazel Output Service protocol.
GetBazelOutputServiceStat(digestFunction *digest.Function) (*bazeloutputservice.BatchStatResponse_Stat, error)
// AppendOutputPathPersistencyDirectoryNode() appends a FileNode
// or SymlinkNode entry to a Directory message that is used to
// persist the state of a Bazel Output Service output path to
// disk.
AppendOutputPathPersistencyDirectoryNode(directory *outputpathpersistency.Directory, name path.Component)
}
67 changes: 37 additions & 30 deletions pkg/filesystem/virtual/pool_backed_file_allocator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@ import (

remoteexecution "github.com/bazelbuild/remote-apis/build/bazel/remote/execution/v2"
re_filesystem "github.com/buildbarn/bb-remote-execution/pkg/filesystem"
"github.com/buildbarn/bb-remote-execution/pkg/proto/bazeloutputservice"
bazeloutputservicerev2 "github.com/buildbarn/bb-remote-execution/pkg/proto/bazeloutputservice/rev2"
"github.com/buildbarn/bb-remote-execution/pkg/proto/outputpathpersistency"
"github.com/buildbarn/bb-remote-execution/pkg/proto/remoteoutputservice"
"github.com/buildbarn/bb-storage/pkg/blobstore"
"github.com/buildbarn/bb-storage/pkg/blobstore/buffer"
"github.com/buildbarn/bb-storage/pkg/digest"
Expand All @@ -22,6 +23,7 @@ import (

"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
"google.golang.org/protobuf/types/known/anypb"
)

var (
Expand Down Expand Up @@ -279,38 +281,43 @@ func (f *fileBackedFile) GetContainingDigests() digest.Set {
return digest.EmptySet
}

func (f *fileBackedFile) GetOutputServiceFileStatus(digestFunction *digest.Function) (*remoteoutputservice.FileStatus, error) {
fileStatus := &remoteoutputservice.FileStatus_File{}
if digestFunction != nil {
f.lock.Lock()
if f.writableDescriptorsCount == 0 {
success := f.acquireFrozenDescriptorLocked()
f.lock.Unlock()
if !success {
return nil, status.Error(codes.NotFound, "File was unlinked before digest computation could start")
}
defer f.releaseFrozenDescriptor()
func (f *fileBackedFile) GetBazelOutputServiceStat(digestFunction *digest.Function) (*bazeloutputservice.BatchStatResponse_Stat, error) {
var locator *anypb.Any
f.lock.Lock()
if f.writableDescriptorsCount == 0 {
success := f.acquireFrozenDescriptorLocked()
f.lock.Unlock()
if !success {
return nil, status.Error(codes.NotFound, "File was unlinked before digest computation could start")
}
defer f.releaseFrozenDescriptor()

blobDigest, err := f.updateCachedDigest(*digestFunction)
if err != nil {
return nil, err
}
fileStatus.Digest = blobDigest.GetProto()
} else {
// Don't report the digest if the file is opened for
// writing. The kernel may still hold on to data that
// needs to be written, meaning that digests computed on
// this end are inaccurate.
//
// By not reporting the digest, the client will
// recompute it itself. This will be consistent with
// what's stored in the kernel's page cache.
f.lock.Unlock()
blobDigest, err := f.updateCachedDigest(*digestFunction)
if err != nil {
return nil, err
}
locator, err = anypb.New(&bazeloutputservicerev2.FileArtifactLocator{
Digest: blobDigest.GetProto(),
})
if err != nil {
return nil, status.Error(codes.Internal, "Failed to marshal locator")
}
} else {
// Don't report the digest if the file is opened for
// writing. The kernel may still hold on to data that
// needs to be written, meaning that digests computed on
// this end are inaccurate.
//
// By not reporting the digest, the client will
// recompute it itself. This will be consistent with
// what's stored in the kernel's page cache.
f.lock.Unlock()
}
return &remoteoutputservice.FileStatus{
FileType: &remoteoutputservice.FileStatus_File_{
File: fileStatus,
return &bazeloutputservice.BatchStatResponse_Stat{
Type: &bazeloutputservice.BatchStatResponse_Stat_File_{
File: &bazeloutputservice.BatchStatResponse_Stat_File{
Locator: locator,
},
},
}, nil
}
Expand Down
Loading

0 comments on commit 94b3776

Please sign in to comment.