mozilla-inbound backout policy subject to change (become similar to autoland)

[Sebastian Hengst]

Hi,

If you don't push code to mozilla-inbound, you can stop reading now.

TL;DR: We would like to change the mozilla-inbound backout policy to be
like autoland’s.

In order to decrease tree closure times for inbound, allow for more
frequent merges from inbound to m-c, and ensure consistent policies
across our integration branches, we propose that mozilla-inbound adopt
the same policy as autoland for how issues with a push (failures,
exception loops, ...) are handled. Rather than asking the developer to
investigate and fix the issue with a follow-up push if possible,
sheriffs will instead backout the patch and notify developers in the bug
and IRC if possible.

If your patch touches so much code that it is always at risk of
bitrotting or is urgent (e.g. Release Management wants it ASAP), talk to
the sheriffs in the #sheriffs channel on IRC
before you push and get their approval to push (this is for checking
that mozilla-inbound will have a recent "good" state for merges).

If your patches got backed out, you can reapply them locally with |hg
graft -fr <revset>| and fix them with |hg histedit| or |hg commit
--amend| before pushing.

Sebastian
Sheriff Trainer & Code Quality Engineer

[Gabriele Svelto]

On 19/06/2018 15:04, Sebastian Hengst wrote:
> TL;DR: We would like to change the mozilla-inbound backout policy to be
> like autoland’s.

Sounds like a very good idea. When I screw up with a patch what worries
me most is wasting other peoples' time with a broken tree. Having
problematic patches backed out quickly means saving time for everybody.

Gabriele

[Boris Zbarsky]

On 6/19/18 9:04 AM, Sebastian Hengst wrote:
> TL;DR: We would like to change the mozilla-inbound backout policy to be
> like autoland’s.

This seems like a pretty reasonable change in general.

Is there a well-documented try syntax string which corresponds to "these
are the things that need to be green to avoid being backed out"?
Presumably that string is not "-p all -u all" because it should exclude
tier2 and lower things, but it's not entirely clear to me what the
autoland backout criteria are. I would assume we want developers to do
a try run with that syntax before pushing to inbound as needed.

-Boris

[Mike Hommey]

For some reason, -p all -u all runs things that are not marked tier-2
but also don't run on autoland/inbound/central. I don't know why, but
that has repeatedly made me waste time trying to figure out why I was
getting oranges on those jobs, which, it turned out, are already busted
with an unmodified tree.

Mike

[Kris Maglione]

On Sun, Jun 24, 2018 at 03:28:45PM -0400, Boris Zbarsky wrote:

I tend to worry about the amount of extra automation runtime
that kind of policy will trigger. I tend to try to limit my try
runs to areas they're likely to affect to avoid wasting
thousands of hours of machine time on things I know cannot be
affected by a patch. The integration trees try to do something
similar, based on changed files. Try does not. When you specify
`-p all -u all`, it really does try to run all tests. Including
things like ccov, which always fail, and also don't matter for
try runs.

I also don't know how much it would affect the oranges I get on
inbound. A significant fraction of those are rebase issues,
because I tend to do my first try run before review (or while
waiting for review), and don't want to do a whole new try run
when I land.

Most of the rest are because I almost always forget to do
Android runs, and sometimes forget what sorts of weird things
can break Windows but not other platforms. But our Android and
Windows slaves tend to be overloaded as it is, and I don't want
to just start throwing extra jobs at them every time I write a
commit, on the odd chance that one of my changes will affect
them. Though there are definitely specific sorts of patches I
should remember to run Android and Windows mochitests on more
often.

[Sebastian Hengst]

The new policy will be used from now on.

- Sebastian

-------- Original-Nachricht --------
Betreff: mozilla-inbound backout policy subject to change (become
similar to autoland)
Von: Sebastian Hengst <she...@mozilla.com>
Datum: 2018-06-19 15:04

> Hi,
>
> If you don't push code to mozilla-inbound, you can stop reading now.
>

> TL;DR: We would like to change the mozilla-inbound backout policy to be
> like autoland’s.
>

