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.

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

Anchor input fetches to source action id #11236

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,11 @@ public long getSize() {
return 0;
}

@Override
public String getActionId() {
return "";
}

@Override
public long getModifiedTime() {
return -1;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ public abstract class FileArtifactValue implements SkyValue, HasDigest {
// TODO(ulfjack): Throw an exception if it's not a file.
public abstract long getSize();

public abstract String getActionId();
werkt marked this conversation as resolved.
Show resolved Hide resolved

/**
* Returns the last modified time; see the documentation of {@link #getDigest} for when this can
* and should be called.
Expand Down Expand Up @@ -365,6 +367,11 @@ public long getSize() {
return 0;
}

@Override
public String getActionId() {
return "";
}

@Override
public boolean wasModifiedSinceDigest(Path path) throws IOException {
return false;
Expand Down Expand Up @@ -425,6 +432,11 @@ public long getSize() {
return 0;
}

@Override
public String getActionId() {
return "";
}

@Override
public boolean wasModifiedSinceDigest(Path path) {
// TODO(ulfjack): Ideally, we'd attempt to detect intra-build modifications here. I'm
Expand Down Expand Up @@ -490,6 +502,11 @@ public long getSize() {
return size;
}

@Override
public String getActionId() {
return "";
}

@Override
public boolean wasModifiedSinceDigest(Path path) throws IOException {
if (proxy == null) {
Expand Down Expand Up @@ -544,11 +561,13 @@ public static final class RemoteFileArtifactValue extends FileArtifactValue {
private final byte[] digest;
private final long size;
private final int locationIndex;
private final String actionId;

public RemoteFileArtifactValue(byte[] digest, long size, int locationIndex) {
public RemoteFileArtifactValue(byte[] digest, long size, int locationIndex, String actionId) {
this.digest = digest;
this.size = size;
this.locationIndex = locationIndex;
this.actionId = actionId;
}

@Override
Expand Down Expand Up @@ -588,6 +607,11 @@ public long getSize() {
return size;
}

@Override
public String getActionId() {
return actionId;
}

@Override
public long getModifiedTime() {
throw new UnsupportedOperationException(
Expand Down Expand Up @@ -642,6 +666,11 @@ public long getSize() {
return 0;
}

@Override
public String getActionId() {
return "";
}

@Override
public long getModifiedTime() {
throw new IllegalStateException();
Expand Down Expand Up @@ -720,6 +749,11 @@ public long getSize() {
return data.length;
}

@Override
public String getActionId() {
return "";
}

@Override
public long getModifiedTime() {
throw new UnsupportedOperationException();
Expand Down Expand Up @@ -802,6 +836,11 @@ public long getSize() {
return size;
}

@Override
public String getActionId() {
return "";
}

@Override
public long getModifiedTime() {
throw new UnsupportedOperationException();
Expand All @@ -825,6 +864,11 @@ public byte[] getDigest() {
return null;
}

@Override
public String getActionId() {
return "";
}

@Override
public FileContentsProxy getContentsProxy() {
throw new UnsupportedOperationException();
Expand Down Expand Up @@ -868,6 +912,11 @@ public byte[] getDigest() {
throw new UnsupportedOperationException();
}

@Override
public String getActionId() {
throw new UnsupportedOperationException();
}

@Override
public FileContentsProxy getContentsProxy() {
throw new UnsupportedOperationException();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,9 @@ public interface MetadataInjector {
* @param digest the digest of the file.
* @param size the size of the file in bytes.
* @param locationIndex is only used in Blaze.
* @param actionId the id of the action that produced this file.
*/
void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex);
void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex, String actionId);

