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