Mark Wu has posted comments on this change.
Change subject: [WIP] hooks: Add Quantum vNIC hook
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(5 inline comments)
....................................................
File vdsm/hooking.py
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.
Line 64: """
Line 65: sys.stderr.write(message)
Line 66: sys.stderr.write("\n")
is it a little bit cleaner to merge them into one line?
sys.stderr.write(message + '\n')
....................................................
File vdsm_hooks/quantumvnic/before_vm_start.py
Line 21: vNIC to a dummy bridge, and it is the agent's responsibility to swap
it to the
Line 22: Quantum bridge.
Line 23:
Line 24: Syntax:
Line 25: { 'quantumvnic_MAC1': 'port id', 'quantumvnic_MAC2': 'port_id',
... }
I suggest to use: quantumvnics: "mac1:portid1, mac2:portid2"
Line 26: Where:
Line 27: MACn should be replaced with the MAC addresses of the virtual NICs
to be
Line 28: connected to Quantum.'''
Line 29:
Line 34: domMac = iface.getElementsByTagName('mac')[0]
Line 35: domMacAddr = domMac.getAttribute('address').lower()
Line 36:
Line 37: if domMacAddr == mac:
Line 38: tapName = ('tap' + portId)[:DEV_MAX_LENGTH]
shouldn't DEV_MAX_LENGTH be 14? len('tap') + 11 (first 11 characters of the
port id)
Anyway, it's better to add a comment to explain the magic number.
Line 39:
Line 40: target = domxml.createElement('target')
Line 41: target.setAttribute('dev', tapName)
Line 42: iface.appendChild(target)
Line 41: target.setAttribute('dev', tapName)
Line 42: iface.appendChild(target)
Line 43:
Line 44: source = iface.getElementsByTagName('source')[0]
Line 45: source.setAttribute('bridge', DUMMY_BRIDGE)
I don't understand why we need bridge the tap device to the dummy bridge at
first. It looks the linux bridge agent in quantum just assume the tap device
isn't enslaved to any other bridge. So it could fail to add it to the quantum
bridge because it's already connected to the dummy bridge.
I suggest you pass quantum network id to the hook, and then it can enslave vif
to the quantum bridge directly.
Line 46:
Line 47:
Line 48: def main():
Line 49: domxml = hooking.read_domxml()
Line 82:
Line 83: if __name__ == '__main__':
Line 84: try:
Line 85: main()
Line 86: # test()
I guess it's not intentionally left here.
Line 87: except:
Line 88: hooking.exit_hook('quantumvnic hook: [unexpected error]: %s\n'
%
--
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mike Kolesnik <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Livnat Peer <[email protected]>
Gerrit-Reviewer: Mark Wu <[email protected]>
Gerrit-Reviewer: Mike Kolesnik <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
