Discussion:
Change in vdsm[master]: vm: Collect vm numa node runtime pin information
oVirt Jenkins CI Server
2014-05-27 05:12:32 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Collect vm numa node runtime pin information
......................................................................


Patch Set 1:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/8534/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/9465/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/9322/ : SUCCESS
--
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: 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: No
fromani
2014-05-29 12:18:27 UTC
Permalink
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
oVirt Jenkins CI Server
2014-06-04 02:26:19 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Collect vm numa node runtime pin information
......................................................................


Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/9682/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/8749/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/9535/ : SUCCESS
--
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: 2
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: No
oVirt Jenkins CI Server
2014-06-06 05:40:01 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Collect vm numa node runtime pin information
......................................................................


Patch Set 3:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/9750/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/8813/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/9599/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/646/ : There was an infra issue, please contact infra at ovirt.org
--
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: 3
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: No
fromani
2014-06-06 07:22:32 UTC
Permalink
Francesco Romani has posted comments on this change.

Change subject: vm: Collect vm numa node runtime pin information
......................................................................


Patch Set 3:

(4 comments)

Looks OK, adding a few comments to improve it even further.

http://gerrit.ovirt.org/#/c/28134/3/lib/vdsm/config.py.in
File lib/vdsm/config.py.in:

Line 171: ('vm_sample_balloon_window', '2', None),
Line 172:
Line 173: ('vm_sample_vcpu_pin_interval', '2',
Line 174: 'How often should we sample each vcpu runtime pinning to '
Line 175: 'which physical cpu core.'),
Still not sure we need to sample so frequently. Is cpu pinning supposed to change that fast?
Line 176:
Line 177: ('vm_sample_vcpu_pin_window', '2', None),
Line 178:
Line 179: ('trust_store_path', '@TRUSTSTORE@',


http://gerrit.ovirt.org/#/c/28134/3/vdsm/supervdsmServer
File vdsm/supervdsmServer:

Line 175: pidFile = "/var/run/libvirt/qemu/%s.pid" % vmName
Line 176: return open(pidFile).read()
Line 177:
Line 178: def _getVcpuPid(self, vmName):
Line 179: runFile = "/var/run/libvirt/qemu/%s.xml" % vmName
I don't like hardcoded paths but I guess we don't really have alternatives here. Do we?

(Yep I see this pattern it is used elsewhere in this very file, room for future work of course.)
Line 180: runInfo = xml.dom.minidom.parse(runFile)
Line 181: vCpus = runInfo.getElementsByTagName('vcpus')[0]
Line 182: vCpuSet = vCpus.getElementsByTagName('vcpu')
Line 183: vCpuPids = {}


Line 192: vCpuMemMaps = {}
Line 193: for vCpuIndex, vCpuPid in vCpuPids.iteritems():
Line 194: numaMapsFile = "/proc/%s/task/%s/numa_maps" % (vmPid, vCpuPid)
Line 195: if os.path.exists(numaMapsFile):
Line 196: with open(numaMapsFile, 'r') as f:
TL;DR: this code is good enough already


I'm not a fan of the

if os.path exists(a_file):
with open(a_file)

I'd rather prefer to do

try
with open(a_file)
except IOError
# something...

or, in pythonic words, I generally prefer EAFP instead of LBYL: http://python.net/~goodger/projects/pycon/2007/idiomatic/handout.html#eafp-vs-lbyl
Line 197: mappingNodes = map(
Line 198: int, re.findall('N(\d+)=\d+', f.read()))
Line 199: vCpuMemMaps[vCpuIndex] = list(set(mappingNodes))
Line 200: return vCpuMemMaps


http://gerrit.ovirt.org/#/c/28134/3/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 489: vCpuMemoryMapping[vCpu])
Line 490: vmNumaNodeRuntimeMap = dict([(k, list(set(v))) for k, v in
Line 491: vmNumaNodeRuntimeMap.iteritems()])
Line 492: if vmNumaNodeRuntimeMap:
Line 493: stats['vNodeRuntimeInfo'] = vmNumaNodeRuntimeMap
Thanks for the improvements in this function.
It is possible to go the extra (final) mile and split it in a few helper methods?
For some reasons I find this code unexpectedly dense.
Line 494:
Line 495: def get(self):
Line 496: stats = {}
Line 497:
--
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: 3
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
oVirt Jenkins CI Server
2014-06-06 09:23:25 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Collect vm numa node runtime pin information
......................................................................


Patch Set 4:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/9751/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/8814/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/9600/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/647/ : SUCCESS
--
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: 4
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: No
fromani
2014-06-09 08:01:20 UTC
Permalink
Francesco Romani has posted comments on this change.

Change subject: vm: Collect vm numa node runtime pin information
......................................................................


Patch Set 4: Code-Review+1

codewise this patch looks good;
The only thing left which I'd like to discuss a bit further is the sampling frequency.
--
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: 4
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: No
fromani
2014-06-09 08:47:18 UTC
Permalink
Francesco Romani has posted comments on this change.

Change subject: vm: Collect vm numa node runtime pin information
......................................................................


Patch Set 4:

Sorry, forgot to add: please add unit tests; there are examples in capsTests.py and vmTests.py you can use.
--
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: 4
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: No
danken
2014-06-09 09:02:38 UTC
Permalink
Dan Kenigsberg has posted comments on this change.

Change subject: vm: Collect vm numa node runtime pin information
......................................................................


Patch Set 4: Code-Review-1

(3 comments)

http://gerrit.ovirt.org/#/c/28134/4/lib/vdsm/config.py.in
File lib/vdsm/config.py.in:

Line 173: ('vm_sample_vcpu_pin_interval', '2',
Line 174: 'How often should we sample each vcpu runtime pinning to '
Line 175: 'which physical cpu core.'),
Line 176:
Line 177: ('vm_sample_vcpu_pin_window', '2', None),
come to think of it - is there any need in a window here (and for balloon too)? Can we set it to a hard-coded 0?
Line 178:
Line 179: ('trust_store_path', '@TRUSTSTORE@',
Line 180: 'Where the certificates and keys are situated.'),
Line 181:


http://gerrit.ovirt.org/#/c/28134/4/vdsm/supervdsmServer
File vdsm/supervdsmServer:

Line 174: def getVmPid(self, vmName):
Line 175: pidFile = "/var/run/libvirt/qemu/%s.pid" % vmName
Line 176: return open(pidFile).read()
Line 177:
Line 178: def _getVcpuPid(self, vmName):
it's better to keep supervdsmServer free of function-specific code. Please move this function's implementation to a new module, and add a unit test for it.

Is there a libvirt API to extract the info? (I suppose not)
Line 179: runFile = "/var/run/libvirt/qemu/%s.xml" % vmName
Line 180: runInfo = xml.dom.minidom.parse(runFile)
Line 181: vCpus = runInfo.getElementsByTagName('vcpus')[0]
Line 182: vCpuSet = vCpus.getElementsByTagName('vcpu')


http://gerrit.ovirt.org/#/c/28134/4/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 428: self._log.exception("Disk %s latency not available", dName)
Line 429:
Line 430: stats[dName].update(dLatency)
Line 431:
Line 432: def _getVmNumaNodeRuntimeInfo(self, stats):
can you shed this function to a new "numa" modules? vm.py is monstrously big. I think that putting it in its own module would make it easier to write it in a more testable way.
Line 433: """
Line 434: Collect vm numa nodes runtime pinning to which host numa nodes
Line 435: information.
Line 436: Host numa node topology:
--
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: 4
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
danken
2014-06-09 09:04:05 UTC
Permalink
Dan Kenigsberg has posted comments on this change.

Change subject: vm: Collect vm numa node runtime pin information
......................................................................


Patch Set 4:

Francesco, what do you think about the sampling rate? 2 seconds is quite often I suppose. How frequently do you expect numa positioning to change? How often is it measured by Engine?
--
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: 4
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: No
fromani
2014-06-10 08:47:05 UTC
Permalink
Francesco Romani has posted comments on this change.

Change subject: vm: Collect vm numa node runtime pin information
......................................................................


Patch Set 4:

(1 comment)

AFAIR stats are polled from engine every 15 (fifteen) seconds. And if this is correct (a simple proof: the sample intervalsin the lib/vdsm/config.py* are all multiple of 15), to poll the pin info every 2s will be just useless, because values will get overwritten without engine knowing. This is what I'm concerned about.

http://gerrit.ovirt.org/#/c/28134/4/lib/vdsm/config.py.in
File lib/vdsm/config.py.in:

Line 173: ('vm_sample_vcpu_pin_interval', '2',
Line 174: 'How often should we sample each vcpu runtime pinning to '
Line 175: 'which physical cpu core.'),
Line 176:
Line 177: ('vm_sample_vcpu_pin_window', '2', None),
Post by danken
come to think of it - is there any need in a window here (and for balloon t
I agree we shouldn't need a window here (-surely not for balloon)

However, we cannot in the current form of AdvancedStatsFunction (see vdsm/virt/sampling.py around line 305), because it expects a non-zero window.
I'll fix this in a future patch.
Line 178:
Line 179: ('trust_store_path', '@TRUSTSTORE@',
Line 180: 'Where the certificates and keys are situated.'),
Line 181:
--
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: 4
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
oVirt Jenkins CI Server
2014-06-12 06:33:19 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Collect vm numa node runtime pin information
......................................................................


Patch Set 5:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9134/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_install_rpm_sanity_gerrit/733/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/9919/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/800/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10074/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5001/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3158/ : SUCCESS
--
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: 5
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: No
fromani
2014-06-12 07:26:28 UTC
Permalink
Francesco Romani has posted comments on this change.

Change subject: vm: Collect vm numa node runtime pin information
......................................................................


Patch Set 5: Code-Review+1

(2 comments)

Good improvements. The code is getting nicer, but we need to address the duplication in tests.
Since we *can* do this in a later patch, it's +1 for me.

http://gerrit.ovirt.org/#/c/28134/5/tests/numaUtilsTests.py
File tests/numaUtilsTests.py:

Line 1: #
Line 2: # Copyright 2012 Red Hat, Inc.
2014 :)
Line 3: #
Line 4: # This program is free software; you can redistribute it and/or modify
Line 5: # it under the terms of the GNU General Public License as published by
Line 6: # the Free Software Foundation; either version 2 of the License, or


Line 111: fake.guestAgent = FakeGuestAgent()
Line 112: fake.conf['devices'] = [] if devices is None else devices
Line 113: fake._guestCpuRunning = runCpu
Line 114: fake._vmStats = FakeVmStatsThread(fake)
Line 115: yield fake
We need to avoid this duplication, by moving this common code in a shared helper.

However, it doesn't make sense to do that within this patch (it is also a bad smell), so either we rebase this patch on top of a new one or provide a follow up patch which factors out the common code.

I'm fine both ways (and I volunteer to do whatever we choose to do), let's just sync up.
Line 116:
Line 117:
Line 118: class TestNumaUtils(TestCaseBase):
Line 119:
--
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: 5
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
fromani
2014-06-12 15:22:47 UTC
Permalink
Francesco Romani has posted comments on this change.

Change subject: vm: Collect vm numa node runtime pin information
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.ovirt.org/#/c/28134/5/tests/numaUtilsTests.py
File tests/numaUtilsTests.py:

Line 111: fake.guestAgent = FakeGuestAgent()
Line 112: fake.conf['devices'] = [] if devices is None else devices
Line 113: fake._guestCpuRunning = runCpu
Line 114: fake._vmStats = FakeVmStatsThread(fake)
Line 115: yield fake
how about importing vmTests and using its FakeVM, and modifying it a bit?
That would be fine for me.
On a later patch we can factor out the shared code in a new module and then everyone will be a bit happier.
Line 116:
Line 117:
Line 118: class TestNumaUtils(TestCaseBase):
Line 119:
--
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: 5
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
danken
2014-06-12 15:04:14 UTC
Permalink
Dan Kenigsberg has posted comments on this change.

Change subject: vm: Collect vm numa node runtime pin information
......................................................................


Patch Set 5: Code-Review-1

(2 comments)

minor comment about code ownership

http://gerrit.ovirt.org/#/c/28134/5/tests/numaUtilsTests.py
File tests/numaUtilsTests.py:

Line 1: #
Line 2: # Copyright 2012 Red Hat, Inc.
Post by fromani
2014 :)
And most probably, HP and not RH.
Line 3: #
Line 4: # This program is free software; you can redistribute it and/or modify
Line 5: # it under the terms of the GNU General Public License as published by
Line 6: # the Free Software Foundation; either version 2 of the License, or


Line 111: fake.guestAgent = FakeGuestAgent()
Line 112: fake.conf['devices'] = [] if devices is None else devices
Line 113: fake._guestCpuRunning = runCpu
Line 114: fake._vmStats = FakeVmStatsThread(fake)
Line 115: yield fake
Post by fromani
We need to avoid this duplication, by moving this common code in a shared h
how about importing vmTests and using its FakeVM, and modifying it a bit?
Line 116:
Line 117:
Line 118: class TestNumaUtils(TestCaseBase):
Line 119:
--
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: 5
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
oVirt Jenkins CI Server
2014-06-13 02:47:29 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Collect vm numa node runtime pin information
......................................................................


Patch Set 6:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9199/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_install_rpm_sanity_gerrit/741/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/9983/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/826/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10138/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5065/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3222/ : SUCCESS
--
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: 6
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: No
fromani
2014-06-13 07:17:44 UTC
Permalink
Francesco Romani has posted comments on this change.

Change subject: vm: Collect vm numa node runtime pin information
......................................................................


Patch Set 6: Code-Review+1

looks good.
--
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: 6
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: No
danken
2014-06-13 07:33:18 UTC
Permalink
Dan Kenigsberg has posted comments on this change.

Change subject: vm: Collect vm numa node runtime pin information
......................................................................


Patch Set 6: Code-Review+2
--
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: 6
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: No
danken
2014-06-13 07:33:19 UTC
Permalink
Dan Kenigsberg has submitted this change and it was merged.

Change subject: vm: Collect vm numa node runtime pin information
......................................................................


vm: Collect vm numa node runtime pin information

Get the information that vm numa node runtime pin on which host numa
nodes.

Change-Id: I20eac3b633efa5f81157f021515425b0c9e15d8f
Bug-Url: https://bugzilla.redhat.com/1100202
Signed-off-by: Bruce Shi <xiao-lei.shi at hp.com>
Reviewed-on: http://gerrit.ovirt.org/28134
Reviewed-by: Francesco Romani <fromani at redhat.com>
Reviewed-by: Dan Kenigsberg <danken at redhat.com>
---
M debian/vdsm.install
M lib/vdsm/config.py.in
M tests/Makefile.am
A tests/numaUtilsTests.py
M vdsm.spec.in
M vdsm/Makefile.am
A vdsm/numaUtils.py
M vdsm/rpc/vdsmapi-schema.json
M vdsm/supervdsmServer
M vdsm/virt/vm.py
10 files changed, 282 insertions(+), 3 deletions(-)

Approvals:
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/28134
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I20eac3b633efa5f81157f021515425b0c9e15d8f
Gerrit-PatchSet: 7
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
oVirt Jenkins CI Server
2014-06-13 07:38:22 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Collect vm numa node runtime pin information
......................................................................


Patch Set 7:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_install_rpm_sanity_gerrit/742/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_create-rpms_merged/1442/ : SUCCESS
--
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: 7
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: No
Loading...