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 gkotton@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com

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 gkotton@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server