Change in vdsm[master]: Add support for redir devices

[Hans de Goede]

Hans De Goede has posted comments on this change.

Change subject: Add support for redir devices
......................................................................


Patch Set 2:

Hi again,

I'm sorry my review comment from yesterday may have been a bit harsh. It is just that
I'm a bit surprised that after working fulltime on this for a week, and then
discussing it for a further week, so after 2 weeks something as fundamental as the API
comes up for discussion. That is something to discuss on day 1, not after 2 weeks.

The (xmlrpc) API is the boundary and interface between ovirt-engine and vdsm, and it is
important that such interfaces are well designed and simple to understand. And it seems
clear to me that e.g.:
devices='{type:redir,device:spicevmc,bus:usb,address:{type:usb,bus:0,port:2}}'
Is simply just clearer and simpler then:
devices='{type:redir,device:spicevmc,specParams{bus:usb},address:{type:usb,bus:0,port:2}}'
And also maps 1 on 1 to libvirt's xml, making it easy for users when troubleshooting
to see the relation between the xmlrpc call contents and the
generated libvirt xml.

I trust that you've good reasons to store certain values inside ovirt-engine inside a
specParams dict, but there is no reason that those internals should leak into an boundary
API / interface. On the contrary such API's should isolate both sides from the other
sides internals!

Luckily there is an easy solution, ovirt can simply keep using the specParams internally,
and when it does a create call to vdsm, it can make a copy of the devices array and then
for each device in the copy take the contents of the specParams dict of that device, add
those to the device's own dict and remove the specParams. And then use the modified
devices copy to pass to the create call. This way we can stop the ovirt-engine specParams
internal from leaking into the API between vdsm and ovirt-engine and thus keep the API
sane.

I know that specParams currently is already used for the vram of video devices, and this
currently requires an ugly hack inside vdsm, prooving that having specParams in the API is
a BAD idea. It will be quite easy to add support for having a vram value directly inside
the device dict, and vdsm can keep support
for the specParams for video devices for a while as a transition phase, and then later on
delete the ugly hack :)

I would be more then happy to write a vdsm patch to add support for having vram as a value
on the device dict level.

Regards,

Hans

--
To view, visit http://gerrit.ovirt.org/4133
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia91922a0f34cb4b32efa2c6397d13fe59aec04e7
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Hans De Goede <hdegoede(a)redhat.com&gt;
Gerrit-Reviewer: Eli Mesika <emesika(a)redhat.com&gt;
Gerrit-Reviewer: Gal Hammer <ghammer(a)redhat.com&gt;
Gerrit-Reviewer: Hans De Goede <hdegoede(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Oved Ourfali <oourfali(a)redhat.com&gt;