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....@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: Xiaolei Shi <xiao-lei....@hp.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