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

RESOLVED FIXED

Status

Release Engineering
General
P3
normal
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: dbaron, Assigned: bhearsum)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

9 years ago
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.
(Assignee)

Updated

9 years ago
Assignee: nobody → bhearsum
Priority: -- → P3
(Assignee)

Comment 1

9 years ago
Created attachment 364336 [details] [diff] [review]
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.)
Attachment #364336 - Flags: review?(dbaron)
Attachment #364336 - Flags: review?(catlee)
(Reporter)

Updated

9 years ago
Attachment #364336 - Flags: review?(dbaron) → review+
(Reporter)

Comment 2

9 years ago
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

Updated

9 years ago
Attachment #364336 - Flags: review?(catlee) → review+
(Assignee)

Comment 3

9 years ago
Created attachment 364351 [details] [diff] [review]
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..
Attachment #364336 - Attachment is obsolete: true
(Assignee)

Comment 4

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

changeset:   204:c1237c4f6dd9
Attachment #364351 - Flags: checked‑in+ checked‑in+
(Assignee)

Comment 5

9 years ago
master reconfig'ed for this.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.