10:33 a.m.
Hans De Goede has uploaded a new change for review.
Change subject: Add support for redir devices
......................................................................
Add support for redir devices
With this patch it is possible to create vms setup for usb redirection with
vdsm, ie:
vdsClient 0 create /dev/null \
vmId=00000001-0002-0003-0004-000000000006 vmName=F14-x86_64 memSize=512 \
macAddr=52:54:00:7a:b4:7e nicModel=virtio bridge=test display=qxl \
spiceDisableTicketing=1 \
devices='{type:disk,device:disk,path:/home/virt-images/f14-i386.qcow2.img,propagateErrors:off,iface:ide,index:0,readonly:False,format:cow}'
\
devices='{type:video,device:qxl,specParams:{vram:65536},address:{type:pci,domain:0x0000,bus:0x00,slot:0x02,function:0x0}}'
\
devices='{type:controller,device:virtio-serial,address:{type:pci,domain:0x0000,bus:0x00,slot:0x03,function:0x0}}'
\
devices='{type:controller,device:usb,model:ich9-ehci1,address:{type:pci,domain:0x0000,bus:0x00,slot:0x04,function:0x7}}'
\
devices='{type:controller,device:usb,model:ich9-uhci1,master:{startport:0},address:{type:pci,domain:0x0000,bus:0x00,slot:0x04,function:0x0,multifunction:on}}'
\
devices='{type:controller,device:usb,model:ich9-uhci2,master:{startport:2},address:{type:pci,domain:0x0000,bus:0x00,slot:0x04,function:0x1}}'
\
devices='{type:controller,device:usb,model:ich9-uhci3,master:{startport:4},address:{type:pci,domain:0x0000,bus:0x00,slot:0x04,function:0x2}}'
\
devices='{type:redir,device:spicevmc,bus:usb,address:{type:usb,bus:0,port:1}}'
\
devices='{type:redir,device:spicevmc,bus:usb,address:{type:usb,bus:0,port:2}}'
\
devices='{type:redir,device:spicevmc,bus:usb,address:{type:usb,bus:0,port:3}}'
\
Change-Id: Ia91922a0f34cb4b32efa2c6397d13fe59aec04e7
Signed-off-by: Hans de Goede <hdegoede(a)redhat.com>
---
M vdsm/libvirtvm.py
M vdsm/vm.py
2 files changed, 18 insertions(+), 3 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/33/4133/1
--
To view, visit http://gerrit.ovirt.org/4133
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia91922a0f34cb4b32efa2c6397d13fe59aec04e7
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Hans De Goede <hdegoede(a)redhat.com>
1:49 a.m.
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>
Gerrit-Reviewer: Eli Mesika <emesika(a)redhat.com>
Gerrit-Reviewer: Gal Hammer <ghammer(a)redhat.com>
Gerrit-Reviewer: Hans De Goede <hdegoede(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourfali(a)redhat.com>
9:54 a.m.
Dan Kenigsberg has submitted this change and it was merged.
Change subject: Add support for redir devices
......................................................................
Add support for redir devices
With this patch it is possible to create vms setup for usb redirection with
vdsm, ie:
vdsClient 0 create /dev/null \
vmId=00000001-0002-0003-0004-000000000006 vmName=F14-x86_64 memSize=512 \
macAddr=52:54:00:7a:b4:7e nicModel=virtio bridge=test display=qxl \
spiceDisableTicketing=1 \
devices='{type:disk,device:disk,path:/home/virt-images/f14-i386.qcow2.img,propagateErrors:off,iface:ide,index:0,readonly:False,format:cow}'
\
devices='{type:video,device:qxl,specParams:{vram:65536},address:{type:pci,domain:0x0000,bus:0x00,slot:0x02,function:0x0}}'
\
devices='{type:controller,device:virtio-serial,address:{type:pci,domain:0x0000,bus:0x00,slot:0x03,function:0x0}}'
\
devices='{type:controller,device:usb,model:ich9-ehci1,address:{type:pci,domain:0x0000,bus:0x00,slot:0x04,function:0x7}}'
\
devices='{type:controller,device:usb,model:ich9-uhci1,master:{startport:0},address:{type:pci,domain:0x0000,bus:0x00,slot:0x04,function:0x0,multifunction:on}}'
\
devices='{type:controller,device:usb,model:ich9-uhci2,master:{startport:2},address:{type:pci,domain:0x0000,bus:0x00,slot:0x04,function:0x1}}'
\
devices='{type:controller,device:usb,model:ich9-uhci3,master:{startport:4},address:{type:pci,domain:0x0000,bus:0x00,slot:0x04,function:0x2}}'
\
devices='{type:redir,device:spicevmc,bus:usb,address:{type:usb,bus:0,port:1}}'
\
devices='{type:redir,device:spicevmc,bus:usb,address:{type:usb,bus:0,port:2}}'
\
devices='{type:redir,device:spicevmc,bus:usb,address:{type:usb,bus:0,port:3}}'
\
Change-Id: Ia91922a0f34cb4b32efa2c6397d13fe59aec04e7
Signed-off-by: Hans de Goede <hdegoede(a)redhat.com>
---
M vdsm/libvirtvm.py
M vdsm/vm.py
2 files changed, 18 insertions(+), 3 deletions(-)
Approvals:
Gal Hammer: Looks good to me, but someone else must approve
Dan Kenigsberg: Verified; Looks good to me, approved
Igor Lvovsky: Looks good to me, but someone else must approve
Oved Ourfali: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.ovirt.org/4133
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia91922a0f34cb4b32efa2c6397d13fe59aec04e7
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Hans De Goede <hdegoede(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eli Mesika <emesika(a)redhat.com>
Gerrit-Reviewer: Gal Hammer <ghammer(a)redhat.com>
Gerrit-Reviewer: Hans De Goede <hdegoede(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourfali(a)redhat.com>
9:54 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: Add support for redir devices
......................................................................
Patch Set 3: Verified; Looks good to me, approved
--
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Hans De Goede <hdegoede(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eli Mesika <emesika(a)redhat.com>
Gerrit-Reviewer: Gal Hammer <ghammer(a)redhat.com>
Gerrit-Reviewer: Hans De Goede <hdegoede(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourfali(a)redhat.com>
6:27 a.m.
Gal Hammer has posted comments on this change.
Change subject: Add support for redir devices
......................................................................
Patch Set 3: Looks good to me, but someone else must approve
--
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Hans De Goede <hdegoede(a)redhat.com>
Gerrit-Reviewer: Eli Mesika <emesika(a)redhat.com>
Gerrit-Reviewer: Gal Hammer <ghammer(a)redhat.com>
Gerrit-Reviewer: Hans De Goede <hdegoede(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourfali(a)redhat.com>
5:51 a.m.
Igor Lvovsky has posted comments on this change.
Change subject: Add support for redir devices
......................................................................
Patch Set 3: Looks good to me, but someone else must approve
It will be good to add _getUnderlyingRedirDevice to get address from libvirt.
But it can be done later.
--
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Hans De Goede <hdegoede(a)redhat.com>
Gerrit-Reviewer: Eli Mesika <emesika(a)redhat.com>
Gerrit-Reviewer: Gal Hammer <ghammer(a)redhat.com>
Gerrit-Reviewer: Hans De Goede <hdegoede(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourfali(a)redhat.com>
5:50 a.m.
Oved Ourfali has posted comments on this change.
Change subject: Add support for redir devices
......................................................................
Patch Set 3: Looks good to me, but someone else must approve
--
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Hans De Goede <hdegoede(a)redhat.com>
Gerrit-Reviewer: Eli Mesika <emesika(a)redhat.com>
Gerrit-Reviewer: Gal Hammer <ghammer(a)redhat.com>
Gerrit-Reviewer: Hans De Goede <hdegoede(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourfali(a)redhat.com>
3:52 a.m.
Igor Lvovsky has posted comments on this change.
Change subject: Add support for redir devices
......................................................................
Patch Set 2: Looks good to me, but someone else must approve
--
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>
Gerrit-Reviewer: Gal Hammer <ghammer(a)redhat.com>
Gerrit-Reviewer: Hans De Goede <hdegoede(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
10:20 a.m.
Hans De Goede has posted comments on this change.
Change subject: Add support for redir devices
......................................................................
Patch Set 2: Verified
Tested and works for me.
--
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>
Gerrit-Reviewer: Gal Hammer <ghammer(a)redhat.com>
Gerrit-Reviewer: Hans De Goede <hdegoede(a)redhat.com>
9:17 a.m.
Gal Hammer has posted comments on this change.
Change subject: Add support for redir devices
......................................................................
Patch Set 2: Looks good to me, but someone else must approve
--
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>
Gerrit-Reviewer: Gal Hammer <ghammer(a)redhat.com>
Gerrit-Reviewer: Hans De Goede <hdegoede(a)redhat.com>
10:29 a.m.
Oved Ourfali has posted comments on this change.
Change subject: Add support for redir devices
......................................................................
Patch Set 2:
Hans,
I apologize for that. You have to understand we are close to feature freeze, and everybody
is working on many tasks here, so it's true we were discussing it for 2 weeks, but
only two days ago I got a chance to really start coding and seeing things in action. It is
the first feature I do in this part of the ovirt engine.
I'll look at it again with the engine guys, and see what we can do there. I'll go
over it with Eli and Igor on Sunday (the ones who designed this API in the first place),
and see what's the best solution for that.
I don't know why the specParams API was decided, and what were the exact reasons for
that. All I know is that that's the way it is now, and not sure we could change the
API in this phase.
I thank you for your help in understanding the issues, and with pushing the thread with
the libvirt guys. Hopefully we will get it working soon.
--
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>
Gerrit-Reviewer: Eli Mesika <emesika(a)redhat.com>
Gerrit-Reviewer: Gal Hammer <ghammer(a)redhat.com>
Gerrit-Reviewer: Hans De Goede <hdegoede(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourfali(a)redhat.com>
1:46 p.m.
Hans De Goede has posted comments on this change.
Change subject: Add support for redir devices
......................................................................
Patch Set 2:
WRT specParams, I believe that the engine then just needs to be changed to pass parameters
which are now inside SpecParams at the same "dict level" as type and address, I
see no need for this nested dict non sense.
To be completely honest, I think specParams is madness, and the madness needs to be
stopped ASAP. A decision has been made to go with libvirt as direct manager of the vms and
vdsm as an agent between libvirt and ovirt-engine.
Since the decision to use libvirt has been made a long time ago, it no longer makes any
sense to have an ovirt private "dialect" for specifying vm parameters / devices.
I strongly believe that the xmlrpc structure of the create parameters should be identical
to the libvirt xml structure. Any translation done by vdsm will only serve to confuse
users trying to figure out the mapping between the create params and the libxml vdsm, as
well as introduce a whole now class of bugs which we can do without.
--
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>
Gerrit-Reviewer: Eli Mesika <emesika(a)redhat.com>
Gerrit-Reviewer: Gal Hammer <ghammer(a)redhat.com>
Gerrit-Reviewer: Hans De Goede <hdegoede(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourfali(a)redhat.com>
1:12 p.m.
Oved Ourfali has posted comments on this change.
Change subject: Add support for redir devices
......................................................................
Patch Set 2:
Hey,
Regardless the results of the upstream discussion on the address part, in the engine we
pass all the attributes besides type and device, as a dictionary called specParams.
So, in this case the model in the controllers, master in the controllers and the bus in
the redir devices, must be passed in this dictionary.
so it would look like:
devices='{type:controller,device:usb,specParams{model:ich9-uhci1,master:{startport:0}},address:{type:pci,domain:0x0000,bus:0x00,slot:0x04,function:0x0,multifunction:on}}'
same for the redir device:
devices='{type:redir,device:spicevmc,specParams{bus:usb},address:{type:usb,bus:0,port:2}}'
Doesn't seem like a major change.
Sorry that my comment comes only now. I just now did some testing and saw that this is
indeed the situation.
Adding Eli to the reviewers to make sure I'm right.
--
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>
Gerrit-Reviewer: Gal Hammer <ghammer(a)redhat.com>
Gerrit-Reviewer: Hans De Goede <hdegoede(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourfali(a)redhat.com>
4414
days inactive
4427
days old
vdsm-patches@lists.fedorahosted.org
12 comments
5 participants
participants (5)
-
Dan Kenigsberg -
ghammer＠redhat.com -
Hans de Goede -
Igor Lvovsky -
Oved Ourfali