[build_manager] add support for remote zip

[paulsemel]

This adds support for remote ZIP.

As of now, performances are quite good locally, and the read ahead mechanism should keep reasonable performance. Also, given that the ClusterFuzz bots are having HDD, numbers might even be better there, as we're only storing on disk when unpacking the build.

The memory consumption of this new feature is contant: it uses at most (and most of the time) 50 MB of RAM.

[letitz]

Only had time to review part of this - did not get down to archive.py, but hopefully this helps already.

[letitz]

Why?

[paulsemel]

I removed that.

[letitz]

Comment needs an update.

[letitz]

Looking good overall! A bunch of small comments.

[letitz]

Does this mean we'll end up downloading fuzz target binaries twice, once to search for magic bytes, and once again to unzip them to disk? Maybe not, depending on how we cache downloaded bytes - I have not read that far yet.

In any case, this has me yearning for a manifest file of some kind that lists fuzz targets in the zip.

[paulsemel]

Yes, but:

1. We ultimately only download a fuzz target twice, because the other ones won't be selected to be unpacked, thus those will only be downloaded once.
2. This is already true anyways. And yes, I agree this is bad, especially the code you're pointing to.

[letitz]

You can reduce nesting in this function by putting this condition first and returning early:

if can_unzip_over_http:
  with build_archive.open_uri(http_build_url) as build:
    yield build
  return

# Download build archive locally.
...

In fact if you can just return the archive instead of yielding it, as suggested above, this becomes very clean:

if can_unzip_over_http:
  return build_archive.open_uri(http_build_url)

# Download build archive locally.
...

[paulsemel]

Ok, I scratched my head around to make this cleaner while still keeping the correct constraints (read the other comment). WDYT?

[letitz]

Can you just return the build archive? I think it will do the same thing, since ArchiveReader (of which BuildArchive is a subclass) is already a context manager. Then I think you can also annotate this function as returning BuildArchive, and get rid of the contextlib import.

[paulsemel]

No, I kind of need the context manager here because I want to delete the file after we used it. I changed the code a bit, and I think that's cleaner, WDYT?

[letitz]

This means we won't cache these bytes - is that intentional? Why not always delegate to _fetch_from_cache() and let it determine whether or not to fetch more bytes from the wire?

[paulsemel]

Yep, that's because the way those archive library works is that they only read what's needed. If the read is greater than 50 MB, that means we're reading a huge file, and we won't need it in following reads. This is only useful for small reads tbh.

[letitz]

Ah, I see. Can you explain that in a comment for the future reader?

Optional comment / musing follows below:

It results in a behavior I find counterintuitive, though that does not terribly matter:

• read(49M) -> fetches 99MB and caches it
• read(50M) -> fetches 50MB, does not cache it

I think this could be addressed by doing something like:

HTTP_MAX_BYTES = 100 * 1024 * 1024

def fetch_size(size):
  """How many bytes to fetch over HTTP to serve `read(size)`."""
  cache_size = min(size + HTTP_READAHEAD_BYTES, HTTP_MAX_BYTES)
  return max(size, cache_size)

Then at least you don't get a cliff effect for network fetches. But is it useful? Probably not?

[letitz]

Thanks for adding a comment. It only explains what is happening, but not why, which is really what's interesting here. In other words, I can see for myself from reading the code that we don't cache large requests, but it does not tell me why that is. You explained it well in your PR comment, can you do that in the code?

[jonathanmetzman]

I'll leave my comments from my first pass as well. I'll try to get to the rest of it today.

[letitz]

Near-LGTM with a few last comments.

The only major thing remaining is _maybe_get_http_build_url(). I would love to switch that to a straightforward string manipulation instead of relying on environment variables.

[letitz]

Note for future work: If searching these bytes take a long time, we might want to search for them "in parallel" by pushing the pattern list into search_bytes_in_file, which can scan each chunk of the file for all patterns (avoiding the need to re-read the file X times) or even reach for something like Aho-Corasick: https://pyahocorasick.readthedocs.io/en/latest/

[letitz]

Ah, I see. Can you explain that in a comment for the future reader?

Optional comment / musing follows below:

It results in a behavior I find counterintuitive, though that does not terribly matter:

• read(49M) -> fetches 99MB and caches it
• read(50M) -> fetches 50MB, does not cache it

I think this could be addressed by doing something like:

HTTP_MAX_BYTES = 100 * 1024 * 1024

def fetch_size(size):
  """How many bytes to fetch over HTTP to serve `read(size)`."""
  cache_size = min(size + HTTP_READAHEAD_BYTES, HTTP_MAX_BYTES)
  return max(size, cache_size)

Then at least you don't get a cliff effect for network fetches. But is it useful? Probably not?

[jonathanmetzman]

I think debug and symbolized builds are a full chrome thing, and aren't used for engine fuzzer builds.

[jonathanmetzman]

One thing to maybe note is that by converting the build URL to HTTP, I think we lose the ability to authenticate without any extra work. This may not be a problem in current chrome builds but may be a problem in the future.

[jonathanmetzman]

E.g. we fuzzed widevine-based builds in the past.

[oliverchang]

Happy to get this merged if this resolves Chrome's immediate issues.

That said, would it also be worthwhile also scoping out what it would take for Chrome to migrate to split builds? i.e.

clusterfuzz/src/clusterfuzz/_internal/build_management/build_manager.py

Line 653 in b529f77

class SplitTargetBuild(RegularBuild):

There's a fair bit of complexity already with the different build types we have, and long term we should aim to simplify this.

[letitz]

LGTM with a couple last comments.

[letitz]

Comment needs an update.

[letitz]

Thanks for adding a comment. It only explains what is happening, but not why, which is really what's interesting here. In other words, I can see for myself from reading the code that we don't cache large requests, but it does not tell me why that is. You explained it well in your PR comment, can you do that in the code?

[letitz]

@oliverchang right, I had filed crbug.com/333965940 to look into that. I think it's the right direction longer term, but I think it significantly more work than this change.

[paulsemel]

Note: whenever this lands, don't forget that you need to add the ALLOW_UNPACK_OVER_HTTP = True environment variable on the job so that this kicks in.