Open Stack

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 at 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 at lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
>


-- 
Davanum Srinivas :: https://twitter.com/dims
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20150827/826f254a/attachment.html>