Change in vdsm[master]: sysctl: turn disable_ipv6 into toggle_ipv6 to simplify logic

Ondřej Svoboda has uploaded a new change for review.

Change subject: sysctl: turn disable_ipv6 into toggle_ipv6 to simplify logic
......................................................................

sysctl: turn disable_ipv6 into toggle_ipv6 to simplify logic

Change-Id: I154f22a5f59174d0e33235d252aa42e463760e04
Signed-off-by: Ondřej Svoboda 
---
M lib/vdsm/sysctl.py
M tests/functional/networkTests.py
M vdsm/network/configurators/ifcfg.py
M vdsm/network/configurators/iproute2.py
M vdsm_hooks/ovs/ovs_before_network_setup_ip.py
5 files changed, 8 insertions(+), 11 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/83/46983/1

diff --git a/lib/vdsm/sysctl.py b/lib/vdsm/sysctl.py
index 957e61f..603c67a 100644
--- a/lib/vdsm/sysctl.py
+++ b/lib/vdsm/sysctl.py
@@ -38,9 +38,9 @@
     set_rp_filter(dev, _RPFILTER_STRICT)

-def disable_ipv6(dev, disable=True):
+def toggle_ipv6(dev, enable):
     with open('/proc/sys/net/ipv6/conf/%s/disable_ipv6' % dev, 'w') as f:
-        f.write('1' if disable else '0')
+        f.write('0' if enable else '1')

def is_disabled_ipv6():
diff --git a/tests/functional/networkTests.py b/tests/functional/networkTests.py
index 674c479..b378079 100644
--- a/tests/functional/networkTests.py
+++ b/tests/functional/networkTests.py
@@ -432,8 +432,8 @@

with veth_pair() as (left, right):
             # disabling IPv6 on Interface for removal of Router Solicitation
-            sysctl.disable_ipv6(left)
-            sysctl.disable_ipv6(right)
+            sysctl.toggle_ipv6(left, False)
+            sysctl.toggle_ipv6(right, False)
             linkSet(left, ['up'])
             linkSet(right, ['up'])

@@ -797,7 +797,7 @@
     @cleanupNet
     @permutations([[True], [False]])
     def testDelNetworkWithMTU(self, bridged):
