Closed Bug 1339604 Opened 3 years ago Closed 3 years ago

stylo builds + tests should only run on m-c and stylo branch to reduce budget impact

Categories

(Infrastructure & Operations :: CIDuty, task)

task
Not set

Tracking

(firefox54 fixed)

RESOLVED FIXED
Tracking Status
firefox54 --- fixed

People

(Reporter: kmoir, Assigned: kmoir)

References

Details

Attachments

(6 files, 4 obsolete files)

No description provided.
Attached patch savemoney.patch (obsolete) — Splinter Review
Assignee: nobody → kmoir
My understanding is that we don't have budget for additional AWS builds for quantum thus we must limit the branches where we run builds and tests.  This patch limits stylo builds to running on m-c and the stylo repo. Please confirm cpeterson :-)
Flags: needinfo?(cpeterson)
Blocks: 1339396
Sorry, this isn't going to be acceptable. See bug 1339396 comment 9.

Also, note that the stylo repo is going away. With the new repo sync work, we are working directly on autoland.
Flags: needinfo?(cpeterson)
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #3)
> Sorry, this isn't going to be acceptable. See bug 1339396 comment 9.
> 
> Also, note that the stylo repo is going away. With the new repo sync work,
> we are working directly on autoland.

(See [1] and [2] for a description of our new workflow)

