Change in vdsm[master]: vdsm: Create VMs for the POWER architecture

danken at redhat.com danken at redhat.com
Thu Jan 2 19:51:48 UTC 2014


Dan Kenigsberg has posted comments on this change.

Change subject: vdsm: Create VMs for the POWER architecture
......................................................................


Patch Set 9: Code-Review-1

(5 comments)

relatively minor comments, let's get it done for 3.4!

....................................................
File vdsm/caps.py
Line 340:         elif targetArch == Architecture.PPC64:
Line 341:             caps['cpuModel'] = 'POWER 7 (fake)'
Line 342:             caps['cpuFlags'] = 'powernv,model_POWER7_v2.3'
Line 343:         else:
Line 344:             raise RuntimeError('Unsupported architecture')
it would be more informative if you include the targetArch in the string.
Line 345:     else:
Line 346:         caps['cpuModel'] = cpuInfo.model()
Line 347:         caps['cpuFlags'] = ','.join(cpuInfo.flags() +
Line 348:                                     _getCompatibleCpuModels())


....................................................
File vdsm/clientIF.py
Line 392:                         self.log.warning('vm %s already exists' %
Line 393:                                          vmParams['vmId'])
Line 394:                         return errCode['exist']
Line 395: 
Line 396:                 if config.getboolean('vars', 'fake_kvm_support'):
same code exists in caps.get(). can we have a caps.getTargetArch() to be used in bothe cases?

May we keep targetArch internal to vm.Vm() (by calling caps.getTargetArch() from within Vm.__init__? it can still be overwritten for testing purposes, or you can mock .getTargetArch.
Line 397:                     targetArch = config.get('vars', 'fake_kvm_architecture')
Line 398:                 else:
Line 399:                     targetArch = platform.machine()
Line 400: 


....................................................
File vdsm/vm.py
Line 2025:         """
Line 2026:         Normalize video device provided by conf.
Line 2027:         """
Line 2028: 
Line 2029:         defaultVideo = {caps.Architecture.X86_64: 'cirrus',
this dictionary is not expect to change, calling it DEFAULT_VIDEOS would make this clearer.
Line 2030:                         caps.Architecture.PPC64: 'vga'}
Line 2031:         if (self.arch not in defaultVideo):
Line 2032:             raise RuntimeError('Unsupported architecture')
Line 2033: 


Line 2028: 
Line 2029:         defaultVideo = {caps.Architecture.X86_64: 'cirrus',
Line 2030:                         caps.Architecture.PPC64: 'vga'}
Line 2031:         if (self.arch not in defaultVideo):
Line 2032:             raise RuntimeError('Unsupported architecture')
please include self.arch in the string, for debug-ability.
Line 2033: 
Line 2034:         vcards = []
Line 2035:         if self.conf.get('display') == 'vnc':
Line 2036:             devType = defaultVideo[self.arch]


....................................................
File vdsm_hooks/faqemu/before_vm_start.py
Line 21: 
Line 22: import hooking
Line 23: from vdsm.config import config
Line 24: 
Line 25: 
intentional?
Line 26: if config.getboolean('vars', 'fake_kvm_support'):
Line 27:     domxml = hooking.read_domxml()
Line 28:     domxml.documentElement.setAttribute("type", "qemu")
Line 29: 


-- 
To view, visit http://gerrit.ovirt.org/18718
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba5026f254d8212f522836f856c2027c04b1d247
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vitor de Lima <vitor.lima at eldorado.org.br>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa <gustavo.pedrosa at eldorado.org.br>
Gerrit-Reviewer: Leonardo Bianconi <leonardo.bianconi at eldorado.org.br>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: Vitor de Lima <vitor.lima at eldorado.org.br>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list