Nir Soffer has posted comments on this change. Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 20: (14 comments) http://gerrit.ovirt.org/#/c/31529/20/vdsm/storage/iscsi.py File vdsm/storage/iscsi.py: Line 192: Line 193: iscsiadm.node_update(iface.name, portalStr, target.iqn, Line 194: "node.startup", "manual") Line 195: except: Line 196: removeIscsiNode(iface, target) > Shouldn't we add the restoreStrictRpFilterIfPossible here, or even better i We should Line 197: raise Line 198: Line 199: Line 200: def removeIscsiNode(iface, target): Line 539: Line 540: return 0 Line 541: Line 542: Line 543: def findSessions(netIfaceName): > Shouldn't this be private at this time? It should Line 544: """ Return list of sessions using netIfaceName """ Line 545: Line 546: return [session for session in iterateIscsiSessions() Line 547: if session.iface.netIfaceName == netIfaceName] Line 546: return [session for session in iterateIscsiSessions() Line 547: if session.iface.netIfaceName == netIfaceName] Line 548: Line 549: Line 550: def needsLooseRpFilter(netIfaceName, dst): > Shouldn't this be private at this time? It should Line 551: """ Line 552: Check if provided netIfaceName is the device used by OS to reach the "dst" Line 553: IP address. If not, rp_filter needs to be set. Only needed before the first Line 554: session using the netIfaceName to the provided IP. Next sessions will take Line 555: advantage of the already configured rp_filter. Line 556: Line 557: Arguments: Line 558: netIfaceName: the device used by the iSCSI session Line 559: dst: the iSCSI session destination IP address dst is little too cryptic here. You call it "hostname" in restoreStrictRpFilterIfPossible, so maybe it is a better term. Line 560: Line 561: Returns: Line 562: Boolean. Line 563: """ Line 566: Line 567: if netIfaceName != getRouteDeviceTo(dst) and len(sessions) == 0: Line 568: return True Line 569: else: Line 570: return False This works, but will be nicer as: return len(sessions) == 0 and netIfaceName != getRouteDeviceTo(dst) Line 571: Line 572: Line 573: def setLooseRpFilterIfNeeded(iface, target): Line 574: """ Line 569: else: Line 570: return False Line 571: Line 572: Line 573: def setLooseRpFilterIfNeeded(iface, target): We use only iface.netIfaceName here, so it seems better to accept netIfaceName instead of iface, and will make this more consistent with needsLooseRpFilter(). Same for target, we use only the target.portal.hostname. Line 574: """ Line 575: Set rp_filter to loose mode if needed, in order to allow multiple iSCSI Line 576: connections in a multiple NIC per subnet configuration. Line 577: Line 571: Line 572: Line 573: def setLooseRpFilterIfNeeded(iface, target): Line 574: """ Line 575: Set rp_filter to loose mode if needed, in order to allow multiple iSCSI If needed is not clear. Do you mean - if this is the first session needing loose rp filter? Line 576: connections in a multiple NIC per subnet configuration. Line 577: Line 578: Arguments: Line 579: iface: iSCSI iface object containing the netIfaceName. Line 580: target: iSCSI target object cointaining the portal hostname. Line 581: """ Line 582: Line 583: netIfaceName = iface.netIfaceName Line 584: if netIfaceName is not None: Better use this pattern: if netIfaceName is None: log ... return continue with normal flow Line 585: hostname = target.portal.hostname Line 586: if needsLooseRpFilter(netIfaceName, hostname): Line 587: supervdsm.getProxy().set_rp_filter_loose(netIfaceName) Line 588: log.info("Setting loose mode rp_filter for device [%s]." % Line 584: if netIfaceName is not None: Line 585: hostname = target.portal.hostname Line 586: if needsLooseRpFilter(netIfaceName, hostname): Line 587: supervdsm.getProxy().set_rp_filter_loose(netIfaceName) Line 588: log.info("Setting loose mode rp_filter for device [%s]." % Instead of [%s] you can use %r Line 589: netIfaceName) Line 590: Line 591: Line 592: def restoreStrictRpFilterIfPossible(sessionInfo): Line 588: log.info("Setting loose mode rp_filter for device [%s]." % Line 589: netIfaceName) Line 590: Line 591: Line 592: def restoreStrictRpFilterIfPossible(sessionInfo): This is the opposite of setLooseRpFilterIfNeeded - right? Then why do you use "IfPossible" here an not "IfNeeded"? Also, it would be nice to use the same signature in both functions: restoreStrictRpFilterIfNeeded(iface, target) Or: restoreStrictRpFilterIfNeeded(netIfaceName, hostname) Line 593: """ Line 594: Set rp_filter back to strict mode when possible. Line 595: Line 596: This is needed to avoid the security breach where an untrusted VM can DoS Line 590: Line 591: Line 592: def restoreStrictRpFilterIfPossible(sessionInfo): Line 593: """ Line 594: Set rp_filter back to strict mode when possible. when possible is not clear. Do you mean - if this is the last session that needed a loose rp fllter? Line 595: Line 596: This is needed to avoid the security breach where an untrusted VM can DoS Line 597: the host by sending it packets with spoofed random sources. Line 598: Line 600: sessionInfo: iSCSI sessionInfo object. Line 601: """ Line 602: Line 603: netIfaceName = sessionInfo.iface.netIfaceName Line 604: if netIfaceName is not None: Same comment about early return on None Line 605: hostname = sessionInfo.target.portal.hostname Line 606: if needsLooseRpFilter(netIfaceName, hostname): Line 607: supervdsm.getProxy().set_rp_filter_strict(netIfaceName) Line 608: log.info("Setting strict mode rp_filter for device [%s]." % Line 602: Line 603: netIfaceName = sessionInfo.iface.netIfaceName Line 604: if netIfaceName is not None: Line 605: hostname = sessionInfo.target.portal.hostname Line 606: if needsLooseRpFilter(netIfaceName, hostname): This name works nicely when setting loose filter, but looks wrong here. I cannot think of better name now. Line 607: supervdsm.getProxy().set_rp_filter_strict(netIfaceName) Line 608: log.info("Setting strict mode rp_filter for device [%s]." % Line 604: if netIfaceName is not None: Line 605: hostname = sessionInfo.target.portal.hostname Line 606: if needsLooseRpFilter(netIfaceName, hostname): Line 607: supervdsm.getProxy().set_rp_filter_strict(netIfaceName) Line 608: log.info("Setting strict mode rp_filter for device [%s]." % [%s] -> %r -- 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: 20 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Amador Pahim <apa...@redhat.com> Gerrit-Reviewer: Amador Pahim <apa...@redhat.com> Gerrit-Reviewer: Antoni Segura Puimedon <t...@midokura.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com> Gerrit-Reviewer: Yoav Kleinberger <yklei...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: jian wang <wjia...@gmail.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
