ASan is the only Linux platform running the separate jittest suite on try

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 months ago

People

(Reporter: philor, Assigned: kmoir)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
I have a vague memory of a bug where we thought that we were turning off having jittest run on both the ubuntu and the fedora slaves, which I suspect is the reason why https://tbpl.mozilla.org/?tree=Try&rev=bc445a9dbee6&showall=1&jobname=jit got no Linux jittests other than on ASan.
(Reporter)

Updated

5 years ago
Blocks: 969991
So we want jittests to run on all linux platforms on try, not just ASan
(Reporter)

Updated

5 years ago
Blocks: 973900

Comment 2

5 years ago
It looks like the problem is at [1] which I guess was added to prevent scheduling on both fedora and ubuntu slaves on Cedar. This condition should probably be moved to where we add the tests for Cedar so that it doesn't affect try.

[1] http://hg.mozilla.org/build/buildbot-configs/file/8cd7c81484d5/mozilla-tests/config.py#l1774

Comment 3

5 years ago
Created attachment 8383685 [details] [diff] [review]
Make logic to skip Fedora slaves only apply to Cedar
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Attachment #8383685 - Flags: review?(bhearsum)
Comment on attachment 8383685 [details] [diff] [review]
Make logic to skip Fedora slaves only apply to Cedar

Review of attachment 8383685 [details] [diff] [review]:
-----------------------------------------------------------------

our configs suck
Attachment #8383685 - Flags: review?(bhearsum) → review+

Comment 6

5 years ago
buildbot-configs patch is merged and live in production! :)

Comment 7

5 years ago
My patch did not work, these are still running on ASAN only.

Comment 8

5 years ago
I'm happy to redo this patch, but I could use some help identifying the root cause of why these are only being scheduled on ASAN for try. Scheduling for cedar is fine.
Flags: needinfo?(bhearsum)
(In reply to Dan Minor [:dminor] from comment #8)
> I'm happy to redo this patch, but I could use some help identifying the root
> cause of why these are only being scheduled on ASAN for try. Scheduling for
> cedar is fine.

Looks like the first check of the block is wrong:
https://github.com/mozilla/build-buildbot-configs/blob/master/mozilla-tests/config.py#L1826

>>> 1 in ([2,3] or [4,5])
False
>>> 1 in ([2,3] or [4,5,1])
False

You should probably check those separately, eg:
if platform not in cedar_platforms or platform not in try_platforms:
  continue
Flags: needinfo?(bhearsum)

Comment 10

5 years ago
D'oh, thanks for catching that. And here I thought it was something mysterious with buildbot.

Comment 11

5 years ago
Created attachment 8392842 [details] [diff] [review]
Fix initial platform check

Switch not in (A or B) to not in A and not in B. If A is not an empty list, (A or B) returns A, so the original code never checked inclusion in B.
Attachment #8392842 - Flags: review?(bhearsum)
Comment on attachment 8392842 [details] [diff] [review]
Fix initial platform check

Review of attachment 8392842 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozilla-tests/config.py
@@ +1822,5 @@
>                          pass
>  
>  # Enable jittests on cedar https://bugzilla.mozilla.org/show_bug.cgi?id=912997
>  for platform in PLATFORMS.keys():
> +    if platform not in BRANCHES['cedar']['platforms'] and platform not in BRANCHES['try']['platforms']:

I think you want "or" not "and". Otherwise you can't control them for cedar and try independently. r=me if you fix that on landing.
Attachment #8392842 - Flags: review?(bhearsum) → review-

Comment 13

5 years ago
Created attachment 8395635 [details] [diff] [review]
Fix platform check

I split the code to handle cedar and try separately. I think this will fix the scheduling problem and be easier to read.
Attachment #8392842 - Attachment is obsolete: true
Attachment #8395635 - Flags: review?(bhearsum)
Comment on attachment 8395635 [details] [diff] [review]
Fix platform check

Review of attachment 8395635 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me now
Attachment #8395635 - Flags: review?(bhearsum) → review+
in production

Comment 17

5 years ago
Looks like my new patch also does not schedule on anything other than ASAN for try:
https://tbpl.mozilla.org/?tree=Try&rev=e1bb7915c000&showall=1

kmoir, since you're looking at bug 973900, do you mind having a look at this bug as well? I don't seem to be getting anywhere with this.
Flags: needinfo?(kmoir)
(Assignee)

Updated

5 years ago
Assignee: dminor → kmoir
Flags: needinfo?(kmoir)
(Assignee)

Comment 18

5 years ago
I probably can get this sorted out in a week or so. I've setup a new test master environment, just looking at the configs to see what's happening.
(Assignee)

Comment 19

5 years ago
Created attachment 8398040 [details] [diff] [review]
bug969988.patch

So the problem is that the jittests added starting here
http://mxr.mozilla.org/build/source/buildbot-configs/mozilla-tests/config.py#1882

but then are disabled here
http://mxr.mozilla.org/build/source/buildbot-configs/mozilla-tests/config.py#2005

because certain tests are deleted unless they reside on cedar

looking more closely, it looks like a 3/17 merge day patch wasn't removed starting here
http://mxr.mozilla.org/build/source/buildbot-configs/mozilla-tests/config.py#1953

I've tested removing the merge day stuff on my test master and the proper jittests appear.  Asking rail for comment since he was on the last merge duty.
Attachment #8398040 - Flags: review?(rail)
(Assignee)

Comment 20

5 years ago
Created attachment 8398104 [details] [diff] [review]
bug969988.patch

Talked to rail, the merge day stuff is still needed for bug 875164.  So modified the patch to just not delete the tests on try
Attachment #8398040 - Attachment is obsolete: true
Attachment #8398040 - Flags: review?(rail)
Attachment #8398104 - Flags: review?(rail)
Comment on attachment 8398104 [details] [diff] [review]
bug969988.patch

A nit:
Can you use the following instead:
if branch in ("cedar", "try"):
Attachment #8398104 - Flags: review?(rail) → review+
(Assignee)

Comment 22

5 years ago
Created attachment 8398123 [details] [diff] [review]
updated patch
in production.
(Assignee)

Comment 24

5 years ago
Comment on attachment 8398123 [details] [diff] [review]
updated patch

checked in before this morning's reconfig
Attachment #8398123 - Flags: checked-in+
(Assignee)

Comment 25

5 years ago
Verified in tbpl and buildapi that tests are running/being scheduled
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Component: General Automation → General
Product: Release Engineering → Release Engineering
You need to log in before you can comment on or make changes to this bug.