Change in vdsm[master]: caps/lib: move CPU architecture details to lib/cpuarch

mpolednik at redhat.com mpolednik at redhat.com
Mon Dec 14 09:32:45 UTC 2015


Martin Polednik has posted comments on this change.

Change subject: caps/lib: move CPU architecture details to lib/cpuarch
......................................................................


Patch Set 2:

(12 comments)

A lot of suggestions make sense for different patch series - the removal of Architecture class, getting rid of CPU_MAP.xml parsing etc. do make sense AFTER we have the code untangled and sitting in it's own module. Only the most essential code changes are included in this series.

https://gerrit.ovirt.org/#/c/49972/2/lib/vdsm/Makefile.am
File lib/vdsm/Makefile.am:

Line 25: 	__init__.py \
Line 26: 	cmdutils.py \
Line 27: 	compat.py \
Line 28: 	concurrent.py \
Line 29: 	cpuarch.py \
> Why not arch.py?  This is the term used in the kernel and in packaging, and
Too many name clashes at this point - the term is too generic e.g. arch = Architecture.x86_64...
Line 30: 	define.py \
Line 31: 	dmidecodeUtil.py \
Line 32: 	exception.py \
Line 33: 	executor.py \


https://gerrit.ovirt.org/#/c/49972/2/lib/vdsm/cpuarch.py
File lib/vdsm/cpuarch.py:

Line 22: 
Line 23: from .config import config
Line 24: 
Line 25: 
Line 26: class Architecture:
> This class useless, we have 4 constants, putting them under a class does no
Good point for future patch (series) to refactor the bits.
Line 27:     X86_64 = 'x86_64'
Line 28:     PPC64 = 'ppc64'
Line 29:     PPC64LE = 'ppc64le'
Line 30:     POWER = (PPC64, PPC64LE)


Line 26: class Architecture:
Line 27:     X86_64 = 'x86_64'
Line 28:     PPC64 = 'ppc64'
Line 29:     PPC64LE = 'ppc64le'
Line 30:     POWER = (PPC64, PPC64LE)
> Having different types for enums is a bad idea, make the user code inconsis
Something I (and afaik fromani) would like to eventually add - it would fit into the patch series mentioned above.
Line 31: 
Line 32: 
Line 33: class UnsupportedArchitecture(Exception):
Line 34:     def __init__(self, target_arch):


Line 34:     def __init__(self, target_arch):
Line 35:         self._target_arch = target_arch
Line 36: 
Line 37:     def __str__(self):
Line 38:         return '{}'.format(self._target_arch)
> This is not very helpful message. Is this part of vdsm api, or internal err
Internal error, not really used outside of cpuarch.
Line 39: 
Line 40: 
Line 41: def arch_to_cpu_map(target_arch=None):
Line 42:     '''


Line 37:     def __str__(self):
Line 38:         return '{}'.format(self._target_arch)
Line 39: 
Line 40: 
Line 41: def arch_to_cpu_map(target_arch=None):
> cpu_map is not good name, since this returns a single value, not a map.
It is still translation to libvirt's CPU_MAP and should be removed as soon as possible as we shouldn't touch that file at all. Not sure if there are any benefits for the time being of different name, and if yes I'd rather see arch_to_libvirt_cpu_map.
Line 42:     '''
Line 43:     Convert the CPU architecture to a libvirt's cpu_map representation.
Line 44: 
Line 45:     Arguments:


Line 85:     '''
Line 86:     return _cpu_arch_to_architecture(platform.machine())
Line 87: 
Line 88: 
Line 89: def target():
> And this is the effective architecture. current and target are very confusi
Mostly agree. 'real' and 'effective' sound somewhat better.
Line 90:     '''
Line 91:     Get the target VM runtime architecture. This function exists to modify the
Line 92:     architecture reported in vds capabilities and VMs. It is required because
Line 93:     some functions require to know the real architecture, while the others are