[1] https://groups.google.com/forum/#!topic/mozilla.dev.platform/acolCIYh0RI
[2] https://groups.google.com/forum/#!topic/mozilla.dev.servo/ZbWd0-eDwRA
Also, just to be clear: The Talos jobs are much less important right now, and are fine to run on central only.
Bobby, to reduce Stylo test load, could we turn off Stylo opt or debug tests on inbound? Since Servo and Stylo commits are all pushed through the autoland repo, tests on autoland seem like they are most likely to identify Stylo regressions.
Flags: needinfo?(bobbyholley)
Attached patch savemoney2.patch (obsolete) — Splinter Review
patch to enable builds on autoland as well
Attachment #8837325 - Attachment is obsolete: true
(In reply to Chris Peterson [:cpeterson] from comment #6)
> Bobby, to reduce Stylo test load, could we turn off Stylo opt or debug tests
> on inbound? Since Servo and Stylo commits are all pushed through the
> autoland repo, tests on autoland seem like they are most likely to identify
> Stylo regressions.

I agree that we could probably reduce the test matrix a bit on inbound.

The easiest target is probably the reftests, where we have 16 jobs (R1-R16) running 4 ways (debug/opt and e10s/non-e10s). The overhead of that dominates everything else, so if we're going to slice it somehow that would be the place to start. I would be ok with turning off 3 of those 4 configurations on inbound, and sticking with only Reftest-debug-e10s, which should reduce the job count on inbound by almost a factor of four.

We should leave the builds, crashtests and mochitest-style running in all the configurations, since they give us more best bang for buck.
Flags: needinfo?(bobbyholley)
Attached patch bug1339604.patchSplinter Review
not sure if this patch is what we want.

I couldn't find a way to specify that we only want to run reftest-stylo e10s debug on one branch, but the rest of the tests on others
Attachment #8837640 - Attachment is obsolete: true
Hmm, this didn't seem to work.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c48f645ac917abc3017bcba00a522aefc9627499&selectedJob=77720143

Not sure how to specify that debug stylo-reftests for e10s should only run on m-i but all stylo reftests should run on other branches. Will ask in channel.
Dustin, looking at the current transforms I see that we can specify e10s on a per platform basis but I'm not sure how to toggle that on a per platform basis for multiple branches

I'm looking to run 
debug stylo-reftests for e10s should only run on m-i but all stylo reftests should run on other branches

hmm, maybe I need to land different patches on different branches?
Flags: needinfo?(dustin)
Yikes, I feel like we're getting endlessly creative with stylo solutions, and the clarity of our configuration is suffering accordingly.  I think there's an argument to make comparing cognitive load of implementing and understanding these solutions to the cost of just executing more stylo-related work.  Anyway, just throwing that out there.

It should be possible to make `e10s:` keyed by both test-platform and project.  Then you could write something like:

    e10s:
        by-test-platform:
            linux64-stylo/opt: true
            linux64-stylo/debug:
                # only run e10s on inbound
                by-platform:
                    mozilla-inbound: true
                    default: false
            default: both

(I'm not sure I got the logic right there, but hopefully the idea of nested by-* is clear from the example)

Note that `project` is not an attribute of a test description, so you will need to add `config.params['project']` to `handle_keyed_by` in taskcluster/taskgraph/transforms/tests.py`:

    for test in tests:
        for field in fields:
            resolve_keyed_by(test, field, item_name=test['test-name'], project=config.params['project'])
        yield test 

and also modify the schema for `e10s` at the top of that file to allow keying by project, too:

    Required('e10s', default='both'): optionally_keyed_by(
        'test-platform', 'project',
        Any(bool, 'both')),
Flags: needinfo?(dustin)
Hmm, I tried this today but it didn't work so I'm debugging.  Dustin why do you refer to by-platform for the changes to taskcluster/ci/test/tests.yml

but the taskcluster/taskgraph/transforms/tests.py refers to project?
Flags: needinfo?(dustin)
I typed the wrong thing, sorry. In my defense, they both start with "p" :)

                # only run e10s on inbound
                by-project:
                    mozilla-inbound: true
                    default: false
Flags: needinfo?(dustin)
Here is my most recent patch which is not working

The problem is that the schema expects a bool value for 

   "In test {!r}:".format(test['test-name']))
  File "/Users/kmoir/hg/mozilla-inbound/taskcluster/taskgraph/util/schema.py", line 25, in validate_schema
    raise Exception('\n'.join(msg) + '\n' + pprint.pformat(obj))
Exception: In test 'reftest-stylo':
expected bool for dictionary value @ data['e10s']['by-test-platform']['linux64-stylo/debug']
expected bool for dictionary value @ data['e10s']['by-test-platform']['linux64-stylo/opt']

So the I think the schema here is wrong

Required('e10s', default='both'): optionally_keyed_by(
        'test-platform', 'project',
        Any(bool, 'both')),

but I don't understand how it should be changed.  It seems like project should be nested under test-platform, not at the same level in the schema
Flags: needinfo?(dustin)
Hm, I didn't implement optionally_keyed_by the way I thought I did.  Let me see if I can fix that..
Flags: needinfo?(dustin)
Attached patch savemoney3.patch (obsolete) — Splinter Review
Aside from getting the test transforms sorted, we should disable the builds on anything other than these branches to save money in the interim.  I noticed the stylo builds are running on some project branches that have no need for them.
Attachment #8838257 - Flags: review?(coop)
Comment on attachment 8838279 [details]
Bug 1339604: Allow nesting of keyed-by values;

https://reviewboard.mozilla.org/r/113206/#review114768

Wow, that is quite a patch. Thank you so much!

So I tried this with the patches I have in the bug.  The error ocurred

 File "/Users/kmoir/hg/mozilla-inbound/taskcluster/taskgraph/util/schema.py", line 138, in resolve_keyed_by
    keyed_by, key, field, item_name))
Exception: No project matching 'try' nor 'default' found while determining item e10s in reftest-stylo

This is because I have this in the tests.yml for stylo
e10s:
    by-test-platform:
        linux64-stylo/opt:
            by-project:
                mozilla-inbound: false
 
 If I switch mozilla-inbound to try it works.  So I think there needs to some additional values added to the defaults.
Attachment #8838279 - Flags: review?(kmoir) → review-
(In reply to Kim Moir [:kmoir] from comment #19)
> e10s:
>     by-test-platform:
>         linux64-stylo/opt:
>             by-project:
>                 mozilla-inbound: false

I think that last bit is the issue: if the platform is linux64-stylo/opt, and project is anything but mozilla-inbound, then it doesn't say what e10s should be set to.  If you add a default value, it should work:

                  default: both   # or 'true' or whatever you would like to take effect for other projects
Comment on attachment 8838279 [details]
Bug 1339604: Allow nesting of keyed-by values;

https://reviewboard.mozilla.org/r/113206/#review114918

Ah, that fixes it then.  Thanks Dustin.
Attachment #8838279 - Flags: review- → review+
Attached patch bug1339604lr2.patch (obsolete) — Splinter Review
Attachment #8838257 - Attachment is obsolete: true
Attachment #8838257 - Flags: review?(coop)
Attachment #8838577 - Flags: review?(dustin)
Comment on attachment 8838577 [details] [diff] [review]
bug1339604lr2.patch

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

::: taskcluster/ci/test/tests.yml
@@ +941,5 @@
> +            linux64-stylo/opt:
> +                by-project:
> +                    mozilla-inbound: false
> +                    default: both
> +            linux64-stylo/debug: 

trailing space
Attachment #8838577 - Flags: review?(dustin) → review+
Comment on attachment 8838279 [details]
Bug 1339604: Allow nesting of keyed-by values;

https://reviewboard.mozilla.org/r/113206/#review114986
fixed whitespace
Attachment #8838577 - Attachment is obsolete: true
Thanks for working on this!

To make sure I understand correctly: This will also enable the reftest-stylo job on autoland, right? We need to turn that on as soon as possible so that we can prevent regressions (more keep popping up), so please land your patch directly to the autoland repo if you're able to. :-)
Pushed by kmoir@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fbcbdd68493
stylo builds + tests should only run on limited branches to reduce budget impact r=dustin
Could I have this patch 

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fbcbdd68493

cherry picked and landed on autoland?
Flags: needinfo?(sheriffs)
Flags: needinfo?(sheriffs)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/95fa240ac387
stylo builds + tests should only run on limited branches to reduce budget impact r=dustin
patch to fix the issue where reftests don't run on m-c etc. See 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1041546e9d721ec47f4ccd1c530f0ce892888b8d&selectedJob=78414573
Attachment #8838710 - Flags: review?(bobbyholley)
Comment on attachment 8838710 [details] [diff] [review]
bug1339604moretests.patch

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

lgtm, though note that the reftest jobs are already running on try, so the try push doesn't necessarily prove anything. Might want somebody who understand the automation stuff to give it a look-over.
Attachment #8838710 - Flags: review?(bobbyholley) → review+
Pushed by kmoir@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fe83738221a
stylo builds + tests should only run on m-c and stylo branch to reduce budget impact r=bholley
noticed that stylo opt reftests are still running on inbound, and only need Reftest-debug-e10s there according to previous comments
Pushed by kmoir@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f8aecb0f44f
stylo builds + tests should only run on m-c and stylo branch to reduce budget impact r=kmoir DONTBUILD
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/02a96520e63c
stylo builds + tests should only run on limited branches to reduce budget impact r=dustin a=merge
https://hg.mozilla.org/mozilla-central/rev/0930fdc4cf8e
stylo builds + tests should only run on m-c and stylo branch to reduce budget impact r=bholley a=merge
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/d11c29c1db3a
stylo builds + tests should only run on m-c and stylo branch to reduce budget impact r=kmoir a=merge
Verified on treeherder that this looks good now.  only e10s debug stylo reftests on m-i, both e10s and non-e10s stylo reftests on stylo, autoland and m-c repos.
(In reply to Kim Moir [:kmoir] from comment #38)
> Verified on treeherder that this looks good now.  only e10s debug stylo
> reftests on m-i, both e10s and non-e10s stylo reftests on stylo, autoland
> and m-c repos.

Great! Thanks for helping us to get this right. :-)
Component: Platform Support → Buildduty
Product: Release Engineering → Infrastructure & Operations
You need to log in before you can comment on or make changes to this bug.