Skip to content

Commit

Permalink
Fix checking remote cache for omitted files in buildevent file (#15405)
Browse files Browse the repository at this point in the history
Enabling both `--noremote_upload_local_results` and
`--incompatible_remote_build_event_upload_respect_no_cache` caused
ByteStreamBuildEventArtifactuploader not to check if some files already
existed remotely because local files were not to be uploaded. When using
remote execution some of these files might exist remotely, so we want to
check remote cache for those files even if we would not upload them if
missing. The test case included depicts this failure case.

Closes #15280.

PiperOrigin-RevId: 442798312
(cherry picked from commit 317375d)

Co-authored-by: Emil Kattainen <emilkattainen@gmail.com>
  • Loading branch information
brentleyjones and exoson authored May 9, 2022
1 parent 2186d15 commit ad74d52
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 20 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -107,12 +107,14 @@ private static final class PathMetadata {
private final Digest digest;
private final boolean directory;
private final boolean remote;
private final boolean omitted;

PathMetadata(Path path, Digest digest, boolean directory, boolean remote) {
PathMetadata(Path path, Digest digest, boolean directory, boolean remote, boolean omitted) {
this.path = path;
this.digest = digest;
this.directory = directory;
this.remote = remote;
this.omitted = omitted;
}

public Path getPath() {
Expand All @@ -130,6 +132,10 @@ public boolean isDirectory() {
public boolean isRemote() {
return remote;
}

public boolean isOmitted() {
return omitted;
}
}

/**
Expand All @@ -138,22 +144,29 @@ public boolean isRemote() {
*/
private PathMetadata readPathMetadata(Path file) throws IOException {
if (file.isDirectory()) {
return new PathMetadata(file, /* digest= */ null, /* directory= */ true, /* remote= */ false);
}
if (omittedFiles.contains(file)) {
return new PathMetadata(file, /*digest=*/ null, /*directory=*/ false, /*remote=*/ false);
return new PathMetadata(
file,
/* digest= */ null,
/* directory= */ true,
/* remote= */ false,
/* omitted= */ false);
}

for (Path treeRoot : omittedTreeRoots) {
if (file.startsWith(treeRoot)) {
omittedFiles.add(file);
return new PathMetadata(file, /*digest=*/ null, /*directory=*/ false, /*remote=*/ false);
boolean omitted = false;
if (omittedFiles.contains(file)) {
omitted = true;
} else {
for (Path treeRoot : omittedTreeRoots) {
if (file.startsWith(treeRoot)) {
omittedFiles.add(file);
omitted = true;
}
}
}

DigestUtil digestUtil = new DigestUtil(file.getFileSystem().getDigestFunction());
Digest digest = digestUtil.compute(file);
return new PathMetadata(file, digest, /* directory= */ false, isRemoteFile(file));
return new PathMetadata(file, digest, /* directory= */ false, isRemoteFile(file), omitted);
}

private static void processQueryResult(
Expand All @@ -166,14 +179,18 @@ private static void processQueryResult(
} else {
PathMetadata remotePathMetadata =
new PathMetadata(
file.getPath(), file.getDigest(), file.isDirectory(), /* remote= */ true);
file.getPath(),
file.getDigest(),
file.isDirectory(),
/* remote= */ true,
file.isOmitted());
knownRemotePaths.add(remotePathMetadata);
}
}
}

private static boolean shouldUpload(PathMetadata path) {
return path.getDigest() != null && !path.isRemote() && !path.isDirectory();
return path.getDigest() != null && !path.isRemote() && !path.isDirectory() && !path.isOmitted();
}

private Single<List<PathMetadata>> queryRemoteCache(
Expand All @@ -182,7 +199,8 @@ private Single<List<PathMetadata>> queryRemoteCache(
List<PathMetadata> filesToQuery = new ArrayList<>();
Set<Digest> digestsToQuery = new HashSet<>();
for (PathMetadata path : paths) {
if (shouldUpload(path)) {
// Query remote cache for files even if omitted from uploading
if (shouldUpload(path) || path.isOmitted()) {
filesToQuery.add(path);
digestsToQuery.add(path.getDigest());
} else {
Expand Down Expand Up @@ -239,7 +257,8 @@ private Single<List<PathMetadata>> uploadLocalFiles(
path.getPath(),
/*digest=*/ null,
path.isDirectory(),
path.isRemote()));
path.isRemote(),
path.isOmitted()));
});
})
.collect(Collectors.toList());
Expand All @@ -265,13 +284,17 @@ private Single<PathConverter> upload(Set<Path> files) {
} catch (IOException e) {
reporterUploadError(e);
return new PathMetadata(
file, /*digest=*/ null, /*directory=*/ false, /*remote=*/ false);
file,
/*digest=*/ null,
/*directory=*/ false,
/*remote=*/ false,
/*omitted=*/ false);
}
})
.collect(Collectors.toList())
.flatMap(paths -> queryRemoteCache(remoteCache, context, paths))
.flatMap(paths -> uploadLocalFiles(remoteCache, context, paths))
.map(paths -> new PathConverterImpl(remoteServerInstanceName, paths, omittedFiles)),
.map(paths -> new PathConverterImpl(remoteServerInstanceName, paths)),
RemoteCache::release);
}

