settingsLogin | Registersettings

[openstack-dev] [Fuel] Code review process in Fuel and related issues

0 votes

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


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
asked Aug 19, 2015 in openstack-dev by Mike_Scherbakov (9,460 points)   1 4 6

14 Responses

0 votes

Mike,

This is a great start.

1) I'd advise to codify a proposal in fuel-specs under a 'policy' directory
(obviously as a review in fuel-specs repo) So everyone agrees to the
structure of the teams and terminology etc. Example oslo uses a directory
to write down some of our decisions.
http://git.openstack.org/cgit/openstack/oslo-specs/tree/specs/policy

2) We don't have SME terminology, but we do have Maintainers both in
oslo-incubator (
http://git.openstack.org/cgit/openstack/oslo-incubator/tree/MAINTAINERS)
and in Rally (https://rally.readthedocs.org/en/latest/project_info.html) So
let's use that

3) Is there a plan to split existing repos to more repos? Then each repo
can have a core team (one core team for one repo), PTL takes care of all
repos and MAINTAINERS take care of directories within a repo. That will
line up well with what we are doing elsewhere in the community (essentially
"Component Lead" is a core team which may not be a single person).

We do not have a concept of SLA anywhere that i know of, so it will have to
be some kind of social consensus and not a real carrot/stick. One way is to
publish/use data about reviews like stackalytics or russell's site (
http://russellbryant.net/openstack-stats/fuel-openreviews.html) and policy
the reviews that drop off the radar during the weekly meetings or something
like that.

Thanks,
Dims

On Wed, Aug 19, 2015 at 4:31 AM, Mike Scherbakov mscherbakov@mirantis.com
wrote:

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


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

--
Davanum Srinivas :: https://twitter.com/dims


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
responded Aug 27, 2015 by Davanum_Srinivas (35,920 points)   2 4 9
0 votes

Hi,

I'm all in for any formalization and automation of review process. The only
concern that I see here is about core reviewers involvement metrics. If we
succeed in reducing the load on core reviewers, it will mean that core
reviewers will do less code reviews. This could lead to core reviewer
demotion.

  • 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

I like the idea with adding reviewers automatically based on MAINTAINERS
file. In such case we can drop this ^^ part of instruction. It would be
nice if Jenkins could add reviewers after CI +1, or we can use gerrit
dashboard for SMEs to not waste their time on review that has not yet
passed CI and does not have +1 from other reviewers.

Regards,
Alex

On Wed, Aug 19, 2015 at 11:31 AM, Mike Scherbakov mscherbakov@mirantis.com
wrote:

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


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
responded Aug 27, 2015 by Aleksandr_Didenko (3,900 points)   1 5
0 votes

Mike,
speaking of automation, AFAIK Boris Pavlovic introduced some scripts
in Rally which do basic preliminary check of review message, checking
that it's formally correct. It should make life of reviewers a bit
easier, you might want to introduce them in Fuel as well, if not yet.
Regards,
Igor Marnat

On Thu, Aug 27, 2015 at 5:37 PM, Aleksandr Didenko
adidenko@mirantis.com wrote:
Hi,

I'm all in for any formalization and automation of review process. The only
concern that I see here is about core reviewers involvement metrics. If we
succeed in reducing the load on core reviewers, it will mean that core
reviewers will do less code reviews. This could lead to core reviewer
demotion.

  • 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

I like the idea with adding reviewers automatically based on MAINTAINERS
file. In such case we can drop this ^^ part of instruction. It would be nice
if Jenkins could add reviewers after CI +1, or we can use gerrit dashboard
for SMEs to not waste their time on review that has not yet passed CI and
does not have +1 from other reviewers.

Regards,
Alex

On Wed, Aug 19, 2015 at 11:31 AM, Mike Scherbakov mscherbakov@mirantis.com
wrote:

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


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
responded Aug 27, 2015 by Igor_Marnat (840 points)   1
0 votes

Hi Mike,

I have several comments.

SLA should be the driver of doing timely reviews, however we can’t allow
to fast-track code into master suffering quality of review ...

As for me the idea of SLA contradicts to qualitative reviews.

Another thing is I got a bit confused by the difference between Core
Reviewer and Component Lead,
aren't those the same persons? Shouldn't every Core Reviewer know the
architecture, best practises
and participate in design architecture sessions?

  • 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).

For example here as far as I'm concerned Component Lead is Core reviewer,
since
he has permissions to merge.

Thanks,

On Wed, Aug 19, 2015 at 11:31 AM, Mike Scherbakov mscherbakov@mirantis.com
wrote:

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


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
responded Aug 27, 2015 by Evgeniy_L (8,580 points)   1 2 4
0 votes
  • SME reviews the code within SLA, which should be defined per component

Also I would like to add, that I'm not against of metrics, we can collect
metrics, in order to figure out if some improvement in the process helped
to speed up reviews, but asking Cores/SMEs to do the job faster will
definitely
affect quality.

So the suggestion is to collect metrics only to prove that some
improvement
in the process helped or didn't help.

On Thu, Aug 27, 2015 at 5:58 PM, Evgeniy L eli@mirantis.com wrote:

Hi Mike,

I have several comments.

SLA should be the driver of doing timely reviews, however we can’t
allow to fast-track code into master suffering quality of review ...

As for me the idea of SLA contradicts to qualitative reviews.

