Closed Bug 637444 Opened 13 years ago Closed 13 years ago

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

Categories

(Release Engineering :: General, defect, P2)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ehsan.akhgari, Assigned: coop)

References

()

Details

Attachments

(2 files, 1 obsolete file)

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.
Assignee: nobody → coop
Status: NEW → ASSIGNED
Priority: -- → P2
I've posted about this to dev.tree-management
Attachment #515729 - Flags: review?(nrthomas)
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.
Attachment #515729 - Flags: review?(nrthomas) → review+
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.
(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.
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 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 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.
Attachment #515764 - Flags: review?(nrthomas) → review+
Attachment #515765 - Flags: review?(nrthomas) → review+
(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.
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.
(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?
(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 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
Attachment #515764 - Flags: checked-in+
Comment on attachment 515765 [details] [diff] [review]
[buildbotcustom] Stop specifying DONTBUILD when updating the blocklist

http://hg.mozilla.org/build/buildbotcustom/rev/832e3da7239c
Attachment #515765 - Flags: checked-in+
Will need to be merged into production/production-0.8 prior to reconfig.
Flags: needs-reconfig?
This got pushed live on Wednesday night.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: needs-reconfig?
Resolution: --- → FIXED
Depends on: 785636
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.