Fix noremote_upload_local_results behavior in combined blobstore by borkaehw · Pull Request #9338 · bazelbuild/bazel

borkaehw · 2019-09-05T19:20:55Z

Disable the upload action to remote cache when --noremote_upload_local_results is set.

This problem would only happen in CombinedDiskHttpBlobStore at this moment when users want read/write permission to disk cache but only read permission to remote cache.

Once the GrpcCache refactoring (making GrpcCache a SimpleBlobStore implementation) is done, CombinedDiskHttpBlobStore should be renamed to CombinedDiskRemoteBlobStore. Then this fix will apply to both GrpcCache and HttpCache.

Fix #8216

buchgr · 2019-09-06T09:01:27Z

@borkaehw Thanks for the patch! I agree that we should have this but it's incompatible behavior. You need to make it so that this new behavior is guarded behind an incompatible flag that is disabled by default.

ishikhman · 2019-09-06T09:13:42Z

@borkaehw here is a link for the policy with some instructions: https://bazel.build/breaking-changes-guide.html

buchgr · 2019-09-06T09:21:35Z

Hmm. I am wondering if it would make more sense if we gave the disk cache it's own knobs and introduced --disk_write_local_results and --disk_accept_cached instead?

ishikhman · 2019-09-06T09:26:26Z

Hmm. I am wondering if it would make more sense if we gave the disk cache it's own knobs and introduced --disk_write_local_results and --disk_accept_cached instead?

Hm, do you know a use case for that?

SrodriguezO · 2019-09-06T17:27:31Z

I can't think of a strong use case for using the disk cache as read-only. To disable it altogether, you might as well not specify a --disk_cache in the config.

In any case, I think this would fall outside the scope of this PR, which is to resolve #8216 (and the changes to remote_upload_local_results's behavior would have to be done anyway).

borkaehw · 2019-09-06T19:39:51Z

I have made the incompatible flag and written a better commit message. I suppose GitHub issue should be created after we merge. Let me know if I did anything wrong. Thanks.

ishikhman

LGTM.
Thanks a lot for the contribution!

ishikhman · 2019-09-10T09:25:16Z

      throws IOException, InterruptedException {
    diskCache.putActionResult(actionKey, actionResult);
-    remoteCache.putActionResult(actionKey, actionResult);
+    if (!options.incompatibleUploadLocalResultsDiskOnly || options.remoteUploadLocalResults) {


I find the following version a bit more readable, but it's up to you: to change it or not :)
if (options.incompatibleUploadLocalResultsDiskOnly && !options.remoteUploadLocalResults)

You'd need to negate the entire clause for those to be equivalent:
!(options.incompatibleUploadLocalResultsDiskOnly && !options.remoteUploadLocalResults)

I think either way works, but I prefer the original : )

I agree with @SrodriguezO, also prefer the original. More suggestions are welcome.

buchgr · 2019-09-10T14:52:21Z

In any case, I think this would fall outside the scope of this PR, which is to resolve #8216 (and the changes to remote_upload_local_results's behavior would have to be done anyway).

I think it's important to be keep the behaviour consistent. This change makes it so that --remote_upload_local_results not longer applies to --disk_cache whether a --remote_cache is specified or not. So I think to stay sane it we should introduce the same behaviour for --remote_accept_cached?

SrodriguezO · 2019-09-10T16:47:50Z

I think it's important to be keep the behaviour consistent

That's a fair point.

Personally, I can't think of a use case for this, but I don't have a strong opinion either way. @borkaehw wdyt?

buchgr · 2019-09-10T18:08:44Z

Personally, I can't think of a use case for this, but I don't have a strong opinion either way.

I don't think we necessarily need the flags that I proposed but I think that we'll need to also disable --remote_accept_cached for the --disk_cache along with --remote_upload_local_results.

Does that sound reasonable?

SrodriguezO · 2019-09-10T18:24:39Z

but I think that we'll need to also disable --remote_accept_cached for the --disk_cache along with --remote_upload_local_results

Oh I see. Yeah we should definitely do that.

borkaehw · 2019-09-10T19:13:12Z

@buchgr thanks for suggestions, it sounds reasonable to me. I will take a closer look.

borkaehw · 2019-09-10T23:33:09Z

Just pushed changes to address comments, please take a look. Thanks.

SrodriguezO · 2019-09-11T22:03:14Z

    }
