From cac2436f298491dbca2c932c80bdf3a64ac39ee6 Mon Sep 17 00:00:00 2001
From: Andreas Scheuring <andreas.scheuring@de.ibm.com>
Date: Thu, 3 Dec 2015 14:54:39 +0100
Subject: [PATCH] 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>
---
 neutron/agent/linux/bridge_lib.py             | 27 ++++++++++++++++++-
 .../functional/agent/linux/test_bridge_lib.py | 14 ++++++++++
 .../tests/unit/agent/linux/test_bridge_lib.py |  7 ++++-
 3 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/neutron/agent/linux/bridge_lib.py b/neutron/agent/linux/bridge_lib.py
index 13610f4cf88..625ae9404f8 100644
--- 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 7c88b8cb9da..5eb90d12c8d 100644
--- 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 d1c842c425b..0c28ac8df2a 100644
--- 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(