Change in vdsm[master]: deployUtil: Add logging.error() to CPU virt check

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has uploaded a new change for review.

Change subject: deployUtil: Add logging.error() to CPU virt check
......................................................................

deployUtil: Add logging.error() to CPU virt check

Adding logging.error() msg to help users in case CPU
virt support is disable in BIOS.

Change-Id: I3baff7594ad2cfcf4844a30586a2e4a4313a1972
Signed-off-by: Douglas Schilling Landgraf 
---
M vdsm_reg/deployUtil.py.in
1 file changed, 3 insertions(+), 0 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/49/3549/1
--
To view, visit http://gerrit.ovirt.org/3549
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3baff7594ad2cfcf4844a30586a2e4a4313a1972
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf

Ayal Baron has posted comments on this change.

Change subject: deployUtil: Add logging.error() to CPU virt check
......................................................................

Patch Set 1: I would prefer that you didn't submit this

(1 inline comment)

....................................................
File vdsm_reg/deployUtil.py.in
Line 1346:         if has_cpu and bios_en == False:
why bios_en gets a message but has_cpu doesn't?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3baff7594ad2cfcf4844a30586a2e4a4313a1972
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg

Change in vdsm[master]: virtEnabledInCpuAndBios: return status/proper msg

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: virtEnabledInCpuAndBios: return status/proper msg
......................................................................

Patch Set 2: Verified

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3baff7594ad2cfcf4844a30586a2e4a4313a1972
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf

Change in vdsm[master]: virtEnabledInCpuAndBios: return status/proper msg

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: virtEnabledInCpuAndBios: return status/proper msg
......................................................................

Patch Set 2: I would prefer that you didn't submit this

(2 inline comments)

minor comments

....................................................
File vdsm_reg/deployUtil.py.in
Line 1364:         retCPU = bios_en and has_cpu
retCPU is a very odd name for a variable (not that I understand why you need a variable here)

Line 1369:         return False, "Exception"
"Exception" is not very useful give at least str(e) or the whole traceback.format_exc().

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3baff7594ad2cfcf4844a30586a2e4a4313a1972
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf

Change in vdsm[master]: virtEnabledInCpuAndBios: return status/proper msg

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: virtEnabledInCpuAndBios: return status/proper msg
......................................................................

Patch Set 3: Verified

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3baff7594ad2cfcf4844a30586a2e4a4313a1972
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf

Change in vdsm[master]: virtEnabledInCpuAndBios: return status/proper msg

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: virtEnabledInCpuAndBios: return status/proper msg
......................................................................

Patch Set 3: I would prefer that you didn't submit this

(2 inline comments)

....................................................
File vds_bootstrap/vds_bootstrap.py
Line 280:             retCPU, msg = deployUtil.virtEnabledInCpuAndBios()
I still do not like the name of the retCPU variable!

it is not just about cpu!

....................................................
File vdsm_reg/deployUtil.py.in
Line 1303: def virtEnabledInCpuAndBios():
it is not your fault --- since this whole module has this problem --- but this function's implementation is wrong, and you are not making it better... It wasn't clear to me on the previous round. sorry.

this library function should NOT swallow all exceptions. and it should not return an English message. It should return two booleans - about cpu and about bios.

the calling function should convert these booleans to proper strings (you can use a dict for that), and handle exceptions.

Even if we cannot fix this now, I'd like to make my opinion heard.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3baff7594ad2cfcf4844a30586a2e4a4313a1972
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf

Change in vdsm[master]: virtEnabledInCpuAndBios: return status/proper msg

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: virtEnabledInCpuAndBios: return status/proper msg
......................................................................

Patch Set 3: (1 inline comment)

....................................................
File vdsm_reg/deployUtil.py.in
Line 1303: def virtEnabledInCpuAndBios():
actually, this whole function was born in sin. a function should have one function. not two. Having two functions:

virtEnabledInCpu()
 virtEnabledInBios()

would have been better.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3baff7594ad2cfcf4844a30586a2e4a4313a1972
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf

Change in vdsm[master]: virtEnabledInCpuAndBios: return status/proper msg

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: virtEnabledInCpuAndBios: return status/proper msg
......................................................................

