Closed Bug 1550611 Opened 1 year ago Closed 8 months ago

Reduce number of builds triggered on push

Categories

(Thunderbird :: Build Config, task)

task
Not set
normal

Tracking

(thunderbird_esr68 fixed, thunderbird69 fixed, thunderbird70 fixed)

RESOLVED FIXED
Thunderbird 70.0
Tracking Status
thunderbird_esr68 --- fixed
thunderbird69 --- fixed
thunderbird70 --- fixed

People

(Reporter: rjl, Assigned: rjl)

References

Details

Attachments

(7 files, 3 obsolete files)

Request to not run shippable builds on push for Thunderbird.

The idea is to have only opt and debug run on push, then shippable runs nightly, effectively restoring the old behavior.

WIP!

TODO:

  • Figure out what's going on with MacOS
  • Should comm-beta and comm-esr be treated differently?

(In reply to Rob Lemley [:rjl] from comment #0)

The idea is to have only opt and debug run on push, then shippable runs nightly, effectively restoring the old behavior.

I noticed that Nightly runs don't actually trigger a build even if the underlying M-C version has changes, see for example here:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=6bd11ad2949dc7454e6b98f9613064621ee319f3

The Nightly run only ran L10N and repack jobs, but based on the build that happened hours before.

For sheriffing that means that in order to get an up-to-date build for Nightly, we must push after the M-C merge or push before the M-C merge with DONTBUILD. In the former case the push will trigger a full build and the the Nightly run will just add the L10N bits, in the latter case, at least the shippable builds will run, like here:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=fc51c0e147cc1544f5fa61bb01b2cfd1ba87a89d

Unless I'm mistaken, the old behaviour was actually less work.

I'll see what I can do. Thanks for the input.

include-push-tasks does not make sense for C-C workflow and should not have
been included.
Removing this will force a full build when the daily build triggers.

help='Whether tasks from the on-push graph should be re-used '
                          'in this graph. This allows cron graphs to avoid rebuilding '
                          'jobs that were built on-push.
Attachment #9064609 - Flags: review?(jorgk)
Comment on attachment 9064609 [details] [diff] [review]
remove include-push-tasks from .cron.yml [landed, comment #7]

OK, so this will force the Daily run to build new shippables since the M-C version could have changed since the last build.

I'm a bit surprised, since these lines were just added here
https://hg.mozilla.org/comm-central/rev/b58e5d1520e0#l1.12
to fix some sort of problem.
Attachment #9064609 - Flags: review?(jorgk) → review+
Keywords: leave-open

Yes I know. I should have backed it out as it didn't fix the problem as I expected.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/25aaacb02b1d
remove include-push-tasks from .cron.yml. r=jorgk DONTBUILD

Keywords: checkin-needed
macosx64-shippable will get run on push.
Attachment #9065288 - Flags: review?(geoff)

Attachment 9065287 [details] [diff] and 9065288 need landing.
My instinct is to just remove the macosx64/opt build altogether and only leave macosx64-shippable.
This disables it instead so if there are jobs further down the pipeline that depend on it, it will build and we don't see bustage.

Attachment #9065287 - Flags: review?(geoff) → review+
Attachment #9065288 - Flags: review?(geoff) → review+
Keywords: leave-open

What's the reason to treat Linux/Windows and Mac differently? Why not just also disable Mac shippable by default? Oh, I see, that because on Linux/Windows the tests run on both "opt" and "shippable opt" and for Mac they only run on "shippable opt". So if we disabled "shippable opt", we'd lose the tests.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b206f4d87208
Do not run Linux/Windows shippable builds on push. r=darktrojan
https://hg.mozilla.org/comm-central/rev/6512afbb0b56
Disable macosx64/opt build on push. r=darktrojan DONTBUILD

Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED

(In reply to Jorg K (GMT+2) from comment #11)

What's the reason to treat Linux/Windows and Mac differently? Why not just also disable Mac shippable by default? Oh, I see, that because on Linux/Windows the tests run on both "opt" and "shippable opt" and for Mac they only run on "shippable opt". So if we disabled "shippable opt", we'd lose the tests.

Right, and I think that move has something to do with Notarized Builds but I'm not sure.

Backout:
https://hg.mozilla.org/comm-central/rev/7321cfcf8b65cff0723ab5f01b2e9d993ab1557a
Backed out 2 changesets (bug 1550611) for breaking test runs on Daily/Nightly-only runs. a=backout

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Thunderbird 68.0 → ---

Oh, I shouldn't have backed out the Mac part, since Mac test did run here:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=6512afbb0b5671acb2c4550b23f492d6cd2f5b72

Tests are back on Linux (and likely Windows when the time comes) here:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=4516cf2270f464e5e8bc19fbf2b0ca6fff10ca58

This has sadly stalled for a month :-(

Some observations: SeaMonkey pushes for suite/ only still trigger builds:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=b26f815835648f3e0e80f9568d2adb33f5c97251

If the last push was a SM push for suite/ only, then Daily will not run tests, that's bad:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=12611e8ca4489c44a3bb6e8c3c685117e6afbd87

So we still want to reduce the number of build and build the right things at all times.

Flags: needinfo?(rob)

(In reply to Jorg K (GMT+2) from comment #16)

Some observations: SeaMonkey pushes for suite/ only still trigger builds:

I suspect the shippable builds require the repack tasks to run, and they're pulling in the shippable-build tasks.

If the last push was a SM push for suite/ only, then Daily will not run tests, that's bad:

Yes. This might be difficult to achieve given the requirement that we don't trigger builds on suite-only checkins, but I'll see what I can come up with.

So we still want to reduce the number of build and build the right things at all times.

Unfortunately, this shippable build thing is great for Firefox, but not so great for Thunderbird's workflow. The "nightly_desktop" target task that the cron task runs to build the Dailies assumes that tests already ran. That's just simply not the case for us.

I'll bump this up my priority queue.

[edit: fix formatting]

Flags: needinfo?(rob)

Quick update: I have some changes that I thought would work out, but there's some problems with tasks getting removed that we want to run, and there's some of the optimization algorithm jumps have me concerned that we will trigger Bug 1414043, and I really want to avoid that. Still working on this!

Bug 1422060 applies here as well.

Attached patch verbump_dontbuild.patch (obsolete) — Splinter Review
This option is passed along to the treescript worker that actually
does the commit. The option is in use by Firefox, so I expect no issues
with it.
Please land upon r+.
Attachment #9079151 - Flags: review?(geoff)
This option is passed along to the treescript worker that actually
does the commit. The option is in use by Firefox, so I expect no issues
with it.
Please land upon r+.

Version 2 removes DONTBUILD from the commit message so it doesn't
cause problems for the sheriffs.
Attachment #9079190 - Flags: review?(geoff)
Attachment #9079151 - Attachment is obsolete: true
Attachment #9079151 - Flags: review?(geoff)
Attachment #9079190 - Flags: review?(geoff) → review+

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/852ab23d79ee
dont build on push when tagging release revisions. r=darktrojan

Status: REOPENED → RESOLVED
Closed: 1 year ago1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 70.0

Hmm, here's the grammar police: What is "dont"?

Comment on attachment 9079190 [details] [diff] [review]
verbump_dontbuild.patch [landed, comment #23]

You could have requested uplift straight away.
Attachment #9079190 - Attachment description: verbump_dontbuild.patch → verbump_dontbuild.patch landed, comment #23]
Attachment #9079190 - Flags: approval-comm-esr68+
Attachment #9079190 - Flags: approval-comm-beta+
Attachment #9079190 - Attachment description: verbump_dontbuild.patch landed, comment #23] → verbump_dontbuild.patch [landed, comment #23]
Attachment #9064609 - Attachment description: remove include-push-tasks from .cron.yml → remove include-push-tasks from .cron.yml [landed, comment #7]
Attachment #9065287 - Attachment description: Do not run shippable builds on push → Do not run shippable builds on push [landed, comment #12; backed out, comment #14]
Attachment #9065288 - Attachment description: Disable macosx64/opt build on push → Disable macosx64/opt build on push [landed, comment #12; backed out, comment #14]

This is not really fixed since the main patches here got backed out. Overall, we're still creating too many unneeded tasks.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

TB 69 beta 2, just missed beta 1, but will be effective when tagging:
https://hg.mozilla.org/releases/comm-beta/rev/15913924cdc856474540c7ca7723e6aa7ebfc342

See Also: → 1586508
Attached patch fix_extra_builds_halloween.patch (obsolete) — Splinter Review
This patch requires the patch waiting for review in bug 1592162.

And I think it will work. We won't really know until the conditions triggering
the undesired builds occur.

If my try job works out, this should address the issues raised in this bug
regarding too many build jobs.

Specifically;
- no more shippable builds on Seamonkey-only pushes
- always run the daily build even when the last push is seamonkey-only
- run the tests on seamonkey-only dailies.

Try is funny, so I'm checking that now.

I compared task lists with mach taskgraph to predict outcomes of the various
scenarios.

I grabbed parameters.yml files from three recent pushes to comm-central.
1) suite-only push, 2) regular thunderbird push, 3) thunderbird push for which
later got used for a Daily, 4) the parameters file for the Daily itself. 
With that I was able to create what the parameters.yml file would be if the
Suite-only push had been the one used for a Daily.

Then it was a matter of running "mach taskgraph optimized" with the various
paramters files and comparing output.

Source tests and toolchain builds may still run for Seamonkey-only pushes. As
toolchains are cached and reused, I don't have a problem with that. Source tests
are pretty small jobs. I can address in a follow-up if desired.

Linux32/opt tests don't exist. So no Linux32/opt tests on push, just on the
shippple variant on push. I can leave as is or bring them back.
Attachment #9105466 - Flags: review?(geoff)
Depends on: 1592162
Comment on attachment 9105466 [details] [diff] [review]
fix_extra_builds_halloween.patch

This makes sense, although your comment on always_shippable isn't right. I guess you wrote it then changed what the code did.
Attachment #9105466 - Flags: review?(geoff) → review+

I guess you're referring to: "Sets "always_target" to true on shippable platforms when running a nightly build."

So let's fix this before landing this.

(In reply to Jorg K (GMT+2) from comment #31)

I guess you're referring to: "Sets "always_target" to true on shippable platforms when running a nightly build."

So let's fix this before landing this.

I will address the comment issue. The other thing is bug 1592162, I need to add to that so Geoff can review before this one works.

A corner case just occurred to me... I need to work on this some more.

When a release is promoted, all of those promotion tasks are going to depend on a "shippable" build. But it may or may not be a nightly build that gets promoted and with this function written like this I believe we are going to see a problem with tests. That should be limited to Seamonkey only pushes. Not likely to happen, but if it's not too hard I would like to address it.

This is just a name change and comment fix. The corner case in mentioned in
comment 13 remains an issue for now. The workaround is to make sure that
any push thats to be promoted to a release is not a Seamonkey-only
code push.
Attachment #9105466 - Attachment is obsolete: true

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d1762836541f
Add nightly attribute to shippable builds. r=darktrojan

Status: REOPENED → RESOLVED
Closed: 1 year ago8 months ago
Resolution: --- → FIXED

Last part landed on Thunderbird 72. I see 10 builds: Linux32, Linux64, Mac, Win32 and Win64 times two for debug/opt, yay \o/.

Let's file a new bug for any other issues.

Spoke too early. Mac (and Linux32) tests aren't run.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch mac-fix.patch (obsolete) — Splinter Review

This is the original Mac bit and the current Mac bit removed:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=84d7e21a6662f8f182a63986d639207ad891fc78

And we can already see it doesn't work :-(

Should work as advertised.
Attachment #9107069 - Flags: review?(geoff)
Attachment #9107069 - Flags: review?(geoff) → review+

For the record: On IRC Geoff said to land the new patch. I would have only used the Linux32 part and reversed the Mac line here:
https://hg.mozilla.org/comm-central/rev/d1762836541f#l2.12

Attachment #9107068 - Attachment is obsolete: true

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/067c9af97ec9
Run macosx64/opt and linux/opt tests. r=darktrojan DONTBUILD

Status: REOPENED → RESOLVED
Closed: 8 months ago8 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.