Change in vdsm[master]: NetReload: netmodels for delNetwork

asegurap at redhat.com asegurap at redhat.com
Mon May 27 21:03:12 UTC 2013


Antoni Segura Puimedon has posted comments on this change.

Change subject: NetReload: netmodels for delNetwork
......................................................................


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

(5 inline comments)

Just a couple of simplifications that might be worth it.

....................................................
File vdsm/configNetwork.py
Line 60:     if bonding:
Line 61:         topObj = Bond.objectivize(bonding, configurator, bondingOptions, nics,
Line 62:                                   mtu, _netinfo)
Line 63:         if opts.get('implicitBonding'):
Line 64:             topObj.destroyOnRemove = True
Since implicitBonding is something that is checked for everytime, I'd maybe add it to the method parameters (default None).
Line 65:     elif nics:
Line 66:         try:
Line 67:             nic, = nics
Line 68:         except ValueError:


Line 239: 
Line 240: 
Line 241: def _validateDelNetwork(network, vlan, bonding, nics, bridged,  _netinfo):
Line 242:     if bonding:
Line 243:         Bond.validateName(bonding)
Wouldn't this check be automatically done at the Bonding objectivization step already?
Line 244:         if set(nics) != set(_netinfo.bondings[bonding]["slaves"]):
Line 245:             raise ConfigNetworkError(ne.ERR_BAD_NIC, 'delNetwork: %s are '
Line 246:                                      'not all nics enslaved to %s' %
Line 247:                                      (nics, bonding))


Line 245:             raise ConfigNetworkError(ne.ERR_BAD_NIC, 'delNetwork: %s are '
Line 246:                                      'not all nics enslaved to %s' %
Line 247:                                      (nics, bonding))
Line 248:     if vlan:
Line 249:         Vlan.validateTag(vlan)
Same as comment in line 243.
Line 250:     if bridged:
Line 251:         assertBridgeClean(network, vlan, bonding, nics)
Line 252: 
Line 253: 


Line 252: 
Line 253: 
Line 254: def _delNonVdsmNetwork(network, vlan, bonding, nics, configWriter):
Line 255:     if network in netinfo.bridges():
Line 256:         configWriter.removeBridge(network)
Here we'll probably have to change it to use configurator.removeBridge.
Line 257:     else:
Line 258:         raise ConfigNetworkError(ne.ERR_BAD_BRIDGE, "Cannot delete network"
Line 259:                                  " %r: It doesn't exist in the system" %
Line 260:                                  network)


Line 273:     configurator = Ifcfg(configWriter)
Line 274: 
Line 275:     if network not in _netinfo.networks:
Line 276:         logging.info("Network %r: doesn't exist in libvirt database", network)
Line 277:         _delNonVdsmNetwork(network, vlan, bonding, nics)
You forgot to pass configWriter, right? In any case, like I said before I'd substitute it for configurator.
Line 278:         return
Line 279: 
Line 280:     nics, vlan, bonding = _netinfo.getNicsVlanAndBondingForNetwork(network)
Line 281:     bridged = _netinfo.networks[network]['bridged']


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9e11fd923a4f5d07d069193278f3eadc94f327ec
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Giuseppe Vallarelli <gvallare 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