Skip to content

Commit

Permalink
Fix a serious regression in remote execution
Browse files Browse the repository at this point in the history
Action inputs would not be uploaded to the CAS for targets tagged
with 'no-remote-cache' or 'no-cache'.

The regression was introduced by 8860c3e
  • Loading branch information
buchgr committed Aug 30, 2019
1 parent 5a9fa49 commit c475d7d
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -245,17 +245,13 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
() -> {
ExecuteRequest request = requestBuilder.build();

// Upload the command and all the inputs into the remote cache, if remote caching
// is enabled and not disabled using tags, {@see Spawns#mayBeCachedRemotely}
if (spawnCacheableRemotely) {
try (SilentCloseable c = prof.profile(UPLOAD_TIME, "upload missing inputs")) {
Map<Digest, Message> additionalInputs = Maps.newHashMapWithExpectedSize(2);
additionalInputs.put(actionKey.getDigest(), action);
additionalInputs.put(commandHash, command);
remoteCache.ensureInputsPresent(merkleTree, additionalInputs, execRoot);
}
// Upload the command and all the inputs into the remote cache.
try (SilentCloseable c = prof.profile(UPLOAD_TIME, "upload missing inputs")) {
Map<Digest, Message> additionalInputs = Maps.newHashMapWithExpectedSize(2);
additionalInputs.put(actionKey.getDigest(), action);
additionalInputs.put(commandHash, command);
remoteCache.ensureInputsPresent(merkleTree, additionalInputs, execRoot);
}

ExecuteResponse reply;
try (SilentCloseable c = prof.profile(REMOTE_EXECUTION, "execute remotely")) {
reply = remoteExecutor.executeRemotely(request);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ public void nonCachableSpawnsShouldNotBeCached_localFallback() throws Exception
runner.exec(spawn, policy);

verify(localRunner).exec(spawn, policy);
verify(cache, never()).upload(any(), any(), any(), any(), any(), any());
verify(cache).ensureInputsPresent(any(), any(), eq(execRoot));
verifyNoMoreInteractions(cache);
}

Expand Down
30 changes: 30 additions & 0 deletions src/test/shell/bazel/remote/remote_execution_http_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -423,4 +423,34 @@ EOF
rm -rf $cache
}

function test_tag_no_remote_cache() {
mkdir -p a
cat > a/BUILD <<'EOF'
genrule(
name = "foo",
srcs = [],
outs = ["foo.txt"],
cmd = "echo \"foo\" > \"$@\"",
tags = ["no-remote-cache"]
)
EOF

bazel build \
--spawn_strategy=local \
--remote_cache=grpc://localhost:${worker_port} \
//a:foo >& $TEST_log || "Failed to build //a:foo"

expect_log "1 local"

bazel clean

bazel build \
--spawn_strategy=local \
--remote_cache=grpc://localhost:${worker_port} \
//a:foo || "Failed to build //a:foo"

expect_log "1 local"
expect_not_log "remote cache hit"
}

run_suite "Remote execution and remote cache tests"
49 changes: 49 additions & 0 deletions src/test/shell/bazel/remote/remote_execution_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -1131,6 +1131,55 @@ EOF
expect_log "uri:.*bytestream://localhost"
}

function test_tag_no_remote_cache() {
mkdir -p a
cat > a/BUILD <<'EOF'
genrule(
name = "foo",
srcs = [],
outs = ["foo.txt"],
cmd = "echo \"foo\" > \"$@\"",
tags = ["no-remote-cache"]
)
EOF

bazel build \
--remote_executor=grpc://localhost:${worker_port} \
//a:foo >& $TEST_log || "Failed to build //a:foo"

expect_log "1 remote"

bazel clean

bazel build \
--remote_executor=grpc://localhost:${worker_port} \
//a:foo || "Failed to build //a:foo"

expect_log "1 remote"
expect_not_log "remote cache hit"
}

function test_tag_no_remote_exec() {
mkdir -p a
cat > a/BUILD <<'EOF'
genrule(
name = "foo",
srcs = [],
outs = ["foo.txt"],
cmd = "echo \"foo\" > \"$@\"",
tags = ["no-remote-exec"]
)
EOF

bazel build \
--spawn_strategy=remote,local \
--remote_executor=grpc://localhost:${worker_port} \
//a:foo >& $TEST_log || "Failed to build //a:foo"

expect_log "1 local"
expect_not_log "1 remote"
}

# TODO(alpha): Add a test that fails remote execution when remote worker
# supports sandbox.

Expand Down

0 comments on commit c475d7d

Please sign in to comment.