Skip to content

Commit

Permalink
Save injected metadata in RemoteActionFileSystem
Browse files Browse the repository at this point in the history 
So that spawn outputs can be accessed among Spwans within the same action using the `FileSystem` API.

This allow us to revert the hack we introduced in #12590. Also fixes the issue described by #15711.

Closes #15711.

Closes #16123.

PiperOrigin-RevId: 469133936
Change-Id: Ide5bcfa0fe2c6a3806d333cd61270e411aa78f80
  • Loading branch information
@coeuvre @copybara-github
coeuvre authored and copybara-github committed Aug 22, 2022
1 parent 5116e92 commit 79b5b5d
Show file tree
Hide file tree
Showing 16 changed files with 176 additions and 201 deletions.
7 changes: 7 additions & 0 deletions src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
import com.google.devtools.build.lib.server.FailureDetails.Spawn.Code;
import com.google.devtools.build.lib.util.CommandFailureUtils;
import com.google.devtools.build.lib.util.io.FileOutErr;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.IOException;
Expand Down Expand Up @@ -353,5 +354,11 @@ public void checkForLostInputs() throws LostInputsExecException {
throw e.toExecException();
}
}

@Nullable
@Override
public FileSystem getActionFileSystem() {
return actionExecutionContext.getActionFileSystem();
}
}
}
5 changes: 5 additions & 0 deletions src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import com.google.devtools.build.lib.actions.cache.MetadataInjector;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.util.io.FileOutErr;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.IOException;
Expand Down Expand Up @@ -270,6 +271,10 @@ SortedMap<PathFragment, ActionInput> getInputMapping(PathFragment baseDirectory)

/** Throws if rewinding is enabled and lost inputs have been detected. */
void checkForLostInputs() throws LostInputsExecException;

/** Returns action-scoped file system or {@code null} if it doesn't exist. */
@Nullable
FileSystem getActionFileSystem();
}

/**
Expand Down
149 changes: 5 additions & 144 deletions src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,22 +26,17 @@
import com.google.devtools.build.lib.actions.ActionExecutionContext;
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.ActionInputHelper;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
import com.google.devtools.build.lib.actions.EnvironmentalExecException;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.ExecutionRequirements;
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.SimpleSpawn;
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnContinuation;
import com.google.devtools.build.lib.actions.SpawnMetrics;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.actions.TestExecException;
import com.google.devtools.build.lib.actions.cache.MetadataHandler;
import com.google.devtools.build.lib.analysis.actions.SpawnAction;
import com.google.devtools.build.lib.analysis.test.TestAttempt;
import com.google.devtools.build.lib.analysis.test.TestResult;
Expand All @@ -57,7 +52,6 @@
import com.google.devtools.build.lib.server.FailureDetails.Execution.Code;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
import com.google.devtools.build.lib.server.FailureDetails.TestAction;
import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.io.FileOutErr;
import com.google.devtools.build.lib.vfs.FileStatus;
Expand All @@ -76,8 +70,6 @@
import java.util.List;
import java.util.Map;
import java.util.TreeMap;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import javax.annotation.Nullable;

/** Runs TestRunnerAction actions. */
Expand Down Expand Up @@ -147,7 +139,7 @@ public TestRunnerSpawn createTestRunnerSpawn(
ImmutableMap.of(),
/*inputs=*/ action.getInputs(),
NestedSetBuilder.emptySet(Order.STABLE_ORDER),
createSpawnOutputs(action),
ImmutableSet.copyOf(action.getSpawnOutputs()),
/*mandatoryOutputs=*/ ImmutableSet.of(),
localResourcesSupplier);
Path execRoot = actionExecutionContext.getExecRoot();
Expand All @@ -159,21 +151,6 @@ public TestRunnerSpawn createTestRunnerSpawn(
action, actionExecutionContext, spawn, tmpDir, workingDirectory, execRoot);
}

