Change in vdsm[master]: caps: Collect numa information
Dan Kenigsberg has posted comments on this change. Change subject: caps: Collect numa information .. Patch Set 12: Code-Review+2 -- 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: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Doron Fediuck dfedi...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Gilad Chaplik gchap...@redhat.com Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa gustavo.pedr...@eldorado.org.br Gerrit-Reviewer: Leonardo Bianconi leonardo.bianc...@eldorado.org.br Gerrit-Reviewer: Martin Sivák msi...@redhat.com Gerrit-Reviewer: Michal Skrivanek michal.skriva...@redhat.com Gerrit-Reviewer: Vitor de Lima vitor.l...@eldorado.org.br Gerrit-Reviewer: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: caps: Collect numa information
Dan Kenigsberg has submitted this change and it was merged.
Change subject: caps: Collect numa information
..
caps: Collect numa information
This patch modifies the caps module to add function of collect
host numa information and report to ovirt engine.
It will add three keys in the rpc response of getCapabilities
whose value are the host's numa related information.
Key numaNodes, the value is a map, each item follows this format:
{'nodeIndex':{'cpus':['int'], 'totalMemory':int}}
nodeIndex - The index of numa node
cpus - The cpu ids(see the cpu id in libvirt api's
capabilities function) which belong to this
numa node
totalMemory- The total memory of this numa node in MB
Key numaNodeDistance, the value is a map, each item follows this
format:
{'nodeIndex':[distanceList]}
nodeIndex - The index of numa node
distanceList - Distances from self to other nodes in sequence,
including self to self
Key autoNumaBalancing, the value is a int, represents the status
of auto numa balancing function.
Change-Id: I63eeb697ab986c3b9cad0dc44f41924f329e52cd
Bug-Url: https://bugzilla.redhat.com/1069303
Signed-off-by: Bruce Shi xiao-lei@hp.com
Reviewed-on: http://gerrit.ovirt.org/23703
Reviewed-by: Francesco Romani from...@redhat.com
Reviewed-by: Martin Sivák msi...@redhat.com
Reviewed-by: Dan Kenigsberg dan...@redhat.com
---
M tests/capsTests.py
A tests/caps_numactl_4_nodes.out
M vdsm/caps.py
M vdsm_api/vdsmapi-schema.json
4 files changed, 200 insertions(+), 1 deletion(-)
Approvals:
Martin Sivák: Looks good to me, but someone else must approve
Dan Kenigsberg: Looks good to me, approved
Francesco Romani: Looks good to me, but someone else must approve
Xiaolei Shi: Verified
--
To view, visit http://gerrit.ovirt.org/23703
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I63eeb697ab986c3b9cad0dc44f41924f329e52cd
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Doron Fediuck dfedi...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Gilad Chaplik gchap...@redhat.com
Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa
gustavo.pedr...@eldorado.org.br
Gerrit-Reviewer: Leonardo Bianconi leonardo.bianc...@eldorado.org.br
Gerrit-Reviewer: Martin Sivák msi...@redhat.com
Gerrit-Reviewer: Michal Skrivanek michal.skriva...@redhat.com
Gerrit-Reviewer: Vitor de Lima vitor.l...@eldorado.org.br
Gerrit-Reviewer: Xiaolei Shi xiao-lei@hp.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: caps: Collect numa information
Francesco Romani has posted comments on this change.
Change subject: caps: Collect numa information
..
Patch Set 12: Code-Review+1
(1 comment)
http://gerrit.ovirt.org/#/c/23703/12/vdsm_api/vdsmapi-schema.json
File vdsm_api/vdsmapi-schema.json:
Line 975: # @UNKNOWN:Can't get the status(maybe not support in this kernel
version)
Line 976: #
Line 977: # Since: 4.15.0
Line 978: ##
Line 979: {'enum': 'AutoNumaBalancingStatus', 'data': ['DISABLE', 'ENABLE',
'UNKNOWN']}
My personal taste is to use lowercase or Capitalized, but I see there are some
enums ALL UPPERCASE (HaMaintenanceMode being the first example), so I think it
is good enough.
Line 980:
Line 981: ##
Line 982: # @HookScriptInfoMap:
Line 983: #
--
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: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Doron Fediuck dfedi...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Gilad Chaplik gchap...@redhat.com
Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa
gustavo.pedr...@eldorado.org.br
Gerrit-Reviewer: Leonardo Bianconi leonardo.bianc...@eldorado.org.br
Gerrit-Reviewer: Martin Sivák msi...@redhat.com
Gerrit-Reviewer: Michal Skrivanek michal.skriva...@redhat.com
Gerrit-Reviewer: Vitor de Lima vitor.l...@eldorado.org.br
Gerrit-Reviewer: Xiaolei Shi xiao-lei@hp.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: caps: Collect numa information
Martin Sivák has posted comments on this change. Change subject: caps: Collect numa information .. Patch Set 12: Code-Review+1 -- 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: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Doron Fediuck dfedi...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Gilad Chaplik gchap...@redhat.com Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa gustavo.pedr...@eldorado.org.br Gerrit-Reviewer: Leonardo Bianconi leonardo.bianc...@eldorado.org.br Gerrit-Reviewer: Martin Sivák msi...@redhat.com Gerrit-Reviewer: Michal Skrivanek michal.skriva...@redhat.com Gerrit-Reviewer: Vitor de Lima vitor.l...@eldorado.org.br Gerrit-Reviewer: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: caps: Collect numa information
oVirt Jenkins CI Server has posted comments on this change. Change subject: caps: Collect numa information .. Patch Set 12: Build Failed http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/8023/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/8136/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/7233/ : SUCCESS -- 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: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Doron Fediuck dfedi...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Gilad Chaplik gchap...@redhat.com Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa gustavo.pedr...@eldorado.org.br Gerrit-Reviewer: Leonardo Bianconi leonardo.bianc...@eldorado.org.br Gerrit-Reviewer: Martin Sivák msi...@redhat.com Gerrit-Reviewer: Michal Skrivanek michal.skriva...@redhat.com Gerrit-Reviewer: Vitor de Lima vitor.l...@eldorado.org.br Gerrit-Reviewer: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: caps: Collect numa information
Francesco Romani has posted comments on this change.
Change subject: caps: Collect numa information
..
Patch Set 11:
(2 comments)
Just a few minor things and it is good for me.
http://gerrit.ovirt.org/#/c/23703/11/vdsm/caps.py
File vdsm/caps.py:
Line 193:
Line 194:
Line 195: # Get the memory stats of a specified numa node
Line 196: # cell -- int, the index of numa node
Line 197: # return value is like {'total': 50321208L, 'free': 47906488L}
Nit: the above comment makes more sense as docstring
Line 198: @utils.memoized
Line 199: def _getMemoryStatsByNumaCell(cell):
Line 200: return libvirtconnection.get().getMemoryStats(cell, 0)
Line 201:
http://gerrit.ovirt.org/#/c/23703/11/vdsm_api/vdsmapi-schema.json
File vdsm_api/vdsmapi-schema.json:
Line 1087: # @numaNodes: Information about host numa topology
Line 1088: #
Line 1089: # @numaNodeDistance:Distance information between each two numa
nodes
Line 1090: #
Line 1091: # @autoNumaBalancing: The status of auto numa balancing function
Please consider adding an enum here, like e.g. NetworkInterfaceState or anyting
else.
Probably is better to mimic the one you added in caps.py
Line 1092: #
Line 1093: # Since: 4.10.0
Line 1094: #
Line 1095: # Notes: Since ovirt-engine cannot parse software versions in
'x.y.z' format,
--
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: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Doron Fediuck dfedi...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Gilad Chaplik gchap...@redhat.com
Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa
gustavo.pedr...@eldorado.org.br
Gerrit-Reviewer: Leonardo Bianconi leonardo.bianc...@eldorado.org.br
Gerrit-Reviewer: Martin Sivák msi...@redhat.com
Gerrit-Reviewer: Michal Skrivanek michal.skriva...@redhat.com
Gerrit-Reviewer: Vitor de Lima vitor.l...@eldorado.org.br
Gerrit-Reviewer: Xiaolei Shi xiao-lei@hp.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: caps: Collect numa information
oVirt Jenkins CI Server has posted comments on this change. Change subject: caps: Collect numa information .. Patch Set 9: Build Successful http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7947/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/7157/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/8059/ : SUCCESS -- 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: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Doron Fediuck dfedi...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Gilad Chaplik gchap...@redhat.com Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa gustavo.pedr...@eldorado.org.br Gerrit-Reviewer: Leonardo Bianconi leonardo.bianc...@eldorado.org.br Gerrit-Reviewer: Martin Sivák msi...@redhat.com Gerrit-Reviewer: Michal Skrivanek michal.skriva...@redhat.com Gerrit-Reviewer: Vitor de Lima vitor.l...@eldorado.org.br Gerrit-Reviewer: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: caps: Collect numa information
oVirt Jenkins CI Server has posted comments on this change. Change subject: caps: Collect numa information .. Patch Set 10: Build Successful http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7948/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/7158/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/8060/ : SUCCESS -- 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: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Doron Fediuck dfedi...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Gilad Chaplik gchap...@redhat.com Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa gustavo.pedr...@eldorado.org.br Gerrit-Reviewer: Leonardo Bianconi leonardo.bianc...@eldorado.org.br Gerrit-Reviewer: Martin Sivák msi...@redhat.com Gerrit-Reviewer: Michal Skrivanek michal.skriva...@redhat.com Gerrit-Reviewer: Vitor de Lima vitor.l...@eldorado.org.br Gerrit-Reviewer: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: caps: Collect numa information
Francesco Romani has posted comments on this change.
Change subject: caps: Collect numa information
..
Patch Set 10:
(5 comments)
http://gerrit.ovirt.org/#/c/23703/10/tests/capsTests.py
File tests/capsTests.py:
Line 40: testPath = os.path.realpath(__file__)
Line 41: dirName = os.path.dirname(testPath)
Line 42: path = os.path.join(dirName, testFileName)
Line 43: with open(path) as src:
Line 44: return (0, src.read().splitlines(False), [])
Let's remove some duplication:
def _getTestData(testFileName):
testPath = os.path.realpath(__file__)
dirName = os.path.dirname(testPath)
path = os.path.join(dirName, testFileName):
with open(path) as src:
return src.read()
def _getCapsNumaDistanceTestData(testFileName):
return (0, _getTestData(testFileName).splitlines(False), [])
in a later patch we will move _getTestData in tests/testrunner.py and remove
even more duplication by letting other tests use it. IIRC there are a few more
places where this can be useful. However, again, stuff for a later unrelated
patch.
Line 45:
Line 46:
Line 47: class TestCaps(TestCaseBase):
Line 48:
Line 132: for res, s in zip(expectedRes, sign):
Line 133: self.assertEqual(res, caps._parseKeyVal(lines, s))
Line 134:
Line 135: @MonkeyPatch(caps, '_getMemoryStatsByNumaCell', lambda x: {
Line 136: 'total': 50321208L, 'free': 47906488L})
OK, this is enough for me to keep the one-line _getMemoryStatsByNumaCell.
Line 137: @MonkeyPatch(caps, '_getCapsXMLStr', lambda: _getCapsTestData(
Line 138: caps_libvirt_amd_6274.out))
Line 139: def testNumaTopology(self):
Line 140: # 2 x AMD 6272 (with Modules)
http://gerrit.ovirt.org/#/c/23703/10/vdsm/caps.py
File vdsm/caps.py:
Line 176: cellInfo = {}
Line 177: cpus = []
Line 178: for cpu in cell.getElementsByTagName('cpu'):
Line 179: cpus.append(cpu.getAttribute('id'))
Line 180: cellInfo['cpus'] = map(int, cpus)
Why not just
cpus.append(int(cpu.getAttribute('id')))
in the for loop above?
please consider a list comprehension if this improves the readability (I'm not
sure: it may or may not).
Line 181: cellIndex = cell.getAttribute('id')
Line 182: memInfo = _getMemoryStatsByNumaCell(int(cellIndex))
Line 183: cellInfo['totalMemory'] = memInfo['total'] / 1024
Line 184: cellsInfo[cellIndex] = cellInfo
Line 199: retcode, out, err = utils.execCmd(['numactl', '--hardware'])
Line 200: if retcode != 0:
Line 201: logging.error(Get error when execute numactl, exc_info=True)
Line 202: return nodeDistance
Line 203: pattern = re.compile(r'\s+(\d+):(.*)')
(disclaimer: personal taste) I'm not really a big fan of regexes but given the
numactl output this is likely the best solution :\
Line 204: for item in out:
Line 205: match = pattern.match(item)
Line 206: if match:
Line 207: nodeDistance[match.group(1)] = map(int,
Line 215:'kernel.numa_balancing'])
Line 216: if len(out) == 0:
Line 217: return 2
Line 218: else:
Line 219: return int(out[0])
we need constants here, especially for this magic '2'
please consider something like (feel free to fix the names)
class AutoNumaBalance:
FOO = 0
BAR = 1
BAZ = 2
and then
if not out:
return AutoNumaBalance.BAZ
else
...
a dict() can be another valid choice as well (probably easier in this use
case), see errCode (lib/vdsm/define.py) and its usage.
Line 220:
Line 221:
Line 222: @utils.memoized
Line 223: def _getEmulatedMachines(arch, capabilities=None):
--
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: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Doron Fediuck dfedi...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Gilad Chaplik gchap...@redhat.com
Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa
gustavo.pedr...@eldorado.org.br
Gerrit-Reviewer: Leonardo Bianconi leonardo.bianc...@eldorado.org.br
Gerrit-Reviewer: Martin Sivák msi...@redhat.com
Gerrit-Reviewer: Michal Skrivanek michal.skriva...@redhat.com
Gerrit-Reviewer: Vitor de Lima vitor.l...@eldorado.org.br
Gerrit-Reviewer: Xiaolei Shi xiao-lei@hp.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: caps: Collect numa information
oVirt Jenkins CI Server has posted comments on this change. Change subject: caps: Collect numa information .. Patch Set 11: Build Successful http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7955/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/7165/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/8067/ : SUCCESS -- 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: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Doron Fediuck dfedi...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Gilad Chaplik gchap...@redhat.com Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa gustavo.pedr...@eldorado.org.br Gerrit-Reviewer: Leonardo Bianconi leonardo.bianc...@eldorado.org.br Gerrit-Reviewer: Martin Sivák msi...@redhat.com Gerrit-Reviewer: Michal Skrivanek michal.skriva...@redhat.com Gerrit-Reviewer: Vitor de Lima vitor.l...@eldorado.org.br Gerrit-Reviewer: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: caps: Collect numa information
Martin Sivák has posted comments on this change. Change subject: caps: Collect numa information .. Patch Set 8: I like it. Fix Francesco's comments and it should be ready. -- 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Doron Fediuck dfedi...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Gilad Chaplik gchap...@redhat.com Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa gustavo.pedr...@eldorado.org.br Gerrit-Reviewer: Leonardo Bianconi leonardo.bianc...@eldorado.org.br Gerrit-Reviewer: Martin Sivák msi...@redhat.com Gerrit-Reviewer: Michal Skrivanek michal.skriva...@redhat.com Gerrit-Reviewer: Vitor de Lima vitor.l...@eldorado.org.br Gerrit-Reviewer: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: caps: Collect numa information
Michal Skrivanek has posted comments on this change.
Change subject: caps: Collect numa information
..
Patch Set 8:
(1 comment)
http://gerrit.ovirt.org/#/c/23703/8/vdsm/caps.py
File vdsm/caps.py:
Line 165:
Line 166:
Line 167: @utils.memoized
Line 168: def _getNumaTopology():
Line 169: capabilities = _getCapsXMLStr()
I'd suggest to use the same code as at line 146. And then as a separate patch
it should be refactored so we don't have to do this in every function:)
Line 170: caps = minidom.parseString(capabilities)
Line 171: host = caps.getElementsByTagName('host')[0]
Line 172: cells = host.getElementsByTagName('cells')[0]
Line 173: cellsInfo = {}
--
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: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Doron Fediuck dfedi...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Gilad Chaplik gchap...@redhat.com
Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa
gustavo.pedr...@eldorado.org.br
Gerrit-Reviewer: Leonardo Bianconi leonardo.bianc...@eldorado.org.br
Gerrit-Reviewer: Martin Sivák msi...@redhat.com
Gerrit-Reviewer: Michal Skrivanek michal.skriva...@redhat.com
Gerrit-Reviewer: Vitor de Lima vitor.l...@eldorado.org.br
Gerrit-Reviewer: Xiaolei Shi xiao-lei@hp.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: caps: Collect numa information
Francesco Romani has posted comments on this change.
Change subject: caps: Collect numa information
..
Patch Set 8:
(3 comments)
It can be already good enough but I think we can make it even better.
http://gerrit.ovirt.org/#/c/23703/8/tests/capsTests.py
File tests/capsTests.py:
Line 30: def _getCapsTestData(testFileName):
Line 31: testPath = os.path.realpath(__file__)
Line 32: dirName = os.path.dirname(testPath)
Line 33: path = os.path.join(dirName, testFileName)
Line 34: return open(path).read()
please use:
with open(path) as src:
return src.read()
Line 35:
Line 36:
Line 37: class TestCaps(TestCaseBase):
Line 38:
http://gerrit.ovirt.org/#/c/23703/8/vdsm/caps.py
File vdsm/caps.py:
Line 177: for cpu in cell.getElementsByTagName('cpu'):
Line 178: cpus.append(cpu.getAttribute('id'))
Line 179: cellInfo['cpus'] = cpus
Line 180: cellIndex = cell.getAttribute('id')
Line 181: memInfo = _getMemoryStatsByNumaCell(int(cellIndex))
Either this int() call...
Line 182: cellInfo['totalMemory'] = str(memInfo['total'] / 1024)
Line 183: cellsInfo[cellIndex] = cellInfo
Line 184: return cellsInfo
Line 185:
Line 185:
Line 186:
Line 187: @utils.memoized
Line 188: def _getMemoryStatsByNumaCell(cell):
Line 189: return libvirtconnection.get().getMemoryStats(int(cell), 0)
...or this one is redundant.
Moreover, do we really need this one-liner function?
Line 190:
Line 191:
Line 192: @utils.memoized
Line 193: def _getEmulatedMachines(arch, capabilities=None):
--
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: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Gilad Chaplik gchap...@redhat.com
Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa
gustavo.pedr...@eldorado.org.br
Gerrit-Reviewer: Leonardo Bianconi leonardo.bianc...@eldorado.org.br
Gerrit-Reviewer: Martin Sivák msi...@redhat.com
Gerrit-Reviewer: Michal Skrivanek michal.skriva...@redhat.com
Gerrit-Reviewer: Vitor de Lima vitor.l...@eldorado.org.br
Gerrit-Reviewer: Xiaolei Shi xiao-lei@hp.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: caps: Collect numa information
oVirt Jenkins CI Server has posted comments on this change. Change subject: caps: Collect numa information .. Patch Set 8: No Builds Executed http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7411/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6618/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7520/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. -- 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa gustavo.pedr...@eldorado.org.br Gerrit-Reviewer: Leonardo Bianconi leonardo.bianc...@eldorado.org.br Gerrit-Reviewer: Martin Sivák msi...@redhat.com Gerrit-Reviewer: Michal Skrivanek michal.skriva...@redhat.com Gerrit-Reviewer: Vitor de Lima vitor.l...@eldorado.org.br Gerrit-Reviewer: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: caps: Collect numa information
Francesco Romani has posted comments on this change.
Change subject: caps: Collect numa information
..
Patch Set 7:
(2 comments)
I think we can improve the code even more by get rid of some function
arguments, if -as code suggests- they are used only for testing.
http://gerrit.ovirt.org/#/c/23703/7/vdsm/caps.py
File vdsm/caps.py:
Line 164: return topology
Line 165:
Line 166:
Line 167: @utils.memoized
Line 168: def _getNumaTopology(capabilities=None):
I think you can get rid of this argument by using more monkeypatch, by
replacing _getCapsXMLStr() in the tests.
Line 169: if capabilities is None:
Line 170: capabilities = _getCapsXMLStr()
Line 171: caps = minidom.parseString(capabilities)
Line 172: host = caps.getElementsByTagName('host')[0]
Line 185: return cellsInfo
Line 186:
Line 187:
Line 188: @utils.memoized
Line 189: def _getMemoryStatsByNumaCell(cell, flags=0):
and get rid of the flags argument here (seems unused both here and in the
tests).
Probably this one-line method can be dropped as well. I'm ok with keeping it if
this helps testing, though.
Line 190: return libvirtconnection.get().getMemoryStats(int(cell), flags)
Line 191:
Line 192:
Line 193: @utils.memoized
--
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: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa
gustavo.pedr...@eldorado.org.br
Gerrit-Reviewer: Leonardo Bianconi leonardo.bianc...@eldorado.org.br
Gerrit-Reviewer: Martin Sivák msi...@redhat.com
Gerrit-Reviewer: Michal Skrivanek michal.skriva...@redhat.com
Gerrit-Reviewer: Vitor de Lima vitor.l...@eldorado.org.br
Gerrit-Reviewer: Xiaolei Shi xiao-lei@hp.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: caps: Collect numa information
oVirt Jenkins CI Server has posted comments on this change. Change subject: caps: Collect numa information .. Patch Set 7: No Builds Executed http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6521/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7305/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7423/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa gustavo.pedr...@eldorado.org.br Gerrit-Reviewer: Leonardo Bianconi leonardo.bianc...@eldorado.org.br Gerrit-Reviewer: Martin Sivák msi...@redhat.com Gerrit-Reviewer: Michal Skrivanek michal.skriva...@redhat.com Gerrit-Reviewer: Vitor de Lima vitor.l...@eldorado.org.br Gerrit-Reviewer: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: caps: Collect numa information
oVirt Jenkins CI Server has posted comments on this change. Change subject: caps: Collect numa information .. Patch Set 8: No Builds Executed http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6522/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7306/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7424/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. -- 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa gustavo.pedr...@eldorado.org.br Gerrit-Reviewer: Leonardo Bianconi leonardo.bianc...@eldorado.org.br Gerrit-Reviewer: Martin Sivák msi...@redhat.com Gerrit-Reviewer: Michal Skrivanek michal.skriva...@redhat.com Gerrit-Reviewer: Vitor de Lima vitor.l...@eldorado.org.br Gerrit-Reviewer: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: caps: Collect numa information
Francesco Romani has posted comments on this change. Change subject: caps: Collect numa information .. Patch Set 6: Code-Review-1 (2 comments) something is missing in the vdsmapi-schema.json http://gerrit.ovirt.org/#/c/23703/6/vdsm/caps.py File vdsm/caps.py: Line 188: def _getMemoryStatsByNumaCell(cell, flags=0, memStats=None): Line 189: if memStats is None: Line 190: memStats = libvirtconnection.get().getMemoryStats(int(cell), flags) Line 191: if type(memStats) is frozenset: Line 192: memStats = dict(memStats) I think we can just call dict() here. What happens if memStats is not a frozenset (maybe plain set)? Line 193: return memStats Line 194: Line 195: Line 196: @utils.memoized http://gerrit.ovirt.org/#/c/23703/6/vdsm_api/vdsmapi-schema.json File vdsm_api/vdsmapi-schema.json: Line 1033: 'emulatedMachines': ['str'], 'ISCSIInitiatorName': 'str', Line 1034: 'HBAInventory': 'HbaInventory', 'vmTypes': ['VmType'], Line 1035: 'memSize': 'uint', 'reservedMem': 'uint', Line 1036: 'guestOverhead': 'uint', 'netConfigDirty': 'bool', Line 1037: 'rngSources': ['VmRngDeviceSource'], 'numaNodes': 'NumaNodeMap'}} This needs to be documented in the schema, I expect the schema compilation fail here otherwise. Line 1038: Line 1039: ## Line 1040: # @Host.getCapabilities: Line 1041: # -- 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: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa gustavo.pedr...@eldorado.org.br Gerrit-Reviewer: Leonardo Bianconi leonardo.bianc...@eldorado.org.br Gerrit-Reviewer: Martin Sivák msi...@redhat.com Gerrit-Reviewer: Michal Skrivanek michal.skriva...@redhat.com Gerrit-Reviewer: Vitor de Lima vitor.l...@eldorado.org.br Gerrit-Reviewer: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: caps: Collect numa information
Francesco Romani has posted comments on this change. Change subject: caps: Collect numa information .. Patch Set 6: (1 comment) http://gerrit.ovirt.org/#/c/23703/6/vdsm_api/vdsmapi-schema.json File vdsm_api/vdsmapi-schema.json: Line 1033: 'emulatedMachines': ['str'], 'ISCSIInitiatorName': 'str', Line 1034: 'HBAInventory': 'HbaInventory', 'vmTypes': ['VmType'], Line 1035: 'memSize': 'uint', 'reservedMem': 'uint', Line 1036: 'guestOverhead': 'uint', 'netConfigDirty': 'bool', Line 1037: 'rngSources': ['VmRngDeviceSource'], 'numaNodes': 'NumaNodeMap'}} This needs to be documented in the schema, I expect the schema compilation (sorry, clicked reply too early) VDSM schema compilation can be puzzling sometimes (cryptic or just very terse messages). Feel free to ping me (IRC, mail) if you need help. Line 1038: Line 1039: ## Line 1040: # @Host.getCapabilities: Line 1041: # -- 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: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa gustavo.pedr...@eldorado.org.br Gerrit-Reviewer: Leonardo Bianconi leonardo.bianc...@eldorado.org.br Gerrit-Reviewer: Martin Sivák msi...@redhat.com Gerrit-Reviewer: Michal Skrivanek michal.skriva...@redhat.com Gerrit-Reviewer: Vitor de Lima vitor.l...@eldorado.org.br Gerrit-Reviewer: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: caps: Collect numa information
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:
--
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa
gustavo.pedr...@eldorado.org.br
Gerrit-Reviewer: Leonardo Bianconi leonardo.bianc...@eldorado.org.br
Gerrit-Reviewer: Martin Sivák msi...@redhat.com
Gerrit-Reviewer: Michal Skrivanek michal.skriva...@redhat.com
Gerrit-Reviewer: Vitor de Lima vitor.l...@eldorado.org.br
Gerrit-Reviewer: Xiaolei Shi xiao-lei@hp.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: caps: Collect numa information
Francesco Romani has posted comments on this change.
Change subject: caps: Collect numa information
..
Patch Set 5: Code-Review+1
(2 comments)
http://gerrit.ovirt.org/#/c/23703/5//COMMIT_MSG
Commit Message:
Line 15: The items are separated by semicolon, the fields in each item
Line 16: are separated by colon.
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
ok, so is cpu,core right?
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
http://gerrit.ovirt.org/#/c/23703/5/vdsm/caps.py
File vdsm/caps.py:
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. On
the other hand, we also have XML processing improvements in the pipeline and I
don't see any obvious issue, in the nd I think it is good enough to go.
Line 186:
Line 187:
Line 188: def _getMemoryStatsByNumaCell(cell, flags=0, memStats=None):
Line 189: if memStats is None:
--
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Martin Sivák msi...@redhat.com
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: caps: Collect numa information
oVirt Jenkins CI Server has posted comments on this change. Change subject: caps: Collect numa information .. Patch Set 4: No Builds Executed http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6272/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7162/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7051/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Martin Sivák msi...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: caps: Collect numa information
Francesco Romani has posted comments on this change. Change subject: caps: Collect numa information .. Patch Set 4: (2 comments) Just a couple of things in the commit message and it is good for me. http://gerrit.ovirt.org/#/c/23703/4//COMMIT_MSG Commit Message: 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. Add one blank line here for readability, please. Line 13: An example of the value: 0:0,1:10240:9500;1:2,3:10240:9700 Line 14: The items are separated by semicolon, the fields in each item Line 15: are separated by colon. Line 16: Take the first item above to explain the meaning of each field: Line 14: The items are separated by semicolon, the fields in each item Line 15: are separated by colon. Line 16: Take the first item above to explain the meaning of each field: Line 17: 0 - The index of numa node Line 18: 0,1- The index of cpu which belong to this numa node Thanks, we're almost there. Is this socket_id,core_id? If so, I'd change the text as something like that The index of cpu (socket, core) which belong to this numa node. Line 19: 10240 - The total memory of this numa node Line 20: 9500 - The free memory of this numa node Line 21: Line 22: Change-Id: I63eeb697ab986c3b9cad0dc44f41924f329e52cd -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Martin Sivák msi...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: caps: Collect numa information
oVirt Jenkins CI Server has posted comments on this change. Change subject: caps: Collect numa information .. Patch Set 5: No Builds Executed http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6291/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7181/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7070/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xiaolei Shi xiao-lei@hp.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Martin Sivák msi...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