Another thing is I got a bit confused by the difference between Core
Reviewer and Component Lead,
aren't those the same persons? Shouldn't every Core Reviewer know the
architecture, best practises
and participate in design architecture sessions?

  • 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).

For example here as far as I'm concerned Component Lead is Core reviewer,
since
he has permissions to merge.

Thanks,

On Wed, Aug 19, 2015 at 11:31 AM, Mike Scherbakov <
mscherbakov@mirantis.com> wrote:

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


OpenStack Development Mailing List (not for usage questions)
Unsubscribe:
OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
responded Aug 27, 2015 by Evgeniy_L (8,580 points)   1 2 4
0 votes

On 27 Aug 2015, at 07:58, Evgeniy L eli@mirantis.com wrote:

Hi Mike,

I have several comments.

SLA should be the driver of doing timely reviews, however we can’t allow to fast-track code into master suffering quality of review ...

As for me the idea of SLA contradicts to qualitative reviews.

We expect cores to be less loaded after this change, so you guys should have more time to spend on right reviews, and not minor stuff. We hope this will also help keeping SLAs.

Another thing is I got a bit confused by the difference between Core Reviewer and Component Lead,
aren't those the same persons? Shouldn't every Core Reviewer know the architecture, best practises
and participate in design architecture sessions?

Not really. You can have many core reviewers, but there should be one component lead. Currently, while Fuel is monolithic, we cannot implement it in technical way. But if we succeed splitting Fuel into smaller projects, component lead will be responsible for (most likely) one repo.

Regards,
--
Tomasz 'Zen' Napierala
Product Engineering - Poland


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
responded Aug 29, 2015 by Tomasz_Napierala (2,100 points)   1 3
0 votes

Hi folks,

So basically..

  • core reviewers won't be feature leads anymore
  • core reviewers won't be assigned to features (or at least not full-time)
  • core reviewers will spend time doing review and participate design meetings
  • core reviewers will spend time triaging bugs

Is that correct?

Thanks,
Igor

On Sun, Aug 30, 2015 at 2:29 AM, Tomasz Napierala
tnapierala@mirantis.com wrote:

On 27 Aug 2015, at 07:58, Evgeniy L eli@mirantis.com wrote:

Hi Mike,

I have several comments.

SLA should be the driver of doing timely reviews, however we can’t allow to fast-track code into master suffering quality of review ...

As for me the idea of SLA contradicts to qualitative reviews.

We expect cores to be less loaded after this change, so you guys should have more time to spend on right reviews, and not minor stuff. We hope this will also help keeping SLAs.

Another thing is I got a bit confused by the difference between Core Reviewer and Component Lead,
aren't those the same persons? Shouldn't every Core Reviewer know the architecture, best practises
and participate in design architecture sessions?

Not really. You can have many core reviewers, but there should be one component lead. Currently, while Fuel is monolithic, we cannot implement it in technical way. But if we succeed splitting Fuel into smaller projects, component lead will be responsible for (most likely) one repo.

Regards,
--
Tomasz 'Zen' Napierala
Product Engineering - Poland


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
responded Sep 1, 2015 by Igor_Kalnitsky (8,940 points)   1 2 4
0 votes

On 01 Sep 2015, at 03:43, Igor Kalnitsky ikalnitsky@mirantis.com wrote:

Hi folks,

So basically..

  • core reviewers won't be feature leads anymore
  • core reviewers won't be assigned to features (or at least not full-time)
  • core reviewers will spend time doing review and participate design meetings
  • core reviewers will spend time triaging bugs

Is that correct?

From what I understand, it is not correct. Core reviewer will still do all this activities in most cases. What we are trying to achieve, is to get core's attention only to reviews, that are already reviewed by SMEs and peers. We
hope this will increase the quality of code core reviewers are getting.

Regards,
--
Tomasz 'Zen' Napierala
Product Engineering - Poland


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
responded Sep 2, 2015 by Tomasz_Napierala (2,100 points)   1 3
0 votes

It won't work that way. You either busy on writing code / leading
feature or doing review. It couldn't be combined effectively. Any
context switch between activities requires an extra time to focus on.

On Wed, Sep 2, 2015 at 5:46 AM, Tomasz Napierala
tnapierala@mirantis.com wrote:

On 01 Sep 2015, at 03:43, Igor Kalnitsky ikalnitsky@mirantis.com wrote:

Hi folks,

So basically..

  • core reviewers won't be feature leads anymore
  • core reviewers won't be assigned to features (or at least not full-time)
  • core reviewers will spend time doing review and participate design meetings
  • core reviewers will spend time triaging bugs

Is that correct?

From what I understand, it is not correct. Core reviewer will still do all this activities in most cases. What we are trying to achieve, is to get core's attention only to reviews, that are already reviewed by SMEs and peers. We
hope this will increase the quality of code core reviewers are getting.

Regards,
--
Tomasz 'Zen' Napierala
Product Engineering - Poland


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
responded Sep 2, 2015 by Igor_Kalnitsky (8,940 points)   1 2 4
0 votes

On 09/02/2015 03:00 AM, Igor Kalnitsky wrote:
It won't work that way. You either busy on writing code / leading
feature or doing review. It couldn't be combined effectively. Any
context switch between activities requires an extra time to focus on.

I don't agree with the above, Igor. I think there's plenty of examples
of people in OpenStack projects that both submit code (and lead
features) that also do code review on a daily basis.

Best,
-jay


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
responded Sep 2, 2015 by Jay_Pipes (59,760 points)   3 11 14
...