private ImmutableSet<ActionInput> createSpawnOutputs(TestRunnerAction action) {
ImmutableSet.Builder<ActionInput> builder = ImmutableSet.builder();
for (ActionInput output : action.getSpawnOutputs()) {
if (output.getExecPath().equals(action.getXmlOutputPath())) {
// HACK: Convert type of test.xml from BasicActionInput to DerivedArtifact. We want to
// inject metadata of test.xml if it is generated remotely and it's currently only possible
// to inject Artifact.
builder.add(createArtifactOutput(action, output.getExecPath()));
} else {
builder.add(output);
}
}
return builder.build();
}

private static ImmutableList<Pair<String, Path>> renameOutputs(
ActionExecutionContext actionExecutionContext,
TestRunnerAction action,
Expand Down Expand Up @@ -326,83 +303,6 @@ private static Map<String, String> setupEnvironment(
action, clientEnv, getTimeout(action), runfilesDir.relativeTo(execRoot), relativeTmpDir);
}

static class TestMetadataHandler implements MetadataHandler {
private final MetadataHandler metadataHandler;
private final ImmutableSet<Artifact> outputs;
private final ConcurrentMap<Artifact, FileArtifactValue> fileMetadataMap =
new ConcurrentHashMap<>();

TestMetadataHandler(MetadataHandler metadataHandler, ImmutableSet<Artifact> outputs) {
this.metadataHandler = metadataHandler;
this.outputs = outputs;
}

@Nullable
@Override
public ActionInput getInput(String execPath) {
return metadataHandler.getInput(execPath);
}

@Nullable
@Override
public FileArtifactValue getMetadata(ActionInput input) throws IOException {
return metadataHandler.getMetadata(input);
}

@Override
public void setDigestForVirtualArtifact(Artifact artifact, byte[] digest) {
metadataHandler.setDigestForVirtualArtifact(artifact, digest);
}

@Override
public FileArtifactValue constructMetadataForDigest(
Artifact output, FileStatus statNoFollow, byte[] injectedDigest) throws IOException {
return metadataHandler.constructMetadataForDigest(output, statNoFollow, injectedDigest);
}

@Override
public ImmutableSet<TreeFileArtifact> getTreeArtifactChildren(SpecialArtifact treeArtifact) {
return metadataHandler.getTreeArtifactChildren(treeArtifact);
}

@Override
public TreeArtifactValue getTreeArtifactValue(SpecialArtifact treeArtifact) throws IOException {
return metadataHandler.getTreeArtifactValue(treeArtifact);
}

@Override
public void markOmitted(Artifact output) {
metadataHandler.markOmitted(output);
}

@Override
public boolean artifactOmitted(Artifact artifact) {
return metadataHandler.artifactOmitted(artifact);
}

@Override
public void resetOutputs(Iterable<? extends Artifact> outputs) {
metadataHandler.resetOutputs(outputs);
}

@Override
public void injectFile(Artifact output, FileArtifactValue metadata) {
if (outputs.contains(output)) {
metadataHandler.injectFile(output, metadata);
}
fileMetadataMap.put(output, metadata);
}

@Override
public void injectTree(SpecialArtifact output, TreeArtifactValue tree) {
metadataHandler.injectTree(output, tree);
}

public boolean fileInjected(Artifact output) {
return fileMetadataMap.containsKey(output);
}
}

private TestAttemptContinuation beginTestAttempt(
TestRunnerAction testAction,
Spawn spawn,
Expand All @@ -420,25 +320,12 @@ private TestAttemptContinuation beginTestAttempt(
Reporter.outErrForReporter(actionExecutionContext.getEventHandler()), out);
}

// We use TestMetadataHandler here mainly because the one provided by actionExecutionContext
// doesn't allow to inject undeclared outputs and test.xml is undeclared by the test action.
TestMetadataHandler testMetadataHandler = null;
if (actionExecutionContext.getMetadataHandler() != null) {
testMetadataHandler =
new TestMetadataHandler(
actionExecutionContext.getMetadataHandler(), testAction.getOutputs());
}

