Merge "Correct return values for bridge sysctl calls"

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):

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)

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(