637444 - Stop landing AMO blocklist updates as DONTBUILD pushes without testing on the try server

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[(no longer active)]

The AMO blocklist update has broken the tree twice so far (unless I've missed some other incidents of this problem), and therefore, it has lost its merit to be landed on mozilla-central as DONTBUILD pushes.

We should stop the automated bot landing those changes, and those changes should be landed first on the try server, and after ensuring that all of the tests runs are green, they should be landed on mozilla-central (by a human).  Individual bugs should be filed for each update, and the bug number should be included in the commit message, with a proper author name (i.e., a human).

Or, to put it another way, these code changes should be landed under the same rules as all of the other code changes land on mozilla-central.

Comment 1

[(no longer active)]

I've posted about this to dev.tree-management

Comment 2

[Chris Cooper [:coop] (he/him)]

Attachment: Turn off automatic blocklist updating

Comment 3

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 515729 [details] [diff] [review]
Turn off automatic blocklist updating

For now this OK. But we should get developers to figure out why the tests are breaking and fix them, because the whole point of auto-landing blocklist data is to improve user experience on first run.

Comment 4

[Dave Townsend [:mossop]]

Maybe they shouldn't be landed DONTBUILD so it is clear what has caused the test failures but I don't think we should be wasting time in doing try runs and landing these changes manually. It simply isn't worth it.

It would be nice to add the bug numbers to the checkins but that isn't really possible with our current infrastructure and I don't think it is worth spending the time to make that happen either. Examining the changes to the XML and the blocklist page should reveal the bug that made them.

We've seen two failures on mozilla-central since we started updating the in-tree blocklist nearly 3 years ago (first manually and I never bothered to run tests before landing and later automatically, in fact it's been landing automatically on 1.9.0 since 3 years ago). In both cases they were faulty tests because no test should be relying on the default blocklist. It might be worth just writing some code to delete blocklist.xml from the extracted Firefox before running test to ensure this.

Comment 5

[Chris Cooper [:coop] (he/him)]

(In reply to comment #4)
> Maybe they shouldn't be landed DONTBUILD so it is clear what has caused the
> test failures but I don't think we should be wasting time in doing try runs and
> landing these changes manually. It simply isn't worth it.

I prefer this.

I'll roll a new patch to disable DONTBUILD, because there's no config flag for that at present. 

> In both cases they were faulty tests
> because no test should be relying on the default blocklist. It might be worth
> just writing some code to delete blocklist.xml from the extracted Firefox
> before running test to ensure this.

Agreed. This is akin to the issue where we shouldn't be relying on external resources in tests.

Comment 6

[Chris Cooper [:coop] (he/him)]

Attachment: [buildbot-configs] Disable blocklst updates on closed tree, but leave updates on.

Comment 7

[Chris Cooper [:coop] (he/him)]

Attachment: [buildbotcustom] Stop specifying DONTBUILD when updating the blocklist

Comment 8

[Nick Thomas [:nthomas] (UTC+12)]

Do we run any blocklist test in browser or are they xpcshell ? We might hit AMO for blocklist data at some point in browser.

Comment 9

[Chris Cooper [:coop] (he/him)]

Comment on attachment 515764 [details] [diff] [review]
[buildbot-configs] Disable blocklst updates on closed tree, but leave updates on.

As Ehsan wishes, these patches make blocklist updates follow normal tree rules. If we have prolonged closures like we have had for m-c, someone will need to run the scripts by hand to get the blocklist updates into the tree.

Comment 10

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 515764 [details] [diff] [review]
[buildbot-configs] Disable blocklst updates on closed tree, but leave updates on.

Glad to see we have this level of control.

Comment 11

[Dave Townsend [:mossop]]

(In reply to comment #8)
> Do we run any blocklist test in browser or are they xpcshell ? We might hit AMO
> for blocklist data at some point in browser.

I skimmed but I don't think there are any browser-chrome tests that test the blocklist service, it's possible we could add some in the future though. In browser-chrome and mochitest tests we redirect the blocklist url to the local test server so it always fails.

Comment 12

[Dave Townsend [:mossop]]

FWIW I think that the time when the tree is closed in the run-up to a release like we are now is the most important time for getting the blocklist changes landed so that it is always ready to ship with. Ideally we'd have more fine-grained control over tree state though I guess.

Comment 13

[(no longer active)]

(In reply to comment #4)
> It would be nice to add the bug numbers to the checkins but that isn't really
> possible with our current infrastructure and I don't think it is worth spending
> the time to make that happen either. Examining the changes to the XML and the
> blocklist page should reveal the bug that made them.

But in case that we need to back the changeset out from mozilla-central, there would be no way for us to notify the people in the know about the backout this way, right?

Comment 14

[Dave Townsend [:mossop]]

(In reply to comment #13)
> (In reply to comment #4)
> > It would be nice to add the bug numbers to the checkins but that isn't really
> > possible with our current infrastructure and I don't think it is worth spending
> > the time to make that happen either. Examining the changes to the XML and the
> > blocklist page should reveal the bug that made them.
> 
> But in case that we need to back the changeset out from mozilla-central, there
> would be no way for us to notify the people in the know about the backout this
> way, right?

You could comment in the bug that added the blocklist changes. This shouldn't be a case where there is no bug for the change to the blocklist it's just difficult to get that bug number into the commit message in an automated fashion, and not having automation traditionally meant that the in-tree blocklist just fell out of sync as people (by which I mean mostly me) never got around to landing changes

Comment 15

[Chris Cooper [:coop] (he/him)]

Comment on attachment 515764 [details] [diff] [review]
[buildbot-configs] Disable blocklst updates on closed tree, but leave updates on.

http://hg.mozilla.org/build/buildbot-configs/rev/7ca2dffa7026

Comment 16

[Chris Cooper [:coop] (he/him)]

Comment on attachment 515765 [details] [diff] [review]
[buildbotcustom] Stop specifying DONTBUILD when updating the blocklist

http://hg.mozilla.org/build/buildbotcustom/rev/832e3da7239c

Comment 17

[Chris Cooper [:coop] (he/him)]

Will need to be merged into production/production-0.8 prior to reconfig.

Comment 18

[Chris Cooper [:coop] (he/him)]

This got pushed live on Wednesday night.