Change RemoteActionInputFetcher to use temp files before staging by sohaibiftikhar · Pull Request #11340 · bazelbuild/bazel

sohaibiftikhar · 2020-05-09T17:28:42Z

Attempts fixing #11339

googlebot · 2020-05-09T17:28:46Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-05-09T17:30:24Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

sohaibiftikhar · 2020-05-09T17:33:20Z


+  private static Path toTmpDownloadPath(Path actualPath) {
+    return actualPath.getParentDirectory().getRelative(
+      actualPath.getBaseName() + "."  + actualPath.hashCode()


This should give a unique + deterministic enough name so that we don't need to sort all downloads before moving to final location.

Why not use the same approach as RemoteCache?

Because RemoteCache collects all outputs before doing the moveToFinalLocation. This class was designed a bit differently in that it can call downloadFileAsync from two different methods (downloadFile and prefetchFiles) and the shared variables downloadsInProgress and downloadedPaths are controlled per download here.

So I would have to move the synchronized blocks to those two methods and that looked like a lot more refactoring to me. I can make that change if you think that is the way to go. But this seems cleaner to me.

sohaibiftikhar · 2020-05-09T17:34:15Z

+                      "Failed to move to final location %s. -> %s", tmpPath, path
+                    );
+                  }
+                  synchronized (lock) {


Moved the synchronize block below. A file is considered to be downloaded only after its permissions are set and it has been moved to its final location.

I think this is wrong. You are doing this even when the above code fails with an IOException

meisterT · 2020-05-13T04:57:05Z

cc @ulfjack

ulfjack

I would generally prefer a temporary location in a place where it can't conflict with other files. In this case, it could potentially conflict (same for RemoteCache). That said, this seems like a reasonable change. I posted a question on the code above. Also, did you consider rm-ing the file after download instead of downloading to a temporary location.

I wonder how @jmmv does this in the dynamic strategy - I think he said he was planning to download to a temp location as well. Is there a way to make the different implementations more consistent with each other?

ulfjack · 2020-05-13T17:54:43Z


+  private static Path toTmpDownloadPath(Path actualPath) {
+    return actualPath.getParentDirectory().getRelative(
+      actualPath.getBaseName() + "."  + actualPath.hashCode()


Why not use the same approach as RemoteCache?

sohaibiftikhar · 2020-05-14T13:47:20Z

Also, did you consider rm-ing the file after download instead of downloading to a temporary location.

There is a path.delete in the onFailure block that does this. The problem is that this never gets called in case the thread gets terminated from the outside. To be honest I don't quite know why. I couldn't find the place in the code where the termination happens.

Maybe someone with more knowledge of that part could point that out.

jmmv · 2020-05-15T13:50:05Z

I went through various iterations to make the "download as temp" feature work (for the internal version of this feature).

Put the temp file next to the output, with a .tmp suffix. This ended up causing trouble with tree artifacts (which would see "extra" files on disk before they were deleted, and failed validation).
Put the temp files in the actions temp dir, using a file name derived from the original execroot-relative path but translating all path separators into underscores. This ended up running into path limits (on macOS).
Put the temp files in the actions temp dir, using a counter for their file names. This has been working well so far.

So essentially, we do this:

Path tempDestFile =
    actionTempsDir.getChild("output-" + lastTempOutput.getAndAdd(1) + ".tmp");

More details here: https://jmmv.dev/2019/12/bazel-dynamic-execution-download-times.html

Would be great if the external logic of this code did the same.

philwo · 2020-09-23T14:25:57Z

@sohaibiftikhar Are you still working on this?

sohaibiftikhar · 2020-09-24T19:06:25Z

I was involved with some other work that put this on a sidetrack. I can finish this by aligning with what @jmmv proposes here.

meisterT · 2020-10-21T11:24:39Z

What's the current state?

meisterT · 2020-10-21T11:25:44Z

cc @coeuvre

coeuvre · 2020-11-11T06:43:15Z

#12453 fixes #11339 by deleting the partial downloaded files. However, changing how RemoteCache handles the temp download files to what @jmmv suggested would be great.

jin · 2021-01-15T05:43:29Z

Closing this PR as as the referenced issue has been fixed.

googlebot added the cla: no label May 9, 2020

sohaibiftikhar force-pushed the FACT-120 branch from ec9eace to 23cb6d3 Compare May 9, 2020 17:30

googlebot added cla: yes and removed cla: no labels May 9, 2020

sohaibiftikhar commented May 9, 2020

View reviewed changes

aiuto requested a review from meisterT May 13, 2020 04:40

ulfjack reviewed May 13, 2020

View reviewed changes

meisterT added the team-Remote-Exec Issues and PRs for the Execution (Remote) team label Oct 21, 2020

Change download path for RemoteActionInputFetcher to tmp path

8c13d43

sohaibiftikhar force-pushed the FACT-120 branch from 23cb6d3 to 8c13d43 Compare October 31, 2020 17:33

coeuvre mentioned this pull request Nov 11, 2020

Change RemoteCache to use the same strategy as dynamic execution for downloading files #12454

Closed

jin closed this Jan 15, 2021

ghost mentioned this pull request Aug 22, 2022

dynamic execution: tree artifact intermittently contains .tmp files #16145

Closed

sohaibiftikhar deleted the FACT-120 branch February 28, 2024 20:30

Conversation

sohaibiftikhar commented May 9, 2020

Uh oh!

googlebot commented May 9, 2020

Uh oh!

googlebot commented May 9, 2020

Uh oh!

sohaibiftikhar May 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ulfjack May 13, 2020

Choose a reason for hiding this comment

Uh oh!

sohaibiftikhar May 14, 2020

Choose a reason for hiding this comment

Uh oh!

sohaibiftikhar May 9, 2020

Choose a reason for hiding this comment

Uh oh!

meisterT May 13, 2020

Choose a reason for hiding this comment

Uh oh!

meisterT commented May 13, 2020

Uh oh!

ulfjack left a comment

Choose a reason for hiding this comment

Uh oh!

ulfjack May 13, 2020

Choose a reason for hiding this comment

Uh oh!

sohaibiftikhar commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmmv commented May 15, 2020

Uh oh!

philwo commented Sep 23, 2020

Uh oh!

sohaibiftikhar commented Sep 24, 2020

Uh oh!

meisterT commented Oct 21, 2020

Uh oh!

meisterT commented Oct 21, 2020

Uh oh!

coeuvre commented Nov 11, 2020

Uh oh!

jin commented Jan 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

sohaibiftikhar May 9, 2020 •

edited

Loading

sohaibiftikhar commented May 14, 2020 •

edited

Loading