Change in vdsm[master]: vdsm: add functionality to acquire host devices

[Martin Polednik]

Martin Polednik has uploaded a new change for review.

Change subject: vdsm: add functionality to acquire host devices
......................................................................

vdsm: add functionality to acquire host devices

Acquire call (libvirt's dettach call) is mandatory to properly
detach device from host.

Change-Id: I530fed56831314286942b8afa62900208812bfb5
Signed-off-by: Martin Polednik <mpolednik(a)redhat.com&gt;
---
M vdsm/hostdev.py
1 file changed, 13 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/89/32389/1

diff --git a/vdsm/hostdev.py b/vdsm/hostdev.py
index 7ad5695..896fe95 100644
--- a/vdsm/hostdev.py
+++ b/vdsm/hostdev.py
@@ -24,6 +24,9 @@
 from vdsm import libvirtconnection
 
 
+DETACHABLE_DEVICES = ('usb', 'pci')
+
+
 def _parse_device_params(device_xml):
     params = {}
 
@@ -116,3 +119,13 @@
             del(devices[device_name])
 
     return devices
+
+
+def acquire(device_name):
+    device = libvirtconnection.get().nodeDeviceLookupByName(device_name)
+    device_params = _parse_device_params(device)
+
+    if device_params['capability'] in DETACHABLE_DEVICES:
+        device.dettach()
+
+    return device_params


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;

Change in vdsm[master]: vdsm: add functionality to detach host devices

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: vdsm: add functionality to detach host devices
......................................................................


vdsm: add functionality to detach host devices

Detach call (libvirt's dettach/detachFlags call) is required to
properly detach device from host. Dettaching device from the host means
unbinding it from it's driver and binding it to VFIO driver, which
serves as KVM device driver.

Change-Id: I530fed56831314286942b8afa62900208812bfb5
Signed-off-by: Martin Polednik <mpolednik(a)redhat.com&gt;
Reviewed-on: http://gerrit.ovirt.org/32389
Reviewed-by: Dan Kenigsberg <danken(a)redhat.com&gt;
---
M vdsm/hostdev.py
1 file changed, 18 insertions(+), 0 deletions(-)

Approvals:
  Dan Kenigsberg: Looks good to me, approved
  Martin Polednik: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: add functionality to detach host devices

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: add functionality to detach host devices
......................................................................


Patch Set 5:

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13387/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13226/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12436/ : FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: vdsm: add functionality to detach host devices

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm: add functionality to detach host devices
......................................................................


Patch Set 4: Code-Review-1

(1 comment)

nit about naming.

http://gerrit.ovirt.org/#/c/32389/4/vdsm/hostdev.py
File vdsm/hostdev.py:

Line 114: 
Line 115:     return devices
Line 116: 
Line 117: 
Line 118: def parse_and_detach_if_detachable(device_name):
detach_if_detachable() is complex-enough name.

instead of the "parse_and_" prefix, place a docspace explaining the dictionary
that is returned.
Line 119:     libvirt_device, device_params = _get_device_ref_and_params(device_name)
Line 120: 
Line 121:     if device_params['capability'] in _DETACH_REQUIRING_CAPS:
Line 122:         libvirt_device.detachFlags(None)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: vdsm: add functionality to detach host devices

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: add functionality to detach host devices
......................................................................


Patch Set 4:

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13291/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13131/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12341/ : FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: vdsm: add functionality to detach host devices

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm: add functionality to detach host devices
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/32389/3/vdsm/hostdev.py
File vdsm/hostdev.py:

Line 117: 
Line 118: def detach(device_name):
Line 119:     libvirt_device, device_params = _get_device_ref_and_params(device_name)
Line 120: 
Line 121:     if device_params['capability'] in _UNMANAGED_DEVICE_CAPS:
>  Hopefully I'm starting to understand - current detach() function
> actually d 
defining a get_parsed_params() and detach() function here, and using them
properly in vm.py seems the right thing to do.
Line 122:         libvirt_device.dettach()
Line 123: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: vdsm: add functionality to detach host devices

[Martin Polednik]

Martin Polednik has posted comments on this change.

Change subject: vdsm: add functionality to detach host devices
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.ovirt.org/#/c/32389/3/vdsm/hostdev.py
File vdsm/hostdev.py:

Line 117: 
Line 118: def detach(device_name):
Line 119:     libvirt_device, device_params = _get_device_ref_and_params(device_name)
Line 120: 
Line 121:     if device_params['capability'] in _UNMANAGED_DEVICE_CAPS:
>  Do you expect that many users would need the semantics of
> "detach the devic 
Hopefully I'm starting to understand - current detach()
function actually does two things: detaches_if_detachable() and returns the parsed XML to
vmdevice space.

I'm ok with splitting it to detach_if_detachable (maybe phrased differently, but
keeping the same logic) and something like get_parsed_params (possibly nicer name) - the
functionality would essentially be the same.

The second option for me is to revert back to the name of acquire - where the acquire
composite function is meant to process the request for device and execute routines
necessary to prepare it for the VM, returning parameters of the prepared device. (that is
what this function does, therefore this would only mean renaming it back to acquire)
Line 122:         libvirt_device.dettach()
Line 123: 


Line 118: def detach(device_name):
Line 119:     libvirt_device, device_params = _get_device_ref_and_params(device_name)
Line 120: 
Line 121:     if device_params['capability'] in _UNMANAGED_DEVICE_CAPS:
Line 122:         libvirt_device.dettach()
>  the spelling mistake of dettach/detach is a sore in my eye. Could you
> use d 
I understand your suffering :P At least on my f20 test box, detachFlags have
signature detachFlags(self, driverName, flags=0) - I'll have a look if I can get
driverName to use hypervisor default...
Line 123: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: vdsm: add functionality to detach host devices

[Martin Polednik]

Martin Polednik has posted comments on this change.

Change subject: vdsm: add functionality to detach host devices
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/32389/3/vdsm/hostdev.py
File vdsm/hostdev.py:

Line 117: 
Line 118: def detach(device_name):
Line 119:     libvirt_device, device_params = _get_device_ref_and_params(device_name)
Line 120: 
Line 121:     if device_params['capability'] in _UNMANAGED_DEVICE_CAPS:
>  logging error makes sense in this case 
I'd refer to
http://gerrit.ovirt.org/#/c/22462/24/vdsm/virt/vm.py (it is old version though, so bear
that in mind): detach is called for every device to acquire initial device_params, only
some devices require the call to libvirt (and this is effort to hide this detail from VM
device class) - it's not error, and not really additional information - it's
simply different workflow for less complicated devices
Line 122:         libvirt_device.dettach()
Line 123: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: vdsm: add functionality to detach host devices

