Re: [ovirt-devel] [vdsm] VmDevices rework

----- Original Message ----- > From: "Dan Kenigsberg" <danken(a)redhat.com&gt; > To: "Martin Polednik" <mpolednik(a)redhat.com&gt; > Cc: devel(a)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).

> 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.

> 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.

> 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!