/**
* Inject the metadata of a tree artifact whose contents are stored remotely.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package com.google.devtools.build.lib.remote;

import build.bazel.remote.execution.v2.Digest;
import build.bazel.remote.execution.v2.RequestMetadata;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableSet;
Expand All @@ -32,6 +33,7 @@
import com.google.devtools.build.lib.profiler.SilentCloseable;
import com.google.devtools.build.lib.remote.common.CacheNotFoundException;
import com.google.devtools.build.lib.remote.util.DigestUtil;
import com.google.devtools.build.lib.remote.util.TracingMetadataUtils;
import com.google.devtools.build.lib.remote.util.Utils;
import com.google.devtools.build.lib.vfs.Path;
import io.grpc.Context;
Expand Down Expand Up @@ -66,12 +68,12 @@ class RemoteActionInputFetcher implements ActionInputPrefetcher {

private final RemoteCache remoteCache;
private final Path execRoot;
private final Context ctx;
private final RequestMetadata requestMetadata;

RemoteActionInputFetcher(RemoteCache remoteCache, Path execRoot, Context ctx) {
RemoteActionInputFetcher(RemoteCache remoteCache, Path execRoot, RequestMetadata requestMetadata) {
this.remoteCache = Preconditions.checkNotNull(remoteCache);
this.execRoot = Preconditions.checkNotNull(execRoot);
this.ctx = Preconditions.checkNotNull(ctx);
this.requestMetadata = Preconditions.checkNotNull(requestMetadata);
}

/**
Expand Down Expand Up @@ -125,9 +127,10 @@ public void prefetchFiles(
e =
new IOException(
String.format(
"Failed to fetch file with hash '%s' because it does not exist remotely."
"Failed to fetch file with hash '%s' into '%s' because it does not exist remotely."
+ " --experimental_remote_outputs=minimal does not work if"
+ " your remote cache evicts files during builds.",
entry.getKey(),
((CacheNotFoundException) e).getMissingDigest().getHash()));
}
ioException = ioException == null ? e : ioException;
Expand Down Expand Up @@ -172,6 +175,10 @@ private ListenableFuture<Void> downloadFileAsync(Path path, FileArtifactValue me

ListenableFuture<Void> download = downloadsInProgress.get(path);
if (download == null) {
Context ctx = TracingMetadataUtils.contextWithMetadata(
requestMetadata.toBuilder()
.setActionId(metadata.getActionId())
.build());
Context prevCtx = ctx.attach();
try {
Digest digest = DigestUtil.buildDigest(metadata.getDigest(), metadata.getSize());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,7 @@ private List<ListenableFuture<FileMetadata>> downloadOutErr(ActionResult result,
*/
@Nullable
public InMemoryOutput downloadMinimal(
String actionId,
ActionResult result,
Collection<? extends ActionInput> outputs,
@Nullable PathFragment inMemoryOutputPath,
Expand Down Expand Up @@ -565,7 +566,7 @@ public InMemoryOutput downloadMinimal(
inMemoryOutput = output;
}
if (output instanceof Artifact) {
injectRemoteArtifact((Artifact) output, metadata, execRoot, metadataInjector);
injectRemoteArtifact((Artifact) output, metadata, execRoot, metadataInjector, actionId);
}
}

