Change in vdsm[master]: Don't down bonds and nics when adding vlan or resizing

wudxw at linux.vnet.ibm.com wudxw at linux.vnet.ibm.com
Tue Aug 13 08:19:14 UTC 2013


Mark Wu has posted comments on this change.

Change subject: Don't down bonds and nics when adding vlan or resizing
......................................................................


Patch Set 7: Code-Review-1

(5 comments)

....................................................
File lib/vdsm/netinfo.py
Line 223:             with open(path) as optFile:
Line 224:                 optName = os.path.basename(path)
Line 225:                 for optValue in optFile.read().rstrip().split(' '):
Line 226:                     opts.append((optName, optValue))
Line 227:     return frozenset(opts)
I prefer this alternative.
Line 228: 
Line 229: 
Line 230: def ports(bridge):
Line 231:     return os.listdir('/sys/class/net/' + bridge + '/brif')


Line 223:             with open(path) as optFile:
Line 224:                 optName = os.path.basename(path)
Line 225:                 for optValue in optFile.read().rstrip().split(' '):
Line 226:                     opts.append((optName, optValue))
Line 227:     return frozenset(opts)
There're many bonding options you can tune for a bonding. You could check them out under /sys/class/net/bondX/bonding.  Even though only a few of them is frequently used, but we still need support the other less used options. So namedtuple is not a good choice.
Line 228: 
Line 229: 
Line 230: def ports(bridge):
Line 231:     return os.listdir('/sys/class/net/' + bridge + '/brif')


....................................................
File vdsm/netconf/ifcfg.py
Line 102: 
Line 103:         # Create bond configuration in case it was a non ifcfg controlled bond
Line 104:         # Needed to be before slave configuration for initscripts to add slave
Line 105:         # to bond.
Line 106:         is_ifcfg_controlled = netinfo.getIfaceCfg(bond.name)
Is it more clear to add another function to check if an interface is controlled?
you could simply check if the ifcfg exists.
Line 107:         are_options_applied = bond.are_options_applied()
Line 108:         if not is_ifcfg_controlled or not are_options_applied:
Line 109:             bridgeName = _netinfo.getBridgedNetworkForIface(bond.name)
Line 110:             if bridgeName:


Line 105:         # to bond.
Line 106:         is_ifcfg_controlled = netinfo.getIfaceCfg(bond.name)
Line 107:         are_options_applied = bond.are_options_applied()
Line 108:         if not is_ifcfg_controlled or not are_options_applied:
Line 109:             bridgeName = _netinfo.getBridgedNetworkForIface(bond.name)
if it's not vdsm controlled, you could skip the check of bridge master.
Line 110:             if bridgeName:
Line 111:                 bond.master = Bridge(bridgeName, self, port=bond)
Line 112:             self.configApplier.addBonding(bond)
Line 113: 


Line 116:                 ifdown(slave.name)  # nics must be down to join a bond
Line 117:                 self.configApplier.addNic(slave)
Line 118:                 ifup(slave.name)
Line 119: 
Line 120:         if not is_ifcfg_controlled or not are_options_applied:
why does the case of is_ifcfg_controlled need re-activate the bond device?
Line 121:             ifdown(bond.name)
Line 122:             ifup(bond.name)
Line 123: 
Line 124:     def configureNic(self, nic, **opts):


-- 
To view, visit http://gerrit.ovirt.org/17491
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id3075a2e65f06416c840c6a98689b77555b22e5d
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Giuseppe Vallarelli <gvallare at redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Petr Ĺ ebek <psebek at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list