From f08b6d010acd2c247a69bc28afeaf21d61ae32a6 Mon Sep 17 00:00:00 2001 From: Mislav Mandaric Date: Tue, 15 Oct 2024 11:35:38 -0700 Subject: [PATCH] [7.5.0] Set oldest_content_accepted for remote downloader requests without the checksum This PR address the #23932 issue with remote downloader. Closes #23970. PiperOrigin-RevId: 686180819 Change-Id: Ia36c5793622bd1ac1fdaa9ef1326ddc385a5a01f (cherry picked from commit 60638af7a78135f2bbe705dc99433307e7f1b79f) --- .../build/lib/remote/downloader/BUILD | 2 ++ .../downloader/GrpcRemoteDownloader.java | 9 +++++ .../build/lib/remote/downloader/BUILD | 2 ++ .../downloader/GrpcRemoteDownloaderTest.java | 34 +++++++++++++++++++ 4 files changed, 47 insertions(+) diff --git a/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD b/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD index dccb139bc94499..1ea404a3597e80 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD @@ -16,6 +16,7 @@ java_library( srcs = glob(["*.java"]), deps = [ "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader", + "//src/main/java/com/google/devtools/build/lib/clock", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/remote:ReferenceCountedChannel", "//src/main/java/com/google/devtools/build/lib/remote:Retrier", @@ -27,6 +28,7 @@ java_library( "//third_party:guava", "//third_party:jsr305", "//third_party/grpc-java:grpc-jar", + "@com_google_protobuf//:protobuf_java_util", "@remoteapis//:build_bazel_remote_asset_v1_remote_asset_java_grpc", "@remoteapis//:build_bazel_remote_asset_v1_remote_asset_java_proto", "@remoteapis//:build_bazel_remote_execution_v2_remote_execution_java_proto", diff --git a/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java b/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java index 12350ebdca5b7c..414c886a3735ab 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java @@ -28,6 +28,7 @@ import com.google.devtools.build.lib.bazel.repository.downloader.Checksum; import com.google.devtools.build.lib.bazel.repository.downloader.Downloader; import com.google.devtools.build.lib.bazel.repository.downloader.HashOutputStream; +import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.remote.ReferenceCountedChannel; @@ -38,12 +39,14 @@ import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.remote.util.Utils; import com.google.devtools.build.lib.vfs.Path; +import com.google.protobuf.util.Timestamps; import io.grpc.CallCredentials; import io.grpc.Channel; import io.grpc.StatusRuntimeException; import java.io.IOException; import java.io.OutputStream; import java.net.URL; +import java.time.Duration; import java.util.List; import java.util.Map; import java.util.Optional; @@ -199,6 +202,12 @@ static FetchBlobRequest newFetchBlobRequest( .setName(QUALIFIER_CHECKSUM_SRI) .setValue(checksum.get().toSubresourceIntegrity()) .build()); + } else { + // If no checksum is provided, never accept cached content. + // Timestamp is offset by an hour to account for clock skew. + requestBuilder.setOldestContentAccepted( + Timestamps.fromMillis( + BlazeClock.instance().now().plus(Duration.ofHours(1)).toEpochMilli())); } if (!Strings.isNullOrEmpty(canonicalId)) { diff --git a/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD b/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD index d49a5aa46aff52..8a33bfd57a711f 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD @@ -24,6 +24,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/authandtls", "//src/main/java/com/google/devtools/build/lib/bazel/repository/cache", "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader", + "//src/main/java/com/google/devtools/build/lib/clock", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/remote", "//src/main/java/com/google/devtools/build/lib/remote/common", @@ -43,6 +44,7 @@ java_library( "//third_party:truth", "//third_party/grpc-java:grpc-jar", "//third_party/protobuf:protobuf_java", + "@com_google_protobuf//:protobuf_java_util", "@remoteapis//:build_bazel_remote_asset_v1_remote_asset_java_grpc", "@remoteapis//:build_bazel_remote_asset_v1_remote_asset_java_proto", "@remoteapis//:build_bazel_remote_execution_v2_remote_execution_java_proto", diff --git a/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java index dbacc73c918c66..60fed77876607e 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java @@ -39,6 +39,7 @@ import com.google.devtools.build.lib.bazel.repository.downloader.Checksum; import com.google.devtools.build.lib.bazel.repository.downloader.Downloader; import com.google.devtools.build.lib.bazel.repository.downloader.UnrecoverableHttpException; +import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.remote.ChannelConnectionWithServerCapabilitiesFactory; import com.google.devtools.build.lib.remote.ReferenceCountedChannel; @@ -51,6 +52,7 @@ import com.google.devtools.build.lib.remote.util.InMemoryCacheClient; import com.google.devtools.build.lib.remote.util.TestUtils; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; +import com.google.devtools.build.lib.testutil.ManualClock; import com.google.devtools.build.lib.testutil.Scratch; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileSystemUtils; @@ -58,6 +60,7 @@ import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.common.options.Options; import com.google.protobuf.ByteString; +import com.google.protobuf.util.Timestamps; import io.grpc.CallCredentials; import io.grpc.ManagedChannel; import io.grpc.Server; @@ -68,7 +71,9 @@ import io.reactivex.rxjava3.core.Single; import java.io.IOException; import java.io.InputStream; +import java.net.URI; import java.net.URL; +import java.time.Duration; import java.util.List; import java.util.Map; import java.util.Optional; @@ -85,6 +90,8 @@ @RunWith(JUnit4.class) public class GrpcRemoteDownloaderTest { + private static final ManualClock clock = new ManualClock(); + private static final DigestUtil DIGEST_UTIL = new DigestUtil(SyscallCache.NO_CACHE, DigestHashFunction.SHA256); @@ -112,6 +119,8 @@ public final void setUp() throws Exception { context = RemoteActionExecutionContext.create(metadata); retryService = MoreExecutors.listeningDecorator(Executors.newScheduledThreadPool(1)); + + BlazeClock.setClock(clock); } @After @@ -207,6 +216,8 @@ public void fetchBlob( .isEqualTo( FetchBlobRequest.newBuilder() .setDigestFunction(DIGEST_UTIL.getDigestFunction()) + .setOldestContentAccepted( + Timestamps.fromMillis(clock.advance(Duration.ofHours(1)))) .addUris("http://example.com/content.txt") .build()); responseObserver.onNext( @@ -385,4 +396,27 @@ public void testFetchBlobRequest() throws Exception { .setValue("foo,bar")) .build()); } + + @Test + public void testFetchBlobRequest_withoutChecksum() throws Exception { + FetchBlobRequest request = + GrpcRemoteDownloader.newFetchBlobRequest( + "instance name", + ImmutableList.of(new URI("http://example.com/").toURL()), + Optional.empty(), + "canonical ID", + DIGEST_UTIL.getDigestFunction(), + ImmutableMap.of()); + + assertThat(request) + .isEqualTo( + FetchBlobRequest.newBuilder() + .setInstanceName("instance name") + .setDigestFunction(DIGEST_UTIL.getDigestFunction()) + .setOldestContentAccepted(Timestamps.fromMillis(clock.advance(Duration.ofHours(1)))) + .addUris("http://example.com/") + .addQualifiers( + Qualifier.newBuilder().setName("bazel.canonical_id").setValue("canonical ID")) + .build()); + } }