From 089a79aab9c6938ba3e73c1e17ecb01760510fc5 Mon Sep 17 00:00:00 2001 From: Maggie Lou Date: Mon, 28 Oct 2024 13:47:37 -0500 Subject: [PATCH] [RB] Fix fetching remote build outputs (#7770) Remote bazel can be used to build a target on a remote runner, and then the CLI will fetch the build outputs to be used locally. A couple things were broken: * The CLI uses certain build events to identify and fetch build outputs, but [this PR](https://github.com/buildbuddy-io/buildbuddy/pull/4490) moved target-level events from the `Event` field to `TargetGroups`. This PR updates the CLI to use the new field * Verifies that the build uses `--remote_upload_local_results` so that remote build outputs can be fetched from the cache * Command parsing failed if there were startup commands * The error code returned when we build remotely and run locally --- cli/remotebazel/BUILD | 1 + cli/remotebazel/remotebazel.go | 55 ++++--- .../remote_bazel/remote_bazel_test.go | 150 +++++++++++++----- 3 files changed, 136 insertions(+), 70 deletions(-) diff --git a/cli/remotebazel/BUILD b/cli/remotebazel/BUILD index c5532ef2727..e9a201d2228 100644 --- a/cli/remotebazel/BUILD +++ b/cli/remotebazel/BUILD @@ -25,6 +25,7 @@ go_library( "//proto:invocation_go_proto", "//proto:remote_execution_go_proto", "//proto:runner_go_proto", + "//proto/api/v1:common_go_proto", "//server/cache/dirtools", "//server/environment", "//server/real_environment", diff --git a/cli/remotebazel/remotebazel.go b/cli/remotebazel/remotebazel.go index 84b0fbba83e..39c878ac8e3 100644 --- a/cli/remotebazel/remotebazel.go +++ b/cli/remotebazel/remotebazel.go @@ -42,6 +42,7 @@ import ( "golang.org/x/sys/unix" "google.golang.org/grpc/metadata" + cmnpb "github.com/buildbuddy-io/buildbuddy/proto/api/v1/common" bespb "github.com/buildbuddy-io/buildbuddy/proto/build_event_stream" bbspb "github.com/buildbuddy-io/buildbuddy/proto/buildbuddy_service" elpb "github.com/buildbuddy-io/buildbuddy/proto/eventlog" @@ -55,7 +56,7 @@ import ( ) const ( - buildBuddyArtifactDir = "bb-out" + BuildBuddyArtifactDir = "bb-out" escapeSeq = "\u001B[" gitConfigSection = "buildbuddy" @@ -437,7 +438,7 @@ func generatePatches(baseCommit string) ([][]byte, error) { untrackedFiles = strings.Trim(untrackedFiles, "\n") if untrackedFiles != "" { for _, uf := range strings.Split(untrackedFiles, "\n") { - if strings.HasPrefix(uf, buildBuddyArtifactDir+"/") { + if strings.HasPrefix(uf, BuildBuddyArtifactDir+"/") { continue } patch, err := diffUntrackedFile(uf) @@ -588,28 +589,21 @@ func lookupBazelInvocationOutputs(ctx context.Context, bbClient bbspb.BuildBuddy return nil, fmt.Errorf("could not retrieve invocation %q: %s", invocationID, err) } - fileSets := make(map[string][]*bespb.File) - outputFileSetNames := make(map[string]struct{}) - for _, e := range childInRsp.GetInvocation()[0].GetEvent() { - switch t := e.GetBuildEvent().GetPayload().(type) { - case *bespb.BuildEvent_NamedSetOfFiles: - fileSets[e.GetBuildEvent().GetId().GetNamedSet().GetId()] = t.NamedSetOfFiles.GetFiles() - case *bespb.BuildEvent_Completed: - for _, og := range t.Completed.GetOutputGroup() { - for _, fs := range og.GetFileSets() { - outputFileSetNames[fs.GetId()] = struct{}{} - } - } - } + if len(childInRsp.GetInvocation()) < 1 { + return nil, fmt.Errorf("invocation %s not found", invocationID) } + inv := childInRsp.GetInvocation()[0] var outputs []*bespb.File - for fsID := range outputFileSetNames { - fs, ok := fileSets[fsID] - if !ok { - return nil, fmt.Errorf("could not find file set with ID %q while fetching outputs", fsID) + for _, g := range inv.TargetGroups { + // The `GetTarget` API only fetches file data for the general + // STATUS_UNSPECIFIED status. For other statuses, it only returns metadata. + if g.Status != cmnpb.Status_STATUS_UNSPECIFIED { + continue + } + for _, t := range g.Targets { + outputs = append(outputs, t.Files...) } - outputs = append(outputs, fs...) } return outputs, nil @@ -639,7 +633,7 @@ func downloadOutputs(ctx context.Context, env environment.Env, mainOutputs []*be if err != nil { return "", nil } - outFile := filepath.Join(outputBaseDir, buildBuddyArtifactDir) + outFile := filepath.Join(outputBaseDir, BuildBuddyArtifactDir) for _, p := range f.GetPathPrefix() { outFile = filepath.Join(outFile, p) } @@ -671,7 +665,7 @@ func downloadOutputs(ctx context.Context, env environment.Env, mainOutputs []*be if err := cachetools.GetBlobAsProto(ctx, bsClient, rn, tree); err != nil { return nil, err } - outDir := filepath.Join(outputBaseDir, buildBuddyArtifactDir, d.GetName()) + outDir := filepath.Join(outputBaseDir, BuildBuddyArtifactDir, d.GetName()) if err := os.MkdirAll(outDir, 0755); err != nil { return nil, err } @@ -725,9 +719,10 @@ func Run(ctx context.Context, opts RunOpts, repoConfig *RepoConfig) (int, error) } fetchOutputs := false runOutput := false - if len(bazelArgs) > 0 && (bazelArgs[0] == "build" || (bazelArgs[0] == "run" && !*runRemotely)) { + bazelCmd, _ := parser.GetBazelCommandAndIndex(bazelArgs) + if bazelCmd == "build" || (bazelCmd == "run" && !*runRemotely) { fetchOutputs = true - if bazelArgs[0] == "run" { + if bazelCmd == "run" { runOutput = true } } @@ -940,14 +935,16 @@ func Run(ctx context.Context, opts RunOpts, repoConfig *RepoConfig) (int, error) execArgs = append(execArgs, arg.GetExecutableArgs(opts.Args)...) log.Debugf("Executing %q with arguments %s", binPath, execArgs) cmd := exec.CommandContext(ctx, binPath, execArgs...) - cmd.Dir = filepath.Join(outputsBaseDir, buildBuddyArtifactDir, runfilesRoot) + cmd.Dir = filepath.Join(outputsBaseDir, BuildBuddyArtifactDir, runfilesRoot) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr err = cmd.Run() if e, ok := err.(*exec.ExitError); ok { return e.ExitCode(), nil + } else if err != nil { + return 1, err } - return 1, err + return 0, nil } } else { log.Warnf("Cannot download outputs - no child invocations found") @@ -1046,6 +1043,12 @@ func parseArgs(commandLineArgs []string) (bazelArgs []string, execArgs []string, bazelArgs = append(bazelArgs, "--config=buildbuddy_bes_results_url") bazelArgs = append(bazelArgs, "--config=buildbuddy_remote_cache") + // If the CLI needs to fetch build outputs, make sure the remote runner uploads them. + bazelCmd, _ := parser.GetBazelCommandAndIndex(bazelArgs) + if (!*runRemotely && bazelCmd == "run") || bazelCmd == "build" { + bazelArgs = append(bazelArgs, "--remote_upload_local_results") + } + return bazelArgs, execArgs, nil } diff --git a/enterprise/server/test/integration/remote_bazel/remote_bazel_test.go b/enterprise/server/test/integration/remote_bazel/remote_bazel_test.go index 0ba732848d8..dfec84eef9c 100644 --- a/enterprise/server/test/integration/remote_bazel/remote_bazel_test.go +++ b/enterprise/server/test/integration/remote_bazel/remote_bazel_test.go @@ -1,10 +1,13 @@ package remote_bazel_test import ( + "bytes" "context" "fmt" "math" "os" + "os/exec" + "path/filepath" "strings" "testing" "time" @@ -87,6 +90,33 @@ func waitForInvocationStatus(t *testing.T, ctx context.Context, bb bbspb.BuildBu require.FailNowf(t, "timeout", "Timed out waiting for invocation to reach expected status %v", expectedStatus) } +func clonePrivateTestRepo(t *testing.T) { + repoName := "private-test-repo" + // If you need to re-generate this PAT, it should only have read access to + // `private-test-repo`, and should be saved as a BB secret in all environments. + username := "maggie-lou" + personalAccessToken := os.Getenv("PRIVATE_TEST_REPO_GIT_ACCESS_TOKEN") + repoURLWithToken := fmt.Sprintf("https://%s:%s@github.com/buildbuddy-io/private-test-repo.git", username, personalAccessToken) + + // Use a dir that is persisted on recycled runners + rootDir := "/root/workspace/remote-bazel-integration-test" + err := os.Setenv("HOME", rootDir) + require.NoError(t, err) + + err = os.MkdirAll(rootDir, 0755) + require.NoError(t, err) + + if _, err := os.Stat(fmt.Sprintf("%s/%s", rootDir, repoName)); os.IsNotExist(err) { + output := testshell.Run(t, rootDir, fmt.Sprintf("git clone %s --filter=blob:none --depth=1", repoURLWithToken)) + require.NotContains(t, output, "fatal") + } + + repoDir := fmt.Sprintf("%s/%s", rootDir, repoName) + err = os.Chdir(repoDir) + require.NoError(t, err) + testshell.Run(t, repoDir, "git pull") +} + func TestWithPublicRepo(t *testing.T) { // Use a dir that is persisted on recycled runners rootDir := "/root/workspace/remote-bazel-integration-test" @@ -154,28 +184,9 @@ func TestWithPublicRepo(t *testing.T) { } func TestWithPrivateRepo(t *testing.T) { - repoName := "private-test-repo" - // If you need to re-generate this PAT, it should only have read access to - // `private-test-repo`, and should be saved as a BB secret in all environments. - username := "maggie-lou" - personalAccessToken := os.Getenv("PRIVATE_TEST_REPO_GIT_ACCESS_TOKEN") - repoURLWithToken := fmt.Sprintf("https://%s:%s@github.com/buildbuddy-io/private-test-repo.git", username, personalAccessToken) + clonePrivateTestRepo(t) - // Use a dir that is persisted on recycled runners - rootDir := "/root/workspace/remote-bazel-integration-test" - err := os.Setenv("HOME", rootDir) - require.NoError(t, err) - - err = os.MkdirAll(rootDir, 0755) - require.NoError(t, err) - - if _, err := os.Stat(fmt.Sprintf("%s/%s", rootDir, repoName)); os.IsNotExist(err) { - output := testshell.Run(t, rootDir, fmt.Sprintf("git clone %s --filter=blob:none --depth=1", repoURLWithToken)) - require.NotContains(t, output, "fatal") - } - - err = os.Chdir(fmt.Sprintf("%s/%s", rootDir, repoName)) - require.NoError(t, err) + personalAccessToken := os.Getenv("PRIVATE_TEST_REPO_GIT_ACCESS_TOKEN") // Run a server and executor locally to run remote bazel against env, bbServer, _ := runLocalServerAndExecutor(t, personalAccessToken) @@ -183,7 +194,7 @@ func TestWithPrivateRepo(t *testing.T) { // Create a workflow for the same repo - will be used to fetch the git token dbh := env.GetDBHandle() require.NotNil(t, dbh) - err = dbh.NewQuery(context.Background(), "create_git_repo_for_test").Create(&tables.GitRepository{ + err := dbh.NewQuery(context.Background(), "create_git_repo_for_test").Create(&tables.GitRepository{ RepoURL: "https://github.com/buildbuddy-io/private-test-repo", GroupID: env.GroupID1, }) @@ -265,28 +276,9 @@ func runLocalServerAndExecutor(t *testing.T, githubToken string) (*rbetest.Env, } func TestCancel(t *testing.T) { - repoName := "private-test-repo" - // If you need to re-generate this PAT, it should only have read access to - // `private-test-repo`, and should be saved as a BB secret in all environments. - username := "maggie-lou" - personalAccessToken := os.Getenv("PRIVATE_TEST_REPO_GIT_ACCESS_TOKEN") - repoURLWithToken := fmt.Sprintf("https://%s:%s@github.com/buildbuddy-io/private-test-repo.git", username, personalAccessToken) - - // Use a dir that is persisted on recycled runners - rootDir := "/root/workspace/remote-bazel-integration-test" - err := os.Setenv("HOME", rootDir) - require.NoError(t, err) - - err = os.MkdirAll(rootDir, 0755) - require.NoError(t, err) + clonePrivateTestRepo(t) - if _, err := os.Stat(fmt.Sprintf("%s/%s", rootDir, repoName)); os.IsNotExist(err) { - output := testshell.Run(t, rootDir, fmt.Sprintf("git clone %s --filter=blob:none --depth=1", repoURLWithToken)) - require.NotContains(t, output, "fatal") - } - - err = os.Chdir(fmt.Sprintf("%s/%s", rootDir, repoName)) - require.NoError(t, err) + personalAccessToken := os.Getenv("PRIVATE_TEST_REPO_GIT_ACCESS_TOKEN") // Run a server and executor locally to run remote bazel against env, bbServer, _ := runLocalServerAndExecutor(t, personalAccessToken) @@ -299,7 +291,7 @@ func TestCancel(t *testing.T) { // Create a workflow for the same repo - will be used to fetch the git token dbh := env.GetDBHandle() require.NotNil(t, dbh) - err = dbh.NewQuery(context.Background(), "create_git_repo_for_test").Create(&tables.GitRepository{ + err := dbh.NewQuery(context.Background(), "create_git_repo_for_test").Create(&tables.GitRepository{ RepoURL: "https://github.com/buildbuddy-io/private-test-repo", GroupID: env.GroupID1, }) @@ -366,3 +358,73 @@ func TestCancel(t *testing.T) { waitForInvocationStatus(t, ctx, bbClient, reqCtx, invocationID, inspb.InvocationStatus_DISCONNECTED_INVOCATION_STATUS) } + +func TestFetchRemoteBuildOutputs(t *testing.T) { + clonePrivateTestRepo(t) + + // Run a server and executor locally to run remote bazel against + personalAccessToken := os.Getenv("PRIVATE_TEST_REPO_GIT_ACCESS_TOKEN") + env, bbServer, _ := runLocalServerAndExecutor(t, personalAccessToken) + + // Create a workflow for the same repo - will be used to fetch the git token + dbh := env.GetDBHandle() + require.NotNil(t, dbh) + err := dbh.NewQuery(context.Background(), "create_git_repo_for_test").Create(&tables.GitRepository{ + RepoURL: "https://github.com/buildbuddy-io/private-test-repo", + GroupID: env.GroupID1, + }) + require.NoError(t, err) + + // Run remote bazel + randomStr := time.Now().String() + exitCode, err := remotebazel.HandleRemoteBazel([]string{ + fmt.Sprintf("--remote_runner=%s", bbServer.GRPCAddress()), + // Have the ci runner use the "none" isolation type because it's simpler + // to setup than a firecracker runner + "--runner_exec_properties=workload-isolation-type=none", + "--runner_exec_properties=container-image=", + // Ensure the build is happening on a clean runner, because if the build + // artifact is locally cached, we won't upload it to the remote cache + // and we won't be able to fetch it. + "--runner_exec_properties=instance_name=" + randomStr, + // Pass a startup flag to test parsing + "--digest_function=BLAKE3", + "build", + ":hello_world_go", + fmt.Sprintf("--remote_header=x-buildbuddy-api-key=%s", env.APIKey1)}) + require.NoError(t, err) + require.Equal(t, 0, exitCode) + + // Check that the remote build output was fetched locally. + // The outputs will be downloaded to a directory that may change with the platform, + // so recursively search for the build output named `hello_world_go`. + findFile := func(rootDir, targetFile string) (string, error) { + var outputPath string + err := filepath.WalkDir(rootDir, func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + + if !d.IsDir() && d.Name() == targetFile { + outputPath = path + return filepath.SkipAll // Stop searching further once the file is found + } + + return nil + }) + return outputPath, err + } + downloadedOutputPath, err := findFile(remotebazel.BuildBuddyArtifactDir, "hello_world_go") + require.NoError(t, err) + + // Make sure we can successfully run the fetched binary. + err = os.Chmod(downloadedOutputPath, 0755) + require.NoError(t, err) + + var buf bytes.Buffer + cmd := exec.Command(downloadedOutputPath) + cmd.Stdout = &buf + err = cmd.Run() + require.NoError(t, err) + require.Equal(t, "Hello! I'm a go program.\n", buf.String()) +}