Nir Soffer has posted comments on this change. Change subject: packaging: setup: showing the GUID instead of the physical dev on iSCSI setup ......................................................................
Patch Set 1: Code-Review-1 (3 comments) I this should be broken to several patches: 1. Use the lun GUID instead of lun number (_customize_lun should return this value 2. Display the lun guid instead of the device name (this patch) http://gerrit.ovirt.org/#/c/37494/1/src/plugins/ovirt-hosted-engine-setup/storage/iscsi.py File src/plugins/ovirt-hosted-engine-setup/storage/iscsi.py: Line 224 Line 225 Line 226 Line 227 Line 228 You should return the GUID of the lun, not the lun number, which is the value you need later to use lun. Line 189: for entry in available_luns: Line 190: lun = None Line 191: for pathstatus in entry['pathstatus']: Line 192: lun = pathstatus['lun'] Line 193: if lun: This is incorrect and not clear. - You should show all luns one or more active paths (those with 'state': 'active') - You should not show pathstatus['lun'] since this number is unique for each path, and an entry may have multiple paths. So the output format should be: index guid size index is the index used to select a lun from the list, not the pathstatus["lun"]. Regarding luns that have only failed paths, you can consider showing them in some failed state, which may be more clear when a user does not see an expected lun in the list, but lets handle this in another patch. For example of getting luns with one or more active path, check here: http://gerrit.ovirt.org/#/c/31875/3/vdsm/storage/multipath.py,cm Line 194: lun_guid[lun] = entry['GUID'] Line 195: lun_size[lun] = int(entry['capacity']) / pow(2, 20) Line 196: values = lun_guid.keys() Line 197: values.sort() Line 191: for pathstatus in entry['pathstatus']: Line 192: lun = pathstatus['lun'] Line 193: if lun: Line 194: lun_guid[lun] = entry['GUID'] Line 195: lun_size[lun] = int(entry['capacity']) / pow(2, 20) Why maintain 2 lists? Can be nicer as a list of tuples. Generally this code should be something like this: luns = [] for entry in available_luns: if self._lun_is_active(entry): guid = entry["GUID"] size = int(entry['capacity']) / pow(2, 20) luns.append((guid, size)) And then when you format the list: for i, (guid, size) in enumerate(luns): lun_list += '\t {i} - {guid} - {size}MB\n'.format( i=i, guid=guid, size=size) Or do the filtering in _iscsi_get_lun_list, so you can work directly with the available_luns like this: for i, entry in enumerate(available_luns): lun_list += '\t {i} - {guid} - {size}MB\n'.format( i=i, guid=entry["GUID"], size=int(entry['capacity']) / pow(2, 20)) Line 196: values = lun_guid.keys() Line 197: values.sort() Line 198: for lunid in values: Line 199: lun_list += '\t {lunid} - {GUID} - {size}MB\n'.format( -- To view, visit http://gerrit.ovirt.org/37494 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic8cd9c02e8c753db5df6816a38317b493c9b1e7f Gerrit-PatchSet: 1 Gerrit-Project: ovirt-hosted-engine-setup Gerrit-Branch: master Gerrit-Owner: Simone Tiraboschi <[email protected]> Gerrit-Reviewer: Lev Veyde <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Sandro Bonazzola <[email protected]> Gerrit-Reviewer: Simone Tiraboschi <[email protected]> Gerrit-Reviewer: Yedidyah Bar David <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
