Correct return values for bridge sysctl calls

This fixes an issue where the lb agent did not plug the
dhcp tap device into the bridge when having vlan networking
set up.  Caused by setting of disable_ipv6 value.

Closes-Bug: #1520618
Change-Id: I0d21fad3a676d1fdd30501ea6a295f1e9b207a3a
Co-Authored-By: Brian Haley <brian.haley@hpe.com>

diff --git a/neutron/agent/linux/bridge_lib.py b/neutron/agent/linux/bridge_lib.py
index 13610f4cf884910c5acf867bc43bb10e2017e5a4..625ae9404f838d1484ecac0496091f85b3ff28f2 100644 (file)
--- a/neutron/agent/linux/bridge_lib.py
+++ b/neutron/agent/linux/bridge_lib.py
@@ -17,8 +17,12 @@
 #    under the License.
 
 import os
+from oslo_log import log as logging
 
 from neutron.agent.linux import ip_lib
+from neutron.i18n import _LE
+
+LOG = logging.getLogger(__name__)
 
 # NOTE(toabctl): Don't use /sys/devices/virtual/net here because not all tap
 # devices are listed here (i.e. when using Xen)
@@ -47,9 +51,30 @@ class BridgeDevice(ip_lib.IPDevice):
         return ip_wrapper.netns.execute(cmd, run_as_root=True)
 
     def _sysctl(self, cmd):
+        """execute() doesn't return the exit status of the command it runs,
+        it returns stdout and stderr. Setting check_exit_code=True will cause
+        it to raise a RuntimeError if the exit status of the command is
+        non-zero, which in sysctl's case is an error. So we're normalizing
+        that into zero (success) and one (failure) here to mimic what
+        "echo $?" in a shell would be.
+
+        This is all because sysctl is too verbose and prints the value you
+        just set on success, unlike most other utilities that print nothing.
+
+        execute() will have dumped a message to the logs with the actual
+        output on failure, so it's not lost, and we don't need to print it
+        here.
+        """
         cmd = ['sysctl', '-w'] + cmd
         ip_wrapper = ip_lib.IPWrapper(self.namespace)
-        return ip_wrapper.netns.execute(cmd, run_as_root=True)
+        try:
+            ip_wrapper.netns.execute(cmd, run_as_root=True,
+                                     check_exit_code=True)
+        except RuntimeError:
+            LOG.exception(_LE("Failed running %s"), cmd)
+            return 1
+
+        return 0
 
     @classmethod
     def addbr(cls, name, namespace=None):

diff --git a/neutron/tests/functional/agent/linux/test_bridge_lib.py b/neutron/tests/functional/agent/linux/test_bridge_lib.py
index 7c88b8cb9da9338c3ece88fe2d9801d6e92d130b..5eb90d12c8d9e938706b793d4d739c87a67e819b 100644 (file)
--- a/neutron/tests/functional/agent/linux/test_bridge_lib.py
+++ b/neutron/tests/functional/agent/linux/test_bridge_lib.py
@@ -59,3 +59,17 @@ class BridgeLibTestCase(base.BaseSudoTestCase):
     def test_get_interfaces_no_bridge(self):
         bridge = bridge_lib.BridgeDevice('--fake--')
         self.assertEqual([], bridge.get_interfaces())
+
+    def test_disable_ipv6(self):
+        sysfs_path = ("/proc/sys/net/ipv6/conf/%s/disable_ipv6" %
+                      self.bridge.name)
+
+        # first, make sure it's enabled
+        with open(sysfs_path, 'r') as sysfs_disable_ipv6_file:
+            sysfs_disable_ipv6 = sysfs_disable_ipv6_file.read()
+            self.assertEqual("0\n", sysfs_disable_ipv6)
+
+        self.assertEqual(0, self.bridge.disable_ipv6())
+        with open(sysfs_path, 'r') as sysfs_disable_ipv6_file:
+            sysfs_disable_ipv6 = sysfs_disable_ipv6_file.read()
+            self.assertEqual("1\n", sysfs_disable_ipv6)

diff --git a/neutron/tests/unit/agent/linux/test_bridge_lib.py b/neutron/tests/unit/agent/linux/test_bridge_lib.py
index d1c842c425ba4cfee7890c0f9c2b78caf271f345..0c28ac8df2a97c566f188bb000570e753701c71b 100644 (file)
--- a/neutron/tests/unit/agent/linux/test_bridge_lib.py
+++ b/neutron/tests/unit/agent/linux/test_bridge_lib.py
@@ -36,6 +36,11 @@ class BridgeLibTest(base.BaseTestCase):
         self.execute.assert_called_once_with(cmd, run_as_root=True)
         self.execute.reset_mock()
 
+    def _verify_bridge_mock_check_exit_code(self, cmd):
+        self.execute.assert_called_once_with(cmd, run_as_root=True,
+                                             check_exit_code=True)
+        self.execute.reset_mock()
+
     def test_is_bridged_interface(self):
         exists = lambda path: path == "/sys/class/net/tapOK/brport"
         with mock.patch('os.path.exists', side_effect=exists):
@@ -64,7 +69,7 @@ class BridgeLibTest(base.BaseTestCase):
 
         br.disable_ipv6()
         cmd = 'net.ipv6.conf.%s.disable_ipv6=1' % self._BR_NAME
-        self._verify_bridge_mock(['sysctl', '-w', cmd])
+        self._verify_bridge_mock_check_exit_code(['sysctl', '-w', cmd])
 
         br.addif(self._IF_NAME)
         self._verify_bridge_mock(