Bazel: The `no-cache` tag / execution requirement is broken in 0.9.0

[tybook]

Description of the problem / feature request / question:

As of release 0.9.0, setting the no-cache execution requirement on a Skylark rule or the no-cache tag on a built-in rule does not properly prevent a target's output from being uploaded to the remote cache server. It _does_ prevent the output from being downloaded from the server at build time, so we're getting the worst of both worlds here: storage space usage on the remote cache server but no build acceleration.

If possible, provide a minimal example to reproduce the problem:

Remote caching enabled in tools/bazel.rc via

build --experimental_remote_spawn_cache
build --remote_rest_cache=http://remote.cache.server.url

1. Build a target with the no-cache tag or execution requirement set.
2. Check your remote cache server and notice its output was uploaded
3. bazel clean to clear local cache
4. Build the target again and notice it is rebuilt locally, not downloading from the remote cache

Environment info

• Operating System:
  Ubuntu 16.04

• Bazel version (output of bazel info release):
  release 0.9.0

Have you found anything relevant by searching the web?

Anything else, information or logs or outputs that would be helpful?

(If they are large, please upload as attachment or provide link).

[tybook]

Related to https://github.com/bazelbuild/bazel/issues/3960

[buchgr]

Thanks for the report. Does this also happen when using
--spawn_strategy=remote --genrule_strategy=remote --strategy=Javac=remote --strategy=Closure=remote
instead of --experimental_remote_spawn_cache?

[tybook]

Applying the no-cache tag to a target when building with

--spawn_strategy=remote 
--genrule_strategy=remote 
--strategy=Javac=remote 
--strategy=Closure=remote

makes it fall back to local unsandboxed execution (is this expected?), which does not use remote caching. We would really like to continue sandboxed building of the targets we want to exclude from remote caching, so this wouldn't be a good workaround for us.

[ulfjack]

It looks like this is due to @ola-rozenfeld's change a22d0e9c14e58b29d81f5a83bdcc6e5fce52eafe.

[ola-rozenfeld]

Interesting, that is the opposite of the behavior I was aiming for with my change. Looking into it.

[buchgr]

@ola-rozenfeld do you have time to work on it right now? This is a release blocker for 0.10.0.

[ola-rozenfeld]

I can look at it Friday the earliest, not right now. Do you have time earlier?

[buchgr]

Yes, I ll fix it.

[ola-rozenfeld]

Great, thank you!

[buchgr]

Interesting, that is the opposite of the behavior I was aiming for with my change. Looking into it.

Ola, your change description indicates that this is exactly what you were aiming for. In there it says "regardless of whether a spawn is cacheable or not, its artifacts should be uploaded to the remote cache."

Can you please elaborate your thoughts behind this behavior - I would like to understand the problem you were trying to solve.

Thanks.

[ola-rozenfeld]

Oh, by output you meant the artifacts? Yes. I'm really sorry, I misread the bug description. Yes, this was intentional -- if a test is marked as non-cacheable (which they are), its XML file and stdout should still be uploaded to the remote cache, to be available for viewing in the CI UI. That was, in fact, the reported bug that my change fixed (test logs were missing remotely).
I understand this is inefficient, both time and storage-wise, but I'm not sure how to solve this. I mean, we could say that tests are special actions and the above logic only applies to tests, but I'm not sure it's the right thing to do.

[buchgr]

Thanks for the clarification. I ll remove this is a release blocker. I think this will need more discussion. Essentially, we would like to upload any blob referenced by the Build Even Protocol (BEP). We should not upload anything else. It's easy to imagine a user marking a target as "no-cache" because of i.e. a humongous output size.

[tybook]

It's easy to imagine a user marking a target as "no-cache" because of i.e. a humongous output size.

This is exactly what we were doing with no-cache up to 0.9.0. Our remote cache's hit rate is now significantly degraded because it fills up with rarely-used huge outputs.

[ola-rozenfeld]

I guess we will have to make this behavior a special case for test actions.
Ugly, but makes sense.

On Wed, Jan 17, 2018 at 1:31 PM, Ty Book [e-mail] wrote:

