Open Stack

 Hi all,
let's discuss code review process in Fuel and what we can improve. For
those who want to just have a quick context of this email, please check out
presentation slides [5].

** Issues **
Depending on a Fuel subproject, I'm aware of two buckets of issues with
code review in Fuel:
a) It is hard to get code reviewed and merged
b) Quality of code review itself could be better

First bucket:
1) It is hard to find subject matter experts who can help and core
reviewers for the area of code, especially if you are new to the project
2) Contributor sometimes receives contradicting opinions from other
reviewers, including cores
3) Assigned / responsible core reviewer is needed for a feature in order to
help in architectural negotiations, guiding through, landing the code into
master
4) Long wait time for getting code reviewed

Quality-related items:
5) Not thorough enough, full review in one shot. For example, reviewer can
put "-1" due to missed comma, but do not notice major gap in the code. It
leads to many patch sets, and demotivation of contributors
6) Some of the core reviewers decreased their involvement, and so number of
reviews has dropped dramatically. However, they still occasionally merge
code. I propose to remove these cores, and get them back if their
involvement is increased back again (I very rarely merge code, but I'm one
of those to be removed from cores). This is standard practice in OpenStack
community as well, see Neutron as example [4, line 270].
7) As a legacy of the past, we still have old core reviewers being able to
merge code in all Fuel repos. All new cores have core rights only for
single repo, which is their primary area of expertise. For example, core
team size for fuel-library is adidenko + whole fuel-core group [7]. In
fact, there are just 4 "trusted" or real core reviewers in fuel-library,
not the whole fuel-core group.

These problems are not new to OpenStack and open source in general. You can
find discussions about same and similar issues in [1], [2], [3].


** Analysis of data **
In order to understand what can be improved, I mined the data at first.
Main source of information was stackalytics.com. Please take a look at few
graphs on slides 4-7 [5], built based on data from stackalytics. Major
conclusions from these graphs:
1) Rather small number of core reviewers (in comparison with overall number
of contributors) reviewing 40-60% of patch sets, depending on repo (40%
fuel-library, 60% fuel-web). See slide #4.
2) Load on core reviewers in Fuel team is higher in average, if you compare
it with some other OpenStack projects. Average load on core reviewer across
Nova, Keystone, Neutron and Cinder is 2.5 reviews a day. In Fuel though it
is 3.6 for fuel-web and 4.6 for fuel-library. See slide #6.
3) Statistics on how fast feedback on code proposed is provided:
- fuel-library: 2095 total reviews in 30 days [13], 80 open reviews,
average wait time for reviewer - 1d 1h [12]
- fuel-web: 1789 total reviews in 30 days [14], 52 open reviews, average
wait time for reviewer - 1d 17h [15]

There is no need to have deep analysis on whether we have well defined
areas of ownership in Fuel components or not: we don’t have it formally
defined, and it’s not documented anywhere. So, finding a right core
reviewer can be challenging task for a new contributor to Fuel, and this
issue has to be addressed.


** Proposed solution **
According to stackalytics, for the whole fuel-group we had 262 reviewers
with 24 core reviewers for the past 180 days [19]. I think that these
numbers can be considered as high enough in order to think about structure
in which code review process would be transparent, understandable and
scalable.

Let’s first agree on the terminology which I’d like to use. It can take
pages of precise definitions, however in this email thread I’d like to
focus on code review process more, and hopefully high level description of
roles would be enough for now.
- Contributor: new contributor, who doesn’t work on Fuel regularly and
doesn’t know team structure (or full time Fuel developer, who just started
his work on Fuel)
- SME: subject matter expert of certain Fuel area of code, which he / she
regularly contributes to and reviews code of other contributors into this
area. Example: network checker or Nailgun agent.
- Core Reviewer: expert in one or few parts of Fuel, who was promoted to
Core Reviewers team thanks to the contribution, high rate of quality
reviews.
- Component Lead: The one who defines architecture of a particular module
or component in Fuel, does majority of merges there, resolves conflicts
between SMEs and / or contributors in the area of responsibility, if
conflicts can’t be resolved by other means. Component Lead has to review
all design specs in its parts where his/her component is under change.
- Fuel PTL: Project Team Lead in its OpenStack standard definition [8],
delegates most of the work to component leads, but helps in cases when
resolution of conflicts between component leads is needed. A way to resolve
conflicts and clear escalation path should help to resolve issue #2. I’d
like to notice, that conflicts in a collaborative work is just normal
phenomenon. Please see more on this at [11].

Fuel currently lacks formal SMEs and their areas of ownership, and
component leads. So my suggestion is to address it separately. Some
examples on how it is documented in different projects: OpenStack Rally
[20], OpenStack Neutron [4, line 105], Docker [10]. Now, in order to solve
some of the issues mentioned at the beginning, we need a structure which
would have a leverage for it. According to the data analysis, load on core
reviewers is extremely high. I think that first step has to be to figure
out a way of offloading some work from them in order to ask for better
results. Namely, I suggest to:
a) identify component leads out of existing core reviewers
b) ensure that component leads for large components like Nailgun or
fuel-library don’t run features or major feature development, so they can
focus on architecture of component, and majority of thorough core reviewers
into component

