Change in vdsm[master]: [WIP] netwiring: [4/4] Add API definitions.

danken at redhat.com danken at redhat.com
Wed Dec 5 20:45:02 UTC 2012


Dan Kenigsberg has posted comments on this change.

Change subject: [WIP] netwiring: [4/4] Add API definitions.
......................................................................


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

(12 inline comments)

....................................................
File vdsm/API.py
Line 360:             return {'status': {'code': errCode['MissParam']['status']['code'],
Line 361:                                'message': 'Missing one of required '
Line 362:                                           'parameters: deviceType'}}
Line 363:         try:
Line 364:             curVm = self._cif.vmContainer[self._UUID]
all over this file we use "v" for this vm. let's conform.
Line 365:         except KeyError:
Line 366:             self.log.warning("vm %s doesn't exist", self._UUID)
Line 367:             return errCode['noVM']
Line 368: 


....................................................
File vdsm/define.py
Line 140: #exitCodes
Line 141: ERROR = 1
Line 142: NORMAL = 0
Line 143: 
Line 144: DUMMY_BRIDGE = 'vdsm-none-br'
please use a bridge name that is less likely to be requested by a user. If we're working with brctl directly, we could choose something like "::::::::" (we prohibit the colon as a bridge name so as not to confuse ifcfg-*).

This fake network is an implementation detail, and it should be filterered out from the netinfo reports.


....................................................
File vdsm/dummybr.py
Line 22: import logging
Line 23: import os
Line 24: import sys
Line 25: 
Line 26: from vdsm import libvirtconnection, define, utils
I hate the "define" module. It's way-too-general name is reason enough not to use it for the implementation detail that is the name of our ephemeral bridge.

I'd prefer to put the constant in this module, or maybe netinfo.
Line 27: 
Line 28: 
Line 29: def createEphemeralBridge(bridgeName):
Line 30:     rc, out, err = utils.execCmd(['brctl', 'addbr', bridgeName])


Line 26: from vdsm import libvirtconnection, define, utils
Line 27: 
Line 28: 
Line 29: def createEphemeralBridge(bridgeName):
Line 30:     rc, out, err = utils.execCmd(['brctl', 'addbr', bridgeName])
we have constant.BRCTL for 'brctl'
Line 31:     if rc != 0:
Line 32:         logging.error('Failed to create ephemeral dummy bridge')
Line 33:         sys.exit(rc)
Line 34: 


Line 28: 
Line 29: def createEphemeralBridge(bridgeName):
Line 30:     rc, out, err = utils.execCmd(['brctl', 'addbr', bridgeName])
Line 31:     if rc != 0:
Line 32:         logging.error('Failed to create ephemeral dummy bridge')
logging stdout/err may be informational here.
Line 33:         sys.exit(rc)
Line 34: 
Line 35: if __name__ == '__main__':
Line 36:     if not os.path.exists('/sys/class/net/%s' % define.DUMMY_BRIDGE):


Line 29: def createEphemeralBridge(bridgeName):
Line 30:     rc, out, err = utils.execCmd(['brctl', 'addbr', bridgeName])
Line 31:     if rc != 0:
Line 32:         logging.error('Failed to create ephemeral dummy bridge')
Line 33:         sys.exit(rc)
it's better practice not to call sys.exit() outside main(). You never know if you'd want to call the function in another use case.
Line 34: 
Line 35: if __name__ == '__main__':
Line 36:     if not os.path.exists('/sys/class/net/%s' % define.DUMMY_BRIDGE):
Line 37:         createEphemeralBridge(define.DUMMY_BRIDGE)


Line 32:         logging.error('Failed to create ephemeral dummy bridge')
Line 33:         sys.exit(rc)
Line 34: 
Line 35: if __name__ == '__main__':
Line 36:     if not os.path.exists('/sys/class/net/%s' % define.DUMMY_BRIDGE):
please remind me - why are we not calling a nic-less configNetwork.addNetwork() instead of re-implementing it here?
Line 37:         createEphemeralBridge(define.DUMMY_BRIDGE)
Line 38: 
Line 39:     conn = libvirtconnection.get()
Line 40:     if define.DUMMY_BRIDGE not in conn.listNetworks():


