Igor Lvovsky has posted comments on this change.
Change subject: Cisco VM-FEX support vdsm hooks
......................................................................
Patch Set 5: (11 inline comments)
Both hooks have almost same code.
Could you please move all identical code to some new modul and just use it in
both hooks
....................................................
File vdsm_hooks/vmfex/before_vm_start.py
Line 82:
Line 83:
Line 84: def getUsableNics():
Line 85: # Scan localhost for physical NICs and return list of physical nics
Line 86: # that have all zeroes MAC
why?
Line 87: # Example ['eth0','eth1']
Line 88: #TODO: switch to glob in the future
Line 89: nics = []
Line 90: cmd = "find /sys/devices/pci* | grep '/net/' |grep address"
Line 84: def getUsableNics():
Line 85: # Scan localhost for physical NICs and return list of physical nics
Line 86: # that have all zeroes MAC
Line 87: # Example ['eth0','eth1']
Line 88: #TODO: switch to glob in the future
why not now? it's too simple
glob.glob('/sys/devices/pci*/*/*/*/net/*/address')
Line 89: nics = []
Line 90: cmd = "find /sys/devices/pci* | grep '/net/' |grep address"
Line 91: p = subprocess.Popen(cmd, stdout=subprocess.PIPE, shell=True)
Line 92: for nic in p.stdout.read().split():
Line 104: <name>direct-pool</name>
Line 105: <forward mode="passthrough">
Line 106: '''
Line 107: for i in getUsableNics():
Line 108: xmlstr += '<interface dev="' + str(i) + '"/> \n'
no need str(), it's already strings
Line 109:
Line 110: xmlstr += ' </forward> \n </network> '
Line 111: conn.networkDefineXML(xmlstr)
Line 112: dpool = conn.networkLookupByName('direct-pool')
Line 107: for i in getUsableNics():
Line 108: xmlstr += '<interface dev="' + str(i) + '"/> \n'
Line 109:
Line 110: xmlstr += ' </forward> \n </network> '
Line 111: conn.networkDefineXML(xmlstr)
What will happens if this direct-pool already exists?
For example if hook run for several VMs.
Do you want to swallow this exception?
Line 112: dpool = conn.networkLookupByName('direct-pool')
Line 113: dpool.setAutostart(1)
Line 114: dpool.create()
Line 115: sys.stderr.write('vmfex: creating Direct-Pool Network \n')
Line 123: fcntl.flock(f.fileno(), fcntl.LOCK_EX)
Line 124: try:
Line 125: dpool = conn.networkLookupByName('direct-pool')
Line 126: dpool.destroy()
Line 127: dpool.undefine()
as above, just remove case
Line 128: sys.stderr.write('vmfex: removing direct-pool \n')
Line 129: finally:
Line 130: fcntl.flock(f.fileno(), fcntl.LOCK_UN)
Line 131:
Line 134: for vm in conn.listDomainsID():
Line 135: domxml = minidom.parseString(conn.lookupByID(vm).XMLDesc(0))
Line 136: for interface in domxml.getElementsByTagName('interface'):
Line 137: for attr in interface.getElementsByTagName('virtualport'):
Line 138: if attr.attributes.items()[0][1] == '802.1Qbh':
is it should be a simple
getAttribute('type') == '802.1Qbh'
Line 139: return True
Line 140: return False
Line 141:
Line 142:
Line 155:
Line 156:
Line 157: def handleDirectPool(conn):
Line 158: #TODO: take this part and everything it uses out and into a
separate
Line 159: # package
module, not package
Line 160:
Line 161: #is direct-pool defined?
Line 162: if 'direct-pool' in conn.listNetworks():
Line 163: #are there VMs running?
Line 157: def handleDirectPool(conn):
Line 158: #TODO: take this part and everything it uses out and into a
separate
Line 159: # package
Line 160:
Line 161: #is direct-pool defined?
IMHO the bellow looks better:
if 'direct-pool' not in conn.listNetworks():
createDirectPool(conn)
return
Line 162: if 'direct-pool' in conn.listNetworks():
Line 163: #are there VMs running?
Line 164: if conn.listDomainsID():
Line 165: #does any of the running VMs havd a Qbh nic?
....................................................
File vdsm_hooks/vmfex/Makefile.am
Line 21: EXTRA_DIST = \
Line 22: before_vm_migrate_destination.py \
Line 23: before_vm_start.py \
Line 24:
Line 25: install-data-local:
trailing spases ?????
Line 26: $(MKDIR_P) $(DESTDIR)$(vdsmhooksdir)/before_vm_start
Line 27: $(INSTALL_SCRIPT) $(srcdir)/before_vm_start.py \
Line 28: $(DESTDIR)$(vdsmhooksdir)/before_vm_start/50_vmfex
Line 29: $(MKDIR_P)
$(DESTDIR)$(vdsmhooksdir)/before_vm_migrate_destination
Line 29: $(MKDIR_P)
$(DESTDIR)$(vdsmhooksdir)/before_vm_migrate_destination
Line 30: $(INSTALL_SCRIPT) $(srcdir)/before_vm_migrate_destination.py \
Line 31:
$(DESTDIR)$(vdsmhooksdir)/before_vm_migrate_destination/50_vmfex
Line 32:
Line 33: uninstall-local:
again
Line 34: $(RM) $(DESTDIR)$(vdsmhooksdir)/before_vm_start/50_vmfex
Line 35: $(RM)
$(DESTDIR)$(vdsmhooksdir)/before_vm_migrate_destination/50_vmfex
....................................................
File vdsm_hooks/vmfex/README
Line 17: <mac address="<mac>"/>
Line 18: <model type="virtio"/>
Line 19: <source bridge="<logical network>"/>
Line 20: </interface>
Line 21:
trailing spaces
Line 22: with the following interface xml:
Line 23: <interface type='network'>
Line 24: <mac address='<mac>'/>
Line 25: <source network='direct-pool'/>
--
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Yasny <[email protected]>
Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Dan Yasny <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Igor Lvovsky <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
