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