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