Open Stack

Hey,

Hi On Mon, 2012-05-14 at 14:51 +0200, Thierry Carrez wrote:
> James E. Blair wrote:
> > Vish, Thierry, and I spent some time together this week at UDS trying to
> > reconcile their needs and your suggestions.  I believe Thierry is going
> > to write that up and send it to the list soon.
> 
> While at UDS we took some time to discuss a subsystem branch models that
> would work for Vish (PTL/developer), Jim (CI/infra) and me (release
> time). We investigated various models and came up with "Vish's
> supercombo subsystem model" (see model 5 at the bottom of
> http://wiki.openstack.org/SubsystemsBranchModel for a graph).

It's great you guys got to dig into this in detail, see inline for
thoughts

> In this model:
> 
> * You have a "master" branch which contains release-ready features and
> bugfixes. Milestones are cut directly from it.
> 
> * Subsystem branches with area experts are used wherever possible. The
> subsystem maintainer should maintain this branch so that it can directly
> be merged into master when "ready" (all or nothing). Subsystem
> maintainers are allowed to propose a merge commit to master.

Yep, except I still think having the subsystem maintainer create and
propose a merge commits will be a scaling issue in this process.

To walk through an example, you've got some work on a subsystem branch:

      o--o--o  <- sub
     /
 o--o--o--o--o--o  <- master

The subsystem maintainer is ready for this to be merged into master, so
creates a merge commit:

      o--o--o-----o  <- sub
     /           /
 o--o--o--o--o--o  <- master

and proposes that. However, there's another subsystem branch merge
already queued which gets merged first. This merge isn't a fast-forward,
but Gerrit is able to do it since there is no merge conflicts:

      o--o--o-----o  <- sub
     /           /
 o--o--o--o--o--o--o  <- master
  \        \      /
   o--o--o--o----+  <- sub2

Now, the "sub" merge commit proposal is accepted but fails to merge
because of merge conflicts. The maintainer creates another merge and
proposes that:

      o--o--o-----o--o  <- sub
     /           /  /
 o--o--o--o--o--o--o  <- master
  \        \      /
   o--o--o--o----+  <- sub2

and that gets approved quickly and merged as a fast-forward:

      o--o--o-----o-+  
     /           /   \
 o--o--o--o--o--o--o--o  <- sub  <- master
  \        \      /
   o--o--o--o----+  <- sub2


The alternative is that the maintainer of the master branch is creating
the merge commits and resolving conflicts (like Linus):

      o--o--o  <- sub
     /
 o--o--o--o--o--o  <- master
  \
   o--o--o  <- sub2

The two subsystem maintainers send their pull requests and the master
maintainer merges them in the same order:

      o--o--o-------+  <- sub
     /               \
 o--o--o--o--o--o--o--o  <- master
  \               /
   o--o--o-------+  <- sub2

If you compare to the result when gerrit is merging, we have two less
merge commits with this model and one less subsystem maintainer
roundtrip.

> * Bugfixes get proposed directly to master

I think bugfixes relevant to a particular subsystem need to go through
master, since that's where the domain experts are doing reviews.

> * Features can be directly proposed to master, although they should be
> redirected to a subsystem branch when one exists for that area
> 
> * Only features that are release-ready should be accepted into master.
> Final approval of merges on master will therefore be limited to the
> corresponding project release team.

Woah, I didn't see that one coming :-)

Deciding what is ready for merging is the project leader's job IMHO.

> * Milestones are directly cut from master. A couple of days before each
> milestone, we will have a soft freeze during which only bugfixes are merged
> 
> * Between the last milestone and RC1, a soft freeze is enforced during
> which only bugfixes are merged (can last for a couple weeks)
> 
> * In order to limit the master freeze, at RC1 and until release, a
> specific release branch is cut from master. That specific release branch
> directly gets release-critical (targeted) bugfixes, and gets merged back
> into master periodically.

This release branch thing (to avoid cherry-picking) is probably workable
since the time between RC1 and release is relatively short, but I think
we'll want to eventually to get to a point where the subsystem branches
have alleviate much of the pain involved with just locking down master
between the last milestone and release.

> Benefits of this model:
> * We enable subsystems, which for larger projects let people specialize
> on an area of the code and avoids having to be an expert in all things
> * Subsystems become a staging ground for features that are not release-ready

This sets off loud alarm bells for me.

A subsystem maintainer should never merge anything which could not be
merged into master without more work. Without that rule, the subsystem
branch gets blocked from merging into master until a feature developer
finishes their work.

For sure, preparation work for features (or incremental steps as part of
feature development) can be merged into a subsystem branch before the
feature is finished. And there should be no issue with the subsystem
maintainer proposing that for merging into master.

As an example, "replace integer IDs with UUIDs" is a feature that took
many iterative steps. It would be crazy for a subsystem maintainer to
merge a first step of that work into a subsystem if it couldn't be
merged into master until the rest of the steps were finished.

...

Generally I'm fairly unexcited at the prospect of adopting this model.
It feels complex, but without really effectively solving the issues
we're seeing.

If I was to pick one change I'd like to see, it would be to have the
project leader directly responsible for merging subsystem pull requests
and handling conflicts.

Yes, it might be hard to see how the already overburdened Vish could do
that, but if we first do the thought experiment of what the end result
would look like I think we'd be really eager to make it work. And I'm
actually really confident we could do it while lightening the load on
Vish.

Cheers,
Mark.