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