From bc54c648aa1f99509c7c36d5e6b570d066689209 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 26 Jan 2021 01:24:39 -0800 Subject: [PATCH] Remote: Use parameters instead of thread-local storage to provide tracing metadata. Remote Execution API defines RequestMetadata which is an optional message attached to any RPC request to tell the server about an external context of the request. Existing code uses Context object from gRPC which is essentially a thread-local storage to provide metadata: 1. We always attach the Context even the underlying implementation isn't based on gRPC. 2. It is error prone in a multi-threaded context. This is the first step of a series changes where we introduce RemoteActionExecutionContext and update RemoteCacheClient#downloadActionResult to use it. These is a regression that networkTime of SpawnMetrics will not be correct since the NetworkTimeInterceptor is no longer installed at RemoteModule and other methods haven't get updated to use the RemoteActionExecutionContext. The networkTime will back to normal once we finish the cleanup. PiperOrigin-RevId: 353819916 --- .../build/lib/remote/GrpcCacheClient.java | 12 +-- ...kTime.java => NetworkTimeInterceptor.java} | 68 ++++----------- .../build/lib/remote/RemoteCache.java | 6 +- .../build/lib/remote/RemoteModule.java | 7 +- .../RemoteRepositoryRemoteExecutor.java | 26 +++++- ...RemoteRepositoryRemoteExecutorFactory.java | 17 ++-- .../build/lib/remote/RemoteSpawnCache.java | 16 +++- .../build/lib/remote/RemoteSpawnRunner.java | 15 +++- .../devtools/build/lib/remote/common/BUILD | 1 + .../build/lib/remote/common/NetworkTime.java | 54 ++++++++++++ .../common/RemoteActionExecutionContext.java | 29 +++++++ .../RemoteActionExecutionContextImpl.java | 39 +++++++++ .../lib/remote/common/RemoteCacheClient.java | 3 +- .../remote/disk/DiskAndRemoteCacheClient.java | 7 +- .../lib/remote/disk/DiskCacheClient.java | 3 +- .../lib/remote/http/HttpCacheClient.java | 3 +- .../lib/remote/util/TracingMetadataUtils.java | 42 +++++---- .../build/lib/remote/GrpcCacheClientTest.java | 21 ++++- .../RemoteRepositoryRemoteExecutorTest.java | 17 ++-- .../lib/remote/RemoteSpawnCacheTest.java | 60 ++++++++++--- .../lib/remote/RemoteSpawnRunnerTest.java | 86 +++++++++++++++---- .../lib/remote/util/InMemoryCacheClient.java | 3 +- .../remote/worker/ActionCacheServer.java | 12 ++- 23 files changed, 408 insertions(+), 139 deletions(-) rename src/main/java/com/google/devtools/build/lib/remote/{util/NetworkTime.java => NetworkTimeInterceptor.java} (60%) create mode 100644 src/main/java/com/google/devtools/build/lib/remote/common/NetworkTime.java create mode 100644 src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContext.java create mode 100644 src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContextImpl.java diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java index 93a5c1c7309488..a88ab82c20f8ed 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java @@ -47,6 +47,7 @@ import com.google.devtools.build.lib.remote.RemoteRetrier.ProgressiveBackoff; import com.google.devtools.build.lib.remote.common.CacheNotFoundException; import com.google.devtools.build.lib.remote.common.MissingDigestsFinder; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; import com.google.devtools.build.lib.remote.common.RemoteCacheClient; import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.util.DigestOutputStream; @@ -142,9 +143,11 @@ private ActionCacheBlockingStub acBlockingStub() { .withDeadlineAfter(options.remoteTimeout.getSeconds(), TimeUnit.SECONDS); } - private ActionCacheFutureStub acFutureStub() { + private ActionCacheFutureStub acFutureStub(RemoteActionExecutionContext context) { return ActionCacheGrpc.newFutureStub(channel) - .withInterceptors(TracingMetadataUtils.attachMetadataFromContextInterceptor()) + .withInterceptors( + TracingMetadataUtils.attachMetadataInterceptor(context.getRequestMetadata()), + new NetworkTimeInterceptor(context::getNetworkTime)) .withCallCredentials(callCredentialsProvider.getCallCredentials()) .withDeadlineAfter(options.remoteTimeout.getSeconds(), TimeUnit.SECONDS); } @@ -240,7 +243,7 @@ private ListenableFuture handleStatus(ListenableFuture downloadActionResult( - ActionKey actionKey, boolean inlineOutErr) { + RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) { GetActionResultRequest request = GetActionResultRequest.newBuilder() .setInstanceName(options.remoteInstanceName) @@ -248,11 +251,10 @@ public ListenableFuture downloadActionResult( .setInlineStderr(inlineOutErr) .setInlineStdout(inlineOutErr) .build(); - Context ctx = Context.current(); return Utils.refreshIfUnauthenticatedAsync( () -> retrier.executeAsync( - () -> ctx.call(() -> handleStatus(acFutureStub().getActionResult(request)))), + () -> handleStatus(acFutureStub(context).getActionResult(request))), callCredentialsProvider); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/NetworkTime.java b/src/main/java/com/google/devtools/build/lib/remote/NetworkTimeInterceptor.java similarity index 60% rename from src/main/java/com/google/devtools/build/lib/remote/util/NetworkTime.java rename to src/main/java/com/google/devtools/build/lib/remote/NetworkTimeInterceptor.java index 52e11ed55e636a..ac953aa48f9d42 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/util/NetworkTime.java +++ b/src/main/java/com/google/devtools/build/lib/remote/NetworkTimeInterceptor.java @@ -1,4 +1,4 @@ -// Copyright 2019 The Bazel Authors. All rights reserved. +// Copyright 2020 The Bazel Authors. All rights reserved. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -11,12 +11,10 @@ // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. // See the License for the specific language governing permissions and // limitations under the License. -package com.google.devtools.build.lib.remote.util; +package com.google.devtools.build.lib.remote; import build.bazel.remote.execution.v2.ExecutionGrpc; -import com.google.common.base.MoreObjects; -import com.google.common.base.Stopwatch; -import com.google.devtools.build.lib.concurrent.ThreadSafety; +import com.google.devtools.build.lib.remote.common.NetworkTime; import io.grpc.CallOptions; import io.grpc.Channel; import io.grpc.ClientCall; @@ -27,59 +25,31 @@ import io.grpc.Metadata; import io.grpc.MethodDescriptor; import io.grpc.Status; -import java.time.Duration; +import java.util.function.Supplier; -/** Reentrant wall clock stopwatch and grpc interceptor for network waits. */ -@ThreadSafety.ThreadSafe -public class NetworkTime { +/** The ClientInterceptor used to track network time. */ +public class NetworkTimeInterceptor implements ClientInterceptor { public static final Context.Key CONTEXT_KEY = Context.key("remote-network-time"); + private final Supplier networkTimeSupplier; - private final Stopwatch wallTime = Stopwatch.createUnstarted(); - private int outstanding = 0; - - private synchronized void start() { - if (!wallTime.isRunning()) { - wallTime.start(); - } - outstanding++; - } - - private synchronized void stop() { - if (--outstanding == 0) { - wallTime.stop(); - } - } - - public Duration getDuration() { - return wallTime.elapsed(); + public NetworkTimeInterceptor(Supplier networkTimeSupplier) { + this.networkTimeSupplier = networkTimeSupplier; } @Override - public String toString() { - return MoreObjects.toStringHelper(this) - .add("outstanding", outstanding) - .add("wallTime", wallTime) - .add("wallTime.isRunning", wallTime.isRunning()) - .toString(); - } - - /** The ClientInterceptor used to track network time. */ - public static class Interceptor implements ClientInterceptor { - @Override - public ClientCall interceptCall( - MethodDescriptor method, CallOptions callOptions, Channel next) { - ClientCall call = next.newCall(method, callOptions); - // prevent accounting for execution wait time - if (method != ExecutionGrpc.getExecuteMethod() - && method != ExecutionGrpc.getWaitExecutionMethod()) { - NetworkTime networkTime = CONTEXT_KEY.get(); - if (networkTime != null) { - call = new NetworkTimeCall<>(call, networkTime); - } + public ClientCall interceptCall( + MethodDescriptor method, CallOptions callOptions, Channel next) { + ClientCall call = next.newCall(method, callOptions); + // prevent accounting for execution wait time + if (method != ExecutionGrpc.getExecuteMethod() + && method != ExecutionGrpc.getWaitExecutionMethod()) { + NetworkTime networkTime = networkTimeSupplier.get(); + if (networkTime != null) { + call = new NetworkTimeCall<>(call, networkTime); } - return call; } + return call; } private static class NetworkTimeCall diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java index ec6e6961b6832f..59138b5a3c1c79 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java @@ -55,6 +55,7 @@ import com.google.devtools.build.lib.remote.RemoteCache.ActionResultMetadata.DirectoryMetadata; import com.google.devtools.build.lib.remote.RemoteCache.ActionResultMetadata.FileMetadata; import com.google.devtools.build.lib.remote.RemoteCache.ActionResultMetadata.SymlinkMetadata; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; import com.google.devtools.build.lib.remote.common.RemoteActionFileArtifactValue; import com.google.devtools.build.lib.remote.common.RemoteCacheClient; import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey; @@ -115,9 +116,10 @@ public RemoteCache( this.digestUtil = digestUtil; } - public ActionResult downloadActionResult(ActionKey actionKey, boolean inlineOutErr) + public ActionResult downloadActionResult( + RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) throws IOException, InterruptedException { - return getFromFuture(cacheProtocol.downloadActionResult(actionKey, inlineOutErr)); + return getFromFuture(cacheProtocol.downloadActionResult(context, actionKey, inlineOutErr)); } /** diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java index 360b2a5e41814f..ecb175e3992a8b 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java @@ -68,7 +68,6 @@ import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.options.RemoteOutputsMode; import com.google.devtools.build.lib.remote.util.DigestUtil; -import com.google.devtools.build.lib.remote.util.NetworkTime; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.remote.util.Utils; import com.google.devtools.build.lib.runtime.BlazeModule; @@ -330,7 +329,6 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { if (loggingInterceptor != null) { interceptors.add(loggingInterceptor); } - interceptors.add(new NetworkTime.Interceptor()); try { execChannel = RemoteCacheClientFactory.createGrpcChannelPool( @@ -357,7 +355,6 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { if (loggingInterceptor != null) { interceptors.add(loggingInterceptor); } - interceptors.add(new NetworkTime.Interceptor()); try { cacheChannel = RemoteCacheClientFactory.createGrpcChannelPool( @@ -551,7 +548,9 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { remoteCache, remoteExecutor, digestUtil, - repoContext, + buildRequestId, + invocationId, + "repository_rule", remoteOptions.remoteInstanceName, remoteOptions.remoteAcceptCached)); } else { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutor.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutor.java index 852bce5ea25c8d..e66897b599ad48 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutor.java @@ -28,11 +28,15 @@ import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.ProfilerTask; import com.google.devtools.build.lib.profiler.SilentCloseable; +import com.google.devtools.build.lib.remote.common.NetworkTime; import com.google.devtools.build.lib.remote.common.OperationObserver; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContextImpl; import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey; import com.google.devtools.build.lib.remote.common.RemoteExecutionClient; import com.google.devtools.build.lib.remote.merkletree.MerkleTree; import com.google.devtools.build.lib.remote.util.DigestUtil; +import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.remote.util.Utils; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor; import com.google.devtools.build.lib.vfs.Path; @@ -49,7 +53,9 @@ public class RemoteRepositoryRemoteExecutor implements RepositoryRemoteExecutor private final RemoteExecutionCache remoteCache; private final RemoteExecutionClient remoteExecutor; private final DigestUtil digestUtil; - private final Context requestCtx; + private final String buildRequestId; + private final String commandId; + private final String actionId; private final String remoteInstanceName; private final boolean acceptCached; @@ -58,13 +64,17 @@ public RemoteRepositoryRemoteExecutor( RemoteExecutionCache remoteCache, RemoteExecutionClient remoteExecutor, DigestUtil digestUtil, - Context requestCtx, + String buildRequestId, + String commandId, + String actionId, String remoteInstanceName, boolean acceptCached) { this.remoteCache = remoteCache; this.remoteExecutor = remoteExecutor; this.digestUtil = digestUtil; - this.requestCtx = requestCtx; + this.buildRequestId = buildRequestId; + this.commandId = commandId; + this.actionId = actionId; this.remoteInstanceName = remoteInstanceName; this.acceptCached = acceptCached; } @@ -100,6 +110,8 @@ public ExecutionResult execute( String workingDirectory, Duration timeout) throws IOException, InterruptedException { + Context requestCtx = + TracingMetadataUtils.contextWithMetadata(buildRequestId, commandId, actionId); Context prev = requestCtx.attach(); try { Platform platform = PlatformUtils.buildPlatformProto(executionProperties); @@ -117,10 +129,16 @@ public ExecutionResult execute( commandHash, merkleTree.getRootDigest(), timeout, acceptCached); Digest actionDigest = digestUtil.compute(action); ActionKey actionKey = new ActionKey(actionDigest); + RemoteActionExecutionContext remoteActionExecutionContext = + new RemoteActionExecutionContextImpl( + TracingMetadataUtils.buildMetadata(buildRequestId, commandId, actionId), + new NetworkTime()); ActionResult actionResult; try (SilentCloseable c = Profiler.instance().profile(ProfilerTask.REMOTE_CACHE_CHECK, "check cache hit")) { - actionResult = remoteCache.downloadActionResult(actionKey, /* inlineOutErr= */ true); + actionResult = + remoteCache.downloadActionResult( + remoteActionExecutionContext, actionKey, /* inlineOutErr= */ true); } if (actionResult == null || actionResult.getExitCode() != 0) { try (SilentCloseable c = diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutorFactory.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutorFactory.java index 3cf49c617f7fea..70ee4d903ca690 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutorFactory.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutorFactory.java @@ -17,7 +17,6 @@ import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutorFactory; -import io.grpc.Context; /** Factory for {@link RemoteRepositoryRemoteExecutor}. */ class RemoteRepositoryRemoteExecutorFactory implements RepositoryRemoteExecutorFactory { @@ -25,7 +24,9 @@ class RemoteRepositoryRemoteExecutorFactory implements RepositoryRemoteExecutorF private final RemoteExecutionCache remoteExecutionCache; private final RemoteExecutionClient remoteExecutor; private final DigestUtil digestUtil; - private final Context requestCtx; + private final String buildRequestId; + private final String commandId; + private final String actionId; private final String remoteInstanceName; private final boolean acceptCached; @@ -34,13 +35,17 @@ class RemoteRepositoryRemoteExecutorFactory implements RepositoryRemoteExecutorF RemoteExecutionCache remoteExecutionCache, RemoteExecutionClient remoteExecutor, DigestUtil digestUtil, - Context requestCtx, + String buildRequestId, + String commandId, + String actionId, String remoteInstanceName, boolean acceptCached) { this.remoteExecutionCache = remoteExecutionCache; this.remoteExecutor = remoteExecutor; this.digestUtil = digestUtil; - this.requestCtx = requestCtx; + this.buildRequestId = buildRequestId; + this.commandId = commandId; + this.actionId = actionId; this.remoteInstanceName = remoteInstanceName; this.acceptCached = acceptCached; } @@ -51,7 +56,9 @@ public RepositoryRemoteExecutor create() { remoteExecutionCache, remoteExecutor, digestUtil, - requestCtx, + buildRequestId, + commandId, + actionId, remoteInstanceName, acceptCached); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java index 51a1d659035784..5bdda3e57d8008 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java @@ -48,12 +48,14 @@ import com.google.devtools.build.lib.profiler.ProfilerTask; import com.google.devtools.build.lib.profiler.SilentCloseable; import com.google.devtools.build.lib.remote.common.CacheNotFoundException; +import com.google.devtools.build.lib.remote.common.NetworkTime; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContextImpl; import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey; import com.google.devtools.build.lib.remote.merkletree.MerkleTree; import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.options.RemoteOutputsMode; import com.google.devtools.build.lib.remote.util.DigestUtil; -import com.google.devtools.build.lib.remote.util.NetworkTime; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.remote.util.Utils; import com.google.devtools.build.lib.remote.util.Utils.InMemoryOutput; @@ -151,9 +153,15 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context) digestUtil.compute(command), merkleTreeRoot, context.getTimeout(), true); // Look up action cache, and reuse the action output if it is found. ActionKey actionKey = digestUtil.computeActionKey(action); + + RemoteActionExecutionContext remoteActionExecutionContext = + new RemoteActionExecutionContextImpl( + TracingMetadataUtils.buildMetadata( + buildRequestId, commandId, actionKey.getDigest().getHash()), + networkTime); Context withMetadata = TracingMetadataUtils.contextWithMetadata(buildRequestId, commandId, actionKey) - .withValue(NetworkTime.CONTEXT_KEY, networkTime); + .withValue(NetworkTimeInterceptor.CONTEXT_KEY, networkTime); Profiler prof = Profiler.instance(); if (options.remoteAcceptCached @@ -165,7 +173,9 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context) try { ActionResult result; try (SilentCloseable c = prof.profile(ProfilerTask.REMOTE_CACHE_CHECK, "check cache hit")) { - result = remoteCache.downloadActionResult(actionKey, /* inlineOutErr= */ false); + result = + remoteCache.downloadActionResult( + remoteActionExecutionContext, actionKey, /* inlineOutErr= */ false); } // In case the remote cache returned a failed action (exit code != 0) we treat it as a // cache miss diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java index f1925062de5a74..c72d9c05cf7c33 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java @@ -66,14 +66,16 @@ import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.ProfilerTask; import com.google.devtools.build.lib.profiler.SilentCloseable; +import com.google.devtools.build.lib.remote.common.NetworkTime; import com.google.devtools.build.lib.remote.common.OperationObserver; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContextImpl; import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey; import com.google.devtools.build.lib.remote.common.RemoteExecutionClient; import com.google.devtools.build.lib.remote.merkletree.MerkleTree; import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.options.RemoteOutputsMode; import com.google.devtools.build.lib.remote.util.DigestUtil; -import com.google.devtools.build.lib.remote.util.NetworkTime; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.remote.util.Utils; import com.google.devtools.build.lib.remote.util.Utils.InMemoryOutput; @@ -244,9 +246,15 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) NetworkTime networkTime = new NetworkTime(); // Look up action cache, and reuse the action output if it is found. ActionKey actionKey = digestUtil.computeActionKey(action); + + RemoteActionExecutionContext remoteActionExecutionContext = + new RemoteActionExecutionContextImpl( + TracingMetadataUtils.buildMetadata( + buildRequestId, commandId, actionKey.getDigest().getHash()), + networkTime); Context withMetadata = TracingMetadataUtils.contextWithMetadata(buildRequestId, commandId, actionKey) - .withValue(NetworkTime.CONTEXT_KEY, networkTime); + .withValue(NetworkTimeInterceptor.CONTEXT_KEY, networkTime); Context previous = withMetadata.attach(); Profiler prof = Profiler.instance(); try { @@ -256,7 +264,8 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) try (SilentCloseable c = prof.profile(ProfilerTask.REMOTE_CACHE_CHECK, "check cache hit")) { cachedResult = acceptCachedResult - ? remoteCache.downloadActionResult(actionKey, /* inlineOutErr= */ false) + ? remoteCache.downloadActionResult( + remoteActionExecutionContext, actionKey, /* inlineOutErr= */ false) : null; } if (cachedResult != null) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/BUILD b/src/main/java/com/google/devtools/build/lib/remote/common/BUILD index 024321d2695b23..515eb63205d0a2 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/common/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/common/BUILD @@ -15,6 +15,7 @@ java_library( srcs = glob(["*.java"]), deps = [ "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", + "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/vfs", "//third_party:guava", "//third_party/protobuf:protobuf_java", diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/NetworkTime.java b/src/main/java/com/google/devtools/build/lib/remote/common/NetworkTime.java new file mode 100644 index 00000000000000..dab14c0879befe --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/remote/common/NetworkTime.java @@ -0,0 +1,54 @@ +// Copyright 2019 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.remote.common; + +import com.google.common.base.MoreObjects; +import com.google.common.base.Stopwatch; +import com.google.devtools.build.lib.concurrent.ThreadSafety; +import java.time.Duration; + +/** Reentrant wall clock stopwatch and grpc interceptor for network waits. */ +@ThreadSafety.ThreadSafe +public class NetworkTime { + + private final Stopwatch wallTime = Stopwatch.createUnstarted(); + private int outstanding = 0; + + public synchronized void start() { + if (!wallTime.isRunning()) { + wallTime.start(); + } + outstanding++; + } + + public synchronized void stop() { + if (--outstanding == 0) { + wallTime.stop(); + } + } + + public Duration getDuration() { + return wallTime.elapsed(); + } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add("outstanding", outstanding) + .add("wallTime", wallTime) + .add("wallTime.isRunning", wallTime.isRunning()) + .toString(); + } + +} diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContext.java b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContext.java new file mode 100644 index 00000000000000..6d61cd0dbdb5e1 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContext.java @@ -0,0 +1,29 @@ +// Copyright 2020 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.remote.common; + +import build.bazel.remote.execution.v2.RequestMetadata; + +/** A context that provide remote execution related information for executing an action remotely. */ +public interface RemoteActionExecutionContext { + + /** Get the {@link RequestMetadata} for the action being executed. */ + RequestMetadata getRequestMetadata(); + + /** + * Get the {@link NetworkTime} instance used to measure the network time during the action + * execution. + */ + NetworkTime getNetworkTime(); +} diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContextImpl.java b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContextImpl.java new file mode 100644 index 00000000000000..f4894e31740778 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContextImpl.java @@ -0,0 +1,39 @@ +// Copyright 2020 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.remote.common; + +import build.bazel.remote.execution.v2.RequestMetadata; + +/** A {@link RemoteActionExecutionContext} implementation */ +public class RemoteActionExecutionContextImpl implements RemoteActionExecutionContext { + + private final RequestMetadata requestMetadata; + private final NetworkTime networkTime; + + public RemoteActionExecutionContextImpl( + RequestMetadata requestMetadata, NetworkTime networkTime) { + this.requestMetadata = requestMetadata; + this.networkTime = networkTime; + } + + @Override + public RequestMetadata getRequestMetadata() { + return requestMetadata; + } + + @Override + public NetworkTime getNetworkTime() { + return networkTime; + } +} diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/RemoteCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteCacheClient.java index ae6a15113f72d9..11fa078357ca8b 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/common/RemoteCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteCacheClient.java @@ -72,7 +72,8 @@ public int hashCode() { * @return A Future representing pending download of an action result. If an action result for * {@code actionKey} cannot be found the result of the Future is {@code null}. */ - ListenableFuture downloadActionResult(ActionKey actionKey, boolean inlineOutErr); + ListenableFuture downloadActionResult( + RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr); /** * Uploads an action result for the {@code actionKey}. diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java index 4c3a65cdc86800..d188191652d12d 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java @@ -20,6 +20,7 @@ import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.MoreExecutors; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; import com.google.devtools.build.lib.remote.common.RemoteCacheClient; import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.vfs.Path; @@ -164,14 +165,14 @@ public ListenableFuture downloadBlob(Digest digest, OutputStream out) { @Override public ListenableFuture downloadActionResult( - ActionKey actionKey, boolean inlineOutErr) { + RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) { if (diskCache.containsActionResult(actionKey)) { - return diskCache.downloadActionResult(actionKey, inlineOutErr); + return diskCache.downloadActionResult(context, actionKey, inlineOutErr); } if (!options.incompatibleRemoteResultsIgnoreDisk || options.remoteAcceptCached) { return Futures.transformAsync( - remoteCache.downloadActionResult(actionKey, inlineOutErr), + remoteCache.downloadActionResult(context, actionKey, inlineOutErr), (actionResult) -> { if (actionResult == null) { return Futures.immediateFuture(null); diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java index 62434d9cd16bb3..3722cc56716523 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java @@ -21,6 +21,7 @@ import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.MoreExecutors; import com.google.devtools.build.lib.remote.common.CacheNotFoundException; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; import com.google.devtools.build.lib.remote.common.RemoteCacheClient; import com.google.devtools.build.lib.remote.util.DigestOutputStream; import com.google.devtools.build.lib.remote.util.DigestUtil; @@ -101,7 +102,7 @@ public ListenableFuture downloadBlob(Digest digest, OutputStream out) { @Override public ListenableFuture downloadActionResult( - ActionKey actionKey, boolean inlineOutErr) { + RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) { return Utils.downloadAsActionResult( actionKey, (digest, out) -> download(digest, out, /* isActionCache= */ true)); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/http/HttpCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/http/HttpCacheClient.java index bdfca850fd92f2..c35d9f074a88cf 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/http/HttpCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/http/HttpCacheClient.java @@ -26,6 +26,7 @@ import com.google.common.util.concurrent.MoreExecutors; import com.google.common.util.concurrent.SettableFuture; import com.google.devtools.build.lib.remote.common.CacheNotFoundException; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; import com.google.devtools.build.lib.remote.common.RemoteCacheClient; import com.google.devtools.build.lib.remote.util.DigestOutputStream; import com.google.devtools.build.lib.remote.util.DigestUtil; @@ -572,7 +573,7 @@ private void getAfterCredentialRefresh(DownloadCommand cmd, SettableFuture @Override public ListenableFuture downloadActionResult( - ActionKey actionKey, boolean inlineOutErr) { + RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) { return Utils.downloadAsActionResult( actionKey, (digest, out) -> get(digest, out, /* casDownload= */ false)); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/TracingMetadataUtils.java b/src/main/java/com/google/devtools/build/lib/remote/util/TracingMetadataUtils.java index 79a0cbb8afa3ee..cb880ce9525ac0 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/util/TracingMetadataUtils.java +++ b/src/main/java/com/google/devtools/build/lib/remote/util/TracingMetadataUtils.java @@ -58,6 +58,22 @@ public static Context contextWithMetadata( return contextWithMetadata(buildRequestId, commandId, actionKey.getDigest().getHash()); } + public static RequestMetadata buildMetadata( + String buildRequestId, String commandId, String actionId) { + Preconditions.checkNotNull(buildRequestId); + Preconditions.checkNotNull(commandId); + Preconditions.checkNotNull(actionId); + return RequestMetadata.newBuilder() + .setCorrelatedInvocationsId(buildRequestId) + .setToolInvocationId(commandId) + .setActionId(actionId) + .setToolDetails( + ToolDetails.newBuilder() + .setToolName("bazel") + .setToolVersion(BlazeVersionInfo.instance().getVersion())) + .build(); + } + /** * Returns a new gRPC context derived from the current context, with {@link RequestMetadata} * accessible by the {@link fromCurrentContext()} method. @@ -67,19 +83,7 @@ public static Context contextWithMetadata( */ public static Context contextWithMetadata( String buildRequestId, String commandId, String actionId) { - Preconditions.checkNotNull(buildRequestId); - Preconditions.checkNotNull(commandId); - Preconditions.checkNotNull(actionId); - RequestMetadata metadata = - RequestMetadata.newBuilder() - .setCorrelatedInvocationsId(buildRequestId) - .setToolInvocationId(commandId) - .setActionId(actionId) - .setToolDetails( - ToolDetails.newBuilder() - .setToolName("bazel") - .setToolVersion(BlazeVersionInfo.instance().getVersion())) - .build(); + RequestMetadata metadata = buildMetadata(buildRequestId, commandId, actionId); return contextWithMetadata(metadata); } @@ -107,8 +111,13 @@ public static RequestMetadata fromCurrentContext() { * @throws {@link IllegalStateException} when the metadata is not defined in the current context. */ public static Metadata headersFromCurrentContext() { + return headersFromRequestMetadata(fromCurrentContext()); + } + + /** Creates a {@link Metadata} containing the {@link RequestMetadata}. */ + public static Metadata headersFromRequestMetadata(RequestMetadata requestMetadata) { Metadata headers = new Metadata(); - headers.put(METADATA_KEY, fromCurrentContext()); + headers.put(METADATA_KEY, requestMetadata); return headers; } @@ -124,6 +133,10 @@ public static ClientInterceptor attachMetadataFromContextInterceptor() { return MetadataUtils.newAttachHeadersInterceptor(headersFromCurrentContext()); } + public static ClientInterceptor attachMetadataInterceptor(RequestMetadata requestMetadata) { + return MetadataUtils.newAttachHeadersInterceptor(headersFromRequestMetadata(requestMetadata)); + } + private static Metadata newMetadataForHeaders(List> headers) { Metadata metadata = new Metadata(); headers.forEach( @@ -169,5 +182,4 @@ public Listener interceptCall( return Contexts.interceptCall(ctx, call, headers, next); } } - } diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java index aadb0eeb4eed43..6d8af4281a60f5 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java @@ -61,6 +61,9 @@ import com.google.devtools.build.lib.clock.JavaClock; import com.google.devtools.build.lib.remote.RemoteRetrier.ExponentialBackoff; import com.google.devtools.build.lib.remote.Retrier.Backoff; +import com.google.devtools.build.lib.remote.common.NetworkTime; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContextImpl; import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey; import com.google.devtools.build.lib.remote.merkletree.MerkleTree; import com.google.devtools.build.lib.remote.options.RemoteOptions; @@ -129,6 +132,7 @@ public class GrpcCacheClientTest { private final String fakeServerName = "fake server for " + getClass(); private Server fakeServer; private Context withEmptyMetadata; + private RemoteActionExecutionContext remoteActionExecutionContext; private Context prevContext; private ListeningScheduledExecutorService retryService; @@ -152,9 +156,13 @@ public final void setUp() throws Exception { FileSystemUtils.createDirectoryAndParents(stdout.getParentDirectory()); FileSystemUtils.createDirectoryAndParents(stderr.getParentDirectory()); outErr = new FileOutErr(stdout, stderr); + remoteActionExecutionContext = + new RemoteActionExecutionContextImpl( + TracingMetadataUtils.buildMetadata( + "none", "none", Digest.getDefaultInstance().getHash()), + new NetworkTime()); withEmptyMetadata = - TracingMetadataUtils.contextWithMetadata( - "none", "none", DIGEST_UTIL.asActionKey(Digest.getDefaultInstance())); + TracingMetadataUtils.contextWithMetadata(remoteActionExecutionContext.getRequestMetadata()); retryService = MoreExecutors.listeningDecorator(Executors.newScheduledThreadPool(1)); prevContext = withEmptyMetadata.attach(); @@ -761,7 +769,9 @@ public void getActionResult( GrpcCacheClient client = newClient(remoteOptions); RemoteCache remoteCache = new RemoteCache(client, remoteOptions, DIGEST_UTIL); remoteCache.downloadActionResult( - DIGEST_UTIL.asActionKey(DIGEST_UTIL.computeAsUtf8("key")), /* inlineOutErr= */ false); + remoteActionExecutionContext, + DIGEST_UTIL.asActionKey(DIGEST_UTIL.computeAsUtf8("key")), + /* inlineOutErr= */ false); } @Test @@ -1052,7 +1062,10 @@ public void getActionResult( (numErrors-- <= 0 ? Status.NOT_FOUND : Status.UNAVAILABLE).asRuntimeException()); } }); - assertThat(getFromFuture(client.downloadActionResult(actionKey, /* inlineOutErr= */ false))) + assertThat( + getFromFuture( + client.downloadActionResult( + remoteActionExecutionContext, actionKey, /* inlineOutErr= */ false))) .isNull(); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutorTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutorTest.java index 80d36bca934639..96c87a6a163581 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutorTest.java @@ -31,7 +31,6 @@ import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor.ExecutionResult; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.protobuf.ByteString; -import io.grpc.Context; import java.io.IOException; import java.nio.charset.StandardCharsets; import java.time.Duration; @@ -62,7 +61,9 @@ public void setup() { remoteCache, remoteExecutor, DIGEST_UTIL, - Context.current(), + "none", + "none", + "repo", /* remoteInstanceName= */ "foo", /* acceptCached= */ true); } @@ -73,7 +74,7 @@ public void testZeroExitCodeFromCache() throws IOException, InterruptedException // Arrange ActionResult cachedResult = ActionResult.newBuilder().setExitCode(0).build(); - when(remoteCache.downloadActionResult(any(), /* inlineOutErr= */ eq(true))) + when(remoteCache.downloadActionResult(any(), any(), /* inlineOutErr= */ eq(true))) .thenReturn(cachedResult); // Act @@ -87,7 +88,7 @@ public void testZeroExitCodeFromCache() throws IOException, InterruptedException /* timeout= */ Duration.ZERO); // Assert - verify(remoteCache).downloadActionResult(any(), anyBoolean()); + verify(remoteCache).downloadActionResult(any(), any(), anyBoolean()); // Don't fallback to execution verify(remoteExecutor, never()).executeRemotely(any(), any()); @@ -100,7 +101,7 @@ public void testNoneZeroExitCodeFromCache() throws IOException, InterruptedExcep // Arrange ActionResult cachedResult = ActionResult.newBuilder().setExitCode(1).build(); - when(remoteCache.downloadActionResult(any(), /* inlineOutErr= */ eq(true))) + when(remoteCache.downloadActionResult(any(), any(), /* inlineOutErr= */ eq(true))) .thenReturn(cachedResult); ExecuteResponse response = ExecuteResponse.newBuilder().setResult(cachedResult).build(); @@ -117,7 +118,7 @@ public void testNoneZeroExitCodeFromCache() throws IOException, InterruptedExcep /* timeout= */ Duration.ZERO); // Assert - verify(remoteCache).downloadActionResult(any(), anyBoolean()); + verify(remoteCache).downloadActionResult(any(), any(), anyBoolean()); // Fallback to execution verify(remoteExecutor).executeRemotely(any(), any()); @@ -137,7 +138,7 @@ public void testInlineStdoutStderr() throws IOException, InterruptedException { .setStdoutRaw(ByteString.copyFrom(stdout)) .setStderrRaw(ByteString.copyFrom(stderr)) .build(); - when(remoteCache.downloadActionResult(any(), /* inlineOutErr= */ eq(true))) + when(remoteCache.downloadActionResult(any(), any(), /* inlineOutErr= */ eq(true))) .thenReturn(cachedResult); ExecuteResponse response = ExecuteResponse.newBuilder().setResult(cachedResult).build(); @@ -154,7 +155,7 @@ public void testInlineStdoutStderr() throws IOException, InterruptedException { /* timeout= */ Duration.ZERO); // Assert - verify(remoteCache).downloadActionResult(any(), /* inlineOutErr= */ eq(true)); + verify(remoteCache).downloadActionResult(any(), any(), /* inlineOutErr= */ eq(true)); assertThat(executionResult.exitCode()).isEqualTo(0); assertThat(executionResult.stdout()).isEqualTo(stdout); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java index c385e45738fd24..fc168ac9b99667 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java @@ -58,6 +58,7 @@ import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; import com.google.devtools.build.lib.exec.util.FakeOwner; import com.google.devtools.build.lib.remote.common.CacheNotFoundException; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey; import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.options.RemoteOutputsMode; @@ -235,7 +236,10 @@ public final void setUp() throws Exception { @Test public void cacheHit() throws Exception { ActionResult actionResult = ActionResult.getDefaultInstance(); - when(remoteCache.downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false))) + when(remoteCache.downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false))) .thenAnswer( new Answer() { @Override @@ -339,7 +343,10 @@ public void noCacheSpawns() throws Exception { simpleSpawnWithExecutionInfo(ImmutableMap.of(requirement, "")); CacheHandle entry = remoteSpawnCache.lookup(uncacheableSpawn, simplePolicy); verify(remoteCache, never()) - .downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false)); + .downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false)); assertThat(entry.hasResult()).isFalse(); SpawnResult result = new SpawnResult.Builder() @@ -372,7 +379,10 @@ public void noRemoteCacheSpawns_remoteCache() throws Exception { SimpleSpawn uncacheableSpawn = simpleSpawnWithExecutionInfo(ImmutableMap.of(requirement, "")); CacheHandle entry = remoteSpawnCache.lookup(uncacheableSpawn, simplePolicy); verify(remoteCache, never()) - .downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false)); + .downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false)); assertThat(entry.hasResult()).isFalse(); SpawnResult result = new SpawnResult.Builder() @@ -406,7 +416,10 @@ public void noRemoteCacheSpawns_combinedCache() throws Exception { SimpleSpawn uncacheableSpawn = simpleSpawnWithExecutionInfo(ImmutableMap.of(requirement, "")); CacheHandle entry = remoteSpawnCache.lookup(uncacheableSpawn, simplePolicy); verify(remoteCache, never()) - .downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false)); + .downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false)); assertThat(entry.hasResult()).isFalse(); SpawnResult result = new SpawnResult.Builder() @@ -429,7 +442,11 @@ public void noRemoteCacheStillUsesLocalCache() throws Exception { SimpleSpawn cacheableSpawn = simpleSpawnWithExecutionInfo(ImmutableMap.of(ExecutionRequirements.NO_REMOTE_CACHE, "")); cache.lookup(cacheableSpawn, simplePolicy); - verify(remoteCache).downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false)); + verify(remoteCache) + .downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false)); } @Test @@ -437,14 +454,22 @@ public void noRemoteExecStillUsesCache() throws Exception { SimpleSpawn cacheableSpawn = simpleSpawnWithExecutionInfo(ImmutableMap.of(ExecutionRequirements.NO_REMOTE_EXEC, "")); cache.lookup(cacheableSpawn, simplePolicy); - verify(remoteCache).downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false)); + verify(remoteCache) + .downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false)); } @Test public void failedActionsAreNotUploaded() throws Exception { // Only successful action results are uploaded to the remote cache. CacheHandle entry = cache.lookup(simpleSpawn, simplePolicy); - verify(remoteCache).downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false)); + verify(remoteCache) + .downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false)); assertThat(entry.hasResult()).isFalse(); SpawnResult result = new SpawnResult.Builder() @@ -514,7 +539,10 @@ public void printWarningIfUploadFails() throws Exception { public void printWarningIfDownloadFails() throws Exception { doThrow(new IOException(io.grpc.Status.UNAVAILABLE.asRuntimeException())) .when(remoteCache) - .downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false)); + .downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false)); CacheHandle entry = cache.lookup(simpleSpawn, simplePolicy); assertThat(entry.hasResult()).isFalse(); @@ -569,7 +597,10 @@ public void orphanedCachedResultIgnored() throws Exception { ActionResult.newBuilder() .addOutputFiles(OutputFile.newBuilder().setPath("/random/file").setDigest(digest)) .build(); - when(remoteCache.downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false))) + when(remoteCache.downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false))) .thenAnswer( new Answer() { @Override @@ -629,7 +660,10 @@ public Void answer(InvocationOnMock invocation) { @Test public void failedCacheActionAsCacheMiss() throws Exception { ActionResult actionResult = ActionResult.newBuilder().setExitCode(1).build(); - when(remoteCache.downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false))) + when(remoteCache.downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false))) .thenReturn(actionResult); CacheHandle entry = cache.lookup(simpleSpawn, simplePolicy); @@ -645,7 +679,8 @@ public void testDownloadMinimal() throws Exception { cache = remoteSpawnCacheWithOptions(remoteOptions); ActionResult success = ActionResult.newBuilder().setExitCode(0).build(); - when(remoteCache.downloadActionResult(any(), /* inlineOutErr= */ eq(false))) + when(remoteCache.downloadActionResult( + any(RemoteActionExecutionContext.class), any(), /* inlineOutErr= */ eq(false))) .thenReturn(success); // act @@ -668,7 +703,8 @@ public void testDownloadMinimalIoError() throws Exception { IOException downloadFailure = new IOException("downloadMinimal failed"); ActionResult success = ActionResult.newBuilder().setExitCode(0).build(); - when(remoteCache.downloadActionResult(any(), /* inlineOutErr= */ eq(false))) + when(remoteCache.downloadActionResult( + any(RemoteActionExecutionContext.class), any(), /* inlineOutErr= */ eq(false))) .thenReturn(success); when(remoteCache.downloadMinimal( any(), any(), anyCollection(), any(), any(), any(), any(), any())) diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java index ebb5d19ae9df4a..e6851d78392f2c 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java @@ -78,6 +78,7 @@ import com.google.devtools.build.lib.exec.util.FakeOwner; import com.google.devtools.build.lib.remote.common.CacheNotFoundException; import com.google.devtools.build.lib.remote.common.OperationObserver; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey; import com.google.devtools.build.lib.remote.common.RemoteExecutionClient; import com.google.devtools.build.lib.remote.options.RemoteOptions; @@ -206,7 +207,10 @@ public void nonCachableSpawnsShouldNotBeCached_remote() throws Exception { // TODO(olaola): verify that the uploaded action has the doNotCache set. verify(cache, never()) - .downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false)); + .downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false)); verify(cache, never()).upload(any(), any(), any(), any(), any(), any()); verifyNoMoreInteractions(localRunner); } @@ -320,7 +324,10 @@ public void treatFailedCachedActionAsCacheMiss_local() throws Exception { remoteOptions.remoteUploadLocalResults = true; ActionResult failedAction = ActionResult.newBuilder().setExitCode(1).build(); - when(cache.downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false))) + when(cache.downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false))) .thenReturn(failedAction); RemoteSpawnRunner runner = spy(newSpawnRunner()); @@ -355,7 +362,10 @@ public void treatFailedCachedActionAsCacheMiss_remote() throws Exception { // remotely ActionResult failedAction = ActionResult.newBuilder().setExitCode(1).build(); - when(cache.downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false))) + when(cache.downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false))) .thenReturn(failedAction); RemoteSpawnRunner runner = newSpawnRunner(); @@ -395,7 +405,10 @@ public void printWarningIfCacheIsDown() throws Exception { Spawn spawn = newSimpleSpawn(); SpawnExecutionContext policy = getSpawnContext(spawn); - when(cache.downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false))) + when(cache.downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false))) .thenThrow(new IOException("cache down")); doThrow(new IOException("cache down")) @@ -437,7 +450,10 @@ public void noRemoteExecutorFallbackFails() throws Exception { Spawn spawn = newSimpleSpawn(); SpawnExecutionContext policy = getSpawnContext(spawn); - when(cache.downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false))) + when(cache.downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false))) .thenReturn(null); IOException err = new IOException("local execution error"); @@ -464,7 +480,10 @@ public void remoteCacheErrorFallbackFails() throws Exception { Spawn spawn = newSimpleSpawn(); SpawnExecutionContext policy = getSpawnContext(spawn); - when(cache.downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false))) + when(cache.downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false))) .thenThrow(new IOException()); IOException err = new IOException("local execution error"); @@ -482,7 +501,10 @@ public void testLocalFallbackFailureRemoteExecutorFailure() throws Exception { RemoteSpawnRunner runner = newSpawnRunner(); - when(cache.downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false))) + when(cache.downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false))) .thenReturn(null); when(executor.executeRemotely(any(ExecuteRequest.class), any(OperationObserver.class))) .thenThrow(new IOException()); @@ -611,7 +633,10 @@ public void cacheDownloadFailureTriggersRemoteExecution() throws Exception { RemoteSpawnRunner runner = newSpawnRunner(); ActionResult cachedResult = ActionResult.newBuilder().setExitCode(0).build(); - when(cache.downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false))) + when(cache.downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false))) .thenReturn(cachedResult); Exception downloadFailure = new BulkTransferException(new CacheNotFoundException(Digest.getDefaultInstance())); @@ -642,7 +667,10 @@ public void resultsDownloadFailureTriggersRemoteExecutionWithSkipCacheLookup() t RemoteSpawnRunner runner = newSpawnRunner(); - when(cache.downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false))) + when(cache.downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false))) .thenReturn(null); ActionResult cachedResult = ActionResult.newBuilder().setExitCode(0).build(); ActionResult execResult = ActionResult.newBuilder().setExitCode(31).build(); @@ -686,7 +714,10 @@ public void testRemoteExecutionTimeout() throws Exception { RemoteSpawnRunner runner = newSpawnRunner(); ActionResult cachedResult = ActionResult.newBuilder().setExitCode(0).build(); - when(cache.downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false))) + when(cache.downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false))) .thenReturn(null); ExecuteResponse resp = ExecuteResponse.newBuilder() @@ -720,7 +751,10 @@ public void testRemoteExecutionTimeoutDoesNotTriggerFallback() throws Exception RemoteSpawnRunner runner = newSpawnRunner(); ActionResult cachedResult = ActionResult.newBuilder().setExitCode(0).build(); - when(cache.downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false))) + when(cache.downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false))) .thenReturn(null); ExecuteResponse resp = ExecuteResponse.newBuilder() @@ -752,7 +786,10 @@ public void testRemoteExecutionCommandFailureDoesNotTriggerFallback() throws Exc RemoteSpawnRunner runner = newSpawnRunner(); ActionResult cachedResult = ActionResult.newBuilder().setExitCode(0).build(); - when(cache.downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false))) + when(cache.downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false))) .thenReturn(null); ExecuteResponse failed = ExecuteResponse.newBuilder() @@ -783,7 +820,10 @@ public void testExitCode_executorfailure() throws Exception { RemoteSpawnRunner runner = newSpawnRunner(); - when(cache.downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false))) + when(cache.downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false))) .thenReturn(null); when(executor.executeRemotely(any(ExecuteRequest.class), any(OperationObserver.class))) .thenThrow(new IOException("reasons")); @@ -805,7 +845,10 @@ public void testExitCode_executionfailure() throws Exception { RemoteSpawnRunner runner = newSpawnRunner(); - when(cache.downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false))) + when(cache.downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false))) .thenThrow(new IOException("reasons")); Spawn spawn = newSimpleSpawn(); @@ -884,7 +927,10 @@ public void testDownloadMinimalOnCacheHit() throws Exception { remoteOptions.remoteOutputsMode = RemoteOutputsMode.MINIMAL; ActionResult succeededAction = ActionResult.newBuilder().setExitCode(0).build(); - when(cache.downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false))) + when(cache.downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false))) .thenReturn(succeededAction); RemoteSpawnRunner runner = newSpawnRunner(); @@ -938,7 +984,10 @@ public void testDownloadMinimalIoError() throws Exception { remoteOptions.remoteOutputsMode = RemoteOutputsMode.MINIMAL; ActionResult succeededAction = ActionResult.newBuilder().setExitCode(0).build(); - when(cache.downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false))) + when(cache.downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false))) .thenReturn(succeededAction); IOException downloadFailure = new IOException("downloadMinimal failed"); when(cache.downloadMinimal(any(), any(), anyCollection(), any(), any(), any(), any(), any())) @@ -971,7 +1020,10 @@ public void testDownloadTopLevel() throws Exception { ActionsTestUtil.createArtifact(outputRoot, outputRoot.getRoot().getRelative("foo.bin")); ActionResult succeededAction = ActionResult.newBuilder().setExitCode(0).build(); - when(cache.downloadActionResult(any(ActionKey.class), /* inlineOutErr= */ eq(false))) + when(cache.downloadActionResult( + any(RemoteActionExecutionContext.class), + any(ActionKey.class), + /* inlineOutErr= */ eq(false))) .thenReturn(succeededAction); RemoteSpawnRunner runner = newSpawnRunner(ImmutableSet.of(topLevelOutput)); diff --git a/src/test/java/com/google/devtools/build/lib/remote/util/InMemoryCacheClient.java b/src/test/java/com/google/devtools/build/lib/remote/util/InMemoryCacheClient.java index 7ba42e687aeb9a..63d0d5965b6aed 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/util/InMemoryCacheClient.java +++ b/src/test/java/com/google/devtools/build/lib/remote/util/InMemoryCacheClient.java @@ -20,6 +20,7 @@ import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.remote.common.CacheNotFoundException; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; import com.google.devtools.build.lib.remote.common.RemoteCacheClient; import com.google.devtools.build.lib.vfs.Path; import com.google.protobuf.ByteString; @@ -90,7 +91,7 @@ public ListenableFuture downloadBlob(Digest digest, OutputStream out) { @Override public ListenableFuture downloadActionResult( - ActionKey actionKey, boolean inlineOutErr) { + RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) { ActionResult actionResult = ac.get(actionKey); if (actionResult == null) { return Futures.immediateFailedFuture(new CacheNotFoundException(actionKey.getDigest())); diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ActionCacheServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ActionCacheServer.java index 35c67c46897f24..8eda8fc4e8dcaf 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ActionCacheServer.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ActionCacheServer.java @@ -18,10 +18,15 @@ import build.bazel.remote.execution.v2.ActionCacheGrpc.ActionCacheImplBase; import build.bazel.remote.execution.v2.ActionResult; import build.bazel.remote.execution.v2.GetActionResultRequest; +import build.bazel.remote.execution.v2.RequestMetadata; import build.bazel.remote.execution.v2.UpdateActionResultRequest; import com.google.common.flogger.GoogleLogger; +import com.google.devtools.build.lib.remote.common.NetworkTime; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContextImpl; import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey; import com.google.devtools.build.lib.remote.util.DigestUtil; +import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import io.grpc.stub.StreamObserver; /** A basic implementation of an {@link ActionCacheImplBase} service. */ @@ -40,8 +45,13 @@ public ActionCacheServer(OnDiskBlobStoreCache cache, DigestUtil digestUtil) { public void getActionResult( GetActionResultRequest request, StreamObserver responseObserver) { try { + RequestMetadata requestMetadata = TracingMetadataUtils.fromCurrentContext(); + RemoteActionExecutionContext context = + new RemoteActionExecutionContextImpl(requestMetadata, new NetworkTime()); + ActionKey actionKey = digestUtil.asActionKey(request.getActionDigest()); - ActionResult result = cache.downloadActionResult(actionKey, /* inlineOutErr= */ false); + ActionResult result = + cache.downloadActionResult(context, actionKey, /* inlineOutErr= */ false); if (result == null) { responseObserver.onError(StatusUtils.notFoundError(request.getActionDigest()));