From 4c5fc1987f36592b5a6d8f127f395311cb29e778 Mon Sep 17 00:00:00 2001 From: Vanja Pejovic Date: Mon, 2 Dec 2024 15:52:54 -0500 Subject: [PATCH] Stop writing failed actions with the invocation ID as part of their key (#7985) Nothing is reading this, since now executions are stored as ExecuteResponses for all actions (passed or failed). --- .../execution_service/execution_service.go | 4 ++-- .../execution_server/execution_server_test.go | 4 ++-- .../remote_execution/executor/executor.go | 12 ++---------- .../test/integration/remote_execution/BUILD | 1 + .../remote_execution/rbetest/rbetest.go | 16 +--------------- .../remote_execution/remote_execution_test.go | 19 ++++++++++--------- enterprise/server/util/execution/BUILD | 1 - enterprise/server/util/execution/execution.go | 5 ++--- server/remote_cache/digest/digest.go | 19 ------------------- tools/cas/cas.go | 16 +--------------- 10 files changed, 21 insertions(+), 76 deletions(-) diff --git a/enterprise/server/execution_service/execution_service.go b/enterprise/server/execution_service/execution_service.go index 743f02fd34c..1a2caec5da5 100644 --- a/enterprise/server/execution_service/execution_service.go +++ b/enterprise/server/execution_service/execution_service.go @@ -120,7 +120,7 @@ func (es *ExecutionService) GetExecution(ctx context.Context, req *espb.GetExecu // that owns the execution, switch to that group's ctx. // Also if the execution was done anonymously, switch to // anonymous ctx. - res, err := execution.GetCachedExecuteResponse(ctx, es.env, ex.ExecutionId) + res, err := execution.GetCachedExecuteResponse(ctx, es.env.GetActionCacheClient(), ex.ExecutionId) if err != nil { return err } @@ -166,7 +166,7 @@ func (es *ExecutionService) WaitExecution(req *espb.WaitExecutionRequest, stream // WriteExecutionProfile writes the uncompressed JSON execution profile in // Google's Trace Event Format. func (es *ExecutionService) WriteExecutionProfile(ctx context.Context, w io.Writer, executionID string) error { - res, err := execution.GetCachedExecuteResponse(ctx, es.env, executionID) + res, err := execution.GetCachedExecuteResponse(ctx, es.env.GetActionCacheClient(), executionID) if err != nil { return status.WrapError(err, "get cached execute response") } diff --git a/enterprise/server/remote_execution/execution_server/execution_server_test.go b/enterprise/server/remote_execution/execution_server/execution_server_test.go index f875acff344..f6eb0ded7dd 100644 --- a/enterprise/server/remote_execution/execution_server/execution_server_test.go +++ b/enterprise/server/remote_execution/execution_server/execution_server_test.go @@ -385,7 +385,7 @@ func testExecuteAndPublishOperation(t *testing.T, platformOverrides map[string]s // Should also be able to fetch the ExecuteResponse from cache. See field // comment on Execution.execute_response_digest for notes on serialization // format. - cachedExecuteResponse, err := execution.GetCachedExecuteResponse(ctx, env, taskID) + cachedExecuteResponse, err := execution.GetCachedExecuteResponse(ctx, env.GetActionCacheClient(), taskID) require.NoError(t, err) assert.Empty(t, cmp.Diff(expectedExecuteResponse, cachedExecuteResponse, protocmp.Transform())) @@ -440,7 +440,7 @@ func TestMarkFailed(t *testing.T) { assert.Equal(t, executionID, ex.ExecutionID) // ExecuteResponse should be cached after marking failed - executeResponse, err := execution.GetCachedExecuteResponse(ctx, env, executionID) + executeResponse, err := execution.GetCachedExecuteResponse(ctx, env.GetActionCacheClient(), executionID) require.NoError(t, err) assert.Equal(t, "It didn't work", executeResponse.GetStatus().GetMessage()) diff --git a/enterprise/server/remote_execution/executor/executor.go b/enterprise/server/remote_execution/executor/executor.go index c11415221b6..d2eff66285b 100644 --- a/enterprise/server/remote_execution/executor/executor.go +++ b/enterprise/server/remote_execution/executor/executor.go @@ -399,18 +399,10 @@ func (s *Executor) ExecuteTaskAndStreamResults(ctx context.Context, st *repb.Sch md.WorkerCompletedTimestamp = timestamppb.Now() actionResult.ExecutionMetadata = md - // If the action failed or do_not_cache is set, upload information about the error via a failed - // ActionResult under an invocation-specific digest, which will not ever be seen by bazel but - // may be viewed via the Buildbuddy UI. - if task.GetAction().GetDoNotCache() || cmdResult.Error != nil || cmdResult.ExitCode != 0 { - resultDigest, err := digest.AddInvocationIDToDigest(req.GetActionDigest(), digestFunction, task.GetInvocationId()) - if err != nil { + if !task.GetAction().GetDoNotCache() && cmdResult.Error == nil && cmdResult.ExitCode == 0 { + if err := cachetools.UploadActionResult(ctx, acClient, adInstanceDigest, actionResult); err != nil { return finishWithErrFn(status.UnavailableErrorf("Error uploading action result: %s", err.Error())) } - adInstanceDigest = digest.NewResourceName(resultDigest, req.GetInstanceName(), rspb.CacheType_AC, digestFunction) - } - if err := cachetools.UploadActionResult(ctx, acClient, adInstanceDigest, actionResult); err != nil { - return finishWithErrFn(status.UnavailableErrorf("Error uploading action result: %s", err.Error())) } // If there's an error that we know the client won't retry, return an error diff --git a/enterprise/server/test/integration/remote_execution/BUILD b/enterprise/server/test/integration/remote_execution/BUILD index a12e4327bf5..7c87c5b4f67 100644 --- a/enterprise/server/test/integration/remote_execution/BUILD +++ b/enterprise/server/test/integration/remote_execution/BUILD @@ -20,6 +20,7 @@ go_test( "//enterprise/server/testutil/buildbuddy_enterprise", "//enterprise/server/testutil/testexecutor", "//enterprise/server/testutil/testredis", + "//enterprise/server/util/execution", "//proto:build_event_stream_go_proto", "//proto:remote_execution_go_proto", "//server/build_event_protocol/build_event_handler", diff --git a/enterprise/server/test/integration/remote_execution/rbetest/rbetest.go b/enterprise/server/test/integration/remote_execution/rbetest/rbetest.go index fd015981b80..ee73bfe4219 100644 --- a/enterprise/server/test/integration/remote_execution/rbetest/rbetest.go +++ b/enterprise/server/test/integration/remote_execution/rbetest/rbetest.go @@ -697,6 +697,7 @@ func (r *Env) AddBuildBuddyServerWithOptions(opts *BuildBuddyServerOptions) *Bui env.SetAuthDB(r.testEnv.GetAuthDB()) env.SetUserDB(r.testEnv.GetUserDB()) env.SetInvocationDB(r.testEnv.GetInvocationDB()) + env.SetActionCacheClient(r.GetActionResultStorageClient()) server := newBuildBuddyServer(r.t, env, opts) r.buildBuddyServers[server] = struct{}{} @@ -918,21 +919,6 @@ func (r *Env) DownloadOutputsToNewTempDir(res *CommandResult) string { return tmpDir } -func (r *Env) GetActionResultForFailedAction(ctx context.Context, cmd *Command, invocationID string) (*repb.ActionResult, error) { - d := cmd.GetActionResourceName().GetDigest() - actionResultDigest, err := digest.AddInvocationIDToDigest(d, cmd.GetActionResourceName().GetDigestFunction(), invocationID) - if err != nil { - assert.FailNow(r.t, fmt.Sprintf("unable to attach invocation ID %q to digest", invocationID)) - } - req := &repb.GetActionResultRequest{ - InstanceName: cmd.GetActionResourceName().GetInstanceName(), - ActionDigest: actionResultDigest, - DigestFunction: cmd.GetActionResourceName().GetDigestFunction(), - } - acClient := r.GetActionResultStorageClient() - return acClient.GetActionResult(context.Background(), req) -} - func (r *Env) GetStdoutAndStderr(ctx context.Context, actionResult *repb.ActionResult, instanceName string) (string, string, error) { stdout := "" if actionResult.GetStdoutDigest() != nil { diff --git a/enterprise/server/test/integration/remote_execution/remote_execution_test.go b/enterprise/server/test/integration/remote_execution/remote_execution_test.go index 57e5cba60f0..d532152d403 100644 --- a/enterprise/server/test/integration/remote_execution/remote_execution_test.go +++ b/enterprise/server/test/integration/remote_execution/remote_execution_test.go @@ -21,6 +21,7 @@ import ( "github.com/buildbuddy-io/buildbuddy/enterprise/server/testutil/buildbuddy_enterprise" "github.com/buildbuddy-io/buildbuddy/enterprise/server/testutil/testexecutor" "github.com/buildbuddy-io/buildbuddy/enterprise/server/testutil/testredis" + "github.com/buildbuddy-io/buildbuddy/enterprise/server/util/execution" "github.com/buildbuddy-io/buildbuddy/server/build_event_protocol/build_event_handler" "github.com/buildbuddy-io/buildbuddy/server/interfaces" "github.com/buildbuddy-io/buildbuddy/server/metrics" @@ -99,8 +100,8 @@ func TestActionResultCacheWithSuccessfulAction(t *testing.T) { res := cmd.Wait() assert.Equal(t, 0, res.ExitCode, "exit code should be propagated") - _, err := rbe.GetActionResultForFailedAction(context.Background(), cmd, invocationID) - assert.True(t, status.IsNotFoundError(err)) + _, err := cachetools.GetActionResult(context.Background(), rbe.GetActionResultStorageClient(), cmd.GetActionResourceName()) + assert.NoError(t, err) } func TestActionResultCacheWithFailedAction(t *testing.T) { @@ -127,11 +128,11 @@ func TestActionResultCacheWithFailedAction(t *testing.T) { assert.Equal(t, 5, res.ExitCode, "exit code should be propagated") ctx := context.Background() - failedActionResult, err := rbe.GetActionResultForFailedAction(ctx, cmd, invocationID) - assert.NoError(t, err) - assert.Equal(t, int32(5), failedActionResult.GetExitCode(), "exit code should be set in action result") - stdout, stderr, err := rbe.GetStdoutAndStderr(ctx, failedActionResult, res.InstanceName) - assert.NoError(t, err) + execRes, err := execution.GetCachedExecuteResponse(ctx, rbe.GetActionResultStorageClient(), res.ID) + require.NoError(t, err) + assert.Equal(t, int32(5), execRes.GetResult().GetExitCode(), "exit code should be set in action result") + stdout, stderr, err := rbe.GetStdoutAndStderr(ctx, execRes.GetResult(), res.InstanceName) + require.NoError(t, err) assert.Equal(t, "hello\n", stdout, "stdout should be propagated") assert.Equal(t, "bye\n", stderr, "stderr should be propagated") @@ -194,11 +195,11 @@ func TestSimpleCommand_Timeout_StdoutStderrStillVisible(t *testing.T) { assert.Equal(t, "ExampleStderr\n", stderr, "stderr should be propagated") taskCount := testmetrics.CounterValue(t, metrics.RemoteExecutionTasksStartedCount) assert.Equal(t, 1, int(taskCount-initialTaskCount), "unexpected number of tasks started") - ar, err = rbe.GetActionResultForFailedAction(ctx, cmd, invocationID) + execRes, err := execution.GetCachedExecuteResponse(ctx, rbe.GetActionResultStorageClient(), res.ID) require.NoError(t, err) assert.Empty( t, - cmp.Diff(res.ActionResult, ar, protocmp.Transform()), + cmp.Diff(res.ActionResult, execRes.GetResult(), protocmp.Transform()), "failed action result should match what was sent in the ExecuteResponse") } diff --git a/enterprise/server/util/execution/BUILD b/enterprise/server/util/execution/BUILD index f90077a65d9..93765a8bda2 100644 --- a/enterprise/server/util/execution/BUILD +++ b/enterprise/server/util/execution/BUILD @@ -9,7 +9,6 @@ go_library( "//proto:execution_stats_go_proto", "//proto:remote_execution_go_proto", "//proto:stored_invocation_go_proto", - "//server/environment", "//server/remote_cache/digest", "//server/tables", "//server/util/proto", diff --git a/enterprise/server/util/execution/execution.go b/enterprise/server/util/execution/execution.go index eef4c3c30f3..3757015871c 100644 --- a/enterprise/server/util/execution/execution.go +++ b/enterprise/server/util/execution/execution.go @@ -5,7 +5,6 @@ import ( "strings" "time" - "github.com/buildbuddy-io/buildbuddy/server/environment" "github.com/buildbuddy-io/buildbuddy/server/remote_cache/digest" "github.com/buildbuddy-io/buildbuddy/server/tables" "github.com/buildbuddy-io/buildbuddy/server/util/proto" @@ -104,7 +103,7 @@ func TableExecToClientProto(in *tables.Execution) (*espb.Execution, error) { return out, nil } -func GetCachedExecuteResponse(ctx context.Context, env environment.Env, taskID string) (*repb.ExecuteResponse, error) { +func GetCachedExecuteResponse(ctx context.Context, ac repb.ActionCacheClient, taskID string) (*repb.ExecuteResponse, error) { rn, err := digest.ParseUploadResourceName(taskID) if err != nil { return nil, err @@ -118,7 +117,7 @@ func GetCachedExecuteResponse(ctx context.Context, env environment.Env, taskID s InstanceName: rn.GetInstanceName(), DigestFunction: rn.GetDigestFunction(), } - rsp, err := env.GetActionCacheClient().GetActionResult(ctx, req) + rsp, err := ac.GetActionResult(ctx, req) if err != nil { return nil, err } diff --git a/server/remote_cache/digest/digest.go b/server/remote_cache/digest/digest.go index 2bbe5a834e5..1d272259f70 100644 --- a/server/remote_cache/digest/digest.go +++ b/server/remote_cache/digest/digest.go @@ -415,25 +415,6 @@ func ComputeForFile(path string, digestType repb.DigestFunction_Value) (*repb.Di return Compute(f, digestType) } -// AddInvocationIDToDigest combines the hash of the input digest and input invocationID and re-hash. -// This is only to be used for failed action results. -func AddInvocationIDToDigest(digest *repb.Digest, digestType repb.DigestFunction_Value, invocationID string) (*repb.Digest, error) { - if digest == nil { - return nil, status.FailedPreconditionError("nil digest") - } - - h, err := HashForDigestType(digestType) - if err != nil { - return nil, err - } - h.Write([]byte(digest.Hash)) - h.Write([]byte(invocationID)) - return &repb.Digest{ - Hash: fmt.Sprintf("%x", h.Sum(nil)), - SizeBytes: digest.SizeBytes, - }, nil -} - func isResourceName(url string, matcher *regexp.Regexp) bool { return matcher.MatchString(url) } diff --git a/tools/cas/cas.go b/tools/cas/cas.go index 1315352e148..17e4012bff7 100644 --- a/tools/cas/cas.go +++ b/tools/cas/cas.go @@ -36,7 +36,6 @@ var ( instanceName = flag.String("remote_instance_name", "", "Remote instance name") apiKey = flag.String("api_key", "", "API key to attach to the outgoing context") - invocationID = flag.String("invocation_id", "", "Invocation ID. This is required when fetching the result of a failed action. Otherwise, it's optional.") showMetadata = flag.Bool("metadata", false, "Whether to fetch and log metadata for the digest (printed to stderr).") showMetadataOnly = flag.Bool("metadata_only", false, "Whether to *only* fetch metadata, not the contents. This will print the metadata to stdout instead of stderr.") @@ -55,10 +54,6 @@ var ( // Show stderr contents: // // bazel run //tools/cas -- -target=grpcs://remote.buildbuddy.dev -digest=HASH/SIZE -type=stderr -// -// Show a failed action result proto (requires invocation ID): -// -// bazel run //tools/cas -- -target=grpcs://remote.buildbuddy.dev -digest=HASH/SIZE -type=ActionResult -invocation_id=IID func main() { flag.Parse() if *target == "" { @@ -143,16 +138,7 @@ func main() { if *blobType == "ActionResult" { ar, err := cachetools.GetActionResult(ctx, acClient, ind) if err != nil { - log.Infof("Could not fetch ActionResult; maybe the action failed. Attempting to fetch failed action using invocation ID = %q", *invocationID) - failedDigest, err := digest.AddInvocationIDToDigest(ind.GetDigest(), ind.GetDigestFunction(), *invocationID) - if err != nil { - log.Fatal(err.Error()) - } - ind := digest.NewResourceName(failedDigest, ind.GetInstanceName(), rspb.CacheType_AC, repb.DigestFunction_SHA256) - ar, err = cachetools.GetActionResult(ctx, acClient, ind) - if err != nil { - log.Fatal(err.Error()) - } + log.Fatal(err.Error()) } printMessage(ar) return