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

danken at redhat.com danken at redhat.com
Thu Sep 25 22:25:16 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 15: Code-Review-1

(2 comments)

http://gerrit.ovirt.org/#/c/31529/15/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 1139:     try:
Line 1140:         path = '/proc/sys/net/ipv4/conf/%s/rp_filter' % dev
Line 1141:         with open(path, 'w') as rp_filter:
Line 1142:             rp_filter.write(mode)
Line 1143:     except OSError as e:
why do we need this error handling? Why do we want to ignore ENOENT? This function should fail hard if it fails to set the rp filter for the device dev.
Line 1144:         if e.errno == errno.ENOENT:
Line 1145:             logging.warning("Device %s does not exist", dev)
Line 1146:         else:
Line 1147:             logging.error("Error setting rp_filter mode [%s] for device [%s]",


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

Line 545: 
Line 546:     return 0
Line 547: 
Line 548: 
Line 549: def _unsetRpFilter(sessionID):
The name of this function is misleading since
* it is named like setRpFilter(), but does not work like it
* it does not always unset; it only tries to do so.

too bad I don't have a good name. Maybe

 setStrictRpFilterIfUnique()

Please a docstring explaining why this function is needed
Line 550:     sessionInfo = getSessionInfo(sessionID)
Line 551: 
Line 552:     if sessionInfo.iface.netIfacename:
Line 553:         for session in iterateIscsiSessions():


-- 
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: 15
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