Change in vdsm[master]: Add iproute2 configurator

asegurap at redhat.com asegurap at redhat.com
Thu Dec 12 13:16:38 UTC 2013


Antoni Segura Puimedon has posted comments on this change.

Change subject: Add iproute2 configurator
......................................................................


Patch Set 22: Code-Review-1

(5 comments)

Some minor things related to compatibility in error messages.

....................................................
File vdsm/netconf/iproute2.py
Line 32: from vdsm.netconfpersistence import RunningConfig
Line 33: from vdsm.utils import execCmd, CommandPath
Line 34: 
Line 35: 
Line 36: _BRCTL = CommandPath('brctl', '/usr/sbin/brctl')
The path is defined in constants.EXT_BRCTL
Line 37: 
Line 38: 
Line 39: class Iproute2(Configurator):
Line 40:     def __init__(self, inRollback=False):


Line 77:         self.runningConfig.setBonding(
Line 78:             bond.name, {'options': bond.options,
Line 79:                         'nics': [slave.name for slave in bond.slaves]})
Line 80: 
Line 81:     def editBonding(self, bond, _netinfo):
I would like to keep the same docstring as in ifcfg.py:
        """
        Modifies the bond so that the bond in the system ends up with the
        same slave and options configuration that are requested. Makes a
        best effort not to interrupt connectivity.
        """
Line 82:         nicsToSet = frozenset(nic.name for nic in bond.slaves)
Line 83:         currentNics = frozenset(_netinfo.getNicsForBonding(bond.name))
Line 84:         nicsToAdd = nicsToSet
Line 85:         nicsToRemove = currentNics


Line 212: 
Line 213:     def addBridge(self, bridge):
Line 214:         rc, _, err = execCmd([_BRCTL.cmd, 'addbr', bridge.name])
Line 215:         if rc != 0:
Line 216:             raise ConfigNetworkError(ERR_USED_BRIDGE, err)
I think the current neterror code that is closer to what happens here is:
ERR_FAILED_IFUP. In the future we should have more fine grained errors though.
Line 217: 
Line 218:     def addBridgePort(self, bridge):
Line 219:         rc, _, err = execCmd([_BRCTL.cmd, 'addif', bridge.name,
Line 220:                               bridge.port.name])


Line 218:     def addBridgePort(self, bridge):
Line 219:         rc, _, err = execCmd([_BRCTL.cmd, 'addif', bridge.name,
Line 220:                               bridge.port.name])
Line 221:         if rc != 0:
Line 222:             raise ConfigNetworkError(ERR_USED_BRIDGE, err)
I think the current neterror code that is closer to what happens here is:
FAILED_IFUP. In the future we should have more fine grained errors though.
Line 223: 
Line 224:     def removeBridge(self, bridge):
Line 225:         rc, _, err = execCmd([_BRCTL.cmd, 'delbr', bridge.name])
Line 226:         if rc != 0:


Line 223: 
Line 224:     def removeBridge(self, bridge):
Line 225:         rc, _, err = execCmd([_BRCTL.cmd, 'delbr', bridge.name])
Line 226:         if rc != 0:
Line 227:             raise ConfigNetworkError(ERR_USED_BRIDGE, err)
This doesn't raise right now with ifcfg. We simply swallow ifdown erroneous rc. If we want to raise, and I tend to agree with it, we should add a ERR_FAILED_IFDOWN
Line 228: 
Line 229:     def addVlan(self, vlan):
Line 230:         ipwrapper.linkAdd(name=vlan.name, linkType='vlan',
Line 231:                           link=vlan.device.name, args=['id', str(vlan.tag)])


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I762cfa30f78c5a46507b86f53e98bcf79dfc5844
Gerrit-PatchSet: 22
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: Assaf Muller <amuller at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Itamar Heim <iheim at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Petr Ĺ ebek <psebek at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list