Skip to content

Commit

Permalink
Fix --build_event_publish_all_actions with --remote_download_minimal
Browse files Browse the repository at this point in the history
The ActionExecutedEvent references the primaryOutput of the executed
action (see ActionExecutedEvent::referencedLocalFiles).
ByteStreamBuildEventArtifactUploader::upload will try to ensure that
this referenced file is present on the remote cache.  It calls
file.isDirectory(), which will eventually go to
RemoteActionFileSystem::getRemoteInputMetadata.  However,
RemoteActionFileSystem::inputArtifactData only contains metadata about
the input files, not action outputs.

This change proposes a fix, where we make RemoteActionFileSystem
implement MetadataInjector, and inject information about output files
from ActionMetadataHandler::injectRemoteFile.

The implementation is likely not correct for all cases, but demonstrates
the issue and a possible approach to fix it.
  • Loading branch information
scele committed Mar 16, 2020
1 parent 3c89a91 commit f031332
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,13 @@
package com.google.devtools.build.lib.remote;

import com.google.common.base.Preconditions;
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.ActionInputMap;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
import com.google.devtools.build.lib.actions.cache.MetadataInjector;
import com.google.devtools.build.lib.vfs.DelegateFileSystem;
import com.google.devtools.build.lib.vfs.Dirent;
import com.google.devtools.build.lib.vfs.FileStatus;
Expand All @@ -30,6 +34,9 @@
import java.io.InputStream;
import java.nio.channels.ReadableByteChannel;
import java.util.Collection;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import javax.annotation.Nullable;

/**
Expand All @@ -42,12 +49,13 @@
*
* <p>This implementation only supports creating local action outputs.
*/
class RemoteActionFileSystem extends DelegateFileSystem {
class RemoteActionFileSystem extends DelegateFileSystem implements MetadataInjector {

private final Path execRoot;
private final Path outputBase;
private final ActionInputMap inputArtifactData;
private final RemoteActionInputFetcher inputFetcher;
private final ConcurrentMap<String, RemoteFileArtifactValue> injectedMetadata = new ConcurrentHashMap<>();

RemoteActionFileSystem(
FileSystem localDelegate,
Expand Down Expand Up @@ -339,6 +347,10 @@ private RemoteFileArtifactValue getRemoteInputMetadata(String execPathString) {
if (m != null && m.isRemote()) {
return (RemoteFileArtifactValue) m;
}
RemoteFileArtifactValue rm = injectedMetadata.get(execPathString);
if (rm != null) {
return rm;
}
return null;
}

Expand Down Expand Up @@ -383,4 +395,33 @@ protected Collection<Dirent> readdir(Path path, boolean followSymlinks) throws I
protected void createHardLink(Path linkPath, Path originalPath) throws IOException {
super.createHardLink(linkPath, originalPath);
}

// MetadataInjector interface
@Override
public void addExpandedTreeOutput(TreeFileArtifact output) {
throw new UnsupportedOperationException();
}

@Override
public void injectDigest(ActionInput output, FileStatus statNoFollow, byte[] digest) {
throw new UnsupportedOperationException();
}

@Override
public void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex) {
RemoteFileArtifactValue m = new FileArtifactValue.RemoteFileArtifactValue(digest, size, locationIndex);
Path transformedPath = getPath(output.getPath().getPathString());
injectedMetadata.put(execPathString(transformedPath), m);
}

@Override
public void injectRemoteDirectory(
Artifact.SpecialArtifact treeArtifact, Map<PathFragment, RemoteFileArtifactValue> children) {
throw new UnsupportedOperationException();
}

@Override
public void markOmitted(ActionInput output) {
throw new UnsupportedOperationException();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,14 @@
import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
import com.google.devtools.build.lib.actions.cache.DigestUtils;
import com.google.devtools.build.lib.actions.cache.MetadataHandler;
import com.google.devtools.build.lib.actions.cache.MetadataInjector;
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
import com.google.devtools.build.lib.vfs.Dirent;
import com.google.devtools.build.lib.vfs.Dirent.Type;
import com.google.devtools.build.lib.vfs.FileStatus;
import com.google.devtools.build.lib.vfs.FileStatusWithDigest;
import com.google.devtools.build.lib.vfs.FileStatusWithDigestAdapter;
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 com.google.devtools.build.lib.vfs.RootedPath;
Expand Down Expand Up @@ -551,6 +553,13 @@ public void injectRemoteFile(Artifact output, byte[] digest, long size, int loca
Preconditions.checkState(
executionMode.get(), "Tried to inject %s outside of execution", output);
store.injectRemoteFile(output, digest, size, locationIndex);

// Ensure that this artifact is also known to the remote filesystem that
// artifactPathResolver resolves to.
FileSystem fs = artifactPathResolver.toPath(output).getFileSystem();
if (fs instanceof MetadataInjector) {
((MetadataInjector) fs).injectRemoteFile(output, digest, size, locationIndex);
}
}

@Override
Expand Down

0 comments on commit f031332

Please sign in to comment.