Change in vdsm[master]: NetReload: Moved addNetwork logic to the configurator

danken at redhat.com danken at redhat.com
Wed Apr 10 19:45:28 UTC 2013


Dan Kenigsberg has posted comments on this change.

Change subject: NetReload: Moved addNetwork logic to the configurator
......................................................................


Patch Set 3: I would prefer that you didn't submit this

(6 inline comments)

....................................................
Commit Message
Line 12: configureBridge, configureVlan, configureBond and configureNic
Line 13: after this patch have to take care of the underlying devices
Line 14: explicitly in the configurator module. However, the underlying
Line 15: device handling will be abstracted to the network models in a
Line 16: future patch. Thus, the code of configureBond for IfcfgConfigurator
IfcfgConfigurator was renamed.
Line 17: is gonna be something like:
Line 18: 
Line 19: self.configWriter.addBonding(params)
Line 20: for slave in bond.slaves:


....................................................
File vdsm/configNetwork.py
Line 292:     if configWriter is None:
Line 293:         configWriter = ConfigWriter()
Line 294:     configurator = Ifcfg(configWriter)
Line 295: 
Line 296:     async = utils.tobool(options.get('blockingdhcp')) and \
double negation confuses me.

if we have 'dhcp' and 'blockingdhcp', we need to be synchronous, and not do the nasty async hack.
Line 297:         options.get('bootproto') == 'dhcp'
Line 298: 
Line 299:     # take down nics that need to be changed
Line 300:     vlanedIfaces = [v['iface'] for v in _netinfo.vlans.values()]


....................................................
File vdsm/netconf/ifcfg.py
Line 41: class Ifcfg(object):
Line 42:     # TODO: Do all the configWriter interaction from here.
Line 43:     def __init__(self, configWriter=None):
Line 44:         self.configWriter = configWriter
Line 45:         self.libvirtAdded = []
should it be public? does the order matter? I suspect

 self._libvirtAdded = set()

would be better.
Line 46: 
Line 47:     def begin(self):
Line 48:         if self.configWriter is None:
Line 49:             self.configWriter = ConfigWriter()


Line 51: 
Line 52:     def rollback(self):
Line 53:         if self.configWriter:
Line 54:             self.configWriter.restoreBackups()
Line 55:             # TODO: Add a try except with logging, but not raising.
Sorry, I lack the context to understand this TODO. Would you elaborate?
Line 56:             for network in self.libvirtAdded:
Line 57:                 self.configWriter.removeLibvirtNetwork(network)
Line 58: 
Line 59:             self.configWriter = None


Line 111:         if network:
Line 112:             self.configWriter.createLibvirtNetwork(network, False, iface)
Line 113:         return iface
Line 114: 
Line 115:     def configureBond(self, network, bonding=None, nics=None, ipaddr=None,
bonding=None ? who would want such a default?
Line 116:                       netmask=None, mtu=None, gateway=None, bridge=None,
Line 117:                       bondingOptions=None, async=False, **options):
Line 118:         mtu = self.configWriter.getMaxMtu(nics, mtu)
Line 119:         self.configWriter.addBonding(bonding, bridge=bridge,


Line 120:                                      bondingOptions=bondingOptions,
Line 121:                                      mtu=mtu, ipaddr=ipaddr,
Line 122:                                      netmask=netmask, gateway=gateway,
Line 123:                                      **options)
Line 124:         options['bootproto'] = None
wouldn't it be prettier to make bootproto a named argument? I do see why it should be hidden in **options.
Line 125:         for nic in nics:
Line 126:             self.configureNic(None, nic, bonding=bonding, mtu=mtu, **options)
Line 127: 
Line 128:         ifup(bonding, async)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I877eddc07a17f0bdd376df7bab8b19ffe52f72ba
Gerrit-PatchSet: 3
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: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list