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