....................................................
File vdsm/libvirtvm.py
Line 1524: 
Line 1525:     def updateInterfaceDevice(self, params):
Line 1526:         # Find the interface to update.
Line 1527:         netDev = None
Line 1528:         for dev in self._devices[vm.NIC_DEVICES][:]:
how about adding a _lookupDeviceByAlias()  function?
Line 1529:             if dev.alias == params['alias']:
Line 1530:                 netDev = dev
Line 1531:                 break
Line 1532:         if netDev is None:


Line 1537:                      'message': 'Missing net device'}}
Line 1538: 
Line 1539:         # Find the interface conf to potentially use in rollbacks.
Line 1540:         netConf = None
Line 1541:         for dev in self.conf['devices'][:]:
I suppose that it was much nicer if each device held a reference to its conf dictionary. then this repeated search could have been avoided.
Line 1542:             if dev['type'] == vm.NIC_DEVICES and \
Line 1543:                     dev['alias'] == params['alias']:
Line 1544:                 netConf = dev
Line 1545:                 break


Line 1593:             source.setAttribute('bridge', netParam)
Line 1594:             link.setAttribute('state', linkValue)
Line 1595: 
Line 1596:         try:
Line 1597:             self._dom.updateDeviceFlags(vnicXML.toxml(encoding='utf-8'),
do we have a shred of an evidence that performing this in two operations has any benefit to a guest? Can a Linux guest even differentiate between the suggested code, and one that uses a single updateDeviceFlags()?

would you be kind to have one patch where this is done in a single command, and add the complexity later?
Line 1598:                                         libvirt.libvirt.VIR_DOMAIN_AFFECT_LIVE)
Line 1599:         except:
Line 1600:             self.log.debug('Request to set the the vnic %s failed. '
Line 1601:                            'Made with the following xml: %s',


Line 1626:                 # In case we fail, we rollback the Network mirroring.
Line 1627:                 for mirrNet in mirroredNetworks:
Line 1628:                     supervdsm.getProxy().unsetPortMirroring(mirrNet,
Line 1629:                                                             netDev['name'])
Line 1630:                 # And also rollback link and network.
no rollback of the conf here?

I think that we urgently need to split this function to:

 findNicDev()
 findNicConf()
 setlink()
 try:
   setNet()
   try:
     setPortMirror
   except:
     rollbackPortMirror
     raise
  except:
    rollbackNet
    raise
 except:
   rollbackLink
   raise

context managers could have been even cooler

  with linkchange:
     with netchange:
        with mirrorchange:
           updateconf
           return success

but that would be an overkill, I suppose. However, I'm sure that the code would look much better if you raise exceptions instead of returning API error code. The translation to error code would be easier if done once, in a final

  except MyUpdateDeviceException:
      return {'status'....}
Line 1631:                 source.setAttribute('bridge', netConfBackup['network'])
Line 1632:                 link.setAttribute(
Line 1633:                     'state', 'up' if netConfBackup['linkActive'] else 'down')
Line 1634:                 self._dom.updateDeviceFlags(


....................................................
File vdsm/vdsmd.init.in
Line 479:         log_failure_msg "$prog: Failed to define network filters on libvirt"
Line 480:         return $ret_val
Line 481:     fi
Line 482: 
Line 483:     python @VDSMDIR@/dummybr.pyc > /dev/null 2>&1
I have no idea why we've disposed of the possibly-useful stderr in nwfilter.pyc. I think it would be nice to have an informational error report.
Line 484:     ret_val=$?
Line 485:     if [ $ret_val -ne 0 ]
Line 486:     then
Line 487:         log_failure_msg "$prog: Failed to create ephemeral dummy bridge."


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3b9b4f49f80466a83e3e13f1042ac2a8866c6bcd
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Alona Kaplan <alkaplan at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky at redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: ShaoHe Feng <shaohef at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list