480240 - leak stats tinderboxes should not upload log or diff dumps when previous step failed

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.1/1235606163.1235607186.13090.gz&fulltext=1 shows the log of a leak stats tinderbox run in which the build crashed (due to a fatal assertion).

However, because of insufficient error handling in the build automation, it uploaded its log anyway.  This meant that a later build:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.1/1235613068.1235613392.27159.gz&fulltext=1
also failed because it couldn't handle the corrupt (incomplete) log from the previous run.

The leak stats tinderbox automation should not be uploading the log if the build crashed.


It seems like the test run in which the build crashed should have just turned the build orange, and it shouldn't have run any further steps.  However, it seems like the build continued after that point.  (It didn't report that any build step failed, as far as I can see in the log.)

However, a whole bunch of other build steps after that point did fail:
  'mv obj-firefox/_leaktest/sdleak.log ...' failed (because we crashed before
                                                    generating the log)
  compare current leak logs complete failed        (because the log was 
                                                    truncated... I'd also
                                                    note that this step is
                                                    misnamed and it should
                                                    be called "summarize"
                                                    rather than "compare")
and yet, even after these failures, we still went on to upload the logs, which messes up comparisons in later builds.

We also went on to produce a bunch of diffbloatdump output, which shouldn't be possible because there was no log, but I'm guessing it was because of the result of the above failed "mv" command from the previous run was still around.

Comment 1

[[e-mail] (:bhearsum)]

Attachment: halt leak test builds more aggressively

This patch will halt builds if any of the following fail: any one of the AliveTests, Compare* steps, diffbloatdump.pl, fix-{linux,macosx}-stack.pl.

It would be good to do this for Codesighs as well, but we run it before uploading builds...and I don't think we want to not publish a build because of a codesighs failure. (We might be able to change things to upload before running codesighs but I recall some weird dependency issues with packaging/codesighs/uploading when I was first setting that up - so I'm not inclined to change it hastily.)

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 364336 [details] [diff] [review]
halt leak test builds more aggressively

I think things should not fail (or warn) for any command involving malloc.log.old, sdleak.tree.old, or any other previous-cycle file, since they're not guaranteed to be there.

So I think you should undo the changes for the second CompareLeakLogs call (which is misnamed).  (I suspect the CompareBloatLogs is ok, but it's worth checking.)

r=dbaron with that

Comment 3

[[e-mail] (:bhearsum)]

Attachment: don't halt/warnOnFailure for CompareLeakLogs on the previous log

Just going to carry review on this one - it's the same thing with two lines removed, as per dbaron's comment. I left in the warn/halt for CompareBloatLogs - AFAICT it die()s on any error, and I don't think there's any point in continuing if it does..

Comment 4

[[e-mail] (:bhearsum)]

Comment on attachment 364351 [details] [diff] [review]
don't halt/warnOnFailure for CompareLeakLogs on the previous log

changeset:   204:c1237c4f6dd9

Comment 5

[[e-mail] (:bhearsum)]

master reconfig'ed for this.