Now, I suggest to go even further and not to expect core reviewers to
review patches which have not been yet reviewed by contributors’ peers
(please see important of it at [17]), SME or which don’t yet have +1 from
CI. In fact, this is the suggestion to adopt dictator-lieutenant delegation
workflow [7]. To be precise, I would expect that:
- Contributor finds SME to review the code. Ideally, contributor can have
his/her peers to help with code review first. Contributor doesn’t bother
SME, if CI has -1 on a patch proposed
- SME reviews the code within SLA, which should be defined per component
- Once SME has reviewed a code, Core Reviewer specialized on a component
reviews the code within SLA. Review inbox [16, “Ready for Core Reviewers”]
can help to find changesets to be reviewed / merged
- If core reviewer has not landed the code yet, Component Lead merges patch
within SLA defined (or declines to merge and provides explanation as part
of review).

SLA should be the driver of doing timely reviews, however we can’t allow to
fast-track code into master suffering quality of review, just in order to
meet SLA. I suggest to see metrics at every IRC weekly meeting, and based
on data - ask for help in review core reviewers from other areas, or reset
expectations of contributors / SMEs on how fast patches can be reviewed and
merged (redefine SLA).

This flow is represented on slides 11-14 [5]. SLAs should solve an issue #4
from the list, and align on expectations. Of course, SLAs defined have to
be documented somewhere in public place.

In order to have convenient and up to date documentation on who are SMEs
and component owners for particular areas of code, I suggest similar schema
to Google’s one [9] (if we can trust this source, but I like the idea
anyway). For Fuel it can look like the following - each top-level directory
of every repository has to have file “MAINTAINERS”, which must have list of
SMEs and name of a Component Lead. Now, for every changeset proposed,
Jenkins can be used to identify folders affected in order to get list of
corresponding SMEs and add them to Gerrit review. This should be convenient
notification for only those who maintain a particular area, and not a spam
for everyone. Such a change should fully address issue #1 from the list.

In order to help feature leads to drive the work, ensure that it can land
to master by certain date and manage expectations across components
properly, we need to identify for every feature, who is the contact point
from core reviewers team in every component under change. This can be
Component Lead or it can be delegated to some other trusted core reviewer.
It is expected that assigned person will participate in periodic sync ups
with feature team, consult how changes should be made in order to align
with architecture, and find right SMEs to help with code review and/or
expertise when needed. This should fully address issue #3.

Quality-related issues #6 and #7 already have suggestions. Issue #5, about
doing thorough review at first pass, needs close attention. PTL and
Component Leads (once identified) have to have a right to remove members of
core team, which do not comply to standards of quality in code review.
There is a great checklist [18], which I’d encourage everyone to follow
while writing code and doing code reviews. Also, there is a specific Fuel
Python-related page [6], which may need to be updated. Accordingly, issues
of such a kind with particular examples should be escalated to PTL.

Thoughts?

[1] http://lists.openstack.org/pipermail/openstack-dev/2015-June/065532.html
[2] https://etherpad.openstack.org/p/liberty-cross-project-in-team-scaling
[3] http://opensource.com/life/14/5/best-code-review-open-source-projects
[4]
http://git.openstack.org/cgit/openstack/neutron/tree/doc/source/policies/core-reviewers.rst
[5]
https://docs.google.com/presentation/d/1HMSovUxujOUwILPSjiuZHqAxo1TujQazA2yGHSsQC6U
[6] https://wiki.openstack.org/wiki/Fuel/Code_Review_Rules
[7] https://git-scm.com/book/en/v2/Distributed-Git-Distributed-Workflows
[8] https://wiki.openstack.org/wiki/PTL_Guide
[9] http://www.quora.com/What-is-Googles-internal-code-review-policy-process
[10] https://github.com/docker/docker/blob/master/MAINTAINERS
[11] https://adtmag.com/articles/2014/12/17/agile-conflict-resolution.aspx
[12] http://stackalytics.com/report/reviews/fuel-library/open
[13] http://stackalytics.com/report/contribution/fuel-library/30
[14] http://stackalytics.com/report/contribution/fuel-web/30
[15] http://stackalytics.com/report/reviews/fuel-web/open
[16] http://bit.ly/1LjYO4t. Full link is available from Fuel wiki:
wiki.openstack.org/wiki/Fuel
[17] "You should probably be spending at least a couple of hours on code
review every day. Not just because the number of code reviewers on a
project has the greatest influence on its velocity, but also because its
the best way to start building trust with your fellow contributors. If you
can show yourself as thoughtful, committed and diligent through your code
reviews, then other code reviewers will be much more inclined to prioritize
your patches and less carefully scrutinize your work."
https://blogs.gnome.org/markmc/2014/06/06/an-ideal-openstack-developer/
[18]
https://developer.ibm.com/opentech/2015/03/27/checklist-performing-openstack-code-reviews/
[19] http://stackalytics.com/report/contribution/fuel-group/180

-- 
Mike Scherbakov
#mihgen
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20150819/8181111c/attachment-0001.html>