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

Save injected metadata in RemoteActionFileSystem #16123

Closed
wants to merge 3 commits into from
Closed

Save injected metadata in RemoteActionFileSystem #16123

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
89607f4
Save injected metadata in RemoteActionFileSystem so that spawn output…
coeuvre Aug 18, 2022
ff17253
Fix tests
coeuvre Aug 18, 2022
04e4f76
Address review comment
coeuvre Aug 18, 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
7 changes: 7 additions & 0 deletions src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
View file
Open in desktop
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
Open in desktop
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 @@ -269,6 +270,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
Open in desktop
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
Open in desktop
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