Change in vdsm[master]: vm: Collect vm numa node runtime pin information
fromani at redhat.com
fromani at redhat.com
Thu May 29 12:18:27 UTC 2014
Francesco Romani has posted comments on this change.
Change subject: vm: Collect vm numa node runtime pin information
......................................................................
Patch Set 1: Code-Review-1
(4 comments)
the schema part looks OK. A few minor comments, but the problem (and the reason for -1) is we should avoid to call libvirt dom methods inside getStats() - or inside its direct callees.
http://gerrit.ovirt.org/#/c/28134/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 2533: int(self.conf['memSize']) * 100)
Line 2534: stats['memUsage'] = utils.convertToStr(int(memUsage))
Line 2535: vmNumaNodeRuntimeInfo = self.getGuestNumaNodeRuntimeInfo()
Line 2536: if vmNumaNodeRuntimeInfo:
Line 2537: stats['vNodeRuntimeInfo'] = vmNumaNodeRuntimeInfo
This hunk would probably be better placed inside _getRunningVmStats()
Line 2538: return stats
Line 2539:
Line 2540: def _getExitedVmStats(self):
Line 2541: stats = {
Line 5064: for dev in self.conf.get('devices', ()):
Line 5065: if dev.get('type') == GRAPHICS_DEVICES:
Line 5066: return dev
Line 5067:
Line 5068: def getGuestNumaNodeRuntimeInfo(self):
please make private if it is only used internally (as it seems)
this method looks complex enough to deserve a docstring which briefly describe what is the expected output and how the input is formatted.
Line 5069: vmNumaNodeRuntimeMap = {}
Line 5070: if 'guestNumaNodes' in self.conf:
Line 5071: pNodesCpusMap = {}
Line 5072: for nodeIndex, numaNode in caps.getNumaTopology().iteritems():
Line 5072: for nodeIndex, numaNode in caps.getNumaTopology().iteritems():
Line 5073: for cpuId in numaNode['cpus']:
Line 5074: pNodesCpusMap[cpuId] = int(nodeIndex)
Line 5075: vNodesCpusMap = {}
Line 5076: for vmNumaNode in self.conf.get('guestNumaNodes'):
what is the benefit of get() here?
Line 5077: vmNumaNodeRuntimeMap[str(vmNumaNode['nodeIndex'])] = []
Line 5078: for vCpuId in map(int, vmNumaNode['cpus'].split(",")):
Line 5079: vNodesCpusMap[vCpuId] = vmNumaNode['nodeIndex']
Line 5080: vCpuRuntimeMap = self._getVcpuRuntimeInfo()
Line 5086: return vmNumaNodeRuntimeMap
Line 5087:
Line 5088: def _getVcpuRuntimeInfo(self):
Line 5089: vCpuRuntimeMap = {}
Line 5090: vCpuInfos = self._dom.vcpus()[0]
Please don't do like this. No libvirt calls in getStats() (or callee by getStats).
Please note we already have at least getBalloonInfo which does that and this is known broken: http://gerrit.ovirt.org/#/c/27696/
Line 5091: for vCpuInfo in vCpuInfos:
Line 5092: vCpuRuntimeMap[vCpuInfo[0]] = vCpuInfo[3]
Line 5093: return vCpuRuntimeMap
Line 5094:
--
To view, visit http://gerrit.ovirt.org/28134
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I20eac3b633efa5f81157f021515425b0c9e15d8f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Xiaolei Shi <xiao-lei.shi at hp.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Martin Sivák <msivak at redhat.com>
Gerrit-Reviewer: Xiaolei Shi <xiao-lei.shi at hp.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list