Patch Set 3: No score

Build Started http://jenkins.ovirt.info/job/vdsm_unit_tests_by_patch/143/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3baff7594ad2cfcf4844a30586a2e4a4313a1972
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: virtEnabledInCpuAndBios: return status/proper msg

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: virtEnabledInCpuAndBios: return status/proper msg
......................................................................

Patch Set 3:

Build Successful

http://jenkins.ovirt.info/job/vdsm_unit_tests_by_patch/143/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3baff7594ad2cfcf4844a30586a2e4a4313a1972
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: virtEnabledInCpuAndBios: return status/proper msg

Ayal Baron has posted comments on this change.

Change subject: virtEnabledInCpuAndBios: return status/proper msg
......................................................................

Patch Set 3: I would prefer that you didn't submit this

(6 inline comments)

....................................................
File vds_bootstrap/vds_bootstrap.py
Line 276:         self.message = ''
Line 277:         self.rc = True
Line 278: 
Line 279:         if self.rc:
Line 280:             retCPU, msg = deployUtil.virtEnabledInCpuAndBios()
s/retCPU/virtSupport/ ?
Line 281:             if retCPU:
Line 282:                 self.vt_svm = "OK"
Line 283:                 self.message = msg
Line 284:             else:

....................................................
File vdsm_reg/deployUtil.py.in
Line 1299:             elif v == 'AuthenticAMD':
Line 1300:                 return v
Line 1301:     return ''
Line 1302: 
Line 1303: def virtEnabledInCpuAndBios():
+1

In the least need a TODO
Line 1304:     try:
Line 1305:         vendor = cpuVendorID()
Line 1306:         logging.debug('CPU vendor is %s', vendor)
Line 1307:

Line 1304:     try:
Line 1305:         vendor = cpuVendorID()
Line 1306:         logging.debug('CPU vendor is %s', vendor)
Line 1307: 
Line 1308:         msg = "Server supports virtualization"
This message should not be configured here as Dan said.
Line 1309: 
Line 1310:         if vendor == 'GenuineIntel':
Line 1311:             bios_en = _vmx_enabled_by_bios()
Line 1312:             has_cpu = _cpu_has_vmx_support();

Line 1319: 
Line 1320:         logging.debug('Virtualization support in cpu: %s, in bios: %s',
Line 1321:             has_cpu, bios_en)
Line 1322: 
Line 1323:         if has_cpu and bios_en == False:
The semantics here are weird.
either 'if has_cpu and not bios_en'
or 'if has_cpu == True and bios_en == False'
Line 1324:             msg = 'Please enable the virtualization support in BIOS!'
Line 1325: 
Line 1326:         if has_cpu == False:
Line 1327:             msg = 'CPU model doesn't have virtualization support!'

Line 1322: 
Line 1323:         if has_cpu and bios_en == False:
Line 1324:             msg = 'Please enable the virtualization support in BIOS!'
Line 1325: 
Line 1326:         if has_cpu == False:
Need to reverse the order of the two 'if's so that you'd have:
if not has_cpu:
 msg = ...
elif not bios_en:
 msg = ...
else:
 msg = "Server supports virtualization"
Line 1327:             msg = 'CPU model doesn't have virtualization support!'
Line 1328: 
Line 1329:         return bios_en and has_cpu, msg
Line 1330:     except:

Line 1328: 
Line 1329:         return bios_en and has_cpu, msg
Line 1330:     except:
Line 1331:         logging.error(traceback.format_exc())
Line 1332:         return False, traceback.format_exc()
why are you returning the traceback? (at most str(e))
I would just raise here
Line 1333: 
Line 1334: def getRemoteFile(IP, port, fileName, timeout=None, certPath=None):
Line 1335:     logging.debug("getRemoteFile start. IP = %s port = %s fileName = "%s""
Line 1336:             % (IP, port, fileName))

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3baff7594ad2cfcf4844a30586a2e4a4313a1972
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: virtEnabledInCpuAndBios: return status/proper msg

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has abandoned this change.

Change subject: virtEnabledInCpuAndBios: return status/proper msg
......................................................................

Patch Set 3: Abandoned

otopi does that.