openstack team mailing list archive

[corvus@xxxxxxxxxxxx (James E. Blair)]

Mark McLoughlin <markmc@xxxxxxxxxx> writes:

> Hey,
>
> We discussed this during the "baking area for features" design summit
> session. I found that discussion fairly frustrating because there were
> so many of us involved and we all were either wanting to discuss
> slightly different things or had a slightly different understanding of
> what we were discussing. So, here's my attempt to put some more
> structure on the discussion.
>
> tl;dr - subsystem branches are managed by trusted domain experts and
> feature branches are just temporary rebasing branches on personal github
> forks. We've got a tonne of work to do figuring out how this would all
> work. We should probably pick a single subsystem and start with that.
>
> ...
>
> Firstly, problem definition:
>
>   - Nova is big, complex and has a fairly massive rate of churn. While 
>     the nova-core team is big, there isn't enough careful review going 
>     on by experts in particular areas and there's a consistently large
>     backlog of reviews.
>
>   - Developers working on features are very keen to have their work 
>     land somewhere and this leads to half-finished features being 
>     merged onto master rather than developers collaborating to get a 
>     feature to a level of completeness and polish before merging into 
>     master.
>
> Some assumptions about the solution:
>
>   - There should be a small number of domain experts who can approve 
>     changes to each of major subsystems. This will encourage 
>     specialization and give more clear lines of responsibility.
>
>   - There should be a small number of project dictators who have final 
>     approval on merge proposals, but who are not expected to review 
>     every patch in great detail. This is good because we need someone 
>     with an overall view of the project who can make sure efforts in 
>     the various subsystems are coordinated, without that someone being 
>     massively overloaded.
>
>   - New features should be developed on a branch and brought to a level 
>     of completeness before being merged into master. This is good 
>     because we don't want half-baked stuff in master but also because 
>     it encourages developers to break their features into stages where 
>     each stage of the work can be brought to completion and merged 
>     before moving on to the next stage.
>
>   - In essence, we're assuming some variation of the kernel distributed 
>     development model.
>
>     (FWIW, my instinct is to avoid the kernel model on projects. Mostly 
>     because it's extremely complex and massive overkill for most 
>     projects. Looking at the kernel history with gitk is enough to send 
>     anyone screaming for the hills. However, Nova seems to be big 
>     enough that we're experiencing the same pressures that drove the 
>     kernel to adopt their model)
>
> Ok, what are "subsystem branches" and how would they work?
>
>   - Subsystem branches would have a small number of maintainers who can 
>     approve a change. These would be domain experts providing strong 
>     oversight over a particular area.
>
>     (In gerrit, this is a branch with a small team or single person who 
>     can +1 approve a review)

Agree.  We can create a branch, say:

  subsystem/scheduler/next

Which is the linux-next equivalent for that subsystem, where new
features should be proposed and merged in preparation for the next
"merge window".  "Merge window" in the case of folsom may be 6 months
long, but would shrink if we move to a more rolling release model.

We can have "subsystem feature branches", which you allude to at the
very end of your message, that can be created by subsystem maintainers:

  subsystem/scheduler/FOO

These can be a central point of development for disruptive new features.
I think it's an important third category of branch that will enable
collaborative development on big projects.

We can allow merge commits to be submitted by subsystem maintainers so
that they can propose a merge of subsystem/scheduler/FOO to
subsystem/scheduler/next, and a merge of subsystem/scheduler/next to
master.

Gerrit handles merge commit proposals just fine -- we disabled them
because early testers had a tendency to make merge commits locally that
pulled in hundreds of commits gerrit didn't know about (which caused
them all to go up for review).  git-review helps avoid that error now,
and we can give very good instructions to the small set of people who
would be doing this.

>   - Project dictators don't need to do detailed reviews of merge 
>     proposals from subsystem maintainers. The dictator's role is mostly 
>     just to sign off on the merge proposal. However, the dictator can 
>     comment in the proposal on things which could have been done better 
>     and the subsystem maintainer should take note of these comments and 
>     perhaps retroactively fix them up. Ultimately, though, the dictator 
>     can have exercise a veto if the merge proposal is unacceptable or 
>     if the subsystem maintainer is consistently making the same 
>     mistakes.
>
>   - It would be up to the project dictators to help drive patches 
>     through the right subsystem branches - e.g. they might object if 
>     one subsystem maintainer merged a patch that inappropriately cut 
>     into another subsystem or they might refuse to merge a given patch
>     into the main branch unless it went through the appropriate 
>     subsystem branch.
>
>     (In gerrit, this would mean a small team or single person who can 
>     +1 approve merge proposals on master. They would -1 proposals
>     submitted against master which should have been submitted against a 
>     subsystem branch.)
>
>   - Subsystem branches might not necessarily be blessed centrally. It 
>     might be a case that anyone can create such a branch and, over 
>     time, establish trust with the project dictators. Subsystem 
>     branches would come and go. This is the mechanism by which 
>     subsystem maintainership is transferred between people over time.
>
>     (In gerrit, this means people need to easily be able to create 
>     their own branches)

