openstack team mailing list archive

[Mark McLoughlin <markmc@xxxxxxxxxx>]

Hi James,

On Tue, 2012-05-08 at 14:03 -0700, James E. Blair wrote:
> 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

Subsystem repos probably make more sense than subsystem branches. A mass
of branches in the one repo isn't ideal, you should be able to choose
which subsystems you're fetching from.

So, that would be:

  openstack/nova/scheduler.git

But another option worth keeping an open mind about is:

  sandywalsh/nova/scheduler.git

In the kernel model, it's all about the subsystem maintainer rather than
the subsystem. 

e.g. rather than the PTL take on the responsibility of deciding what the
subsystems are and who the subsystem maintainers, you allow people to
step up and prove themselves in that role. The PTL decides on who the
subsystem maintainers are simply by deciding who to pull from.

Or put it another way, if the PTL anoints someone as a subsystem
maintainer and finds that maintainer is pretty much AWOL, then we have
to go through this unnecessarily painful process of closing down the
subsystem branch or finding a new maintainer. I see parallels with what
happened with the Nova sub-teams formed at the Essex summit.

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

Hmm, I don't think you want to confuse the issue thinking about "next"
branches yet. We need subsystem branches for what's going into *this*
release before worrying about branches for the next release.

We could well have an integration thing similar to linux-next, but not
just for "next" branches.

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

Something like that could work, but I'd leave it until the next
iteration.

The kernel gets by pretty well without any real idea of a collaborative
rebasing feature branch, so I don't see it as an immediate item to
tackle.

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

Ok, say we have:

  sandywalsh/nova/scheduler.git:master

and:

  openstack/nova.git:master

And it looks like:

  o--o--o--o  <-- nova.git:master
      \
       o--o--o--..--o--o <-- scheduler.git:master
           \       /
            o--o--o

What happens in gerrit when Sandy proposes the tip of his master branch
for merging into nova.git:master ?

Ideally what you want is for the nova.git maintainer to just see this as
a pull-request, see all tree of commits it includes, be able to poke
through those commits, but just quickly say "yep, that looks good to
merge"

I think what you're saying is that Sandy would need to create this merge
commit:

  o--o--o--o  <-- nova.git:master
      \     \------------\
       o--o--o--..--o--o--o <-- scheduler.git:master
           \       /
            o--o--o

and propose that?

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

It's all about who the PTL is willing to pull from. The PTL shouldn't
have to decide that ahead of time, he should decide that based on what
he's being asked to pull and how thoroughly it has been reviewed etc.

e.g. say Sandy stepped up and decided he wanted to start a scheduler
subsystem tree. IMHO, without any bureaucracy, he should be able to
create that tree, take patches, ask people to help him with reviews and
propose that. At that point, Vish would be in a position to judge
whether he's going to pull from Sandy.

Now maybe not everyone can create their own repos, maybe you need core
status or something.

