598098 - change fuzzer jobs to only work on builds that have symbols available

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P4)

Description

[Jesse Ruderman]

* Would prevent fuzzing jobs from pulling a build that doesn't have symbols uploaded yet. (When this happens, the fuzzer thinks all crashes are "new").

* Would prevent build ID collisions from resulting in frankenbuilds (where the symbols don't correspond to the binary).  Instead of file overwrites, you'd get an error from the mv command.

Comment 1

[Chris AtLee [:catlee]]

The idea here is to copy files to a temporary directory on the same filesystem as their final location, and then do an atomic rename of that temp directory to the real directory name.

We currently upload files into /tmp, and then copy them one by one into their final location.  /tmp is on a different file system than where the builds finally end up.

Comment 2

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

jesse and I just talked about this - seems safer to change fuzzing to only download builds that have symbols present at the time of download.

Comment 3

[Chris AtLee [:catlee]]

(In reply to comment #2)
> jesse and I just talked about this - seems safer to change fuzzing to only
> download builds that have symbols present at the time of download.

Huh?  I think the original idea is still valid.  There's no way to tell if the builds/symbols being downloaded are complete as it stands right now.

Comment 4

[Jesse Ruderman]

joduinn doesn't want to do mv-synchronization of the entire directory (when the build, tests, and symbols are all uploaded).  But you're saying we need to do it at least at the file level, so I don't download an incomplete file?

fwiw I don't know whether I've been downloading incomplete files. I only find out about errors in fuzz jobs when you tell me about them ;)

Comment 5

[Chris AtLee [:catlee]]

Not sure why not.  In any case, we should be doing copies from /tmp to a temp file on the same file system, and then an atomic rename to the final dest.

It's possible you've been getting incomplete files...I don't know how apache handles serving files that are currently being written to.

Comment 6

[Jesse Ruderman]

Hmm, also joduinn and I forgot about paragraph 2 of comment 0.

Comment 7

[Syed Albiz [:salbiz]]

Attachment: make post_upload.py use atomic renames

Initial patch to copy files into a hidden temporary directory in the destination and do an atomic rename once the file is copied onto the filesystem.

Comment 8

[Chris AtLee [:catlee]]

Comment on attachment 494016 [details] [diff] [review]
make post_upload.py use atomic renames

This looks great for individual files.

Can we do atomic directory copies too?  i.e. create tmpdir, copy files into it, rename tmpdir to realdir?

Comment 9

[Syed Albiz [:salbiz]]

Attachment: atomic copy directory

This will need to be thoroughly tested to make sure it doesn't break anything, which I have not done yet. It uses a single tmpdir to copy the entire directory of files, allowing for a state of atomic copying where we either have all the files completely copied, or none at all.

Comment 10

[Chris AtLee [:catlee]]

Comment on attachment 494835 [details] [diff] [review]
atomic copy directory

>-def CopyFileToDir(original_file, source_dir, dest_dir, preserve_dirs=False):
>+def make_path(path):
>+    """ Ensure that a directory path exists, create it anew if it does not """
>+    if not os.path.isdir(path):
>+        try:
>+            os.makedirs(path, 0755)
>+        except OSError, e:
>+            if e.errno == EEXIST:
>+                print "%s already exists, continuing anyways" % path
>+            else:
>+                raise
>+

make_path should raise an error if path exists but isn't a directory

>+def CopyFiles(files, dest_dir, func_copy):
>+    """ Atomic copy files from one directory to another using a tmpdir to
>+    store intermediate copy, then do an atomic rename. func_copy defines
>+    rules for filtering and operating on each file """
>+    make_path(os.path.dirname(dest_dir))
>+    tmpdir = tempfile.mkdtemp(prefix=".~", dir=os.path.dirname(dest_dir))
>+    for f in files:
>+        func_copy(f, tmpdir)
>+    atomic_rmtree(dest_dir)
>+    os.rename(tmpdir, dest_dir)

So I'm not sure we want to remove dest_dir here...What if files already exist in the destination directory?

Maybe it only makes sense to do atomic directory copies if the directory doesn't already exist.  If it does exist, then atomic file copies may be the best we can do.

Comment 11

[Syed Albiz [:salbiz]]

Attachment: fall back to per-file atomic copy if the directory path already exists

Done, was hoping we could avoid this, but I guess this is the best way to go. Expanded the tests as well.

Comment 12

[Syed Albiz [:salbiz]]

Comment on attachment 494016 [details] [diff] [review]
make post_upload.py use atomic renames

Based on IRC discussions, I think the consensus was that atomic directory copies don't get us very much and introduce a whack of additional problems, so falling back to using this patch.

Comment 13

[[e-mail] (:bhearsum)]

Comment on attachment 494016 [details] [diff] [review]
make post_upload.py use atomic renames

changeset:   1911:9a926a987cdf

Comment 14

[[e-mail] (:bhearsum)]

Syed, is there anything else to do in this bug?

Comment 15

[Syed Albiz [:salbiz]]

Should be all done now since we dropped the idea of atomic directory copies.