Skip to content

Commit 2b82190

Browse files
fmeumcopybara-github
fmeum
authored and
copybara-github
committed
Wire up PathMapper in RemoteExecutionService
`PathMapper`s rewrite paths in command lines to make them more cache friendly, which requires executor support to stage files at the rewritten paths. This commit wires up the `PathMapper` used by a given `Spawn` in `RemoteExecutionService`, which ensures that paths of inputs and outputs are correctly mapped before being sent off to the remote executor and mapped back to the correct local paths when downloading the results. An end-to-end test will be added in #18155, but requires #19718, #19719, and #19721. Work towards #6526 Closes #19721. PiperOrigin-RevId: 573806130 Change-Id: Ibbd4ff641eb301d78f5ec54813e65788d786fcea
1 parent 7565756 commit 2b82190

File tree

4 files changed

+212
-15
lines changed

4 files changed

+212
-15
lines changed

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,10 @@ public Command getCommand() {
117117
return command;
118118
}
119119

120+
public RemotePathResolver getRemotePathResolver() {
121+
return remotePathResolver;
122+
}
123+
120124
@Nullable
121125
public MerkleTree getMerkleTree() {
122126
return merkleTree;

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

Lines changed: 37 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,13 @@ public class RemoteExecutionService {
160160
private final Reporter reporter;
161161
private final boolean verboseFailures;
162162
private final Path execRoot;
163-
private final RemotePathResolver remotePathResolver;
163+
164+
/**
165+
* Do not use directly, instead use the per-spawn resolver created in {@link
166+
* #buildRemoteAction(Spawn, SpawnExecutionContext)}.
167+
*/
168+
private final RemotePathResolver baseRemotePathResolver;
169+
164170
private final String buildRequestId;
165171
private final String commandId;
166172
private final DigestUtil digestUtil;
@@ -200,7 +206,7 @@ public RemoteExecutionService(
200206
this.reporter = reporter;
201207
this.verboseFailures = verboseFailures;
202208
this.execRoot = execRoot;
203-
this.remotePathResolver = remotePathResolver;
209+
this.baseRemotePathResolver = remotePathResolver;
204210
this.buildRequestId = buildRequestId;
205211
this.commandId = commandId;
206212
this.digestUtil = digestUtil;
@@ -343,7 +349,8 @@ Cache<Object, CompletableFuture<MerkleTree>> getMerkleTreeCache() {
343349
return merkleTreeCache;
344350
}
345351

346-
private SortedMap<PathFragment, ActionInput> buildOutputDirMap(Spawn spawn) {
352+
private SortedMap<PathFragment, ActionInput> buildOutputDirMap(
353+
Spawn spawn, RemotePathResolver remotePathResolver) {
347354
TreeMap<PathFragment, ActionInput> outputDirMap = new TreeMap<>();
348355
for (ActionInput output : spawn.getOutputFiles()) {
349356
if (output instanceof Artifact && ((Artifact) output).isTreeArtifact()) {
@@ -360,12 +367,14 @@ private MerkleTree buildInputMerkleTree(
360367
Spawn spawn,
361368
SpawnExecutionContext context,
362369
ToolSignature toolSignature,
363-
@Nullable SpawnScrubber spawnScrubber)
370+
@Nullable SpawnScrubber spawnScrubber,
371+
RemotePathResolver remotePathResolver)
364372
throws IOException, ForbiddenActionInputException {
365373
// Add output directories to inputs so that they are created as empty directories by the
366374
// executor. The spec only requires the executor to create the parent directory of an output
367375
// directory, which differs from the behavior of both local and sandboxed execution.
368-
SortedMap<PathFragment, ActionInput> outputDirMap = buildOutputDirMap(spawn);
376+
SortedMap<PathFragment, ActionInput> outputDirMap =
377+
buildOutputDirMap(spawn, remotePathResolver);
369378
boolean useMerkleTreeCache = remoteOptions.remoteMerkleTreeCache;
370379
if (toolSignature != null || spawnScrubber != null) {
371380
// The Merkle tree cache is not yet compatible with scrubbing or marking tool files.
@@ -531,10 +540,16 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context
531540
throws IOException, ExecException, ForbiddenActionInputException, InterruptedException {
532541
remoteActionBuildingSemaphore.acquire();
533542
try {
543+
// Create a remote path resolver that is aware of the spawn's path mapper, which rewrites
544+
// the paths of the inputs and outputs as well as paths appearing in the command line for
545+
// execution. This is necessary to ensure that artifacts are correctly emitted into and staged
546+
// from the unmapped location locally.
547+
RemotePathResolver remotePathResolver =
548+
RemotePathResolver.createMapped(baseRemotePathResolver, execRoot, spawn.getPathMapper());
534549
ToolSignature toolSignature = getToolSignature(spawn, context);
535550
SpawnScrubber spawnScrubber = scrubber != null ? scrubber.forSpawn(spawn) : null;
536551
final MerkleTree merkleTree =
537-
buildInputMerkleTree(spawn, context, toolSignature, spawnScrubber);
552+
buildInputMerkleTree(spawn, context, toolSignature, spawnScrubber, remotePathResolver);
538553

539554
// Get the remote platform properties.
540555
Platform platform;
@@ -751,7 +766,8 @@ private ListenableFuture<FileMetadata> downloadFile(
751766
RemoteActionExecutionContext context,
752767
ProgressStatusListener progressStatusListener,
753768
FileMetadata file,
754-
Path tmpPath) {
769+
Path tmpPath,
770+
RemotePathResolver remotePathResolver) {
755771
checkNotNull(remoteCache, "remoteCache can't be null");
756772

757773
try {
@@ -992,7 +1008,9 @@ private DirectoryMetadata parseDirectory(
9921008
}
9931009

9941010
ActionResultMetadata parseActionResultMetadata(
995-
RemoteActionExecutionContext context, RemoteActionResult result)
1011+
RemoteActionExecutionContext context,
1012+
RemoteActionResult result,
1013+
RemotePathResolver remotePathResolver)
9961014
throws IOException, InterruptedException {
9971015
checkNotNull(remoteCache, "remoteCache can't be null");
9981016

@@ -1097,7 +1115,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
10971115

10981116
ActionResultMetadata metadata;
10991117
try (SilentCloseable c = Profiler.instance().profile("Remote.parseActionResultMetadata")) {
1100-
metadata = parseActionResultMetadata(context, result);
1118+
metadata = parseActionResultMetadata(context, result, action.getRemotePathResolver());
11011119
}
11021120

11031121
// The expiration time for remote cache entries.
@@ -1134,7 +1152,9 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
11341152
if (!isInMemoryOutputFile && shouldDownload(result, execPath)) {
11351153
Path tmpPath = tempPathGenerator.generateTempPath();
11361154
realToTmpPath.put(file.path, tmpPath);
1137-
downloadsBuilder.add(downloadFile(context, progressStatusListener, file, tmpPath));
1155+
downloadsBuilder.add(
1156+
downloadFile(
1157+
context, progressStatusListener, file, tmpPath, action.getRemotePathResolver()));
11381158
} else {
11391159
remoteActionFileSystem.injectRemoteFile(
11401160
file.path().asFragment(),
@@ -1164,7 +1184,9 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
11641184
if (shouldDownload(result, file.path.relativeTo(execRoot))) {
11651185
Path tmpPath = tempPathGenerator.generateTempPath();
11661186
realToTmpPath.put(file.path, tmpPath);
1167-
downloadsBuilder.add(downloadFile(context, progressStatusListener, file, tmpPath));
1187+
downloadsBuilder.add(
1188+
downloadFile(
1189+
context, progressStatusListener, file, tmpPath, action.getRemotePathResolver()));
11681190
} else {
11691191
remoteActionFileSystem.injectRemoteFile(
11701192
file.path().asFragment(),
@@ -1322,7 +1344,7 @@ private Single<UploadManifest> buildUploadManifestAsync(
13221344
remoteOptions,
13231345
remoteCache.getCacheCapabilities(),
13241346
digestUtil,
1325-
remotePathResolver,
1347+
action.getRemotePathResolver(),
13261348
action.getActionKey(),
13271349
action.getAction(),
13281350
action.getCommand(),
@@ -1442,7 +1464,9 @@ public void uploadInputsIfNotPresent(RemoteAction action, boolean force)
14421464
SpawnExecutionContext context = action.getSpawnExecutionContext();
14431465
ToolSignature toolSignature = getToolSignature(spawn, context);
14441466
SpawnScrubber spawnScrubber = scrubber != null ? scrubber.forSpawn(spawn) : null;
1445-
merkleTree = buildInputMerkleTree(spawn, context, toolSignature, spawnScrubber);
1467+
merkleTree =
1468+
buildInputMerkleTree(
1469+
spawn, context, toolSignature, spawnScrubber, action.getRemotePathResolver());
14461470
}
14471471

14481472
remoteExecutionCache.ensureInputsPresent(

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

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,16 +15,20 @@
1515

1616
import static com.google.common.base.Preconditions.checkNotNull;
1717

18+
import com.google.common.base.Preconditions;
1819
import com.google.devtools.build.lib.actions.ActionInput;
1920
import com.google.devtools.build.lib.actions.ActionInputHelper;
2021
import com.google.devtools.build.lib.actions.ForbiddenActionInputException;
22+
import com.google.devtools.build.lib.actions.PathMapper;
2123
import com.google.devtools.build.lib.actions.Spawn;
2224
import com.google.devtools.build.lib.exec.SpawnInputExpander;
25+
import com.google.devtools.build.lib.exec.SpawnInputExpander.InputVisitor;
2326
import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
2427
import com.google.devtools.build.lib.vfs.Path;
2528
import com.google.devtools.build.lib.vfs.PathFragment;
2629
import java.io.IOException;
2730
import java.util.SortedMap;
31+
import java.util.concurrent.ConcurrentHashMap;
2832

2933
/**
3034
* A {@link RemotePathResolver} is used to resolve input/output paths for remote execution from
@@ -225,4 +229,70 @@ public Path outputPathToLocalPath(ActionInput actionInput) {
225229
return ActionInputHelper.toInputPath(actionInput, execRoot);
226230
}
227231
}
232+
233+
/**
234+
* Adapts a given base {@link RemotePathResolver} to also apply a {@link PathMapper} to map (and
235+
* inverse map) paths.
236+
*/
237+
static RemotePathResolver createMapped(
238+
RemotePathResolver base, Path execRoot, PathMapper pathMapper) {
239+
if (pathMapper.isNoop()) {
240+
return base;
241+
}
242+
return new RemotePathResolver() {
243+
private final ConcurrentHashMap<PathFragment, PathFragment> inverse =
244+
new ConcurrentHashMap<>();
245+
246+
@Override
247+
public String getWorkingDirectory() {
248+
return base.getWorkingDirectory();
249+
}
250+
251+
@Override
252+
public SortedMap<PathFragment, ActionInput> getInputMapping(
253+
SpawnExecutionContext context, boolean willAccessRepeatedly)
254+
throws IOException, ForbiddenActionInputException {
255+
return base.getInputMapping(context, willAccessRepeatedly);
256+
}
257+
258+
@Override
259+
public void walkInputs(Spawn spawn, SpawnExecutionContext context, InputVisitor visitor)
260+
throws IOException, ForbiddenActionInputException {
261+
base.walkInputs(spawn, context, visitor);
262+
}
263+
264+
@Override
265+
public String localPathToOutputPath(Path path) {
266+
return localPathToOutputPath(path.relativeTo(execRoot));
267+
}
268+
269+
@Override
270+
public String localPathToOutputPath(PathFragment execPath) {
271+
return base.localPathToOutputPath(map(execPath));
272+
}
273+
274+
@Override
275+
public Path outputPathToLocalPath(String outputPath) {
276+
return execRoot.getRelative(
277+
inverseMap(base.outputPathToLocalPath(outputPath).relativeTo(execRoot)));
278+
}
279+
280+
private PathFragment map(PathFragment path) {
281+
PathFragment mappedPath = pathMapper.map(path);
282+
PathFragment previousPath = inverse.put(mappedPath, path);
283+
Preconditions.checkState(
284+
previousPath == null || previousPath.equals(path),
285+
"Two different paths %s and %s map to the same path %s",
286+
previousPath,
287+
path,
288+
mappedPath);
289+
return mappedPath;
290+
}
291+
292+
private PathFragment inverseMap(PathFragment path) {
293+
return Preconditions.checkNotNull(
294+
inverse.get(path), "Failed to find original path for mapped path %s", path);
295+
}
296+
};
297+
}
228298
}

‎src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java‎

Lines changed: 101 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,8 @@
115115
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
116116
import com.google.devtools.common.options.Options;
117117
import com.google.protobuf.ByteString;
118+
import com.google.testing.junit.testparameterinjector.TestParameter;
119+
import com.google.testing.junit.testparameterinjector.TestParameterInjector;
118120
import java.io.IOException;
119121
import java.util.Random;
120122
import java.util.concurrent.CountDownLatch;
@@ -126,14 +128,13 @@
126128
import org.junit.Rule;
127129
import org.junit.Test;
128130
import org.junit.runner.RunWith;
129-
import org.junit.runners.JUnit4;
130131
import org.mockito.ArgumentMatchers;
131132
import org.mockito.Mock;
132133
import org.mockito.junit.MockitoJUnit;
133134
import org.mockito.junit.MockitoRule;
134135

135136
/** Tests for {@link RemoteExecutionService}. */
136-
@RunWith(JUnit4.class)
137+
@RunWith(TestParameterInjector.class)
137138
public class RemoteExecutionServiceTest {
138139
@Rule public final MockitoRule mockito = MockitoJUnit.rule();
139140
@Rule public final RxNoGlobalErrorsRule rxNoGlobalErrorsRule = new RxNoGlobalErrorsRule();
@@ -1540,6 +1541,49 @@ public void downloadOutputs_missingMandatoryOutputs_reportError() throws Excepti
15401541
assertThat(error).hasMessageThat().containsMatch("expected output .+ does not exist.");
15411542
}
15421543

1544+
@Test
1545+
public void downloadOutputs_pathUnmapped() throws Exception {
1546+
// Test that the output of a remote action with path mapping applied is downloaded into the
1547+
// correct unmapped local path.
1548+
Digest d1 = cache.addContents(remoteActionExecutionContext, "content1");
1549+
Digest d2 = cache.addContents(remoteActionExecutionContext, "content2");
1550+
Artifact output1 = ActionsTestUtil.createArtifact(artifactRoot, "bin/config/dir/output1");
1551+
Artifact output2 = ActionsTestUtil.createArtifact(artifactRoot, "bin/other_dir/output2");
1552+
ActionResult r =
1553+
ActionResult.newBuilder()
1554+
.setExitCode(0)
1555+
// The action result includes the mapped paths.
1556+
.addOutputFiles(
1557+
OutputFile.newBuilder().setPath("outputs/bin/dir/output1").setDigest(d1))
1558+
.addOutputFiles(
1559+
OutputFile.newBuilder().setPath("outputs/bin/other_dir/output2").setDigest(d2))
1560+
.build();
1561+
PathMapper pathMapper =
1562+
execPath -> PathFragment.create(execPath.getPathString().replaceAll("config/", ""));
1563+
Spawn spawn =
1564+
new SpawnBuilder("unused")
1565+
.withOutput(output1)
1566+
.withOutput(output2)
1567+
.setPathMapper(pathMapper)
1568+
.build();
1569+
RemoteActionResult result = RemoteActionResult.createFromCache(CachedActionResult.remote(r));
1570+
FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
1571+
when(remoteOutputChecker.shouldDownloadOutput(output1.getExecPath())).thenReturn(true);
1572+
when(remoteOutputChecker.shouldDownloadOutput(output2.getExecPath())).thenReturn(true);
1573+
RemoteExecutionService service = newRemoteExecutionService();
1574+
RemoteAction action = service.buildRemoteAction(spawn, context);
1575+
1576+
InMemoryOutput inMemoryOutput = service.downloadOutputs(action, result);
1577+
1578+
assertThat(inMemoryOutput).isNull();
1579+
RemoteActionFileSystem actionFs = context.getActionFileSystem();
1580+
assertThat(actionFs.getDigest(output1.getPath().asFragment())).isEqualTo(toBinaryDigest(d1));
1581+
assertThat(readContent(output1.getPath(), UTF_8)).isEqualTo("content1");
1582+
assertThat(actionFs.getDigest(output2.getPath().asFragment())).isEqualTo(toBinaryDigest(d2));
1583+
assertThat(readContent(output2.getPath(), UTF_8)).isEqualTo("content2");
1584+
assertThat(context.isLockOutputFilesCalled()).isTrue();
1585+
}
1586+
15431587
@Test
15441588
public void uploadOutputs_uploadDirectory_works() throws Exception {
15451589
// Test that uploading a directory works.
@@ -2266,6 +2310,61 @@ public void buildRemoteActionWithScrubbing() throws Exception {
22662310
.toByteString());
22672311
}
22682312

2313+
@Test
2314+
public void buildRemoteActionWithPathMapping(@TestParameter boolean remoteMerkleTreeCache)
2315+
throws Exception {
2316+
remoteOptions.remoteMerkleTreeCache = remoteMerkleTreeCache;
2317+
2318+
var mappedInput = ActionsTestUtil.createArtifact(artifactRoot, "bin/config/input1");
2319+
fakeFileCache.createScratchInput(mappedInput, "value1");
2320+
var unmappedInput = ActionsTestUtil.createArtifact(artifactRoot, "bin/input2");
2321+
fakeFileCache.createScratchInput(unmappedInput, "value2");
2322+
var outputDir =
2323+
ActionsTestUtil.createTreeArtifactWithGeneratingAction(
2324+
artifactRoot, "bin/config/output_dir");
2325+
PathMapper pathMapper =
2326+
execPath -> PathFragment.create(execPath.getPathString().replaceAll("config/", ""));
2327+
Spawn spawn =
2328+
new SpawnBuilder("unused")
2329+
.withInputs(mappedInput, unmappedInput)
2330+
.withOutputs("outputs/bin/config/dir/output1", "outputs/bin/other_dir/output2")
2331+
.withOutputs(outputDir)
2332+
.setPathMapper(pathMapper)
2333+
.build();
2334+
FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
2335+
RemoteExecutionService service = newRemoteExecutionService(remoteOptions);
2336+
2337+
// Check that inputs and outputs of the remote action are mapped correctly.
2338+
var remoteAction = service.buildRemoteAction(spawn, context);
2339+
assertThat(remoteAction.getInputMap(false))
2340+
.containsExactly(
2341+
PathFragment.create("outputs/bin/input1"), mappedInput,
2342+
PathFragment.create("outputs/bin/input2"), unmappedInput);
2343+
assertThat(remoteAction.getCommand().getOutputFilesList())
2344+
.containsExactly("outputs/bin/dir/output1", "outputs/bin/other_dir/output2");
2345+
assertThat(remoteAction.getCommand().getOutputDirectoriesList())
2346+
.containsExactly("outputs/bin/output_dir");
2347+
assertThat(remoteAction.getCommand().getOutputPathsList())
2348+
.containsExactly(
2349+
"outputs/bin/dir/output1", "outputs/bin/other_dir/output2", "outputs/bin/output_dir");
2350+
2351+
// Check that the Merkle tree nodes are mapped correctly, including the output directory.
2352+
var merkleTree = remoteAction.getMerkleTree();
2353+
var outputsDirectory =
2354+
merkleTree.getDirectoryByDigest(merkleTree.getRootProto().getDirectories(0).getDigest());
2355+
assertThat(outputsDirectory.getDirectoriesCount()).isEqualTo(1);
2356+
var binDirectory =
2357+
merkleTree.getDirectoryByDigest(outputsDirectory.getDirectories(0).getDigest());
2358+
assertThat(
2359+
binDirectory.getFilesList().stream().map(FileNode::getName).collect(toImmutableList()))
2360+
.containsExactly("input1", "input2");
2361+
assertThat(
2362+
binDirectory.getDirectoriesList().stream()
2363+
.map(DirectoryNode::getName)
2364+
.collect(toImmutableList()))
2365+
.containsExactly("output_dir");
2366+
}
2367+
22692368
private Spawn newSpawnFromResult(RemoteActionResult result) {
22702369
return newSpawnFromResult(ImmutableMap.of(), result);
22712370
}

0 commit comments

Comments
 (0)