-        MTU = '1280'  # required for sysctl.disable_ipv6() on the bridge
+        MTU = '1280'  # required for sysctl.toggle_ipv6() on the bridge
         with dummyIf(2) as nics:
             status, msg = self.vdsm_net.addNetwork(NETWORK_NAME, vlan=VLAN_ID,
                                                    bond=BONDING_NAME,
diff --git a/vdsm/network/configurators/ifcfg.py b/vdsm/network/configurators/ifcfg.py
index c2b0345..7530105 100644
--- a/vdsm/network/configurators/ifcfg.py
+++ b/vdsm/network/configurators/ifcfg.py
@@ -103,7 +103,7 @@
         if not bridge.ipv6.address and not bridge.ipv6.ipv6autoconf and (
                 not bridge.ipv6.dhcpv6):
             wait_for_device(bridge.name)
-            sysctl.disable_ipv6(bridge.name)
+            sysctl.toggle_ipv6(bridge.name, False)

def configureVlan(self, vlan, **opts):
         self.configApplier.addVlan(vlan, **opts)
diff --git a/vdsm/network/configurators/iproute2.py b/vdsm/network/configurators/iproute2.py
index d20f77a..7008fad 100644
--- a/vdsm/network/configurators/iproute2.py
+++ b/vdsm/network/configurators/iproute2.py
@@ -79,7 +79,7 @@
         if not bridge.ipv6.address and not bridge.ipv6.ipv6autoconf and (
                 not bridge.ipv6.dhcpv6):
             wait_for_device(bridge.name)
-            sysctl.disable_ipv6(bridge.name)
+            sysctl.toggle_ipv6(bridge.name, False)
         self._addSourceRoute(bridge)
         if 'custom' in opts and 'bridge_opts' in opts['custom']:
             self.configApplier._setBridgeOpts(bridge,
diff --git a/vdsm_hooks/ovs/ovs_before_network_setup_ip.py b/vdsm_hooks/ovs/ovs_before_network_setup_ip.py
index 90640a4..9609051 100644
--- a/vdsm_hooks/ovs/ovs_before_network_setup_ip.py
+++ b/vdsm_hooks/ovs/ovs_before_network_setup_ip.py
@@ -70,10 +70,7 @@
         ipwrapper.addrAdd(iface, ipv4.address, ipv4.netmask)
     if ipv4.gateway and ipv4.defaultRoute:
         ipwrapper.routeAdd(['default', 'via', ipv4.gateway])
-    if ipv6.address or ipv6.ipv6autoconf or ipv6.dhcpv6:
-        sysctl.disable_ipv6(iface, disable=False)
-    else:
-        sysctl.disable_ipv6(iface)
+    sysctl.toggle_ipv6(iface, ipv6.address or ipv6.ipv6autoconf or ipv6.dhcpv6)
     if ipv6.address:
         ipv6addr, ipv6netmask = ipv6.address.split('/')
         ipwrapper.addrAdd(iface, ipv6addr, ipv6netmask, family=6)

[e-mail] has posted comments on this change.

Change subject: sysctl: turn disable_ipv6 into toggle_ipv6 to simplify logic
......................................................................

Patch Set 1:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Petr Horáček has posted comments on this change.

Change subject: sysctl: turn disable_ipv6 into toggle_ipv6 to simplify logic
......................................................................

Patch Set 1:

i'd prefer to rename it to set_ipv6 or better create two functions enable and disable_ipv6. what do you think?

Ondřej Svoboda has posted comments on this change.

Change subject: sysctl: turn disable_ipv6 into toggle_ipv6 to simplify logic
......................................................................

Patch Set 1:

I see no reason for two functions turning the same switch on or off.

"set_ipv6" is similar to the rest of functions in sysctl, but "disable_ipv6" is only a boolean – so I chose the name "toggle", which indicates there are only two states.

[e-mail] has posted comments on this change.

Change subject: sysctl: turn disable_ipv6 into toggle_ipv6 to simplify logic
......................................................................

Patch Set 2:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Ido Barkan has posted comments on this change.

Change subject: sysctl: turn disable_ipv6 into toggle_ipv6 to simplify logic
......................................................................

Patch Set 2: Code-Review+1

Ondřej Svoboda has posted comments on this change.

Change subject: sysctl: turn disable_ipv6 into toggle_ipv6 to simplify logic
......................................................................

Patch Set 2: Verified+1

Tried creating a network with no IPv6 functionality and the representing bridge had disable_ipv6 turned on (and as a result, there was not even an IPv6 link-local address).

Two tests failed, 3 were skipped.

======================================================================
FAIL: testSetupNetworksEmergencyDevicesCleanupVlanOverwrite(False) (functional.networkTests.NetworkTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/share/vdsm/tests/testlib.py", line 67, in wrapper
    return f(self, *args)
  File "/usr/share/vdsm/tests/testValidation.py", line 170, in wrapper
    return f(*args, **kwargs)
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 221, in wrapper
    func(*args, **kwargs)
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 2462, in testSetupNetworksEmergencyDevicesCleanupVlanOverwrite
    self.assertBondDoesntExist(BONDING_NAME)
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 299, in assertBondDoesntExist
    '%s found unexpectedly' % bondName)
AssertionError: bond11 found unexpectedly
-------------------- >> begin captured logging << --------------------
root: INFO: Adding network test-network({'vlan': '27', 'nic': 'dummy_7rtUY', 'dhcpv6': False, 'mtu': '1500', 'bootproto': 'none', 'bridged': False, 'defaultRoute': False})
root: INFO: Adding bond11({'nics': ['dummy_GZ5qM', 'dummy_GhojC'], 'options': ''})
root: INFO: Adding network test-network({'vlan': '27', 'nic': 'dummy_7rtUY', 'dhcpv6': False, 'mtu': '1500', 'bootproto': 'none', 'bridged': False, 'defaultRoute': False})
root: INFO: Adding bond11({'nics': ['dummy_GZ5qM', 'dummy_GhojC'], 'options': ''})
--------------------- >> end captured logging << ---------------------

======================================================================
FAIL: test_keep_initial_bond_slaves_ip_config (functional.networkTests.NetworkTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 221, in wrapper
    func(*args, **kwargs)
  File "/usr/share/vdsm/tests/testValidation.py", line 105, in wrapper
    return f(*args, **kwargs)
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 2823, in test_keep_initial_bond_slaves_ip_config
    self.assertIn(IP_ADDRESS_AND_CIDR, ipv4addrs)
AssertionError: '192.0.2.1/24' not found in []
-------------------- >> begin captured logging << --------------------
root: DEBUG: /sbin/ip -4 addr add dev dummy_GZ5qM 192.0.2.1/24 (cwd None)
root: DEBUG: SUCCESS: <err> = ''; <rc> = 0
root: DEBUG: /sbin/ip -6 addr add dev dummy_GZ5qM fdb3:84e5:4ff4:55e3::1/64 (cwd None)
root: DEBUG: SUCCESS: <err> = ''; <rc> = 0
root: INFO: Adding bond11({'nics': ['dummy_GZ5qM', 'dummy_GhojC'], 'options': ''})
root: DEBUG: /sbin/ip addr flush dev dummy_GZ5qM scope global (cwd None)
root: DEBUG: SUCCESS: <err> = ''; <rc> = 0
--------------------- >> end captured logging << ---------------------

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: sysctl: turn disable_ipv6 into toggle_ipv6 to simplify logic
......................................................................

Patch Set 2: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/46983/2/lib/vdsm/sysctl.py
File lib/vdsm/sysctl.py:

Line 37: def set_rp_filter_strict(dev):
Line 38:     set_rp_filter(dev, _RPFILTER_STRICT)
Line 39: 
Line 40: 
Line 41: def toggle_ipv6(dev, enable):
The term Toggle usually means "flip current state", which we don't want.

How about Enable-ipv6? Set-ipv6?
Line 42:     with open('/proc/sys/net/ipv6/conf/%s/disable_ipv6' % dev, 'w') as f:
Line 43:         f.write('0' if enable else '1')
Line 44: 
Line 45:

Ondřej Svoboda has posted comments on this change.

Change subject: sysctl: turn disable_ipv6 into toggle_ipv6 to simplify logic
......................................................................

Patch Set 2:

(1 comment)

https://gerrit.ovirt.org/#/c/46983/2/lib/vdsm/sysctl.py
File lib/vdsm/sysctl.py:

Line 37: def set_rp_filter_strict(dev):
Line 38:     set_rp_filter(dev, _RPFILTER_STRICT)
Line 39: 
Line 40: 
Line 41: def toggle_ipv6(dev, enable):
> The term Toggle usually means "flip current state", which we don't want.

That's true. I'll go with the more neutral "set".
Line 42:     with open('/proc/sys/net/ipv6/conf/%s/disable_ipv6' % dev, 'w') as f:
Line 43:         f.write('0' if enable else '1')
Line 44: 
Line 45:

Ondřej Svoboda has posted comments on this change.

Change subject: sysctl: turn disable_ipv6 into toggle_ipv6 to simplify logic
......................................................................

Patch Set 2:

(1 comment)

https://gerrit.ovirt.org/#/c/46983/2/lib/vdsm/sysctl.py
File lib/vdsm/sysctl.py:

Line 37: def set_rp_filter_strict(dev):
Line 38:     set_rp_filter(dev, _RPFILTER_STRICT)
Line 39: 
Line 40: 
Line 41: def toggle_ipv6(dev, enable):
> That's true. I'll go with the more neutral "set".

Actually, "enable" is much better.
Line 42:     with open('/proc/sys/net/ipv6/conf/%s/disable_ipv6' % dev, 'w') as f:
Line 43:         f.write('0' if enable else '1')
Line 44: 
Line 45:

Change in vdsm[master]: sysctl: turn disable_ipv6 into switch_ipv6 to simplify logic

[e-mail] has posted comments on this change.

Change subject: sysctl: turn disable_ipv6 into switch_ipv6 to simplify logic
......................................................................

Patch Set 3:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Change in vdsm[master]: sysctl: turn disable_ipv6 into switch_ipv6 to simplify logic

Ondřej Svoboda has posted comments on this change.

Change subject: sysctl: turn disable_ipv6 into switch_ipv6 to simplify logic
......................................................................

Patch Set 3: Verified+1

I only renamed "toggle" to "switch" (and changed an extra comment).