Dan Kenigsberg has posted comments on this change.
Change subject: clientIF: Check image alignment using virt-alignment-scan
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(4 inline comments)
....................................................
File tests/alignmentScanTests.py
Line 32: cmd = ["/sbin/sfdisk", "-uS", "--force", imagepath]
Line 33: cmd_input = "128,,\n" if aligned else "1,,\n"
Line 34: rc, out, err = execCmd(cmd, data=cmd_input)
Line 35: assert rc == 0
Line 36:
what was wrong with the former validation? I do not understand why you bothered
to touch this module.
Line 37:
Line 38: class AlignmentScanTests(TestCaseBase):
Line 39:
Line 40: def test_help_response(self):
....................................................
File vdsm/alignmentScan.py
Line 59: # Successful exit, some partitions have alignment < 4K
Line 60: # which can result in poor performance on most hypervisors.
Line 61: pass
Line 62: else:
Line 63: raise Exception("Unexpected return code from "
why have you changed the specific exception into a generic one?
Line 64: "virt-alignment-scan: %d" % rc)
Line 65: outList = []
Line 66: for line in out:
Line 67: outList.append(ScanOutput(*line.split(None, 3)))
....................................................
File vdsm/clientIF.py
Line 399: self.teardownVolumePath(drive['GUID'])
Line 400:
Line 401: aligning = {}
Line 402: for line in out:
Line 403: aligning[out[line]['partitionName']] = (
I'm surprised that
out[line]
works - I thought that "out" is a list of ScanOutputs, the index has to be a
number.
Line 404: out[line]['alignmentScanResult'] == "ok")
Line 405: return aligning
Line 406:
Line 407: def _recoverExistingVms(self):
Line 400:
Line 401: aligning = {}
Line 402: for line in out:
Line 403: aligning[out[line]['partitionName']] = (
Line 404: out[line]['alignmentScanResult'] == "ok")
I think it's pretty ugly to expose the "ok" string in this level. The original
virtAlignScan code returned a boolean, which is what you actually care about.
Also, it would be nice to use the named tuple features:
out[0].alignmentScanResult
Line 405: return aligning
Line 406:
Line 407: def _recoverExistingVms(self):
Line 408: try:
--
To view, visit http://gerrit.ovirt.org/12003
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia925f5f138948acca623f6379b7b811474a43ffe
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Eduardo <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Yeela Kaplan <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
