Open Stack

On 3/12/2014 7:29 PM, Arnaud Legendre wrote:
> Hi Matt,
>
> I totally agree with you and actually we have been discussing this a lot internally the last few weeks.
> . As a top priority, the driver MUST integrate with oslo.vmware. This will be achieved through this chain of patches [1]. We want these patches to be merged before other things.
> I think we should stop introducing more complexity which makes the task of refactoring more and more complicated. The integration with oslo.vmware is not a refactoring but should be seen as a way to get a more "lightweight" version of the driver which will make the task of refactoring a bit easier.
> . Then, we want to actually refactor, we got several meetings to know what is the best strategy to adopt going forward (and avoid reproducing the same mistakes).
> The highest priority is spawn(): we need to make it modular, remove nested methods. This refactoring work should include the integration with the image handler framework [2] and introducing the notion of image type object to avoid all these conditions on types of images inside the core logic.

Breaking up the spawn method to make it modular and thus testable or 
refactoring to use oslo.vmware, order there doesn't seem to really 
matter to me since both sound good.  But this scares me:

"This refactoring work should include the integration with the image 
handler framework"

Hopefully the refactoring being talked about here with oslo.vmware and 
breaking spawn into chunks can be done *before* any work to refactor the 
vmware driver to use the multiple image locations feature - it will 
probably have to be given that was reverted out of Icehouse and will 
have some prerequisite work to do before it will land in Juno.

> . I would like to see you cores to be "involved" in this design since you will be reviewing the code at some point. "involved" here can be interpreted as review the design, and/ or actually participate to the design discussions. I would like to get your POV on this.
>
> Let me know if this approach makes sense.
>
> Thanks,
> Arnaud
>
> [1] https://review.openstack.org/#/c/70175/
> [2] https://review.openstack.org/#/c/33409/
>
>
> ----- Original Message -----
> From: "Matt Riedemann" <mriedem at linux.vnet.ibm.com>
> To: openstack-dev at lists.openstack.org
> Sent: Wednesday, March 12, 2014 11:28:23 AM
> Subject: Re: [openstack-dev] [nova] An analysis of code review in Nova
>
>
>
> On 2/25/2014 6:36 AM, Matthew Booth wrote:
>> I'm new to Nova. After some frustration with the review process,
>> specifically in the VMware driver, I decided to try to visualise how the
>> review process is working across Nova. To that end, I've created 2
>> graphs, both attached to this mail.
>>
>> Both graphs show a nova directory tree pruned at the point that a
>> directory contains less than 2% of total LOCs. Additionally, /tests and
>> /locale are pruned as they make the resulting graph much busier without
>> adding a great deal of useful information. The data for both graphs was
>> generated from the most recent 1000 changes in gerrit on Monday 24th Feb
>> 2014. This includes all pending changes, just over 500, and just under
>> 500 recently merged changes.
>>
>> pending.svg shows the percentage of LOCs which have an outstanding
>> change against them. This is one measure of how hard it is to write new
>> code in Nova.
>>
>> merged.svg shows the average length of time between the
>> ultimately-accepted version of a change being pushed and being approved.
>>
>> Note that there are inaccuracies in these graphs, but they should be
>> mostly good. Details of generation here:
>> https://urldefense.proofpoint.com/v1/url?u=https://github.com/mdbooth/heatmap&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=5wWaXo2oVaivfKLCMyU6Z9UTO8HOfeGCzbGHAT4gZpo%3D%0A&m=q%2BhYPEq%2BGxlDrGrMdbYCWuaLhZOwXwRpMQwWxkSied4%3D%0A&s=9a9e8ba562a81e0d00ca4190fbda306617637473ba5e721e4071d8d0ae20175c. This code is obviously
>> single-purpose, but is free for re-use if anyone feels so inclined.
>>
>> The first graph above (pending.svg) is the one I was most interested in,
>> and shows exactly what I expected it to. Note the size of 'vmwareapi'.
>> If you check out Nova master, 24% of the vmwareapi driver has an
>> outstanding change against it. It is practically impossible to write new
>> code in vmwareapi without stomping on an oustanding patch. Compare that
>> to the libvirt driver at a much healthier 3%.
>>
>> The second graph (merged.svg) is an attempt to look at why that is.
>> Again comparing the VMware driver with the libvirt we can see that at 12
>> days, it takes much longer for a change to be approved in the VMware
>> driver than in the libvirt driver. I suspect that this isn't the whole
>> story, which is likely a combination of a much longer review time with
>> very active development.
>>
>> What's the impact of this? As I said above, it obviously makes it very
>> hard to come in as a new developer of the VMware driver when almost a
>> quarter of it has been rewritten, but you can't see it. I am very new to
>> this and others should validate my conclusions, but I also believe this
>> is having a detrimental impact to code quality. Remember that the above
>> 12 day approval is only the time for the final version to be approved.
>> If a change goes through multiple versions, each of those also has an
>> increased review period, meaning that the time from first submission to
>> final inclusion is typically very, very protracted. The VMware driver
>> has its fair share of high priority issues and functionality gaps, and
>> the developers are motived to get it in the best possible shape as
>> quickly as possible. However, it is my impression that when problems
>> stem from structural issues, the developers choose to add metaphorical
>> gaffer tape rather than fix them, because fixing both creates a
>> dependency chain which pushes the user-visible fix months into the
>> future. In this respect the review process is dysfunctional, and is
>> actively detrimental to code quality.
>>
>> Unfortunately I'm not yet sufficiently familiar with the project to
>> offer a solution. A core reviewer who regularly looks at it is an
>> obvious fix. A less obvious fix might involve a process which allows
>> developers to work on a fork which is periodically merged, rather like
>> the kernel.
>>
>> Matt
>>
>>
>>
>> _______________________________________________
>> OpenStack-dev mailing list
>> OpenStack-dev at lists.openstack.org
>> https://urldefense.proofpoint.com/v1/url?u=http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=5wWaXo2oVaivfKLCMyU6Z9UTO8HOfeGCzbGHAT4gZpo%3D%0A&m=q%2BhYPEq%2BGxlDrGrMdbYCWuaLhZOwXwRpMQwWxkSied4%3D%0A&s=f45e9c5395962acd9184246580e317582cb00b9428861ef7853989b96afc842f
>>
>
> When I originally read this I had some ideas in mind for a response
> regarding review latency with the vmware driver patches, but felt like
> anything I said, albeit what I consider honest, would sound
> bad/offensive in some way, and didn't want to convey that.
>
> But this came up in IRC today:
>
> https://urldefense.proofpoint.com/v1/url?u=https://blueprints.launchpad.net/nova/%2Bspec/vmware-spawn-refactor&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=5wWaXo2oVaivfKLCMyU6Z9UTO8HOfeGCzbGHAT4gZpo%3D%0A&m=q%2BhYPEq%2BGxlDrGrMdbYCWuaLhZOwXwRpMQwWxkSied4%3D%0A&s=3f90de5965e50ab37a5f404a805eb5290e3dab4499bee8f288935619541ee6b2
>
> That spurred some discussion around this same topic and I think
> highlights one of the major issues, which is code quality and the design
> of the driver.
>
> For example, the driver's spawn method is huge and there are a lot of
> nested methods within it.  There are a lot of vmware patches and a lot
> of blueprints, and a lot of them touch spawn.  When I'm reviewing them,
> I'm looking for new conditions and checking to see if those are unit
> tested (positive/error cases) and a lot of the time I find it very
> difficult to tell if they are or not.  I think a lot of that has to do
> with how the vmware tests are scaffolded with fakes to simulate a
> vcenter backend, but it makes it very difficult if you're not extremely
> familiar with that code to know if something is covered or not.
>
> And I've actually asked in bp reviews before, 'you have this new/changed
> condition, where is it tested' and the response is more or less 'we plan
> on refactoring this later and will add unit tests then' or 'it's hard to
> test this path without a lot of changes to how the tests are working'.
> That's unacceptable to me, and I generally give up on the review after that.
>
> So to move this all forward, I think that bp above should be top
> priority for the vmware team in Juno to keep bp patches moving at the
> pace they do, because the features and refactoring just keeps coming and
> at least for me it's very hard to burn out on looking at those reviews.
>

-- 

Thanks,

Matt Riedemann