Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remote: Fix a bug that outputs of actions tagged with no-remote are u… #15212

Closed
wants to merge 5 commits into from
Closed

Remote: Fix a bug that outputs of actions tagged with no-remote are u… #15212

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
38ddc2a
Remote: Fix a bug that outputs of actions tagged with no-remote are u…
coeuvre Apr 11, 2022
8571f37
fix tests
coeuvre Apr 11, 2022
034a226
fix tests
coeuvre Apr 11, 2022
b9cc232
fix tests
coeuvre Apr 12, 2022
473869c
address review comments
coeuvre Apr 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -256,7 +257,8 @@ private Single<PathConverter> upload(Set<Path> 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,
Expand Down
57 changes: 35 additions & 22 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@
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;
Expand Down Expand Up @@ -258,7 +259,7 @@ static Command buildCommand(
public static class RemoteAction {
private final Spawn spawn;
private final SpawnExecutionContext spawnExecutionContext;
private final RemoteActionExecutionContext remoteActionExecutionContext;
private final RemoteActionExecutionContext context;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a good idea to rename this, given that there are two different context objects?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Revert the rename. Extracted RemoteAction to an upper level.

private final RemotePathResolver remotePathResolver;
private final MerkleTree merkleTree;
private final Digest commandHash;
Expand All @@ -269,7 +270,7 @@ public static class RemoteAction {
RemoteAction(
Spawn spawn,
SpawnExecutionContext spawnExecutionContext,
RemoteActionExecutionContext remoteActionExecutionContext,
RemoteActionExecutionContext context,
RemotePathResolver remotePathResolver,
MerkleTree merkleTree,
Digest commandHash,
Expand All @@ -278,7 +279,7 @@ public static class RemoteAction {
ActionKey actionKey) {
this.spawn = spawn;
this.spawnExecutionContext = spawnExecutionContext;
this.remoteActionExecutionContext = remoteActionExecutionContext;
this.context = context;
this.remotePathResolver = remotePathResolver;
this.merkleTree = merkleTree;
this.commandHash = commandHash;
Expand All @@ -287,8 +288,8 @@ public static class RemoteAction {
this.actionKey = actionKey;
}

public RemoteActionExecutionContext getRemoteActionExecutionContext() {
return remoteActionExecutionContext;
public RemoteActionExecutionContext getContext() {
return context;
}

/** Returns the {@link ActionExecutionMetadata} that owns this action. */
Expand Down Expand Up @@ -333,7 +334,7 @@ public SortedMap<PathFragment, ActionInput> getInputMap()
* execution.
*/
public NetworkTime getNetworkTime() {
return remoteActionExecutionContext.getNetworkTime();
return context.getNetworkTime();
}
}

Expand Down Expand Up @@ -617,9 +618,11 @@ public RemoteActionResult lookupCache(RemoteAction action)
throws IOException, InterruptedException {
checkState(shouldAcceptCachedResult(action.spawn), "spawn doesn't accept cached result");

action.context.setStep(Step.CHECK_ACTION_CACHE);

CachedActionResult cachedActionResult =
remoteCache.downloadActionResult(
action.remoteActionExecutionContext, action.actionKey, /* inlineOutErr= */ false);
action.context, action.actionKey, /* inlineOutErr= */ false);

if (cachedActionResult == null) {
return null;
Expand All @@ -639,7 +642,7 @@ private ListenableFuture<FileMetadata> downloadFile(RemoteAction action, FileMet
try {
ListenableFuture<Void> future =
remoteCache.downloadFile(
action.remoteActionExecutionContext,
action.context,
remotePathResolver.localPathToOutputPath(file.path()),
toTmpDownloadPath(file.path()),
file.digest(),
Expand Down Expand Up @@ -763,7 +766,7 @@ private void createSymlinks(Iterable<SymlinkMetadata> symlinks) throws IOExcepti

private void injectRemoteArtifact(
RemoteAction action, Artifact output, ActionResultMetadata metadata) throws IOException {
RemoteActionExecutionContext context = action.remoteActionExecutionContext;
RemoteActionExecutionContext context = action.context;
MetadataInjector metadataInjector = action.spawnExecutionContext.getMetadataInjector();
Path path = remotePathResolver.outputPathToLocalPath(output);
if (output.isTreeArtifact()) {
Expand Down Expand Up @@ -947,7 +950,7 @@ ActionResultMetadata parseActionResultMetadata(RemoteAction action, RemoteAction
dirMetadataDownloads.put(
remotePathResolver.outputPathToLocalPath(dir.getPath()),
Futures.transformAsync(
remoteCache.downloadBlob(action.remoteActionExecutionContext, dir.getTreeDigest()),
remoteCache.downloadBlob(action.context, dir.getTreeDigest()),
(treeBytes) ->
immediateFuture(Tree.parseFrom(treeBytes, ExtensionRegistry.getEmptyRegistry())),
directExecutor()));
Expand Down Expand Up @@ -1006,6 +1009,8 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
checkState(!shutdown.get(), "shutdown");
checkNotNull(remoteCache, "remoteCache can't be null");

action.context.setStep(Step.DOWNLOAD_OUTPUTS);

ActionResultMetadata metadata;
try (SilentCloseable c = Profiler.instance().profile("Remote.parseActionResultMetadata")) {
metadata = parseActionResultMetadata(action, result);
Expand Down Expand Up @@ -1035,6 +1040,13 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
}
}
}

// 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.context.getExecuteResponse() != null) {
remoteCache.uploadActionResult(action.context, action.actionKey, result.actionResult);
}
}

FileOutErr outErr = action.spawnExecutionContext.getFileOutErr();
Expand Down Expand Up @@ -1072,8 +1084,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re

FileOutErr tmpOutErr = outErr.childOutErr();
List<ListenableFuture<Void>> outErrDownloads =
remoteCache.downloadOutErr(
action.remoteActionExecutionContext, result.actionResult, tmpOutErr);
remoteCache.downloadOutErr(action.context, result.actionResult, tmpOutErr);
for (ListenableFuture<Void> future : outErrDownloads) {
downloadsBuilder.add(transform(future, (v) -> null, directExecutor()));
}
Expand Down Expand Up @@ -1138,7 +1149,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
try (SilentCloseable c = Profiler.instance().profile("Remote.downloadInMemoryOutput")) {
if (inMemoryOutput != null) {
ListenableFuture<byte[]> inMemoryOutputDownload =
remoteCache.downloadBlob(action.remoteActionExecutionContext, inMemoryOutputDigest);
remoteCache.downloadBlob(action.context, inMemoryOutputDigest);
waitForBulkTransfer(
ImmutableList.of(inMemoryOutputDownload), /* cancelRemainingOnInterrupt=*/ true);
byte[] data = getFromFuture(inMemoryOutputDownload);
Expand Down Expand Up @@ -1218,8 +1229,7 @@ public void uploadOutputs(RemoteAction action, SpawnResult spawnResult)
buildUploadManifestAsync(action, spawnResult)
.flatMap(
manifest ->
manifest.uploadAsync(
action.getRemoteActionExecutionContext(), remoteCache, reporter)),
manifest.uploadAsync(action.getContext(), remoteCache, reporter)),
RemoteCache::release)
.subscribeOn(scheduler)
.subscribe(
Expand All @@ -1244,7 +1254,7 @@ public void onError(@NonNull Throwable e) {
try (SilentCloseable c =
Profiler.instance().profile(ProfilerTask.UPLOAD_TIME, "upload outputs")) {
UploadManifest manifest = buildUploadManifest(action, spawnResult);
manifest.upload(action.getRemoteActionExecutionContext(), remoteCache, reporter);
manifest.upload(action.getContext(), remoteCache, reporter);
} catch (IOException e) {
reportUploadError(e);
}
Expand Down Expand Up @@ -1272,13 +1282,15 @@ public void uploadInputsIfNotPresent(RemoteAction action, boolean force)
checkState(!shutdown.get(), "shutdown");
checkState(mayBeExecutedRemotely(action.spawn), "spawn can't be executed remotely");

action.context.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);
remoteExecutionCache.ensureInputsPresent(
action.remoteActionExecutionContext, action.merkleTree, additionalInputs, force);
action.context, action.merkleTree, additionalInputs, force);
}

/**
Expand All @@ -1293,6 +1305,8 @@ public RemoteActionResult executeRemotely(
checkState(!shutdown.get(), "shutdown");
checkState(mayBeExecutedRemotely(action.spawn), "spawn can't be executed remotely");

action.context.setStep(Step.EXECUTE_REMOTELY);

ExecuteRequest.Builder requestBuilder =
ExecuteRequest.newBuilder()
.setInstanceName(remoteOptions.remoteInstanceName)
Expand All @@ -1309,8 +1323,9 @@ public RemoteActionResult executeRemotely(

ExecuteRequest request = requestBuilder.build();

ExecuteResponse reply =
remoteExecutor.executeRemotely(action.remoteActionExecutionContext, request, observer);
ExecuteResponse reply = remoteExecutor.executeRemotely(action.context, request, observer);

action.context.setExecuteResponse(reply);

return RemoteActionResult.createFromResponse(reply);
}
Expand Down Expand Up @@ -1339,9 +1354,7 @@ public ServerLogs maybeDownloadServerLogs(RemoteAction action, ExecuteResponse r
serverLogs.logCount++;
getFromFuture(
remoteCache.downloadFile(
action.remoteActionExecutionContext,
serverLogs.lastLogPath,
e.getValue().getDigest()));
action.context, serverLogs.lastLogPath, e.getValue().getDigest()));
}
}
}
Expand Down
85 changes: 61 additions & 24 deletions src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
private final Spawn spawn;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mark as @Nullable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

private final RequestMetadata requestMetadata;
private final NetworkTime networkTime;

@Nullable private ExecuteResponse executeResponse;
private Step step;

public RemoteActionExecutionContext(
Spawn spawn, RequestMetadata requestMetadata, NetworkTime networkTime) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also - should we make the ctor private to enforce construction through one of the create() methods?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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;
Expand All @@ -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());
}
}
Loading