From a29cde8218afcf78541344abf7c645f734a11f33 Mon Sep 17 00:00:00 2001 From: Ed Schouten Date: Tue, 28 Mar 2023 10:52:57 -0700 Subject: [PATCH] Set the digest_function field as part of all relevant gRPC requests In the following PR we extended most of the REv2 *Request messages to take an explicit digest function: https://github.com/bazelbuild/remote-apis/pull/235 This eliminates the ambiguity between digest functions that have the same hash length (e.g., MD5 and MURMUR3, SHA256 and SHA256TREE). This change extends the REv2 client in Bazel to set the digest_function field explicitly, so that the server does not need to use any heuristics to pick a digest function when processing requests. As we are now going to call DigestUtil.getDigestFunction() far more frequently, alter DigestUtil to precompute the value upon construction. This change was tested by letting Bazel forward all traffic through an instance of bb_clientd that was patched up to require the use of `digest_function`. PiperOrigin-RevId: 520074622 Change-Id: Ie92dc368c502b9bc3fddef56a5eb0a238760f673 (cherry picked from commit 0a8380bec9dceae1bffabddcccd459e82d8674b1) Signed-off-by: Brentley Jones --- .../devtools/build/lib/remote/GrpcCacheClient.java | 7 ++++++- .../build/lib/remote/RemoteExecutionService.java | 1 + .../lib/remote/RemoteRepositoryRemoteExecutor.java | 1 + .../devtools/build/lib/remote/util/DigestUtil.java | 10 ++++++++-- .../devtools/build/lib/remote/GrpcCacheClientTest.java | 2 ++ 5 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java index ce0e917837f0c5..470d3845dc3273 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java @@ -117,6 +117,7 @@ private int computeMaxMissingBlobsDigestsPerMessage() { final int overhead = FindMissingBlobsRequest.newBuilder() .setInstanceName(options.remoteInstanceName) + .setDigestFunction(digestUtil.getDigestFunction()) .build() .getSerializedSize(); final int tagSize = @@ -185,7 +186,9 @@ public ListenableFuture> findMissingDigests( } // Need to potentially split the digests into multiple requests. FindMissingBlobsRequest.Builder requestBuilder = - FindMissingBlobsRequest.newBuilder().setInstanceName(options.remoteInstanceName); + FindMissingBlobsRequest.newBuilder() + .setInstanceName(options.remoteInstanceName) + .setDigestFunction(digestUtil.getDigestFunction()); List> getMissingDigestCalls = new ArrayList<>(); for (Digest digest : digests) { requestBuilder.addBlobDigests(digest); @@ -260,6 +263,7 @@ public ListenableFuture downloadActionResult( GetActionResultRequest request = GetActionResultRequest.newBuilder() .setInstanceName(options.remoteInstanceName) + .setDigestFunction(digestUtil.getDigestFunction()) .setActionDigest(actionKey.getDigest()) .setInlineStderr(inlineOutErr) .setInlineStdout(inlineOutErr) @@ -289,6 +293,7 @@ public ListenableFuture uploadActionResult( .updateActionResult( UpdateActionResultRequest.newBuilder() .setInstanceName(options.remoteInstanceName) + .setDigestFunction(digestUtil.getDigestFunction()) .setActionDigest(actionKey.getDigest()) .setActionResult(actionResult) .build())), diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index a2cf4ae1597505..c3545b669c338a 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -1482,6 +1482,7 @@ public RemoteActionResult executeRemotely( ExecuteRequest.Builder requestBuilder = ExecuteRequest.newBuilder() .setInstanceName(remoteOptions.remoteInstanceName) + .setDigestFunction(digestUtil.getDigestFunction()) .setActionDigest(action.getActionKey().getDigest()) .setSkipCacheLookup(!acceptCachedResult); if (remoteOptions.remoteResultCachePriority != 0) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutor.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutor.java index 69f521e84da95f..3a731f72709be2 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutor.java @@ -167,6 +167,7 @@ public ExecutionResult execute( ExecuteRequest.newBuilder() .setActionDigest(actionDigest) .setInstanceName(remoteInstanceName) + .setDigestFunction(digestUtil.getDigestFunction()) .setSkipCacheLookup(!acceptCached) .build(); diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java b/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java index 16055f13a7ee3d..76b258443890ca 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java +++ b/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java @@ -35,14 +35,15 @@ public class DigestUtil { private final XattrProvider xattrProvider; private final DigestHashFunction hashFn; + private final DigestFunction.Value digestFunction; public DigestUtil(XattrProvider xattrProvider, DigestHashFunction hashFn) { this.xattrProvider = xattrProvider; this.hashFn = hashFn; + this.digestFunction = getDigestFunctionFromHashFunction(hashFn); } - /** Returns the currently used digest function. */ - public DigestFunction.Value getDigestFunction() { + private static DigestFunction.Value getDigestFunctionFromHashFunction(DigestHashFunction hashFn) { for (String name : hashFn.getNames()) { try { return DigestFunction.Value.valueOf(name); @@ -53,6 +54,11 @@ public DigestFunction.Value getDigestFunction() { return DigestFunction.Value.UNKNOWN; } + /** Returns the currently used digest function. */ + public DigestFunction.Value getDigestFunction() { + return digestFunction; + } + public Digest compute(byte[] blob) { return buildDigest(hashFn.getHashFunction().hashBytes(blob).toString(), blob.length); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java index 24212e84978627..41fcca47ada4ce 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java @@ -30,6 +30,7 @@ import build.bazel.remote.execution.v2.Command; import build.bazel.remote.execution.v2.ContentAddressableStorageGrpc.ContentAddressableStorageImplBase; import build.bazel.remote.execution.v2.Digest; +import build.bazel.remote.execution.v2.DigestFunction; import build.bazel.remote.execution.v2.Directory; import build.bazel.remote.execution.v2.DirectoryNode; import build.bazel.remote.execution.v2.FileNode; @@ -799,6 +800,7 @@ public void updateActionResult( assertThat(request) .isEqualTo( UpdateActionResultRequest.newBuilder() + .setDigestFunction(DigestFunction.Value.SHA256) .setActionDigest(fooDigest) .setActionResult(result) .build());