We could allow people to create their own branches, but what's the goal?
If the idea of creating subsystem branches is to get the right people
reviewing them, then letting anyone create a subsystem branch means that
reviewers have to go seek out new branches.  I think that subsystem
branches _need_ to be blessed centrally and only created under the
auspices of the PTL/core team so that the right reviewers are assigned
to the task.

>     (What's more difficult to imagine in gerrit is how a new, potential 
>     subsystem maintainer comes along, starts hoovering up patches into 
>     her branch and submitting them in batches. Where does she hoover 
>     them up from and how does she say "I've merged this into my branch, 
>     don't merge it via another branch")

I think it should be top-down, not bottom up, to maintain the usefulness
of the review system.

>   - Bisectability remains important. Subsystem maintainers don't merge 
>     broken commits into their subsystem branch and the project 
>     dictators can enforce this using their veto. It is not good enough 
>     for subsystem maintainers to consistently merge broken commits into 
>     their branch, fix it up with a later commit and include both 
>     commits their merge proposals.
>
>     (I don't think we'd use Jenkins to enforce this, but subsystem 
>     maintainers might use it as a tool to help them catch issues. So, 
>     the full set of gating tests would only gate merges into master but 
>     subsystem branches might choose to gate merges into their branch on 
>     the unit tests. Subsystem maintainers might also use Smokestack to 
>     pre-gate merge proposals to the subsystem branch)

If it's not enforced, broken commits will happen.  I think we can gate
subsystem branches identically to master.  I'm not opposed to making
them advisory or excluding them entirely (since master will still be
gated).  We also can have some but not all gating (or advisory) jobs for
subsystem branches.

Basically, I think we have a choice: maintain as much bisectability as
possible with gating, or relax gating at the cost of universal
bisectability.  I think for subsystem feature branches to be really
useful, we will need to relax gating there, and I think bisectability is
an ideal to which we need not aspire.

My recommendation would be:

Gate master as we do now.
Gate subsystem branches as we do master.
Gate subsystem feature branches with unit and pep8 tests (even if you're
  working on a big feature, these shouldn't fail, right? :)
Run other tests in an advisory mode on subsystem feature branches.

But which tests and how they are run is a continuum and we can adjust.

>   - Subsystem branches would not rebase unless the project dictator 
>     outright rejects a merge request from the subsystem branch (i.e.
>     "I'm not merging commit abcdef0! Fix it and rebase!"). This means 
>     the subsystem maintainer will need to regularly (probably only when 
>     there are conflicts to be dealt with) merge master back into the
>     subsystem branch.

As pointed out elsewhere, that makes a complicated but perfectly
workable git graph.  If changes line up neatly at subsystem boundaries,
there shouldn't be much need for merging master back into the branch
very often.  I don't anticipate a problem with merging master back into
the subsystem branches as frequently as needed.

>   - Plausible subsystem branches are e.g.:
>
>       - OpenStack APIs
>       - EC2 API
>       - virt
>          - libvirt driver
>          - xenapi driver
>          - vmware driver
>       - networking
>       - volumes
>       - scheduler
>       - RPC
>
>     Deciding which areas make sense as a subsystem branch is 
>     non-trivial.
>
>     Should there be a "DB" subsystem? Probably not, because that would 
>     mean every new feature needs to come through this branch or, 
>     alternatively, the DB maintainer would need to accept DB schema 
>     additions without the context of how it's being used higher up the 
>     stack.
>
>     Ok, so why does it make sense to have an "OpenStack APIs" 
>     subsystem? Don't all features affect that branch too? Well, maybe, 
>     but the APIs really do need strong oversight. Perhaps we can be 
>     confident that we can add e.g. a new scheduler feature through the
>     scheduler branch and then later merge any API additions through the 
>     APIs branch.
>
> And how about feature branches?
>
>   - Feature branches are relatively short-lived (i.e. weeks or months
>     rather than years) branches for a specific feature. They are a
>     mechanism for developers to work on a patch series in the open until
>     the feature is complete enough to be merged into a subsystem branch
>     or master.
>
>     (I'm not sure gerrit is right for this. Why not just do it in 
>     folk's github forks? I think all people are looking for is for 
>     people to be more aware of feature branches. How about if you put 
>     details of your feature branch in the blueprint for the feature?)
>
>     (If not using gerrit, can developers configure Jenkins to CI their 
>     branch? Or is Smokestack the right tool?)

Are they collaborative?  Perhaps if your feature is large enough to have
other people collaborating on it, you should ask the subsystem
maintainer to start a subsystem feature branch for it.

If they are not collaborative, then perhaps we, as a project, don't need
to have an opinion on how they are done.

Keep in mind the new gerrit "draft change" feature, which lets someone
push a change to gerrit and get early feedback on it, and that feedback
stays with the change as it evolves.

Our main goals are to enable adequate code review and testing of changes
going into the project.  The closer a change gets to being merged, the
more we care about that; conversely, we're not as interested in how a
change starts its life.

If a developer writes a patch locally and submits it for review, that's
cool.  If he writes it (or a series of patches) and puts it on github,
that's fine.  If he emails the diff to some colleagues or a mailing list
to ask their opinion, great.

Perhaps as it gets closer to being ready for inclusion, he'll upload it
as a draft to gerrit to get feedback from potential future reviewers,
and that feedback will live with the patch.

Then it gets proposed formally for review (and we know how that works).

There's one thing I explicitly don't want to encourage, and that's
making a github fork, merging changes from other people, _squashing_
them, and proposing that as a single change which is missing important
authorship information.  We need to make sure we have the right
project-level tools to handle that kind of collaboration.  I think
subsystem feature branches fills that need.

I also don't want to push people to a system where the review of a
change happens outside of the code review system for the project.  When
a change or series of changes is ready to receive code review, we should
start using the code review tool -- otherwise, it will make it harder
for the right reviewers to find and review the right changes.

>   - Feature branches rebase, do not contain merge commits and each 
>     commit on the branch is functional, bisectable and self-contained.
>
>   - When a feature branch is ready to be merged into a subsystem 
>     branch, the patch series is submitted for review. The subsystem 
>     maintainer will likely require changes to individual patches and 
>     these changes would be made on the feature branch and squashed back 
>     into the appropriate individual patch.
>
>     (Ideally gerrit's "topic review" feature will get upstream and 
>     we'll use that. This would mean that a patch series could be 
>     proposed for review as a single logical unit while still keeping 
>     individual patches as separate commits)
>
>   - Because feature branches rebase, active day-to-day collaboration
>     with others is difficult. You certainly can't have multiple people
>     rebasing the same branch, that way lies madness.
>
>     There are ways to have multiple people work actively on the same 
>     rebasing branch e.g.
>
>       http://blogs.gnome.org/markmc/2011/02/26/git-rebasing-cont/
>
>     but, ultimately, feature branches are going to be owned by a single 
>     person who might incorporate patches from others.
>
>     (Incorporating the work of others, rebasing and squashing means a 
>     patch might have multiple contributors but only one author listed in
>     git. That makes CLA enforcement impossible, but we should drop the 
>     CLA in favour of the kernel-like Signed-off-by: tag. See this 
>     discussion: https://lists.launchpad.net/openstack/msg06544.html )

I strongly agree, but we can't do that unilaterally.  I think the best
approach is to wait until the foundation is formed and responsible for
the CLA, and then talk to the Foundation's counsel about changing that.

At this point we have CLA/authorship vagueness combined with rebasing
complexity teaming up to make this approach undesirable as something we
advocate.

>   - One option for longer-lived, active collaboration is for a subsystem
>     maintainer to create a feature branch and review the work as it is 
>     ongoing. The idea being that the subsystem maintainer commits to 
>     not requiring the feature branch to be rebased before it is merged 
>     into the subsystem branch.

I like this idea and addressed it up in the subsystem section.

---------------------------------------------------------------------
Summary of my interpretation/recommendations:

Implement subsystem branches largely as described by Mark.  
  - eg: /refs/heads/subsystem/scheduler/next
  - There should be a subsystem review group that can CRVW+2 and APRV+1
    changes to that branch.
  - Changes to that branch should be gated just as master.
  - Subsystem maintainers can propose merge commits to master.
Implement subsystem feature branches basically as described by Mark:
  - eg: /refs/heads/subsystem/scheduler/FOO
  - Can be created/deleted by respective subsystem maintainers.
  - Changes to these branches should be gated on a subset of tests.
    - This will end global bisectability of the project on any tests
      that are not gating these branches.
Essentially do nothing regarding individual feature branches:
  - Bring to people's attention the use of Draft Reviews for beginning
    the review process early.
  - Otherwise, how individual development is performed or exhibited is
    out of scope.

-Jim