From eb3e7110f832bc40e5cd51e327ebb7a3475f1f26 Mon Sep 17 00:00:00 2001 From: Son Luong Ngoc Date: Thu, 27 Apr 2023 10:37:33 +0200 Subject: [PATCH] executor/dirtools: ensure UploadTree set result symlinks (#3800) Co-authored-by: Brandon Duffany --- .../server/remote_execution/dirtools/BUILD | 2 + .../remote_execution/dirtools/dirtools.go | 35 +- .../dirtools/dirtools_test.go | 312 ++++++++++++++++++ .../server/remote_execution/runner/runner.go | 2 +- .../remote_execution/workspace/workspace.go | 4 +- 5 files changed, 350 insertions(+), 5 deletions(-) diff --git a/enterprise/server/remote_execution/dirtools/BUILD b/enterprise/server/remote_execution/dirtools/BUILD index fcc7f07b0c8..387dcb4a724 100644 --- a/enterprise/server/remote_execution/dirtools/BUILD +++ b/enterprise/server/remote_execution/dirtools/BUILD @@ -39,12 +39,14 @@ go_test( "//proto:remote_execution_go_proto", "//proto:resource_go_proto", "//server/remote_cache/byte_stream_server", + "//server/remote_cache/content_addressable_storage_server", "//server/remote_cache/digest", "//server/testutil/testenv", "//server/testutil/testfs", "//server/util/hash", "//server/util/prefix", "@com_github_stretchr_testify//assert", + "@com_github_stretchr_testify//require", "@go_googleapis//google/bytestream:bytestream_go_proto", ], ) diff --git a/enterprise/server/remote_execution/dirtools/dirtools.go b/enterprise/server/remote_execution/dirtools/dirtools.go index f4966e0df29..2cc5a045a9f 100644 --- a/enterprise/server/remote_execution/dirtools/dirtools.go +++ b/enterprise/server/remote_execution/dirtools/dirtools.go @@ -99,6 +99,10 @@ func NewDirHelper(rootDir string, outputDirectories, outputPaths []string, dirPe // of the output_directories, so go ahead and do that. The directories are // also present in output_paths, so they're forgotten after creation here // and then selected for upload through the output_paths. + // + // As of Bazel 6.0.0, the output directories are included as part of the + // InputRoot, so this hack is only needed for older clients. + // See: https://github.com/bazelbuild/bazel/pull/15366 for more info. for _, outputDirectory := range outputDirectories { fullPath := filepath.Join(c.rootDir, outputDirectory) c.dirsToCreate = append(c.dirsToCreate, fullPath) @@ -285,7 +289,7 @@ func uploadFiles(uploader *cachetools.BatchCASUploader, fc interfaces.FileCache, return nil } -func UploadTree(ctx context.Context, env environment.Env, dirHelper *DirHelper, instanceName string, digestFunction repb.DigestFunction_Value, rootDir string, actionResult *repb.ActionResult) (*TransferInfo, error) { +func UploadTree(ctx context.Context, env environment.Env, dirHelper *DirHelper, instanceName string, digestFunction repb.DigestFunction_Value, rootDir string, cmd *repb.Command, actionResult *repb.ActionResult) (*TransferInfo, error) { txInfo := &TransferInfo{} startTime := time.Now() treesToUpload := make([]string, 0) @@ -325,7 +329,7 @@ func UploadTree(ctx context.Context, env environment.Env, dirHelper *DirHelper, return nil, err } fqfn := filepath.Join(fullPath, info.Name()) - if info.Mode().IsDir() { + if info.IsDir() { // Don't recurse on non-uploadable directories. if !dirHelper.ShouldUploadAnythingInDir(fqfn) { continue @@ -357,6 +361,33 @@ func UploadTree(ctx context.Context, env environment.Env, dirHelper *DirHelper, if err != nil { return nil, err } + + // OutputFileSymlinks and OutputDirectorySymlinks are deprecated in REAPI v2.1, + // but Bazel still uses them as of v6.2.0. + // + // TODO(sluongng): Set OutputSymlinks when Bazel has support for it. + // References: + // - https://github.com/bazelbuild/remote-apis/blob/35aee1c4a4250d3df846f7ba3e4a4e66cb014ecd/build/bazel/remote/execution/v2/remote_execution.proto#L1071 + // - https://github.com/bazelbuild/bazel/pull/18198 + // - https://github.com/bazelbuild/bazel/pull/18202 + symlinkPath := trimPathPrefix(fqfn, rootDir) + symlink := &repb.OutputSymlink{ + Path: symlinkPath, + Target: target, + } + for _, expectedFile := range cmd.OutputFiles { + if symlinkPath == expectedFile { + actionResult.OutputFileSymlinks = append(actionResult.OutputFileSymlinks, symlink) + break + } + } + for _, expectedDir := range cmd.OutputDirectories { + if symlinkPath == expectedDir { + actionResult.OutputDirectorySymlinks = append(actionResult.OutputDirectorySymlinks, symlink) + break + } + } + directory.Symlinks = append(directory.Symlinks, &repb.SymlinkNode{ Name: trimPathPrefix(fqfn, rootDir), Target: target, diff --git a/enterprise/server/remote_execution/dirtools/dirtools_test.go b/enterprise/server/remote_execution/dirtools/dirtools_test.go index 942368576dc..b684730d6af 100644 --- a/enterprise/server/remote_execution/dirtools/dirtools_test.go +++ b/enterprise/server/remote_execution/dirtools/dirtools_test.go @@ -2,6 +2,7 @@ package dirtools_test import ( "context" + "io/fs" "os" "path/filepath" "strings" @@ -10,18 +11,322 @@ import ( "github.com/buildbuddy-io/buildbuddy/enterprise/server/remote_execution/dirtools" "github.com/buildbuddy-io/buildbuddy/enterprise/server/remote_execution/filecache" "github.com/buildbuddy-io/buildbuddy/server/remote_cache/byte_stream_server" + "github.com/buildbuddy-io/buildbuddy/server/remote_cache/content_addressable_storage_server" "github.com/buildbuddy-io/buildbuddy/server/remote_cache/digest" "github.com/buildbuddy-io/buildbuddy/server/testutil/testenv" "github.com/buildbuddy-io/buildbuddy/server/testutil/testfs" "github.com/buildbuddy-io/buildbuddy/server/util/hash" "github.com/buildbuddy-io/buildbuddy/server/util/prefix" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" repb "github.com/buildbuddy-io/buildbuddy/proto/remote_execution" rspb "github.com/buildbuddy-io/buildbuddy/proto/resource" bspb "google.golang.org/genproto/googleapis/bytestream" ) +func TestUploadTree(t *testing.T) { + for _, tc := range []struct { + name string + cmd *repb.Command + directoryPaths []string + fileContents map[string]string + symlinkPaths map[string]string + + expectedResult *repb.ActionResult + expectedInfo *dirtools.TransferInfo + }{ + { + name: "NoFiles", + cmd: &repb.Command{}, + directoryPaths: []string{}, + fileContents: map[string]string{}, + symlinkPaths: map[string]string{}, + expectedResult: &repb.ActionResult{}, + expectedInfo: &dirtools.TransferInfo{ + FileCount: 0, + BytesTransferred: 0, + }, + }, + { + name: "SomeFile", + cmd: &repb.Command{ + OutputFiles: []string{"fileA.txt"}, + }, + directoryPaths: []string{}, + fileContents: map[string]string{ + "fileA.txt": "a", + }, + symlinkPaths: map[string]string{}, + expectedResult: &repb.ActionResult{ + OutputFiles: []*repb.OutputFile{ + { + Path: "fileA.txt", + Digest: &repb.Digest{ + SizeBytes: 1, + Hash: hash.String("a"), + }, + }, + }, + }, + expectedInfo: &dirtools.TransferInfo{ + FileCount: 1, + BytesTransferred: 1, + }, + }, + { + name: "OutputDirectory", + cmd: &repb.Command{ + OutputDirectories: []string{"a"}, + }, + directoryPaths: []string{ + "a", + }, + fileContents: map[string]string{ + "a/fileA.txt": "a", + }, + symlinkPaths: map[string]string{}, + expectedResult: &repb.ActionResult{ + OutputDirectories: []*repb.OutputDirectory{ + { + Path: "a", + TreeDigest: &repb.Digest{ + SizeBytes: 85, + Hash: "895545df6841b7efb2e9cc903a4eac7a60c645199be059f6056817ae6feb071d", + }, + }, + }, + OutputFiles: []*repb.OutputFile{ + { + Path: "a/fileA.txt", + Digest: &repb.Digest{ + SizeBytes: 1, + Hash: hash.String("a"), + }, + }, + }, + }, + expectedInfo: &dirtools.TransferInfo{ + FileCount: 2, + BytesTransferred: 84, + }, + }, + { + name: "SymlinkToFile", + cmd: &repb.Command{ + OutputFiles: []string{ + "fileA.txt", + "linkA.txt", + }, + }, + directoryPaths: []string{}, + fileContents: map[string]string{ + "fileA.txt": "a", + }, + symlinkPaths: map[string]string{ + "linkA.txt": "fileA.txt", + }, + expectedResult: &repb.ActionResult{ + OutputFiles: []*repb.OutputFile{ + { + Path: "fileA.txt", + Digest: &repb.Digest{ + SizeBytes: 1, + Hash: "ca978112ca1bbdcafac231b39a23dc4da786eff8147c4e72b9807785afee48bb", + }, + }, + }, + OutputFileSymlinks: []*repb.OutputSymlink{ + { + Path: "linkA.txt", + Target: "fileA.txt", + }, + }, + }, + expectedInfo: &dirtools.TransferInfo{ + FileCount: 1, + BytesTransferred: 1, + }, + }, + { + name: "SymlinkToDirectory", + cmd: &repb.Command{ + OutputDirectories: []string{ + "a", + "linkA", + }, + }, + directoryPaths: []string{ + "a", + }, + fileContents: map[string]string{ + "a/fileA.txt": "a", + }, + symlinkPaths: map[string]string{ + "linkA": "a", + }, + expectedResult: &repb.ActionResult{ + OutputDirectories: []*repb.OutputDirectory{ + { + Path: "a", + TreeDigest: &repb.Digest{ + SizeBytes: 85, + Hash: "895545df6841b7efb2e9cc903a4eac7a60c645199be059f6056817ae6feb071d", + }, + }, + }, + OutputFiles: []*repb.OutputFile{ + { + Path: "a/fileA.txt", + Digest: &repb.Digest{ + SizeBytes: 1, + Hash: hash.String("a"), + }, + }, + }, + OutputDirectorySymlinks: []*repb.OutputSymlink{ + { + Path: "linkA", + Target: "a", + }, + }, + }, + expectedInfo: &dirtools.TransferInfo{ + FileCount: 2, + BytesTransferred: 84, + }, + }, + { + // We don't support dangling symlinks currently, but this could change + // in the future as Bazel started to support some cases of dangling + // symlinks starting from Bazel 6.0.0. + // + // TODO(sluongng): Support dangling symlinks after implementing support for + // Command.output_paths and ActionResult.output_symlinks. + name: "DanglingSymlinkIsIgnored", + cmd: &repb.Command{ + OutputFiles: []string{"a/fileA.txt"}, + OutputDirectories: []string{"a"}, + }, + directoryPaths: []string{"a"}, + fileContents: map[string]string{"a/fileA.txt": "a"}, + symlinkPaths: map[string]string{ + "a/linkB": "b", + }, + expectedResult: &repb.ActionResult{ + OutputDirectories: []*repb.OutputDirectory{ + { + Path: "a", + TreeDigest: &repb.Digest{ + SizeBytes: 99, + Hash: "85fe6d19a6bd4cad6c5a3576929e1c894653c38da15fe44d22897e17dd44f8c6", + }, + }, + }, + OutputFiles: []*repb.OutputFile{ + { + Path: "a/fileA.txt", + Digest: &repb.Digest{ + SizeBytes: 1, + Hash: hash.String("a"), + }, + }, + }, + }, + expectedInfo: &dirtools.TransferInfo{ + FileCount: 2, + BytesTransferred: 98, + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + env, ctx := testEnv(t) + rootDir := testfs.MakeTempDir(t) + + // Prepare inputs + testfs.WriteAllFileContents(t, rootDir, tc.fileContents) + for name, target := range tc.symlinkPaths { + err := os.Symlink(target, filepath.Join(rootDir, name)) + require.NoError(t, err) + } + + var outputPaths []string + for _, path := range tc.directoryPaths { + outputPaths = append(outputPaths, path) + } + for path := range tc.fileContents { + outputPaths = append(outputPaths, path) + } + for path := range tc.symlinkPaths { + outputPaths = append(outputPaths, path) + } + dirHelper := dirtools.NewDirHelper(rootDir, []string{} /*outputDirs*/, outputPaths, fs.FileMode(0o755)) + + actionResult := &repb.ActionResult{} + txInfo, err := dirtools.UploadTree(ctx, env, dirHelper, "", repb.DigestFunction_SHA256, rootDir, tc.cmd, actionResult) + require.NoError(t, err) + + assert.Equal(t, tc.expectedInfo.FileCount, txInfo.FileCount) + assert.Equal(t, tc.expectedInfo.BytesTransferred, txInfo.BytesTransferred) + + for _, file := range tc.expectedResult.OutputFiles { + has, err := env.GetCache().Contains(ctx, &rspb.ResourceName{ + InstanceName: "", + CacheType: rspb.CacheType_CAS, + Digest: file.Digest, + }) + assert.NoError(t, err) + assert.True(t, has) + } + for _, expectedDir := range tc.expectedResult.OutputDirectories { + found := false + for _, dir := range actionResult.OutputDirectories { + if dir.Path == expectedDir.Path { + assert.Equal(t, expectedDir.TreeDigest.SizeBytes, dir.TreeDigest.SizeBytes) + assert.Equal(t, expectedDir.TreeDigest.Hash, dir.TreeDigest.Hash) + found = true + break + } + } + assert.True(t, found) + } + for _, expectedSymlink := range tc.expectedResult.OutputSymlinks { + found := false + for _, symlink := range actionResult.OutputSymlinks { + if symlink.Path == expectedSymlink.Path { + assert.Equal(t, expectedSymlink.Target, symlink.Target) + found = true + break + } + } + assert.True(t, found) + } + for _, expectedSymlink := range tc.expectedResult.OutputFileSymlinks { + found := false + for _, symlink := range actionResult.OutputFileSymlinks { + if symlink.Path == expectedSymlink.Path { + assert.Equal(t, expectedSymlink.Target, symlink.Target) + found = true + break + } + } + assert.True(t, found) + } + for _, expectedSymlink := range tc.expectedResult.OutputDirectorySymlinks { + found := false + for _, symlink := range actionResult.OutputDirectorySymlinks { + if symlink.Path == expectedSymlink.Path { + assert.Equal(t, expectedSymlink.Target, symlink.Target) + found = true + break + } + } + assert.True(t, found) + } + }) + } +} + func TestDownloadTree(t *testing.T) { env, ctx := testEnv(t) tmpDir := testfs.MakeTempDir(t) @@ -215,17 +520,24 @@ func testEnv(t *testing.T) (*testenv.TestEnv, context.Context) { if err != nil { t.Errorf("error attaching user prefix: %v", err) } + casServer, err := content_addressable_storage_server.NewContentAddressableStorageServer(env) + if err != nil { + t.Error(err) + } byteStreamServer, err := byte_stream_server.NewByteStreamServer(env) if err != nil { t.Error(err) } grpcServer, runFunc := env.LocalGRPCServer() + repb.RegisterContentAddressableStorageServer(grpcServer, casServer) bspb.RegisterByteStreamServer(grpcServer, byteStreamServer) go runFunc() + conn, err := env.LocalGRPCConn(ctx) if err != nil { t.Error(err) } + env.SetContentAddressableStorageClient(repb.NewContentAddressableStorageClient(conn)) env.SetByteStreamClient(bspb.NewByteStreamClient(conn)) filecacheRootDir := testfs.MakeTempDir(t) fileCacheMaxSizeBytes := int64(10e9) diff --git a/enterprise/server/remote_execution/runner/runner.go b/enterprise/server/remote_execution/runner/runner.go index 262697a7879..c74a6ddd4b0 100644 --- a/enterprise/server/remote_execution/runner/runner.go +++ b/enterprise/server/remote_execution/runner/runner.go @@ -434,7 +434,7 @@ func (r *commandRunner) Run(ctx context.Context) *interfaces.CommandResult { } func (r *commandRunner) UploadOutputs(ctx context.Context, ioStats *repb.IOStats, actionResult *repb.ActionResult, cmdResult *interfaces.CommandResult) error { - txInfo, err := r.Workspace.UploadOutputs(ctx, actionResult, cmdResult) + txInfo, err := r.Workspace.UploadOutputs(ctx, r.task.Command, actionResult, cmdResult) if err != nil { return err } diff --git a/enterprise/server/remote_execution/workspace/workspace.go b/enterprise/server/remote_execution/workspace/workspace.go index 5d1713c1138..47b699cfc17 100644 --- a/enterprise/server/remote_execution/workspace/workspace.go +++ b/enterprise/server/remote_execution/workspace/workspace.go @@ -232,7 +232,7 @@ func (ws *Workspace) CleanInputsIfNecessary(keep map[string]*repb.FileNode) erro // UploadOutputs uploads any outputs created by the last executed command // as well as the command's stdout and stderr. -func (ws *Workspace) UploadOutputs(ctx context.Context, actionResult *repb.ActionResult, cmdResult *interfaces.CommandResult) (*dirtools.TransferInfo, error) { +func (ws *Workspace) UploadOutputs(ctx context.Context, cmd *repb.Command, actionResult *repb.ActionResult, cmdResult *interfaces.CommandResult) (*dirtools.TransferInfo, error) { ws.mu.Lock() defer ws.mu.Unlock() if ws.removing { @@ -270,7 +270,7 @@ func (ws *Workspace) UploadOutputs(ctx context.Context, actionResult *repb.Actio }) eg.Go(func() error { var err error - txInfo, err = dirtools.UploadTree(egCtx, ws.env, ws.dirHelper, instanceName, digestFunction, ws.Path(), actionResult) + txInfo, err = dirtools.UploadTree(egCtx, ws.env, ws.dirHelper, instanceName, digestFunction, ws.Path(), cmd, actionResult) return err }) if err := eg.Wait(); err != nil {