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