Amador Pahim has uploaded a new change for review.
Change subject: [RFC] Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
[RFC] Sysctl to allow iSCSI multipath with multiple NICs in the same subnet
When two or more interfaces are in the same subnet, the default OS behaviour won't allow the independent traffic between local NICs and iSCSI targets.
These sysctl tweaks are needed to have iSCSI multipath working in such topology:
eth1 (192.168.25.200) --| |-- iSCSI Target (192.168.25.10) eth2 (192.168.25.201) --|
The proposed changes are reccomended by the storage vendor and are described in documents bellow: - http://en.community.dell.com/dell-groups/dtcmedia/m/mediagallery/20371245/do... - https://support.equallogic.com/WorkArea/DownloadAsset.aspx?id=8727
Change-Id: Ibf93d49317c76aece764e53e58e0ff28868f16b0 Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1112861 Signed-off-by: Amador Pahim apahim@redhat.com --- M vdsm/vdsm-sysctl.conf 1 file changed, 11 insertions(+), 0 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/29/31529/1
diff --git a/vdsm/vdsm-sysctl.conf b/vdsm/vdsm-sysctl.conf index ecd1386..332379c 100644 --- a/vdsm/vdsm-sysctl.conf +++ b/vdsm/vdsm-sysctl.conf @@ -1,3 +1,14 @@ #Set dirty page parameters vm.dirty_ratio = 5 vm.dirty_background_ratio = 2 + +#Set ARP to avoid RST when interfaces are in the same subnet. +net.ipv4.conf.all.arp_ignore = 1 +net.ipv4.conf.all.arp_announce = 2 + +#Set rp_filter to "loose mode" allowing multiple iSCSI connections in +#a multiple NIC per subnet configuration. +net.ipv4.conf.all.rp_filter = 2 + +#Workaround for netfilter issue marking packets as invalid under heavy load. +net.netfilter.nf_conntrack_tcp_be_liberal = 1
Amador Pahim has posted comments on this change.
Change subject: [RFC] Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 1:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/1/vdsm/vdsm-sysctl.conf File vdsm/vdsm-sysctl.conf:
Line 9: #Set rp_filter to "loose mode" allowing multiple iSCSI connections in Line 10: #a multiple NIC per subnet configuration. Line 11: net.ipv4.conf.all.rp_filter = 2 Line 12: Line 13: #Workaround for netfilter issue marking packets as invalid under heavy load. Still valid?
oVirt Jenkins CI Server has posted comments on this change.
Change subject: [RFC] Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 1:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/10943/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/11885/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/11728/ : SUCCESS
Amador Pahim has posted comments on this change.
Change subject: [RFC] Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 1:
(1 comment)
Need network team review.
Question: are these tunables acceptable for any deployment?
http://gerrit.ovirt.org/#/c/31529/1/vdsm/vdsm-sysctl.conf File vdsm/vdsm-sysctl.conf:
Line 9: #Set rp_filter to "loose mode" allowing multiple iSCSI connections in Line 10: #a multiple NIC per subnet configuration. Line 11: net.ipv4.conf.all.rp_filter = 2 Line 12: Line 13: #Workaround for netfilter issue marking packets as invalid under heavy load.
Still valid?
The only references I have for this is for RHEL5: https://bugzilla.redhat.com/show_bug.cgi?id=493226
Dan Kenigsberg has posted comments on this change.
Change subject: [RFC] Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 1: Code-Review-1
(4 comments)
Thanks, Amador. Could you verify that tweaking nf_conntrack_tcp_be_liberal is indeed no longer required?
http://gerrit.ovirt.org/#/c/31529/1//COMMIT_MSG Commit Message:
Line 3: AuthorDate: 2014-08-14 14:49:29 -0300 Line 4: Commit: Amador Pahim apahim@redhat.com Line 5: CommitDate: 2014-08-14 15:37:34 -0300 Line 6: Line 7: [RFC] Sysctl to allow iSCSI multipath with multiple NICs in the same subnet every patch is a "request for comments". please drop this prefix. Line 8: Line 9: When two or more interfaces are in the same subnet, the default OS Line 10: behaviour won't allow the independent traffic between local NICs and Line 11: iSCSI targets.
http://gerrit.ovirt.org/#/c/31529/1/vdsm/vdsm-sysctl.conf File vdsm/vdsm-sysctl.conf:
Line 1: #Set dirty page parameters unrelated to this patch, but should probably flow with the times and move this file to /usr/lib/sysctl.d/ Line 2: vm.dirty_ratio = 5 Line 3: vm.dirty_background_ratio = 2 Line 4: Line 5: #Set ARP to avoid RST when interfaces are in the same subnet.
Line 3: vm.dirty_background_ratio = 2 Line 4: Line 5: #Set ARP to avoid RST when interfaces are in the same subnet. Line 6: net.ipv4.conf.all.arp_ignore = 1 Line 7: net.ipv4.conf.all.arp_announce = 2 According to https://bugzilla.redhat.com/show_bug.cgi?id=168960#c1 this sounds acceptable. Line 8: Line 9: #Set rp_filter to "loose mode" allowing multiple iSCSI connections in Line 10: #a multiple NIC per subnet configuration. Line 11: net.ipv4.conf.all.rp_filter = 2
Line 9: #Set rp_filter to "loose mode" allowing multiple iSCSI connections in Line 10: #a multiple NIC per subnet configuration. Line 11: net.ipv4.conf.all.rp_filter = 2 Line 12: Line 13: #Workaround for netfilter issue marking packets as invalid under heavy load.
The only references I have for this is for RHEL5:
... which is long solved. Unless we have proof/understanding that this is required on el6 and above, we should not keep this.
Dan Kenigsberg has posted comments on this change.
Change subject: [RFC] Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 1:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/1/vdsm/vdsm-sysctl.conf File vdsm/vdsm-sysctl.conf:
Line 9: #Set rp_filter to "loose mode" allowing multiple iSCSI connections in Line 10: #a multiple NIC per subnet configuration. Line 11: net.ipv4.conf.all.rp_filter = 2 Line 12: Line 13: #Workaround for netfilter issue marking packets as invalid under heavy load.
... which is long solved. Unless we have proof/understanding that this is r
argh, the bug does not really explain why nf_conntrack_tcp_be_liberal=1 is useful, it only complains about an ancient problem in persisting its value.
Still, I'd rather not play with it without clear explanation. Let's drop it and see how things fly.
Amador Pahim has posted comments on this change.
Change subject: [RFC] Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 1:
(2 comments)
http://gerrit.ovirt.org/#/c/31529/1//COMMIT_MSG Commit Message:
Line 3: AuthorDate: 2014-08-14 14:49:29 -0300 Line 4: Commit: Amador Pahim apahim@redhat.com Line 5: CommitDate: 2014-08-14 15:37:34 -0300 Line 6: Line 7: [RFC] Sysctl to allow iSCSI multipath with multiple NICs in the same subnet
every patch is a "request for comments". please drop this prefix.
Done Line 8: Line 9: When two or more interfaces are in the same subnet, the default OS Line 10: behaviour won't allow the independent traffic between local NICs and Line 11: iSCSI targets.
http://gerrit.ovirt.org/#/c/31529/1/vdsm/vdsm-sysctl.conf File vdsm/vdsm-sysctl.conf:
Line 9: #Set rp_filter to "loose mode" allowing multiple iSCSI connections in Line 10: #a multiple NIC per subnet configuration. Line 11: net.ipv4.conf.all.rp_filter = 2 Line 12: Line 13: #Workaround for netfilter issue marking packets as invalid under heavy load.
argh, the bug does not really explain why nf_conntrack_tcp_be_liberal=1 is
Yes, seems like something written in the storage vendor manuals and never revisited. Removing.
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 2:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/10949/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/11891/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/11734/ : SUCCESS
Sergey Gotliv has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
In general it requires network team review, patch looks good to me based on the Equalogic docs therefore +1.
http://gerrit.ovirt.org/#/c/31529/2//COMMIT_MSG Commit Message:
Line 19: Line 20: The proposed changes are reccomended by the storage vendor and are described Line 21: in documents bellow: Line 22: - http://en.community.dell.com/dell-groups/dtcmedia/m/mediagallery/20371245/do... Line 23: - https://support.equallogic.com/WorkArea/DownloadAsset.aspx?id=8727 Consider to change this commit message and summarize your changes instead sending me to read entire equalogic documentation:
ARP Flux problem described in chapter 2.1.4 here http://linux-ip.net/html/ether-arp.html and solution is described in the document mentioned by you but this is 5 lines chapter while the whole document contains 25 pages.
Other than that this patch looks ok. Line 24: Line 25: Change-Id: Ibf93d49317c76aece764e53e58e0ff28868f16b0 Line 26: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1112861
Nir Soffer has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 2:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/2/vdsm/vdsm-sysctl.conf File vdsm/vdsm-sysctl.conf:
Line 1: #Set dirty page parameters Line 2: vm.dirty_ratio = 5 Line 3: vm.dirty_background_ratio = 2 Line 4: Please add here a paragraph explaining why both settings are needed (e.g. using multipath when nics use same subnet) and the bug url. Line 5: #Set ARP to avoid RST when interfaces are in the same subnet. Line 6: net.ipv4.conf.all.arp_ignore = 1 Line 7: net.ipv4.conf.all.arp_announce = 2 Line 8:
Nir Soffer has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 2:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/2//COMMIT_MSG Commit Message:
Line 14: topology: Line 15: Line 16: eth1 (192.168.25.200) --| Line 17: |-- iSCSI Target (192.168.25.10) Line 18: eth2 (192.168.25.201) --| Does it make sense to use this setup? The reason you use multipath is to have more reliable setup, if one network fails, you can access the storage using another network. When both nics are using the same network, you don't get much from this setup, right? Line 19: Line 20: The proposed changes are reccomended by the storage vendor and are described Line 21: in documents bellow: Line 22: - http://en.community.dell.com/dell-groups/dtcmedia/m/mediagallery/20371245/do...
Sergey Gotliv has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 2:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/2//COMMIT_MSG Commit Message:
Line 14: topology: Line 15: Line 16: eth1 (192.168.25.200) --| Line 17: |-- iSCSI Target (192.168.25.10) Line 18: eth2 (192.168.25.201) --|
Does it make sense to use this setup? The reason you use multipath is to ha
Nir, I thought exactly as you do, therefore I missed that case in the first place. Unfortunately, Amador brings this case from the field which means the real customer uses the system that way.... Line 19: Line 20: The proposed changes are reccomended by the storage vendor and are described Line 21: in documents bellow: Line 22: - http://en.community.dell.com/dell-groups/dtcmedia/m/mediagallery/20371245/do...
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 3:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/10984/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/11926/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/11769/ : SUCCESS
Nir Soffer has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 4:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/4/vdsm/vdsm-sysctl.conf File vdsm/vdsm-sysctl.conf:
Line 8: net.ipv4.conf.all.arp_announce = 2 Line 9: Line 10: #Set rp_filter to "loose mode" allowing multiple iSCSI connections in a Line 11: #multiple NIC per subnet configuration (BZ#1112861) Line 12: net.ipv4.conf.all.rp_filter = 2 If I understand correctly, we have one issue (BZ 1112861), that requires the 3 changes. So we need one comment block explaining the motivation and solution, and under it we can put the 3 changes.
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 4:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/10985/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/11927/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/11770/ : SUCCESS
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 5:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/10986/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/11928/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/11771/ : SUCCESS
Dan Kenigsberg has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 5:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/5/vdsm/vdsm-sysctl.conf File vdsm/vdsm-sysctl.conf:
Line 10: # from all Ethernet interfaces, we are setting "arp_ignore=1" and Line 11: # "arp_announce=2" Line 12: # To allow multiple iSCSI connections in a multiple NIC per subnet Line 13: # configuration, reverse path filtering must be loosened with "rp_filter=2" Line 14: net.ipv4.conf.all.arp_ignore = 1 Setting net.ipv4.conf.default.arp_ignore etc. would let a local admin override these values per interface, in case they have a good reason to use different values.
Could you verify that *.default.* has the same effect on the values for interfaces used for iscsi bonding? Line 15: net.ipv4.conf.all.arp_announce = 2
Amador Pahim has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 5:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/5/vdsm/vdsm-sysctl.conf File vdsm/vdsm-sysctl.conf:
Line 10: # from all Ethernet interfaces, we are setting "arp_ignore=1" and Line 11: # "arp_announce=2" Line 12: # To allow multiple iSCSI connections in a multiple NIC per subnet Line 13: # configuration, reverse path filtering must be loosened with "rp_filter=2" Line 14: net.ipv4.conf.all.arp_ignore = 1
Setting net.ipv4.conf.default.arp_ignore etc. would let a local admin overr
Yes, same effect. Changing to default. Line 15: net.ipv4.conf.all.arp_announce = 2
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 6:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11028/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/11970/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/11813/ : SUCCESS
Nir Soffer has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 6:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/6/vdsm/vdsm-sysctl.conf File vdsm/vdsm-sysctl.conf:
Line 12: # To allow multiple iSCSI connections in a multiple NIC per subnet Line 13: # configuration, reverse path filtering must be loosened with "rp_filter=2" Line 14: net.ipv4.conf.default.arp_ignore = 1 Line 15: net.ipv4.conf.default.arp_announce = 2 Line 16: net.ipv4.conf.default.rp_filter = 2 Is it clear to to the next network hacker that we are using "default" so an admin can override this?
If not, maybe add a note about it in the comment.
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 7:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11080/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12022/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/11869/ : SUCCESS
Dan Kenigsberg has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 7: Code-Review-1
(1 comment)
http://gerrit.ovirt.org/#/c/31529/7/vdsm/vdsm-sysctl.conf File vdsm/vdsm-sysctl.conf:
Line 13: # configuration, reverse path filtering must be loosened with "rp_filter=2" Line 14: # (using "default" so an admin can override it per interface if needed) Line 15: net.ipv4.conf.default.arp_ignore = 1 Line 16: net.ipv4.conf.default.arp_announce = 2 Line 17: net.ipv4.conf.default.rp_filter = 2 According to http://www.ietf.org/rfc/rfc3704.txt (section 2.4) loose mode has security disadvantages. If an untrusted VM is connected to a bridge with an IP address and with this setting, the VM could DoS the host by sending it packages with spoofed random sources.
It would be more prudent to set loose mode only on specific interfaces that need it - though doing so properly may be a bit tedious.
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 8:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11437/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12381/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12226/ : SUCCESS
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 9:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11442/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12386/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12231/ : SUCCESS
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 10:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11446/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12390/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12235/ : SUCCESS
Dan Kenigsberg has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 10: Code-Review-1
(3 comments)
http://gerrit.ovirt.org/#/c/31529/10/lib/vdsm/constants.py.in File lib/vdsm/constants.py.in:
Line 147: EXT_SETSID = '@SETSID_PATH@' Line 148: EXT_SH = '/bin/sh' # The shell path is invariable Line 149: EXT_SU = '@SU_PATH@' Line 150: EXT_SUDO = '@SUDO_PATH@' Line 151: EXT_SYSCTL = '@SYSCTL_PATH@' We try to make this module thinner, by using CommandPath in runtime. Line 152: Line 153: EXT_TAR = '@TAR_PATH@' Line 154: EXT_TEE = '@TEE_PATH@' Line 155: EXT_TUNE2FS = '@TUNE2FS_PATH@'
http://gerrit.ovirt.org/#/c/31529/10/lib/vdsm/utils.py File lib/vdsm/utils.py:
Line 1137: Line 1138: def sysctlCmd(key, value): Line 1139: option = '%s=%s' % (key, value) Line 1140: cmd = [constants.EXT_SYSCTL, '-w', option] Line 1141: rc, out, err = execCmd(cmd, sudo=True) Please avoid using sudo - use define a new supervdsm entry point, instead. Line 1142: if rc != 0:
http://gerrit.ovirt.org/#/c/31529/10/vdsm/storage/iscsi.py File vdsm/storage/iscsi.py:
Line 163: key, value, hideValue=True) Line 164: Line 165: if iface.netIfacename: Line 166: sysctlOpt = 'net.ipv4.conf.%s.rp_filter' % iface.netIfacename Line 167: sysctlCmd(sysctlOpt, '2') The toughest thing in this approach is to properly handle removal of iscsi sessions. When no session uses the network iface, we should revert its rp_filter to strict. Line 168: Line 169: iscsiadm.node_login(iface.name, portalStr, target.iqn) Line 170: Line 171: iscsiadm.node_update(iface.name, portalStr, target.iqn,
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 11:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11479/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12423/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12268/ : SUCCESS
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 12:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11481/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12425/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12270/ : SUCCESS
Amador Pahim has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 12:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/12/vdsm/supervdsmServer File vdsm/supervdsmServer:
Line 355: Line 356: @logDecorator Line 357: def sysctlCmd(self, key, value): Line 358: option = '='.join([key, value]) Line 359: cmd = ['/usr/sbin/sysctl', '-w', option] Sorry, I left this here. Next patch set will change to CommandPath, as indicated in last review. Line 360: rc, out, err = utils.execCmd(cmd) Line 361: if rc: Line 362: self.log.error("Sysctl -w command failed rc=%s, " Line 363: "out="%s", err="%s"", rc, out, err)
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)
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 13:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11494/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12438/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12283/ : SUCCESS
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 14:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11499/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12443/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12288/ : SUCCESS
Amador Pahim has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 14:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/14/vdsm/storage/iscsi.py File vdsm/storage/iscsi.py:
Line 539: Line 540: return res Line 541: Line 542: Line 543: def disconnectiScsiSession(sessionID): Please notice there is an issue reported for engine:
https://bugzilla.redhat.com/show_bug.cgi?id=1140850
Whenever it get fixed, disconnectiScsiSession() is supposed to work out of the box. Line 544: # FIXME : Should throw exception on error Line 545: sessionID = int(sessionID) Line 546: _unsetRpFilter(sessionID) Line 547: try:
Antoni Segura Puimedon has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 14: Code-Review-1
(1 comment)
http://gerrit.ovirt.org/#/c/31529/14/vdsm/supervdsmServer File vdsm/supervdsmServer:
Line 357: @logDecorator Line 358: def sysctlCmd(self, key, value): Line 359: option = '='.join([key, value]) Line 360: cmd = [_SYSCTL_BINARY.cmd, '-w', option] Line 361: rc, out, err = utils.execCmd(cmd) Instead of starting a process like sysctl, I think it'd be best to use the newer and preferred interface of file access to /proc/sys/
for example you have: /proc/sys/net/ipv4/conf/default/arp_ignore Line 362: if rc: Line 363: self.log.error("Sysctl -w command failed rc=%s, " Line 364: "out="%s", err="%s"", rc, out, err) Line 365: raise OSError(errno.EINVAL, "Could not set sysctl option "
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 15:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11542/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12486/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12331/ : SUCCESS
Antoni Segura Puimedon 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
the use of sysfs looks good now ;-)
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():
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:
(1 comment)
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.
set_rp_filter() 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)
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?
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 16:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11721/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12665/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12510/ : SUCCESS
Amador Pahim has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 16:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/16/vdsm/vdsm-sysctl.conf File vdsm/vdsm-sysctl.conf:
Line 7: # and iSCSI targets (BZ#1112861). The kernel parameters bellow are needed to 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" 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* always 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? Line 12: net.ipv4.conf.default.arp_ignore = 1
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:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/15/vdsm/storage/iscsi.py File vdsm/storage/iscsi.py:
Line 559: session.id)) Line 560: return Line 561: Line 562: supervdsm.getProxy().setRpFilter(sessionInfo.iface.netIfacename, Line 563: _SYSCTL_RPFILTER_STRICT)
The problem with this approach is the fact we may have several sessions usi
Yes, I agree to this simplification. We should add a release note in the BZ, that if an interface is used by ovirt storage it may loose its original rpfilter setting.
Dan Kenigsberg has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 16: Code-Review+1
(1 comment)
http://gerrit.ovirt.org/#/c/31529/16/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: Sorry if I did not make myself clear. There is no point in handling the exception here. It would be logged by the caller. Please drop the whole try-except. Line 1144: logging.error("Error setting rp_filter mode [%s] for device [%s]", Line 1145: mode, dev, exc_info=True)
Dan Kenigsberg has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 16:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/16/vdsm/vdsm-sysctl.conf File vdsm/vdsm-sysctl.conf:
Line 7: # and iSCSI targets (BZ#1112861). The kernel parameters bellow are needed to 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"
Dan, some patch sets ago we changed this from *all* to *default*. Reading n
Sysctl.conf takes effect on boot, when there are no "existent devices".
" default " allows to tweak params to any value (even lower than what is set here)
Also, it was Tsirkin that suggested using default.
So unless there's a functional bug, let's keep default. Line 12: net.ipv4.conf.default.arp_ignore = 1
Amador Pahim has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 16:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/16/vdsm/vdsm-sysctl.conf File vdsm/vdsm-sysctl.conf:
Line 7: # and iSCSI targets (BZ#1112861). The kernel parameters bellow are needed to 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"
Sysctl.conf takes effect on boot, when there are no "existent devices".
Done Line 12: net.ipv4.conf.default.arp_ignore = 1
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 17:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11790/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12736/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12579/ : SUCCESS
Dan Kenigsberg has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 17: Code-Review+1
Antoni Segura Puimedon has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 17:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/17/vdsm/vdsm-sysctl.conf File vdsm/vdsm-sysctl.conf:
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 Line 13: net.ipv4.conf.default.arp_announce = 2 Have you tried this with running more than one VM on the host and making them communicate among themselves and to other machines in the network?
Amador Pahim has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 17: Verified+1
(1 comment)
http://gerrit.ovirt.org/#/c/31529/17/vdsm/vdsm-sysctl.conf File vdsm/vdsm-sysctl.conf:
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 Line 13: net.ipv4.conf.default.arp_announce = 2
Have you tried this with running more than one VM on the host and making th
Yes, I have. No issues with that in my env.
Antoni Segura Puimedon has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 17: Code-Review+1
Then it's okay for me.
Nir Soffer has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 17: Code-Review-1
(9 comments)
Setting loose rpfilter seems too loose, even when not needed.
Also suggested ways to improve the code, can be done later if we don't have time now.
http://gerrit.ovirt.org/#/c/31529/17/lib/vdsm/utils.py File lib/vdsm/utils.py:
Line 1136: Line 1137: Line 1138: def set_rp_filter(dev, mode): Line 1139: path = '/proc/sys/net/ipv4/conf/%s/rp_filter' % dev Line 1140: with open(path, 'w') as rp_filter: We don't need the default buffering in this case, so we should use unbuffered file:
open(path, 'w', 0)
Actually we don't need a file object at all. This is more efficient, but needs more code:
fd = os.open(path, os.O_WRONLY) try: os.write(fd, mode) finally: os.close()
Also I think we better move this to new sysctl module, so we have nice place for the mode constants. This module is became too big.
http://gerrit.ovirt.org/#/c/31529/17/vdsm/storage/iscsi.py File vdsm/storage/iscsi.py:
Line 44: DEFAULT_TPGT = 1 Line 45: ISCSI_DEFAULT_PORT = 3260 Line 46: SCAN_PATTERN = "/sys/class/scsi_host/host*/scan" Line 47: _SYSCTL_RPFILTER_STRICT = '0' Line 48: _SYSCTL_RPFILTER_LOOSE = '2' These constants do not belong to this module Line 49: Line 50: IscsiSession = namedtuple("IscsiSession", "id, iface, target, credentials") Line 51: Line 52: _iscsiadmTransactionLock = RLock()
Line 186: key = "node.session." + key Line 187: iscsiadm.node_update(iface.name, portalStr, target.iqn, Line 188: key, value, hideValue=True) Line 189: Line 190: if iface.netIfaceName: Why having a netIfaceName means that we need loose rpfilter?
If I understand the issue correctly, this is needed only when we have tow ifaces in the same subnet?
So it seem that this modification should be in upper layer, when we have the details of the connections, and we detect such case.
Or, like done in disconnect - set the filter only if there is another interface using the same device? Line 191: supervdsm.getProxy().setRpFilter(iface.netIfaceName, Line 192: _SYSCTL_RPFILTER_LOOSE) Line 193: Line 194: iscsiadm.node_login(iface.name, portalStr, target.iqn)
Line 188: key, value, hideValue=True) Line 189: Line 190: if iface.netIfaceName: Line 191: supervdsm.getProxy().setRpFilter(iface.netIfaceName, Line 192: _SYSCTL_RPFILTER_LOOSE) This is little too wordy, lets add a sysctl module with these functions and constants:
# lib/vdsm/sysctl.py
RPFILTER_LOOSE = ... RPFILTER_STRICT = ...
def set_rpfilter(dev, mode): supervdsm.getProxy().setRpFilter(iface.netIfaceName, mode)
# Will be called by supervdsm def _set_rpfilter(dev, mode): write to /proc... Line 193: Line 194: iscsiadm.node_login(iface.name, portalStr, target.iqn) Line 195: Line 196: iscsiadm.node_update(iface.name, portalStr, target.iqn,
Line 530: Line 531: def disconnectiScsiSession(sessionID): Line 532: # FIXME : Should throw exception on error Line 533: sessionID = int(sessionID) Line 534: setStrictRpFilterIfUnique(sessionID) This would be more clear:
netIfaceName = getSessionInfo(sessionID).iface.netIfaceName if netIfaceName: sessions = findSessions(netIfaceName) if len(sessions) == 1: set_rpfilter(netIfaceName, _SYSCTL_RPFILTER_STRICT) else: log about keeping rpfilter loose... Line 535: try: Line 536: iscsiadm.session_logout(sessionID) Line 537: except iscsiadm.IscsiError as e: Line 538: return e[0]
Line 539: Line 540: return 0 Line 541: Line 542: Line 543: def setStrictRpFilterIfUnique(sessionID): This name is not clear. Line 544: """ Line 545: Set rp_filter back to strict mode if the device in use by the session is Line 546: not used by any other iSCSI session. Line 547: This is needed to avoid the security breach where an untrusted VM can DoS
Line 542: Line 543: def setStrictRpFilterIfUnique(sessionID): Line 544: """ Line 545: Set rp_filter back to strict mode if the device in use by the session is Line 546: not used by any other iSCSI session. Add empty line between paragraphs. Line 547: This is needed to avoid the security breach where an untrusted VM can DoS Line 548: the host by sending it packages with spoofed random sources. Line 549: Arguments: Line 550: sessionID
Line 544: """ Line 545: Set rp_filter back to strict mode if the device in use by the session is Line 546: not used by any other iSCSI session. Line 547: This is needed to avoid the security breach where an untrusted VM can DoS Line 548: the host by sending it packages with spoofed random sources. packages -> packets Line 549: Arguments: Line 550: sessionID Line 551: """ Line 552:
Line 557: and session.id != sessionID): Line 558: log.debug("Skipping unset rp_filter for session [%s]. " Line 559: "net_ifacename [%s] in use by session [%s]." % Line 560: (sessionID, sessionInfo.iface.netIfaceName, Line 561: session.id)) Lets extract this part to:
def findSessions(netIfaceName): """ Return list of sessions using netIfaceName """ return [session for session in iterateIscsiSessions() if session.iface.netIfaceName == netIfaceName]
Then we can use it when we add or remove sessions, to decide if the rpfilter should be modified. Line 562: return Line 563: Line 564: supervdsm.getProxy().setRpFilter(sessionInfo.iface.netIfaceName,
Dan Kenigsberg has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 17: -Code-Review
(2 comments)
Thanks, Nir for the review. We'd need at least one more round.
http://gerrit.ovirt.org/#/c/31529/17/lib/vdsm/utils.py File lib/vdsm/utils.py:
Line 1136: Line 1137: Line 1138: def set_rp_filter(dev, mode): Line 1139: path = '/proc/sys/net/ipv4/conf/%s/rp_filter' % dev Line 1140: with open(path, 'w') as rp_filter:
We don't need the default buffering in this case, so we should use unbuffer
Why is buffering an issue in this case? (I don't believe that it is)
Please keep using Python constructs unless there's a know efficiency issue.
Creating a sysctl module is a nice idea. utils's junk yard is indeed growing too fast.
http://gerrit.ovirt.org/#/c/31529/17/vdsm/storage/iscsi.py File vdsm/storage/iscsi.py:
Line 186: key = "node.session." + key Line 187: iscsiadm.node_update(iface.name, portalStr, target.iqn, Line 188: key, value, hideValue=True) Line 189: Line 190: if iface.netIfaceName:
Why having a netIfaceName means that we need loose rpfilter?
+1 for loosening only on the second interface. Line 191: supervdsm.getProxy().setRpFilter(iface.netIfaceName, Line 192: _SYSCTL_RPFILTER_LOOSE) Line 193: Line 194: iscsiadm.node_login(iface.name, portalStr, target.iqn)
Nir Soffer has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 17:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/17/lib/vdsm/utils.py File lib/vdsm/utils.py:
Line 1136: Line 1137: Line 1138: def set_rp_filter(dev, mode): Line 1139: path = '/proc/sys/net/ipv4/conf/%s/rp_filter' % dev Line 1140: with open(path, 'w') as rp_filter:
Why is buffering an issue in this case? (I don't believe that it is)
buffering is not an issue, this is just bloat. Check all the code involved in creating a file object and writing, when the needed operations are just 3 syscalls.
I don't suggest to replace all open calls with this, but we can have an utility to write stuff to sysfs/procfs, or just open an fd in a context manager:
@contextmanager def open_fd(path, options): fd = os.open(path, options) try: yield fd finally: os.close(fd)
Then writing efficient code is almost nice as using open:
with open_fd(path, os.O_WRONLY) as fd: os.write(fd, data)
Not required now, just something for later.
Amador Pahim has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 17:
(8 comments)
http://gerrit.ovirt.org/#/c/31529/17/lib/vdsm/utils.py File lib/vdsm/utils.py:
Line 1136: Line 1137: Line 1138: def set_rp_filter(dev, mode): Line 1139: path = '/proc/sys/net/ipv4/conf/%s/rp_filter' % dev Line 1140: with open(path, 'w') as rp_filter:
buffering is not an issue, this is just bloat. Check all the code involved
Ok, we better create sysctl module now and do it right from scratch. Done.
http://gerrit.ovirt.org/#/c/31529/17/vdsm/storage/iscsi.py File vdsm/storage/iscsi.py:
Line 44: DEFAULT_TPGT = 1 Line 45: ISCSI_DEFAULT_PORT = 3260 Line 46: SCAN_PATTERN = "/sys/class/scsi_host/host*/scan" Line 47: _SYSCTL_RPFILTER_STRICT = '0' Line 48: _SYSCTL_RPFILTER_LOOSE = '2'
These constants do not belong to this module
Done Line 49: Line 50: IscsiSession = namedtuple("IscsiSession", "id, iface, target, credentials") Line 51: Line 52: _iscsiadmTransactionLock = RLock()
Line 186: key = "node.session." + key Line 187: iscsiadm.node_update(iface.name, portalStr, target.iqn, Line 188: key, value, hideValue=True) Line 189: Line 190: if iface.netIfaceName:
+1 for loosening only on the second interface.
Ack. Line 191: supervdsm.getProxy().setRpFilter(iface.netIfaceName, Line 192: _SYSCTL_RPFILTER_LOOSE) Line 193: Line 194: iscsiadm.node_login(iface.name, portalStr, target.iqn)
Line 188: key, value, hideValue=True) Line 189: Line 190: if iface.netIfaceName: Line 191: supervdsm.getProxy().setRpFilter(iface.netIfaceName, Line 192: _SYSCTL_RPFILTER_LOOSE)
This is little too wordy, lets add a sysctl module with these functions and
Done Line 193: Line 194: iscsiadm.node_login(iface.name, portalStr, target.iqn) Line 195: Line 196: iscsiadm.node_update(iface.name, portalStr, target.iqn,
Line 539: Line 540: return 0 Line 541: Line 542: Line 543: def setStrictRpFilterIfUnique(sessionID):
This name is not clear.
Done Line 544: """ Line 545: Set rp_filter back to strict mode if the device in use by the session is Line 546: not used by any other iSCSI session. Line 547: This is needed to avoid the security breach where an untrusted VM can DoS
Line 542: Line 543: def setStrictRpFilterIfUnique(sessionID): Line 544: """ Line 545: Set rp_filter back to strict mode if the device in use by the session is Line 546: not used by any other iSCSI session.
Add empty line between paragraphs.
Done Line 547: This is needed to avoid the security breach where an untrusted VM can DoS Line 548: the host by sending it packages with spoofed random sources. Line 549: Arguments: Line 550: sessionID
Line 544: """ Line 545: Set rp_filter back to strict mode if the device in use by the session is Line 546: not used by any other iSCSI session. Line 547: This is needed to avoid the security breach where an untrusted VM can DoS Line 548: the host by sending it packages with spoofed random sources.
packages -> packets
Done Line 549: Arguments: Line 550: sessionID Line 551: """ Line 552:
Line 557: and session.id != sessionID): Line 558: log.debug("Skipping unset rp_filter for session [%s]. " Line 559: "net_ifacename [%s] in use by session [%s]." % Line 560: (sessionID, sessionInfo.iface.netIfaceName, Line 561: session.id))
Lets extract this part to:
Done Line 562: return Line 563: Line 564: supervdsm.getProxy().setRpFilter(sessionInfo.iface.netIfaceName,
Amador Pahim has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 17: -Verified
(1 comment)
setting rp_filter=2 from second session on will not work in some cases.
http://gerrit.ovirt.org/#/c/31529/17/vdsm/storage/iscsi.py File vdsm/storage/iscsi.py:
Line 186: key = "node.session." + key Line 187: iscsiadm.node_update(iface.name, portalStr, target.iqn, Line 188: key, value, hideValue=True) Line 189: Line 190: if iface.netIfaceName:
+1 for loosening only on the second interface.
I coded it (loosening only on the second interface) but had problems in my tests. If the first connection to iscsi happens through a device not expected by the OS as the device for that connection, the rp_filter=2 is needed for the first connection. I will improve this code, but I have to keep the rp_filter=2 for the first connection as well. Line 191: supervdsm.getProxy().setRpFilter(iface.netIfaceName, Line 192: _SYSCTL_RPFILTER_LOOSE) Line 193: Line 194: iscsiadm.node_login(iface.name, portalStr, target.iqn)
Amador Pahim has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 17:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/17/vdsm/storage/iscsi.py File vdsm/storage/iscsi.py:
Line 186: key = "node.session." + key Line 187: iscsiadm.node_update(iface.name, portalStr, target.iqn, Line 188: key, value, hideValue=True) Line 189: Line 190: if iface.netIfaceName:
I coded it (loosening only on the second interface) but had problems in my
Or, better, I will probe the os device using netinfo.getRouteDeviceTo() and set rp_filter=2 when os_device != netIfaceName OR len(sessions) >= 1 Line 191: supervdsm.getProxy().setRpFilter(iface.netIfaceName, Line 192: _SYSCTL_RPFILTER_LOOSE) Line 193: Line 194: iscsiadm.node_login(iface.name, portalStr, target.iqn)
Nir Soffer has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 17:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/17/vdsm/storage/iscsi.py File vdsm/storage/iscsi.py:
Line 186: key = "node.session." + key Line 187: iscsiadm.node_update(iface.name, portalStr, target.iqn, Line 188: key, value, hideValue=True) Line 189: Line 190: if iface.netIfaceName:
Or, better, I will probe the os device using netinfo.getRouteDeviceTo() and
Can you describe in more details why changing the rpfilter does not work when adding the second session? How can we reproduce this issue? Line 191: supervdsm.getProxy().setRpFilter(iface.netIfaceName, Line 192: _SYSCTL_RPFILTER_LOOSE) Line 193: Line 194: iscsiadm.node_login(iface.name, portalStr, target.iqn)
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 18:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13385/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/565/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/547... : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13224/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12434/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el7_created/5/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc21_created/1/ : SUCCESS
Amador Pahim has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 18:
(3 comments)
http://gerrit.ovirt.org/#/c/31529/18/lib/vdsm/Makefile.am File lib/vdsm/Makefile.am:
Line 33: profile.py \ Line 34: qemuimg.py \ Line 35: sslutils.py \ Line 36: utils.py \ Line 37: sysctl.py \ out of sort. fixing. Line 38: vdscli.py \ Line 39: virtsparsify.py \ Line 40: xmlrpc.py \ Line 41: $(NULL)
http://gerrit.ovirt.org/#/c/31529/18/lib/vdsm/sysctl.py File lib/vdsm/sysctl.py:
Line 18: # Refer to the README and COPYING files for full details of the license Line 19: # Line 20: Line 21: Line 22: _RPFILTER_STRICT = '0' Strict mode is "1". "0" means "No source validation". Fixing. Line 23: _RPFILTER_LOOSE = '2' Line 24: Line 25: Line 26: def set_rp_filter(dev, mode):
http://gerrit.ovirt.org/#/c/31529/18/vdsm.spec.in File vdsm.spec.in:
Line 1230: %{python_sitelib}/%{vdsm_name}/qemuimg.py* Line 1231: %{python_sitelib}/%{vdsm_name}/netconfpersistence.py* Line 1232: %{python_sitelib}/%{vdsm_name}/sslutils.py* Line 1233: %{python_sitelib}/%{vdsm_name}/utils.py* Line 1234: %{python_sitelib}/%{vdsm_name}/sysctl.py* Out of sort. Fixing. Line 1235: %{python_sitelib}/%{vdsm_name}/vdscli.py* Line 1236: %{python_sitelib}/%{vdsm_name}/virtsparsify.py* Line 1237: %{python_sitelib}/%{vdsm_name}/xmlrpc.py* Line 1238: %{python_sitelib}/%{vdsm_name}/tool/__init__.py*
Nir Soffer has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 18:
(7 comments)
http://gerrit.ovirt.org/#/c/31529/18/vdsm/storage/iscsi.py File vdsm/storage/iscsi.py:
Line 185: key = "node.session." + key Line 186: iscsiadm.node_update(iface.name, portalStr, target.iqn, Line 187: key, value, hideValue=True) Line 188: Line 189: setDevLooseRpFilter(iface.netIfaceName, target.portal.hostname) Lets simplify and send the session to this function:
setDevLooseRpFilter(iface) Line 190: Line 191: iscsiadm.node_login(iface.name, portalStr, target.iqn) Line 192: Line 193: iscsiadm.node_update(iface.name, portalStr, target.iqn,
Line 535: try: Line 536: iscsiadm.session_logout(sessionID) Line 537: except iscsiadm.IscsiError as e: Line 538: return e[0] Line 539: and simply by sending sessionInfo - the function will get what it needs from the session. Line 540: setDevStrictRpFilter(netIfaceName, hostname) Line 541: Line 542: return 0 Line 543:
Line 548: return [session for session in iterateIscsiSessions() Line 549: if session.iface.netIfaceName == netIfaceName] Line 550: Line 551: Line 552: def setDevLooseRpFilter(netIfaceName, hostname): This is name is misleading - this will set loose rp filter only if needed.
You can name it loosenRpFilterIfNeeded, or maybeLossenRpFilter - but both are little too long.
I think a better approach would be to separate the function that tell if a device needs a loose rp filter and the functions that change the rp filter.
if needsLooseRpFilter(iface): loosenRpFilter(iface)
And same for disconnect - can we use the same logic here?
if not needsLoosenRpFilter(iface): tightenRpFilter(iface) Line 553: """ Line 554: Set rp_filter to loose mode if the device in use by the iSCSI session is Line 555: not the device used by OS to reach the target IP. Line 556:
Line 557: This is needed to allow multiple iSCSI connections in a multiple NIC per Line 558: subnet configuration. Line 559: Line 560: Arguments: Line 561: netIfaceName hostname missing Line 562: """ Line 563: Line 564: if netIfaceName != getRouteDeviceTo(hostname): Line 565: supervdsm.getProxy().set_rp_filter_loose(netIfaceName)
Line 563: Line 564: if netIfaceName != getRouteDeviceTo(hostname): Line 565: supervdsm.getProxy().set_rp_filter_loose(netIfaceName) Line 566: log.debug("Setting loose mode rp_filter for device [%s]." % Line 567: netIfaceName) Should be log.info - we made a significant change in the machine configuration. Line 568: Line 569: Line 570: def setDevStrictRpFilter(netIfaceName, hostname): Line 571: """
Line 576: This is needed to avoid the security breach where an untrusted VM can DoS Line 577: the host by sending it packets with spoofed random sources. Line 578: Line 579: Arguments: Line 580: sessionID Please update to reflect reality Line 581: """ Line 582: Line 583: sessions = findSessions(netIfaceName) Line 584:
Line 584: Line 585: if netIfaceName != getRouteDeviceTo(hostname) and len(sessions) == 0: Line 586: supervdsm.getProxy().set_rp_filter_strict(netIfaceName) Line 587: log.debug("Setting strict mode rp_filter for device [%s]." % Line 588: netIfaceName) Should be log.info - we made a significant change in the machine configuration.
Amador Pahim has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 18:
(7 comments)
http://gerrit.ovirt.org/#/c/31529/18/vdsm/storage/iscsi.py File vdsm/storage/iscsi.py:
Line 185: key = "node.session." + key Line 186: iscsiadm.node_update(iface.name, portalStr, target.iqn, Line 187: key, value, hideValue=True) Line 188: Line 189: setDevLooseRpFilter(iface.netIfaceName, target.portal.hostname)
Lets simplify and send the session to this function:
I don't have a session here, before creating the session. We can simplify with:
setDevLooseRpFilter(iface, target)
But that's not what the to next two lines are doing.
Done. Line 190: Line 191: iscsiadm.node_login(iface.name, portalStr, target.iqn) Line 192: Line 193: iscsiadm.node_update(iface.name, portalStr, target.iqn,
Line 535: try: Line 536: iscsiadm.session_logout(sessionID) Line 537: except iscsiadm.IscsiError as e: Line 538: return e[0] Line 539:
and simply by sending sessionInfo - the function will get what it needs fro
Done Line 540: setDevStrictRpFilter(netIfaceName, hostname) Line 541: Line 542: return 0 Line 543:
Line 548: return [session for session in iterateIscsiSessions() Line 549: if session.iface.netIfaceName == netIfaceName] Line 550: Line 551: Line 552: def setDevLooseRpFilter(netIfaceName, hostname):
This is name is misleading - this will set loose rp filter only if needed.
Done Line 553: """ Line 554: Set rp_filter to loose mode if the device in use by the iSCSI session is Line 555: not the device used by OS to reach the target IP. Line 556:
Line 557: This is needed to allow multiple iSCSI connections in a multiple NIC per Line 558: subnet configuration. Line 559: Line 560: Arguments: Line 561: netIfaceName
hostname missing
Done Line 562: """ Line 563: Line 564: if netIfaceName != getRouteDeviceTo(hostname): Line 565: supervdsm.getProxy().set_rp_filter_loose(netIfaceName)
Line 563: Line 564: if netIfaceName != getRouteDeviceTo(hostname): Line 565: supervdsm.getProxy().set_rp_filter_loose(netIfaceName) Line 566: log.debug("Setting loose mode rp_filter for device [%s]." % Line 567: netIfaceName)
Should be log.info - we made a significant change in the machine configurat
Done Line 568: Line 569: Line 570: def setDevStrictRpFilter(netIfaceName, hostname): Line 571: """
Line 576: This is needed to avoid the security breach where an untrusted VM can DoS Line 577: the host by sending it packets with spoofed random sources. Line 578: Line 579: Arguments: Line 580: sessionID
Please update to reflect reality
Done Line 581: """ Line 582: Line 583: sessions = findSessions(netIfaceName) Line 584:
Line 584: Line 585: if netIfaceName != getRouteDeviceTo(hostname) and len(sessions) == 0: Line 586: supervdsm.getProxy().set_rp_filter_strict(netIfaceName) Line 587: log.debug("Setting strict mode rp_filter for device [%s]." % Line 588: netIfaceName)
Should be log.info - we made a significant change in the machine configurat
Done
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 19:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13386/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/566/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/548... : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13225/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12435/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el7_created/6/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc21_created/2/ : SUCCESS
Nir Soffer has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 18:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/18/vdsm/storage/iscsi.py File vdsm/storage/iscsi.py:
Line 185: key = "node.session." + key Line 186: iscsiadm.node_update(iface.name, portalStr, target.iqn, Line 187: key, value, hideValue=True) Line 188: Line 189: setDevLooseRpFilter(iface.netIfaceName, target.portal.hostname)
I don't have a session here, before creating the session. We can simplify w
Right, my error.
The next lines are iscsiadm interface, which is a low level module wrapping iscsiadm.
Here we are in the scsi module, and we can select the type of arguments that makes the code easier to work with. Line 190: Line 191: iscsiadm.node_login(iface.name, portalStr, target.iqn) Line 192: Line 193: iscsiadm.node_update(iface.name, portalStr, target.iqn,
Amador Pahim has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 19: Verified+1
Setting rp_filter to loose mode only for devices distinct from the one picked by the OS for that network worked for every test I ran.
Dan Kenigsberg has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 19: Code-Review-1
(1 comment)
http://gerrit.ovirt.org/#/c/31529/19/vdsm/storage/hsm.py File vdsm/storage/hsm.py:
Line 150: def _resolveIscsiIface(ifaceName, initiatorName, netIfaceName): Line 151: if not ifaceName: Line 152: return iscsi.IscsiInterface('default') Line 153: Line 154: for iface in iscsi.iterateIscsiInterfaces(): if iface.name = ifaceName already exists, but lacks the requested netIfaceName, we have a problem that we should not ignore, not blindly overwrite.
I *think* that the solution would be to parse
iscsiadm -m iface list
better, and extract the netIfaceName from it. At the moment iscsiadm.iface_list() ignores all but the iscsi interface name. Instead, it should return a named tuple of all extracted info
# iscsiadm -m iface list default tcp,<empty>,<empty>,<empty>,<empty> iser iser,<empty>,<empty>,<empty>,<empty> eth1 tcp,<empty>,<empty>,SAN1,<empty> eth2 tcp,<empty>,<empty>,SAN2,<empty>
Please add this parsing in a separate patch, that includes its own unit test. Line 155: if iface.name == ifaceName: Line 156: iface.netIfaceName = netIfaceName Line 157: return iface Line 158:
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 20:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc21_created/157... : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/722/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/14027/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/704... : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el7_created/162/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/13238/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/14195/ : FAILURE
Amador Pahim has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 20: Verified+1
Dan Kenigsberg has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 20: Code-Review+1
Federico Simoncelli has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 20:
(5 comments)
Mostly ok.
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 in removeIscsiNode? 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? 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? 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 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 589: netIfaceName) Shouldn't we log the case where netIfaceName is None? Is it expected to be a common valid case that is not worth mention in the logs?
Also logging (debug) that we're skipping to set the rp_filter_loose is interesting (else part of the if block). Line 590: Line 591: Line 592: def restoreStrictRpFilterIfPossible(sessionInfo): Line 593: """
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]." % Same comments above applies here as well.
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
Amador Pahim has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 20:
(9 comments)
http://gerrit.ovirt.org/#/c/31529/20/vdsm/storage/iscsi.py File vdsm/storage/iscsi.py:
Line 539: Line 540: return 0 Line 541: Line 542: Line 543: def findSessions(netIfaceName):
Shouldn't this be private at this time?
Done 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?
Done 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 restoreStrictRpFi
Done 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:
Done 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 netIfaceN
Done 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
Done 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 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?
Done 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
Done 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 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 c
Agreed. Keeping as is for another round. Line 607: supervdsm.getProxy().set_rp_filter_strict(netIfaceName) Line 608: log.info("Setting strict mode rp_filter for device [%s]." %
automation@ovirt.org has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 21:
* Update tracker::#1112861::OK * Check Bug-Url::OK * Check Public Bug::#1112861::OK, public bug * Check Product::#1112861::OK, Correct product Red Hat Enterprise Virtualization Manager * Check TR::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 22:
* Update tracker::#1112861::OK * Check Bug-Url::OK * Check Public Bug::#1112861::OK, public bug * Check Product::#1112861::OK, Correct product Red Hat Enterprise Virtualization Manager * Check TR::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 23:
* Update tracker::#1112861::OK * Check Bug-Url::OK * Check Public Bug::#1112861::OK, public bug * Check Product::#1112861::OK, Correct product Red Hat Enterprise Virtualization Manager * Check TR::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Dan Kenigsberg has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 23:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/23/lib/vdsm/sysctl.py File lib/vdsm/sysctl.py:
Line 1: # Line 2: # Copyright 2014 Red Hat, Inc. since the review of this patch is dragging on and on, would you be kind to split the creation of this sysctl module to its own patch?
I'd love to use it to disable ipv6 instead what's currently done in http://gerrit.ovirt.org/#/c/35061/11/tests/functional/networkTests.py Line 3: # Line 4: # This program is free software; you can redistribute it and/or modify Line 5: # it under the terms of the GNU General Public License as published by Line 6: # the Free Software Foundation; either version 2 of the License, or
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 21:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc21_created/245... : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el7_created/250/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/13774/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/14731/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/810/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/14563/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/793... : FAILURE
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 22:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc21_created/248... : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el7_created/253/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/13783/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/14740/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/813/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/14572/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/796... : SUCCESS
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 23:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc21_created/249... : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el7_created/254/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/13784/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/14741/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/814/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/14573/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/797... : SUCCESS
Amador Pahim has posted comments on this change.
Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 23:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/23/lib/vdsm/sysctl.py File lib/vdsm/sysctl.py:
Line 1: # Line 2: # Copyright 2014 Red Hat, Inc.
since the review of this patch is dragging on and on, would you be kind to
Done Line 3: # Line 4: # This program is free software; you can redistribute it and/or modify Line 5: # it under the terms of the GNU General Public License as published by Line 6: # the Free Software Foundation; either version 2 of the License, or
automation@ovirt.org has posted comments on this change.
Change subject: Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 24:
* Update tracker::#1112861::OK * Check Bug-Url::OK * Check Public Bug::#1112861::OK, public bug * Check Product::#1112861::OK, Correct product Red Hat Enterprise Virtualization Manager * Check TR::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 24:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/14760/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/14592/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/13804/ : FAILURE
Amador Pahim has posted comments on this change.
Change subject: Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 24: Verified+1
Ping
engine is ready for this: http://gerrit.ovirt.org/#/c/32383/
Yaniv Bronhaim has posted comments on this change.
Change subject: Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 24: Code-Review-1
this is not the way vdsm configures other services conf files. we use vdsm-tool configure verb for that. although afaiu from the patch you plan to modify the configuration on runtime. I still prefer you do it with the configure.py interface with revert option and validation. btw, can that config depend on vdsm.conf values?
Amador Pahim has posted comments on this change.
Change subject: Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 24:
I started this change using vdsm-sysctl.conf and during reviews we agreed about the importance in set rp_filter to loose mode on runtime and also to set back to strict mode also on runtime.
automation@ovirt.org has posted comments on this change.
Change subject: Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 25:
* Update tracker::#1112861::OK * Check Bug-Url::OK * Check Public Bug::#1112861::OK, public bug * Check Product::#1112861::OK, Correct product Red Hat Enterprise Virtualization Manager * Check TR::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Yaniv Bronhaim has posted comments on this change.
Change subject: Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 25: Code-Review+1
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 25:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/14795/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/14627/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/13839/ : FAILURE
Federico Simoncelli has posted comments on this change.
Change subject: Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 25:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/25/vdsm/storage/iscsi.py File vdsm/storage/iscsi.py:
Line 613: if netIfaceName is None: Line 614: log.info("iSCSI iface.net_ifacename not provided. Skipping.") Line 615: return Line 616: Line 617: if _needsLooseRpFilter(netIfaceName, hostname): Shouldn't this be:
if not _needsLooseRpFilter(netIfaceName, hostname): ...
Do we need two methods? Can't we just have one? (should be simpler)
def updateRpFilterIfNeeded(netIfaceName, hostname): ... if _needsLooseRpFilter(netIfaceName, hostname): supervdsm.getProxy().set_rp_filter_loose(netIfaceName) log.info("Setting loose mode rp_filter for device %r." % netIfaceName) else: supervdsm.getProxy().set_rp_filter_strict(netIfaceName) log.info("Setting strict mode rp_filter for device %r." % netIfaceName)
Technically if in addIscsiNode we move updateRpFilterIfNeeded after the node_login it should work, right? (Not sure about this one, it requires some research). Line 618: supervdsm.getProxy().set_rp_filter_strict(netIfaceName) Line 619: log.info("Setting strict mode rp_filter for device %r." % Line 620: netIfaceName) Line 621: else:
Amador Pahim has posted comments on this change.
Change subject: Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 25:
(1 comment)
http://gerrit.ovirt.org/#/c/31529/25/vdsm/storage/iscsi.py File vdsm/storage/iscsi.py:
Line 613: if netIfaceName is None: Line 614: log.info("iSCSI iface.net_ifacename not provided. Skipping.") Line 615: return Line 616: Line 617: if _needsLooseRpFilter(netIfaceName, hostname):
Shouldn't this be:
Shouldn't this be:
if not _needsLooseRpFilter(netIfaceName, hostname): ...
No. _needsLooseRpFilter is not a good name here, but we will set rp_filter back to strict if this function returns true, which means no session is using the device and the device is different from the device picked by OS to reach the target IP.
Do we need two methods? Can't we just have one? (should be simpler)
They are not if/else conditions. We have to set rp_filter to loose when it's the first session using a device different from the device picked by the OS to reach the target IP. For the second session using the same device, we assume rp_filter is already in loose mode. The same for any additional session using the same device. When no sessions are using the device anymore, so we can set rp_filter back to strict.
Technically if in addIscsiNode we move updateRpFilterIfNeeded after the node_login it should work, right? (Not sure about this one, it requires some research).
No, it should not. vdsm will wait for node_login, which will not complete successfully due to the rp_filter in strict mode. So we have to set it to loose mode before the iscsi connection. Line 618: supervdsm.getProxy().set_rp_filter_strict(netIfaceName) Line 619: log.info("Setting strict mode rp_filter for device %r." % Line 620: netIfaceName) Line 621: else:
Dan Kenigsberg has posted comments on this change.
Change subject: Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 25:
(2 comments)
http://gerrit.ovirt.org/#/c/31529/25/vdsm/storage/iscsi.py File vdsm/storage/iscsi.py:
Line 542: Line 543: return 0 Line 544: Line 545: Line 546: def _findSessions(netIfaceName): a nicer name could be
def _sessionsUsingNetiface(netIfaceName): for session in iterateIscsiSessions(): if session.iface.netIfaceName == netIfaceName: yield session
and then len(sessions) == 0 can be replaced with
not any(sessions) Line 547: """ Return list of sessions using netIfaceName """ Line 548: Line 549: return [session for session in iterateIscsiSessions() Line 550: if session.iface.netIfaceName == netIfaceName]
Line 569: Line 570: return len(sessions) == 0 and netIfaceName != getRouteDeviceTo(hostname) Line 571: Line 572: Line 573: def setLooseRpFilterIfNeeded(netIfaceName, hostname): If we define
setRpFilterIfNeeded(netIfaceName, hostname, looseOrStrcit):
we can merge these two functions into one, and also inline _needsLooseRpFilter into it. Line 574: """ Line 575: Set rp_filter to loose mode if it's the first session and the netIfaceName Line 576: device is not the device used by the OS to reach the 'hostname'. Line 577:
automation@ovirt.org has posted comments on this change.
Change subject: Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 26:
* Update tracker::#1112861::OK * Check Bug-Url::OK * Check Public Bug::#1112861::OK, public bug * Check Product::#1112861::OK, Correct product Red Hat Enterprise Virtualization Manager * Check TR::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 26:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/14929/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/14760/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/13972/ : FAILURE
Federico Simoncelli has posted comments on this change.
Change subject: Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 26: Code-Review+1
(2 comments)
Minor nits but ready to be merged.
http://gerrit.ovirt.org/#/c/31529/26/vdsm/storage/iscsi.py File vdsm/storage/iscsi.py:
Line 558: Line 559: if not any(sessions) and netIfaceName != getRouteDeviceTo(hostname): Line 560: if loose_mode: Line 561: supervdsm.getProxy().set_rp_filter_loose(netIfaceName) Line 562: log.info("Setting loose mode rp_filter for device %r." % Nit: "Setting" announces the execution (should preceed set_rp_filter_loose).
If you want to leave it after then it should be "Lose mode rp_filter set for device..." Line 563: netIfaceName) Line 564: else: Line 565: supervdsm.getProxy().set_rp_filter_strict(netIfaceName) Line 566: log.info("Setting strict mode rp_filter for device %r." %
Line 562: log.info("Setting loose mode rp_filter for device %r." % Line 563: netIfaceName) Line 564: else: Line 565: supervdsm.getProxy().set_rp_filter_strict(netIfaceName) Line 566: log.info("Setting strict mode rp_filter for device %r." % Same here.
automation@ovirt.org has posted comments on this change.
Change subject: Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 27:
* Update tracker::#1112861::OK * Check Bug-Url::OK * Check Public Bug::#1112861::OK, public bug * Check Product::#1112861::OK, Correct product Red Hat Enterprise Virtualization Manager * Check TR::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 27:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/14930/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/14761/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/13973/ : FAILURE
Federico Simoncelli has posted comments on this change.
Change subject: Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 27: Code-Review+2
Amador Pahim has posted comments on this change.
Change subject: Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 27: Verified+1
Yaniv Bronhaim has posted comments on this change.
Change subject: Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 27: Code-Review+1
Dan Kenigsberg has submitted this change and it was merged.
Change subject: Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet
When two or more interfaces are in the same subnet, the default OS behaviour won't allow the independent traffic between local NICs and iSCSI targets.
These sysctl tweaks are needed to have iSCSI multipath working in such topology:
eth1 (192.168.25.200) --| |-- iSCSI Target (192.168.25.10) eth2 (192.168.25.201) --|
For the ARP flux problem, where the machine may respond to ARP requests from both Ethernet interfaces, we are setting "arp_ignore=1" and "arp_announce=2"
To allow multiple iSCSI connections in a multiple NIC per subnet configuration, reverse path filtering must be loosened with "rp_filter=2" for the interfaces.
Change-Id: Ibf93d49317c76aece764e53e58e0ff28868f16b0 Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1112861 Signed-off-by: Amador Pahim apahim@redhat.com Reviewed-on: http://gerrit.ovirt.org/31529 Reviewed-by: Federico Simoncelli fsimonce@redhat.com Reviewed-by: Yaniv Bronhaim ybronhei@redhat.com --- M vdsm/storage/iscsi.py M vdsm/vdsm-sysctl.conf 2 files changed, 60 insertions(+), 1 deletion(-)
Approvals: Yaniv Bronhaim: Looks good to me, but someone else must approve Federico Simoncelli: Looks good to me, approved Amador Pahim: Verified
automation@ovirt.org has posted comments on this change.
Change subject: Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 28:
* Update tracker::#1112861::OK * Set MODIFIED::bug 1112861::::#1112861::::IGNORE, not oVirt prod but Red Hat Enterprise Virtualization Manager
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Set sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 28:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/6291/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_create-rpms-el7-x86_64_merged/470/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_create-rpms-el6-x86_64_merged/469/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_create-rpms-fc20-x86_64_merged/462/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_create-rpms-fc21-x86_64_merged/445/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master-libgfapi_create-rpms-el7-x86_64_mer... : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master-libgfapi_create-rpms-el6-x86_64_mer... : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master-libgfapi_create-rpms-fc20-x86_64_me... : FAILURE
http://jenkins.ovirt.org/job/vdsm_master-libgfapi_create-rpms-fc21-x86_64_me... : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/4456/ : FAILURE
vdsm-patches@lists.fedorahosted.org