Change in vdsm[master]: Qunatum POC changes

Gary Kotton has uploaded a new change for review.

Change subject: Qunatum POC changes
......................................................................

Qunatum POC changes

Change-Id: I992a0dd278daefa27e87b696c584bd7cd590c78e
Signed-off-by: Gary Kotton 
---
M .gitignore
M vdsm/libvirtvm.py
A vdsm/quantum.py
3 files changed, 144 insertions(+), 29 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/43/4043/1
--
To view, visit http://gerrit.ovirt.org/4043
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I992a0dd278daefa27e87b696c584bd7cd590c78e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gary Kotton 
Gerrit-Reviewer: Federico Simoncelli

Saggi Mizrahi has posted comments on this change.

Change subject: Qunatum POC changes
......................................................................

Patch Set 1: I would prefer that you didn't submit this

I know it's a POC but it would be benfitial for the final product if you wrap all the exec calls in easy to use python modules for each process. It would at least make things more readable. :)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I992a0dd278daefa27e87b696c584bd7cd590c78e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gary Kotton 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Qunatum POC changes
......................................................................

Patch Set 1: I would prefer that you didn't submit this

(13 inline comments)

I've managed to give you 0 quantum-related comments: everything below is about style or code design, nothing about flow.

....................................................
File vdsm/libvirtvm.py
Line 922:         if hasattr(self, 'q_plugin'):
few people in this forum may crucify me for this, but how about quantum.QuantumNIC inheriting from NetworkInterfaceDevice ?

Line 966:         if hasattr(self, 'q_plugin'):
inheriting would give you the opportunity to have your own docstring to show how domxml is going to look like.

Line 999:             if hasattr(self, 'bootOrder'):
too much repeated code

Line 2063:             if hasattr(nic, 'q_plugin'):
we should consider what happens if vdsm abruptly dies beforethis cleanup - someone has to collect all the orphan tap devices. though iirc there was an option for non-persistent ones.

....................................................
File vdsm/quantum.py
Line 2: # Copyright 2009-2011 Red Hat, Inc.
I would swear in court that this file did not exist in 2009.

Line 23: class Quantum(object):
seems like this is a QuantumVif object, right?

Line 25:         self.log = log
if you do not intend a data variable to be accessible outside of the object, please declare so by naming it self._log.

Line 32:         self.log.debug("Quantum : %s : %s/%s/%s", self.plugin, self.q_network_id,
no need for the "Quantum :" prefix, as the module name is logged and gives that hint already.

Line 34:         if plugin == 'LinuxBridge':
this string is part of the api of the module (and frankly, of Vdsm). please define it as a module CONSTANT.

Line 36:             self.vifDelete = self.vifDeleteLinuxBridge
it seems that you are re-implementing inheritance here. How about inheriting from QuantumVif, and having a factory function return the actual object according to the plugin arg?

Line 43:     def vifAdd(self, vmUuid, maddAddr):
it's python - if you call a method on an object that misses it, the caller should expect to die. Failing with a log may not be any better.

oh, and ethernet addresses makes me madd, too ;-)

Line 57:         command = ["/usr/bin/ovs-vsctl", "--", "--may-exist", "add-port", "br-int", self.deviceName,
please keep lines shorter than 80 chars (and add new modules to pep8whitelist).

Line 66:         utils.execCmd(command, sudo=True)
this requires adding /usr/bin/ovs-vsctl to vdsm's sudoers file.
we very much prefer adding a specific verb to supervdsm.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I992a0dd278daefa27e87b696c584bd7cd590c78e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gary Kotton 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Qunatum POC changes
......................................................................

Patch Set 1: No score

Build Started http://jenkins.ovirt.info/job/vdsm_unit_tests_by_patch/82/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I992a0dd278daefa27e87b696c584bd7cd590c78e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gary Kotton 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Qunatum POC changes
......................................................................

Patch Set 1:

Build Successful

http://jenkins.ovirt.info/job/vdsm_unit_tests_by_patch/82/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I992a0dd278daefa27e87b696c584bd7cd590c78e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gary Kotton 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has abandoned this change.

Change subject: Qunatum POC changes
......................................................................

Abandoned

We've taken the hook way for integration.