Michal Skrivanek has posted comments on this change.

Change subject: vdsm: add functionality to detach host devices
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/32389/3/vdsm/hostdev.py
File vdsm/hostdev.py:

Line 117: 
Line 118: def detach(device_name):
Line 119:     libvirt_device, device_params = _get_device_ref_and_params(device_name)
Line 120: 
Line 121:     if device_params['capability'] in _UNMANAGED_DEVICE_CAPS:
>  scsi, scsi_device, scsi_target, usb (actually might be a bug above,
> will lo 
logging error makes sense in this case
Line 122:         libvirt_device.dettach()
Line 123: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: vdsm: add functionality to detach host devices

[Martin Polednik]

Martin Polednik has posted comments on this change.

Change subject: vdsm: add functionality to detach host devices
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/32389/3/vdsm/hostdev.py
File vdsm/hostdev.py:

Line 117: 
Line 118: def detach(device_name):
Line 119:     libvirt_device, device_params = _get_device_ref_and_params(device_name)
Line 120: 
Line 121:     if device_params['capability'] in _UNMANAGED_DEVICE_CAPS:
>  can you attach non-(pci,usb) devices? could you give me an example
> for such 
scsi, scsi_device, scsi_target, usb (actually might be a bug above, will
look at it as dettach/reattach should be called against usb_device, not usb); the check
whether given device is or is not managed by libvirt (== manually needs to be
dettached/reattached) would have to be in the HostDev class, exposing quite a low level
idea in the high level VM device space
Line 122:         libvirt_device.dettach()
Line 123: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: vdsm: add functionality to detach host devices

[Martin Polednik]

Martin Polednik has posted comments on this change.

Change subject: vdsm: add functionality to detach host devices
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/32389/3/vdsm/hostdev.py
File vdsm/hostdev.py:

Line 117: 
Line 118: def detach(device_name):
Line 119:     libvirt_device, device_params = _get_device_ref_and_params(device_name)
Line 120: 
Line 121:     if device_params['capability'] in _UNMANAGED_DEVICE_CAPS:
>  We were asked to det(t?)atch a device that is not pci nor usb. Why?
> And why 
For the sake of consistency - only _UNMANAGED_DEVICE_CAPS actually require
the real detach (unbinding driver -> vfio bind) but having the check for other devices
here removes low-level logic from VmDevice class that would otherwise have to handle
this.
Line 122:         libvirt_device.dettach()
Line 123: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: vdsm: add functionality to detach host devices

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm: add functionality to detach host devices
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.ovirt.org/#/c/32389/3//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2014-09-03 14:58:40 +0200
Line 4: Commit:     Martin Polednik <mpolednik(a)redhat.com&gt;
Line 5: CommitDate: 2014-10-13 16:50:38 +0200
Line 6: 
Line 7: vdsm: add functionality to detach host devices
"vdsm:" prefix is pointless, please drop it.

Please use a consistent spelling for "dettach" (same as the one chosen by
libvirt)
Line 8: 
Line 9: Detach call (libvirt's dettach call) is required to properly
Line 10: detach device from host. Dettaching device from the host means
Line 11: unbinding it from it's driver and binding it to VFIO driver, which


Line 7: vdsm: add functionality to detach host devices
Line 8: 
Line 9: Detach call (libvirt's dettach call) is required to properly
Line 10: detach device from host. Dettaching device from the host means
Line 11: unbinding it from it's driver and binding it to VFIO driver, which
it's -> its
Line 12: serves as KVM device driver.
Line 13: 
Line 14: Change-Id: I530fed56831314286942b8afa62900208812bfb5


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: vdsm: add functionality to detach host devices

[Martin Polednik]

Martin Polednik has posted comments on this change.

Change subject: vdsm: add functionality to detach host devices
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/32389/3/vdsm/hostdev.py
File vdsm/hostdev.py:

Line 57: 
Line 58: def _get_device_ref_and_params(device_name):
Line 59:     libvirt_device = libvirtconnection.get().\
Line 60:         nodeDeviceLookupByName(device_name)
Line 61:     return libvirt_device, _parse_device_params(libvirt_device.XMLDesc(0))
>  how often this xml is supposed to change? 
No, no, no, no please
no - the patch was remade from caching version to this one ;) It'll be called each
time VM is created or destroyed.
Line 62: 
Line 63: 
Line 64: def _get_devices_from_vms(vmContainer):
Line 65:     """


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: vdsm: add functionality to detach host devices

Francesco Romani has posted comments on this change.

Change subject: vdsm: add functionality to detach host devices
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/32389/3/vdsm/hostdev.py
File vdsm/hostdev.py:

Line 57: 
Line 58: def _get_device_ref_and_params(device_name):
Line 59:     libvirt_device = libvirtconnection.get().\
Line 60:         nodeDeviceLookupByName(device_name)
Line 61:     return libvirt_device, _parse_device_params(libvirt_device.XMLDesc(0))
how often this xml is supposed to change?
after each device change, I guess.
Can we somehow cache the parsed XML?
(probably better to do that in a different patch)
Line 62: 
Line 63: 
Line 64: def _get_devices_from_vms(vmContainer):
Line 65:     """


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: vdsm: add functionality to detach host devices

Francesco Romani has posted comments on this change.

Change subject: vdsm: add functionality to detach host devices
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/32389/3/vdsm/hostdev.py
File vdsm/hostdev.py:

Line 57: 
Line 58: def _get_device_ref_and_params(device_name):
Line 59:     libvirt_device = libvirtconnection.get().\
Line 60:         nodeDeviceLookupByName(device_name)
Line 61:     return libvirt_device, _parse_device_params(libvirt_device.XMLDesc(0))
>  No, no, no, no please no - the patch was remade from caching version
> to thi 
Yep, now I remember. Right, discard my comment.
Line 62: 
Line 63: 
Line 64: def _get_devices_from_vms(vmContainer):
Line 65:     """


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: vdsm: add functionality to detach host devices

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm: add functionality to detach host devices
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.ovirt.org/#/c/32389/3/vdsm/hostdev.py
File vdsm/hostdev.py:

Line 117: 
Line 118: def detach(device_name):
Line 119:     libvirt_device, device_params = _get_device_ref_and_params(device_name)
Line 120: 
Line 121:     if device_params['capability'] in _UNMANAGED_DEVICE_CAPS:
>  I'd refer to http://gerrit.ovirt.org/#/c/22462/24/vdsm/virt/vm.py
> (it is ol 
Do you expect that many users would need the semantics of "detach the
device if it's pci/usb, and skip otherwise"? If so, you can

  def detach_if_detachable()

that calls this detach() when needed. detach_if_detachable() can be defined here or at the
user.
Line 122:         libvirt_device.dettach()
Line 123: 


Line 118: def detach(device_name):
Line 119:     libvirt_device, device_params = _get_device_ref_and_params(device_name)
Line 120: 
Line 121:     if device_params['capability'] in _UNMANAGED_DEVICE_CAPS:
Line 122:         libvirt_device.dettach()
the spelling mistake of dettach/detach is a sore in my eye. Could you use detachFlags(0)
instead?
Line 123: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: vdsm: add functionality to detach host devices

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: add functionality to detach host devices
......................................................................


Patch Set 3:

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12928/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12770/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11979/ : FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: vdsm: add functionality to detach host devices

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: add functionality to detach host devices
......................................................................


Patch Set 2: Code-Review-1 Verified-1

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12922/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12764/ : UNSTABLE

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11973/ : FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Martin Polednik]

Martin Polednik has posted comments on this change.

Change subject: vdsm: add functionality to acquire host devices
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.ovirt.org/#/c/32389/1/vdsm/hostdev.py
File vdsm/hostdev.py:

Line 23: 
Line 24: from vdsm import libvirtconnection
Line 25: 
Line 26: 
Line 27: DETACHABLE_DEVICES = ('usb', 'pci')
>  these are capabilities, not devices, right? if so, please rename.
agreed
Line 28: 
Line 29: 
Line 30: def _parse_device_params(device_xml):
Line 31:     params = {}


Line 120: 
Line 121:     return devices
Line 122: 
Line 123: 
Line 124: def acquire(device_name):
>  What about this name? Libvirt calls the action "detach".
> Why should not we? 
This function is to be called inside device's constructor,
the difference is that the libvirt sees that stack from below:

[host [device1 device2 device3]] -> libvirt really does dettach the device, but from
VDSM's pov:

[libvirt [device1 device2 device3]] -> [vm1 vm2 vm3] here, we're acquiring the
device from libvirt and giving it to one of our VMs
Line 125:     device = libvirtconnection.get().nodeDeviceLookupByName(device_name)
Line 126:     device_params = _parse_device_params(device)
Line 127: 
Line 128:     if device_params['capability'] in DETACHABLE_DEVICES:


Line 124: def acquire(device_name):
Line 125:     device = libvirtconnection.get().nodeDeviceLookupByName(device_name)
Line 126:     device_params = _parse_device_params(device)
Line 127: 
Line 128:     if device_params['capability'] in DETACHABLE_DEVICES:
>  I'm a bit surprised: it is quite dangerous to silently ignore an
> action, ju 
Acquire will be called by any device - even non-detachable (at least to
get device_params) and we therefore have to limit devices for which the real detach is
called
Line 129:         device.dettach()
Line 130: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: vdsm: add functionality to detach host devices

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm: add functionality to detach host devices
......................................................................


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: vdsm: add functionality to detach host devices

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: add functionality to detach host devices
......................................................................


Patch Set 6:

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13835/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/13046/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13998/ : FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: vdsm: add functionality to detach host devices

[Martin Polednik]

Martin Polednik has posted comments on this change.

Change subject: vdsm: add functionality to detach host devices
......................................................................


Patch Set 6: Verified+1

Verified manually on hostdev enabled system

# ls /dev/vfio/
vfio
# virsh nodedev-detach pci_0000_05_10_0                                                   
                                                              
Device pci_0000_05_10_0 detached
# ls /dev/vfio/
22  vfio
# virsh nodedev-reattach pci_0000_05_10_0                                                 
                                                                                          
             
Device pci_0000_05_10_0 re-attached
# ls /dev/vfio/
vfio

# echo "detach_detachable('pci_0000_05_10_0')" >>
/usr/share/vdsm/hostdev.py
# python /usr/share/vdsm/hostdev.py
# ls /dev/vfio/
22  vfio

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: vdsm: add functionality to detach host devices

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: add functionality to detach host devices
......................................................................


Patch Set 7:

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master-libfapi_create-rpms-el6-x86_64_m... :
FAILURE

http://jenkins.ovirt.org/job/vdsm_master_create-rpms-fc21-x86_64_merged/264/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master-libfapi_create-rpms-fc20-x86_64_... :
SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_create-rpms_merged_test_debug/481/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master-libfapi_create-rpms-el7-x86_64_m... :
SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_create-rpms-fc20-x86_64_merged/279/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/4273/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_create-rpms-el6-x86_64_merged/286/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_create-rpms-el7-x86_64_merged/288/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master-libfapi_create-rpms-fc21-x86_64_... :
SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/6111/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm: add functionality to acquire host devices
......................................................................


Patch Set 1: Code-Review-1

(3 comments)

http://gerrit.ovirt.org/#/c/32389/1/vdsm/hostdev.py
File vdsm/hostdev.py:

Line 23: 
Line 24: from vdsm import libvirtconnection
Line 25: 
Line 26: 
Line 27: DETACHABLE_DEVICES = ('usb', 'pci')
these are capabilities, not devices, right? if so, please rename.

use underscore prefix if you see no need to expose this tuple out of this module.
Line 28: 
Line 29: 
Line 30: def _parse_device_params(device_xml):
Line 31:     params = {}


Line 120: 
Line 121:     return devices
Line 122: 
Line 123: 
Line 124: def acquire(device_name):
What about this name? Libvirt calls the action "detach". Why should not we?
Line 125:     device = libvirtconnection.get().nodeDeviceLookupByName(device_name)
Line 126:     device_params = _parse_device_params(device)
Line 127: 
Line 128:     if device_params['capability'] in DETACHABLE_DEVICES:


Line 124: def acquire(device_name):
Line 125:     device = libvirtconnection.get().nodeDeviceLookupByName(device_name)
Line 126:     device_params = _parse_device_params(device)
Line 127: 
Line 128:     if device_params['capability'] in DETACHABLE_DEVICES:
I'm a bit surprised: it is quite dangerous to silently ignore an action, just becuase
the input was not "detachable". A better option is to raise an error. Why should
we ignore silly requests to detach non-detachable devices?
Line 129:         device.dettach()
Line 130: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: add functionality to acquire host devices
......................................................................


Patch Set 1:

Build Successful 

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11284/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12226/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12073/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No