Mark Wu has posted comments on this change.
Change subject: NetReload: netmodels for addNetwork
......................................................................
Patch Set 4: I would prefer that you didn't submit this
(8 inline comments)
Antoni, I am very sorry for the long delay.
....................................................
File tests/configNetworkTests.py
Line 52: self.assertEqual(cneContext.exception.errCode, errCode)
Line 53:
Line 54: # Monkey patch the real network detection from the netinfo module.
Line 55: @MonkeyPatch(netinfo, 'networks', _fakeNetworks)
Line 56: @MonkeyPatch(netinfo, 'getMaxMtu', lambda *x: 1500)
I don't understand why you need monkey patch the following the functions. They
will not be invoked in the following tests.
Line 57: @MonkeyPatch(netconf.ifcfg, 'ifdown', lambda *x:
raiseInvalidOpException())
Line 58: @MonkeyPatch(netconf.ifcfg, 'ifup', lambda *x:
raiseInvalidOpException())
Line 59: @MonkeyPatch(Bond, 'configure', lambda *x:
raiseInvalidOpException())
Line 60: @MonkeyPatch(Bridge, 'configure', lambda *x:
raiseInvalidOpException())
....................................................
File vdsm/configNetwork.py
Line 76: bootproto=bootproto)
Line 77: return topObj
Line 78:
Line 79:
Line 80: def objectivizeBond(bonding, bondingOptions, nics, mtu, _netinfo,
+1
Line 81: configurator):
Line 82: if bonding and nics:
Line 83: slaves = []
Line 84: bondMtu = netinfo.getMaxMtu(nics, mtu)
Line 80: def objectivizeBond(bonding, bondingOptions, nics, mtu, _netinfo,
Line 81: configurator):
Line 82: if bonding and nics:
Line 83: slaves = []
Line 84: bondMtu = netinfo.getMaxMtu(nics, mtu)
the nics are unused. why do you need get a maximum mtu of them?
Line 85: for nic in nics:
Line 86: _validateNicUnused(nic, _netinfo)
Line 87: slaves.append(Nic(nic, configurator, mtu=bondMtu))
Line 88: elif bonding in _netinfo.bondings: # Implicit bonding.
Line 86: _validateNicUnused(nic, _netinfo)
Line 87: slaves.append(Nic(nic, configurator, mtu=bondMtu))
Line 88: elif bonding in _netinfo.bondings: # Implicit bonding.
Line 89: nics = [nic for nic in _netinfo.getNicsForBonding(bonding)]
Line 90: bondMtu = netinfo.getMaxMtu(nics, mtu)
for an existing bond, the mtu of its slaves should be same as the bond. I think
we don't need compare the slave's mtu.
What we need to care about is the new mtu should be maximum of the value given
in API and the existing value, because it could be multiplexed by another
network with a bigger mtu.
So here we just compare the mtu of existing bonding and given mtu.
Line 91: slaves = [Nic(nic, configurator, mtu=bondMtu) for nic in nics]
Line 92: bondingOptions = _netinfo.bondings[bonding]['cfg'].get(
Line 93: 'BONDING_OPTS')
Line 94: else:
Line 119: if nic not in _netinfo.nics:
Line 120: raise ConfigNetworkError(ne.ERR_BAD_NIC, 'unknown nic: %r' %
nic)
Line 121: elif nicBond and nicBond != bonding:
Line 122: raise ConfigNetworkError(ne.ERR_USED_NIC, 'nic %s already
enslaved to '
Line 123: '%s' % (nic, nicBond))
It looks you need getMaxMtu here too if the nic is not used as a slave of bond
Line 124: return Nic(nic, configurator, mtu=mtu)
Line 125:
Line 126:
Line 127: def _validateInterNetworkCompatibility(ni, vlan, iface, bridged):
....................................................
File vdsm/netconf/ifcfg.py
Line 70: netmask = bridge.ip.inet.netmask
Line 71: gateway = bridge.ip.inet.gateway
Line 72: bootproto = bridge.ip.bootproto
Line 73: except AttributeError:
Line 74: ipaddr = netmask = gateway = bootproto = None
I suggest add a base class for all network entities and add a function to the
base class to get the tuple of (ipaddr, netmask, gateway, bootproto). Then all
entities can share the code. It could make the code a little bit cleaner.
Line 75: self.configWriter.addBridge(bridge.name, ipaddr=ipaddr,
Line 76: netmask=netmask, mtu=bridge.mtu,
Line 77: gateway=gateway,
bootproto=bootproto,
Line 78: **opts)
Line 74: ipaddr = netmask = gateway = bootproto = None
Line 75: self.configWriter.addBridge(bridge.name, ipaddr=ipaddr,
Line 76: netmask=netmask, mtu=bridge.mtu,
Line 77: gateway=gateway,
bootproto=bootproto,
Line 78: **opts)
Do you think it's even cooler to make configWriter.addXXX use netmodel object
as its argument? Or you prefer to change it in a follow up patch?
Line 79: ifdown(bridge.name)
Line 80: for port in bridge.ports:
Line 81: port.configure(bridge=bridge.name, **opts)
Line 82: ifup(bridge.name, bootproto == 'dhcp' and
....................................................
File vdsm/netmodels.py
Line 165: logging.debug('Removing bond %r with nics = %s', self.name,
Line 166: self.slaves)
Line 167: self.configurator.removeBond(self.name, self.slaves)
Line 168:
Line 169: @staticmethod
any reason using staticmethod instead of classmethod in class Bond? I prefer
to stick on one choice.
Line 170: def validateName(name):
Line 171: if not re.match('^bond[0-9]+$', name):
Line 172: raise ConfigNetworkError(ne.ERR_BAD_BONDING,
Line 173: '%r is not a valid bonding
device name' %
--
To view, visit http://gerrit.ovirt.org/14303
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba5c5b84760e27245cbe34c3b290c54e51278e72
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegu...@redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Giuseppe Vallarelli <gvall...@redhat.com>
Gerrit-Reviewer: Mark Wu <wu...@linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
