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

Ensure namedSetOfFiles URIs specify blob type correctly #19044

Closed
wants to merge 1 commit into from
+109 −31
Closed

Ensure namedSetOfFiles URIs specify blob type correctly #19044

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
87 changes: 66 additions & 21 deletions src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@
import static com.google.devtools.build.lib.remote.util.Utils.grpcAwareErrorMessage;

import build.bazel.remote.execution.v2.Digest;
import build.bazel.remote.execution.v2.DigestFunction;
import build.bazel.remote.execution.v2.RequestMetadata;
import com.google.common.base.Ascii;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Maps;
Expand Down Expand Up @@ -135,20 +137,23 @@ private static final class PathMetadata {
private final boolean symlink;
private final boolean remote;
private final boolean omitted;
private final DigestFunction.Value digestFunction;

PathMetadata(
Path path,
Digest digest,
boolean directory,
boolean symlink,
boolean remote,
boolean omitted) {
boolean omitted,
DigestFunction.Value digestFunction) {
this.path = path;
this.digest = digest;
this.directory = directory;
this.symlink = symlink;
this.remote = remote;
this.omitted = omitted;
this.digestFunction = digestFunction;
}

public Path getPath() {
Expand All @@ -174,13 +179,19 @@ public boolean isRemote() {
public boolean isOmitted() {
return omitted;
}

public DigestFunction.Value getDigestFunction() {
return digestFunction;
}
}

/**
* Collects metadata for {@code file}. Depending on the underlying filesystem used this method
* might do I/O.
*/
private PathMetadata readPathMetadata(Path path, LocalFile file) throws IOException {
DigestUtil digestUtil = new DigestUtil(xattrProvider, path.getFileSystem().getDigestFunction());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just use path.getFileSystem().getDigestFunction() instead of creating a DigestUtil.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DigestUtil exposes the DigestFunction.Value which is distinct from DigestHashFunction which is returned by the method you're suggesting.

So we're using DigestUtil to convert basically.

Copy link
Contributor Author

@tylerwilliams tylerwilliams Jul 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also -- sorry it wasn't clear, DigestUtil was already being created in this function (L217), i just moved it up so I can reference it :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh, yes. Sorry I didn't read the code carefully.


if (file.type == LocalFileType.OUTPUT_DIRECTORY
|| (file.type == LocalFileType.OUTPUT && path.isDirectory())) {
return new PathMetadata(
Expand All @@ -189,7 +200,8 @@ private PathMetadata readPathMetadata(Path path, LocalFile file) throws IOExcept
/* directory= */ true,
/* symlink= */ false,
/* remote= */ false,
/* omitted= */ false);
/* omitted= */ false,
/* digestFunction= */ digestUtil.getDigestFunction());
}
if (file.type == LocalFileType.OUTPUT_SYMLINK) {
return new PathMetadata(
Expand All @@ -198,7 +210,8 @@ private PathMetadata readPathMetadata(Path path, LocalFile file) throws IOExcept
/* directory= */ false,
/* symlink= */ true,
/* remote= */ false,
/* omitted= */ false);
/* omitted= */ false,
/* digestFunction= */ digestUtil.getDigestFunction());
}

PathFragment filePathFragment = path.asFragment();
Expand All @@ -214,10 +227,15 @@ private PathMetadata readPathMetadata(Path path, LocalFile file) throws IOExcept
}
}

DigestUtil digestUtil = new DigestUtil(xattrProvider, path.getFileSystem().getDigestFunction());
Digest digest = digestUtil.compute(path);
return new PathMetadata(
path, digest, /* directory= */ false, /* symlink= */ false, isRemoteFile(path), omitted);
path,
digest,
/* directory= */ false,
/* symlink= */ false,
isRemoteFile(path),
omitted,
digestUtil.getDigestFunction());
}

