Chunk FindMissingBlobsRequest appropriately #20708

tgolsson · 2024-03-22T21:18:41Z

No description provided.

jasonwbarnett · 2024-04-05T01:16:49Z

I haven't forgotten about this @tgolsson. I just haven't made time to test this. I'll attempt to get to this no later than end of next week (4/14/2024).

jasonwbarnett · 2024-04-12T18:30:14Z

@tgolsson it worked!

huonw

Nice

huonw · 2024-05-13T20:09:34Z

src/rust/engine/remote_provider/remote_provider_reapi/src/byte_store.rs

+        let instance_name = "";
+        // NOTE[TSolberg]:
+        // This test is a bit of a hack, but it's the best way I could think of to ensure that the size of the
+        // FindMissingBlobsRequest is roughly what we expect. The only delta would be the encoding of the


Sentence is truncated: maybe?

Suggested change

// FindMissingBlobsRequest is roughly what we expect. The only delta would be the encoding of the

// FindMissingBlobsRequest is roughly what we expect. The only delta would be the encoding of the instance name

huonw · 2024-05-13T20:14:08Z

src/rust/engine/remote_provider/remote_provider_reapi/src/byte_store.rs

-            instance_name: self.instance_name.as_ref().cloned().unwrap_or_default(),
-            blob_digests: digests.into_iter().map(|d| d.into()).collect::<Vec<_>>(),
-        };
+        let blob_digests = digests.into_iter().map(|d| d.into()).collect::<Vec<_>>();


One option here would be collecting into a set instead, to deduplicate any repetition. For instance empty files, or a node_modules folder with the same package installed a lot of times in different places (if that's a thing). Thoughts?

Now that I write it out, maybe best for that to be a separate change, so never mind, but floating it here to explain my thinking.

huonw · 2024-05-13T20:17:08Z

src/rust/engine/remote_provider/remote_provider_reapi/src/byte_store.rs

-        };
+        let blob_digests = digests.into_iter().map(|d| d.into()).collect::<Vec<_>>();
+
+        let max_digests_per_request: usize = (4 * 1024 * 1024


Could this 4MiB be pulled out into constant with a comment that explains where it comes from?

huonw · 2024-05-13T23:29:41Z

docs/notes/2.22.x.md

@@ -68,6 +68,10 @@ Setting [the `orphan_files_behaviour = "ignore"` option](https://www.pantsbuild.

 ### Plugin API changes

+## Bugfixes
+
+- Remote cache: `FindMissingBlobsRequest` will now make multiple request if the number of files is large. (#20708)


Similar to #20492 (comment):

we've historically put this sort of change into a dedicated "remote caching/execution" section: https://github.com/pantsbuild/pants/blob/main/docs/notes/2.20.x.md#remote-cachingexecution

we might want to make the PR reference a manual link

huonw · 2024-05-13T23:34:18Z

src/rust/engine/remote_provider/remote_provider_reapi/src/byte_store.rs

+            - self
+                .instance_name
+                .as_ref()
+                .cloned()
+                .unwrap_or_default()
+                .len())


Is the instance name encoded into the gRPC message as exactly its UTF-8 bytes? I'd personally be expecting an extra one or two bytes for either a varint-encoded length or a null terminator (the first I imagine). If we get unlucky with configuration, a request could exceed the 4MiB limit by 1 byte and that'd be very annoying! (e.g. a 44 byte instance_name would make for a 4MiB request (59918 * 70 + 44) and thus any extra content would be too large)

I guess we could either:

make this exactly precise (and a loop over different instance_names to the test)

just add semi-arbitrary slop factor:

Suggested change

- self

.instance_name

.as_ref()

.cloned()

.unwrap_or_default()

.len())

- self

.instance_name

.as_ref()

.cloned()

.unwrap_or_default()

.len() - 10)

huonw · 2024-05-13T23:46:29Z

src/rust/engine/remote_provider/remote_provider_reapi/src/byte_store_tests.rs

+        provider
+            .list_missing_digests(&mut test_data.into_iter())
+            .await,
+        Ok(HashSet::new())


Thanks for the test. I think there's a few plausible mistakes that we could introduce in future that won't be caught:

