[openstack-dev] [nova] An analysis of code review in Nova
hartsock at acm.org
Sat Mar 22 23:33:39 UTC 2014
On Sat, Mar 22, 2014 at 11:55 AM, Matt Riedemann
<mriedem at linux.vnet.ibm.com> wrote:
> On 3/22/2014 5:19 AM, Shawn Hartsock wrote:
>> On Fri, Mar 14, 2014 at 6:58 PM, Dan Smith <dms at danplanet.com> wrote:
>>> Review latency will be directly affected by how good the refactoring
>>> changes are staged. If they are small, on-topic and easy to validate,
>>> they will go quickly. They should be linearized unless there are some
>>> places where multiple sequences of changes make sense (i.e. refactoring
>>> a single file that results in no changes required to others).
>> I'm going to bring this to the next
>> https://wiki.openstack.org/wiki/Meetings/VMwareAPI we can start
>> working on how we'll set the order for this kind of work. Currently we
>> have a whole blueprint for refactoring a single method. That seems
>> silly. I'll want to come up with a plan around how to restructure the
>> driver so we can avoid some of the messes we've seen in the past.
> I think the point of starting with refactoring the nested method mess in the
> spawn method was it (1) seemed relatively trivial (think fast review
> turnaround) and (2) should be a good "bang for the buck" kind of change, as
> a lot of the original complaint was related to how hard it is to verify
> changes in the giant spawn method are tested - which you also point out
>> I want to avoid one big refactor effort that drags on, but I also want
>> to address bigger problems we have inside the driver. For example,
> I also want to avoid a big refactor effort dragging on, and I like the
> thinking on design changes, but are they doing going to be happening at the
> same time? Or is the complete re-design going to supersede the refactoring?
> My only concern is biting off more than can be chewed in juno-1.
> Plus there is the refactor to use oslo.vmware, where does that fit into
>> vm_util.py seems to have become burdened with work that it shouldn't
>> have. It also performs a great number of unnecessary round trips using
>> a vm_ref to pull individual vm details over one at a time. Introducing
>> a VirtualMachine object that held all these references would simplify
>> some operations (I'm not the first person to suggest this and it
>> wasn't novel to me either when it was presented.)
>> It would seem Juno-1 would be the time to make these changes and we
>> need to serialize this work to keep reviewers from losing their
>> marbles trying to track it all. I would like to work out a plan for
>> this in conjunction with interested core-reviewers who would be
>> willing to more or less sponsor this work. Because of the problems
>> Matt points out, I don't want to tackle this in a haphazard or
>> piece-meal way since it could completely disrupt any new blueprint
>> work people may have targeted for Juno.
> Yeah, definitely need a plan here. I'd like to see things prioritized based
> on what can be fixed in a relatively isolated way which gives a good return
> on coding/reviewing investment, e.g. pulling those nested methods out of
> spawn so they can be unit tested individually with mock rather than a large,
> seemingly rigid and scaffolded test framework.
Just because you said something about it :-)
I wrote this up off-the-cuff:
This is about the size of each of these refactors that I'm thinking
of. I don't want to get much bigger than this and I don't want to
force a round-trip up to oslo.vmware and back to Nova for this size of
change. I don't really think we have to table the inclusion of
oslo.vmware integration in Juno in order to do this kind of work.
However, just as we pointed out at the top of the thread, there's a
danger that this kind of work can stall new feature adds so the timing
and inclusion of it is sensitive.
>> Having said that, on this driver, new blueprints in the last several
>> cycles have introduced serious feature regressions. Several minor bug
>> fixes have altered and introduced key architectural components that
>> have broken multiple critical features. In my professional opinion
>> this has a root cause based on the drivers tightly coupled and
>> non-cohesive internal design.
>> The driver design is tightly coupled in that a change in one area
>> forces multiple updates and changes *throughout* the rest of the
>> driver. This is true in testing as well. The testing design often
>> requires you to trace the entire codebase if you add a single optional
>> parameter to a single method. This does not have to be true.
> Yup, this is my major complaint and ties into what I'm saying above, I find
> it really difficult to determine most of the time where a change is tested.
> Because of the nature of the driver code and my lack of actually writing
> features in it, as a reviewer I don't know if a change in X is going to
> break Y, so I rely on solid test coverage and the testing needs to be more
> natural to follow than it currently is.
>> The driver design is non-cohesive in that important details and
>> related information is spread throughout the driver. You must be aware
>> at all times (for example) whether or not your current operation
>> requires you to check if your vm_ref is outdated (we just worked on
>> several last minute critical bugs for RC1 where myself and others
>> pulled all nighters to fix the issue in a bad case of Heroic
>> I would like to stop the http://c2.com/cgi/wiki?CodeVomit please. May we?
> I know this isn't going to be easy so I'm really glad you're planning on
> tackling it in Juno. I'll tentatively sign up to help sponsor this but I'm
> not going to be able to commit all of my review bandwidth to a ton of
> changes for refactor and re-design. Hopefully targets will become more
> clear as the team gets the plans in place.
> Matt Riedemann
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
# Shawn.Hartsock - twitter: @hartsock - plus.google.com/+ShawnHartsock
More information about the OpenStack-dev