RFC: Review policy for bustage fixes
As the pace of bustages hitting Thunderbird has increased with rapid
changes to m-c, we've struggled to keep up. Jörg has been focused on
this issue, but has requested that we clarify the review policy for
these bustage fixes. He is in the untenable position of being
responsible for keeping c-c buildable, but there are not adequate
reviewers to do timely reviews of needed changes.
There has been some discussions of this issue by the Thunderbird
Council, but really this is a technical issue that is best resolved by
the active developers, which is this maildev list.
Let me put forward a policy proposal, which as I understand it is
largely what is proposed by Jörg, with some additional details filled in:
-
We introduce the concept of post-landing reviews for bustage
fixes. That is, qualified developers are allowed to land bustage fixes
to c-c and c-beta Thunderbird directories without review. Let's call
such patches "PLR patches" for "Post Landing Review patches". Landings
to c-beta also require the usual approval from the release engineer
(currently Jörg). This policy only applies to patches needed to keep c-c
or c-b builds usable, that is fixing a failure to build, significant new
test failures, or major regressions. -
"Qualified developers" who can land PLR patches are module owners
and peers of the relevant directories. (Should we allow staff engineers,
currently Jörg, to land PLR patches in other c-c directories,
particularly Calendar and Chat?) -
"Qualified developers" may also backout PLR patches prior to
review, even those landed by others, should they believe that the patch
is inappropriate. -
PLR patches shall be assigned to a reviewer for review. (Should we
create a generic review account, like tb-reviewer, that could receive
this assignment?) -
PLR patches with pending reviews shall be flagged as such by
adding the word "PLR" to the BMO whiteboard for the relevant bug. This
flag shall be removed by the reviewer when the patch is reviewed. This
allows tracking of pending PLR reviews. -
We set the goal to complete all PLR patch reviews that are part of
a major release prior to that major release.
Although this policy is not common in Mozilla, variants are fairly
common in other open source projects. We are actively trying to hire
additional developers for Thunderbird at the moment, and we may change
this policy when we get to the point where there are enough staff
developers to accomplish these reviews in a timely manner.
Comments?
On 01/11/2017 21:50, R Kent James wrote:
Should we allow staff engineers, currently Jörg, to land PLR patches in other c-c directories, particularly Calendar and Chat?)
Absolutely. Bustage that hits Calendar (not uncommon) could cause a huge test failure and make sheriffing practically impossible.
Jörg.
On 11/1/2017 5:00 PM, Jörg Knobloch wrote:
On 01/11/2017 21:50, R Kent James wrote:
Should we allow staff engineers, currently Jörg, to land PLR patches
in other c-c directories, particularly Calendar and Chat?)
Absolutely. Bustage that hits Calendar (not uncommon) could cause a
huge test failure and make sheriffing practically impossible.
Jörg.
To add some flavor, the council felt it was essential that Jorg's build
bustage fix work not be unduly impeded, that there be a process in
bugzilla, and that the process should reduce stress and pressure on the
person doing such work. It is good to see a potential solution being
adopted.
----- Pôvodná správa -----
Predmet: [Maildev] RFC: Review policy for bustage fixes
Od: R Kent James kent@caspia.com
Pre: Thunderbird email developers maildev@lists.thunderbird.net
Dátum: Wed, 1 Nov 2017 13:50:17 -0700
1) We introduce the concept of post-landing reviews for bustage
fixes. That is, qualified developers are allowed to land bustage fixes
to c-c and c-beta Thunderbird directories without review. Let's call
such patches "PLR patches" for "Post Landing Review patches". Landings
to c-beta also require the usual approval from the release engineer
(currently Jörg). This policy only applies to patches needed to keep c-c
or c-b builds usable, that is fixing a failure to build, significant new
test failures, or major regressions.
Thanks, I have commented in the past that this seems a good idea and
worked so far (executed informally).
2) "Qualified developers" who can land PLR patches are module owners
and peers of the relevant directories. (Should we allow staff engineers,
currently Jörg, to land PLR patches in other c-c directories,
particularly Calendar and Chat?)
The respective module owners could decide this for their modules. But if
they do not allow staff/Jorg to do it, then they must be ready to fix
stuff quickly themselves.
4) PLR patches shall be assigned to a reviewer for review. (Should we
create a generic review account, like tb-reviewer, that could receive
this assignment?)
I vote for assigning to a real reviewer before landing.
aceman
On 03-11-2017 22:31, ace wrote:
4) PLR patches shall be assigned to a reviewer for review. (Should we
create a generic review account, like tb-reviewer, that could receive
this assignment?)
I vote for assigning to a real reviewer before landing.
Thinking some more about this, I agree it may be preferable to flag a
real reviewer.
-Magnus
Hi Folks,
sorry for the late reply on this, and thanks for putting it together. I
totally agree that we need to formalize the process that Jörg hast been
working under, otherwise it will (and has) caused grief among others
because of breaking the current process. I also see how the current
process with the review situation we have makes it difficult to keep the
tree green.
Some comments inline:
1) We introduce the concept of post-landing reviews for bustage
fixes. That is, qualified developers are allowed to land bustage fixes
to c-c and c-beta Thunderbird directories without review. Let's call
such patches "PLR patches" for "Post Landing Review patches". Landings
to c-beta also require the usual approval from the release engineer
(currently Jörg). This policy only applies to patches needed to keep
c-c or c-b builds usable, that is fixing a failure to build,
significant new test failures, or major regressions.
I'd like to suggest to revise/clarify this so that there is at least one
more set of eyes for landings to comm-beta. The person creating the
patch should not at the same time give it the rubber stamp for landing,
approve it for beta, and land it there. So concretely, since Jörg takes
care of beta approvals, if he is also the one who pushed the bustage fix
(either creating the patch himself or piecing together someone else's
code snippet), I don't believe it is a good idea to have him do the beta
approval. We should appoint someone else in this case.
The consequence if we don't do this is that one person can push a patch
from creation to beta (and essentially also to release) without extra
eyes. Getting someone to approve a patch is much less effort than a
review, so I think this is a reasonable restriction.
The exception would likely be in build config if it is a patch that is
required to keep the builds working at all, but I suspect that in this
case Tom can easily step in either as patch author, reviewer, or approver.
2) "Qualified developers" who can land PLR patches are module
owners and peers of the relevant directories. (Should we allow staff
engineers, currently Jörg, to land PLR patches in other c-c
directories, particularly Calendar and Chat?)
If "Owners and Peers" are the people that can do this, essentially
everyone that can review can also land without review. I think this is
too broad. I'm not sure if we should just restrict this to those in the
"sheriff team" (so, Jörg) which we can expand, or if there is something
inbetween.
For calendar I'm fine with build config changes as PLR, but would like
to continue to pre-review anything that goes above it. If that blocks
tests from being green and the patch is not reasonably going to be
written within a few days or a week, I am fine with temporarily
disabling calendar tests (without review) after reasonable effort has
been made to get review. If it blocks the build from being green it is
likely a build config issue, if this is not the case then I will either
take care of the review quickly, or we can consider further exceptions.
The reason for this, and also my general concern with PLR, is that the
patches being created are often band-aids just to get the tree green
again. If reviews are not done in a timely manner, then those band aids
will stick forever, which decreases the quality of the product and will
bite us later. Given that at current calendar is not a very active
component, I think it is reasonable to go this way because possible test
failures or disabled tests are not blocking active development. Of
course, there may be more m-c patches that break calendar, but that is
my responsibility as module owner to prevent and act on.
3) "Qualified developers" may also backout PLR patches prior to
review, even those landed by others, should they believe that the
patch is inappropriate.
Given the premise that module owners and peers are qualified developers,
does this mean that (for the sake of example) Magnus could back out a
patch that was landed to keep the tree green, if he feels that there are
issues he would flag in a review, hence breaking the tree until everyone
is happy with the patch? I could see this as a potential point for
friction as on the one hand there is Jörg's need to keep the tree green,
and (again for the sake of example) Magnus' need to have a patch pushed
that doesn't have review issues.
4) PLR patches shall be assigned to a reviewer for review. (Should
we create a generic review account, like tb-reviewer, that could
receive this assignment?)
I liked the idea of a generic reviewer account as they have done it in
the core build config component. The obvious downsides are that nobody
specific is responsible, and it could lead to patches not being reviewed
because "oh well, someone else will take care". What we could do is have
a generic reviewer account, along with one or more triage owners that
will assign specific reviews in case the patch falls into the expertise
area of someone specific.
It seems a specific reviewer is what most people want here, I am fine
with that.
5) PLR patches with pending reviews shall be flagged as such by
adding the word "PLR" to the BMO whiteboard for the relevant bug. This
flag shall be removed by the reviewer when the patch is reviewed. This
allows tracking of pending PLR reviews.
We could request a keyword for this as well as it will be common enough.
6) We set the goal to complete all PLR patch reviews that are part
of a major release prior to that major release.
How do we make sure this happens? Do we not release until all patches
are reviewed?
Although this policy is not common in Mozilla, variants are fairly
common in other open source projects. We are actively trying to hire
additional developers for Thunderbird at the moment, and we may change
this policy when we get to the point where there are enough staff
developers to accomplish these reviews in a timely manner.
I agree this should be a temporary measure. Sure, it will take a new
developer some time to get up to speed, but we could change it then so
that the new developer takes care of the review, and if they do not feel
comfortable with the area of code yet they could keep the PLR keyword
and ask for second review from someone else.
Personally I would want a less loose strategy that restricts PLR reviews
to failures that fundamentally break tests (e.g. a simple incoming m-c
change that breaks all tests), and possibly reviews that take longer
than a certain time. Failures inbetween would be starred on treeherder,
just like Mozilla used to do before m-i. I think it should be reasonable
to wait for review a few days instead of pushing everything as a bustage
fix. From what I have gathered there is strong opposition to this mainly
from Jörg, but I would be interested to hear from everyone if we can
find a compromise inbetween.
Philipp
On 05/11/2017 11:17, Philipp Kewisch wrote:
Personally I would want a less loose strategy that restricts PLR reviews
to failures that fundamentally break tests (e.g. a simple incoming m-c
change that breaks all tests), and possibly reviews that take longer
than a certain time. Failures inbetween would be starred on treeherder,
just like Mozilla used to do before m-i. I think it should be reasonable
to wait for review a few days instead of pushing everything as a bustage
fix. From what I have gathered there is strong opposition to this mainly
from Jörg, but I would be interested to hear from everyone if we can
find a compromise inbetween.
In general you need to allow any measure that allows effective
sheriffing. If I have a test run with 100 failures, whacking a star onto
them doesn't help detecting the 101st and 102nd additional failures.
Surely we can live with one or two failing tests for a while. That is
the case anyway when the failure needs to be fixed in M-C.
I don't understand why we're discussing beta here. I can't recall a
single bustage fix incident on beta. IIRC, all beta uplifts have been
properly reviewed, and there was never any opposition to the author (me)
also being the approver (me) of the uplift.
Jörg.
On 11/5/17 11:35 AM, Jörg Knobloch wrote:
On 05/11/2017 11:17, Philipp Kewisch wrote:
Personally I would want a less loose strategy that restricts PLR reviews
to failures that fundamentally break tests (e.g. a simple incoming m-c
change that breaks all tests), and possibly reviews that take longer
than a certain time. Failures inbetween would be starred on treeherder,
just like Mozilla used to do before m-i. I think it should be reasonable
to wait for review a few days instead of pushing everything as a bustage
fix. From what I have gathered there is strong opposition to this mainly
from Jörg, but I would be interested to hear from everyone if we can
find a compromise inbetween.
In general you need to allow any measure that allows effective
sheriffing. If I have a test run with 100 failures, whacking a star
onto them doesn't help detecting the 101st and 102nd additional
failures. Surely we can live with one or two failing tests for a
while. That is the case anyway when the failure needs to be fixed in M-C.
I agree with that, but I'd like us to be a bit more creative as to what
these measures are. Yes, sheriffing needs to be effective, but there are
also other factors that need to be taken into account that are equally
important.
I don't understand why we're discussing beta here. I can't recall a
single bustage fix incident on beta. IIRC, all beta uplifts have been
properly reviewed, and there was never any opposition to the author
(me) also being the approver (me) of the uplift.
This is not about past incidents. The wording Kent used suggested that
pushes can be done without restrictions, so I think we should clarify
the policy. I have no issue if you are author and approver, but then you
shouldn't be reviewer, and this is a case where I think PLR should not
apply. Otherwise nobody has really seen the patches before they go to
beta/release. As mentioned, it shouldn't be to hard to find someone else
to approve if you are both author and reviewer (or author and no review
due to PLR).
Also unrelated to this reply, can we use [plr] in the whiteboard, no
explanation? This is more in line with how the whiteboard is being used
and allows for more tags if necessary.
Philipp
Philipp Kewisch wrote on 05.11.17 11:17:
3) "Qualified developers" may also backout PLR patches prior to
review, even those landed by others, should they believe that the
patch is inappropriate.
Given the premise that module owners and peers are qualified developers,
does this mean that (for the sake of example) Magnus could back out a
patch that was landed to keep the tree green, if he feels that there are
issues he would flag in a review, hence breaking the tree until everyone
is happy with the patch?
Yes. This is a necessary correction.
If we allow checkin without review, then those reviewers must have at
least the option to object - both before and after checkin, to avoid races.
In other words, we switch from "default deny" (can't land without
review) to "default allow" (can land without review), but that doesn't
override the reviewer.
If the reviewer is not OK with the patch and the committer is aware
that, the "default allow" policy no longer applies and it's an explicit
deny and the "land without review" doesn't apply, because there's
effectively a negative review.
Same should then apply after the fact, i.e. backout after landing, to
avoid that the committer hides things or outraces the reviewer.
Then, the resolution for the broken build could be creates another patch
using a different approach. For such controversial fixes, there are
usually alternatives.
Jörg Knobloch wrote on 05.11.17 11:35:
In general you need to allow any measure that allows effective sheriffing
I don't subscribe to this goal overriding everything else. It completely
ignores other goals.
The "wild west" ("sherrif") didn't work out so well. Even the police
today does not have absolute powers.
If the police cannot just break into doors just to catch a mischief,
then even less so we can commit any patch no matter which just to keep
the tree green. Some patches are just a bad idea, e.g. the fork of the
necko stream implementation, and it's better to have the tree red until
you have a better patch than to commit this, and the tree green, and
move on to other breakage.
If you don't have another manpower, then we need to add manpower. If we
can't get more manpower, then we need to think of completely different
solutions. Hiding issues with patchwork will only get us into even
bigger trouble later on.
Ben