[openstack-dev] [cinder] L3 low pri review queue starvation

Gorka Eguileor geguileo at redhat.com
Wed Sep 2 09:19:02 UTC 2015


On Tue, Sep 01, 2015 at 09:30:26AM -0600, John Griffith wrote:
> On Tue, Sep 1, 2015 at 5:57 AM, Tom Barron <tpb at dyncloud.net> wrote:
> 
> > [Yesterday while discussing the following issue on IRC, jgriffith
> > suggested that I post to the dev list in preparation for a discussion in
> > Wednesday's cinder meeting.]
> >
> > Please take a look at the 10 "Low" priority reviews in the cinder
> > Liberty 3 etherpad that were punted to Mitaka yesterday. [1]
> >
> > Six of these *never* [2] received a vote from a core reviewer. With the
> > exception of the first in the list, which has 35 patch sets, none of the
> > others received a vote before Friday, August 28.  Of these, none had
> > more than -1s on minor issues, and these have been remedied.
> >
> > Review https://review.openstack.org/#/c/213855 "Implement
> > manage/unmanage snapshot in Pure drivers" is a great example:
> >
> >    * approved blueprint for a valuable feature
> >    * pristine code
> >    * passes CI and Jenkins (and by the deadline)
> >    * never reviewed
> >
> > We have 11 core reviewers, all of whom were very busy doing reviews
> > during L3, but evidently this set of reviews didn't really have much
> > chance of making it.  This looks like a classic case where the
> > individually rational priority decisions of each core reviewer
> > collectively resulted in starving the Low Priority review queue.
> >

I can't speak for other cores, but in my case reviewing was mostly not
based on my own priorities, I reviewed patches based on the already set
priority of each patch as well as patches that I was already
reviewing.

Some of those medium priority patches took me a lot of time to review,
since they were not trivial (some needed some serious rework).  As for
patches I was already reviewing, as you can imagine it wouldn't be fair
to just ignore a patch that I've been reviewing for some time just when
it's almost ready and the deadline is closing in.

Having said that I have to agree that those patches didn't have much
chances, and I apologize for my part on that.  While it is no excuse I
have to agree with jgriffith when he says that those patches should have
pushed cores for reviews (even if this is clearly not the "right" way to
manage it).

> > One way to remedy would be for the 11 core reviewers to devote a day or
> > two to cleaning up this backlog of 10 outstanding reviews rather than
> > punting all of them out to Mitaka.
> >
> > Thanks for your time and consideration.
> >
> > Respectfully,
> >
> > -- Tom Barron
> >
> > [1] https://etherpad.openstack.org/p/cinder-liberty-3-reviews
> > [2] At the risk of stating the obvious, in this count I ignore purely
> > procedural votes such as the final -2.
> >
> > __________________________________________________________________________
> > 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
> >
> 
> ​Thanks Tom, this is sadly an ongoing problem every release.  I think we
> have a number of things we can talk about at the summit to try and
> make some of this better.  I honestly think that if people were to
> actually "use" launchpad instead of creating tracking etherpads
> everywhere it would help.  What I mean is that there is a ranked
> targeting of items in Launchpad and we should use it, core team
> members should know that as the source of truth and things that must
> get reviewed.
> 

I agree, we should use Launchpad's functionality to track BPs and Bugs
targeted for each milestone, and maybe we can discuss on a workflow that
helps us reduce starvation at the same time that helps us keep track of
code reviewers responsible for each item.

Just spitballing here, but we could add to BP's work items and Bug's
comments what core members will be responsible for reviewing related
patches.  Although this means cores will have to check this on every
review they do that has a BP or Bug number, so if there are already 2
cores responsible for that feature they should preferably just move on
to other patches and if there are not 2 core reviewers they should add
themselves to LP.  This way patch owners know who they should bug for
reviews on their patches and if there are no core reviewers for them
they should look for some (or wait for them to "claim" that
bug/feature).

> As far as Liberty and your patches; Yesterday was the freeze point, the
> entire Cinder team agreed on that (yourself included both at the mid-cycle
> meet up and at the team meeting two weeks ago when Thingee reiterated the
> deadlines).  If you noticed last week that your patches weren't going
> anywhere YOU should've wrangled up some reviews.
> 
> Furthermore, I've explained every release for the last 3 or 4 years that
> there's no silver bullet, no magic process when it come to review
> throughput.  ESPECIALLY when it comes to the 3'rd milestone.  You can try
> landing strips, priority listed etherpads, sponsors etc etc but the fact is
> that things happen, the gate slows down (or we completely break on the
> Cinder side like we did yesterday).  This doesn't mean "oh, well then you
> get another day or two", it means stuff happens and it sucks but first
> course of action is drop low priority items.  It just means if you really
> wanted it you probably should've made it happen earlier.  Just so you know,
> I run into this every release as well.  I had a number of things in
> progress that I had hoped to finish last week and yesterday, BUT my
> priority shifted to trying to help get the cinder patches back on track and
> get the items in Launchpad updated to actually reflect something that was
> somewhat possible.
> 
> The only thing that works is "submit early and review often" it's simple.
> 

While this is true, sometimes it's not a matter of submitting early,
because some patches just keep getting ignored and when deadline closes
in low priority patches will always have higher priority patches that
will go before them, unless we set a workflow that will give them a
better chance of getting reviews.

> Finally, I pointed out to you yesterday that we could certainly discuss as
> a team what to do with your patches.  BUT that given how terribly far
> behind we were in the process that I wanted reviewers to focus on medium,
> high and critical prioritized items.  That's what prioritization's are for,
> it means when crunch time hits and things hit the fan it's usually the
> "low" priority things that get bumped.
> 
> Thanks,

I am not against discussing this, but in all fairness we should discuss
*everybody's* patches that were targeted for L3, not just tbarron's.

Cheers,
Gorka.



More information about the OpenStack-dev mailing list