Expand All @@ -589,7 +590,8 @@ private void injectRemoteArtifact(
Artifact output,
ActionResultMetadata metadata,
Path execRoot,
MetadataInjector metadataInjector)
MetadataInjector metadataInjector,
String actionId)
throws IOException {
if (output.isTreeArtifact()) {
DirectoryMetadata directory =
Expand All @@ -612,7 +614,8 @@ private void injectRemoteArtifact(
new RemoteFileArtifactValue(
DigestUtil.toBinaryDigest(file.digest()),
file.digest().getSizeBytes(),
/* locationIndex= */ 1);
/* locationIndex= */ 1,
actionId);
childMetadata.put(p, r);
}
metadataInjector.injectRemoteDirectory(
Expand All @@ -628,7 +631,8 @@ private void injectRemoteArtifact(
output,
DigestUtil.toBinaryDigest(outputMetadata.digest()),
outputMetadata.digest().getSizeBytes(),
/* locationIndex= */ 1);
/* locationIndex= */ 1,
actionId);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package com.google.devtools.build.lib.remote;

import build.bazel.remote.execution.v2.DigestFunction;
import build.bazel.remote.execution.v2.RequestMetadata;
import build.bazel.remote.execution.v2.ServerCapabilities;
import com.google.auth.Credentials;
import com.google.common.base.Preconditions;
Expand Down Expand Up @@ -662,12 +663,14 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB
env.getOptions().getOptions(RemoteOptions.class), "RemoteOptions");
RemoteOutputsMode remoteOutputsMode = remoteOptions.remoteOutputsMode;
if (!remoteOutputsMode.downloadAllOutputs()) {
Context ctx =
TracingMetadataUtils.contextWithMetadata(
env.getBuildRequestId(), env.getCommandId().toString(), "fetch-remote-inputs");
RequestMetadata requestMetadata =
RequestMetadata.newBuilder()
.setCorrelatedInvocationsId(env.getBuildRequestId())
.setToolInvocationId(env.getCommandId().toString())
.build();
actionInputFetcher =
new RemoteActionInputFetcher(
actionContextProvider.getRemoteCache(), env.getExecRoot(), ctx);
actionContextProvider.getRemoteCache(), env.getExecRoot(), requestMetadata);
builder.setActionInputPrefetcher(actionInputFetcher);
remoteOutputService.setActionInputFetcher(actionInputFetcher);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
prof.profile(ProfilerTask.REMOTE_DOWNLOAD, "download outputs minimal")) {
inMemoryOutput =
remoteCache.downloadMinimal(
actionKey.getDigest().getHash(),
result,
spawn.getOutputFiles(),
inMemoryOutputPath,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
} else {
try {
return downloadAndFinalizeSpawnResult(
actionKey.getDigest().getHash(),
cachedResult,
/* cacheHit= */ true,
spawn,
Expand Down Expand Up @@ -341,6 +342,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)

try {
return downloadAndFinalizeSpawnResult(
actionKey.getDigest().getHash(),
actionResult,
reply.getCachedResult(),
spawn,
Expand Down Expand Up @@ -413,6 +415,7 @@ static void spawnMetricsAccounting(
}

private SpawnResult downloadAndFinalizeSpawnResult(
String actionId,
ActionResult actionResult,
boolean cacheHit,
Spawn spawn,
Expand Down Expand Up @@ -441,6 +444,7 @@ private SpawnResult downloadAndFinalizeSpawnResult(
Profiler.instance().profile(REMOTE_DOWNLOAD, "download outputs minimal")) {
inMemoryOutput =
remoteCache.downloadMinimal(
actionId,
actionResult,
spawn.getOutputFiles(),
inMemoryOutputPath,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,18 +70,21 @@ public static Context contextWithMetadata(
Preconditions.checkNotNull(buildRequestId);
Preconditions.checkNotNull(commandId);
Preconditions.checkNotNull(actionId);
RequestMetadata.Builder metadata =
RequestMetadata metadata =
RequestMetadata.newBuilder()
.setCorrelatedInvocationsId(buildRequestId)
.setToolInvocationId(commandId);
metadata.setActionId(actionId);
metadata
.setToolDetails(
ToolDetails.newBuilder()
.setToolName("bazel")
.setToolVersion(BlazeVersionInfo.instance().getVersion()))
.build();
return Context.current().withValue(CONTEXT_KEY, metadata.build());
.setToolInvocationId(commandId)
.setActionId(actionId)
.setToolDetails(
ToolDetails.newBuilder()
.setToolName("bazel")
.setToolVersion(BlazeVersionInfo.instance().getVersion()))
.build();
return contextWithMetadata(metadata);
}

public static Context contextWithMetadata(RequestMetadata metadata) {
return Context.current().withValue(CONTEXT_KEY, metadata);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -542,7 +542,7 @@ public void injectDigest(ActionInput output, FileStatus statNoFollow, byte[] dig
}

@Override
public void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex) {
public void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex, String actionId) {
Preconditions.checkArgument(
isKnownOutput(output), output + " is not a declared output of this action");
Preconditions.checkArgument(
Expand All @@ -551,7 +551,7 @@ public void injectRemoteFile(Artifact output, byte[] digest, long size, int loca
output);
Preconditions.checkState(
executionMode.get(), "Tried to inject %s outside of execution", output);
store.injectRemoteFile(output, digest, size, locationIndex);
store.injectRemoteFile(output, digest, size, locationIndex, actionId);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,9 @@ void addTreeArtifactContents(Artifact artifact, TreeFileArtifact contents) {
treeArtifactContents.computeIfAbsent(artifact, a -> Sets.newConcurrentHashSet()).add(contents);
}

void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex) {
void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex, String actionId) {
injectOutputData(
output, new FileArtifactValue.RemoteFileArtifactValue(digest, size, locationIndex));
output, new FileArtifactValue.RemoteFileArtifactValue(digest, size, locationIndex, actionId));
}

final void injectOutputData(Artifact output, FileArtifactValue artifactValue) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,11 @@ public long getSize() {
throw new UnsupportedOperationException();
}

@Override
public String getActionId() {
throw new UnsupportedOperationException();
}

@Override
public long getModifiedTime() {
throw new UnsupportedOperationException();
Expand Down
Loading