Dan Kenigsberg has posted comments on this change.
Change subject: caps: Collect numa information ......................................................................
Patch Set 5: Code-Review-1
(5 comments)
Welcome, Xiaolei Shi. I believe that your suggested API needs more discussion.
http://gerrit.ovirt.org/#/c/23703/5//COMMIT_MSG Commit Message:
Line 17: Take the first item above to explain the meaning of each field: Line 18: 0 - The index of numa node Line 19: 0,1 - The index of cpus(core) which belong to this numa node Line 20: 10240 - The total memory of this numa node Line 21: 9500 - The free memory of this numa node Who is going to consume this information? I suppose that any client would have to parse the string you have described. Wouldn't it be nicer (albeit inefficient) to return a parsed structure such as a dictionary
{ numaNodeIndex : {'cpus': [0,1], totmem: 10240, freemem: 9500} }
if you opt for a squashed, serialized api as the one you have proposed, please explain why a structured API is not preferable.
Besides that, reporting the free memory in getCaps seems wrong - it may be free on Vdsm startup, but quickly used when VM starts to run.
A third point worth considering is the extendibility of your API. libvirt provides more information - allocation of cpus to cores and sockets. I do not know if this is going to be useful to us, but it deserves thinking. In general, sticking to libvirt's modeling is better than inventing a completely new one.
Please include the motivation for your chosen API in the commit message. Line 22: Line 23: Change-Id: I63eeb697ab986c3b9cad0dc44f41924f329e52cd Line 24: Bug-Url: https://bugzilla.redhat.com/1010059
Line 20: 10240 - The total memory of this numa node Line 21: 9500 - The free memory of this numa node Line 22: Line 23: Change-Id: I63eeb697ab986c3b9cad0dc44f41924f329e52cd Line 24: Bug-Url: https://bugzilla.redhat.com/1010059 This bug is currently private. Can you open it to the public?
http://gerrit.ovirt.org/#/c/23703/5/vdsm/caps.py File vdsm/caps.py:
Line 178: for cpu in cell.getElementsByTagName('cpu'): Line 179: cpus.append(cpu.getAttribute('id')) Line 180: cellInfo.append(','.join(cpus)) Line 181: memInfo = _getMemoryStatsByNumaCell(int(cellInfo[0]), 0, memStats) Line 182: cellInfo.append(str(memInfo['total']/1024)) I do not know why pep8 did not cry, but please separate operator with spaces. Line 183: cellInfo.append(str(memInfo['free']/1024)) Line 184: cellsInfo.append(':'.join(cellInfo)) Line 185: return ';'.join(cellsInfo) Line 186:
Line 181: memInfo = _getMemoryStatsByNumaCell(int(cellInfo[0]), 0, memStats) Line 182: cellInfo.append(str(memInfo['total']/1024)) Line 183: cellInfo.append(str(memInfo['free']/1024)) Line 184: cellsInfo.append(':'.join(cellInfo)) Line 185: return ';'.join(cellsInfo)
I feel this can be improved somehow but I don't have any suggestion here. O
I share Francesco's feeling: the serialization of the data into a string is better left as a task for the transport. Typical clients would like to have direct access to the data fields. Line 186: Line 187: Line 188: def _getMemoryStatsByNumaCell(cell, flags=0, memStats=None): Line 189: if memStats is None:
Line 184: cellsInfo.append(':'.join(cellInfo)) Line 185: return ';'.join(cellsInfo) Line 186: Line 187: Line 188: def _getMemoryStatsByNumaCell(cell, flags=0, memStats=None): typically, memory usage is a volatile measure. As such, freemem does not fit well into getCaps, which is polled very seldom. Line 189: if memStats is None: Line 190: memStats = libvirtconnection.get().getMemoryStats(int(cell), flags) Line 191: return memStats Line 192:
vdsm-patches@lists.fedorahosted.org