Skip to content

Commit

Permalink
Resolve symlinks correctly even when they cross filesystems.
Browse files Browse the repository at this point in the history 
Instead of delegating RemoteActionFileSystem#resolveSymbolicLinks to the local filesystem, use its base implementation together with a custom implementation of FileSystem#resolveOneLink which is able to stitch together the local and remote filesystems.

In addition, have every filesystem method that needs to resolve symlinks symlinks call resolveSymbolicLinks explicitly (as delegating to the underlying filesystems would cause cross-filesystem symlinks not to be followed correctly).

The end goal of these changes is to let SkyframeActionExecutor traverse the RemoteActionFileSystem normally when collecting metadata for non-symlink outputs materialized as symlinks, instead of relying on the latter to provide pre-constructed metadata. This is only a step on the way there; the way SkyframeActionExecutor collects metadata remains unchanged for now.

PiperOrigin-RevId: 552743574
Change-Id: I5921ded8f7b9c51c31406daf0441ee3c2e17308b
  • Loading branch information
@tjgq @copybara-github
tjgq authored and copybara-github committed Aug 1, 2023
1 parent 5563046 commit ef9c3b3
Show file tree
Hide file tree
Showing 3 changed files with 250 additions and 44 deletions.
112 changes: 74 additions & 38 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,16 @@ public class RemoteActionFileSystem extends AbstractFileSystemWithCustomStat {
@Nullable private ActionExecutionMetadata action = null;
@Nullable private MetadataInjector metadataInjector = null;

/** Describes how to handle symlinks when calling {@link #statUnchecked}. */
private enum FollowMode {
/** Dereference the entire path. This is equivalent to {@link Symlinks.FOLLOW}. */
FOLLOW_ALL,
/** Dereference the parent path. This is equivalent to {@link Symlinks.NOFOLLOW}. */
FOLLOW_PARENT,
/** Do not dereference. This is only used internally to resolve symlinks efficiently. */
FOLLOW_NONE
};

RemoteActionFileSystem(
FileSystem localFs,
PathFragment execRootFragment,
Expand Down Expand Up @@ -144,7 +154,7 @@ boolean isRemote(Path path) {
}

private boolean isRemote(PathFragment path) {
var status = statInMemory(path, /* followSymlinks= */ true);
var status = statInMemory(path, FollowMode.FOLLOW_ALL);
return (status instanceof FileStatusWithMetadata)
&& ((FileStatusWithMetadata) status).getMetadata().isRemote();
}
Expand Down Expand Up @@ -243,7 +253,7 @@ private void maybeInjectMetadataForSymlinkOrDownload(PathFragment linkPath, Arti
} else {
RemoteFileArtifactValue metadata = null;

var status = statInMemory(targetPath, /* followSymlinks= */ true);
var status = statInMemory(targetPath, FollowMode.FOLLOW_ALL);
if (status instanceof FileStatusWithMetadata
&& ((FileStatusWithMetadata) status).getMetadata().isRemote()) {
metadata = (RemoteFileArtifactValue) ((FileStatusWithMetadata) status).getMetadata();
Expand Down Expand Up @@ -340,7 +350,7 @@ public byte[] getxattr(PathFragment path, String name, boolean followSymlinks)

@Override
protected byte[] getFastDigest(PathFragment path) throws IOException {
var stat = statInMemory(path, /* followSymlinks= */ true);
var stat = statInMemory(path, FollowMode.FOLLOW_ALL);
if (stat instanceof FileStatusWithDigest) {
return ((FileStatusWithDigest) stat).getDigest();
}
Expand All @@ -349,7 +359,7 @@ protected byte[] getFastDigest(PathFragment path) throws IOException {

@Override
protected byte[] getDigest(PathFragment path) throws IOException {
var status = statInMemory(path, /* followSymlinks= */ true);
var status = statInMemory(path, FollowMode.FOLLOW_ALL);
if (status instanceof FileStatusWithDigest) {
return ((FileStatusWithDigest) status).getDigest();
}
Expand All @@ -361,7 +371,7 @@ protected byte[] getDigest(PathFragment path) throws IOException {
// permissions.

private boolean existsInMemory(PathFragment path) {
return statInMemory(path, /* followSymlinks= */ true) != null;
return statInMemory(path, FollowMode.FOLLOW_ALL) != null;
}

@Override
Expand Down Expand Up @@ -443,43 +453,45 @@ protected void chmod(PathFragment path, int mode) throws IOException {

@Override
protected PathFragment readSymbolicLink(PathFragment path) throws IOException {
if (isRemote(path)) {
// We don't support symlinks as remote action outputs.
throw new IOException(path + " is not a symbolic link");
if (isOutput(path)) {
try {
return remoteOutputTree.getPath(path).readSymbolicLink();
} catch (FileNotFoundException e) {
// Intentionally ignored.
}
}
return localFs.getPath(path).readSymbolicLink();
}

@Override
protected void createSymbolicLink(PathFragment linkPath, PathFragment targetFragment)
throws IOException {
localFs.getPath(linkPath).createSymbolicLink(targetFragment);
if (isOutput(linkPath)) {
remoteOutputTree.getPath(linkPath).createSymbolicLink(targetFragment);
}

localFs.getPath(linkPath).createSymbolicLink(targetFragment);
}

@Nullable
@Override
protected Path resolveSymbolicLinks(PathFragment path) throws IOException {
return localFs.getPath(path).resolveSymbolicLinks();
protected PathFragment resolveOneLink(PathFragment path) throws IOException {
// The base implementation attempts to readSymbolicLink first and falls back to stat, but that
// unnecessarily allocates a NotASymlinkException in the overwhelmingly likely non-symlink case.
// It's more efficient to stat unconditionally.
//
// The parent path has already been canonicalized, so FOLLOW_NONE is effectively the same as
// FOLLOW_PARENT, but much more efficient as it doesn't call stat recursively.
var stat = statUnchecked(path, FollowMode.FOLLOW_NONE);
return stat.isSymbolicLink() ? readSymbolicLink(path) : null;
}

// -------------------- Implementations based on stat() --------------------

@Override
protected long getLastModifiedTime(PathFragment path, boolean followSymlinks) throws IOException {
try {
// We can't get mtime for a remote file, use mtime of in-memory file node instead.
return remoteOutputTree
.getPath(path)
.getLastModifiedTime(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW);
} catch (FileNotFoundException e) {
// Intentionally ignored
}

return localFs
.getPath(path)
.getLastModifiedTime(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW);
FileStatus stat = stat(path, followSymlinks);
return stat.getLastModifiedTime();
}

@Override
Expand All @@ -497,11 +509,6 @@ protected boolean exists(PathFragment path, boolean followSymlinks) {
}
}

@Override
public boolean exists(PathFragment path) {
return exists(path, /* followSymlinks= */ true);
}

@Nullable
@Override
protected FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws IOException {
Expand All @@ -524,15 +531,30 @@ protected FileStatus statNullable(PathFragment path, boolean followSymlinks) {

@Override
protected FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException {
var status = statInMemory(path, followSymlinks);
return statUnchecked(path, followSymlinks ? FollowMode.FOLLOW_ALL : FollowMode.FOLLOW_PARENT);
}

@Nullable
private FileStatus statUnchecked(PathFragment path, FollowMode followMode) throws IOException {
if (followMode == FollowMode.FOLLOW_ALL) {
path = resolveSymbolicLinks(path).asFragment();
} else if (followMode == FollowMode.FOLLOW_PARENT) {
PathFragment parent = path.getParentDirectory();
if (parent != null) {
path = resolveSymbolicLinks(parent).asFragment().getChild(path.getBaseName());
}
}

var status = statInMemory(path, followMode);
if (status != null) {
return status;
}
return localFs.getPath(path).stat(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW);
// The path has already been canonicalized above.
return localFs.getPath(path).stat(Symlinks.NOFOLLOW);
}

@Nullable
private FileStatus statInMemory(PathFragment path, boolean followSymlinks) {
private FileStatus statInMemory(PathFragment path, FollowMode followMode) {
if (path.startsWith(execRoot)) {
var execPath = path.relativeTo(execRoot);
var metadata = inputArtifactData.getMetadata(execPath);
Expand All @@ -541,7 +563,8 @@ private FileStatus statInMemory(PathFragment path, boolean followSymlinks) {
}
}

return remoteOutputTree.statNullable(path, followSymlinks);
return remoteOutputTree.statNullable(
path, /* followSymlinks= */ followMode == FollowMode.FOLLOW_ALL);
}

private static FileStatusWithMetadata statFromMetadata(FileArtifactValue m) {
Expand Down Expand Up @@ -730,12 +753,12 @@ protected Collection<Dirent> readdir(PathFragment path, boolean followSymlinks)

boolean existsRemotely = false;

path = resolveSymbolicLinks(path).asFragment();

if (isOutput(path)) {
try {
for (var entry :
remoteOutputTree
.getPath(path)
.readdir(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW)) {
for (var entry : remoteOutputTree.getPath(path).readdir(Symlinks.NOFOLLOW)) {
entry = maybeFollowSymlinkForDirent(path, entry, followSymlinks);
entries.put(entry.getName(), entry);
}
existsRemotely = true;
Expand All @@ -745,8 +768,8 @@ protected Collection<Dirent> readdir(PathFragment path, boolean followSymlinks)
}

try {
for (var entry :
localFs.getPath(path).readdir(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW)) {
for (var entry : localFs.getPath(path).readdir(Symlinks.NOFOLLOW)) {
entry = maybeFollowSymlinkForDirent(path, entry, followSymlinks);
entries.put(entry.getName(), entry);
}
} catch (FileNotFoundException e) {
Expand All @@ -759,6 +782,19 @@ protected Collection<Dirent> readdir(PathFragment path, boolean followSymlinks)
return ImmutableList.sortedCopyOf(entries.values());
}

private Dirent maybeFollowSymlinkForDirent(
PathFragment dirPath, Dirent entry, boolean followSymlinks) {
if (!followSymlinks || !entry.getType().equals(Dirent.Type.SYMLINK)) {
return entry;
}
PathFragment path = dirPath.getChild(entry.getName());
FileStatus st = statNullable(path, /* followSymlinks= */ true);
if (st == null) {
return new Dirent(entry.getName(), Dirent.Type.UNKNOWN);
}
return new Dirent(entry.getName(), direntFromStat(st));
}

/*
* -------------------- TODO(buchgr): Not yet implemented --------------------
*
Expand Down
1 change: 1 addition & 0 deletions src/test/java/com/google/devtools/build/lib/remote/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ java_test(
"@googleapis//:google_rpc_code_java_proto",
"@googleapis//:google_rpc_error_details_java_proto",
"@googleapis//:google_rpc_status_java_proto",
"@maven//:com_google_testparameterinjector_test_parameter_injector",
"@remoteapis//:build_bazel_remote_execution_v2_remote_execution_java_grpc",
"@remoteapis//:build_bazel_remote_execution_v2_remote_execution_java_proto",
"@remoteapis//:build_bazel_semver_semver_java_proto",
Expand Down
Loading

0 comments on commit ef9c3b3

Please sign in to comment.