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 ae57825d11a2bb..0e3b79cedf070f 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; @@ -271,7 +272,8 @@ private Single upload(Set files) { 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 00000000000000..e680ea484d818c --- /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 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 063ff493715393..62fecc7a92cee5 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 @@ static Command buildCommand( 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 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 @@ public int hashCode() { @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 @@ private ListenableFuture downloadFile(RemoteAction action, FileMet try { ListenableFuture 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 createSymlinks(Iterable symlinks) throws IOExcepti 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 @@ ActionResultMetadata parseActionResultMetadata(RemoteAction action, RemoteAction 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())); @@ -1005,6 +926,8 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re 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); @@ -1012,8 +935,8 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re 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. @@ -1027,16 +950,28 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re && !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> downloadsBuilder = ImmutableList.builder(); @@ -1045,7 +980,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re shouldDownloadAllSpawnOutputs( remoteOutputsMode, /* exitCode = */ result.getExitCode(), - hasFilesToDownload(action.spawn.getOutputFiles(), filesToDownload)); + hasFilesToDownload(action.getSpawn().getOutputFiles(), filesToDownload)); if (downloadOutputs) { HashSet queuedFilePaths = new HashSet<>(); @@ -1080,7 +1015,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re FileOutErr tmpOutErr = outErr.childOutErr(); List> outErrDownloads = remoteCache.downloadOutErr( - action.remoteActionExecutionContext, result.actionResult, tmpOutErr); + action.getRemoteActionExecutionContext(), result.actionResult, tmpOutErr); for (ListenableFuture future : outErrDownloads) { downloadsBuilder.add(transform(future, (v) -> null, directExecutor())); } @@ -1100,7 +1035,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re // Ensure that we are the only ones writing to the output files when using the dynamic spawn // strategy. - action.spawnExecutionContext.lockOutputFiles(); + action.getSpawnExecutionContext().lockOutputFiles(); if (downloadOutputs) { moveOutputsToFinalLocation(downloads); @@ -1121,9 +1056,9 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re } 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); @@ -1143,7 +1078,8 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re try (SilentCloseable c = Profiler.instance().profile("Remote.downloadInMemoryOutput")) { if (inMemoryOutput != null) { ListenableFuture inMemoryOutputDownload = - remoteCache.downloadBlob(action.remoteActionExecutionContext, inMemoryOutputDigest); + remoteCache.downloadBlob( + action.getRemoteActionExecutionContext(), inMemoryOutputDigest); waitForBulkTransfer( ImmutableList.of(inMemoryOutputDownload), /* cancelRemainingOnInterrupt=*/ true); byte[] data = getFromFuture(inMemoryOutputDownload); @@ -1169,9 +1105,9 @@ private Single buildUploadManifestAsync( () -> { ImmutableList.Builder 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."); } @@ -1182,11 +1118,11 @@ private Single buildUploadManifestAsync( 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()); }); } @@ -1211,7 +1147,7 @@ UploadManifest buildUploadManifest(RemoteAction action, SpawnResult spawnResult) 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"); @@ -1275,15 +1211,17 @@ private void reportUploadError(Throwable error) { 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 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); } /** @@ -1296,12 +1234,14 @@ public RemoteActionResult executeRemotely( 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 @@ -1315,7 +1255,9 @@ public RemoteActionResult executeRemotely( 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); } @@ -1344,7 +1286,7 @@ public ServerLogs maybeDownloadServerLogs(RemoteAction action, ExecuteResponse r 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 3222d6a1ecdc27..3ce198104d2e9d 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 d3f0c008726ff6..8440220704e8f2 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 @@ -56,7 +56,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 c27eeb90463fbe..78cac3980c2a71 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 @@ default ActionExecutionMetadata getSpawnOwner() { 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 1b099457962490..00000000000000 --- 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 e5a936cf66d8bc..c337a5ebe3cdf1 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 DiskAndRemoteCacheClient( public ListenableFuture uploadActionResult( RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) { ListenableFuture 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 @@ public void close() { @Override public ListenableFuture 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); } - ListenableFuture future = diskCache.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); + } - if (options.isRemoteExecutionEnabled() - || shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) { + ListenableFuture future = diskCache.uploadFile(context, digest, file); + if (shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) { future = Futures.transformAsync( future, v -> remoteCache.uploadFile(context, digest, file), directExecutor()); @@ -93,9 +103,20 @@ public ListenableFuture uploadFile( @Override public ListenableFuture 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 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 @@ public ListenableFuture uploadBlob( @Override public ListenableFuture> findMissingDigests( RemoteActionExecutionContext context, Iterable 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 @@ public ListenableFuture downloadBlob( 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 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 dad364ca44b1be..9d39facdc6a0cc 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 3c97b692edb029..7a153d64bf1c30 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 @@ -62,7 +62,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 8731a7b1a77852..037a386469dda9 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -88,7 +88,8 @@ EOF || fail "Failed to build //a:foo with remote cache" } -function test_remote_grpc_via_unix_socket_proxy() { +# TODO(b/211478955): Deflake and re-enable. +function DISABLED_test_remote_grpc_via_unix_socket_proxy() { case "$PLATFORM" in darwin|freebsd|linux|openbsd) ;; @@ -126,7 +127,8 @@ EOF rmdir "${socket_dir}" } -function test_remote_grpc_via_unix_socket_direct() { +# TODO(b/211478955): Deflake and re-enable. +function DISABLED_test_remote_grpc_via_unix_socket_direct() { case "$PLATFORM" in darwin|freebsd|linux|openbsd) ;; @@ -2105,7 +2107,6 @@ function test_combined_disk_remote_exec_with_flag_combinations() { # 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" @@ -2129,15 +2130,13 @@ function genrule_combined_disk_remote_exec() { # 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. @@ -2167,13 +2166,14 @@ EOF 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. bazel build $spawn_flags $testcase_flags $remote_exec_flags $grpc_flags $disk_flags //a:test &> $TEST_log \ || fail "CASE 1 Failed to build" + rm -rf ${TEST_TMPDIR}/test_expected echo "Hello world" > ${TEST_TMPDIR}/test_expected expect_log "2 processes: 1 internal, 1 remote." "CASE 1: unexpected action line [[$(grep processes $TEST_log)]]" diff bazel-genfiles/a/test.txt ${TEST_TMPDIR}/test_expected \ @@ -2182,10 +2182,13 @@ EOF 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 @@ -2202,10 +2205,8 @@ EOF # 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 @@ -2222,7 +2223,7 @@ EOF 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)]]" @@ -2236,7 +2237,7 @@ EOF 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)]]" @@ -2255,7 +2256,7 @@ EOF 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)]]" @@ -2263,6 +2264,7 @@ EOF } 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}" @@ -2427,13 +2429,24 @@ EOF 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 < $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 < $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.