-    boolean checkCache = options.remoteAcceptCached;
-
-    if (checkCache) {


How come you removed this?

hmm I see that you moved the progress status down to the other block, but will the logic between here and there cause a delay?

It passes the tests. I don't see a strong reason to keep it separate. I might be wrong. @ishikhman do you have more context on it?

It wouldn't cause tests to fail, the message might just not show up right away

I don't see a bit difference to be honest. Should be fine both ways.

SrodriguezO · 2019-09-11T22:11:54Z


-    if (options.remoteUploadLocalResults) {
+    if (options.remoteUploadLocalResults ||
+        (options.incompatibleRemoteResultsDiskOnly && options.diskCache != null && !options.diskCache.isEmpty())) {


Can we move the options.diskCache != null && !options.diskCache.isEmpty()) part to a helper usesDiskCache method?

SrodriguezO · 2019-09-11T22:20:05Z

  public boolean remoteUploadLocalResults;

+  @Option(
+      name = "incompatible_remote_results_disk_only",


Can we rename to incompatible_remote_results_flags_ignore_disk? The current name makes me think this flag makes remote results flags apply to the disk cache only (which is the opposite of what they do)

Sounds good. I prefer just incompatible_remote_results_ignore_disk to make it a bit shorter.

SrodriguezO · 2019-09-11T22:43:15Z

+      help =
+          "If set to true, --noremote_upload_local_results and --noremote_accept_cached "
+              + "will not apply to the disk cache. "
+              + "If a combined cache is used, --noremote_upload_local_results will cause "


+ "If a combined cache is used: " + "\n\t--noremote_upload_local_results will cause results to be written to the disk cache, but not uploaded to the remote cache" + "\n\t--noremote_accept_cached will result in Bazel checking for results in the disk cache, but not in the remote cache." + "See #8216 for details."

ulfjack · 2019-10-02T09:51:38Z

Ping @buchgr

I think there might be people who use a read-only disk cache on NFS, which is written by CI, and read by everyone else.

borkaehw · 2019-10-07T17:26:09Z

@ishikhman friendly ping here.

I think this is valuable for people. I am happy to address feedback and pushing it toward merged.

ishikhman · 2019-10-08T08:44:23Z

@borkaehw still LGTM :)
Pinging @buchgr for merging part

SrodriguezO · 2019-10-22T21:22:01Z

@buchgr friendly ping : )

SrodriguezO · 2019-11-04T17:40:28Z

@buchgr can you please merge this when you have a minute? It was approved nearly a month ago and is just pending merge. Or maybe @ishikhman can merge it? We've been using this on our fork without issue, and we'd hate to see it rot.

borkaehw · 2019-11-06T18:26:43Z

Since #9200 was merged. Force pushed to address conflicts.

borkaehw · 2019-11-11T22:05:38Z

Since #10200 was merged. Force pushed to address conflicts.

kastiglione · 2019-11-12T04:06:07Z

Just wanted to chime in and say we've been waiting for many releases (since 0.17 or earlier), hoping for this fix soon. Another friendly ping @buchgr :)

borkaehw · 2019-11-18T21:13:52Z

Since #10233 was merged. Force pushed to address conflicts.

Also added tests for combined disk and grpc cache.

borkaehw · 2019-11-18T21:35:14Z

Not sure why //src/test/shell/bazel:bazel_proto_library_test is failing. It doesn't seem to be related to my change.

… in combined blobstore Fixes bazelbuild#8216 RELNOTES: If --noremote_upload_local_results and --disk_cache are provided at the same time, Bazel will only upload local results to disk cache but not remote cache. If --noremote_accept_cached and --disk cache are provided at the same time, Bazel will only check results from disk cache but not remote cache.

borkaehw · 2019-11-25T20:04:41Z

Force push to fix //src/test/shell/bazel:bazel_proto_library_test.

borkaehw · 2019-11-25T21:01:01Z

@buchgr friendly ping. Thanks for making grpc-disk combined cache #10233, it improves our developer workflow dramatically. This fix is the final piece blocking us from using official release of Bazel (we have carried this commit on custom Bazel for a long time). This simple PR will bring a huge value to the community. Would you be able to take a quick look?

borkaehw · 2019-12-19T21:56:05Z

This PR breaks the CI on master, but not the CI on PRs. Here is the fix. #10462

CI is broken because of #9338. It's called `processwrapper-sandbox` on MacOS. This should fix it. Note: MacOS never failed on #9338 until the tests got merged. I wonder why we couldn't catch it earlier. How can we verify it won't fail again? Closes #10462. PiperOrigin-RevId: 286582888

googlebot added the cla: yes label Sep 5, 2019

borkaehw force-pushed the fix-noremote-upload-local-results branch 2 times, most recently from c875fd7 to de90588 Compare September 5, 2019 23:12

borkaehw marked this pull request as ready for review September 5, 2019 23:27

borkaehw requested review from buchgr, ishikhman and ola-rozenfeld as code owners September 5, 2019 23:27

borkaehw force-pushed the fix-noremote-upload-local-results branch from de90588 to 8dfae94 Compare September 6, 2019 19:34

SrodriguezO reviewed Sep 6, 2019

View reviewed changes

Comment thread src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java Outdated

borkaehw force-pushed the fix-noremote-upload-local-results branch from 8dfae94 to ab923b3 Compare September 6, 2019 20:48

ishikhman reviewed Sep 10, 2019

View reviewed changes

borkaehw force-pushed the fix-noremote-upload-local-results branch 2 times, most recently from b2e3e95 to 8630d72 Compare September 10, 2019 23:32

irengrig assigned buchgr Sep 11, 2019

irengrig added the team-Remote-Exec Issues and PRs for the Execution (Remote) team label Sep 11, 2019

SrodriguezO reviewed Sep 11, 2019

View reviewed changes

borkaehw force-pushed the fix-noremote-upload-local-results branch 2 times, most recently from eaefc90 to 4385531 Compare September 11, 2019 23:03

borkaehw force-pushed the fix-noremote-upload-local-results branch from 4385531 to c14deac Compare November 6, 2019 17:36

borkaehw force-pushed the fix-noremote-upload-local-results branch from c14deac to fa2fc22 Compare November 11, 2019 20:12

borkaehw force-pushed the fix-noremote-upload-local-results branch from fa2fc22 to 7fba47a Compare November 18, 2019 20:49

borkaehw force-pushed the fix-noremote-upload-local-results branch from 7fba47a to 765e9ae Compare November 18, 2019 21:19

borkaehw force-pushed the fix-noremote-upload-local-results branch from 765e9ae to ef3bb98 Compare November 25, 2019 19:36

bazel-io closed this in 9c0cab4 Dec 19, 2019

borkaehw mentioned this pull request Dec 19, 2019

Fix combined http/grpc cache tests #10462

Closed

borkaehw deleted the fix-noremote-upload-local-results branch December 20, 2019 18:00

coeuvre mentioned this pull request Mar 30, 2022

incompatible_remote_results_ignore_disk #15148

Closed

Conversation

borkaehw commented Sep 5, 2019

Uh oh!

buchgr commented Sep 6, 2019

Uh oh!

ishikhman commented Sep 6, 2019

Uh oh!

buchgr commented Sep 6, 2019

Uh oh!

ishikhman commented Sep 6, 2019

Uh oh!

SrodriguezO commented Sep 6, 2019

Uh oh!

borkaehw commented Sep 6, 2019

Uh oh!

Uh oh!

ishikhman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

buchgr commented Sep 10, 2019

Uh oh!

SrodriguezO commented Sep 10, 2019

Uh oh!

buchgr commented Sep 10, 2019

Uh oh!

SrodriguezO commented Sep 10, 2019

Uh oh!

borkaehw commented Sep 10, 2019

Uh oh!

borkaehw commented Sep 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SrodriguezO Sep 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ulfjack commented Oct 2, 2019

Uh oh!

borkaehw commented Oct 7, 2019

Uh oh!

ishikhman commented Oct 8, 2019

Uh oh!

SrodriguezO commented Oct 22, 2019

Uh oh!

SrodriguezO commented Nov 4, 2019

Uh oh!

borkaehw commented Nov 6, 2019

Uh oh!

borkaehw commented Nov 11, 2019

Uh oh!

kastiglione commented Nov 12, 2019

Uh oh!

borkaehw commented Nov 18, 2019

Uh oh!

borkaehw commented Nov 18, 2019

SrodriguezO Sep 11, 2019 •

edited

Loading