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 <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Igor Lvovsky <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
_______________________________________________
vdsm-patches mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/vdsm-patches
