Remote: Fix a bug that outputs of actions tagged with no-remote are u…
…ploaded to remote cache when remote execution is enabled.
Fixes https://github.com/bazelbuild/bazel/issues/14900.
Also fixes an issue that action result from just remotely executed action is not saved to disk cache. The root cause is the action result is inlined in the execution response hence not downloaded through remote cache, hence not saved to disk cache. This results in the second build misses the disk cache, but it can still hit the remote cache and fill the disk cache. The third build can hit disk cache.
Closes #15212.
PiperOrigin-RevId: 441426469
diff --git a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java
index 76e755b..693aca6 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java
@@ -30,6 +30,7 @@
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext;
+import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext.Step;
import com.google.devtools.build.lib.remote.util.DigestUtil;
import com.google.devtools.build.lib.remote.util.TracingMetadataUtils;
import com.google.devtools.build.lib.vfs.Path;
@@ -256,7 +257,8 @@
RequestMetadata metadata =
TracingMetadataUtils.buildMetadata(buildRequestId, commandId, "bes-upload", null);
- RemoteActionExecutionContext context = RemoteActionExecutionContext.createForBES(metadata);
+ RemoteActionExecutionContext context = RemoteActionExecutionContext.create(metadata);
+ context.setStep(Step.UPLOAD_BES_FILES);
return Single.using(
remoteCache::retain,
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteAction.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteAction.java
new file mode 100644
index 0000000..e680ea4
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteAction.java
@@ -0,0 +1,134 @@
+// Copyright 2022 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;
+
+import build.bazel.remote.execution.v2.Action;
+import build.bazel.remote.execution.v2.Command;
+import build.bazel.remote.execution.v2.Digest;
+import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.ForbiddenActionInputException;
+import com.google.devtools.build.lib.actions.Spawn;
+import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
+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.RemoteCacheClient.ActionKey;
+import com.google.devtools.build.lib.remote.common.RemotePathResolver;
+import com.google.devtools.build.lib.remote.merkletree.MerkleTree;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import java.io.IOException;
+import java.util.SortedMap;
+
+/** A value class representing an action which can be executed remotely. */
+public class RemoteAction {
+
+ private final Spawn spawn;
+ private final SpawnExecutionContext spawnExecutionContext;
+ private final RemoteActionExecutionContext remoteActionExecutionContext;
+ private final RemotePathResolver remotePathResolver;
+ private final MerkleTree merkleTree;
+ private final Digest commandHash;
+ private final Command command;
+ private final Action action;
+ private final ActionKey actionKey;
+
+ RemoteAction(
+ Spawn spawn,
+ SpawnExecutionContext spawnExecutionContext,
+ RemoteActionExecutionContext remoteActionExecutionContext,
+ RemotePathResolver remotePathResolver,
+ MerkleTree merkleTree,
+ Digest commandHash,
+ Command command,
+ Action action,
+ ActionKey actionKey) {
+ this.spawn = spawn;
+ this.spawnExecutionContext = spawnExecutionContext;
+ this.remoteActionExecutionContext = remoteActionExecutionContext;
+ this.remotePathResolver = remotePathResolver;
+ this.merkleTree = merkleTree;
+ this.commandHash = commandHash;
+ this.command = command;
+ this.action = action;
+ this.actionKey = actionKey;
+ }
+
+ public RemoteActionExecutionContext getRemoteActionExecutionContext() {
+ return remoteActionExecutionContext;
+ }
+
+ public SpawnExecutionContext getSpawnExecutionContext() {
+ return spawnExecutionContext;
+ }
+
+ /** Returns the {@link Spawn} that owns this action. */
+ public Spawn getSpawn() {
+ return spawn;
+ }
+
+ /**
+ * Returns the sum of file sizes plus protobuf sizes used to represent the inputs of this action.
+ */
+ public long getInputBytes() {
+ return merkleTree.getInputBytes();
+ }
+
+ /** Returns the number of input files of this action. */
+ public long getInputFiles() {
+ return merkleTree.getInputFiles();
+ }
+
+ /** Returns the id this is action. */
+ public String getActionId() {
+ return actionKey.getDigest().getHash();
+ }
+
+ /** Returns the {@link ActionKey} of this action. */
+ public ActionKey getActionKey() {
+ return actionKey;
+ }
+
+ /** Returns underlying {@link Action} of this remote action. */
+ public Action getAction() {
+ return action;
+ }
+
+ public Digest getCommandHash() {
+ return commandHash;
+ }
+
+ public Command getCommand() {
+ return command;
+ }
+
+ public MerkleTree getMerkleTree() {
+ return merkleTree;
+ }
+
+ /**
+ * Returns a {@link SortedMap} which maps from input paths for remote action to {@link
+ * ActionInput}.
+ */
+ public SortedMap<PathFragment, ActionInput> getInputMap()
+ throws IOException, ForbiddenActionInputException {
+ return remotePathResolver.getInputMapping(spawnExecutionContext);
+ }
+
+ /**
+ * Returns the {@link NetworkTime} instance used to measure the network time during the action
+ * execution.
+ */
+ public NetworkTime getNetworkTime() {
+ return remoteActionExecutionContext.getNetworkTime();
+ }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
index d00110a..7a233df 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
@@ -67,7 +67,6 @@
import com.google.common.eventbus.Subscribe;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
-import com.google.devtools.build.lib.actions.ActionExecutionMetadata;
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
@@ -96,10 +95,10 @@
import com.google.devtools.build.lib.remote.RemoteExecutionService.ActionResultMetadata.FileMetadata;
import com.google.devtools.build.lib.remote.RemoteExecutionService.ActionResultMetadata.SymlinkMetadata;
import com.google.devtools.build.lib.remote.common.BulkTransferException;
-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.OutputDigestMismatchException;
import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext;
+import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext.Step;
import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey;
import com.google.devtools.build.lib.remote.common.RemoteCacheClient.CachedActionResult;
import com.google.devtools.build.lib.remote.common.RemoteExecutionClient;
@@ -254,89 +253,6 @@
return command.build();
}
- /** A value class representing an action which can be executed remotely. */
- public static class RemoteAction {
- private final Spawn spawn;
- private final SpawnExecutionContext spawnExecutionContext;
- private final RemoteActionExecutionContext remoteActionExecutionContext;
- private final RemotePathResolver remotePathResolver;
- private final MerkleTree merkleTree;
- private final Digest commandHash;
- private final Command command;
- private final Action action;
- private final ActionKey actionKey;
-
- RemoteAction(
- Spawn spawn,
- SpawnExecutionContext spawnExecutionContext,
- RemoteActionExecutionContext remoteActionExecutionContext,
- RemotePathResolver remotePathResolver,
- MerkleTree merkleTree,
- Digest commandHash,
- Command command,
- Action action,
- ActionKey actionKey) {
- this.spawn = spawn;
- this.spawnExecutionContext = spawnExecutionContext;
- this.remoteActionExecutionContext = remoteActionExecutionContext;
- this.remotePathResolver = remotePathResolver;
- this.merkleTree = merkleTree;
- this.commandHash = commandHash;
- this.command = command;
- this.action = action;
- this.actionKey = actionKey;
- }
-
- public RemoteActionExecutionContext getRemoteActionExecutionContext() {
- return remoteActionExecutionContext;
- }
-
- /** Returns the {@link ActionExecutionMetadata} that owns this action. */
- public ActionExecutionMetadata getOwner() {
- return spawn.getResourceOwner();
- }
-
- /**
- * Returns the sum of file sizes plus protobuf sizes used to represent the inputs of this
- * action.
- */
- public long getInputBytes() {
- return merkleTree.getInputBytes();
- }
-
- /** Returns the number of input files of this action. */
- public long getInputFiles() {
- return merkleTree.getInputFiles();
- }
-
- /** Returns the id this is action. */
- public String getActionId() {
- return actionKey.getDigest().getHash();
- }
-
- /** Returns underlying {@link Action} of this remote action. */
- public Action getAction() {
- return action;
- }
-
- /**
- * Returns a {@link SortedMap} which maps from input paths for remote action to {@link
- * ActionInput}.
- */
- public SortedMap<PathFragment, ActionInput> getInputMap()
- throws IOException, ForbiddenActionInputException {
- return remotePathResolver.getInputMapping(spawnExecutionContext);
- }
-
- /**
- * Returns the {@link NetworkTime} instance used to measure the network time during the action
- * execution.
- */
- public NetworkTime getNetworkTime() {
- return remoteActionExecutionContext.getNetworkTime();
- }
- }
-
private static boolean useRemoteCache(RemoteOptions options) {
return !isNullOrEmpty(options.remoteCache) || !isNullOrEmpty(options.remoteExecutor);
}
@@ -615,11 +531,15 @@
@Nullable
public RemoteActionResult lookupCache(RemoteAction action)
throws IOException, InterruptedException {
- checkState(shouldAcceptCachedResult(action.spawn), "spawn doesn't accept cached result");
+ checkState(shouldAcceptCachedResult(action.getSpawn()), "spawn doesn't accept cached result");
+
+ action.getRemoteActionExecutionContext().setStep(Step.CHECK_ACTION_CACHE);
CachedActionResult cachedActionResult =
remoteCache.downloadActionResult(
- action.remoteActionExecutionContext, action.actionKey, /* inlineOutErr= */ false);
+ action.getRemoteActionExecutionContext(),
+ action.getActionKey(),
+ /* inlineOutErr= */ false);
if (cachedActionResult == null) {
return null;
@@ -639,12 +559,12 @@
try {
ListenableFuture<Void> future =
remoteCache.downloadFile(
- action.remoteActionExecutionContext,
+ action.getRemoteActionExecutionContext(),
remotePathResolver.localPathToOutputPath(file.path()),
toTmpDownloadPath(file.path()),
file.digest(),
new RemoteCache.DownloadProgressReporter(
- action.spawnExecutionContext::report,
+ action.getSpawnExecutionContext()::report,
remotePathResolver.localPathToOutputPath(file.path()),
file.digest().getSizeBytes()));
return transform(future, (d) -> file, directExecutor());
@@ -763,8 +683,8 @@
private void injectRemoteArtifact(
RemoteAction action, Artifact output, ActionResultMetadata metadata) throws IOException {
- RemoteActionExecutionContext context = action.remoteActionExecutionContext;
- MetadataInjector metadataInjector = action.spawnExecutionContext.getMetadataInjector();
+ RemoteActionExecutionContext context = action.getRemoteActionExecutionContext();
+ MetadataInjector metadataInjector = action.getSpawnExecutionContext().getMetadataInjector();
Path path = remotePathResolver.outputPathToLocalPath(output);
if (output.isTreeArtifact()) {
DirectoryMetadata directory = metadata.directory(path);
@@ -947,7 +867,8 @@
dirMetadataDownloads.put(
remotePathResolver.outputPathToLocalPath(dir.getPath()),
Futures.transformAsync(
- remoteCache.downloadBlob(action.remoteActionExecutionContext, dir.getTreeDigest()),
+ remoteCache.downloadBlob(
+ action.getRemoteActionExecutionContext(), dir.getTreeDigest()),
(treeBytes) ->
immediateFuture(Tree.parseFrom(treeBytes, ExtensionRegistry.getEmptyRegistry())),
directExecutor()));
@@ -1006,6 +927,8 @@
checkState(!shutdown.get(), "shutdown");
checkNotNull(remoteCache, "remoteCache can't be null");
+ action.getRemoteActionExecutionContext().setStep(Step.DOWNLOAD_OUTPUTS);
+
ActionResultMetadata metadata;
try (SilentCloseable c = Profiler.instance().profile("Remote.parseActionResultMetadata")) {
metadata = parseActionResultMetadata(action, result);
@@ -1013,8 +936,8 @@
if (result.success()) {
// Check that all mandatory outputs are created.
- for (ActionInput output : action.spawn.getOutputFiles()) {
- if (action.spawn.isMandatoryOutput(output)) {
+ for (ActionInput output : action.getSpawn().getOutputFiles()) {
+ if (action.getSpawn().isMandatoryOutput(output)) {
// Don't check output that is tree artifact since spawn could generate nothing under that
// directory. Remote server typically doesn't create directory ahead of time resulting in
// empty tree artifact missing from action cache entry.
@@ -1028,16 +951,28 @@
&& !metadata.symlinks.containsKey(localPath)) {
throw new IOException(
"Invalid action cache entry "
- + action.actionKey.getDigest().getHash()
+ + action.getActionKey().getDigest().getHash()
+ ": expected output "
+ prettyPrint(output)
+ " does not exist.");
}
}
}
+
+ // When downloading outputs from just remotely executed action, the action result comes from
+ // Execution response which means, if disk cache is enabled, action result hasn't been
+ // uploaded to it. Upload action result to disk cache here so next build could hit it.
+ if (useDiskCache(remoteOptions)
+ && action.getRemoteActionExecutionContext().getExecuteResponse() != null) {
+ getFromFuture(
+ remoteCache.uploadActionResult(
+ action.getRemoteActionExecutionContext(),
+ action.getActionKey(),
+ result.actionResult));
+ }
}
- FileOutErr outErr = action.spawnExecutionContext.getFileOutErr();
+ FileOutErr outErr = action.getSpawnExecutionContext().getFileOutErr();
ImmutableList.Builder<ListenableFuture<FileMetadata>> downloadsBuilder =
ImmutableList.builder();
@@ -1046,7 +981,7 @@
shouldDownloadAllSpawnOutputs(
remoteOutputsMode,
/* exitCode = */ result.getExitCode(),
- hasFilesToDownload(action.spawn.getOutputFiles(), filesToDownload));
+ hasFilesToDownload(action.getSpawn().getOutputFiles(), filesToDownload));
if (downloadOutputs) {
for (FileMetadata file : metadata.files()) {
@@ -1073,7 +1008,7 @@
FileOutErr tmpOutErr = outErr.childOutErr();
List<ListenableFuture<Void>> outErrDownloads =
remoteCache.downloadOutErr(
- action.remoteActionExecutionContext, result.actionResult, tmpOutErr);
+ action.getRemoteActionExecutionContext(), result.actionResult, tmpOutErr);
for (ListenableFuture<Void> future : outErrDownloads) {
downloadsBuilder.add(transform(future, (v) -> null, directExecutor()));
}
@@ -1091,8 +1026,9 @@
// Ensure that we are the only ones writing to the output files when using the dynamic spawn
// strategy.
- action.spawnExecutionContext.lockOutputFiles(
- result.getExitCode(), result.getMessage(), tmpOutErr);
+ action
+ .getSpawnExecutionContext()
+ .lockOutputFiles(result.getExitCode(), result.getMessage(), tmpOutErr);
// Will these be properly garbage-collected if the above throws an exception?
tmpOutErr.clearOut();
tmpOutErr.clearErr();
@@ -1116,9 +1052,9 @@
} else {
ActionInput inMemoryOutput = null;
Digest inMemoryOutputDigest = null;
- PathFragment inMemoryOutputPath = getInMemoryOutputPath(action.spawn);
+ PathFragment inMemoryOutputPath = getInMemoryOutputPath(action.getSpawn());
- for (ActionInput output : action.spawn.getOutputFiles()) {
+ for (ActionInput output : action.getSpawn().getOutputFiles()) {
if (inMemoryOutputPath != null && output.getExecPath().equals(inMemoryOutputPath)) {
Path localPath = remotePathResolver.outputPathToLocalPath(output);
FileMetadata m = metadata.file(localPath);
@@ -1138,7 +1074,8 @@
try (SilentCloseable c = Profiler.instance().profile("Remote.downloadInMemoryOutput")) {
if (inMemoryOutput != null) {
ListenableFuture<byte[]> inMemoryOutputDownload =
- remoteCache.downloadBlob(action.remoteActionExecutionContext, inMemoryOutputDigest);
+ remoteCache.downloadBlob(
+ action.getRemoteActionExecutionContext(), inMemoryOutputDigest);
waitForBulkTransfer(
ImmutableList.of(inMemoryOutputDownload), /* cancelRemainingOnInterrupt=*/ true);
byte[] data = getFromFuture(inMemoryOutputDownload);
@@ -1164,9 +1101,9 @@
() -> {
ImmutableList.Builder<Path> outputFiles = ImmutableList.builder();
// Check that all mandatory outputs are created.
- for (ActionInput outputFile : action.spawn.getOutputFiles()) {
+ for (ActionInput outputFile : action.getSpawn().getOutputFiles()) {
Path localPath = execRoot.getRelative(outputFile.getExecPath());
- if (action.spawn.isMandatoryOutput(outputFile) && !localPath.exists()) {
+ if (action.getSpawn().isMandatoryOutput(outputFile) && !localPath.exists()) {
throw new IOException(
"Expected output " + prettyPrint(outputFile) + " was not created locally.");
}
@@ -1177,11 +1114,11 @@
remoteOptions,
digestUtil,
remotePathResolver,
- action.actionKey,
- action.action,
- action.command,
+ action.getActionKey(),
+ action.getAction(),
+ action.getCommand(),
outputFiles.build(),
- action.spawnExecutionContext.getFileOutErr(),
+ action.getSpawnExecutionContext().getFileOutErr(),
spawnResult.exitCode());
});
}
@@ -1206,7 +1143,7 @@
public void uploadOutputs(RemoteAction action, SpawnResult spawnResult)
throws InterruptedException, ExecException {
checkState(!shutdown.get(), "shutdown");
- checkState(shouldUploadLocalResults(action.spawn), "spawn shouldn't upload local result");
+ checkState(shouldUploadLocalResults(action.getSpawn()), "spawn shouldn't upload local result");
checkState(
SpawnResult.Status.SUCCESS.equals(spawnResult.status()) && spawnResult.exitCode() == 0,
"shouldn't upload outputs of failed local action");
@@ -1270,15 +1207,17 @@
public void uploadInputsIfNotPresent(RemoteAction action, boolean force)
throws IOException, InterruptedException {
checkState(!shutdown.get(), "shutdown");
- checkState(mayBeExecutedRemotely(action.spawn), "spawn can't be executed remotely");
+ checkState(mayBeExecutedRemotely(action.getSpawn()), "spawn can't be executed remotely");
+
+ action.getRemoteActionExecutionContext().setStep(Step.UPLOAD_INPUTS);
RemoteExecutionCache remoteExecutionCache = (RemoteExecutionCache) remoteCache;
// Upload the command and all the inputs into the remote cache.
Map<Digest, Message> additionalInputs = Maps.newHashMapWithExpectedSize(2);
- additionalInputs.put(action.actionKey.getDigest(), action.action);
- additionalInputs.put(action.commandHash, action.command);
+ additionalInputs.put(action.getActionKey().getDigest(), action.getAction());
+ additionalInputs.put(action.getCommandHash(), action.getCommand());
remoteExecutionCache.ensureInputsPresent(
- action.remoteActionExecutionContext, action.merkleTree, additionalInputs, force);
+ action.getRemoteActionExecutionContext(), action.getMerkleTree(), additionalInputs, force);
}
/**
@@ -1291,12 +1230,14 @@
RemoteAction action, boolean acceptCachedResult, OperationObserver observer)
throws IOException, InterruptedException {
checkState(!shutdown.get(), "shutdown");
- checkState(mayBeExecutedRemotely(action.spawn), "spawn can't be executed remotely");
+ checkState(mayBeExecutedRemotely(action.getSpawn()), "spawn can't be executed remotely");
+
+ action.getRemoteActionExecutionContext().setStep(Step.EXECUTE_REMOTELY);
ExecuteRequest.Builder requestBuilder =
ExecuteRequest.newBuilder()
.setInstanceName(remoteOptions.remoteInstanceName)
- .setActionDigest(action.actionKey.getDigest())
+ .setActionDigest(action.getActionKey().getDigest())
.setSkipCacheLookup(!acceptCachedResult);
if (remoteOptions.remoteResultCachePriority != 0) {
requestBuilder
@@ -1310,7 +1251,9 @@
ExecuteRequest request = requestBuilder.build();
ExecuteResponse reply =
- remoteExecutor.executeRemotely(action.remoteActionExecutionContext, request, observer);
+ remoteExecutor.executeRemotely(action.getRemoteActionExecutionContext(), request, observer);
+
+ action.getRemoteActionExecutionContext().setExecuteResponse(reply);
return RemoteActionResult.createFromResponse(reply);
}
@@ -1339,7 +1282,7 @@
serverLogs.logCount++;
getFromFuture(
remoteCache.downloadFile(
- action.remoteActionExecutionContext,
+ action.getRemoteActionExecutionContext(),
serverLogs.lastLogPath,
e.getValue().getDigest()));
}
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 3c13fcc..691cd23 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
@@ -38,7 +38,6 @@
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.RemoteExecutionService.RemoteAction;
import com.google.devtools.build.lib.remote.RemoteExecutionService.RemoteActionResult;
import com.google.devtools.build.lib.remote.common.BulkTransferException;
import com.google.devtools.build.lib.remote.common.CacheNotFoundException;
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 c8e6fec..f1160eb 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
@@ -51,7 +51,6 @@
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.RemoteExecutionService.RemoteAction;
import com.google.devtools.build.lib.remote.RemoteExecutionService.RemoteActionResult;
import com.google.devtools.build.lib.remote.RemoteExecutionService.ServerLogs;
import com.google.devtools.build.lib.remote.common.BulkTransferException;
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
index c27eeb9..78cac39 100644
--- 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
@@ -13,37 +13,71 @@
// limitations under the License.
package com.google.devtools.build.lib.remote.common;
+import build.bazel.remote.execution.v2.ExecuteResponse;
import build.bazel.remote.execution.v2.RequestMetadata;
import com.google.devtools.build.lib.actions.ActionExecutionMetadata;
import com.google.devtools.build.lib.actions.Spawn;
import javax.annotation.Nullable;
/** A context that provide remote execution related information for executing an action remotely. */
-public interface RemoteActionExecutionContext {
- /** The type of the context. */
- enum Type {
- REMOTE_EXECUTION,
- BUILD_EVENT_SERVICE,
+public class RemoteActionExecutionContext {
+ /** The current step of the context. */
+ public enum Step {
+ INIT,
+ CHECK_ACTION_CACHE,
+ UPLOAD_INPUTS,
+ EXECUTE_REMOTELY,
+ UPLOAD_OUTPUTS,
+ DOWNLOAD_OUTPUTS,
+ UPLOAD_BES_FILES,
}
- /** Returns the {@link Type} of the context. */
- Type getType();
+ @Nullable private final Spawn spawn;
+ private final RequestMetadata requestMetadata;
+ private final NetworkTime networkTime;
+
+ @Nullable private ExecuteResponse executeResponse;
+ private Step step;
+
+ private RemoteActionExecutionContext(
+ @Nullable Spawn spawn, RequestMetadata requestMetadata, NetworkTime networkTime) {
+ this.spawn = spawn;
+ this.requestMetadata = requestMetadata;
+ this.networkTime = networkTime;
+ this.step = Step.INIT;
+ }
+
+ /** Returns current {@link Step} of the context. */
+ public Step getStep() {
+ return step;
+ }
+
+ /** Sets current {@link Step} of the context. */
+ public void setStep(Step step) {
+ this.step = step;
+ }
/** Returns the {@link Spawn} of the action being executed or {@code null}. */
@Nullable
- Spawn getSpawn();
+ public Spawn getSpawn() {
+ return spawn;
+ }
/** Returns the {@link RequestMetadata} for the action being executed. */
- RequestMetadata getRequestMetadata();
+ public RequestMetadata getRequestMetadata() {
+ return requestMetadata;
+ }
/**
* Returns the {@link NetworkTime} instance used to measure the network time during the action
* execution.
*/
- NetworkTime getNetworkTime();
+ public NetworkTime getNetworkTime() {
+ return networkTime;
+ }
@Nullable
- default ActionExecutionMetadata getSpawnOwner() {
+ public ActionExecutionMetadata getSpawnOwner() {
Spawn spawn = getSpawn();
if (spawn == null) {
return null;
@@ -52,23 +86,26 @@
return spawn.getResourceOwner();
}
- /** Creates a {@link SimpleRemoteActionExecutionContext} with given {@link RequestMetadata}. */
- static RemoteActionExecutionContext create(RequestMetadata metadata) {
- return new SimpleRemoteActionExecutionContext(
- /*type=*/ Type.REMOTE_EXECUTION, /*spawn=*/ null, metadata, new NetworkTime());
+ public void setExecuteResponse(@Nullable ExecuteResponse executeResponse) {
+ this.executeResponse = executeResponse;
+ }
+
+ @Nullable
+ public ExecuteResponse getExecuteResponse() {
+ return executeResponse;
+ }
+
+ /** Creates a {@link RemoteActionExecutionContext} with given {@link RequestMetadata}. */
+ public static RemoteActionExecutionContext create(RequestMetadata metadata) {
+ return new RemoteActionExecutionContext(/*spawn=*/ null, metadata, new NetworkTime());
}
/**
- * Creates a {@link SimpleRemoteActionExecutionContext} with given {@link Spawn} and {@link
+ * Creates a {@link RemoteActionExecutionContext} with given {@link Spawn} and {@link
* RequestMetadata}.
*/
- static RemoteActionExecutionContext create(@Nullable Spawn spawn, RequestMetadata metadata) {
- return new SimpleRemoteActionExecutionContext(
- /*type=*/ Type.REMOTE_EXECUTION, spawn, metadata, new NetworkTime());
- }
-
- static RemoteActionExecutionContext createForBES(RequestMetadata metadata) {
- return new SimpleRemoteActionExecutionContext(
- /*type=*/ Type.BUILD_EVENT_SERVICE, /*spawn=*/ null, metadata, new NetworkTime());
+ public static RemoteActionExecutionContext create(
+ @Nullable Spawn spawn, RequestMetadata metadata) {
+ return new RemoteActionExecutionContext(spawn, metadata, new NetworkTime());
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/SimpleRemoteActionExecutionContext.java b/src/main/java/com/google/devtools/build/lib/remote/common/SimpleRemoteActionExecutionContext.java
deleted file mode 100644
index 1b09945..0000000
--- a/src/main/java/com/google/devtools/build/lib/remote/common/SimpleRemoteActionExecutionContext.java
+++ /dev/null
@@ -1,56 +0,0 @@
-// 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;
-import com.google.devtools.build.lib.actions.Spawn;
-import javax.annotation.Nullable;
-
-/** A {@link RemoteActionExecutionContext} implementation */
-public class SimpleRemoteActionExecutionContext implements RemoteActionExecutionContext {
-
- private final Type type;
- private final Spawn spawn;
- private final RequestMetadata requestMetadata;
- private final NetworkTime networkTime;
-
- public SimpleRemoteActionExecutionContext(
- Type type, Spawn spawn, RequestMetadata requestMetadata, NetworkTime networkTime) {
- this.type = type;
- this.spawn = spawn;
- this.requestMetadata = requestMetadata;
- this.networkTime = networkTime;
- }
-
- @Override
- public Type getType() {
- return type;
- }
-
- @Nullable
- @Override
- public Spawn getSpawn() {
- return spawn;
- }
-
- @Override
- public RequestMetadata getRequestMetadata() {
- return requestMetadata;
- }
-
- @Override
- public NetworkTime getNetworkTime() {
- return networkTime;
- }
-}
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 e5a936c..c337a5e 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
@@ -25,6 +25,7 @@
import com.google.common.util.concurrent.ListenableFuture;
import com.google.devtools.build.lib.remote.common.LazyFileOutputStream;
import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext;
+import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext.Step;
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;
@@ -55,7 +56,11 @@
public ListenableFuture<Void> uploadActionResult(
RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) {
ListenableFuture<Void> future = diskCache.uploadActionResult(context, actionKey, actionResult);
- if (shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) {
+ // Only upload action result to remote cache if we are uploading local outputs. This method
+ // could be called when we are downloading outputs from remote executor if disk cache is enabled
+ // because we want to upload the action result to it.
+ if (context.getStep() == Step.UPLOAD_OUTPUTS
+ && shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) {
future =
Futures.transformAsync(
future,
@@ -74,15 +79,20 @@
@Override
public ListenableFuture<Void> uploadFile(
RemoteActionExecutionContext context, Digest digest, Path file) {
- // For BES upload, we only upload to the remote cache.
- if (context.getType() == RemoteActionExecutionContext.Type.BUILD_EVENT_SERVICE) {
+ RemoteActionExecutionContext.Step step = context.getStep();
+
+ // For UPLOAD_INPUTS, only upload to remote cache.
+ if (step == Step.UPLOAD_INPUTS) {
+ return remoteCache.uploadFile(context, digest, file);
+ }
+
+ // For UPLOAD_BES_FILES, only upload to remote cache.
+ if (step == Step.UPLOAD_BES_FILES) {
return remoteCache.uploadFile(context, digest, file);
}
ListenableFuture<Void> future = diskCache.uploadFile(context, digest, file);
-
- if (options.isRemoteExecutionEnabled()
- || shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) {
+ if (shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) {
future =
Futures.transformAsync(
future, v -> remoteCache.uploadFile(context, digest, file), directExecutor());
@@ -93,9 +103,20 @@
@Override
public ListenableFuture<Void> uploadBlob(
RemoteActionExecutionContext context, Digest digest, ByteString data) {
+ RemoteActionExecutionContext.Step step = context.getStep();
+
+ // For UPLOAD_INPUTS, only upload to remote cache.
+ if (step == Step.UPLOAD_INPUTS) {
+ return remoteCache.uploadBlob(context, digest, data);
+ }
+
+ // For BES upload, only upload to the remote cache.
+ if (step == Step.UPLOAD_BES_FILES) {
+ return remoteCache.uploadBlob(context, digest, data);
+ }
+
ListenableFuture<Void> future = diskCache.uploadBlob(context, digest, data);
- if (options.isRemoteExecutionEnabled()
- || shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) {
+ if (shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) {
future =
Futures.transformAsync(
future, v -> remoteCache.uploadBlob(context, digest, data), directExecutor());
@@ -106,17 +127,19 @@
@Override
public ListenableFuture<ImmutableSet<Digest>> findMissingDigests(
RemoteActionExecutionContext context, Iterable<Digest> digests) {
- // If remote execution, find missing digests should only look at
+ RemoteActionExecutionContext.Step step = context.getStep();
+
+ // For UPLOAD_INPUTS, find missing digests should only look at
// the remote cache, not the disk cache because the remote executor only
// has access to the remote cache, not the disk cache.
// Also, the DiskCache always returns all digests as missing
// and we don't want to transfer all the files all the time.
- if (options.isRemoteExecutionEnabled()) {
+ if (step == Step.UPLOAD_INPUTS) {
return remoteCache.findMissingDigests(context, digests);
}
- // For BES upload, we only check the remote cache.
- if (context.getType() == RemoteActionExecutionContext.Type.BUILD_EVENT_SERVICE) {
+ // For UPLOAD_BES_FILES, we only check the remote cache.
+ if (step == Step.UPLOAD_BES_FILES) {
return remoteCache.findMissingDigests(context, digests);
}
@@ -169,7 +192,8 @@
final OutputStream tempOut;
tempOut = new LazyFileOutputStream(tempPath);
- if (options.isRemoteExecutionEnabled()
+ // Always download outputs for just remotely executed action.
+ if (context.getExecuteResponse() != null
|| shouldAcceptCachedResultFromRemoteCache(options, context.getSpawn())) {
ListenableFuture<Void> download =
closeStreamOnError(remoteCache.downloadBlob(context, digest, tempOut), tempOut);
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
index f57a5a6..e0cda7d 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
@@ -80,7 +80,6 @@
import com.google.devtools.build.lib.events.StoredEventHandler;
import com.google.devtools.build.lib.exec.util.FakeOwner;
import com.google.devtools.build.lib.exec.util.SpawnBuilder;
-import com.google.devtools.build.lib.remote.RemoteExecutionService.RemoteAction;
import com.google.devtools.build.lib.remote.RemoteExecutionService.RemoteActionResult;
import com.google.devtools.build.lib.remote.common.BulkTransferException;
import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext;
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 c46e05d..4503118 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
@@ -64,7 +64,6 @@
import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus;
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.RemoteExecutionService.RemoteAction;
import com.google.devtools.build.lib.remote.RemoteExecutionService.RemoteActionResult;
import com.google.devtools.build.lib.remote.common.CacheNotFoundException;
import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext;
diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
index ae03c48..de06eb5 100755
--- a/src/test/shell/bazel/remote/remote_execution_test.sh
+++ b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -2198,7 +2198,6 @@
# Should be some disk cache hits, just not remote.
"--noremote_accept_cached --incompatible_remote_results_ignore_disk"
)
- #
for flags in "${testcases[@]}"; do
genrule_combined_disk_remote_exec "$flags"
@@ -2222,15 +2221,13 @@
# if exist in disk cache or remote cache, don't run remote exec, don't update caches.
# [CASE]disk_cache, remote_cache: remote_exec, disk_cache, remote_cache
- # 1) notexist notexist run OK - , update
+ # 1) notexist notexist run OK update, update
# 2) notexist exist no run update, no update
# 3) exist notexist no run no update, no update
# 4) exist exist no run no update, no update
# 5) another rule that depends on 4, but run before 5
# Our setup ensures the first 2 columns, our validation checks the last 3.
- # NOTE that remote_exec will NOT update the disk cache, we expect the remote
- # execution to update the remote_cache and when we pull from the remote cache
- # we will then mirror to the disk cache.
+ # NOTE that remote_exec will UPDATE the disk cache.
#
# We measure if it was run remotely via the "1 remote." in the output and caches
# from the cache hit on the same line.
@@ -2260,7 +2257,7 @@
echo "INFO: RUNNING testcase($testcase_flags)"
# Case 1)
# disk_cache, remote_cache: remote_exec, disk_cache, remote_cache
- # notexist notexist run OK - , update
+ # notexist notexist run OK update, update
#
# Do a build to populate the disk and remote cache.
# Then clean and do another build to validate nothing updates.
@@ -2275,10 +2272,13 @@
disk_action_cache_files="$(count_disk_ac_files "$cache")"
remote_action_cache_files="$(count_remote_ac_files)"
- [[ "$disk_action_cache_files" == 0 ]] || fail "Expected 0 disk action cache entries, not $disk_action_cache_files"
+ [[ "$disk_action_cache_files" == 1 ]] || fail "CASE 1: Expected 1 disk action cache entries, not $disk_action_cache_files"
# Even though bazel isn't writing the remote action cache, we expect the worker to write one or the
# the rest of our tests will fail.
- [[ "$remote_action_cache_files" == 1 ]] || fail "Expected 1 remote action cache entries, not $remote_action_cache_files"
+ [[ "$remote_action_cache_files" == 1 ]] || fail "CASE 1: Expected 1 remote action cache entries, not $remote_action_cache_files"
+
+ rm -rf $cache
+ mkdir $cache
# Case 2)
# disk_cache, remote_cache: remote_exec, disk_cache, remote_cache
@@ -2295,10 +2295,8 @@
# ensure disk and remote cache populated
disk_action_cache_files="$(count_disk_ac_files "$cache")"
remote_action_cache_files="$(count_remote_ac_files)"
- if [[ "$testcase_flags" != --noremote_accept_cached* ]]; then
- [[ "$disk_action_cache_files" == 1 ]] || fail "Expected 1 disk action cache entries, not $disk_action_cache_files"
- [[ "$remote_action_cache_files" == 1 ]] || fail "Expected 1 remote action cache entries, not $remote_action_cache_files"
- fi
+ [[ "$disk_action_cache_files" == 1 ]] || fail "CASE 2: Expected 1 disk action cache entries, not $disk_action_cache_files"
+ [[ "$remote_action_cache_files" == 1 ]] || fail "CASE 2: Expected 1 remote action cache entries, not $remote_action_cache_files"
# Case 3)
# disk_cache, remote_cache: remote_exec, disk_cache, remote_cache
@@ -2315,7 +2313,7 @@
bazel clean
bazel build $spawn_flags $testcase_flags $remote_exec_flags $grpc_flags $disk_flags //a:test &> $TEST_log \
|| fail "CASE 3 failed to build"
- if [[ "$testcase_flags" == --noremote_accept_cached* ]]; then
+ if [[ "$testcase_flags" == --noremote_accept_cached ]]; then
expect_log "2 processes: 1 internal, 1 remote." "CASE 3: unexpected action line [[$(grep processes $TEST_log)]]"
else
expect_log "2 processes: 1 disk cache hit, 1 internal." "CASE 3: unexpected action line [[$(grep processes $TEST_log)]]"
@@ -2329,7 +2327,7 @@
bazel clean
bazel build $spawn_flags $testcase_flags $remote_exec_flags $grpc_flags $disk_flags //a:test &> $TEST_log \
|| fail "CASE 4 failed to build"
- if [[ "$testcase_flags" == --noremote_accept_cached* ]]; then
+ if [[ "$testcase_flags" == --noremote_accept_cached ]]; then
expect_log "2 processes: 1 internal, 1 remote." "CASE 4: unexpected action line [[$(grep processes $TEST_log)]]"
else
expect_log "2 processes: 1 disk cache hit, 1 internal." "CASE 4: unexpected action line [[$(grep processes $TEST_log)]]"
@@ -2348,7 +2346,7 @@
bazel clean
bazel build $spawn_flags $testcase_flags --genrule_strategy=remote $remote_exec_flags $grpc_flags $disk_flags //a:test2 &> $TEST_log \
|| fail "CASE 5 failed to build //a:test2"
- if [[ "$testcase_flags" == --noremote_accept_cached* ]]; then
+ if [[ "$testcase_flags" == --noremote_accept_cached ]]; then
expect_log "3 processes: 1 internal, 2 remote." "CASE 5: unexpected action line [[$(grep processes $TEST_log)]]"
else
expect_log "3 processes: 1 disk cache hit, 1 internal, 1 remote." "CASE 5: unexpected action line [[$(grep processes $TEST_log)]]"
@@ -2356,6 +2354,7 @@
}
function test_combined_disk_remote_exec_nocache_tag() {
+ rm -rf ${TEST_TMPDIR}/test_expected
local cache="${TEST_TMPDIR}/disk_cache"
local flags=("--disk_cache=$cache"
"--remote_cache=grpc://localhost:${worker_port}"
@@ -2520,13 +2519,24 @@
rm -rf $cache
}
-function test_combined_cache_with_no_remote_cache_tag() {
+function test_combined_cache_with_no_remote_cache_tag_remote_cache() {
# Test that actions with no-remote-cache tag can hit disk cache of a combined cache but
# remote cache is disabled.
+ combined_cache_with_no_remote_cache_tag "--remote_cache=grpc://localhost:${worker_port}"
+}
+
+function test_combined_cache_with_no_remote_cache_tag_remote_execution() {
+ # Test that actions with no-remote-cache tag can hit disk cache of a combined cache but
+ # remote cache is disabled.
+
+ combined_cache_with_no_remote_cache_tag "--remote_executor=grpc://localhost:${worker_port}"
+}
+
+function combined_cache_with_no_remote_cache_tag() {
local cache="${TEST_TMPDIR}/cache"
local disk_flags="--disk_cache=$cache"
- local grpc_flags="--remote_cache=grpc://localhost:${worker_port}"
+ local grpc_flags="$@"
mkdir -p a
cat > a/BUILD <<EOF
@@ -2560,11 +2570,39 @@
bazel build $grpc_flags //a:test --incompatible_remote_results_ignore_disk=true &> $TEST_log \
|| fail "Failed to build //a:test"
expect_not_log "1 remote cache hit" "Should not get cache hit from grpc cache"
- expect_log "1 .*-sandbox" "Rebuild target failed"
diff bazel-genfiles/a/test.txt ${TEST_TMPDIR}/test_expected \
|| fail "Rebuilt target generated different result"
}
+function test_combined_cache_with_no_remote_tag() {
+ # Test that outputs of actions tagged with no-remote should not be uploaded
+ # to remote cache when remote execution is enabled. See
+ # https://github.com/bazelbuild/bazel/issues/14900.
+ mkdir -p a
+ cat > a/BUILD <<EOF
+package(default_visibility = ["//visibility:public"])
+genrule(
+name = 'test',
+cmd = 'echo "Hello world" > \$@',
+outs = [ 'test.txt' ],
+tags = ['no-remote'],
+)
+EOF
+
+ cache_dir=$(mktemp -d)
+ bazel build \
+ --disk_cache=${cache_dir} \
+ --remote_executor=grpc://localhost:${worker_port} \
+ --incompatible_remote_results_ignore_disk=true \
+ //a:test &> $TEST_log \
+ || fail "Failed to build //a:test"
+
+ remote_ac_files="$(count_remote_ac_files)"
+ [[ "$remote_ac_files" == 0 ]] || fail "Expected 0 remote action cache entries, not $remote_ac_files"
+ remote_cas_files="$(count_remote_cas_files)"
+ [[ "$remote_cas_files" == 0 ]] || fail "Expected 0 remote cas entries, not $remote_cas_files"
+}
+
function test_repo_remote_exec() {
# Test that repository_ctx.execute can execute a command remotely.
