Skip to content

Commit 998e762

Browse files
fmeumcopybara-github
fmeum
authored and
copybara-github
committed
Distinguish between input and output prefetching in request metadata
ea4ad30 added an action as context for prefetches with BwoB, but assumed that all callers would use the prefetcher for inputs to locally executed actions. However, since then, it has also been used to download outputs of completed actions that have been explicitly requested (e.g. as outputs of top-level targets or those matching the regex path patterns). This resulted in downloads with a `prefetcher` action ID and action details for the action that produced the file, rather than consumed it, resulting in confusing situations for observability tools. This is fixed by separately tracking the reason for the fetch. Using an action ID of `output` when the action has the requested file as an output, and "input" when the action has the requested file as an input. Closes bazelbuild#25040. PiperOrigin-RevId: 719246746 Change-Id: Ib95ff65ba68112b1a38ab3022e5b1a19ef74cc9f
1 parent 61f8063 commit 998e762

File tree

9 files changed

+156
-56
lines changed

9 files changed

+156
-56
lines changed

‎src/main/java/com/google/devtools/build/lib/actions/ActionInputPrefetcher.java‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
/** Prefetches files to local disk. */
2222
public interface ActionInputPrefetcher {
2323
public static final ActionInputPrefetcher NONE =
24-
(action, inputs, metadataProvider, priority) ->
24+
(action, inputs, metadataProvider, priority, reason) ->
2525
// Do nothing.
2626
immediateVoidFuture();
2727

@@ -49,6 +49,15 @@ public enum Priority {
4949
LOW,
5050
}
5151

52+
/** The reason for prefetching. */
53+
enum Reason {
54+
/** The requested files are needed as inputs to the given action. */
55+
INPUTS,
56+
57+
/** The requested files are requested as outputs of the given action. */
58+
OUTPUTS,
59+
}
60+
5261
/**
5362
* Initiates best-effort prefetching of all given inputs.
5463
*
@@ -60,5 +69,6 @@ ListenableFuture<Void> prefetchFiles(
6069
ActionExecutionMetadata action,
6170
Iterable<? extends ActionInput> inputs,
6271
InputMetadataProvider metadataProvider,
63-
Priority priority);
72+
Priority priority,
73+
Reason reason);
6474
}

‎src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import com.google.devtools.build.lib.actions.ActionExecutionMetadata;
3030
import com.google.devtools.build.lib.actions.ActionInput;
3131
import com.google.devtools.build.lib.actions.ActionInputPrefetcher.Priority;
32+
import com.google.devtools.build.lib.actions.ActionInputPrefetcher.Reason;
3233
import com.google.devtools.build.lib.actions.ArtifactExpander;
3334
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
3435
import com.google.devtools.build.lib.actions.EnvironmentalExecException;
@@ -296,7 +297,8 @@ public ListenableFuture<Void> prefetchInputs() throws ForbiddenActionInputExcept
296297
getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true)
297298
.values(),
298299
getInputMetadataProvider(),
299-
Priority.MEDIUM),
300+
Priority.MEDIUM,
301+
Reason.INPUTS),
300302
BulkTransferException.class,
301303
(BulkTransferException e) -> {
302304
if (executionOptions.useNewExitCodeForLostInputs

‎src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java‎

Lines changed: 25 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -255,14 +255,14 @@ protected abstract ListenableFuture<Void> doDownloadFile(
255255
Path tempPath,
256256
PathFragment execPath,
257257
FileArtifactValue metadata,
258-
Priority priority)
258+
Priority priority,
259+
Reason reason)
259260
throws IOException;
260261

261262
protected void prefetchVirtualActionInput(VirtualActionInput input) throws IOException {}
262263

263264
/**
264-
* Fetches remotely stored action outputs, that are inputs to this spawn, and stores them under
265-
* their path in the output base.
265+
* Fetches remotely stored action outputs and stores them under their path in the output base.
266266
*
267267
* <p>The {@code inputs} may not contain any unexpanded directories.
268268
*
@@ -276,13 +276,14 @@ public ListenableFuture<Void> prefetchFiles(
276276
ActionExecutionMetadata action,
277277
Iterable<? extends ActionInput> inputs,
278278
InputMetadataProvider metadataProvider,
279-
Priority priority) {
280-
return prefetchFilesInterruptibly(action, inputs, metadataProvider::getInputMetadata, priority);
279+
Priority priority,
280+
Reason reason) {
281+
return prefetchFilesInterruptibly(
282+
action, inputs, metadataProvider::getInputMetadata, priority, reason);
281283
}
282284

283285
/**
284-
* Fetches remotely stored action outputs, that are inputs to this spawn, and stores them under
285-
* their path in the output base.
286+
* Fetches remotely stored action outputs and stores them under their path in the output base.
286287
*
287288
* <p>The {@code inputs} may not contain any unexpanded directories.
288289
*
@@ -300,7 +301,8 @@ public ListenableFuture<Void> prefetchFilesInterruptibly(
300301
ActionExecutionMetadata action,
301302
Iterable<? extends ActionInput> inputs,
302303
MetadataSupplier metadataSupplier,
303-
Priority priority) {
304+
Priority priority,
305+
Reason reason) {
304306
List<ActionInput> files = new ArrayList<>();
305307

306308
for (ActionInput input : inputs) {
@@ -334,7 +336,8 @@ public ListenableFuture<Void> prefetchFilesInterruptibly(
334336
try (var s = Profiler.instance().profile("compose prefetches")) {
335337
for (var file : files) {
336338
transfers.add(
337-
prefetchFile(action, dirsWithOutputPermissions, metadataSupplier, file, priority));
339+
prefetchFile(
340+
action, dirsWithOutputPermissions, metadataSupplier, file, priority, reason));
338341
}
339342
}
340343

@@ -365,7 +368,8 @@ private ListenableFuture<Void> prefetchFile(
365368
Set<Path> dirsWithOutputPermissions,
366369
MetadataSupplier metadataSupplier,
367370
ActionInput input,
368-
Priority priority) {
371+
Priority priority,
372+
Reason reason) {
369373
try {
370374
if (input instanceof VirtualActionInput virtualActionInput) {
371375
prefetchVirtualActionInput(virtualActionInput);
@@ -397,7 +401,8 @@ private ListenableFuture<Void> prefetchFile(
397401
dirsWithOutputPermissions,
398402
input,
399403
metadata,
400-
priority);
404+
priority,
405+
reason);
401406

402407
if (symlink != null) {
403408
result = result.andThen(plantSymlink(symlink));
@@ -516,7 +521,8 @@ private Completable downloadFileNoCheckRx(
516521
Set<Path> dirsWithOutputPermissions,
517522
ActionInput actionInput,
518523
FileArtifactValue metadata,
519-
Priority priority) {
524+
Priority priority,
525+
Reason reason) {
520526
// If the path to be prefetched is a non-dangling symlink, prefetch its target path instead.
521527
// Note that this only applies to symlinks created by spawns (or, currently, with the internal
522528
// version of BwoB); symlinks created in-process through an ActionFileSystem should have already
@@ -561,7 +567,8 @@ private Completable downloadFileNoCheckRx(
561567
tempPath,
562568
finalPath.relativeTo(execRoot),
563569
metadata,
564-
priority),
570+
priority,
571+
reason),
565572
directExecutor())
566573
.doOnComplete(
567574
() -> {
@@ -729,7 +736,11 @@ public void finalizeAction(Action action, OutputMetadataStore outputMetadataStor
729736
try (var s = Profiler.instance().profile(ProfilerTask.REMOTE_DOWNLOAD, "Download outputs")) {
730737
getFromFuture(
731738
prefetchFilesInterruptibly(
732-
action, outputsToDownload, outputMetadataStore::getOutputMetadata, Priority.HIGH));
739+
action,
740+
outputsToDownload,
741+
outputMetadataStore::getOutputMetadata,
742+
Priority.HIGH,
743+
Reason.OUTPUTS));
733744
}
734745
}
735746
}

‎src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import com.google.devtools.build.lib.actions.ActionInputHelper;
3232
import com.google.devtools.build.lib.actions.ActionInputMap;
3333
import com.google.devtools.build.lib.actions.ActionInputPrefetcher.Priority;
34+
import com.google.devtools.build.lib.actions.ActionInputPrefetcher.Reason;
3435
import com.google.devtools.build.lib.actions.Artifact;
3536
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
3637
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
@@ -757,7 +758,11 @@ private void downloadFileIfRemote(PathFragment path) throws IOException {
757758
}
758759
getFromFuture(
759760
inputFetcher.prefetchFiles(
760-
action, ImmutableList.of(input), inputArtifactData, Priority.CRITICAL));
761+
action,
762+
ImmutableList.of(input),
763+
inputArtifactData,
764+
Priority.CRITICAL,
765+
Reason.INPUTS));
761766
} catch (InterruptedException e) {
762767
Thread.currentThread().interrupt();
763768
throw new IOException(String.format("Received interrupt while fetching file '%s'", path), e);

‎src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,11 +84,19 @@ protected ListenableFuture<Void> doDownloadFile(
8484
Path tempPath,
8585
PathFragment execPath,
8686
FileArtifactValue metadata,
87-
Priority priority)
87+
Priority priority,
88+
Reason reason)
8889
throws IOException {
8990
checkArgument(metadata.isRemote(), "Cannot download file that is not a remote file.");
9091
RequestMetadata requestMetadata =
91-
TracingMetadataUtils.buildMetadata(buildRequestId, commandId, "prefetcher", action);
92+
TracingMetadataUtils.buildMetadata(
93+
buildRequestId,
94+
commandId,
95+
switch (reason) {
96+
case INPUTS -> "input";
97+
case OUTPUTS -> "output";
98+
},
99+
action);
92100
RemoteActionExecutionContext context = RemoteActionExecutionContext.create(requestMetadata);
93101

94102
Digest digest = DigestUtil.buildDigest(metadata.getDigest(), metadata.getSize());

‎src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import com.google.devtools.build.lib.actions.ActionInputMap;
3030
import com.google.devtools.build.lib.actions.ActionInputPrefetcher;
3131
import com.google.devtools.build.lib.actions.ActionInputPrefetcher.Priority;
32+
import com.google.devtools.build.lib.actions.ActionInputPrefetcher.Reason;
3233
import com.google.devtools.build.lib.actions.Artifact;
3334
import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
3435
import com.google.devtools.build.lib.actions.CompletionContext;
@@ -463,7 +464,8 @@ private void downloadArtifact(
463464
var action =
464465
ActionUtils.getActionForLookupData(env, derivedArtifact.getGeneratingActionKey());
465466
var future =
466-
actionInputPrefetcher.prefetchFiles(action, filesToDownload, inputMap, Priority.LOW);
467+
actionInputPrefetcher.prefetchFiles(
468+
action, filesToDownload, inputMap, Priority.LOW, Reason.OUTPUTS);
467469
futures.add(future);
468470
}
469471
} else {
@@ -477,7 +479,7 @@ private void downloadArtifact(
477479
ActionUtils.getActionForLookupData(env, derivedArtifact.getGeneratingActionKey());
478480
var future =
479481
actionInputPrefetcher.prefetchFiles(
480-
action, ImmutableList.of(artifact), inputMap, Priority.LOW);
482+
action, ImmutableList.of(artifact), inputMap, Priority.LOW, Reason.OUTPUTS);
481483
futures.add(future);
482484
}
483485
}

0 commit comments

Comments
 (0)