Skip to content

Commit dc59d9e

Browse files
coeuvrecoeuvre
authored
[5.x] Make remote BES uploader better (bazelbuild#14472)
* 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
1 parent 90965b0 commit dc59d9e

File tree

8 files changed

+154
-31
lines changed

8 files changed

+154
-31
lines changed

‎src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,7 @@ private Single<PathConverter> upload(Set<Path> files) {
252252

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

257257
return Single.using(
258258
remoteCache::retain,

‎src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java‎

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -356,19 +356,6 @@ public boolean shouldAcceptCachedResult(Spawn spawn) {
356356
}
357357
}
358358

359-
public static boolean shouldUploadLocalResults(
360-
RemoteOptions remoteOptions, @Nullable Map<String, String> executionInfo) {
361-
if (useRemoteCache(remoteOptions)) {
362-
if (useDiskCache(remoteOptions)) {
363-
return shouldUploadLocalResultsToCombinedDisk(remoteOptions, executionInfo);
364-
} else {
365-
return shouldUploadLocalResultsToRemoteCache(remoteOptions, executionInfo);
366-
}
367-
} else {
368-
return shouldUploadLocalResultsToDiskCache(remoteOptions, executionInfo);
369-
}
370-
}
371-
372359
/**
373360
* Returns {@code true} if the local results of the {@code spawn} should be uploaded to remote
374361
* cache.
@@ -378,7 +365,15 @@ public boolean shouldUploadLocalResults(Spawn spawn) {
378365
return false;
379366
}
380367

381-
return shouldUploadLocalResults(remoteOptions, spawn.getExecutionInfo());
368+
if (useRemoteCache(remoteOptions)) {
369+
if (useDiskCache(remoteOptions)) {
370+
return shouldUploadLocalResultsToCombinedDisk(remoteOptions, spawn);
371+
} else {
372+
return shouldUploadLocalResultsToRemoteCache(remoteOptions, spawn);
373+
}
374+
} else {
375+
return shouldUploadLocalResultsToDiskCache(remoteOptions, spawn);
376+
}
382377
}
383378

384379
/** Returns {@code true} if the spawn may be executed remotely. */

‎src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -754,12 +754,14 @@ private void parseNoCacheOutputs(AnalysisResult analysisResult) {
754754
}
755755

756756
for (ConfiguredTarget configuredTarget : analysisResult.getTargetsToBuild()) {
757+
// This will either dereference an alias chain, or return the final ConfiguredTarget.
758+
configuredTarget = configuredTarget.getActual();
759+
757760
if (configuredTarget instanceof RuleConfiguredTarget) {
758761
RuleConfiguredTarget ruleConfiguredTarget = (RuleConfiguredTarget) configuredTarget;
759762
for (ActionAnalysisMetadata action : ruleConfiguredTarget.getActions()) {
760763
boolean uploadLocalResults =
761-
RemoteExecutionService.shouldUploadLocalResults(
762-
remoteOptions, action.getExecutionInfo());
764+
Utils.shouldUploadLocalResultsToRemoteCache(remoteOptions, action.getExecutionInfo());
763765
if (!uploadLocalResults) {
764766
for (Artifact output : action.getOutputs()) {
765767
if (output.isTreeArtifact()) {

‎src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContext.java‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,14 @@
2020

2121
/** A context that provide remote execution related information for executing an action remotely. */
2222
public interface RemoteActionExecutionContext {
23+
/** The type of the context. */
24+
enum Type {
25+
REMOTE_EXECUTION,
26+
BUILD_EVENT_SERVICE,
27+
}
28+
29+
/** Returns the {@link Type} of the context. */
30+
Type getType();
2331

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

4755
/** Creates a {@link SimpleRemoteActionExecutionContext} with given {@link RequestMetadata}. */
4856
static RemoteActionExecutionContext create(RequestMetadata metadata) {
49-
return new SimpleRemoteActionExecutionContext(/*spawn=*/ null, metadata, new NetworkTime());
57+
return new SimpleRemoteActionExecutionContext(
58+
/*type=*/ Type.REMOTE_EXECUTION, /*spawn=*/ null, metadata, new NetworkTime());
5059
}
5160

5261
/**
5362
* Creates a {@link SimpleRemoteActionExecutionContext} with given {@link Spawn} and {@link
5463
* RequestMetadata}.
5564
*/
5665
static RemoteActionExecutionContext create(@Nullable Spawn spawn, RequestMetadata metadata) {
57-
return new SimpleRemoteActionExecutionContext(spawn, metadata, new NetworkTime());
66+
return new SimpleRemoteActionExecutionContext(
67+
/*type=*/ Type.REMOTE_EXECUTION, spawn, metadata, new NetworkTime());
68+
}
69+
70+
static RemoteActionExecutionContext createForBES(RequestMetadata metadata) {
71+
return new SimpleRemoteActionExecutionContext(
72+
/*type=*/ Type.BUILD_EVENT_SERVICE, /*spawn=*/ null, metadata, new NetworkTime());
5873
}
5974
}

‎src/main/java/com/google/devtools/build/lib/remote/common/SimpleRemoteActionExecutionContext.java‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,17 +20,24 @@
2020
/** A {@link RemoteActionExecutionContext} implementation */
2121
public class SimpleRemoteActionExecutionContext implements RemoteActionExecutionContext {
2222

23+
private final Type type;
2324
private final Spawn spawn;
2425
private final RequestMetadata requestMetadata;
2526
private final NetworkTime networkTime;
2627

2728
public SimpleRemoteActionExecutionContext(
28-
Spawn spawn, RequestMetadata requestMetadata, NetworkTime networkTime) {
29+
Type type, Spawn spawn, RequestMetadata requestMetadata, NetworkTime networkTime) {
30+
this.type = type;
2931
this.spawn = spawn;
3032
this.requestMetadata = requestMetadata;
3133
this.networkTime = networkTime;
3234
}
3335

36+
@Override
37+
public Type getType() {
38+
return type;
39+
}
40+
3441
@Nullable
3542
@Override
3643
public Spawn getSpawn() {

‎src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -74,13 +74,14 @@ public void close() {
7474
@Override
7575
public ListenableFuture<Void> uploadFile(
7676
RemoteActionExecutionContext context, Digest digest, Path file) {
77+
// For BES upload, we only upload to the remote cache.
78+
if (context.getType() == RemoteActionExecutionContext.Type.BUILD_EVENT_SERVICE) {
79+
return remoteCache.uploadFile(context, digest, file);
80+
}
81+
7782
ListenableFuture<Void> future = diskCache.uploadFile(context, digest, file);
7883

79-
boolean uploadForSpawn = context.getSpawn() != null;
80-
// If not upload for spawn e.g. for build event artifacts, we always upload files to remote
81-
// cache.
82-
if (!uploadForSpawn
83-
|| options.isRemoteExecutionEnabled()
84+
if (options.isRemoteExecutionEnabled()
8485
|| shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) {
8586
future =
8687
Futures.transformAsync(
@@ -113,6 +114,12 @@ public ListenableFuture<ImmutableSet<Digest>> findMissingDigests(
113114
if (options.isRemoteExecutionEnabled()) {
114115
return remoteCache.findMissingDigests(context, digests);
115116
}
117+
118+
// For BES upload, we only check the remote cache.
119+
if (context.getType() == RemoteActionExecutionContext.Type.BUILD_EVENT_SERVICE) {
120+
return remoteCache.findMissingDigests(context, digests);
121+
}
122+
116123
ListenableFuture<ImmutableSet<Digest>> diskQuery =
117124
diskCache.findMissingDigests(context, digests);
118125
if (shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) {

‎src/test/shell/bazel/remote/remote_execution_test.sh‎

Lines changed: 93 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3296,7 +3296,7 @@ EOF
32963296
//a:consumer >& $TEST_log || fail "Failed to build without remote cache"
32973297
}
32983298

3299-
function test_uploader_respsect_no_cache() {
3299+
function test_uploader_respect_no_cache() {
33003300
mkdir -p a
33013301
cat > a/BUILD <<EOF
33023302
genrule(
@@ -3318,7 +3318,34 @@ EOF
33183318
expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
33193319
}
33203320

3321-
function test_uploader_respsect_no_cache_trees() {
3321+
function test_uploader_alias_action_respect_no_cache() {
3322+
mkdir -p a
3323+
cat > a/BUILD <<EOF
3324+
genrule(
3325+
name = 'foo',
3326+
outs = ["foo.txt"],
3327+
cmd = "echo \"foo bar\" > \$@",
3328+
tags = ["no-cache"],
3329+
)
3330+
3331+
alias(
3332+
name = 'foo-alias',
3333+
actual = '//a:foo',
3334+
)
3335+
EOF
3336+
3337+
bazel build \
3338+
--remote_cache=grpc://localhost:${worker_port} \
3339+
--incompatible_remote_build_event_upload_respect_no_cache \
3340+
--build_event_json_file=bep.json \
3341+
//a:foo-alias >& $TEST_log || fail "Failed to build"
3342+
3343+
cat bep.json > $TEST_log
3344+
expect_not_log "a:foo.*bytestream://"
3345+
expect_log "command.profile.gz.*bytestream://"
3346+
}
3347+
3348+
function test_uploader_respect_no_cache_trees() {
33223349
mkdir -p a
33233350
cat > a/output_dir.bzl <<'EOF'
33243351
def _gen_output_dir_impl(ctx):
@@ -3365,7 +3392,7 @@ EOF
33653392
expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
33663393
}
33673394

3368-
function test_uploader_respsect_no_upload_results() {
3395+
function test_uploader_respect_no_upload_results() {
33693396
mkdir -p a
33703397
cat > a/BUILD <<EOF
33713398
genrule(
@@ -3387,7 +3414,7 @@ EOF
33873414
expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
33883415
}
33893416

3390-
function test_uploader_respsect_no_upload_results_combined_cache() {
3417+
function test_uploader_respect_no_upload_results_combined_cache() {
33913418
mkdir -p a
33923419
cat > a/BUILD <<EOF
33933420
genrule(
@@ -3397,9 +3424,11 @@ genrule(
33973424
)
33983425
EOF
33993426

3427+
cache_dir=$(mktemp -d)
3428+
34003429
bazel build \
34013430
--remote_cache=grpc://localhost:${worker_port} \
3402-
--disk_cache="${TEST_TMPDIR}/disk_cache" \
3431+
--disk_cache=$cache_dir \
34033432
--remote_upload_local_results=false \
34043433
--incompatible_remote_build_event_upload_respect_no_cache \
34053434
--build_event_json_file=bep.json \
@@ -3409,7 +3438,65 @@ EOF
34093438
expect_not_log "a:foo.*bytestream://" || fail "local files are converted"
34103439
expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
34113440
remote_cas_files="$(count_remote_cas_files)"
3412-
[[ "$remote_cas_files" == 1 ]] || fail "Expected 1 remote action cache entries, not $remote_cas_files"
3441+
[[ "$remote_cas_files" == 1 ]] || fail "Expected 1 remote cas entries, not $remote_cas_files"
3442+
}
3443+
3444+
function test_uploader_ignore_disk_cache_of_combined_cache() {
3445+
mkdir -p a
3446+
cat > a/BUILD <<EOF
3447+
genrule(
3448+
name = 'foo',
3449+
outs = ["foo.txt"],
3450+
cmd = "echo \"foo bar\" > \$@",
3451+
tags = ["no-cache"],
3452+
)
3453+
EOF
3454+
3455+
cache_dir=$(mktemp -d)
3456+
3457+
bazel build \
3458+
--remote_cache=grpc://localhost:${worker_port} \
3459+
--disk_cache=$cache_dir \
3460+
--incompatible_remote_build_event_upload_respect_no_cache \
3461+
--build_event_json_file=bep.json \
3462+
//a:foo >& $TEST_log || fail "Failed to build"
3463+
3464+
cat bep.json > $TEST_log
3465+
expect_not_log "a:foo.*bytestream://" || fail "local files are converted"
3466+
expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
3467+
3468+
disk_cas_files="$(count_disk_cas_files $cache_dir)"
3469+
[[ "$disk_cas_files" == 0 ]] || fail "Expected 0 disk cas entries, not $disk_cas_files"
3470+
}
3471+
3472+
function test_uploader_incompatible_remote_results_ignore_disk() {
3473+
mkdir -p a
3474+
cat > a/BUILD <<EOF
3475+
genrule(
3476+
name = 'foo',
3477+
outs = ["foo.txt"],
3478+
cmd = "echo \"foo bar\" > \$@",
3479+
tags = ["no-remote"],
3480+
)
3481+
EOF
3482+
3483+
cache_dir=$(mktemp -d)
3484+
3485+
bazel build \
3486+
--remote_cache=grpc://localhost:${worker_port} \
3487+
--disk_cache=$cache_dir \
3488+
--incompatible_remote_build_event_upload_respect_no_cache \
3489+
--incompatible_remote_results_ignore_disk \
3490+
--build_event_json_file=bep.json \
3491+
//a:foo >& $TEST_log || fail "Failed to build"
3492+
3493+
cat bep.json > $TEST_log
3494+
expect_not_log "a:foo.*bytestream://" || fail "local files are converted"
3495+
expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
3496+
3497+
disk_cas_files="$(count_disk_cas_files $cache_dir)"
3498+
# foo.txt, stdout and stderr for action 'foo'
3499+
[[ "$disk_cas_files" == 3 ]] || fail "Expected 3 disk cas entries, not $disk_cas_files"
34133500
}
34143501

34153502
run_suite "Remote execution and remote cache tests"

‎src/test/shell/bazel/remote/remote_utils.sh‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,16 @@ function count_disk_ac_files() {
7171
fi
7272
}
7373

74+
# Pass in the root of the disk cache and count number of files under /cas directory
75+
# output int to stdout
76+
function count_disk_cas_files() {
77+
if [ -d "$1/cas" ]; then
78+
expr $(find "$1/cas" -type f | wc -l)
79+
else
80+
echo 0
81+
fi
82+
}
83+
7484
function count_remote_ac_files() {
7585
if [ -d "$cas_path/ac" ]; then
7686
expr $(find "$cas_path/ac" -type f | wc -l)

0 commit comments

Comments
 (0)