1 a.m.
Mark Wu has uploaded a new change for review.
Change subject: Make vdsm/configNetwork.py PEP8 clean
......................................................................
Make vdsm/configNetwork.py PEP8 clean
Change-Id: I02be4c0cdb1868a8518a73e22dfb77adedc08f3f
Signed-off-by: Mark Wu <wudxw(a)linux.vnet.ibm.com>
---
M Makefile.am
M vdsm/configNetwork.py
2 files changed, 106 insertions(+), 42 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/58/7758/1
diff --git a/Makefile.am b/Makefile.am
index 40d7658..4900cc5 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -47,6 +47,7 @@
vdsm/blkid.py \
vdsm/caps.py \
vdsm/clientIF.py \
+ vdsm/configNetwork.py \
vdsm/constants.py.in \
vdsm/debugPluginClient.py \
vdsm/define.py \
diff --git a/vdsm/configNetwork.py b/vdsm/configNetwork.py
index 74d19c7..df807bf 100755
--- a/vdsm/configNetwork.py
+++ b/vdsm/configNetwork.py
@@ -17,7 +17,11 @@
# Refer to the README and COPYING files for full details of the license
#
-import sys, subprocess, os, re, traceback
+import sys
+import subprocess
+import os
+import re
+import traceback
import pipes
import pwd
import time
@@ -42,11 +46,13 @@
MAX_BRIDGE_NAME_LEN = 15
ILLEGAL_BRIDGE_CHARS = frozenset(':. \t')
+
class ConfigNetworkError(Exception):
def __init__(self, errCode, message):
self.errCode = errCode
self.message = message
Exception.__init__(self, self.errCode, self.message)
+
def ifdown(iface):
"Bring down an interface"
@@ -60,6 +66,7 @@
if not line.endswith(' does not exist!')]))
return p.returncode
+
def ifup(iface):
"Bring up an interface"
p = subprocess.Popen([constants.EXT_IFUP, iface], stdout=subprocess.PIPE,
@@ -69,6 +76,7 @@
logging.info(out)
if err.strip():
logging.warn(err)
+
def ifaceUsers(iface):
"Returns a list of entities using the interface"
@@ -87,8 +95,12 @@
users.add(v)
return users
+
def nicOtherUsers(bridge, vlan, bonding, nic):
- "Returns a list of interfaces using a nic, other than the specified one (used
for validation)"
+ """
+ Returns a list of interfaces using a nic,
+ other than the specified one (used for validation)
+ """
if bonding:
owner = bonding
elif vlan:
@@ -101,8 +113,12 @@
users.discard(owner)
return users
+
def bondingOtherUsers(bridge, vlan, bonding):
- "Return a list of nics/interfaces using a bonding, other than the specified one
(used for validation)"
+ """
+ Return a list of nics/interfaces using a bonding,
+ other than the specified one (used for validation)
+ """
if vlan:
owner = bonding + '.' + vlan
else:
@@ -123,6 +139,8 @@
# nicSort(["p33p2", "p33p10"]) => ["p33p10",
"p33p2"]
# nicSort(["p331", "p33p1"]) => ["p33p1",
"p331"]
#
+
+
def nicSort(nics):
"Return a list of nics/interfaces ordered by name"
@@ -141,6 +159,7 @@
nics_list.append((prefix, intidx, stridx + postfix))
return [x + z for x, y, z in sorted(nics_list)]
+
class ConfigWriter(object):
NET_CONF_PREF = netinfo.NET_CONF_DIR + 'ifcfg-'
@@ -274,7 +293,10 @@
self._persistentBackup(filename)
def _atomicBackup(self, filename):
- """Backs up configuration to memory, for a later rollback in case
of error."""
+ """
+ Backs up configuration to memory,
+ for a later rollback in case of error.
+ """
if filename not in self._backups:
try:
@@ -378,8 +400,9 @@
'disabled', exc_info=True)
def _createConfFile(self, conf, name, ipaddr=None, netmask=None,
- gateway=None, bootproto=None, mtu=None, onboot='yes', **kwargs):
+ gateway=None, bootproto=None, mtu=None, onboot='yes', **kwargs):
""" Create ifcfg-* file with proper fields per device
"""
+
cfg = """DEVICE=%s\nONBOOT=%s\n""" %
(pipes.quote(name),
pipes.quote(onboot))
cfg += conf
@@ -454,8 +477,9 @@
bootproto, mtu, onboot, **kwargs)
# create the bonding device to avoid initscripts noise
- if bonding not in open('/sys/class/net/bonding_masters').read().split():
- open('/sys/class/net/bonding_masters',
'w').write('+%s\n' % bonding)
+ bondMastersPath = '/sys/class/net/bonding_masters'
+ if bonding not in open(bondMastersPath).read().split():
+ open(bondMastersPath, 'w').write('+%s\n' % bonding)
def addNic(self, nic, bonding=None, bridge=None, mtu=None,
ipaddr=None, netmask=None, gateway=None, bootproto=None,
@@ -489,8 +513,8 @@
cf = self.NET_CONF_PREF + nic
self._backup(cf)
try:
- hwlines = [ line for line in open(cf).readlines()
- if line.startswith('HWADDR=') ]
+ hwlines = [line for line in open(cf).readlines()
+ if line.startswith('HWADDR=')]
l = ['DEVICE=%s\n' % nic, 'ONBOOT=yes\n'] + hwlines
open(cf, 'w').writelines(l)
except IOError:
@@ -528,8 +552,8 @@
its value or None if not found
"""
with open(conffile) as f:
- entries = [ line for line in f.readlines()
- if line.startswith(entry + '=') ]
+ entries = [line for line in f.readlines()
+ if line.startswith(entry + '=')]
if len(entries) != 0:
value = entries[0].split('=', 1)[1]
return value.strip()
@@ -554,8 +578,8 @@
the configuration file
"""
with open(conffile) as f:
- entries = [ line for line in f.readlines()
- if not line.startswith(entry + '=') ]
+ entries = [line for line in f.readlines()
+ if not line.startswith(entry + '=')]
if not delete:
entries.append('\n' + entry + '=' + value)
@@ -606,12 +630,14 @@
else:
return
- nics, delvlan, bonding = _netinfo.getNicsVlanAndBondingForNetwork(bridge)
+ nics, delvlan, bonding = (_netinfo
+ .getNicsVlanAndBondingForNetwork(bridge))
if delvlan is None:
return
if bonding:
- iface_bridged = _netinfo.getBridgedNetworksAndVlansForIface(bonding)
+ iface_bridged = (_netinfo
+ .getBridgedNetworksAndVlansForIface(bonding))
vlans = [v for (_, v) in iface_bridged]
iface = bonding
else:
@@ -641,14 +667,18 @@
cf = self.NET_CONF_PREF + nics[0]
self._updateConfigValue(cf, 'MTU', str(newmtu), newmtu is None)
+
def isBridgeNameValid(bridgeName):
return bridgeName and len(bridgeName) <= MAX_BRIDGE_NAME_LEN and \
len(set(bridgeName) & ILLEGAL_BRIDGE_CHARS) == 0 and \
not bridgeName.startswith('-')
+
def validateBridgeName(bridgeName):
if not isBridgeNameValid(bridgeName):
- raise ConfigNetworkError(ne.ERR_BAD_BRIDGE, "Bridge name isn't valid:
%r"%bridgeName)
+ raise ConfigNetworkError(ne.ERR_BAD_BRIDGE,
+ "Bridge name isn't valid: %r" %
bridgeName)
+
def _validateIpAddress(address):
try:
@@ -662,27 +692,36 @@
return False
return True
+
def validateIpAddress(ipAddr):
if not _validateIpAddress(ipAddr):
- raise ConfigNetworkError(ne.ERR_BAD_ADDR, "Bad IP address: %r"%ipAddr)
+ raise ConfigNetworkError(ne.ERR_BAD_ADDR,
+ "Bad IP address: %r" % ipAddr)
+
def validateNetmask(netmask):
if not _validateIpAddress(netmask):
- raise ConfigNetworkError(ne.ERR_BAD_ADDR, "Bad netmask: %r"%netmask)
+ raise ConfigNetworkError(ne.ERR_BAD_ADDR,
+ "Bad netmask: %r" % netmask)
+
def validateGateway(gateway):
if not _validateIpAddress(gateway):
- raise ConfigNetworkError(ne.ERR_BAD_ADDR, "Bad gateway: %r"%gateway)
+ raise ConfigNetworkError(ne.ERR_BAD_ADDR,
+ "Bad gateway: %r" % gateway)
+
def validateBondingName(bonding):
if not re.match('^bond[0-9]+$', bonding):
- raise ConfigNetworkError(ne.ERR_BAD_BONDING, '%r is not a valid bonding
device name' % bonding)
+ raise ConfigNetworkError(ne.ERR_BAD_BONDING,
+ '%r is not a valid bonding device name' % bonding)
+
def validateBondingOptions(bonding, bondingOptions):
'Example: BONDING_OPTS="mode=802.3ad miimon=150"'
try:
for option in bondingOptions.split():
- key,value = option.split('=')
+ key, value = option.split('=')
if not os.path.exists('/sys/class/net/%(bonding)s/bonding/%(key)s'
% locals()):
raise ConfigNetworkError(ne.ERR_BAD_BONDING,
@@ -691,12 +730,15 @@
raise ConfigNetworkError(ne.ERR_BAD_BONDING,
"Error parsing bonding options: %r" % bondingOptions)
+
def validateVlanId(vlan):
try:
if not 0 <= int(vlan) <= MAX_VLAN_ID:
- raise ConfigNetworkError(ne.ERR_BAD_VLAN, 'vlan id out of range: %r, must
be 0..%s' % (vlan, MAX_VLAN_ID))
+ raise ConfigNetworkError(ne.ERR_BAD_VLAN,
+ 'vlan id out of range: %r, must be 0..%s' % (vlan, MAX_VLAN_ID))
except ValueError:
raise ConfigNetworkError(ne.ERR_BAD_VLAN, 'vlan id must be a number')
+
def _validateInterNetworkCompatibility(ni, vlan, iface, bridged):
"""
@@ -737,6 +779,7 @@
raise ConfigNetworkError(ne.ERR_BAD_PARAMS,
"interface %r already has networks" % \
(iface))
+
def _addNetworkValidation(_netinfo, network, vlan, bonding, nics, ipaddr,
netmask, gateway, bondingOptions, bridged=True,
@@ -817,9 +860,11 @@
else:
_validateInterNetworkCompatibility(_netinfo, vlan, nic, bridged)
+
def addNetwork(network, vlan=None, bonding=None, nics=None, ipaddr=None,
netmask=None, mtu=None, gateway=None, force=False,
- configWriter=None, bondingOptions=None, bridged=True, **options):
+ configWriter=None, bondingOptions=None, bridged=True,
+ **options):
nics = nics or ()
_netinfo = netinfo.NetInfo()
bridged = utils.tobool(bridged)
@@ -831,9 +876,9 @@
if not utils.tobool(force):
logging.debug('validating network...')
_addNetworkValidation(_netinfo, network=network,
- vlan=vlan, bonding=bonding, nics=nics, ipaddr=ipaddr,
- netmask=netmask, gateway=gateway, bondingOptions=bondingOptions,
- bridged=bridged, **options)
+ vlan=vlan, bonding=bonding, nics=nics, ipaddr=ipaddr,
+ netmask=netmask, gateway=gateway, bondingOptions=bondingOptions,
+ bridged=bridged, **options)
logging.info("Adding network %s with vlan=%s, bonding=%s, nics=%s,"
" bondingOptions=%s, mtu=%s, bridged=%s, options=%s",
@@ -932,6 +977,7 @@
# add libvirt network
configWriter.createLibvirtNetwork(network, bridged, iface)
+
def assertBridgeClean(bridge, vlan, bonding, nics):
brifs = os.listdir('/sys/class/net/%s/brif/' % bridge)
for nic in nics:
@@ -949,7 +995,9 @@
pass
if brifs:
- raise ConfigNetworkError(ne.ERR_USED_BRIDGE, 'bridge %s has interfaces %s
connected' % (bridge, brifs))
+ raise ConfigNetworkError(ne.ERR_USED_BRIDGE,
+ 'bridge %s has interfaces %s connected' % (bridge, brifs))
+
def showNetwork(network):
_netinfo = netinfo.NetInfo()
@@ -975,12 +1023,14 @@
print "vlan=%s, bonding=%s, nics=%s" % (vlan, bonding, nics)
+
def listNetworks():
_netinfo = netinfo.NetInfo()
print "Networks:", _netinfo.networks.keys()
print "Vlans:", _netinfo.vlans.keys()
print "Nics:", _netinfo.nics.keys()
print "Bondings:", _netinfo.bondings.keys()
+
def delNetwork(network, vlan=None, bonding=None, nics=None, force=False,
configWriter=None, implicitBonding=True, **options):
@@ -1069,6 +1119,7 @@
ifdown(name)
ifup(name)
+
def clientSeen(timeout):
start = time.time()
while timeout >= 0:
@@ -1079,19 +1130,23 @@
return False
-def editNetwork(oldBridge, newBridge, vlan=None, bonding=None, nics=None, **options):
+def editNetwork(oldBridge, newBridge, vlan=None, bonding=None, nics=None,
+ **options):
configWriter = ConfigWriter()
try:
delNetwork(oldBridge, configWriter=configWriter, **options)
- addNetwork(newBridge, vlan=vlan, bonding=bonding, nics=nics,
configWriter=configWriter, **options)
+ addNetwork(newBridge, vlan=vlan, bonding=bonding, nics=nics,
+ configWriter=configWriter, **options)
except:
configWriter.restoreBackups()
raise
if utils.tobool(options.get('connectivityCheck', False)):
- if not clientSeen(int(options.get('connectivityTimeout',
CONNECTIVITY_TIMEOUT_DEFAULT))):
+ if not clientSeen(int(options.get('connectivityTimeout',
+ CONNECTIVITY_TIMEOUT_DEFAULT))):
delNetwork(newBridge, force=True)
configWriter.restoreBackups()
return define.errCode['noConPeer']['status']['code']
+
def _validateNetworkSetup(networks={}, bondings={}):
_netinfo = netinfo.NetInfo()
@@ -1099,7 +1154,8 @@
for network, networkAttrs in networks.iteritems():
if networkAttrs.get('remove', False):
if set(networkAttrs) - set(['remove']):
- raise ConfigNetworkError(ne.ERR_BAD_PARAMS, "Cannot specify any
attribute when removing")
+ raise ConfigNetworkError(ne.ERR_BAD_PARAMS,
+ "Cannot specify any attribute when removing")
for bonding, bondingAttrs in bondings.iteritems():
validateBondingName(bonding)
@@ -1108,14 +1164,17 @@
if bondingAttrs.get('remove', False):
if bonding not in _netinfo.bondings:
- raise ConfigNetworkError(ne.ERR_BAD_BONDING, 'Cannot remove bonding
%s: Doesn\'t exist' % bonding)
+ raise ConfigNetworkError(ne.ERR_BAD_BONDING,
+ 'Cannot remove bonding %s: Doesn\'t exist' %
bonding)
continue
nics = bondingAttrs.get('nics', None)
if not nics:
- raise ConfigNetworkError(ne.ERR_BAD_PARAMS, "Must specify nics for
bonding")
+ raise ConfigNetworkError(ne.ERR_BAD_PARAMS,
+ "Must specify nics for bonding")
if not set(nics).issubset(set(_netinfo.nics)):
- raise ConfigNetworkError(ne.ERR_BAD_NIC, "Unknown nics in:
%r"%list(nics))
+ raise ConfigNetworkError(ne.ERR_BAD_NIC,
+ "Unknown nics in: %r" % list(nics))
def _editBondings(bondings, configWriter):
@@ -1166,6 +1225,7 @@
for nic in nicSort(bondAttrs['nics']):
ifup(nic)
+
def _removeBondings(bondings, configWriter):
""" Remove bond interface """
logger = logging.getLogger("_removeBondings")
@@ -1191,7 +1251,7 @@
Params:
networks - dict of key=network, value=attributes
- where 'attributes' is a dict with the following optional items:
+ where 'attributes' is a dict with the following optional items:
vlan=<id>
bonding="<name>" | nic="<name>"
(bonding and nics are mutually exclusive)
@@ -1206,7 +1266,7 @@
remove=True (other attributes can't be specified)
bondings - dict of key=bonding, value=attributes
- where 'attributes' is a dict with the following optional items:
+ where 'attributes' is a dict with the following optional items:
nics=["<nic1>" , "<nic2>", ...]
options="<bonding-options>"
-- OR --
@@ -1283,9 +1343,8 @@
elif d['bonding'] in _ni.bondings:
logger.debug("Updating bond %r info",
d['bonding'])
d['nics'] =
_ni.bondings[d['bonding']]['slaves']
- d['bondingOptions'] = \
-
_ni.bondings[d['bonding']]['cfg'].get('BONDING_OPTS',
- None)
+ d['bondingOptions'] =
(_ni.bondings[d['bonding']]
+ ['cfg'].get('BONDING_OPTS',
None))
else:
d['nics'] = [d.pop('nic')]
d['force'] = force
@@ -1313,16 +1372,19 @@
logger.error(e, exc_info=True)
raise
+
def setSafeNetworkConfig():
"""Declare current network configuration as
'safe'"""
subprocess.Popen([constants.EXT_VDSM_STORE_NET_CONFIG])
+
def usage():
print """Usage:
./configNetwork.py add Network <attributes> <options>
edit oldNetwork newNetwork <attributes> <options>
del Network <options>
- setup Network [None|attributes] [++ Network [None|attributes] [++
...]] [:: <options>]
+ setup Network [None|attributes] \
+[++ Network [None|attributes] [++ ...]] [:: <options>]
attributes = [vlan=...] [bonding=...] [nics=<nic1>,...]
options = [Force=<True|False>]
[bridged=<True|False>]...
@@ -1336,6 +1398,7 @@
API.Global.translateNetOptionsToNew(kwargs)
return kwargs
+
def main():
if len(sys.argv) <= 1:
@@ -1367,9 +1430,9 @@
kwargs['nics'] = kwargs['nics'].split(',')
editNetwork(oldBridge, newBridge, **kwargs)
elif sys.argv[1] == 'setup':
- batchCommands, options = utils.listSplit( sys.argv[2:], '::', 1 )
+ batchCommands, options = utils.listSplit(sys.argv[2:], '::', 1)
d = {}
- for batchCommand in utils.listSplit( batchCommands, '++' ):
+ for batchCommand in utils.listSplit(batchCommands, '++'):
d[batchCommand[0]] = _parseKwargs(batchCommand[1:]) or None
setupNetworks(d, **_parseKwargs(options))
elif sys.argv[1] == 'show':
--
To view, visit http://gerrit.ovirt.org/7758
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I02be4c0cdb1868a8518a73e22dfb77adedc08f3f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
1:37 a.m.
Dan Kenigsberg has submitted this change and it was merged.
Change subject: Make vdsm/configNetwork.py PEP8 clean
......................................................................
Make vdsm/configNetwork.py PEP8 clean
Change-Id: I02be4c0cdb1868a8518a73e22dfb77adedc08f3f
Signed-off-by: Mark Wu <wudxw(a)linux.vnet.ibm.com>
---
M Makefile.am
M vdsm/configNetwork.py
2 files changed, 124 insertions(+), 58 deletions(-)
Approvals:
Dan Kenigsberg: Verified; Looks good to me, approved
--
To view, visit http://gerrit.ovirt.org/7758
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I02be4c0cdb1868a8518a73e22dfb77adedc08f3f
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com>
1:37 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: Make vdsm/configNetwork.py PEP8 clean
......................................................................
Patch Set 4: Verified
verified using pydiff that no new discrepancies have been introduced by my rebase.
--
To view, visit http://gerrit.ovirt.org/7758
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I02be4c0cdb1868a8518a73e22dfb77adedc08f3f
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com>
1:33 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: Make vdsm/configNetwork.py PEP8 clean
......................................................................
Patch Set 4: Looks good to me, approved
sorry, I had to rebase due to recent changes by Igor and myself.
--
To view, visit http://gerrit.ovirt.org/7758
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I02be4c0cdb1868a8518a73e22dfb77adedc08f3f
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com>
4:30 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: Make vdsm/configNetwork.py PEP8 clean
......................................................................
Patch Set 3: Looks good to me, approved
--
To view, visit http://gerrit.ovirt.org/7758
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I02be4c0cdb1868a8518a73e22dfb77adedc08f3f
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com>
12:35 a.m.
Zhou Zheng Sheng has posted comments on this change.
Change subject: Make vdsm/configNetwork.py PEP8 clean
......................................................................
Patch Set 3: Verified; Looks good to me, but someone else must approve
Run through autobuild.sh.
pydiff reports 10 differences, all are acceptable. The detailed reports is as follow:
10 difference(s)
first file: configNetworko.py
second file: configNetwork.py
((20, ('subprocess', None)), ('20 + -', None))
((20, ('os', None)), ('20 + -', None))
((20, ('re', None)), ('20 + -', None))
((20, ('traceback', None)), ('20 + -', None))
(('20 +', None), (21, Import([('subprocess', None)])))
(('20 +', None), (22, Import([('os', None)])))
(('20 +', None), (23, Import([('re', None)])))
(('20 +', None), (24, Import([('traceback', None)])))
((457,
"If([(Compare(Name('bonding'), [('not in',
CallFunc(Getattr(CallFunc(Getattr(CallFunc(Name('open'),
[Const('/sys/class/net/bonding_masters')], None, None), 'read'), [], None,
None), 'split'), [], None, None))]),
Stmt([Discard(CallFunc(Getattr(CallFunc(Name('open'),
[Const('/sys/class/net/bonding_masters'), Const('w')], None, None),
'write'), [Mod((Const('+%s\\n'), Name('bonding')))], None,
None))]))], None)"),
(481,
"Assign([AssName('bondMastersPath', 'OP_ASSIGN')],
Const('/sys/class/net/bonding_masters'))"))
(('457 +', None),
(482,
If([(Compare(Name('bonding'), [('not in',
CallFunc(Getattr(CallFunc(Getattr(CallFunc(Name('open'),
[Name('bondMastersPath')], None, None), 'read'), [], None, None),
'split'), [], None, None))]),
Stmt([Discard(CallFunc(Getattr(CallFunc(Name('open'),
[Name('bondMastersPath'), Const('w')], None, None), 'write'),
[Mod((Const('+%s\n'), Name('bonding')))], None, None))]))], None)))
First 8 differences are about splitting long import into pieces. Last two are about
defining a variable for a long string. They are all acceptable.
--
To view, visit http://gerrit.ovirt.org/7758
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I02be4c0cdb1868a8518a73e22dfb77adedc08f3f
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com>
1:41 a.m.
Mark Wu has posted comments on this change.
Change subject: Make vdsm/configNetwork.py PEP8 clean
......................................................................
Patch Set 3:
You give a lot of good suggestions on what the tool doesn't cover. I have fixed it as
you suggest. It makes the code more clean. Thanks for your review!
--
To view, visit http://gerrit.ovirt.org/7758
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I02be4c0cdb1868a8518a73e22dfb77adedc08f3f
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com>
3:36 a.m.
Mark Wu has posted comments on this change.
Change subject: Make vdsm/configNetwork.py PEP8 clean
......................................................................
Patch Set 1:
I am going to submit a new patch to improve the style.
--
To view, visit http://gerrit.ovirt.org/7758
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I02be4c0cdb1868a8518a73e22dfb77adedc08f3f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com>
1:07 a.m.
Mark Wu has posted comments on this change.
Change subject: Make vdsm/configNetwork.py PEP8 clean
......................................................................
Patch Set 1:
I separate pep8 clean patch from the refactor patch to make it easy to review.
--
To view, visit http://gerrit.ovirt.org/7758
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I02be4c0cdb1868a8518a73e22dfb77adedc08f3f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com>
12:58 a.m.
Zhou Zheng Sheng has posted comments on this change.
Change subject: Make vdsm/configNetwork.py PEP8 clean
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(16 inline comments)
....................................................
File vdsm/configNetwork.py
Line 437: gateway=None, bootproto=None, delay='0',
onboot='yes', **kwargs):
Line 438: """ Create ifcfg-* file with proper fields for bridge
"""
Line 439: conf = 'TYPE=Bridge\nDELAY=%s\n' % pipes.quote(delay)
Line 440: self._createConfFile(conf, name, ipaddr, netmask, gateway,
Line 441: bootproto, mtu, onboot, **kwargs)
Seems too much indent. Is it better we let "bootproto" align with
"conf"?
Line 442:
Line 443: def addVlan(self, vlanId, iface, network, mtu=None, bridged=True,
Line 444: ipaddr=None, netmask=None, gateway=None, bootproto=None,
Line 445: onboot='yes', **kwargs):
Line 579: the configuration file
Line 580: """
Line 581: with open(conffile) as f:
Line 582: entries = [line for line in f.readlines()
Line 583: if not line.startswith(entry + '=')]
Maybe "if" can align with "line" in the upper line.
Line 584:
Line 585: if not delete:
Line 586: entries.append('\n' + entry + '=' + value)
Line 587:
Line 671:
Line 672: def isBridgeNameValid(bridgeName):
Line 673: return bridgeName and len(bridgeName) <= MAX_BRIDGE_NAME_LEN and \
Line 674: len(set(bridgeName) & ILLEGAL_BRIDGE_CHARS) == 0 and \
Line 675: not bridgeName.startswith('-')
Seems parenthesis can be used here.
return (bridgeName and len(bridgeName) <= MAX_BRIDGE_NAME_LEN and
len(set(bridgeName) & ILLEGAL_BRIDGE_CHARS) == 0 and
not bridgeName.startswith('-'))
Line 676:
Line 677:
Line 678: def validateBridgeName(bridgeName):
Line 679: if not isBridgeNameValid(bridgeName):
Line 724: try:
Line 725: for option in bondingOptions.split():
Line 726: key, value = option.split('=')
Line 727: if not
os.path.exists('/sys/class/net/%(bonding)s/bonding/%(key)s'
Line 728: % locals()):
How about
if not os.path.exists(
'/sys/class/net/%(bonding)s/bonding/%(key)s' % locals()):
Line 729: raise ConfigNetworkError(ne.ERR_BAD_BONDING,
Line 730: "%r is not a valid bonding option" % key)
Line 731: except ValueError:
Line 732: raise ConfigNetworkError(ne.ERR_BAD_BONDING,
Line 737: try:
Line 738: if not 0 <= int(vlan) <= MAX_VLAN_ID:
Line 739: raise ConfigNetworkError(ne.ERR_BAD_VLAN,
Line 740: 'vlan id out of range: %r, must be
0..%s'
Line 741: % (vlan, MAX_VLAN_ID))
Usually the line breaks after a binary operation like '%'.
Can I suggest:
raise ConfigNetworkError(
ne.ERR_BAD_VLAN, 'vlan id out of range: %r, must be 0..%s' %
(vlan, MAX_VLAN_ID))
Line 742: except ValueError:
Line 743: raise ConfigNetworkError(ne.ERR_BAD_VLAN, 'vlan id must be a
number')
Line 744:
Line 745:
Line 757: # is a non-VLANed network over our iface
Line 758: for (iface_net, iface_vlan) in ifaces:
Line 759: if iface_vlan is None:
Line 760: raise ConfigNetworkError(ne.ERR_BAD_PARAMS,
Line 761: "interface %r already member of network
%r" % \
This backslash is redundant.
Line 762: (iface, iface_net))
Line 763:
Line 764: ifaces_bridgeless = tuple(ni.getBridgelessNetworksAndVlansForIface(iface))
Line 765: ifaces_bridged = tuple(ni.getBridgedNetworksAndVlansForIface(iface))
Line 779: # Want to add non-VLANed bridged network,
Line 780: # check whether interface is empty
Line 781: elif ifaces_bridged or ifaces_bridgeless:
Line 782: raise ConfigNetworkError(ne.ERR_BAD_PARAMS,
Line 783: "interface %r already has networks" % \
This backslash is redundant.
Line 784: (iface))
Line 785:
Line 786:
Line 787: def _addNetworkValidation(_netinfo, network, vlan, bonding, nics, ipaddr,
Line 844: # still relevant if bonding is None
Line 845: bondingForNics = _netinfo.getBondingForNic(nic)
Line 846: if bondingForNics and bondingForNics != bonding:
Line 847: raise ConfigNetworkError(ne.ERR_USED_NIC,
Line 848: "nic %s already enslaved to %s" % \
This backslash is redundant.
Line 849: (nic, bondingForNics))
Line 850:
Line 851: # Make sure nics don't used by vlans if bond requested
Line 852: if bonding:
Line 852: if bonding:
Line 853: vlansForNic = tuple(_netinfo.getVlansForNic(nic))
Line 854: if vlansForNic:
Line 855: raise ConfigNetworkError(ne.ERR_USED_NIC,
Line 856: "nic %s already used by vlans %s"
% \
This backslash is redundant.
Line 857: (nic, vlansForNic))
Line 858: networksForNic = tuple(_netinfo.getNetworksForIface(nic))
Line 859: if networksForNic:
Line 860: raise ConfigNetworkError(ne.ERR_USED_NIC,
Line 857: (nic, vlansForNic))
Line 858: networksForNic = tuple(_netinfo.getNetworksForIface(nic))
Line 859: if networksForNic:
Line 860: raise ConfigNetworkError(ne.ERR_USED_NIC,
Line 861: "nic %s already used by networks
%s" % \
This backslash is redundant.
Line 862: (nic, networksForNic))
Line 863: else:
Line 864: _validateInterNetworkCompatibility(_netinfo, vlan, nic, bridged)
Line 865:
Line 906: ifdown(nic)
Line 907:
Line 908: if bridged:
Line 909: configWriter.addBridge(network, ipaddr=ipaddr, netmask=netmask,
Line 910: mtu=mtu, gateway=gateway, **options)
Could "mtu" align with "network"?
Line 911: ifdown(network)
Line 912: # We need to define (if requested) ip, mask & gateway on ifcfg-*
Line 913: # only on most top device according to following order:
Line 914: # bridge -> vlan -> bond -> nic
Line 940: configWriter.addBonding(bonding, bridge=bridgeForNic,
Line 941: bondingOptions=bondingOptions,
Line 942: mtu=max(prevmtu, mtu),
Line 943: ipaddr=ipaddr, netmask=netmask,
Line 944: gateway=gateway, **options)
Can it be
configWriter.addBonding(bonding, bridge=bridgeForNic,
bondingOptions=bondingOptions,
mtu=max(prevmtu, mtu),
ipaddr=ipaddr, netmask=netmask,
gateway=gateway, **options)
Line 945: # reset ip, netmask, gateway for lower level devices
Line 946: ipaddr = netmask = gateway = None
Line 947:
Line 948: for nic in nics:
Line 1066: if bonding:
Line 1067: validateBondingName(bonding)
Line 1068: if set(nics) !=
set(_netinfo.bondings[bonding]["slaves"]):
Line 1069: raise ConfigNetworkError(ne.ERR_BAD_NIC,
Line 1070: "delNetwork: %s are not all nics enslaved to
%s" % \
This backslash is redundant.
Line 1071: (nics, bonding))
Line 1072: if vlan:
Line 1073: validateVlanId(vlan)
Line 1074: if bridged:
Line 1144: configWriter.restoreBackups()
Line 1145: raise
Line 1146: if utils.tobool(options.get('connectivityCheck', False)):
Line 1147: if not clientSeen(int(options.get('connectivityTimeout',
Line 1148: CONNECTIVITY_TIMEOUT_DEFAULT))):
Is it better
if not clientSeen(int(options.get('connectivityTimeout',
CONNECTIVITY_TIMEOUT_DEFAULT))):
Line 1149: delNetwork(newBridge, force=True)
Line 1150: configWriter.restoreBackups()
Line 1151: return
define.errCode['noConPeer']['status']['code']
Line 1152:
Line 1187: _netinfo = netinfo.NetInfo()
Line 1188:
Line 1189: for bond, bondAttrs in bondings.iteritems():
Line 1190: logger.debug("Creating/Editing bond %s with attributes %s",
Line 1191: bond, bondAttrs)
Can "bond" align with "Creating" ?
Line 1192:
Line 1193: brNets = list(_netinfo.getBridgedNetworksForIface(bond))
Line 1194: # Only one bridged-non-VLANed network allowed on same nic/bond
Line 1195: bridge = brNets[0] if brNets else None
Line 1346: elif d['bonding'] in _ni.bondings:
Line 1347: logger.debug("Updating bond %r info",
d['bonding'])
Line 1348: d['nics'] =
_ni.bondings[d['bonding']]['slaves']
Line 1349: d['bondingOptions'] =
(_ni.bondings[d['bonding']]
Line 1350:
['cfg'].get('BONDING_OPTS', None))
How about
d['bondingOptions'] = \
_ni.bondings[d['bonding']]['cfg'].get(
'BONDING_OPTS', None))
Line 1351: else:
Line 1352: d['nics'] = [d.pop('nic')]
Line 1353: d['force'] = force
Line 1354:
--
To view, visit http://gerrit.ovirt.org/7758
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I02be4c0cdb1868a8518a73e22dfb77adedc08f3f
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com>
4281
days inactive
4285
days old
vdsm-patches@lists.fedorahosted.org
9 comments
3 participants
participants (3)
-
Dan Kenigsberg -
wudxw＠linux.vnet.ibm.com -
zhshzhou＠linux.vnet.ibm.com