Skip to content

Commit

Permalink
Remote: Don't upload BES referenced blobs to disk cache.
Browse files Browse the repository at this point in the history
Fixes bazelbuild#14435.

Closes bazelbuild#14451.

PiperOrigin-RevId: 417629899
  • Loading branch information
coeuvre committed Dec 23, 2021
1 parent 90965b0 commit 99d2fbe
Show file tree
Hide file tree
Showing 6 changed files with 84 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ private Single<PathConverter> upload(Set<Path> files) {

RequestMetadata metadata =
TracingMetadataUtils.buildMetadata(buildRequestId, commandId, "bes-upload", null);
RemoteActionExecutionContext context = RemoteActionExecutionContext.create(metadata);
RemoteActionExecutionContext context = RemoteActionExecutionContext.createForBES(metadata);

return Single.using(
remoteCache::retain,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@

/** A context that provide remote execution related information for executing an action remotely. */
public interface RemoteActionExecutionContext {
/** The type of the context. */
enum Type {
REMOTE_EXECUTION,
BUILD_EVENT_SERVICE,
}

/** Returns the {@link Type} of the context. */
Type getType();

/** Returns the {@link Spawn} of the action being executed or {@code null}. */
@Nullable
Expand All @@ -46,14 +54,21 @@ default ActionExecutionMetadata getSpawnOwner() {

/** Creates a {@link SimpleRemoteActionExecutionContext} with given {@link RequestMetadata}. */
static RemoteActionExecutionContext create(RequestMetadata metadata) {
return new SimpleRemoteActionExecutionContext(/*spawn=*/ null, metadata, new NetworkTime());
return new SimpleRemoteActionExecutionContext(
/*type=*/ Type.REMOTE_EXECUTION, /*spawn=*/ null, metadata, new NetworkTime());
}

/**
* Creates a {@link SimpleRemoteActionExecutionContext} with given {@link Spawn} and {@link
* RequestMetadata}.
*/
static RemoteActionExecutionContext create(@Nullable Spawn spawn, RequestMetadata metadata) {
return new SimpleRemoteActionExecutionContext(spawn, metadata, new NetworkTime());
return new SimpleRemoteActionExecutionContext(
/*type=*/ Type.REMOTE_EXECUTION, spawn, metadata, new NetworkTime());
}

static RemoteActionExecutionContext createForBES(RequestMetadata metadata) {
return new SimpleRemoteActionExecutionContext(
/*type=*/ Type.BUILD_EVENT_SERVICE, /*spawn=*/ null, metadata, new NetworkTime());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,24 @@
/** A {@link RemoteActionExecutionContext} implementation */
public class SimpleRemoteActionExecutionContext implements RemoteActionExecutionContext {

private final Type type;
private final Spawn spawn;
private final RequestMetadata requestMetadata;
private final NetworkTime networkTime;

public SimpleRemoteActionExecutionContext(
Spawn spawn, RequestMetadata requestMetadata, NetworkTime networkTime) {
Type type, Spawn spawn, RequestMetadata requestMetadata, NetworkTime networkTime) {
this.type = type;
this.spawn = spawn;
this.requestMetadata = requestMetadata;
this.networkTime = networkTime;
}

@Override
public Type getType() {
return type;
}

@Nullable
@Override
public Spawn getSpawn() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,13 +74,14 @@ public void close() {
@Override
public ListenableFuture<Void> uploadFile(
RemoteActionExecutionContext context, Digest digest, Path file) {
// For BES upload, we only upload to the remote cache.
if (context.getType() == RemoteActionExecutionContext.Type.BUILD_EVENT_SERVICE) {
return remoteCache.uploadFile(context, digest, file);
}

ListenableFuture<Void> future = diskCache.uploadFile(context, digest, file);

boolean uploadForSpawn = context.getSpawn() != null;
// If not upload for spawn e.g. for build event artifacts, we always upload files to remote
// cache.
if (!uploadForSpawn
|| options.isRemoteExecutionEnabled()
if (options.isRemoteExecutionEnabled()
|| shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) {
future =
Futures.transformAsync(
Expand Down Expand Up @@ -113,6 +114,12 @@ public ListenableFuture<ImmutableSet<Digest>> findMissingDigests(
if (options.isRemoteExecutionEnabled()) {
return remoteCache.findMissingDigests(context, digests);
}

// For BES upload, we only check the remote cache.
if (context.getType() == RemoteActionExecutionContext.Type.BUILD_EVENT_SERVICE) {
return remoteCache.findMissingDigests(context, digests);
}

ListenableFuture<ImmutableSet<Digest>> diskQuery =
diskCache.findMissingDigests(context, digests);
if (shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) {
Expand Down
42 changes: 36 additions & 6 deletions src/test/shell/bazel/remote/remote_execution_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3296,7 +3296,7 @@ EOF
//a:consumer >& $TEST_log || fail "Failed to build without remote cache"
}

function test_uploader_respsect_no_cache() {
function test_uploader_respect_no_cache() {
mkdir -p a
cat > a/BUILD <<EOF
genrule(
Expand All @@ -3318,7 +3318,7 @@ EOF
expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
}

function test_uploader_respsect_no_cache_trees() {
function test_uploader_respect_no_cache_trees() {
mkdir -p a
cat > a/output_dir.bzl <<'EOF'
def _gen_output_dir_impl(ctx):
Expand Down Expand Up @@ -3365,7 +3365,7 @@ EOF
expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
}

function test_uploader_respsect_no_upload_results() {
function test_uploader_respect_no_upload_results() {
mkdir -p a
cat > a/BUILD <<EOF
genrule(
Expand All @@ -3387,7 +3387,7 @@ EOF
expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
}

function test_uploader_respsect_no_upload_results_combined_cache() {
function test_uploader_respect_no_upload_results_combined_cache() {
mkdir -p a
cat > a/BUILD <<EOF
genrule(
Expand All @@ -3397,9 +3397,11 @@ genrule(
)
EOF

cache_dir=$(mktemp -d)

bazel build \
--remote_cache=grpc://localhost:${worker_port} \
--disk_cache="${TEST_TMPDIR}/disk_cache" \
--disk_cache=$cache_dir \
--remote_upload_local_results=false \
--incompatible_remote_build_event_upload_respect_no_cache \
--build_event_json_file=bep.json \
Expand All @@ -3409,7 +3411,35 @@ EOF
expect_not_log "a:foo.*bytestream://" || fail "local files are converted"
expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
remote_cas_files="$(count_remote_cas_files)"
[[ "$remote_cas_files" == 1 ]] || fail "Expected 1 remote action cache entries, not $remote_cas_files"
[[ "$remote_cas_files" == 1 ]] || fail "Expected 1 remote cas entries, not $remote_cas_files"
}

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

cache_dir=$(mktemp -d)

bazel build \
--remote_cache=grpc://localhost:${worker_port} \
--disk_cache=$cache_dir \
--incompatible_remote_build_event_upload_respect_no_cache \
--build_event_json_file=bep.json \
//a:foo >& $TEST_log || fail "Failed to build"

cat bep.json > $TEST_log
expect_not_log "a:foo.*bytestream://" || fail "local files are converted"
expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"

disk_cas_files="$(count_disk_cas_files $cache_dir)"
[[ "$disk_cas_files" == 0 ]] || fail "Expected 0 disk cas entries, not $disk_cas_files"
}

run_suite "Remote execution and remote cache tests"
10 changes: 10 additions & 0 deletions src/test/shell/bazel/remote/remote_utils.sh
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,16 @@ function count_disk_ac_files() {
fi
}

# Pass in the root of the disk cache and count number of files under /cas directory
# output int to stdout
function count_disk_cas_files() {
if [ -d "$1/cas" ]; then
expr $(find "$1/cas" -type f | wc -l)
else
echo 0
fi
}

function count_remote_ac_files() {
if [ -d "$cas_path/ac" ]; then
expr $(find "$cas_path/ac" -type f | wc -l)
Expand Down

0 comments on commit 99d2fbe

Please sign in to comment.