Ayal Baron has posted comments on this change.
Change subject: multipath: Remove unused 'deduceType' and MIXED_DEV
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(4 inline comments)
....................................................
File vdsm/storage/hsm.py
Line 1831:
Line 1832: # FIXME: pathListIter() should not return empty records
Line 1833: for dev in multipath.pathListIter(guids):
Line 1834: if ((storageType and dev["devtype"] != storageType) or
Line 1835: (not storageType)):
why "or (not storageType)" ?
This looks like you've reversed the logic (lambda dev: True) and basically means
you're skipping the entire loop in case storageType was not passed.
If this were correct you could move the if not storageType before the 'for' loop
and avoid the call to pathListIter.
Am I missing something here?
Line 1836: continue
Line 1837:
Line 1838: pv = pvs.get(dev.get('guid', ""))
Line 1839: if pv is not None:
Line 2771: if dev is None:
Line 2772: self.log.warn("dev %s was not found in %s",
Line 2773: getGuid(pv), pathDict)
Line 2774: continue
Line 2775: if vgInfo["type"] is None:
why not use same logic here?
i.e.
devTypes = set()
...
devTypes.update(dev["devtype"])
...
if len(devTypes) > 1:
vgInfo["type"] = multipath.DEV_ISCSI
else:
vgInfo["type"] = devTypes.pop()
?
Line 2776: vgInfo["type"] = dev["devtype"]
Line 2777: elif vgInfo["type"] != dev["devtype"]:
Line 2778: vgInfo["type"] = multipath.DEV_ISCSI
Line 2779:
....................................................
File vdsm/storage/multipath.py
Line 38:
Line 39: import storage_exception as se
Line 40:
Line 41: DEV_FCP = 2
Line 42: DEV_ISCSI = 3
you've changed these to match the sd definitions of FCP_DOMAIN and ISCSI_DOMAIN for
the filtering in hsm.
This means that we're now keeping 2 sets of constants that need to contain the same
values which is of course problematic.
Since we don't want to import sd.py here nor change the value in sd to point to these
values, the only place I can think of to reconcile the 2 is in hsm (i.e. not assume
they're the same value but rather have a dictionary in hsm that translates or
something.
Alternatively you can refactor the "domain types" out of sd.py into a separate
module and rename to storage connection types or something, but that's a lot more work
and I'm not sure it's worth it.
Line 43:
Line 44: MAX_CONF_COPIES = 5
Line 45:
Line 46: TOXIC_CHARS = '()*+?|^$.\\'
Line 354: pathInfo["type"] = DEV_FCP
Line 355:
Line 356: devInfo["paths"].append(pathInfo)
Line 357:
Line 358: if len(devtypes) > 1:
need a comment here to state that if the device contains both FCP and iSCSI paths we treat
it as iSCSI as that is the connection type we need to manage.
Line 359: devInfo["devtype"] = DEV_ISCSI
Line 360: else:
Line 361: devInfo["devtype"] = devtypes.pop()
Line 362:
--
To view, visit
http://gerrit.ovirt.org/13363
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I37a94c3d67641f1c78d8fbecd63cbf1480c6e1b0
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan <ykaplan(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server