Change in vdsm[master]: Skip netdev probe from sessions missing iscsi sysfs (i.e. us...

nsoffer at redhat.com nsoffer at redhat.com
Wed Mar 11 12:13:30 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: Skip netdev probe from sessions missing iscsi sysfs (i.e. using hardware iSCSI)
......................................................................


Patch Set 5: Code-Review-1

(5 comments)

https://gerrit.ovirt.org/#/c/38354/5//COMMIT_MSG
Commit Message:

Line 19: Now we learned the hard way that there is a valid case for missing
Line 20: /sys/devices/platform/hostN/sessionS, which is using hardware iSCSI (qla4xxx).
Line 21: 
Line 22: This patch is fixing the code to allow missing
Line 23: /sys/devices/platform/hostN/sessionS.
This is wrong. It should fix the code so when using hardware iscsi, we do fail when this path does not exists.

There is probably additional check that we can do to verify this this is hardware iscsi.

Can you point us to documentation about this?
Line 24: 
Line 25: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1197292
Line 26: Change-Id: Ie6f273fddff2235f15197c6689de5e6374fda6f0


https://gerrit.ovirt.org/#/c/38354/5/vdsm/storage/iscsi.py
File vdsm/storage/iscsi.py:

Line 125:     iscsi_connection = getIscsiConnectionPath(sessionID)
Line 126: 
Line 127:     try:
Line 128:         iscsi_host = getIscsiHostPath(sessionID)
Line 129:         netdev = os.path.join(iscsi_host, "netdev")
This line cannot raise NoSessionPath - so you should use else block (see bellow)
Line 130:     except NoSessionPath:
Line 131:         netdev = None
Line 132: 
Line 133:     if not os.path.isdir(iscsi_session) or not os.path.isdir(iscsi_connection):


Line 126: 
Line 127:     try:
Line 128:         iscsi_host = getIscsiHostPath(sessionID)
Line 129:         netdev = os.path.join(iscsi_host, "netdev")
Line 130:     except NoSessionPath:
You did not address my concern about no host path - how do we know that this is caused by hardware iscsi and not some error in our code/system?
Line 131:         netdev = None
Line 132: 
Line 133:     if not os.path.isdir(iscsi_session) or not os.path.isdir(iscsi_connection):
Line 134:         raise OSError(errno.ENOENT, "No such session")


Line 127:     try:
Line 128:         iscsi_host = getIscsiHostPath(sessionID)
Line 129:         netdev = os.path.join(iscsi_host, "netdev")
Line 130:     except NoSessionPath:
Line 131:         netdev = None
else:
    netdev = None
Line 132: 
Line 133:     if not os.path.isdir(iscsi_session) or not os.path.isdir(iscsi_connection):
Line 134:         raise OSError(errno.ENOENT, "No such session")
Line 135: 


Line 146:     res = []
Line 147:     for fname in (targetname, iface, tpgt, user, passwd, paddr, pport, netdev):
Line 148:         if fname is None:
Line 149:             res.append("")
Line 150:             continue
Are we ok with None value in any other value than netdev?

Since this patch is fixing the issue with netdev, we should handle only None netdev here.
Line 151:         try:
Line 152:             with open(fname, "r") as f:
Line 153:                 res.append(f.read().strip())
Line 154:         except (OSError, IOError):


-- 
To view, visit https://gerrit.ovirt.org/38354
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6f273fddff2235f15197c6689de5e6374fda6f0
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amador Pahim <apahim at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Amador Pahim <apahim at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipchuk at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list