Change in vdsm[master]: Configurator module implementing pyroute2. So far it's missi...

asegurap at redhat.com asegurap at redhat.com
Wed Apr 16 13:33:42 UTC 2014


Antoni Segura Puimedon has posted comments on this change.

Change subject: Configurator module implementing pyroute2. So far it's missing ip route and addr flush functionality.
......................................................................


Patch Set 1: Code-Review-1

(20 comments)

-1 means that changes are needed ;-)

http://gerrit.ovirt.org/#/c/26846/1/vdsm/network/configurators/pyroute_two.py
File vdsm/network/configurators/pyroute_two.py:

Line 1: # Copyright 2013 Red Hat, Inc.
I believe this should be Copyright 2014 Robert Cernak
Line 2: #
Line 3: # This program is free software; you can redistribute it and/or modify
Line 4: # it under the terms of the GNU General Public License as published by
Line 5: # the Free Software Foundation; either version 2 of the License, or


Line 43:     '/usr/bin/ethtool',  # Arch
Line 44: )
Line 45: 
Line 46: 
Line 47: class PyrouteTwo(Configurator):
Most of this class is like vdsm/network/configurators/iproute2.py:Iproute2

It would make a lot of sense to inherit from that class instead of from Configurator and just override the methods/things that you need.
Line 48:     def __init__(self, inRollback=False):
Line 49:         super(PyrouteTwo, self).__init__(ConfigApplier(), inRollback)
Line 50:         self.runningConfig = RunningConfig()
Line 51: 


Line 141:         self.configApplier.ifdown(bridge)
Line 142:         self.configApplier.removeBridge(bridge)
Line 143:         if bridge.port:
Line 144:             bridge.port.remove()
Line 145:         self.configApplier.releaseSocket()
You should probably only release the socket when the configurator ends the transaction, not after each change.
Line 146:         logging.debug("testmark.PyR2.remBridge")
Line 147: 
Line 148:     def removeVlan(self, vlan):
Line 149:         self.configApplier.ifdown(vlan)


