Skip to content

Commit

Permalink
[5.x] Make remote BES uploader better (bazelbuild#14472)
Browse files Browse the repository at this point in the history
* Remote: Don't upload BES referenced blobs to disk cache.

Fixes bazelbuild#14435.

Closes bazelbuild#14451.

PiperOrigin-RevId: 417629899

* Remote: Make `--incompatible_remote_build_event_upload_respect_no_cache` working with alias.

Fixes bazelbuild#14456.

Closes bazelbuild#14461.

PiperOrigin-RevId: 417637635

* Remote: Make --incompatible_remote_build_event_upload_respect_no_cache working with --incompatible_remote_results_ignore_disk.

Fixes bazelbuild#14463.

Closes bazelbuild#14468.

PiperOrigin-RevId: 417984062
  • Loading branch information
coeuvre authored Dec 23, 2021
1 parent 90965b0 commit dc59d9e
Show file tree
Hide file tree
Showing 8 changed files with 154 additions and 31 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 @@ -356,19 +356,6 @@ public boolean shouldAcceptCachedResult(Spawn spawn) {
}
}

public static boolean shouldUploadLocalResults(
RemoteOptions remoteOptions, @Nullable Map<String, String> executionInfo) {
if (useRemoteCache(remoteOptions)) {
if (useDiskCache(remoteOptions)) {
return shouldUploadLocalResultsToCombinedDisk(remoteOptions, executionInfo);
} else {
return shouldUploadLocalResultsToRemoteCache(remoteOptions, executionInfo);
}
} else {
return shouldUploadLocalResultsToDiskCache(remoteOptions, executionInfo);
}
}

/**
* Returns {@code true} if the local results of the {@code spawn} should be uploaded to remote
* cache.
Expand All @@ -378,7 +365,15 @@ public boolean shouldUploadLocalResults(Spawn spawn) {
return false;
}

return shouldUploadLocalResults(remoteOptions, spawn.getExecutionInfo());
if (useRemoteCache(remoteOptions)) {
if (useDiskCache(remoteOptions)) {
return shouldUploadLocalResultsToCombinedDisk(remoteOptions, spawn);
} else {
return shouldUploadLocalResultsToRemoteCache(remoteOptions, spawn);
}
} else {
return shouldUploadLocalResultsToDiskCache(remoteOptions, spawn);
}
}

/** Returns {@code true} if the spawn may be executed remotely. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -754,12 +754,14 @@ private void parseNoCacheOutputs(AnalysisResult analysisResult) {
}

for (ConfiguredTarget configuredTarget : analysisResult.getTargetsToBuild()) {
// This will either dereference an alias chain, or return the final ConfiguredTarget.
configuredTarget = configuredTarget.getActual();

if (configuredTarget instanceof RuleConfiguredTarget) {
RuleConfiguredTarget ruleConfiguredTarget = (RuleConfiguredTarget) configuredTarget;
for (ActionAnalysisMetadata action : ruleConfiguredTarget.getActions()) {
boolean uploadLocalResults =
RemoteExecutionService.shouldUploadLocalResults(
remoteOptions, action.getExecutionInfo());
Utils.shouldUploadLocalResultsToRemoteCache(remoteOptions, action.getExecutionInfo());
if (!uploadLocalResults) {
for (Artifact output : action.getOutputs()) {
if (output.isTreeArtifact()) {
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
99 changes: 93 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,34 @@ EOF
expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
}

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

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

cat bep.json > $TEST_log
expect_not_log "a:foo.*bytestream://"
expect_log "command.profile.gz.*bytestream://"
}

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 +3392,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 +3414,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 +3424,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 +3438,65 @@ 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"
}

function test_uploader_incompatible_remote_results_ignore_disk() {
mkdir -p a
cat > a/BUILD <<EOF
genrule(
name = 'foo',
outs = ["foo.txt"],
cmd = "echo \"foo bar\" > \$@",
tags = ["no-remote"],
)
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 \
--incompatible_remote_results_ignore_disk \
--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)"
# foo.txt, stdout and stderr for action 'foo'
[[ "$disk_cas_files" == 3 ]] || fail "Expected 3 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 dc59d9e

Please sign in to comment.