Change in vdsm[master]: Cisco VM-FEX support vdsm hooks

danken at redhat.com danken at redhat.com
Wed Sep 12 19:32:39 UTC 2012


Dan Kenigsberg has posted comments on this change.

Change subject: Cisco VM-FEX support vdsm hooks
......................................................................


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

(8 inline comments)

I have several stylistic comments inside. The most difficult issue is to avoid concurrently running createDirectPool. I think createDirectPool should look like

with file('/var/run/vdsm/hook-vmfex.lock', 'w') as f:
    fcntl.flock(f.fileno(), fcntl.LOCK_EX)
    try:
       # something that should happen only once, host-wide
    finally:
        fcntl.flock(f.fileno(), fcntl.LOCK_UN)

....................................................
File vdsm_hooks/vmfex/before_vm_migrate_destination.py
Line 53:     nics = []
Line 54:     cmd = "find /sys/devices/pci* | grep '/net/' |grep address"
Line 55:     p = subprocess.Popen(cmd, stdout=subprocess.PIPE, shell=True)
Line 56:     s = p.stdout.read().split()
Line 57:     for i in s:
I think using glob would be quicker and safer

 for i in glob.iglob('/sys/devices/pci0000:00/*/net/*/address'):
Line 58:         mac = open(i).read().strip()
Line 59:         if mac == '00:00:00:00:00:00':
Line 60:             nics.append(i.split('/')[-2])
Line 61:     return nics


Line 65:     xmlstr = '''<network>
Line 66:        <name>direct-pool</name>
Line 67:        <forward mode="passthrough">
Line 68:     '''
Line 69:     for i in getUsableNics():
"i" seems a bad naming for nicName. or nic.
Line 70:         xmlstr += '<interface dev="' + str(i) + '"/> \n'
Line 71: 
Line 72:     xmlstr += ' </forward> \n </network> '
Line 73:     conn.networkDefineXML(xmlstr)


Line 83:         #connect to libvirtd
Line 84:         conn = libvirtconnection.get('qemu:///system')
Line 85: 
Line 86:         #check for running VMs. If there are VMs running, skip
Line 87:         #creating a dNIC pool
the code is pretty clear for doing this skip. what I'm missing is the reason WHY we should not create the pool with VMs up.
Line 88:         if not conn.listDomainsID():
Line 89:             #check if direct-pool is created
Line 90:             if 'direct-pool' not in conn.listNetworks():
Line 91:                 createDirectPool(conn)


Line 89:             #check if direct-pool is created
Line 90:             if 'direct-pool' not in conn.listNetworks():
Line 91:                 createDirectPool(conn)
Line 92:             else:
Line 93:                 if not conn.networkLookupByName('direct-pool').isActive():
I think you could set "dpool" right here, and save a couple of libvirt calls.
Line 94:                     conn.networkLookupByName('direct-pool').create()
Line 95:                     conn.networkLookupByName('direct-pool').setAutostart(1)
Line 96: 
Line 97:                 #check if the defined dNIC pool didn't change


Line 96: 
Line 97:                 #check if the defined dNIC pool didn't change
Line 98:                 dpool = conn.networkLookupByName('direct-pool')
Line 99:                 definedNics = []
Line 100:                 for i in dpool.XMLDesc(0).split():
parsing xml yourself is clearly calling for troubles. (think: what if the nic name has "dev=" in it?) 

you'd better use minidom.parseString() to obtain an xml object, call its getElementsByTagName('interface'), and extract the device from there.

This can sit in its own, easily testable, function.
Line 101:                     if 'dev=' in i:
Line 102:                         definedNics.append(str(i.split('=')[1].split('/')[0].strip("'")))
Line 103:                 #if the defined NIC pool doesn't match the usable NIC list,
Line 104:                 #recreate the network


Line 107:                     dpool.undefine()
Line 108:                     createDirectPool(conn)
Line 109: 
Line 110:         if 'direct-pool' not in conn.listNetworks():
Line 111:             createDirectPool(conn)
just a second - you create the dpool again, regardless if a VM is up? I'm confused.
Line 112:     except:
Line 113:         sys.stderr.write('vmfex: [unexpected error]: %s\n' % traceback.format_exc())


....................................................
File vdsm_hooks/vmfex/before_vm_start.py
Line 99:         i += 1
Line 100:     return vnicmap
Line 101: 
Line 102: 
Line 103: def createDirectPool(conn):
there is a lot of code duplication between this script and the one the I reviewed earlier. It would be safer to share the code in a single module (that could be copied to two places on deployment).
Line 104:     xmlstr = '''<network>
Line 105:        <name>direct-pool</name>
Line 106:        <forward mode="passthrough">
Line 107:     '''


Line 120: if 'vmfex' in os.environ:
Line 121:     try:
Line 122:         sys.stderr.write('vmfex: starting to edit VM \n')
Line 123:         #connect to libvirtd
Line 124:         conn = libvirtconnection.get('qemu:///system')
you should not pass anything to libvirtconnection.get()
Line 125:         # Check for running VMs. If there are VMs running, skip
Line 126:         # creating a dNIC pool
Line 127:         if not conn.listDomainsID():
Line 128: 


--
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Yasny <dyasny at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Dan Yasny <dyasny at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list