> In order to decrease tree closure times for inbound, allow for more
> frequent merges from inbound to m-c, and ensure consistent policies
> across our integration branches, we propose that mozilla-inbound adopt
> the same policy as autoland for how issues with a push (failures,
> exception loops, ...) are handled. Rather than asking the developer to
> investigate and fix the issue with a follow-up push if possible,
> sheriffs will instead backout the patch and notify developers in the bug
> and IRC if possible.
>
> If your patch touches so much code that it is always at risk of
> bitrotting or is urgent (e.g. Release Management wants it ASAP), talk to
> the sheriffs in the #sheriffs channel on IRC
> before you push and get their approval to push (this is for checking
> that mozilla-inbound will have a recent "good" state for merges).
>
> If your patches got backed out, you can reapply them locally with |hg
> graft -fr <revset>| and fix them with |hg histedit| or |hg commit
> --amend| before pushing.
>
> Sebastian
> Sheriff Trainer & Code Quality Engineer
>

> _______________________________________________
> firefox-dev mailing list
> firef...@mozilla.org
> https://mail.mozilla.org/listinfo/firefox-dev

[Sebastian Hengst]

-------- Original-Nachricht --------
Betreff: Re: mozilla-inbound backout policy subject to change (become
similar to autoland)
Von: Boris Zbarsky <bzba...@mit.edu>
Datum: 2018-06-24 21:28

The recommend Try practices can be found at
https://wiki.mozilla.org/Sheriffing/How_To/Recommended_Try_Practices

Tier 1 needs to pass.

Tier 2 either needs to pass or (not be a mass failure and the developer
have an ETA for fix [up to 1 day]).

Tier 3 isn't watched by sheriffs.

Because new test suites and platforms start on a different platform than
tier 1, testing all jobs which are tier 1 and only those with a try
syntax is very hard. E.g. there are talos jobs in tier 1 and 2 and
Marionette on Android debug in tier 2 but as tier 1 on Android opt.