Line 219:         if ipConfig.ipaddr:
Line 220:             self.removeIpConfig(iface)
Line 221:             logging.debug("testmark.PyR2.sIpC: ip: %s , mask: %s",
Line 222:                           iface.name, iface.mask)
Line 223: #            self.ip.interfaces[iface.name]['address'] = ipConfig.ipaddr
Since these things need to be implemented (presumably in this same patch, it would be appropriate that the patch name would be preceded by a '[WIP]'.
Line 224: #            self.ip.interfaces[iface.name]['mask'] = ipConfig.netmask
Line 225: #            self.ip.interfaces[iface.name].commit()
Line 226:             dev = self.ip.nl.link_lookup(ifname=iface.name)[0]
Line 227:             self.ip.nl.addr('add', dev, address=ipConfig.ipaddr,


Line 244:     def setIfaceMtu(self, iface, mtu):
Line 245:         logging.debug("sIMtu: iface=%s, mtu=%s, mtuINT=%d",
Line 246:                       iface, mtu, int(mtu))
Line 247:         self.ip.interfaces[iface]['mtu'] = int(mtu)
Line 248:         self.ip.interfaces[iface].commit()
As a first approach it is okay to commit() on each change to the device. 

You might want to consider for future patches (certainly not this one) that the commit() is done after the device is wholly configured.
Line 249: #       ipwrapper.linkSet(iface, ['mtu', str(mtu)])
Line 250:         logging.debug("testmark.PyR2.ConfApplier.setIfaceMtu")
Line 251: 
Line 252:     def setBondingMtu(self, iface, mtu):


Line 255: 
Line 256:     def ifup(self, iface):
Line 257:         self.ip.interfaces[iface.name].up()
Line 258:         self.ip.interfaces[iface.name].commit()
Line 259: #       ipwrapper.linkSet(iface.name, ['up'])
Why is this commented out instead of removed?
Line 260:         if iface.ipConfig.bootproto == 'dhcp':
Line 261:             dhclient = DhcpClient(iface.name)
Line 262:             dhclient.start(iface.ipConfig.async)
Line 263:         logging.debug("testmark.PyR2.ConfApplier.ifup")


Line 265:     def ifdown(self, iface):
Line 266:         logging.debug("iDown: iface.name %s", iface.name)
Line 267:         self.ip.interfaces[iface.name].down()
Line 268:         self.ip.interfaces[iface.name].commit()
Line 269: #       ipwrapper.linkSet(iface.name, ['down'])
Why is this commented out instead of removed?
Line 270:         dhclient = DhcpClient(iface.name)
Line 271:         dhclient.shutdown()
Line 272:         logging.debug("testmark.PyR2.ConfApplier.ifdown")
Line 273: 


Line 280:         logging.debug("testmark.PyR2.ConfApplier.setIfaceConfigAndUp")
Line 281: 
Line 282:     def addBridge(self, bridge):
Line 283:         logging.debug("rB: bridge.name: %s , ip object: %s",
Line 284:                       bridge.name, self.ip.nl)
All these debug messages are a bit too much. You should debug more on failure cases and not for tracking application flow.
Line 285:         iface = self.ip.create(kind='bridge', ifname=bridge.name)
Line 286:         iface.commit()
Line 287: #       rc, _, err = execCmd([EXT_BRCTL, 'addbr', bridge.name])
Line 288: #       if rc != 0:


Line 282:     def addBridge(self, bridge):
Line 283:         logging.debug("rB: bridge.name: %s , ip object: %s",
Line 284:                       bridge.name, self.ip.nl)
Line 285:         iface = self.ip.create(kind='bridge', ifname=bridge.name)
Line 286:         iface.commit()
If an exception happens here you should raise ConfigNetworkError, not whichever error pyroute2 raises, so you should catch and re-raise the appropriate kind (with the added information from the pyroute2 exception).
Line 287: #       rc, _, err = execCmd([EXT_BRCTL, 'addbr', bridge.name])
Line 288: #       if rc != 0:
Line 289: #           raise ConfigNetworkError(ERR_FAILED_IFUP, err)
Line 290: 


Line 285:         iface = self.ip.create(kind='bridge', ifname=bridge.name)
Line 286:         iface.commit()
Line 287: #       rc, _, err = execCmd([EXT_BRCTL, 'addbr', bridge.name])
Line 288: #       if rc != 0:
Line 289: #           raise ConfigNetworkError(ERR_FAILED_IFUP, err)
This comments should be removed.
Line 290: 
Line 291:     def addBridgePort(self, bridge):
Line 292:         with self.ip.interfaces[bridge.name] as iface:
Line 293:             iface.begin()


Line 296:         logging.debug("testmarkPyR2.ConfApplier.addBridgePort")
Line 297:  #      rc, _, err = execCmd([EXT_BRCTL, 'addif', bridge.name,
Line 298:  #                            bridge.port.name])
Line 299:  #      if rc != 0:
Line 300:  #           raise ConfigNetworkError(ERR_FAILED_IFUP, err)
This comments should be removed.
Line 301: 
Line 302:     def removeBridge(self, bridge):
Line 303:         logging.debug("rB: bridge.name: %s , ip object: %s",
Line 304:                       bridge.name, self.ip.nl)


Line 306:         self.ip.interfaces[bridge.name].commit()
Line 307:         logging.debug("testmarkPyR2.ConfApplier.remBridge")
Line 308: #       rc, _, err = execCmd([EXT_BRCTL, 'delbr', bridge.name])
Line 309: #       if rc != 0:
Line 310: #           raise ConfigNetworkError(ERR_FAILED_IFDOWN, err)
This comments should be removed.
Line 311: 
Line 312:     def addVlan(self, vlan):
Line 313:         dev = self.ip.nl.link_lookup(ifname=vlan.device.name)[0]
Line 314:         iface = self.ip.create(kind='vlan', ifname=vlan.name,


Line 309: #       if rc != 0:
Line 310: #           raise ConfigNetworkError(ERR_FAILED_IFDOWN, err)
Line 311: 
Line 312:     def addVlan(self, vlan):
Line 313:         dev = self.ip.nl.link_lookup(ifname=vlan.device.name)[0]
This could probably be better as:

    dev, = self.ip.nl.link_lookup(ifname=vlan.device.name)

This way, if more than a device would be returned (which is unexpected) it would fail instead of silently going ahead.
Line 314:         iface = self.ip.create(kind='vlan', ifname=vlan.name,
Line 315:                                link=dev, vlan_id=vlan.tag)
Line 316:         iface.commit()
Line 317:         logging.debug("testmarkPyR2.ConfApplier.addVlan")


Line 315:                                link=dev, vlan_id=vlan.tag)
Line 316:         iface.commit()
Line 317:         logging.debug("testmarkPyR2.ConfApplier.addVlan")
Line 318: #       ipwrapper.linkAdd(name=vlan.name, linkType='vlan',
Line 319: #                         link=vlan.device.name, args=['id', str(vlan.tag)])
This comments should be removed.
Line 320: 
Line 321:     def removeVlan(self, vlan):
Line 322:         self.ip.interfaces[vlan.name].remove()
Line 323:         self.ip.interfaces[vlan.name].commit()


Line 321:     def removeVlan(self, vlan):
Line 322:         self.ip.interfaces[vlan.name].remove()
Line 323:         self.ip.interfaces[vlan.name].commit()
Line 324:         logging.debug("testmarkPyR2.ConfApplier.remVlan")
Line 325: #       ipwrapper.linkDel(vlan.name)
This comment should be removed.
Line 326: 
Line 327:     def addBond(self, bond):
Line 328:         if bond.name not in netinfo.bondings():
Line 329:             logging.debug('Add new bonding %s', bond)


Line 327:     def addBond(self, bond):
Line 328:         if bond.name not in netinfo.bondings():
Line 329:             logging.debug('Add new bonding %s', bond)
Line 330:             with open(netinfo.BONDING_MASTERS, 'w') as f:
Line 331:                 f.write('+%s' % bond.name)
You should check with peter saveliev if it is not possible to create it with pyroute2 instead. If I recall correctly, he made it in a way that if the kernel cannot create it with netlink, pyroute2 itself will write to BONDING_MASTERS so you don't have to do it.
Line 332:         logging.debug("testmarkPyR2.ConfApplier.addBond")
Line 333: 
Line 334:     def removeBond(self, bond):
Line 335:         if bond.name not in netinfo.REQUIRED_BONDINGS:


Line 334:     def removeBond(self, bond):
Line 335:         if bond.name not in netinfo.REQUIRED_BONDINGS:
Line 336:             logging.debug('Remove bonding %s', bond)
Line 337:             with open(netinfo.BONDING_MASTERS, 'w') as f:
Line 338:                 f.write('-%s' % bond.name)
Same as my previous comment.
Line 339:         logging.debug("testmarkPyR2.ConfApplier.remBond")
Line 340: 
Line 341:     def addBondSlave(self, bond, slave):
Line 342:         logging.debug('Add slave %s to bonding %s', slave, bond)


Line 341:     def addBondSlave(self, bond, slave):
Line 342:         logging.debug('Add slave %s to bonding %s', slave, bond)
Line 343:         self.ifdown(slave)
Line 344:         with open(netinfo.BONDING_SLAVES % bond.name, 'w') as f:
Line 345:             f.write('+%s' % slave.name)
Same as previous comments
Line 346:         self.ifup(slave)
Line 347:         logging.debug("testmarkPyR2.ConfApplier.addBondSlave")
Line 348: 
Line 349:     def removeBondSlave(self, bond, slave):


Line 348: 
Line 349:     def removeBondSlave(self, bond, slave):
Line 350:         logging.debug('Remove slave %s from bonding %s', slave, bond)
Line 351:         with open(netinfo.BONDING_SLAVES % bond.name, 'w') as f:
Line 352:             f.write('-%s' % slave.name)
Same as previous comments
Line 353:         logging.debug("testmarkPyR2.ConfApplier.remBondSlave")
Line 354: 
Line 355:     def addBondOptions(self, bond):
Line 356:         logging.debug('Add bond options %s', bond.options)


Line 356:         logging.debug('Add bond options %s', bond.options)
Line 357:         for option in bond.options.split():
Line 358:             key, value = option.split('=')
Line 359:             with open(netinfo.BONDING_OPT % (bond.name, key), 'w') as f:
Line 360:                 f.write(value)
You should check if they can be set with pyroute2 in a backwards compatible way.
Line 361:         logging.debug("testmarkPyR2.ConfApplier.addBondOptions")
Line 362: 
Line 363:     def createLibvirtNetwork(self, network, bridged, iface, qosInbound=None,
Line 364:                              qosOutbound=None):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I01ad024b245af95503a1d9463757d656f2a89918
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Robert Cernak <robocernak at gmail.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list