From ec7be346adc00c4bde22d116fca80ef59da66121 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 25 Oct 2022 02:27:50 -0700 Subject: [PATCH] Temporarily set parent directory of the input to writable if it is not. PiperOrigin-RevId: 483615392 Change-Id: Ic8b301ef6fd004a88f9a563e04ba961f6dfb5708 --- .../remote/AbstractActionInputPrefetcher.java | 66 +++++++++++++++---- .../lib/skyframe/SkyframeActionExecutor.java | 42 +++++++----- 2 files changed, 78 insertions(+), 30 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java index 81b7ebe1d73c3f..2b203949f370df 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java @@ -70,6 +70,20 @@ public abstract class AbstractActionInputPrefetcher implements ActionInputPrefet protected final Path execRoot; protected final ImmutableList patternsToDownload; + private static class Context { + private final Set nonWritableDirs = Sets.newConcurrentHashSet(); + + public void addNonWritableDir(Path dir) { + nonWritableDirs.add(dir); + } + + public void finalizeContext() throws IOException { + for (Path path : nonWritableDirs) { + path.setWritable(false); + } + } + } + /** Priority for the staging task. */ protected enum Priority { /** @@ -176,27 +190,37 @@ protected ListenableFuture prefetchFiles( files.add(input); } + Context context = new Context(); + Flowable treeDownloads = Flowable.fromIterable(trees.entrySet()) .flatMapSingle( entry -> toTransferResult( prefetchInputTreeOrSymlink( - metadataProvider, entry.getKey(), entry.getValue(), priority))); + context, + metadataProvider, + entry.getKey(), + entry.getValue(), + priority))); Flowable fileDownloads = Flowable.fromIterable(files) .flatMapSingle( input -> toTransferResult( - prefetchInputFileOrSymlink(metadataProvider, input, priority))); + prefetchInputFileOrSymlink(context, metadataProvider, input, priority))); Flowable transfers = Flowable.merge(treeDownloads, fileDownloads); - Completable prefetch = mergeBulkTransfer(transfers).onErrorResumeNext(this::onErrorResumeNext); + Completable prefetch = + Completable.using( + () -> context, ctx -> mergeBulkTransfer(transfers), Context::finalizeContext) + .onErrorResumeNext(this::onErrorResumeNext); return toListenableFuture(prefetch); } private Completable prefetchInputTreeOrSymlink( + Context context, MetadataProvider provider, SpecialArtifact tree, List treeFiles, @@ -216,7 +240,7 @@ private Completable prefetchInputTreeOrSymlink( PathFragment prefetchExecPath = treeMetadata.getMaterializationExecPath().orElse(execPath); Completable prefetch = - prefetchInputTree(provider, prefetchExecPath, treeFiles, treeMetadata, priority); + prefetchInputTree(context, provider, prefetchExecPath, treeFiles, treeMetadata, priority); // If prefetching to a different path, plant a symlink into it. if (!prefetchExecPath.equals(execPath)) { @@ -249,6 +273,7 @@ private boolean shouldDownloadAnyTreeFiles( } private Completable prefetchInputTree( + Context context, MetadataProvider provider, PathFragment execPath, List treeFiles, @@ -303,7 +328,7 @@ private Completable prefetchInputTree( } } checkState(dir.equals(path)); - finalizeDownload(tempPath, path); + finalizeDownload(context, tempPath, path); } for (Path dir : dirs) { @@ -337,7 +362,8 @@ private Completable prefetchInputTree( } private Completable prefetchInputFileOrSymlink( - MetadataProvider metadataProvider, ActionInput input, Priority priority) throws IOException { + Context context, MetadataProvider metadataProvider, ActionInput input, Priority priority) + throws IOException { if (input instanceof VirtualActionInput) { prefetchVirtualActionInput((VirtualActionInput) input); return Completable.complete(); @@ -353,7 +379,7 @@ private Completable prefetchInputFileOrSymlink( PathFragment prefetchExecPath = metadata.getMaterializationExecPath().orElse(execPath); Completable prefetch = - downloadFileNoCheckRx(execRoot.getRelative(prefetchExecPath), metadata, priority); + downloadFileNoCheckRx(context, execRoot.getRelative(prefetchExecPath), metadata, priority); // If prefetching to a different path, plant a symlink into it. if (!prefetchExecPath.equals(execPath)) { @@ -371,15 +397,16 @@ private Completable prefetchInputFileOrSymlink( *

The file will be written into a temporary file and moved to the final destination after the * download finished. */ - private Completable downloadFileRx(Path path, FileArtifactValue metadata, Priority priority) { + private Completable downloadFileRx( + Context context, Path path, FileArtifactValue metadata, Priority priority) { if (!canDownloadFile(path, metadata)) { return Completable.complete(); } - return downloadFileNoCheckRx(path, metadata, priority); + return downloadFileNoCheckRx(context, path, metadata, priority); } private Completable downloadFileNoCheckRx( - Path path, FileArtifactValue metadata, Priority priority) { + Context context, Path path, FileArtifactValue metadata, Priority priority) { if (path.isSymbolicLink()) { try { path = path.getRelative(path.readSymbolicLink()); @@ -402,7 +429,7 @@ private Completable downloadFileNoCheckRx( directExecutor()) .doOnComplete( () -> { - finalizeDownload(tempPath, finalPath); + finalizeDownload(context, tempPath, finalPath); completed.set(true); }), tempPath -> { @@ -439,11 +466,24 @@ public void downloadFile(Path path, FileArtifactValue metadata) protected ListenableFuture downloadFileAsync( PathFragment path, FileArtifactValue metadata, Priority priority) { + Context context = new Context(); return toListenableFuture( - downloadFileRx(execRoot.getFileSystem().getPath(path), metadata, priority)); + Completable.using( + () -> context, + ctx -> + downloadFileRx(context, execRoot.getFileSystem().getPath(path), metadata, priority), + Context::finalizeContext)); } - private void finalizeDownload(Path tmpPath, Path path) throws IOException { + private void finalizeDownload(Context context, Path tmpPath, Path path) throws IOException { + Path parentDir = path.getParentDirectory(); + // In case the parent directory of the destination is not writable, temporarily change it to + // writable. b/254844173. + if (parentDir != null && !parentDir.isWritable()) { + context.addNonWritableDir(parentDir); + parentDir.setWritable(true); + } + // The permission of output file is changed to 0555 after action execution. We manually change // the permission here for the downloaded file to keep this behaviour consistent. tmpPath.chmod(0555); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java index 644f289e193e5b..65777cec7c6458 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java @@ -727,8 +727,10 @@ void updateActionCache( // Skyframe has already done all the filesystem access needed for outputs and swallows // IOExceptions for inputs. So an IOException is impossible here. throw new IllegalStateException( - "failed to update action cache for " + action.prettyPrint() - + ", but all outputs should already have been checked", e); + "failed to update action cache for " + + action.prettyPrint() + + ", but all outputs should already have been checked", + e); } } @@ -866,12 +868,12 @@ void recordExecutionError() { } /** - * Returns true if the Builder is winding down (i.e. cancelling outstanding - * actions and preparing to abort.) - * The builder is winding down iff: + * Returns true if the Builder is winding down (i.e. cancelling outstanding actions and preparing + * to abort.) The builder is winding down iff: + * *

    - *
  • we had an execution error - *
  • we are not running with --keep_going + *
  • we had an execution error + *
  • we are not running with --keep_going *
*/ private boolean isBuilderAborting() { @@ -1197,8 +1199,10 @@ private ActionExecutionValue actuallyCompleteAction( Artifact primaryOutput = action.getPrimaryOutput(); Path primaryOutputPath = actionExecutionContext.getInputPath(primaryOutput); try { - Preconditions.checkState(action.inputsDiscovered(), - "Action %s successfully executed, but inputs still not known", action); + Preconditions.checkState( + action.inputsDiscovered(), + "Action %s successfully executed, but inputs still not known", + action); try { flushActionFileSystem(actionExecutionContext.getActionFileSystem(), outputService); @@ -1481,12 +1485,15 @@ private static void reportMissingOutputFile( String msg = prefix + "is a dangling symbolic link"; reporter.handle(Event.error(action.getOwner().getLocation(), msg)); } else { - String suffix = genrule ? " by genrule. This is probably " - + "because the genrule actually didn't create this output, or because the output was a " - + "directory and the genrule was run remotely (note that only the contents of " - + "declared file outputs are copied from genrules run remotely)" : ""; - reporter.handle(Event.error( - action.getOwner().getLocation(), prefix + "was not created" + suffix)); + String suffix = + genrule + ? " by genrule. This is probably because the genrule actually didn't create this" + + " output, or because the output was a directory and the genrule was run" + + " remotely (note that only the contents of declared file outputs are copied" + + " from genrules run remotely)" + : ""; + reporter.handle( + Event.error(action.getOwner().getLocation(), prefix + "was not created" + suffix)); } } @@ -1496,8 +1503,9 @@ private static void reportOutputTreeArtifactErrors( if (e instanceof FileNotFoundException) { errorMessage = String.format("TreeArtifact %s was not created", output.prettyPrint()); } else { - errorMessage = String.format( - "Error while validating output TreeArtifact %s : %s", output, e.getMessage()); + errorMessage = + String.format( + "Error while validating output TreeArtifact %s : %s", output, e.getMessage()); } reporter.handle(Event.error(action.getOwner().getLocation(), errorMessage));