[ovirt-devel] [vdsm] VmDevices rework

[Dan Kenigsberg]

On Mon, May 04, 2015 at 05:21:29AM -0400, Martin Polednik wrote:
> 
> 
> ----- Original Message -----
> > From: "Dan Kenigsberg" <danken at redhat.com>
> > To: "Martin Polednik" <mpolednik at redhat.com>
> > Cc: devel at ovirt.org
> > Sent: Friday, May 1, 2015 5:06:04 PM
> > Subject: Re: [ovirt-devel] [vdsm] VmDevices rework
> > 
> > On Tue, Apr 28, 2015 at 04:28:12AM -0400, Martin Polednik wrote:
> > > Hello everyone,
> > > 
> > > I have started working on line of patches that deal with current state of
> > > VM devices
> > > in VDSM. There is a wiki page at [1] describing the issues, phases and
> > > final goals
> > > for this work. Additionally, there is formal naming system for code related
> > > to
> > > devices. I would love to hear your opinions and comments about this effort!
> > > (and please note that this is very long term work)
> > > 
> > > [1] http://www.ovirt.org/Feature/VmDevices_rework
> > 
> > Hi Martin,
> > 
> > It's a great and just initiative. Renaming is good, but I'd like to hear
> > more about your final goal - besides the hope that it the code is
> > shorter.
> > 
> > In my opion you should state your high-level goal explicitly:
> > device_object should be self-sufficient regarding: initiation, state
> > change, stat extraction and stat reporting.
> > 
> > Comments:
> > 1. We want to drop the legacy conf support. In
> >    https://gerrit.ovirt.org/40104 we have removed support of engine<3.3
> >    so production environment does not need it any more.
> > 
> >    There might be an issue with command-line test scripts, though that
> >    might be abusing the legacy args. At the very least it should be
> >    marked as deprecated.
> 
> Therefore, the move still makes sense, as for the deprecation - I fully
> agree. Will try to figure a nice way to mention it (logging and docs I guess).

At the very least, you should mark any use of legacy with a log ERROR.

>
> > 2. dev_map is an unfortunate historical mistake. The fact that it is a
> >    map of device type to device_object lists helps no one. It
> >    should be hidden behind one or two lookup functions. Each object
> >    class should be able to iterate its instances, and add filtering on
> >    top of that.
> 
> Interesting point, but you still need to somehow store the device
> instances. Having the implementation hidden is fine (and the _device name
> already suggests that) but it still needs to exist.

Of course. And you may keep the current mapping for the mean while. But
it should be layered away by a set of accessors.

>
> > 3. A definition like "Type of the device is dev_type" does not help for
> >    someone who knows what is dev_type. Please give examples.
> >    The "formal naming system" could use some wiki styling.
> 
> Will work on that!
> 
> > 4. I did not understand "Libvirt XML parsing and processing is dumb -
> >    missing orchestration object to delegate XML chunks to devices themself"
> 
> Too terse indeed, will try to rephrase. The issue is the fact that each dev_type
> parsing iterates over all device elements in XML, leading to O(n^2) complexity
> just for the parsing.

The it's not XML parsing per se, but the convesion of the data into
objects. Please explain this issue on wiki.

>
> > 5. It seems that you have further "phases" planned, but not written.
> >    Could you provide at least the title, or "charter" of each phase? 1.1
> >    is about renaming. 1.2 is about legacy. Do you have phases 2, 3..?
> 
> Still in process of figuring out the naming of the phases and their order,
> I'll try to add them when it makes sense (I just can't spit 10 names out of
> my head right now that would make sense in a long term).
> 
> > Death (or at least diet) to vm.py!
> 
> +1 :) Thanks for the review!