Expand Down Expand Up @@ -305,23 +328,25 @@ private static class PathConverterImpl implements PathConverter {
private final Set<Path> skippedPaths;
private final Set<Path> localPaths;

PathConverterImpl(
String remoteServerInstanceName, List<PathMetadata> uploads, Set<Path> localPaths) {
PathConverterImpl(String remoteServerInstanceName, List<PathMetadata> uploads) {
Preconditions.checkNotNull(uploads);
this.remoteServerInstanceName = remoteServerInstanceName;
pathToDigest = new HashMap<>(uploads.size());
ImmutableSet.Builder<Path> skippedPaths = ImmutableSet.builder();
ImmutableSet.Builder<Path> localPaths = ImmutableSet.builder();
for (PathMetadata pair : uploads) {
Path path = pair.getPath();
Digest digest = pair.getDigest();
if (digest != null) {
if (!pair.isRemote() && pair.isOmitted()) {
localPaths.add(path);
} else if (digest != null) {
pathToDigest.put(path, digest);
} else {
skippedPaths.add(path);
}
}
this.skippedPaths = skippedPaths.build();
this.localPaths = localPaths;
this.localPaths = localPaths.build();
}

@Override
Expand Down
29 changes: 29 additions & 0 deletions src/test/shell/bazel/remote/remote_execution_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3498,6 +3498,35 @@ EOF
[[ "$remote_cas_files" == 1 ]] || fail "Expected 1 remote cas entries, not $remote_cas_files"
}

function test_remote_exec_convert_remote_file() {
mkdir -p a
cat > a/BUILD <<'EOF'
sh_test(
name = "test",
srcs = ["test.sh"],
)
EOF
cat > a/test.sh <<'EOF'
#!/bin/bash
set -e
echo \"foo\"
exit 0
EOF
chmod 755 a/test.sh

bazel test \
--remote_executor=grpc://localhost:${worker_port} \
--build_event_json_file=bep.json \
--noremote_upload_local_results \
--incompatible_remote_build_event_upload_respect_no_cache \
//a:test >& $TEST_log || "Failed to test //a:test"

cat bep.json > $TEST_log
expect_not_log "test\.log.*file://" || fail "remote files generated in remote execution are not converted"
expect_log "test\.log.*bytestream://" || fail "remote files generated in remote execution are not converted"
}


function test_uploader_ignore_disk_cache_of_combined_cache() {
mkdir -p a
cat > a/BUILD <<EOF
Expand Down

0 comments on commit ad74d52

Please sign in to comment.