For the following usage groups (this doesn't cover everybody), this is
what I would recommend:

platform backend (no frontend like e.g. permission prompts for a website
permission, or hardware/heavily OS dependent features): Linux x64 both
opt and debug builds, pick the test suites you need (mochitest plain +
crashtest + xpcshell + web-platform-test + ...)

platform assuming it affects all platforms (with frontend like e.g.
permission prompts for a website permission, or hardware/heavily OS
dependent features): opt builds for Linux x64, macOS, Windows, Android;
Linux x64 debug builds, pick the test suites you need (mochitest plain +
crashtest + xpcshell + web-platform-test + ...)

frontend: linux64 opt, add macOS and Windows if it depends on platform
specific features, test mochitest browser-chrome and/or devtools,
mochitest clipboard (contains both browser-chrome and devtools tests
which perform better on beefier/our machines?), mochitest a11y,
marionette, xpcshell + ...

This a rough guideline and there is no syntax which will provide 100%
(some tier 1 jobs like localization only run on mozilla-central and not
the integration repositories).

If there is an issue with a patch which hasn't been spotted on Try
because it was e.g. intermittent, didn't affect every platform, was in a
test suite which focuses on testing something else or the patch
bitrotted, it will get backed out and be realistically regarded as
unavoidable.

The Try pushes are for a reasonable protection against cross-platform
permafails in test suites designed to test those changes. The faster
backouts shall keep inbound closed for shorter timeframes and backed out
patches shall be fixed locally and pushed again.

- Sebastian

[Dave Townsend]

On Tue, Jun 26, 2018 at 3:06 PM Sebastian Hengst <ar...@gmx.de> wrote:

> -------- Original-Nachricht --------
> Betreff: Re: mozilla-inbound backout policy subject to change (become
> similar to autoland)
> Von: Boris Zbarsky <bzba...@mit.edu>
> Datum: 2018-06-24 21:28
> > On 6/19/18 9:04 AM, Sebastian Hengst wrote:
> >> TL;DR: We would like to change the mozilla-inbound backout policy to
> >> be like autoland’s.
> >
> > This seems like a pretty reasonable change in general.
> >
> > Is there a well-documented try syntax string which corresponds to "these
> > are the things that need to be green to avoid being backed out"?
> > Presumably that string is not "-p all -u all" because it should exclude
> > tier2 and lower things, but it's not entirely clear to me what the
> > autoland backout criteria are. I would assume we want developers to do
> > a try run with that syntax before pushing to inbound as needed.
> >
> > -Boris
>
> The recommend Try practices can be found at
> https://wiki.mozilla.org/Sheriffing/How_To/Recommended_Try_Practices

This doesn't seem to answer the question. I frequently do patches where I
can't reliably guess the extent of test breakage and want to run a full set
to be on the safe side. What try configuration gives us tier 1 results only?

[Gregory Szorc]

On Tue, Jun 26, 2018 at 3:45 PM, Dave Townsend <dtow...@mozilla.com>
wrote:

> On Tue, Jun 26, 2018 at 3:06 PM Sebastian Hengst <ar...@gmx.de> wrote:
>

> > -------- Original-Nachricht --------
> > Betreff: Re: mozilla-inbound backout policy subject to change (become
> > similar to autoland)
> > Von: Boris Zbarsky <bzba...@mit.edu>
> > Datum: 2018-06-24 21:28
> > > On 6/19/18 9:04 AM, Sebastian Hengst wrote:
> > >> TL;DR: We would like to change the mozilla-inbound backout policy to
> > >> be like autoland’s.
> > >
> > > This seems like a pretty reasonable change in general.
> > >
> > > Is there a well-documented try syntax string which corresponds to
> "these
> > > are the things that need to be green to avoid being backed out"?
> > > Presumably that string is not "-p all -u all" because it should exclude
> > > tier2 and lower things, but it's not entirely clear to me what the
> > > autoland backout criteria are. I would assume we want developers to do
> > > a try run with that syntax before pushing to inbound as needed.
> > >
> > > -Boris
> >
> > The recommend Try practices can be found at
> > https://wiki.mozilla.org/Sheriffing/How_To/Recommended_Try_Practices
>
>

> This doesn't seem to answer the question. I frequently do patches where I
> can't reliably guess the extent of test breakage and want to run a full set
> to be on the safe side. What try configuration gives us tier 1 results
> only?
>

I agree that the default behavior of `-p all -u all -t all` is running too
much and confusing people. I filed bug 1471404 to change the behavior.

Will that be sufficient to fix the issue?

FWIW there were also some discussions at the all hands about in-tree
profiles for `mach try` to make it easier for people to perform common test
scenarios. Would this be a better solution?

[Dave Townsend]

On Tue, Jun 26, 2018 at 3:54 PM Gregory Szorc <g...@mozilla.com> wrote:

> On Tue, Jun 26, 2018 at 3:45 PM, Dave Townsend <dtow...@mozilla.com>
> wrote:
>
>> On Tue, Jun 26, 2018 at 3:06 PM Sebastian Hengst <ar...@gmx.de> wrote:
>>

>> > -------- Original-Nachricht --------
>> > Betreff: Re: mozilla-inbound backout policy subject to change (become
>> > similar to autoland)
>> > Von: Boris Zbarsky <bzba...@mit.edu>
>> > Datum: 2018-06-24 21:28
>> > > On 6/19/18 9:04 AM, Sebastian Hengst wrote:
>> > >> TL;DR: We would like to change the mozilla-inbound backout policy to
>> > >> be like autoland’s.
>> > >
>> > > This seems like a pretty reasonable change in general.
>> > >
>> > > Is there a well-documented try syntax string which corresponds to
>> "these
>> > > are the things that need to be green to avoid being backed out"?
>> > > Presumably that string is not "-p all -u all" because it should
>> exclude
>> > > tier2 and lower things, but it's not entirely clear to me what the
>> > > autoland backout criteria are. I would assume we want developers to
>> do
>> > > a try run with that syntax before pushing to inbound as needed.
>> > >
>> > > -Boris
>> >
>> > The recommend Try practices can be found at
>> > https://wiki.mozilla.org/Sheriffing/How_To/Recommended_Try_Practices
>>
>>

>> This doesn't seem to answer the question. I frequently do patches where I
>> can't reliably guess the extent of test breakage and want to run a full
>> set
>> to be on the safe side. What try configuration gives us tier 1 results
>> only?
>>
>
> I agree that the default behavior of `-p all -u all -t all` is running too
> much and confusing people. I filed bug 1471404 to change the behavior.
>
> Will that be sufficient to fix the issue?
>

Sounds good to me.

[Andreas Tolfsen]

Also sprach Gregory Szorc:

> FWIW there were also some discussions at the all hands about in-tree
> profiles for `mach try` to make it easier for people to perform
> common test scenarios. Would this be a better solution?

For as long as we can’t reliably track which changed files impact
what tests, pre-programmed test scenarios seem like a good idea.

I’ve often found myself sharing my try syntax strings with new
contributors. I have these exposed as environment variables for
easy access, e.g. "./mach try $trymn”.

If I instead could tell them to run a given test scenario which
included a recipe of most likely offenders that would be a big
improvement compared to try-fuzzy or copying—what to a new contributor
might seem like black magic—a string.

[Boris Zbarsky]

On 6/26/18 6:53 PM, Gregory Szorc wrote:
> I agree that the default behavior of `-p all -u all -t all` is running too
> much and confusing people. I filed bug 1471404 to change the behavior.
>
> Will that be sufficient to fix the issue?

The proposal is to have that syntax run all tier1 and tier2 things, right?

It seems to me that we should have a way of running just tier1, since
tier2 problems should not lead to immediate backout.

> FWIW there were also some discussions at the all hands about in-tree
> profiles for `mach try` to make it easier for people to perform common test
> scenarios. Would this be a better solution?

This would be useful too, of course.

-Boris

[Mike Hommey]

On Wed, Jun 27, 2018 at 12:36:22AM -0400, Boris Zbarsky wrote:
> On 6/26/18 6:53 PM, Gregory Szorc wrote:
> > I agree that the default behavior of `-p all -u all -t all` is running too
> > much and confusing people. I filed bug 1471404 to change the behavior.
> >
> > Will that be sufficient to fix the issue?
>
> The proposal is to have that syntax run all tier1 and tier2 things, right?
>
> It seems to me that we should have a way of running just tier1, since tier2
> problems should not lead to immediate backout.

Perma bustage of tier-2 jobs usually do.

Mike

[Boris Zbarsky]

On 6/27/18 12:50 AM, Mike Hommey wrote:
> Perma bustage of tier-2 jobs usually do.

Of a bunch of them, or of just one tier-2 job (but permanent bustage
across all pushes)?

My understanding was that tier-2 meant it can be broken for a bit
without backout happening. If that's not the case, how does it differ
from tier-1?

-Boris

[Kris Maglione]

Just to add one other point to this discussion:

I *hate* backouts. I don't just mean of my patches. I mean in
general. Whenever I come across a backout in a blame walk or a
bisect, it makes my life much more difficult. And I know I'm not
alone in this.

I thought the policy for autoland was pretty reasonable when it
was meant to use history rewriting to actually remove bad
changesets from change history, but I haven't been a fan of it
since that plan was abandoned.

If this new policy gives us open trees a slightly higher
percentage of the time at the expense of winding up with more
backouts for trivial ESLint bustages or one-line test fixes, I
don't think it's worth it. Having to wait for trees to re-open
is not a huge inconvenience to me. Dealing with mangled blame
is.

On Sun, Jun 24, 2018 at 03:28:45PM -0400, Boris Zbarsky wrote:

[Myk Melez]

Boris, did you ever get an answer to this question?

It remains relevant, as I just had a change [1] backed out immediately
for busting Btup, which is currently categorized as tier-2.

Sheriffing/Job Visibility Policy [2] describes tier-2 as:
> Jobs are shown by default on Treeherder, but are not sheriff-managed.
> Results will be shown on Treeherder "for information only". New test
> failures/bustage will not result in a backout, but a tracking bug will
> be filed when observed.
It seems like this document is either out-of-date, and the tier-2 policy
has changed, or Btup is miscategorized.

-myk

[1]
https://phabricator.services.mozilla.com/rMOZILLACENTRAL08fa47a24e89697e4e43177860b55cc28298bbff
[2]
https://wiki.mozilla.org/Sheriffing/Job_Visibility_Policy#Overview_of_the_Job_Visibility_Tiers

[Boris Zbarsky]

On 8/8/18 8:11 PM, Myk Melez wrote:
> Boris, did you ever get an answer to this question?

I did not.

-Boris

[Sebastian Hengst]

-------- Original-Nachricht --------
Betreff: Re: mozilla-inbound backout policy subject to change (become
similar to autoland)

Von: Myk Melez <m...@mykzilla.org>
Datum: 2018-08-09 02:11

> Boris Zbarsky wrote on 2018-06-26 21:53:
>> On 6/27/18 12:50 AM, Mike Hommey wrote:
>>> Perma bustage of tier-2 jobs usually do.
>>
>> Of a bunch of them, or of just one tier-2 job (but permanent bustage
>> across all pushes)?
>>
>> My understanding was that tier-2 meant it can be broken for a bit
>> without backout happening.  If that's not the case, how does it differ
>> from tier-1?

Tier 2 can permafail if there is an ETA for a fix the developer and the
sheriff have agreed upon. The backout of Myk's push was a mistake and
the sheriffs have been reminded to contact the developer if there issues
with tier 2 jobs after a push.

>
> It remains relevant, as I just had a change [1] backed out immediately
> for busting Btup, which is currently categorized as tier-2.
>
> Sheriffing/Job Visibility Policy [2] describes tier-2 as:
>> Jobs are shown by default on Treeherder, but are not sheriff-managed.
>> Results will be shown on Treeherder "for information only". New test
>> failures/bustage will not result in a backout, but a tracking bug will
>> be filed when observed.
> It seems like this document is either out-of-date, and the tier-2 policy
> has changed, or Btup is miscategorized.

Tier 2 jobs are going to be re-evaluated if that tier is still suitable
or if it's something which should not be broken because we rely on it
(e.g. QuantumRender, Btup is also heading for this).


Sebastian

[Myk Melez]

Sebastian Hengst wrote on 2018-08-09 08:51:
> Tier 2 jobs are going to be re-evaluated if that tier is still
> suitable or if it's something which should not be broken because we
> rely on it (e.g. QuantumRender, Btup is also heading for this).

Good to know, thanks, Sebastian. Is there a way to track that
re-evaluation, perhaps a bug or a thread in another forum? (It's ok if
not, as long as there's a way to find out the result, such as a post to
this thread once a decision has been made.)

-myk

[Sebastian Hengst]

The job visibility policy [1] says that tier 2 is observed but unmanaged
while the documentation on tier 2 platforms [2] explains that changes
causing issue for tier 2 are not immediately backed out but get reverted
if no fix is found.

The job visibility policy and the classification of jobs as tier 2 is
currently under review.

Having jobs permafail without an enforced ETA for a fix causes those
failures to accumulate and makes the jobs not observable anymore. For
that reason and to be able to get a quick overview of the state of
mozilla-central and Try pushes, merges to mozilla-central aim for merge
candidate revisions without issues on Tier 2. If the developer whose
push started tier 2 failures doesn't answer messages from the sheriffs
about the issue, there is no ETA for a fix and the change gets reverted.

Prolonged permafailures on tier 2 have never been allowed in the last 3
years.

[1] https://wiki.mozilla.org/Sheriffing/Job_Visibility_Policy
[2]
https://developer.mozilla.org/en-US/docs/Mozilla/Supported_build_configurations

Sebastian

-------- Original-Nachricht --------
Betreff: mozilla-inbound backout policy subject to change (become
similar to autoland)

Von: Sebastian Hengst <she...@mozilla.com>
Datum: 2018-06-19 15:04

[Sebastian Hengst]

The job visibility policy [1] says that tier 2 is observed but unmanaged
while the documentation on tier 2 platforms [2] explains that changes
causing issue for tier 2 are not immediately backed out but get reverted
if no fix is found.

The job visibility policy and the classification of jobs as tier 2 is
currently under review.

Having jobs permafail without an enforced ETA for a fix causes those
failures to accumulate and makes the jobs not observable anymore. For
that reason and to be able to get a quick overview of the state of
mozilla-central and Try pushes, merges to mozilla-central aim for merge
candidate revisions without issues on Tier 2. If the developer whose
push started tier 2 failures doesn't answer messages from the sheriffs
about the issue, there is no ETA for a fix and the change gets reverted.

Prolonged permafailures on tier 2 have never been allowed in the last 3
years.

[1] https://wiki.mozilla.org/Sheriffing/Job_Visibility_Policy
[2]
https://developer.mozilla.org/en-US/docs/Mozilla/Supported_build_configurations

Sebastian

-------- Original-Nachricht --------
Betreff: mozilla-inbound backout policy subject to change (become
similar to autoland)
Von: Sebastian Hengst <she...@mozilla.com>
Datum: 2018-06-19 15:04