Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix http regex string to add support for cas.v2 http requests #663

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

HunterCrosland
Copy link

Adds support to http.go regex string to allow cas.v2 in http request

@HunterCrosland HunterCrosland marked this pull request as draft May 26, 2023 20:48
@HunterCrosland
Copy link
Author

I am not sure why this is failing HTML requests, but we are getting error responses locally on latest container version.

CURL requests to valid SHAs using /cas/SHA returns resource name must be a SHA256 hash in hex. got '/cas.v2/SHA'

I can investigate this more next week.

@HunterCrosland
Copy link
Author

I am not sure why this is failing HTML requests, but we are getting error responses locally on latest container version.

CURL requests to valid SHAs using /cas/SHA returns resource name must be a SHA256 hash in hex. got '/cas.v2/SHA'

I can investigate this more next week.

Realized regex match groups got messed up, fixed it.

@HunterCrosland HunterCrosland marked this pull request as ready for review May 26, 2023 22:24
Copy link
Collaborator

@mostynb mostynb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution- I assume you would like this behaviour because you're chaining two bazel-remote instances with the http proxy backend? If that is the case, I wonder if it would be better to add a new option to the http proxy backend instead?

@HunterCrosland
Copy link
Author

Thanks for the contribution- I assume you would like this behaviour because you're chaining two bazel-remote instances with the http proxy backend? If that is the case, I wonder if it would be better to add a new option to the http proxy backend instead?

You are spot-on with the why -- and I really don't mind how. I am not entirely familiar with the routing on the backend, so that's probably something I will need to do over a weekend. In the meantime for our CI we've rolled back to a previous image version built prior to cas.v2 support. I built an image based on the changes I made here but it didn't have the correct entrypoint by default, and that's going to be another weekend issue.

@mostynb
Copy link
Collaborator

mostynb commented May 31, 2023

This used to work out of the box before bazel-remote had a compressed storage option (which is now the default). You should still be able to use a current build to chain together two bazel-remote instances with the http proxy backend by using --storage_mode uncompressed on the front end (and optionally on the back end).

If the front end uses the default --storage_mode zstd then it uploads compressed blobs using the /cas.v2/<sha256> path to distinguish them from regular/uncompressed blobs, and gives the error you probably saw. But if we know that the proxy backend can handle zstd compressed blobs correctly, then we could add a flag like --http_proxy.mode bazel-remote2 to make it upload both compressed and uncompressed blobs to /cas/<sha256> paths. I like this idea, because it doesn't expand the client-side API.

@liam-baker-sm
Copy link

liam-baker-sm commented Oct 4, 2024

--http_proxy.mode bazel-remote2 This would support our usecase as well.
Bazel is now running --remote_cache_compression in production without error, so enabling compressed storage for the second bazel-remote instance would improve its capacity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants