[ovirt-devel] [vdsm][oVirt 4.0] trimming down vm.py, stage 1

[Francesco Romani]

----- Original Message -----
> From: "Francesco Romani" <fromani at redhat.com>
> To: devel at ovirt.org
> Sent: Monday, November 30, 2015 10:49:58 AM
> Subject: Re: [ovirt-devel] [vdsm][oVirt 4.0] trimming down vm.py, stage 1
> 
> ----- Original Message -----
> > From: "Francesco Romani" <fromani at redhat.com>
> > To: devel at ovirt.org
> > Sent: Tuesday, November 24, 2015 11:51:10 AM
> > Subject: [ovirt-devel] [vdsm][oVirt 4.0] trimming down vm.py, stage 1
> 
> [...]
> > We have roughly five weeks left until the end of the year.
> > Sounds like a good timespan for the first stage, and I'm aiming for go down
> > under
> > the 4000 line mark.
> > 
> > So we have roughly 1200 lines to eliminate somehow.
> > 
> > This is the initial plan I'm thinking off, in expected increasing
> > difficulty
> > level, from easies to hardest:
> 
> And here's the first status update

Here's the second status update

> > 1. LiveMergeCleanupThread:
> > Chance to break things: minimal
> > - roughly 120 lines (10% of the goal)
> > - can be moved verbatim (cut/paste) elsewhere, like vdsm/virt/livemerge.py,
> >   fixing only import paths.
> 
> Patch ready and not published. I was thinking about moving more code in the
> newly
> created livemerge.py, but not sure about the partitioning. I'll just push
> the patch so we can discuss there.

Done here: 

https://gerrit.ovirt.org/#/q/status:open+project:vdsm+branch:master+topic:vm-trim-stage-1-p1

I think this code could and should be split, not sure about the boundary.
Please take this as draft and material for discussion.
This is self contained and low-hanging fruit but not urgent, so hurry at all to review
(I know storage devs are busy those days).

> > 2. the findDrive* family of functions:
> > Chance to break things: minimal/very low
> > - roughly 50 lines (~4% of the goal)
> > - can be moved with minimal changes elsewhere, like
> > vdsm/virt/storage/????.py,
> 
> Done in https://gerrit.ovirt.org/#/c/49209/

Again not completeley sure about the split, however this ended up being free functions.
 
> > 3. the getConf* faimily of functions:
> > Chance to break things: medium-to-high in the 3.x branch, low on 4.x
> > because
> > we drop backward compat.
> > - roughly 110 lines (~9% of the goal)
> > - just drop them for 4.0!
> > - alternatively, need to figure out a proper place, maybe a new module?
> 
> Just dropped in https://gerrit.ovirt.org/#/c/49173/
> 
> The patch is quite large but simple, in the end all the code is guarded
> by a couple of bug if()s.
> 
> The only concern is we break compatibility with older Engines, so maybe
> merge this later in the development cycles.
> Except for maybe some glitches, I don't expect further work here (of course
> except verification!)

Dan added good points about further backward compatibility concerns.
Will address his comments and update before the end of the year.
Migration from 3.6 VDSMs should work fine, but will be tested, of course.

> > 4. the Devices/VM setup bits
> > All scatthered through vm.py (e.g. _buildDomainXML, DeviceMapping table)
> > Chance to break things: medium-to-low, depends on individual patches
> > - all summed up, roughly 200 lines (~18% of the goal)
> > - need serious cleanup
> > - no definite plan
> > - mostly is cut/paste or transform Vm methods into functions
> 
> Not tackled yet

Still no progress.
  
> > 5. the getUnderlying* familiy of functions:
> > Chance to break things: low (maybe medium on some cases, to lean on the
> > safe
> > side)
> > - roughly 550 lines (~46% of the goal)
> > - easy to test (just run vms!)
> > - can be moved in the devices/ subpackage with some changes.
> > - require further work in the area to properly integrate this code in the
> > device objects
> > - more work is planned in this area, including move from minidom to
> > ElementTree

This is gonna be large, but could be taken in steps, the first are almost mechanic.
Unfortunately, the largest bits (Drive, Interface) are the less easy to simplify.

I begun with some cleanup which may want anyway. This alone factorizes five devices
classes, and in the end shaves ~100 lines from vm.py:

https://gerrit.ovirt.org/#/q/status:open+project:vdsm+branch:master+topic:vm-trim-stage-1-p4-pre

To avoid to clog gerrit and because reviewers resources are scarce as usual, will work
now to have posted series merged or abandonded (should we find out this is wrong approach) before
to post the remaining patches for stage 1.

Bests,

-- 
Francesco Romani
RedHat Engineering Virtualization R & D
Phone: 8261328
IRC: fromani