Remote: Ignore blobs referenced in BEP if the generating action cannot be cached remotely.

[coeuvre]

Introduces new flag --incompatible_remote_build_event_upload_respect_no_cache which when set to true, remote uploader won't upload blobs referenced in BEP if the generating action cannot be cached remotely.

Fixes #11473.

[brentleyjones]

Thank you soooo much for this! ❤️

[brentleyjones]

This interacts in an interesting way with --noremote_upload_local_results. I would expect that when I use that flag with this new flag, that things like the timing profile would still be uploaded (as long as --experimental_build_event_upload_strategy=remote), as they aren't the result of a local action. I don't see a way to allow those to be uploaded without uploading normal outputs.

[coeuvre]

Good catch! Updated to use the same method that used to determine whether Bazel should upload local results. So the behaviours should be aligned.

[brentleyjones]

I haven't had a chance to test yet, but it sounds like you made it reenforce the behavior I didn't want? I want it so --noremote_upload_local_results does not affect BES upload, only no-remote/no-remote-cache affects it. If it's already that, ignore my noise 😅.

And if that sounds like a weird request, it's because we need a way for local developers to upload BES specific artifacts (timing profile mainly), without uploading everything else. In our CI case we upload everything (besides for those tagged with no-remote-cache). And since --experimental_build_event_upload_strategy=remote exists, one could reason that even with --noremote_upload_local_results and --incompatible_remote_build_event_upload_respect_no_cache, certain things would still be uploaded (and if you didn't want any uploaded, you could set --experimental_build_event_upload_strategy=local).

[brentleyjones]

Maybe my specific case (which I think is going to be a common setup for BES users) needs the no-bes-upload tag. Actually a no-remote-cache-upload tag combined with this new flag would work.

The more I think about this, I think this is the best route, to keep your new flag pure. Plus there are other uses for no-remote-cache-upload, as half of our current use of no-remote-cache is just to prevent upload... we would accept a download if one existed from another method, such as remote execution.

[coeuvre]

Thanks for your input. I know it is somewhat confusing.

Some artifacts in BEP are outputs of actions, some of them are generated by Bazel (e.g. timing profile). This change only applies to artifacts that are outputs of actions.

So with my latest change, if --noremote_upload_local_results is set, outputs are not uploaded to remote cache but other artifacts (including timing profile) are still uploaded. I think this matches your case.

Basically, this PR ensures that if the outputs are not uploaded by spawns, they will not be uploaded by BES uploader as well.

Maybe my specific case (which I think is going to be a common setup for BES users) needs the no-bes-upload tag. Actually a no-remote-cache-upload tag combined with this new flag would work.

The more I think about this, I think this is the best route, to keep your new flag pure. Plus there are other uses for no-remote-cache-upload, as half of our current use of no-remote-cache is just to prevent upload... we would accept a download if one existed from another method, such as remote execution.

I like this idea! I will create another PR for no-remote-cache-upload.

[brentleyjones]

So with my latest change, if --noremote_upload_local_results is set, outputs are not uploaded to remote cache but other artifacts (including timing profile) are still uploaded. I think this matches your case.

I didn't see that. With your latest changes, the timing profile still wasn't uploaded for me 🤔. Only when I set --remote_upload_local_results did they upload.

[brentleyjones]

Another question: how should this affect test log upload? That is another thing that stopped uploading for us with --experimental_build_event_upload_strategy=local.

[coeuvre]

I didn't see that. With your latest changes, the timing profile still wasn't uploaded for me 🤔. Only when I set --remote_upload_local_results did they upload.

I was referring to "command.profile.gz" and the newly added tests are checking that it is uploaded. Are there other files in your build that you want to be uploaded but not? If so, can you share a minimal repro?

Another question: how should this affect test log upload? That is another thing that stopped uploading for us with --experimental_build_event_upload_strategy=local.

test logs are treated as action outputs. i.e. they are uploaded if the result of test actions can be uploaded. Or not uploaded if tagged with "no-cache".

[brentleyjones]

I'm working on reducing my reproduction. With a simple case I see the profile is uploaded, but in our project it's not. I should have something soon.

[brentleyjones]

It's when using a combined cache.

This fails to upload the profile:

build --remote_cache=remote.buildbuddy.io
build --bes_backend=grpcs://remote.buildbuddy.io
build --bes_results_url=https://app.buildbuddy.io/invocation/
build --disk_cache=~/.bazel_cache
build --incompatible_remote_build_event_upload_respect_no_cache
build --noremote_upload_local_results

But this doesn't:

build --remote_cache=remote.buildbuddy.io
build --bes_backend=grpcs://remote.buildbuddy.io
build --bes_results_url=https://app.buildbuddy.io/invocation/
build --disk_cache=
build --incompatible_remote_build_event_upload_respect_no_cache
build --noremote_upload_local_results

[coeuvre]

I found the root cause, but it is hard to fix due to how combined cache works today: the cache itself uses remote options to decide whether upload blobs to remote cache regardless of what kind of artifacts.

I may need to update the combined cache to make it works for this case.

[coeuvre]

Fixed combined cache and added support for tag no-remote-cache-upload. I will split the PR into small CLs when importing.

@brentleyjones please have another try.

[brentleyjones]

The latest code works. Thanks @coeuvre!

[meteorcloudy]

@philwo I think this needs your review ;)

[brentleyjones]

#14389

[alexeagle]

It seems that we're missing the incompatible-change tracking bug for flipping the default value of this flag to true. @brentleyjones do you know how to create that issue? Can the bot do it?

[brentleyjones]

I do not. @meteorcloudy do you know?

[meteorcloudy]

The flag author should do it, it seems we are missing tracking issue for a number of remote execution related incompatible flags? @coeuvre

[coeuvre]

Created #15150.