private static void processQueryResult(
Expand All @@ -235,7 +253,8 @@ private static void processQueryResult(
file.isDirectory(),
file.isSymlink(),
/* remote= */ true,
file.isOmitted());
file.isOmitted(),
file.getDigestFunction());
knownRemotePaths.add(remotePathMetadata);
}
}
Expand Down Expand Up @@ -336,7 +355,8 @@ private Single<List<PathMetadata>> uploadLocalFiles(
// set remote to true so the PathConverter will use bytestream://
// scheme to convert the URI for this file
/* remote= */ true,
path.isOmitted()))
path.isOmitted(),
path.getDigestFunction()))
.onErrorResumeNext(
error -> {
reportUploadError(error, path.getPath(), path.getDigest());
Expand Down Expand Up @@ -375,7 +395,8 @@ private Single<PathConverter> doUpload(Map<Path, LocalFile> files) {
/* directory= */ false,
/* symlink= */ false,
/* remote= */ false,
/* omitted= */ false);
/* omitted= */ false,
DigestFunction.Value.SHA256);
}
})
.collect(Collectors.toList())
Expand Down Expand Up @@ -419,7 +440,7 @@ public ReferenceCounted touch(Object o) {
private static class PathConverterImpl implements PathConverter {

private final String remoteServerInstanceName;
private final Map<Path, Digest> pathToDigest;
private final Map<Path, PathMetadata> pathToMetadata;
private final Set<Path> skippedPaths;
private final Set<Path> localPaths;

Expand All @@ -429,18 +450,18 @@ private static class PathConverterImpl implements PathConverter {
RemoteBuildEventUploadMode remoteBuildEventUploadMode) {
Preconditions.checkNotNull(uploads);
this.remoteServerInstanceName = remoteServerInstanceName;
pathToDigest = Maps.newHashMapWithExpectedSize(uploads.size());
pathToMetadata = Maps.newHashMapWithExpectedSize(uploads.size());
ImmutableSet.Builder<Path> skippedPaths = ImmutableSet.builder();
ImmutableSet.Builder<Path> localPaths = ImmutableSet.builder();
for (PathMetadata pair : uploads) {
Path path = pair.getPath();
Digest digest = pair.getDigest();
for (PathMetadata metadata : uploads) {
Path path = metadata.getPath();
Digest digest = metadata.getDigest();
if (digest != null) {
// Always use bytestream:// in MINIMAL mode
if (remoteBuildEventUploadMode == RemoteBuildEventUploadMode.MINIMAL) {
pathToDigest.put(path, digest);
} else if (pair.isRemote()) {
pathToDigest.put(path, digest);
pathToMetadata.put(path, metadata);
} else if (metadata.isRemote()) {
pathToMetadata.put(path, metadata);
} else {
localPaths.add(path);
}
Expand All @@ -452,6 +473,14 @@ private static class PathConverterImpl implements PathConverter {
this.localPaths = localPaths.build();
}

private static boolean isOldStyleDigestFunction(DigestFunction.Value digestFunction) {
// Old-style digest functions (SHA256, etc) are distinguishable by the length
// of their hash alone and do not require extra specification, but newer
// digest functions (which may have the same length hashes as the older
// functions!) must be explicitly specified in the upload resource name.
return digestFunction.getNumber() <= 7;
}

@Override
@Nullable
public String apply(Path path) {
Expand All @@ -461,18 +490,34 @@ public String apply(Path path) {
return String.format("file://%s", path.getPathString());
}

Digest digest = pathToDigest.get(path);
if (digest == null) {
PathMetadata metadata = pathToMetadata.get(path);
if (metadata == null) {
if (skippedPaths.contains(path)) {
return null;
}
// It's a programming error to reference a file that has not been uploaded.
throw new IllegalStateException(
String.format("Illegal file reference: '%s'", path.getPathString()));
}
return String.format(
"bytestream://%s/blobs/%s/%d",
remoteServerInstanceName, digest.getHash(), digest.getSizeBytes());

Digest digest = metadata.getDigest();
DigestFunction.Value digestFunction = metadata.getDigestFunction();
String out;
if (isOldStyleDigestFunction(digestFunction)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a unit test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

out =
String.format(
"bytestream://%s/blobs/%s/%d",
remoteServerInstanceName, digest.getHash(), digest.getSizeBytes());
} else {
out =
String.format(
"bytestream://%s/blobs/%s/%s/%d",
remoteServerInstanceName,
Ascii.toLowerCase(digestFunction.getValueDescriptor().getName()),
digest.getHash(),
digest.getSizeBytes());
}
return out;
}
}
}
1 change: 1 addition & 0 deletions src/test/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 @@ -94,6 +94,7 @@ java_test(
"//src/main/java/com/google/devtools/build/lib/util/io",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//src/main/java/com/google/devtools/build/lib/vfs/bazel",
"//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs",
"//src/main/java/com/google/devtools/common/options",
"//src/main/protobuf:failure_details_java_proto",
Expand Down
52 changes: 42 additions & 10 deletions ...t/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.SyscallCache;
import com.google.devtools.build.lib.vfs.bazel.BazelHashFunctions;
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
import com.google.devtools.common.options.Options;
import io.grpc.Server;
Expand Down Expand Up @@ -177,7 +178,7 @@ public void uploadsShouldWork() throws Exception {
filesToUpload.put(
file,
new LocalFile(
file, LocalFileType.OUTPUT, /*artifact=*/ null, /*artifactMetadata=*/ null));
file, LocalFileType.OUTPUT, /* artifact= */ null, /* artifactMetadata= */ null));
}
serviceRegistry.addService(new MaybeFailOnceUploadService(blobsByHash));

Expand Down Expand Up @@ -220,7 +221,7 @@ public void uploadsShouldWork_fewerPermitsThanUploads() throws Exception {
filesToUpload.put(
file,
new LocalFile(
file, LocalFileType.OUTPUT, /*artifact=*/ null, /*artifactMetadata=*/ null));
file, LocalFileType.OUTPUT, /* artifact= */ null, /* artifactMetadata= */ null));
}
serviceRegistry.addService(new MaybeFailOnceUploadService(blobsByHash));

Expand Down Expand Up @@ -310,6 +311,31 @@ public void testUnknown_uploadedIfFile() throws Exception {
artifactUploader.release();
}

@Test
public void testUnknown_uploadedIfFileBlake3() throws Exception {
FileSystem fs = new InMemoryFileSystem(new JavaClock(), BazelHashFunctions.BLAKE3);
Path file = fs.getPath("/file");
file.getOutputStream().close();
Map<Path, LocalFile> filesToUpload = new HashMap<>();
filesToUpload.put(
file,
new LocalFile(
file, LocalFileType.OUTPUT, /* artifact= */ null, /* artifactMetadata= */ null));
RemoteRetrier retrier =
TestUtils.newRemoteRetrier(() -> new FixedBackoff(1, 0), (e) -> true, retryService);
ReferenceCountedChannel refCntChannel = new ReferenceCountedChannel(channelConnectionFactory);
RemoteCache remoteCache = newRemoteCache(refCntChannel, retrier);
ByteStreamBuildEventArtifactUploader artifactUploader = newArtifactUploader(remoteCache);

PathConverter pathConverter = artifactUploader.upload(filesToUpload).get();
String hash = BaseEncoding.base16().lowerCase().encode(file.getDigest());
long size = file.getFileSize();
String conversion = pathConverter.apply(file);
assertThat(conversion)
.isEqualTo("bytestream://localhost/instance/blobs/blake3/" + hash + "/" + size);
artifactUploader.release();
}

@Test
public void testUnknown_notUploadedIfDirectory() throws Exception {
Path dir = fs.getPath("/dir");
Expand Down Expand Up @@ -352,7 +378,7 @@ public void someUploadsFail_succeedsWithWarningMessages() throws Exception {
filesToUpload.put(
file,
new LocalFile(
file, LocalFileType.OUTPUT, /*artifact=*/ null, /*artifactMetadata=*/ null));
file, LocalFileType.OUTPUT, /* artifact= */ null, /* artifactMetadata= */ null));
}
String hashOfBlobThatShouldFail = blobsByHash.keySet().iterator().next().toString();
serviceRegistry.addService(
Expand Down Expand Up @@ -437,7 +463,7 @@ public void remoteFileShouldNotBeUploaded_actionFs() throws Exception {
assertThat(remotePath.getFileSystem()).isEqualTo(remoteFs);
LocalFile file =
new LocalFile(
remotePath, LocalFileType.OUTPUT, /*artifact=*/ null, /*artifactMetadata=*/ null);
remotePath, LocalFileType.OUTPUT, /* artifact= */ null, /* artifactMetadata= */ null);

// act

Expand Down Expand Up @@ -488,10 +514,16 @@ public void remoteFileShouldNotBeUploaded_findMissingDigests() throws Exception
ImmutableMap.of(
remoteFile,
new LocalFile(
remoteFile, LocalFileType.OUTPUT, /*artifact=*/ null, /*artifactMetadata=*/ null),
remoteFile,
LocalFileType.OUTPUT,
/* artifact= */ null,
/* artifactMetadata= */ null),
localFile,
new LocalFile(
localFile, LocalFileType.OUTPUT, /*artifact=*/ null, /*artifactMetadata=*/ null));
localFile,
LocalFileType.OUTPUT,
/* artifact= */ null,
/* artifactMetadata= */ null));
PathConverter pathConverter = artifactUploader.upload(files).get();

// assert
Expand Down Expand Up @@ -549,11 +581,11 @@ private ByteStreamBuildEventArtifactUploader newArtifactUploader(RemoteCache rem
return new ByteStreamBuildEventArtifactUploader(
MoreExecutors.directExecutor(),
reporter,
/*verboseFailures=*/ true,
/* verboseFailures= */ true,
remoteCache,
/*remoteServerInstanceName=*/ "localhost/instance",
/*buildRequestId=*/ "none",
/*commandId=*/ "none",
/* remoteServerInstanceName= */ "localhost/instance",
/* buildRequestId= */ "none",
/* commandId= */ "none",
SyscallCache.NO_CACHE,
RemoteBuildEventUploadMode.ALL);
}
Expand Down