Antoni Segura Puimedon has posted comments on this change.
Change subject: NetReload: netmodels for addNetwork
......................................................................
Patch Set 4: (6 inline comments)
....................................................
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)
The reason why I have to monkey patch them is because when something goes
wrong, e.g., you made a change to the validation code that lets an erroneous
case through to the configuration applying stage, you want the test to abort
without affecting the testing machine (nothing would happen if the tests are
not run as root, but if you do, you can mess up your dev machine networking).
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 50:
Line 51: def objectivizeNetwork(net, bridged=None, vlan=None, bonding=None,
Line 52: bondingOptions=None, nics=None, mtu=None,
ipaddr=None,
Line 53: netmask=None, gateway=None, bootproto=None,
Line 54: _netinfo=None, configurator=None, **opts):
In Python the default arguments are not processed on each call, but on module
loading. Thus, this change that you propose would make that all the calls would
share an instance instead of getting a new one each time (like the current code
does).
Line 55: if configurator is None:
Line 56: configurator = Ifcfg()
Line 57: if _netinfo is None:
Line 58: _netinfo = netinfo.NetInfo()
....................................................
File vdsm/netconf/ifcfg.py
Line 63: if self.configWriter:
Line 64: self.configWriter = None
Line 65: self._libvirtAdded = set()
Line 66:
Line 67: def configureBridge(self, bridge, network=None, **opts):
Different configurators are the reason for that. I will check other
possibilities for this that maintain the ability to use multiple configurators.
Line 68: try:
Line 69: ipaddr = bridge.ip.inet.address
Line 70: netmask = bridge.ip.inet.netmask
Line 71: gateway = bridge.ip.inet.gateway
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)
I think that is a good idea. I wanted to leave configWriter untouched, but this
single change makes sense. Thanks for proposing it.
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 118: def __repr__(self):
Line 119: return 'Bridge(' + self.name + ')' + '\n|-' + '\n|-'.join(
Line 120: [repr(port) for port in self.ports])
Line 121:
Line 122: def configure(self, **opts):
It is passed through as part of the opts, but since "explicit is better than
implicit" I'll change that.
Line 123: self.configurator.configureBridge(self, **opts)
Line 124:
Line 125: def remove(self, force=False):
Line 126: logging.debug('Removing bridge %r with ports = %s', self.name,
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
Just that it is not using class information for anything and thus the class
explicit parameter would be useless. I'm not opposed to changing it to class
method though...
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
