Change in vdsm[master]: Sysctl to allow iSCSI multipath with multiple NICs in the sa...

danken at redhat.com danken at redhat.com
Fri Sep 12 09:37:05 UTC 2014


Dan Kenigsberg has posted comments on this change.

Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet
......................................................................


Patch Set 12: Code-Review-1

(2 comments)

This patch has a nasty side effect - it loosens rp-filter even for iscsi sessions that does not need it, which may be insecure if netInterface is a bridge.

How are we going to block/warn about this?

http://gerrit.ovirt.org/#/c/31529/12/vdsm/storage/iscsi.py
File vdsm/storage/iscsi.py:

Line 187:                     iscsiadm.node_update(iface.name, portalStr, target.iqn,
Line 188:                                          key, value, hideValue=True)
Line 189: 
Line 190:             if iface.netIfacename:
Line 191:                 setIscsiIfaceSysctlRpfilter(iface.netIfacename, '2')
Please define a CONSTANT. '2' is not very clear.
Line 192: 
Line 193:             iscsiadm.node_login(iface.name, portalStr, target.iqn)
Line 194: 
Line 195:             iscsiadm.node_update(iface.name, portalStr, target.iqn,


Line 535:     # FIXME : Should throw exception on error
Line 536:     sessionID = int(sessionID)
Line 537:     sessionInfo = getSessionInfo(sessionID)
Line 538: 
Line 539:     if sessionInfo.iface.netIfacename:
That's not good enough. You should iterate through all other sessions, and only if none of them is using the netIface, reset its value. A single network interface may be used by multiple sessions.

Here, too, a

 _SYSCTL_RPFILTER_STRICT = '0'

would make things more readable.
Line 540:         setIscsiIfaceSysctlRpfilter(sessionInfo.iface.netIfacename, '0')
Line 541: 
Line 542:     try:
Line 543:         iscsiadm.session_logout(sessionID)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf93d49317c76aece764e53e58e0ff28868f16b0
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amador Pahim <apahim at redhat.com>
Gerrit-Reviewer: Amador Pahim <apahim at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgotliv 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