long startTimeMillis = actionExecutionContext.getClock().currentTimeMillis();
SpawnStrategyResolver resolver = actionExecutionContext.getContext(SpawnStrategyResolver.class);
SpawnContinuation spawnContinuation;
try {
spawnContinuation =
resolver.beginExecution(
spawn,
actionExecutionContext
.withFileOutErr(testOutErr)
.withMetadataHandler(testMetadataHandler));
resolver.beginExecution(spawn, actionExecutionContext.withFileOutErr(testOutErr));
} catch (InterruptedException e) {
if (streamed != null) {
streamed.close();
Expand All @@ -448,7 +335,6 @@ private TestAttemptContinuation beginTestAttempt(
}
return new BazelTestAttemptContinuation(
testAction,
testMetadataHandler,
actionExecutionContext,
spawn,
resolvedPaths,
Expand Down Expand Up @@ -559,12 +445,6 @@ private static com.google.protobuf.Duration toProtoDuration(Duration d) {
return Durations.fromNanos(d.toNanos());
}

private static Artifact.DerivedArtifact createArtifactOutput(
TestRunnerAction action, PathFragment outputPath) {
Artifact.DerivedArtifact testLog = (Artifact.DerivedArtifact) action.getTestLog();
return DerivedArtifact.create(testLog.getRoot(), outputPath, testLog.getArtifactOwner());
}

/**
* A spawn to generate a test.xml file from the test log. This is only used if the test does not
* generate a test.xml file itself.
Expand Down Expand Up @@ -610,7 +490,7 @@ private static Spawn createXmlGeneratingSpawn(
/*inputs=*/ NestedSetBuilder.create(
Order.STABLE_ORDER, action.getTestXmlGeneratorScript(), action.getTestLog()),
/*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
/*outputs=*/ ImmutableSet.of(createArtifactOutput(action, action.getXmlOutputPath())),
/*outputs=*/ ImmutableSet.of(ActionInputHelper.fromPath(action.getXmlOutputPath())),
/*mandatoryOutputs=*/ null,
SpawnAction.DEFAULT_RESOURCE_SET);
}
Expand Down Expand Up @@ -763,7 +643,6 @@ public void finalizeCancelledTest(List<FailedAttemptResult> failedAttempts) thro

private final class BazelTestAttemptContinuation extends TestAttemptContinuation {
private final TestRunnerAction testAction;
@Nullable private final TestMetadataHandler testMetadataHandler;
private final ActionExecutionContext actionExecutionContext;
private final Spawn spawn;
private final ResolvedPaths resolvedPaths;
Expand All @@ -776,7 +655,6 @@ private final class BazelTestAttemptContinuation extends TestAttemptContinuation

BazelTestAttemptContinuation(
TestRunnerAction testAction,
@Nullable TestMetadataHandler testMetadataHandler,
ActionExecutionContext actionExecutionContext,
Spawn spawn,
ResolvedPaths resolvedPaths,
Expand All @@ -787,7 +665,6 @@ private final class BazelTestAttemptContinuation extends TestAttemptContinuation
TestResultData.Builder testResultDataBuilder,
ImmutableList<SpawnResult> spawnResults) {
this.testAction = testAction;
this.testMetadataHandler = testMetadataHandler;
this.actionExecutionContext = actionExecutionContext;
this.spawn = spawn;
this.resolvedPaths = resolvedPaths;
Expand Down Expand Up @@ -827,7 +704,6 @@ public TestAttemptContinuation execute()
if (!nextContinuation.isDone()) {
return new BazelTestAttemptContinuation(
testAction,
testMetadataHandler,
actionExecutionContext,
spawn,
resolvedPaths,
Expand Down Expand Up @@ -918,7 +794,6 @@ public TestAttemptContinuation execute()
appendCoverageLog(coverageOutErr, fileOutErr);
return new BazelCoveragePostProcessingContinuation(
testAction,
testMetadataHandler,
actionExecutionContext,
spawn,
resolvedPaths,
Expand Down Expand Up @@ -955,20 +830,14 @@ public TestAttemptContinuation execute()
}

Path xmlOutputPath = resolvedPaths.getXmlOutputPath();
boolean testXmlGenerated = xmlOutputPath.exists();
if (!testXmlGenerated && testMetadataHandler != null) {
testXmlGenerated =
testMetadataHandler.fileInjected(
createArtifactOutput(testAction, testAction.getXmlOutputPath()));
}

// If the test did not create a test.xml, and --experimental_split_xml_generation is enabled,
// then we run a separate action to create a test.xml from test.log. We do this as a spawn
// rather than doing it locally in-process, as the test.log file may only exist remotely (when
// remote execution is enabled), and we do not want to have to download it.
if (executionOptions.splitXmlGeneration
&& fileOutErr.getOutputPath().exists()
&& !testXmlGenerated) {
&& !xmlOutputPath.exists()) {
Spawn xmlGeneratingSpawn =
createXmlGeneratingSpawn(testAction, spawn.getEnvironment(), spawnResults.get(0));
SpawnStrategyResolver spawnStrategyResolver =
Expand All @@ -979,10 +848,7 @@ public TestAttemptContinuation execute()
try {
SpawnContinuation xmlContinuation =
spawnStrategyResolver.beginExecution(
xmlGeneratingSpawn,
actionExecutionContext
.withFileOutErr(xmlSpawnOutErr)
.withMetadataHandler(testMetadataHandler));
xmlGeneratingSpawn, actionExecutionContext.withFileOutErr(xmlSpawnOutErr));
return new BazelXmlCreationContinuation(
resolvedPaths, xmlSpawnOutErr, testResultDataBuilder, spawnResults, xmlContinuation);
} catch (InterruptedException e) {
Expand Down Expand Up @@ -1080,7 +946,6 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept

private final class BazelCoveragePostProcessingContinuation extends TestAttemptContinuation {
private final ResolvedPaths resolvedPaths;
@Nullable private final TestMetadataHandler testMetadataHandler;
private final FileOutErr fileOutErr;
private final Closeable streamed;
private final TestResultData.Builder testResultDataBuilder;
Expand All @@ -1092,7 +957,6 @@ private final class BazelCoveragePostProcessingContinuation extends TestAttemptC

BazelCoveragePostProcessingContinuation(
TestRunnerAction testAction,
@Nullable TestMetadataHandler testMetadataHandler,
ActionExecutionContext actionExecutionContext,
Spawn spawn,
ResolvedPaths resolvedPaths,
Expand All @@ -1102,7 +966,6 @@ private final class BazelCoveragePostProcessingContinuation extends TestAttemptC
ImmutableList<SpawnResult> primarySpawnResults,
SpawnContinuation spawnContinuation) {
this.testAction = testAction;
this.testMetadataHandler = testMetadataHandler;
this.actionExecutionContext = actionExecutionContext;
this.spawn = spawn;
this.resolvedPaths = resolvedPaths;
Expand All @@ -1127,7 +990,6 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept
if (!nextContinuation.isDone()) {
return new BazelCoveragePostProcessingContinuation(
testAction,
testMetadataHandler,
actionExecutionContext,
spawn,
resolvedPaths,
Expand Down Expand Up @@ -1164,7 +1026,6 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept

return new BazelTestAttemptContinuation(
testAction,
testMetadataHandler,
actionExecutionContext,
spawn,
resolvedPaths,
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/remote/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/main/java/com/google/devtools/build/lib/vfs:output_service",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//src/main/java/com/google/devtools/build/skyframe",
"//src/main/java/com/google/devtools/common/options",
"//src/main/protobuf:failure_details_java_proto",
"//third_party:auth",
Expand Down
Loading

0 comments on commit 79b5b5d

Please sign in to comment.