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