It's easy to imagine a user marking a target as "no-cache" because of i.e.
a humongous output size.
This is exactly what we were doing with no-cache up to 0.9.0. Our remote
cache hit rate is now significantly degraded because it fills up with
rarely-used huge outputs.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/bazelbuild/bazel/issues/4343#issuecomment-358398355,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AYoKuGtMEMDpD4qBp3lM2kQVaT8EkUgUks5tLjxlgaJpZM4RKYkC
.

[buchgr]

I guess we will have to make this behavior a special case for test actions.
Ugly, but makes sense.

That's not enough. In order to solve this once and for all everything referenced in the BEP needs to be uploaded.

[ola-rozenfeld]

Right. What are the other things referenced by the BEP?

On Thu, Jan 18, 2018 at 7:02 AM, Jakob Buchgraber [e-mail]
wrote:

I guess we will have to make this behavior a special case for test actions.
Ugly, but makes sense.

That's not enough. In order to solve this once and for all everything
referenced in the BEP needs to be uploaded.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/bazelbuild/bazel/issues/4343#issuecomment-358626748,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AYoKuHrk6eTyyMM2Fg45VOFVtVY1gnRpks5tLzLpgaJpZM4RKYkC
.

[tybook]

Is there any update on this?

[buchgr]

@tybook Yes, I am working on resolving this. I expect this to land in about 3 weeks.

[benjaminp]

This bug indicates a tension between the needs of local-but-possibly-cached execution and remote execution. To wit, users of --remote_upload_local_results seem to expect no-cache to prevent all cache interaction, whereas a remote execution, cachable or not, that doesn't upload anything is useless. Would it make sense to never upload anything for no-cache actions running locally but still do output uploads for remotely executing no-cache actions?

[nicolov]

Any update on this?

[leighst]

We'd love to see some traction on this. We have to jump through some serious hoops right now to get caching to work well for us.

[mirandaconrado]

@buchgr do you know if anyone is working on this?

[buchgr]

Nobody is working on this right now. If anyone is interested in picking this up - I'd be happy to talk some more to get you started!

[buchgr]

It's my understanding that #6033 is a massive step in the right direction but doesn't fix the problem entirely because many rules don't pass tags through to actions as well as some actions don't pass them through to spawns.

[tybook]

It's my understanding that #6033 is a massive step in the right direction but doesn't fix the problem entirely because many rules don't pass tags through to actions as well as some actions don't pass them through to spawns.

Do you mean #7011 here? Either way, this is great progress! Thanks all for the work!

[buchgr]

Yep, no-cache generally works for tests but we need to invest some time to fix each action individually. For example, no-cache doesn't work for cc_binary. The following test will fail:

function test_nocache_outputs_not_uploaded() {
  # Test that outputs of a target marked with no-cache are not
  # uploaded to the remote cache. This is a regression test
  # for https://github.com/bazelbuild/bazel/issues/4343
  mkdir -p a
  cat > a/BUILD <<EOF
package(default_visibility = ["//visibility:public"])
cc_binary(
  name = 'program',
  srcs = [ 'program.cc' ],
  tags = ["no-cache"],
)
EOF
  cat > a/program.cc <<EOF
#include <iostream>
int main() { std::cout << "Hello World!" << std::endl; return 0; }
EOF
  bazel build \
      --remote_cache=localhost:${worker_port} \
      //a:program >& $TEST_log \
      || fail "Expected build to succeed"
   $(is_file_uploaded bazel-bin/a/program) \
     && fail "Expected bazel-bin/a/program to not be uploaded to the remote cache"
}

cc: @benjaminp

[tybook]

@buchgr Do you know off-hand if no-cache currently works with genrule and java_binary? Those are the two most important to the codebase I work on.

[buchgr]

I believe it doesn't work for both - but there is only one way to find out :-)

[benjaminp]

It certainly works on genrule.

[glukasiknuro]

What would be needed to pass no_cache tag for cc_binary ?
It would help in cases when stamped binaries are used - right now binary is uploaded to remote cache after each build. But it is not used, because stamp will never match.

[buchgr]

duplicate of #8004