Line 98:     The runtime architecture of VDSM
Line 99:     or
Line 100:     raises UnsupportedArchitecture exception.
Line 101:     '''
Line 102:     if config.getboolean('vars', 'fake_kvm_support'):
> Is this a new config? rename it to fake_kvm_enable if new.
It is not, for the sake of backwards compatibility.
Line 103:         return _cpu_arch_to_architecture(
Line 104:             config.get('vars', 'fake_kvm_architecture'))
Line 105:     else:
Line 106:         return current()


Line 109: def _cpu_arch_to_architecture(target_arch):
Line 110:     mapping = {
Line 111:         'x86_64': Architecture.X86_64,
Line 112:         'ppc64': Architecture.PPC64,
Line 113:         'ppc64le': Architecture.PPC64LE
> Looks like the input and the output of this function are the same. Do we re
The input is a string while output is class attribute - the mapping exists as we lack the predicates.
Line 114:     }
Line 115: 
Line 116:     try:
Line 117:         arch = mapping[target_arch]


https://gerrit.ovirt.org/#/c/49972/2/tests/vmTests.py
File tests/vmTests.py:

Line 212: 
Line 213:         for vmConf, osXML in zip(vmConfs, expectedXMLs):
Line 214:             vmConf.update(self.conf)
Line 215:             domxml = vmxml.Domain(vmConf, self.log,
Line 216:                                   cpuarch.Architecture.X86_64)
> Are you sure you need to break this line? other lines around look the same.
See the first argument - vmConf pushes the line to 80 while 'conf' used elsewhere keeps the lines at 78.
Line 217:             domxml.appendOs()
Line 218:             xml = find_xml_element(domxml.dom.toxml(), './os')
Line 219:             self.assertXMLEqual(xml, osXML)
Line 220: 


https://gerrit.ovirt.org/#/c/49972/2/vdsm/supervdsmServer
File vdsm/supervdsmServer:

Line 157:     def getHardwareInfo(self, *args, **kwargs):
Line 158:         if platform.machine() in ('x86_64', 'i686'):
Line 159:             from vdsm.dmidecodeUtil import getHardwareInfoStructure
Line 160:             return getHardwareInfoStructure()
Line 161:         elif platform.machine() in cpuarch.Architecture.POWER:
> Here is the proof that cpuarch._cpu_arch_to_architecture is not neeed :-)
This is fixed later in the series.
Line 162:             from vdsm.ppc64HardwareInfo import getHardwareInfoStructure
Line 163:             return getHardwareInfoStructure()
Line 164:         else:
Line 165:             #  not implemented over other architecture


https://gerrit.ovirt.org/#/c/49972/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 318:         self._released = False
Line 319:         self._releaseLock = threading.Lock()
Line 320:         self.saveState()
Line 321:         self._watchdogEvent = {}
Line 322:         self.arch = cpuarch.target()
> I like short name, but this is not clear - target_arch()?
I like the cpuarch.effective() and cpuarch.real() kind of a lot.
Line 323: 
Line 324:         if self.arch not in cpuarch.Architecture.POWER and \
Line 325:                 self.arch != cpuarch.Architecture.X86_64:
Line 326:             raise RuntimeError('Unsupported architecture: %s' % self.arch)


Line 321:         self._watchdogEvent = {}
Line 322:         self.arch = cpuarch.target()
Line 323: 
Line 324:         if self.arch not in cpuarch.Architecture.POWER and \
Line 325:                 self.arch != cpuarch.Architecture.X86_64:
> Having both string and tuple/list constants in the same enum is very bad id
Again, something future series might solve. Current patch series doesn't touch the architectural logic outside of it's scope and aims to fix fakekvm and faqemu (as none of this work).
Line 326:             raise RuntimeError('Unsupported architecture: %s' % self.arch)
Line 327: 
Line 328:         self._powerDownEvent = threading.Event()
Line 329:         self._liveMergeCleanupThreads = {}


-- 
To view, visit https://gerrit.ovirt.org/49972
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I46bb51769562fbb9d816d190c2aebfd0660cc3b0
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list