Skip to content

Commit

Permalink
Make output file handling conformant to REv2.0 and REv2.1
Browse files Browse the repository at this point in the history 
- We shouldn't cache directory file descriptors during execution, as
  this leads to us uploading empty output directories in case the build
  action deletes and recreates output directories.
- We should add support for REv2.1 output_paths.

Instead of trying to add all of this to LocalBuildExecutor, let's add a
helper type called OutputHierarchy that does most of the heavy lifting
for us. This makes things a lot easier to test.

Fixes: #46, #48, #49
  • Loading branch information
@EdSchouten
EdSchouten committed Jun 26, 2020
1 parent 3d34b3d commit 55f4220
Show file tree
Hide file tree
Showing 5 changed files with 1,090 additions and 437 deletions.
2 changes: 2 additions & 0 deletions pkg/builder/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ go_library(
"logging_build_executor.go",
"metrics_build_executor.go",
"naive_build_directory.go",
"output_hierarchy.go",
"root_build_directory_creator.go",
"shared_build_directory_creator.go",
"storage_flushing_build_executor.go",
Expand Down Expand Up @@ -65,6 +66,7 @@ go_test(
"in_memory_build_queue_test.go",
"local_build_executor_test.go",
"naive_build_directory_test.go",
"output_hierarchy_test.go",
"root_build_directory_creator_test.go",
"shared_build_directory_creator_test.go",
"storage_flushing_build_executor_test.go",
Expand Down
220 changes: 17 additions & 203 deletions pkg/builder/local_build_executor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"os"
"path"
"strings"
"time"

remoteexecution "github.com/bazelbuild/remote-apis/build/bazel/remote/execution/v2"
Expand All @@ -15,9 +14,7 @@ import (
"github.com/buildbarn/bb-storage/pkg/cas"
"github.com/buildbarn/bb-storage/pkg/clock"
"github.com/buildbarn/bb-storage/pkg/digest"
"github.com/buildbarn/bb-storage/pkg/filesystem"
"github.com/buildbarn/bb-storage/pkg/util"
"github.com/golang/protobuf/proto"
"github.com/golang/protobuf/ptypes"
"github.com/golang/protobuf/ptypes/empty"

Expand Down Expand Up @@ -49,113 +46,6 @@ func NewLocalBuildExecutor(contentAddressableStorage cas.ContentAddressableStora
}
}

func (be *localBuildExecutor) uploadDirectory(ctx context.Context, outputDirectory filesystem.Directory, parentDigest digest.Digest, children map[string]*remoteexecution.Directory, components []string) (*remoteexecution.Directory, error) {
files, err := outputDirectory.ReadDir()
if err != nil {
return nil, util.StatusWrapf(err, "Failed to read output directory %#v", path.Join(components...))
}

var directory remoteexecution.Directory
for _, file := range files {
name := file.Name()
childComponents := append(components, name)
// The elided default case of the below switch statement would
// represent a UNIX socket, FIFO or device node. These files cannot
// be represented in a Directory message. They are simply skipped.
// Returning an error would make the overall user experience worse.
switch fileType := file.Type(); fileType {
case filesystem.FileTypeRegularFile, filesystem.FileTypeExecutableFile:
childDigest, err := be.contentAddressableStorage.PutFile(ctx, outputDirectory, name, parentDigest)
if err != nil {
return nil, util.StatusWrapf(err, "Failed to store output file %#v", path.Join(childComponents...))
}
directory.Files = append(directory.Files, &remoteexecution.FileNode{
Name: name,
Digest: childDigest.GetPartialDigest(),
IsExecutable: fileType == filesystem.FileTypeExecutableFile,
})
case filesystem.FileTypeDirectory:
childDirectory, err := outputDirectory.EnterDirectory(name)
if err != nil {
return nil, util.StatusWrapf(err, "Failed to enter output directory %#v", path.Join(childComponents...))
}
child, err := be.uploadDirectory(ctx, childDirectory, parentDigest, children, childComponents)
childDirectory.Close()
if err != nil {
return nil, err
}

// Compute digest of the child directory. This requires serializing it.
data, err := proto.Marshal(child)
if err != nil {
return nil, util.StatusWrapf(err, "Failed to marshal output directory %#v", path.Join(childComponents...))
}
digestGenerator := parentDigest.NewGenerator()
if _, err := digestGenerator.Write(data); err != nil {
return nil, util.StatusWrapf(err, "Failed to compute digest of output directory %#v", path.Join(childComponents...))
}
childDigest := digestGenerator.Sum()

children[childDigest.GetKey(digest.KeyWithoutInstance)] = child
directory.Directories = append(directory.Directories, &remoteexecution.DirectoryNode{
Name: name,
Digest: childDigest.GetPartialDigest(),
})
case filesystem.FileTypeSymlink:
target, err := outputDirectory.Readlink(name)
if err != nil {
return nil, util.StatusWrapf(err, "Failed to read output symlink %#v", path.Join(childComponents...))
}
directory.Symlinks = append(directory.Symlinks, &remoteexecution.SymlinkNode{
Name: name,
Target: target,
})
}
}
return &directory, nil
}

func (be *localBuildExecutor) uploadTree(ctx context.Context, outputDirectory filesystem.Directory, parentDigest digest.Digest, components []string) (digest.Digest, error) {
// Gather all individual directory objects and turn them into a tree.
children := map[string]*remoteexecution.Directory{}
root, err := be.uploadDirectory(ctx, outputDirectory, parentDigest, children, components)
if err != nil {
return digest.BadDigest, err
}
tree := &remoteexecution.Tree{
Root: root,
}
for _, child := range children {
tree.Children = append(tree.Children, child)
}
treeDigest, err := be.contentAddressableStorage.PutTree(ctx, tree, parentDigest)
if err != nil {
return digest.BadDigest, util.StatusWrapf(err, "Failed to store output directory %#v", path.Join(components...))
}
return treeDigest, err
}

func (be *localBuildExecutor) createOutputParentDirectory(inputRootDirectory filesystem.Directory, outputParentPath string) (filesystem.DirectoryCloser, error) {
// Create and enter successive components, closing the former.
components := strings.FieldsFunc(outputParentPath, func(r rune) bool { return r == '/' })
d := filesystem.NopDirectoryCloser(inputRootDirectory)
for n, component := range components {
if component != "." {
if err := d.Mkdir(component, 0777); err != nil && !os.IsExist(err) {
d.Close()
return nil, util.StatusWrapf(err, "Failed to create output directory %#v", path.Join(components[:n+1]...))
}
d2, err := d.EnterDirectory(component)
d.Close()
if err != nil {
return nil, util.StatusWrapf(err, "Failed to enter output directory %#v", path.Join(components[:n+1]...))
}
d = d2
}
}
return d, nil
}

func (be *localBuildExecutor) createCharacterDevices(inputRootDirectory BuildDirectory) error {
if err := inputRootDirectory.Mkdir("dev", 0777); err != nil && !os.IsExist(err) {
return util.StatusWrap(err, "Unable to create /dev directory in input root")
Expand Down Expand Up @@ -276,47 +166,21 @@ func (be *localBuildExecutor) Execute(ctx context.Context, filePool re_filesyste
}
}

// Create and open parent directories of where we expect to see output.
// Build rules generally expect the parent directories to already be
// there. We later use the directory handles to extract output files.
outputParentDirectories := map[string]filesystem.Directory{}
// Create parent directories of output files and directories.
// These are not declared in the input root explicitly.
outputHierarchy := NewOutputHierarchy(command.WorkingDirectory)
for _, outputDirectory := range command.OutputDirectories {
// Although REv2 explicitly document that only parents
// of output directories are created (i.e., not the
// output directory itself), Bazel recently changed its
// behaviour to do so after all for local execution. See
// these issues for details:
//
// https://github.com/bazelbuild/bazel/issues/6262
// https://github.com/bazelbuild/bazel/issues/6393
//
// For now, be consistent with Bazel. What the intended
// protocol behaviour is should be clarified at some
// point. What is especially confusing is that creating
// these directories up front somewhat rules out the
// possibility of omitting output directories and using
// OutputDirectorySymlinks.
if _, ok := outputParentDirectories[outputDirectory]; !ok {
dir, err := be.createOutputParentDirectory(inputRootDirectory, path.Join(command.WorkingDirectory, outputDirectory))
if err != nil {
attachErrorToExecuteResponse(response, err)
return response
}
outputParentDirectories[outputDirectory] = dir
defer dir.Close()
}
outputHierarchy.AddDirectory(outputDirectory)
}
for _, outputFile := range command.OutputFiles {
dirPath := path.Dir(outputFile)
if _, ok := outputParentDirectories[dirPath]; !ok {
dir, err := be.createOutputParentDirectory(inputRootDirectory, path.Join(command.WorkingDirectory, dirPath))
if err != nil {
attachErrorToExecuteResponse(response, err)
return response
}
outputParentDirectories[dirPath] = dir
defer dir.Close()
}
outputHierarchy.AddFile(outputFile)
}
for _, outputPath := range command.OutputPaths {
outputHierarchy.AddPath(outputPath)
}
if err := outputHierarchy.CreateParentDirectories(inputRootDirectory); err != nil {
attachErrorToExecuteResponse(response, err)
return response
}

// Create a directory inside the build directory that build
Expand Down Expand Up @@ -370,9 +234,9 @@ func (be *localBuildExecutor) Execute(ctx context.Context, filePool re_filesyste
},
}

// Upload command output. In the common case, the files are
// empty. If that's the case, don't bother setting the digest to
// keep the ActionResult small.
// Upload command output. In the common case, the stdout and
// stderr files are empty. If that's the case, don't bother
// setting the digest to keep the ActionResult small.
if stdoutDigest, err := be.contentAddressableStorage.PutFile(ctx, buildDirectory, "stdout", actionDigest); err != nil {
attachErrorToExecuteResponse(response, util.StatusWrap(err, "Failed to store stdout"))
} else if stdoutDigest.GetSizeBytes() > 0 {
Expand All @@ -383,58 +247,8 @@ func (be *localBuildExecutor) Execute(ctx context.Context, filePool re_filesyste
} else if stderrDigest.GetSizeBytes() > 0 {
response.Result.StderrDigest = stderrDigest.GetPartialDigest()
}

// Upload output files.
for _, outputFile := range command.OutputFiles {
outputParentDirectory := outputParentDirectories[path.Dir(outputFile)]
outputBaseName := path.Base(outputFile)
if fileInfo, err := outputParentDirectory.Lstat(outputBaseName); err == nil {
switch fileType := fileInfo.Type(); fileType {
case filesystem.FileTypeRegularFile, filesystem.FileTypeExecutableFile:
if digest, err := be.contentAddressableStorage.PutFile(ctx, outputParentDirectory, outputBaseName, actionDigest); err == nil {
response.Result.OutputFiles = append(response.Result.OutputFiles, &remoteexecution.OutputFile{
Path: outputFile,
Digest: digest.GetPartialDigest(),
IsExecutable: fileType == filesystem.FileTypeExecutableFile,
})
} else {
attachErrorToExecuteResponse(
response,
util.StatusWrapf(err, "Failed to store output file %#v", outputFile))
}
case filesystem.FileTypeSymlink:
if target, err := outputParentDirectory.Readlink(outputBaseName); err == nil {
response.Result.OutputFileSymlinks = append(response.Result.OutputFileSymlinks, &remoteexecution.OutputSymlink{
Path: outputFile,
Target: target,
})
} else {
attachErrorToExecuteResponse(
response,
util.StatusWrapf(err, "Failed to read output symlink %#v", outputFile))
}
default:
attachErrorToExecuteResponse(
response,
status.Errorf(codes.Internal, "Output file %#v is not a regular file or symlink", outputFile))
}
} else if !os.IsNotExist(err) {
attachErrorToExecuteResponse(
response,
util.StatusWrapf(err, "Failed to read attributes of output file %#v", outputFile))
}
}

// Upload output directories.
for _, outputDirectory := range command.OutputDirectories {
if digest, err := be.uploadTree(ctx, outputParentDirectories[outputDirectory], actionDigest, []string{outputDirectory}); err == nil {
response.Result.OutputDirectories = append(response.Result.OutputDirectories, &remoteexecution.OutputDirectory{
Path: outputDirectory,
TreeDigest: digest.GetPartialDigest(),
})
} else {
attachErrorToExecuteResponse(response, err)
}
if err := outputHierarchy.UploadOutputs(ctx, inputRootDirectory, be.contentAddressableStorage, actionDigest, response.Result); err != nil {
attachErrorToExecuteResponse(response, err)
}

return response
Expand Down
Loading

0 comments on commit 55f4220

Please sign in to comment.