Open Stack

On Mon, Nov 18, 2013 at 3:53 PM, Mark McLoughlin <markmc at redhat.com> wrote:
> On Mon, 2013-11-18 at 17:24 +0000, Duncan Thomas wrote:
>> Random OSLO updates with no list of what changed, what got fixed etc
>> are unlikely to get review attention - doing such a review is
>> extremely difficult. I was -2ing them and asking for more info, but
>> they keep popping up. I'm really not sure what the best way of
>> updating from OSLO is, but this isn't it.
>
> Best practice is to include a list of changes being synced, for example:
>
>   https://review.openstack.org/54660
>
> Every so often, we throw around ideas for automating the generation of
> this changes list - e.g. cinder would have the oslo-incubator commit ID
> for each module stored in a file in git to tell us when it was last
> synced.
>
> Mark.
>
>
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Been away on vacation so I'm afraid I'm a bit late on this... but;

I think the point Duncan is bringing up here is that there are some
VERY large and significant patches coming from OSLO pulls.  The DB
patch in particular being over 1K lines of code to a critical portion
of the code is a bit unnerving to try and do a review on.  I realize
that there's a level of trust that goes with the work that's done in
OSLO and synchronizing those changes across the projects, but I think
a few key concerns here are:

1. Doing huge pulls from OSLO like the DB patch here are nearly
impossible to thoroughly review and test.  Over time we learn a lot
about real usage scenarios and the database and tweak things as we go,
so seeing a patch set like this show up is always a bit unnerving and
frankly nobody is overly excited to review it.

2. Given a certain level of *trust* for the work that folks do on the
OSLO side in submitting these patches and new additions, I think some
of the responsibility on the review of the code falls on the OSLO
team.  That being said there is still the issue of how these changes
will impact projects *other* than Nova which I think is sometimes
neglected.  There have been a number of OSLO synchs pushed to Cinder
that fail gating jobs, some get fixed, some get abandoned, but in
either case it shows that there wasn't any testing done with projects
other than Nova (PLEASE note, I'm not referring to this particular
round of patches or calling any patch set out, just stating a
historical fact).

3. We need better documentation in commit messages explaining why the
changes are necessary and what they do for us.  I'm sorry but in my
opinion the answer "it's the latest in OSLO and Nova already has it"
is not enough of an answer in my opinion.  The patches mentioned in
this thread in my opinion met the minimum requirements because they at
least reference the OSLO commit which is great.  In addition I'd like
to see something to address any discovered issues or testing done with
the specific projects these changes are being synced to.

I'm in no way saying I don't want Cinder to play nice with the common
code or to get in line with the way other projects do things but I am
saying that I think we have a ways to go in terms of better
communication here and in terms of OSLO code actually keeping in mind
the entire OpenStack eco-system as opposed to just changes that were
needed/updated in Nova.  Cinder in particular went through some pretty
massive DB re-factoring and changes during Havana and there was a lot
of really good work there but it didn't come without a cost and the
benefits were examined and weighed pretty heavily.  I also think that
some times the indirection introduced by adding some of the
openstack.common code is unnecessary and in some cases makes things
more difficult than they should be.

I'm just not sure that we always do a very good ROI investigation or
risk assessment on changes, and that opinion applies to ALL changes in
OpenStack projects, not OSLO specific or anything else.

All of that being said, a couple of those syncs on the list are
outdated.  We should start by doing a fresh pull for these and if
possible add some better documentation in the commit messages as to
the justification for the patches that would be great.  We can take a
closer look at the changes and the history behind them and try to get
some review progress made here.  Mark mentioned some good ideas
regarding capturing commit ID's from synchronization pulls and I'd
like to look into that a bit as well.

Thanks,
John