Francesco Romani has posted comments on this change. Change subject: caps: Collect numa infomation ......................................................................
Patch Set 3: Code-Review-1 (10 comments) http://gerrit.ovirt.org/#/c/23703/3//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2014-01-23 11:06:02 +0800 Line 4: Commit: Bruce Shi <[email protected]> Line 5: CommitDate: 2014-01-28 01:11:16 -0800 Line 6: Line 7: caps: Collect numa infomation typo Line 8: Line 9: This patch modifies the caps module to add function of collect Line 10: host numa information and report to ovirt engine. Line 11: It will add a key named numaNodes in the xml rpc response of Line 8: Line 9: This patch modifies the caps module to add function of collect Line 10: host numa information and report to ovirt engine. Line 11: It will add a key named numaNodes in the xml rpc response of Line 12: getCapabilities whose value is the host's numa information. thanks for adding this; what I meant was a short description and explanation of the expected format of the new value. Line 13: Line 14: Change-Id: I63eeb697ab986c3b9cad0dc44f41924f329e52cd Line 15: Bug-Url: https://bugzilla.redhat.com/1010059 http://gerrit.ovirt.org/#/c/23703/3/tests/capsTests.py File tests/capsTests.py: Line 116: self.assertEqual(res, caps._parseKeyVal(lines, s)) Line 117: Line 118: def testNumaTopology(self): Line 119: testPath = os.path.realpath(__file__) Line 120: dirName = os.path.split(testPath)[0] This should be nicer: dirName = os.path.dirname(testPath) Line 121: testMemStats = {'total': 50321208L, 'free': 47906488L} Line 122: # 2 x Intel E5649 (with Hyperthreading) Line 123: path = os.path.join(dirName, "caps_libvirt_intel_E5649.out") Line 124: t = caps._getNumaTopology(file(path).read(), testMemStats) Line 118: def testNumaTopology(self): Line 119: testPath = os.path.realpath(__file__) Line 120: dirName = os.path.split(testPath)[0] Line 121: testMemStats = {'total': 50321208L, 'free': 47906488L} Line 122: # 2 x Intel E5649 (with Hyperthreading) please add a blank line here for readability (just before the comment) Line 123: path = os.path.join(dirName, "caps_libvirt_intel_E5649.out") Line 124: t = caps._getNumaTopology(file(path).read(), testMemStats) Line 125: self.assertEqual(t, '0:0,2,4,6,8,10,12,14,16,18,20,22:49141:46783;' Line 126: + '1:1,3,5,7,9,11,13,15,17,19,21,23:49141:46783') Line 122: # 2 x Intel E5649 (with Hyperthreading) Line 123: path = os.path.join(dirName, "caps_libvirt_intel_E5649.out") Line 124: t = caps._getNumaTopology(file(path).read(), testMemStats) Line 125: self.assertEqual(t, '0:0,2,4,6,8,10,12,14,16,18,20,22:49141:46783;' Line 126: + '1:1,3,5,7,9,11,13,15,17,19,21,23:49141:46783') Here and below: you dont need the `+` here because the python VM automatically joins two adjacent string literals. Line 127: # 2 x AMD 6272 (with Modules) Line 128: path = os.path.join(dirName, "caps_libvirt_amd_6274.out") Line 129: t = caps._getNumaTopology(file(path).read(), testMemStats) Line 130: self.assertEqual(t, '0:0,1,2,3,4,5,6,7:49141:46783;' Line 123: path = os.path.join(dirName, "caps_libvirt_intel_E5649.out") Line 124: t = caps._getNumaTopology(file(path).read(), testMemStats) Line 125: self.assertEqual(t, '0:0,2,4,6,8,10,12,14,16,18,20,22:49141:46783;' Line 126: + '1:1,3,5,7,9,11,13,15,17,19,21,23:49141:46783') Line 127: # 2 x AMD 6272 (with Modules) same here Line 128: path = os.path.join(dirName, "caps_libvirt_amd_6274.out") Line 129: t = caps._getNumaTopology(file(path).read(), testMemStats) Line 130: self.assertEqual(t, '0:0,1,2,3,4,5,6,7:49141:46783;' Line 131: + '1:8,9,10,11,12,13,14,15:49141:46783;' Line 130: self.assertEqual(t, '0:0,1,2,3,4,5,6,7:49141:46783;' Line 131: + '1:8,9,10,11,12,13,14,15:49141:46783;' Line 132: + '2:16,17,18,19,20,21,22,23:49141:46783;' Line 133: + '3:24,25,26,27,28,29,30,31:49141:46783') Line 134: # 1 x Intel E31220 (normal Multi-core) same here Line 135: path = os.path.join(dirName, "caps_libvirt_intel_E31220.out") Line 136: t = caps._getNumaTopology(file(path).read(), testMemStats) Line 132: + '2:16,17,18,19,20,21,22,23:49141:46783;' Line 133: + '3:24,25,26,27,28,29,30,31:49141:46783') Line 134: # 1 x Intel E31220 (normal Multi-core) Line 135: path = os.path.join(dirName, "caps_libvirt_intel_E31220.out") Line 136: t = caps._getNumaTopology(file(path).read(), testMemStats) here (and above), please use open(path).read() or even better, a two-line helper (if isn't somewhere already) with open(path) as f: data = f.read() # use `data` as you need Line 133: + '3:24,25,26,27,28,29,30,31:49141:46783') Line 134: # 1 x Intel E31220 (normal Multi-core) Line 135: path = os.path.join(dirName, "caps_libvirt_intel_E31220.out") Line 136: t = caps._getNumaTopology(file(path).read(), testMemStats) Line 137: self.assertEqual(t, '0:0,1,2,3:49141:46783') That is what I was talking about in the note of the commit message: a succint explanation of the format of those strings would be nice. For example: I can guess here the 0 socket has four cores/threads, but is this right? and what about the last two numbers? http://gerrit.ovirt.org/#/c/23703/3/vdsm/caps.py File vdsm/caps.py: 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) Line 186: please remove. to catch this and many other coding style violations (if any!) you could run make check-local on your VDSM git tree. Line 187: Line 188: def _getMemoryStatsByNumaCell(cell, flags=0, memStats=None): Line 189: if memStats is None: Line 190: memStats = libvirtconnection.get().getMemoryStats(int(cell), flags) -- To view, visit http://gerrit.ovirt.org/23703 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I63eeb697ab986c3b9cad0dc44f41924f329e52cd Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xiaolei Shi <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Martin Sivák <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
