1113460 - test jobs that use minidump stackwalk should get it from tooltool and not tools repo

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Jordan Lund (:jlund)]

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

Comment 1

[Jordan Lund (:jlund)]

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

Comment 2

[Jordan Lund (:jlund)]

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

Comment 3

[Jonathan Griffin (:jgriffin)]

It wasn't intentional, just an oversight.

Comment 4

[Jordan Lund (:jlund)]

Attachment: 141222_bug_1113460_use_tooltool_for_minidump-m-c.patch

adds tooltool manifests. the incoming mozharness patch until this lands across all trees

Comment 5

[Jordan Lund (:jlund)]

Attachment: 141222_bug_1113460_use_tooltool_for_minidump-mozharness.patch

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 :)

Comment 6

[Jonathan Griffin (:jgriffin)]

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!

Comment 7

[Jonathan Griffin (:jgriffin)]

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 ?

Comment 8

[Jordan Lund (:jlund)]

(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

Comment 9

[Jordan Lund (:jlund)]

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

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/927ca48b5f29

Comment 11

[Jordan Lund (:jlund)]

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

Comment 12

[Jordan Lund (:jlund)]

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

Comment 13

[Jordan Lund (:jlund)]

mozharness has been merged to production. patches are live :)

Comment 14

[Jordan Lund (:jlund)]

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

Comment 15

[Armen [:armenzg]]

https://hg.mozilla.org/build/ash-mozharness/rev/84461b54c692

Comment 16

[Jordan Lund (:jlund)]

Attachment: 150318_bug_1113460_use_tooltool_for_minidump-mh.patch

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

Comment 17

[Jonathan Griffin (:jgriffin)]

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

Comment 18

[Jordan Lund (:jlund)]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/98428cf569e6

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/938177ece421

Comment 21

[Jordan Lund (:jlund)]

sweet. thanks! I think we are all done here for now

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Jordan Lund (:jlund)]

commented in 1146855

Comment 25

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.