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