only sending each unique digest once, thus invalidating this test (i.e. if we do the HashSet idea, the list_missing_digests will be working with just one digest and thus only send one request)

skipping the second+ request (either not sending them at all, or not including the results in the final aggregation)

Not sure about this one: if we break this in another way and start sending >4MiB requests, does the StubCAS reject the requests (i.e. does this test fail without the fix)?

I think we can address each of them:

check assert_eq!(cas.request_count(RequestType::CASFindMissingBlobs), 2) so that the test starts failing if that core assumption changes

add two digests that is missing, within each "chunk" (e.g. one at the start and one at the end)

add request size enforcement to StubCAS if not already there (this might cause other tests to fail, so we might want it to be optional until we resolve them)

huonw · 2024-05-13T23:47:39Z

src/rust/engine/remote_provider/remote_provider_reapi/src/byte_store.rs

+            };
+
+            msg
+        });

        workunit_store::increment_counter_if_in_workunit(Metric::RemoteStoreExistsAttempts, 1);


I'm... not sure whether these should be counting list_missing_digests calls (i.e. remain here) or network requests (i.e. move within the map(|request| { ... }) closure). 🤔

huonw · 2024-05-13T23:51:09Z

src/rust/engine/remote_provider/remote_provider_reapi/Cargo.toml

@@ -21,6 +21,7 @@ tokio-util = { workspace = true, features = ["io"] }
 tonic = { workspace = true }
 uuid = { workspace = true, features = ["v4"] }
 workunit_store = { path = "../../workunit_store" }
+prost = { workspace = true }


It looks like this should technically be in dev-dependencies, since it's only used by tests... but I suspect it doesn't matter given it's already in the non-dev dependency tree. Only worth moving if you're making other changes.

huonw · 2024-05-13T23:51:51Z

src/rust/engine/remote_provider/remote_provider_reapi/src/byte_store.rs


-        let client = self.cas_client.as_ref().clone();
+        let requests = blob_digests.chunks(max_digests_per_request).map(|digests| {


This core of the fix is very nice, good choice!

huonw · 2024-05-13T23:58:30Z

Oh, also, does this fix #20674? Could you update the PR description like "fixes #20674" to have the appropriate auto-close behaviour. Thanks!

tgolsson mentioned this pull request Mar 22, 2024

remote cache ResourceExhausted/OutOfRange errors #20674

Open

buchgr mentioned this pull request Apr 4, 2024

allow configuring grpc max receive message size buchgr/bazel-remote#738

Open

tgolsson added 2 commits May 13, 2024 18:04

add failing test

cad2ea4

fix test

885784c

tgolsson added category:bugfix Bug fixes for released features remote labels May 13, 2024

lint

b7d8edc

tgolsson force-pushed the ts/max-message-size-find-missing-blobs branch from 694efe3 to b7d8edc Compare May 13, 2024 16:07

add notes

0890fb9

tgolsson marked this pull request as ready for review May 13, 2024 16:17

tgolsson requested a review from huonw May 13, 2024 16:17

huonw reviewed May 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chunk FindMissingBlobsRequest appropriately #20708

Chunk FindMissingBlobsRequest appropriately #20708

tgolsson commented Mar 22, 2024

jasonwbarnett commented Apr 5, 2024

jasonwbarnett commented Apr 12, 2024

huonw left a comment

huonw May 13, 2024

huonw May 13, 2024

huonw May 13, 2024

huonw May 13, 2024

huonw May 13, 2024

huonw May 13, 2024

huonw May 13, 2024

huonw May 13, 2024

huonw May 13, 2024

huonw commented May 13, 2024

	// FindMissingBlobsRequest is roughly what we expect. The only delta would be the encoding of the
	// FindMissingBlobsRequest is roughly what we expect. The only delta would be the encoding of the instance name


		let client = self.cas_client.as_ref().clone();
		let requests = blob_digests.chunks(max_digests_per_request).map(\|digests\| {

Chunk FindMissingBlobsRequest appropriately #20708

Are you sure you want to change the base?

Chunk FindMissingBlobsRequest appropriately #20708

Conversation

tgolsson commented Mar 22, 2024

jasonwbarnett commented Apr 5, 2024

jasonwbarnett commented Apr 12, 2024

huonw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw commented May 13, 2024