Change in vdsm[master]: hooks: Add OpenStack Network vNIC hook

gvallare at redhat.com gvallare at redhat.com
Sat Jun 22 10:18:07 UTC 2013


Giuseppe Vallarelli has posted comments on this change.

Change subject: hooks: Add OpenStack Network vNIC hook
......................................................................


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

(6 inline comments)

Almost ready a few fixes see comments

....................................................
File configure.ac
Line 227: 	vdsm_hooks/pincpu/Makefile
Line 228: 	vdsm_hooks/promisc/Makefile
Line 229: 	vdsm_hooks/qemucmdline/Makefile
Line 230: 	vdsm_hooks/qos/Makefile
Line 231: 	vdsm_hooks/openstacknet/Makefile
You should move this line after vdsm_hooks/numa/Makefile as you did in a very similar case (I missed it in the previous patchset).
Line 232: 	vdsm_hooks/scratchpad/Makefile
Line 233: 	vdsm_hooks/smbios/Makefile
Line 234: 	vdsm_hooks/sriov/Makefile
Line 235: 	vdsm_hooks/vhostmd/Makefile


....................................................
File vdsm/hooking.py
Line 39: import os
Line 40: import sys
Line 41: from xml.dom import minidom
Line 42: 
Line 43: from storage.misc import execCmd
It's not related to the changeset I guess we should not depend on storage here. In the vdsm.utils there's an execCmd result of a recent refactoring.
The storage's execCmd is a 'wrapper' around utils.execCmd.
Short answer from vdsm.utils import execCmd.
Line 44: from vdsm.utils import tobool
Line 45: 
Line 46: # make pyflakes happy
Line 47: execCmd


Line 55: def write_domxml(domxml):
Line 56:     file(os.environ['_hook_domxml'], 'w').write(domxml.toxml(encoding='utf-8'))
Line 57: 
Line 58: 
Line 59: def exit_hook(message, return_code=2):
It's not clear to me why you're using stderr in both cases when an error happens and when it doesn't happen (a different return_code can be provided). My understanding is that you want to use this function mostly in case of failures.
Line 60:     """
Line 61:     Exit the hook with a given message, which will be printed to the standard
Line 62:     error stream. A newline will be printed at the end.
Line 63:     The default return code is 2 for signaling that an error occurred.


....................................................
File vdsm_hooks/openstacknet/after_device_create.py
Line 41:     tapName = ('tap' + portId)[:DEV_MAX_LENGTH]
Line 42:     command = ['/usr/sbin/brctl', 'delif', DUMMY_BRIDGE, tapName]
Line 43:     retcode, out, err = hooking.execCmd(command, sudo=True, raw=True)
Line 44: 
Line 45:     # On some systems, brctl is not at the above location (i.e. Ubuntu)
In distutils.spawn there's an handy function called find_executable that given executable name returns its path. It may be useful in this case, give it a try.
Line 46:     if retcode == 127:
Line 47:         command[0] = '/sbin/brctl'
Line 48:         retcode, out, err = hooking.execCmd(command, sudo=True, raw=True)
Line 49:     if retcode != 0:


Line 46:     if retcode == 127:
Line 47:         command[0] = '/sbin/brctl'
Line 48:         retcode, out, err = hooking.execCmd(command, sudo=True, raw=True)
Line 49:     if retcode != 0:
Line 50:         hooking.exit_hook("Can't disconnect %s from %s, due to: %s"
Watching it's usage I'm not sure exit_hook is an appropriate name, it's more related to next steps to do in case of failure.
Line 51:                           % (tapName, DUMMY_BRIDGE, err))
Line 52: 
Line 53: 
Line 54: def main():


....................................................
File vdsm_hooks/openstacknet/before_device_create.py
Line 18: 
Line 19: import os
Line 20: import sys
Line 21: import traceback
Line 22: import argparse
Nitpick argparse before os.
Line 23: from xml.dom import minidom
Line 24: 
Line 25: import hooking
Line 26: from vdsm.netinfo import DUMMY_BRIDGE


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I744b83d9c6027bd817e5d4171a77a005611b9818
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mike Kolesnik <mkolesni at redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.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: Livnat Peer <lpeer at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Mike Kolesnik <mkolesni at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list