Closed
Bug 1113460
Opened 9 years ago
Closed 9 years ago
test jobs that use minidump stackwalk should get it from tooltool and not tools repo
Categories
(Release Engineering :: Applications: MozharnessCore, defect)
Tracking
(firefox34 fixed, firefox35 fixed, firefox36 fixed, firefox37 fixed, firefox38 fixed, firefox-esr31 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)
People
(Reporter: jlund, Assigned: jlund)
References
Details
Attachments
(2 files, 1 obsolete file)
2.20 KB,
patch
|
jgriffin
:
review+
jlund
:
checked-in+
|
Details | Diff | Splinter Review |
27.86 KB,
patch
|
jgriffin
:
review+
jlund
:
checked-in+
|
Details | Diff | Splinter Review |
in an effort to reduce how many times we rm and clone our tools repo, it seems there are many jobs that barely even need the tools repo at all. a common use for rm/cloning tools on every job is for the minidump stackwalk bits. turns out this stretches across ff, fennec, and b2g tests and there are a few different ways the tests go about doing that, depending on the platform and test suite. this bug will: 1) get such test jobs to avail of tooltool to grab the required minidumps instead of using tools repo 2) standardize how that is done
Assignee | ||
Comment 1•9 years ago
|
||
it looks like all of the linux64/osx64 gecko tests and the android 2.3 (linux64 based) tests use the wrong minidump stackwalk binary. platform.architecture() actually returns a tuple: (bits, linkage) so lines like this[1] will always fail will have to address that in this bug too [1] http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/testing/testbase.py?rev=4a83adea2532#561
Assignee | ||
Comment 2•9 years ago
|
||
IIUC, marionette windows jobs don't use MINIDUMP_STACKWALK at all. log example[1] I think that's because download_minidump_stackwalk[2] is not defined for the windows case[3]. jgriffin, before I add it, just a sanity check that it wasn't purposely being skipped for windows [1] https://tbpl.mozilla.org/php/getParsedLog.php?id=54830091&full=1&branch=mozilla-central [2] http://mxr.mozilla.org/build/source/mozharness/configs/marionette/prod_config.py?rev=3a32e88478ff#40 [3] http://mxr.mozilla.org/build/source/mozharness/configs/marionette/windows_config.py
Flags: needinfo?(jgriffin)
Assignee | ||
Comment 4•9 years ago
|
||
adds tooltool manifests. the incoming mozharness patch until this lands across all trees
Attachment #8540451 -
Flags: review?(jgriffin)
Assignee | ||
Comment 5•9 years ago
|
||
see previous attachment for associated m-c manifest patch. there are _many_ scripts and platforms this touches. I think this try push covers them all: https://tbpl.mozilla.org/?tree=Try&rev=e2dddcfda2b9 jgriffin: there are not many in releng who would know these parts of the infra so I'm asking you for the review. please bounce it to someone else if you don't think you're appropriate for it :)
Attachment #8540455 -
Flags: review?(jgriffin)
Comment 6•9 years ago
|
||
Comment on attachment 8540451 [details] [diff] [review] 141222_bug_1113460_use_tooltool_for_minidump-m-c.patch Review of attachment 8540451 [details] [diff] [review]: ----------------------------------------------------------------- rubber stamp!
Attachment #8540451 -
Flags: review?(jgriffin) → review+
Comment 7•9 years ago
|
||
Comment on attachment 8540455 [details] [diff] [review] 141222_bug_1113460_use_tooltool_for_minidump-mozharness.patch Review of attachment 8540455 [details] [diff] [review]: ----------------------------------------------------------------- Nice, this looks much more sane. ::: mozharness/mozilla/testing/testbase.py @@ +588,5 @@ > + output_dir=dirs['abs_work_dir'], > + cache=c.get('tooltool_cache') > + ) > + except KeyError: > + self.error('missing required: c["tooltool_servers"]') Is this the only thing that can throw KeyError? If not, better to make the error message more dynamic to avoid confusion later. Also, should we return if we hit this? @@ +595,5 @@ > + minidump_stackwalk_path) > + if os.path.exists(abs_minidump_path): > + self.minidump_stackwalk_path = abs_minidump_path > + else: > + self.warning("minidump stackwalk path was given but couldn't be determined. " maybe s/determined/found ?
Attachment #8540455 -
Flags: review?(jgriffin) → review+
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Jonathan Griffin (:jgriffin) from comment #7) > Comment on attachment 8540455 [details] [diff] [review] > 141222_bug_1113460_use_tooltool_for_minidump-mozharness.patch > > Review of attachment 8540455 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nice, this looks much more sane. > > ::: mozharness/mozilla/testing/testbase.py > @@ +588,5 @@ > > + output_dir=dirs['abs_work_dir'], > > + cache=c.get('tooltool_cache') > > + ) > > + except KeyError: > > + self.error('missing required: c["tooltool_servers"]') > > Is this the only thing that can throw KeyError? If not, better to make the > error message more dynamic to avoid confusion later. Also, should we return > if we hit this? that except key list used to be much bigger but I *think* it is just tooltool servers that is a hard requirement. I'll beef up the error message suggesting 'a' key is missing and that to check if tooltool_servers exists in self.config. fixed > > @@ +595,5 @@ > > + minidump_stackwalk_path) > > + if os.path.exists(abs_minidump_path): > > + self.minidump_stackwalk_path = abs_minidump_path > > + else: > > + self.warning("minidump stackwalk path was given but couldn't be determined. " > > maybe s/determined/found ? thanks, makes sense. fixed
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8540451 [details] [diff] [review] 141222_bug_1113460_use_tooltool_for_minidump-m-c.patch thanks! inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/927ca48b5f29
Attachment #8540451 -
Flags: checked-in+
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Assignee | ||
Comment 11•9 years ago
|
||
uplifted across release branches remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/0c9bed169769 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/7ee021371caf remote: https://hg.mozilla.org/releases/mozilla-release/rev/f4217563f156 remote: https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/05dd053f1d90 remote: https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/9470bec28e72 remote: https://hg.mozilla.org/releases/mozilla-esr31/rev/1be092ad0bae remote: https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/521882fd2cb9
status-b2g-v1.4:
--- → fixed
status-b2g-v2.0:
--- → fixed
status-b2g-v2.1:
--- → fixed
status-firefox34:
--- → fixed
status-firefox35:
--- → fixed
status-firefox36:
--- → fixed
status-firefox37:
--- → fixed
status-firefox-esr31:
--- → fixed
Assignee | ||
Comment 12•9 years ago
|
||
Comment on attachment 8540455 [details] [diff] [review] 141222_bug_1113460_use_tooltool_for_minidump-mozharness.patch on default: https://hg.mozilla.org/build/mozharness/rev/88c4800a33cb interdiff from review: http://people.mozilla.org/~jlund/141230_1113460_use_tooltool_for_minidump-mozharness-interdiff.patch manifests should be uplifted everywhere now. There will likely be more than one twig branch that is behind m-c and will burn jobs until it merges. here is one final try push that is against canonical mozharness but the default branch (since we haven't merged to prod yet). I'm doing a full run through as I'm paranoid about talos. it shouldn't be affected but since talos inherits TestMixin, it be good to test that too: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c55b58575854 finally, I've cc'd buildduty and sheriffs since this is quite an invasive patch
Attachment #8540455 -
Flags: checked-in+
Assignee | ||
Comment 13•9 years ago
|
||
mozharness has been merged to production. patches are live :)
Assignee | ||
Comment 14•9 years ago
|
||
backed out for reasons stated here https://bugzilla.mozilla.org/show_bug.cgi?id=1117313#c1 will follow up when I am back from PTO
Updated•9 years ago
|
Assignee: nobody → jlund
Assignee | ||
Comment 16•9 years ago
|
||
we are having hg.m.o issues again. while the cause is unknown, tackling this item should help alleviate load. This patch is virtually the same but it does add tooltool bits for the newly created lucid tests and it also 'chmod +x' the stackwalk binary so we don't hit bug 1117313 again. here is an interdiff of I think the only changes from last time (aside from some bitrot merge conflicts): http://people.mozilla.org/~jlund/150318_bug_1113460_use_tooltool_for_minidump-mh-interdiff.patch
Attachment #8540455 -
Attachment is obsolete: true
Attachment #8579505 -
Flags: review?(jgriffin)
Comment 17•9 years ago
|
||
Comment on attachment 8579505 [details] [diff] [review] 150318_bug_1113460_use_tooltool_for_minidump-mh.patch Review of attachment 8579505 [details] [diff] [review]: ----------------------------------------------------------------- r+ based on past review and the interdiff
Attachment #8579505 -
Flags: review?(jgriffin) → review+
Assignee | ||
Comment 18•9 years ago
|
||
Comment on attachment 8579505 [details] [diff] [review] 150318_bug_1113460_use_tooltool_for_minidump-mh.patch on default: remote: https://hg.mozilla.org/build/mozharness/rev/5615101b3a40 merged to prod: remote: https://hg.mozilla.org/build/mozharness/rev/75c435ef19ca mozharness.json rev bump on m-c: remote: https://hg.mozilla.org/mozilla-central/rev/2e2222a40262
Attachment #8579505 -
Flags: checked-in+
Comment 19•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/98428cf569e6
status-firefox38:
--- → fixed
Assignee | ||
Comment 21•9 years ago
|
||
sweet. thanks! I think we are all done here for now
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment 22•9 years ago
|
||
I'm guessing this is why we haven't been getting usable stacks for most Windows crashes in automation lately, i.e. https://treeherder.mozilla.org/logviewer.html#?job_id=705366&repo=mozilla-aurora
Flags: needinfo?(jlund)
Comment 23•9 years ago
|
||
Another example: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-beta-win32-debug/1427159850/mozilla-beta_win7-ix-debug_test-mochitest-other-bm109-tests1-windows-build14.txt.gz
Assignee | ||
Comment 24•9 years ago
|
||
commented in 1146855
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(jlund)
Comment 25•6 years ago
|
||
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in
before you can comment on or make changes to this bug.
Description
•