> >     (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.

Don't follow.

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

Yeah, that works.

Now, when you say "gate master", the gating tests would only be applied
to the merge commit into master, right?

So, if new tests were added between the time stuff was merged into the
subsystem branch and the subsystem branch was merged into master, you
wouldn't have to rebase the subsystem branch to get it to pass?

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

Ignoring subsystem-maintainer controlled feature branches for now. As I
said, I think they can wait.

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

Yep, with the most stringent tests applied to the merge commit into
master.

> >   - 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? 

No, to me, the definition of a feature branch is that it is rebasing. It
is a series of commits (no merges) which has not yet transitioned out of
the rebasing phase.

It's very hard to collaborate effectively on a rebasing branch.

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

The subsystem vs feature branch distinction is really a rebasing vs
non-rebasing branch distinction.

This would be a non-rebasing branch, which anyone can submit changes to
and will eventually be proposed to a subsystem branch or directly to
master.

The key thing is that the commits will never be rebased and they should
be bisectable. People may well want to stage stuff in a rebasing branch
(for feedback from others etc.) before pushing it to the non-rebasing
branch.

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

You as the maintainer of our gerrit system may not care, but I care that
we have a culture of baking our feature work to a level of completeness
on a rebasing branch before transitioning it to a non-rebasing tree that
ends up feeding into master.

Working on features using non-rebasing branches can be as much a part of
our development process as blueprints, whether or not gerrit is involved
in this part.

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

Yeah, I was initially fairly hopeful of this, but the fact that it's
private and you explicitly have to add people as reviewers is pretty
sucky. Also, it's not great for patch series.

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

I agree with that.

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

Yep. But similarly, we don't really force the blueprint process on
people but it's a strong part of our feature development culture.
Re-basing feature branches on github could similarly be a part of it,
without it needing to be enforced.

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

I mostly agree with that, but am also concious that your concern may be
largely about CLA enforcement. I think our CLA enforcement sucks, I'd
prefer us to move to using Signed-off-by.

There is nothing wrong with you having a rebasing feature branch,
someone sending you a fix for a serious issue in patch 5 of 8 and you
squashing the fix into patch 5.

That's captured perfectly fine in the kernel development process using
Signed-off-by

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

Yes, non-rebasing branches are needed for collaboration.

But, conversely ... that means you need to commit to not rebasing in
order to collaborate. You can't collaborate on a half-baked,
non-bisectable series of patches.

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

You talked about not caring where development is done before it is
collaborative (i.e. before moving to the non-rebasing stage).

Same goes for code review. Code review is useful at all stages of
development, so it's not like we're trying to mandate code review can't
happen outside gerrit.

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

Agree on unilaterally. I'm concerned that the CLA requirement is encoded
in the Foundation by-laws draft and would require a super-majority to
change as things stand. I think that's overkill and would be happy for
the Foundation to be formed with the current CLA policy so long as it
only requires a vote of the board of directors to change.

Richard Fontana (Red Hat's counsel on the drafting committee) is very
much in favour of us switching to Signed-off-by:

https://lists.launchpad.net/openstack/msg06544.html

But anyone else who feels strongly about this should be sure to advocate
for it themselves too.

Anyway, slight tangent ... :-)

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

Yeah, I'm not advocating collaboration on re-basing branches.

OTOH, you can't stop people squashing patches or collaborating on
patches so let's not get too hung up on the CLA enforcement thing.

As an example, Russell Bryant and William Henry wrote the qpid support
together. Russell was listed in Author:, William was added to Authors
and he also signed the CLA. He could easily have not done any of that.

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

Cool, right - again, it's the distinction between rebasing and
non-rebasing branches that's important.

> ---------------------------------------------------------------------
> Summary of my interpretation/recommendations:
> 
> Implement subsystem branches largely as described by Mark.  
>   - eg: /refs/heads/subsystem/scheduler/next

Would like to avoid that clutter in the main repo, so separate repos for
subsystems.

Also this isn't, at least initially, about "next" branches - e.g. a
scheduler subsystem branch would be accepting patches for Folsom, right
up until the Folsom release.

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

That works.

>   - Subsystem maintainers can propose merge commits to master.

And then Jenkins creates another merge commit to merge that merge commit
into master? Uggh.

I've a whole other topic here that I want to get into, but this email is
huge already so I'll follow up on it.

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

I'd give subsystem maintainers their own repos, allow them to create
non-rebasing branches with gating applied and then experiment with
different approaches - i.e. subsystem master branch, a "next" or
"folsom" branch, and collaborative, non-rebasing feature branches.

Then subsystem maintainers can propose any of these branches to be
merged into any branches of the main repo and we have nova-core apply an
evolving policy around approving those proposals.

> Essentially do nothing regarding individual feature branches:
>   - Bring to people's attention the use of Draft Reviews for beginning
>     the review process early.

While they remain private apart from a select few reviewers explicitly
added, I don't think they help.

Also, rebasing feature branches should be relatively series of patches
which gerrit isn't great for.

>   - Otherwise, how individual development is performed or exhibited is
>     out of scope.

Out of scope for gerrit, but not out of scope for our development
process.

Again, I point to Dan Berrange's excellent example of how this should be
done:

  1) A rebasing patch set pushed to github

       https://github.com/berrange/nova/tree/libvirt-xml-config-v1

  2) A blueprint and wiki page:

       https://blueprints.launchpad.net/nova/+spec/libvirt-xml-config-apis
       http://wiki.openstack.org/LibvirtXMLConfigAPIs

  3) An RFC sent to the mailing list, soliciting broader discussion 
     than just straightforward code review:

       https://lists.launchpad.net/openstack/msg08408.html

  4) The patches proposed and reviewed in gerrit once they were ready 
     to transition out of the rebasing phase:

       https://review.openstack.org/#/q/topic:bp/libvirt-xml-config-apis+status:merged,n,z

I think the RFC email with the WIP branch in github is an excellent
addition to our blueprint+gerrit feature development process.

Cheers,
Mark.