Skip to content

Commit

Permalink
Ensure namedSetOfFiles URIs specify blob type correctly
Browse files Browse the repository at this point in the history 
I noticed when testing the BLAKE3 digest function that uploaded files were being referenced incorrectly in the BES because they were missing the digest type. This CL fixes that.

Before: https://github.com/bazelbuild/bazel/assets/141737/52781f1b-b897-48f0-8956-f63c57b59436

After: https://github.com/bazelbuild/bazel/assets/141737/01ebc61b-3512-4ca5-8e2d-f47ad5f086b7

PiperOrigin-RevId: 553405394
Change-Id: Ic976e5a58f80ab8b5270b4aedc6204c22562533a

(cherry picked from commit 1929367)
Signed-off-by: Brentley Jones <[email protected]>
  • Loading branch information
@tylerwilliams @brentleyjones
tylerwilliams authored and brentleyjones committed Aug 7, 2023
1 parent a94fc35 commit b9b4713
Show file tree
Hide file tree
Showing 3 changed files with 116 additions and 31 deletions.
91 changes: 70 additions & 21 deletions src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java
View file
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 @@ -133,13 +135,21 @@ private static final class PathMetadata {
private final boolean directory;
private final boolean remote;
private final boolean omitted;

PathMetadata(Path path, Digest digest, boolean directory, boolean remote, boolean omitted) {
private final DigestFunction.Value digestFunction;

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

public Path getPath() {
Expand All @@ -161,20 +171,27 @@ 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 file) throws IOException {
DigestUtil digestUtil = new DigestUtil(xattrProvider, file.getFileSystem().getDigestFunction());

if (file.isDirectory()) {
return new PathMetadata(
file,
/* digest= */ null,
/* directory= */ true,
/* remote= */ false,
/* omitted= */ false);
/* omitted= */ false,
/* digestFunction= */ digestUtil.getDigestFunction());
}

PathFragment filePathFragment = file.asFragment();
Expand All @@ -190,9 +207,14 @@ private PathMetadata readPathMetadata(Path file) throws IOException {
}
}

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

private static void processQueryResult(
Expand All @@ -209,7 +231,8 @@ private static void processQueryResult(
file.getDigest(),
file.isDirectory(),
/* remote= */ true,
file.isOmitted());
file.isOmitted(),
file.getDigestFunction());
knownRemotePaths.add(remotePathMetadata);
}
}
Expand Down Expand Up @@ -305,7 +328,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 @@ -341,7 +365,8 @@ private Single<PathConverter> upload(Set<Path> files) {
/* digest= */ null,
/* directory= */ false,
/* remote= */ false,
/* omitted= */ false);
/* omitted= */ false,
DigestFunction.Value.SHA256);
}
})
.collect(Collectors.toList())
Expand Down Expand Up @@ -385,7 +410,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 @@ -395,18 +420,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 @@ -418,6 +443,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 @@ -427,18 +460,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)) {
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
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,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
55 changes: 45 additions & 10 deletions ...t/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package com.google.devtools.build.lib.remote;

import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assume.assumeNotNull;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.doAnswer;
Expand Down Expand Up @@ -65,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 @@ -176,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 @@ -219,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 All @@ -245,6 +247,33 @@ public void uploadsShouldWork_fewerPermitsThanUploads() throws Exception {
assertThat(refCntChannel.isShutdown()).isTrue();
}

@Test
public void testUnknown_uploadedIfFileBlake3() throws Exception {
assumeNotNull(BazelHashFunctions.BLAKE3);

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 testUploadDirectoryDoesNotCrash() throws Exception {
Path dir = fs.getPath("/dir");
Expand Down Expand Up @@ -286,7 +315,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 @@ -370,7 +399,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 @@ -421,10 +450,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 @@ -481,11 +516,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

0 comments on commit b9b4713

Please sign in to comment.