Antoni Segura Puimedon has posted comments on this change.
Change subject: hook: Cisco VM-FEX support
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(10 inline comments)
Not a final review yet, but some things to fix for both files (mostly
readability and one hiccup that might not exist with direct-pools libvirt
network object). I'll check the before_vm_start in more detail later.
....................................................
File vdsm_hooks/vmfex/before_vm_migrate_destination.py
Line 58: nics.append(i.split('/')[-2])
Line 59: return nics
Line 60:
Line 61: def createDirectPool(conn):
Line 62: xmlstr = """<network> \n <name>direct-pool</name> \n <forward
mode="passthrough"> \n """
The line is too long to pass PEP8. Other people might disagree, but I find xml
embedding in the code much easier to read when it is something like:
xmlstr = """<network>
<name>direct-pool</name>
<forward mode="passthrough">
"""
Alternatively, just use implicit string concatenation by splitting the line
towards the end before it reaches the PEP8 critical point.
Line 63: for i in getUsableNics():
Line 64: s = '<interface dev="' + str(i) + '"/> \n'
Line 65: xmlstr += s
Line 66:
Line 61: def createDirectPool(conn):
Line 62: xmlstr = """<network> \n <name>direct-pool</name> \n <forward
mode="passthrough"> \n """
Line 63: for i in getUsableNics():
Line 64: s = '<interface dev="' + str(i) + '"/> \n'
Line 65: xmlstr += s
The variable 's' is not necessary in my opinion.
you could just append to xmlstr and use string formatting for increased
readability.
Line 66:
Line 67: xmlstr += """ </forward> \n </network> """
Line 68: conn.networkDefineXML(xmlstr)
Line 69: dpool = conn.networkLookupByName('direct-pool')
Line 63: for i in getUsableNics():
Line 64: s = '<interface dev="' + str(i) + '"/> \n'
Line 65: xmlstr += s
Line 66:
Line 67: xmlstr += """ </forward> \n </network> """
I don't think triple quote is necessary in this case.
Line 68: conn.networkDefineXML(xmlstr)
Line 69: dpool = conn.networkLookupByName('direct-pool')
Line 70: dpool.setAutostart(1)
Line 71: dpool.create()
Line 69: dpool = conn.networkLookupByName('direct-pool')
Line 70: dpool.setAutostart(1)
Line 71: dpool.create()
Line 72:
Line 73: if os.environ.has_key('vmfex'):
I suggest using the more pythonic:
if 'vmfex' in os.environ:
Line 74: try:
Line 75: #connect to libvirtd
Line 76: conn = libvirtconnection.get('qemu:///system')
Line 77:
Line 75: #connect to libvirtd
Line 76: conn = libvirtconnection.get('qemu:///system')
Line 77:
Line 78: #check for running VMs. If there are VMs running, skip
creating a dNIC pool
Line 79: if len(conn.listDomainsID()) == 0:
it's simpler to just test:
if not conn.listDomainsID():
Line 80: #check if direct-pool is created
Line 81: if 'direct-pool' not in conn.listNetworks():
Line 82: createDirectPool(conn)
Line 83: else:
Line 80: #check if direct-pool is created
Line 81: if 'direct-pool' not in conn.listNetworks():
Line 82: createDirectPool(conn)
Line 83: else:
Line 84: if conn.networkLookupByName('direct-pool').isActive()
== 0:
Again, better to test 'not' than compare to zero.
Line 85: conn.networkLookupByName('direct-pool').create()
Line 86:
conn.networkLookupByName('direct-pool').setAutostart(1)
Line 87:
Line 88: #check if the defined dNIC pool didn't change
Line 87:
Line 88: #check if the defined dNIC pool didn't change
Line 89: dpool = conn.networkLookupByName('direct-pool')
Line 90: definedNics = []
Line 91: for i in dpool.split():
I didn't try with a directpool network, but for me
conn.networkLookupByName('default') does not return me an object in which I can
do split().
Line 92: if 'dev=' in i:
Line 93:
definedNics.append(str(i.split('=')[1].split('/')[0].strip("'")))
Line 94: #if the defined NIC pool doesn't match the usable NIC
list, recreate the network
Line 95: if set(definedNics) != set(getUsableNics()):
Line 93:
definedNics.append(str(i.split('=')[1].split('/')[0].strip("'")))
Line 94: #if the defined NIC pool doesn't match the usable NIC
list, recreate the network
Line 95: if set(definedNics) != set(getUsableNics()):
Line 96: conn.networkLookupByName('direct-pool').destroy()
Line 97: conn.networkLookupByName('direct-pool').undefine()
can't we use dpool instead of repeating the lookups?
Line 98: createDirectPool(conn)
Line 99:
Line 100: if 'direct-pool' not in conn.listNetworks():
Line 101: createDirectPool(conn)
....................................................
File vdsm_hooks/vmfex/before_vm_start.py
Line 38: </virtualport>
Line 39: <model type='virtio'/>
Line 40: </interface>
Line 41:
Line 42: Dynamic network with libvirt (define a NIC pool, so libvirt can assign
VMs to NICs dynamically):
Too long for PEP8.
Line 43:
Line 44: <network>
Line 45: <name>direct-pool</name>
Line 46: <forward mode="passthrough">
Line 143:
Line 144: try:
Line 145: #Get the vmfex line
Line 146: vmfex = os.environ['vmfex']
Line 147:
Whitespaces alert.
Line 148: #Get the VM's xml definition
Line 149: domxml = hooking.read_domxml()
Line 150: devices = domxml.getElementsByTagName('devices')[0]
Line 151: vnicmap = enumNics(devices)
--
To view, visit http://gerrit.ovirt.org/7547
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I45a7fa46919bb39a648dff190c40618395990e91
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Yasny <[email protected]>
Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]>
Gerrit-Reviewer: Dan Yasny <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
