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

apahim at redhat.com apahim at redhat.com
Mon Sep 29 13:23:26 UTC 2014


Amador Pahim has posted comments on this change.

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


Patch Set 15:

(5 comments)

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

Line 1134:     flags |= os.O_NONBLOCK
Line 1135:     fcntl.fcntl(fd, fcntl.F_SETFL, flags)
Line 1136: 
Line 1137: 
Line 1138: def setRpFilter(dev, mode):
> Please use new-style function names for new functions in this module.
Done
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 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 f
Done
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
Done
Line 550:     sessionInfo = getSessionInfo(sessionID)
Line 551: 
Line 552:     if sessionInfo.iface.netIfacename:
Line 553:         for session in iterateIscsiSessions():


Line 559:                            session.id))
Line 560:                 return
Line 561: 
Line 562:         supervdsm.getProxy().setRpFilter(sessionInfo.iface.netIfacename,
Line 563:                                          _SYSCTL_RPFILTER_STRICT)
> I suggest to save the origin rp_filter value before it is changed by setRpF
The problem with this approach is the fact we may have several sessions using the same netIfacename. So, to do what you're suggesting, we need to save the value for rp_filter before the first session touch it and keep this value until no session is using the netIfacename anymore.

I'd prefer to keep the current approach, since users can set the value they want in net.ipv4.conf.all.rp_filter (kernel will use the MAX between "all" and "eth*")


http://gerrit.ovirt.org/#/c/31529/15/vdsm/vdsm-sysctl.conf
File vdsm/vdsm-sysctl.conf:

Line 8: # accomodate this case:
Line 9: # For the ARP flux problem, where the machine may respond to ARP requests
Line 10: # from all Ethernet interfaces, we are setting "arp_ignore=1" and
Line 11: # "arp_announce=2"
Line 12: net.ipv4.conf.default.arp_ignore = 1
Dan, some patch sets ago we changed this from *all* to *default*. Reading now about the difference between it, I've found *default* gets copied to newly created interfaces only once, while *all* allways gets applied in addition to the current specific device setting (for arp_ignore and arp_announce, kernel will pick MAX up). Using *default*:

- Newly created devices will copy the value from *default*.
- Changes in *all* will only have effect if the number is greater than the used by device.
- Existent devices will not copy the value from *default*.

So, to assure this will work, we should use *all* or control it the same way we are going to do with rp_filter.

Could you comment?


-- 
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: jian wang <wjian84 at gmail.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list