Merge "remove use of brctl from vif_plug_linux_bridge"

releasenotes/notes/brctl-removal-a5b0e69b865afa57.yaml

@@ -0,0 +1,10 @@
+---
+other:
+  - |
+    With this release, packagers of ``os-vif`` no longer need to create a
+    depency on ``brctl``. ``brctl`` is largely considered obsolete and has
+    been replaced with iproute2 by default in many linux distributions.
+    RHEL 8 will not ship ``brctl`` in its default repos. As part of a larger
+    effort to remove usage of ``brctl`` from OpenStack ``os-vif`` has
+    replaced its usage of ``brctl`` with ``pyroute2``. This does not introduce
+    any new requirements as ``pyroute2`` is already a requirement.

vif_plug_linux_bridge/linux_net.py

@@ -25,7 +25,6 @@ from os_vif.internal.command import ip as ip_lib
 from oslo_concurrency import lockutils
 from oslo_concurrency import processutils
 from oslo_log import log as logging
-from oslo_utils import excutils
 
 from vif_plug_linux_bridge import privsep
 
@@ -97,6 +96,59 @@ def ensure_bridge(bridge, interface, net_attrs=None, gateway=True,
         _ensure_bridge_filtering(bridge, gateway)
 
 
+# TODO(sean-k-mooney): extract into common module
+def _disable_ipv6(bridge):
+    """disable ipv6 for bridge if available, must be called from
+       privsep context.
+    :param bridge: string bridge name
+    """
+    disv6 = ('/proc/sys/net/ipv6/conf/%s/disable_ipv6' %
+             bridge)
+    if os.path.exists(disv6):
+        with open(disv6, 'w') as f:
+            f.write('1')
+
+
+def _update_bridge_routes(interface, bridge):
+    """Updates routing table for a given bridge and interface.
+    :param interface: string interface name
+    :param bridge: string bridge name
+    """
+    # TODO(sean-k-mooney): investigate deleting all this route
+    # handling code. The vm tap devices should never have an ip,
+    # this is old nova networks code and i dont think it will ever
+    # be needed in os-vif.
+    # NOTE(vish): This will break if there is already an ip on the
+    #             interface, so we move any ips to the bridge
+    # NOTE(danms): We also need to copy routes to the bridge so as
+    #              not to break existing connectivity on the interface
+    old_routes = []
+    out, err = processutils.execute('ip', 'route', 'show', 'dev',
+                                    interface)
+    for line in out.split('\n'):
+        fields = line.split()
+        if fields and 'via' in fields:
+            old_routes.append(fields)
+            processutils.execute('ip', 'route', 'del', *fields)
+            out, err = processutils.execute('ip', 'addr', 'show', 'dev',
+                                            interface, 'scope', 'global')
+    for line in out.split('\n'):
+        fields = line.split()
+        if fields and fields[0] == 'inet':
+            if fields[-2] in ('secondary', 'dynamic', ):
+                params = fields[1:-2]
+            else:
+                params = fields[1:-1]
+                processutils.execute(*_ip_bridge_cmd('del', params,
+                                                     fields[-1]),
+                                     check_exit_code=[0, 2, 254])
+                processutils.execute(*_ip_bridge_cmd('add', params,
+                                                     bridge),
+                                     check_exit_code=[0, 2, 254])
+    for fields in old_routes:
+        processutils.execute('ip', 'route', 'add', *fields)
+
+
 @privsep.vif_plug.entrypoint
 def _ensure_bridge_privileged(bridge, interface, net_attrs, gateway,
                               filtering=True, mtu=None):
@@ -118,70 +170,18 @@ def _ensure_bridge_privileged(bridge, interface, net_attrs, gateway,
     """
     if not ip_lib.exists(bridge):
         LOG.debug('Starting Bridge %s', bridge)
-        try:
+        ip_lib.add(bridge, 'bridge')
-            processutils.execute('brctl', 'addbr', bridge)
+        _disable_ipv6(bridge)
-        except Exception:
-            with excutils.save_and_reraise_exception() as ectx:
-                ectx.reraise = not ip_lib.exists(bridge)
-        processutils.execute('brctl', 'setfd', bridge, 0)
-        # processutils.execute('brctl setageing %s 10' % bridge)
-        processutils.execute('brctl', 'stp', bridge, 'off')
-        disv6 = ('/proc/sys/net/ipv6/conf/%s/disable_ipv6' % bridge)
-        if os.path.exists(disv6):
-            processutils.execute('tee',
-                                 disv6,
-                                 process_input='1',
-                                 check_exit_code=[0, 1])
-        # (danwent) bridge device MAC address can't be set directly.
-        # instead it inherits the MAC address of the first device on the
-        # bridge, which will either be the vlan interface, or a
-        # physical NIC.
         ip_lib.set(bridge, state='up')
 
-    if interface:
+    if interface and ip_lib.exists(interface):
         LOG.debug('Adding interface %(interface)s to bridge %(bridge)s',
                   {'interface': interface, 'bridge': bridge})
-        out, err = processutils.execute('brctl', 'addif', bridge,
+        ip_lib.set(interface, master=bridge, state='up',
-                                        interface, check_exit_code=False)
+                   check_exit_code=[0, 2, 254])
-        if (err and err != "device %s is already a member of a bridge; "
-              "can't enslave it to bridge %s.\n" % (interface, bridge)):
-            msg = _('Failed to add interface: %s') % err
-            raise Exception(msg)
-
-        ip_lib.set(interface, state='up')
 
         _set_device_mtu(interface, mtu)
-
+        _update_bridge_routes(interface, bridge)
-        # NOTE(vish): This will break if there is already an ip on the
-        #             interface, so we move any ips to the bridge
-        # NOTE(danms): We also need to copy routes to the bridge so as
-        #              not to break existing connectivity on the interface
-        old_routes = []
-        out, err = processutils.execute('ip', 'route', 'show', 'dev',
-                                        interface)
-        for line in out.split('\n'):
-            fields = line.split()
-            if fields and 'via' in fields:
-                old_routes.append(fields)
-                processutils.execute('ip', 'route', 'del', *fields)
-        out, err = processutils.execute('ip', 'addr', 'show', 'dev', interface,
-                                        'scope', 'global')
-        for line in out.split('\n'):
-            fields = line.split()
-            if fields and fields[0] == 'inet':
-                if fields[-2] in ('secondary', 'dynamic', ):
-                    params = fields[1:-2]
-                else:
-                    params = fields[1:-1]
-                processutils.execute(*_ip_bridge_cmd('del', params,
-                                                     fields[-1]),
-                                     check_exit_code=[0, 2, 254])
-                processutils.execute(*_ip_bridge_cmd('add', params,
-                                                     bridge),
-                                     check_exit_code=[0, 2, 254])
-        for fields in old_routes:
-            processutils.execute('ip', 'route', 'add', *fields)
-
         # NOTE(sean-k-mooney):
         # The bridge mtu cannont be set until after an
         # interface is added due to bug:

vif_plug_linux_bridge/tests/unit/test_linux_net.py

@@ -11,7 +11,6 @@
 # under the License.
 
 import mock
-import os.path
 import testtools
 
 import fixtures
@@ -72,100 +71,80 @@ class LinuxNetTest(testtools.TestCase):
         mock_ip_set.assert_has_calls(set_calls)
         mock_set_mtu.assert_called_once_with('vlan123', 1500)
 
-    @mock.patch.object(processutils, "execute")
+    @mock.patch.object(linux_net, "_ensure_bridge_privileged")
-    @mock.patch.object(ip_lib, "exists", return_value=True)
+    @mock.patch.object(linux_net, "_ensure_bridge_filtering")
-    def test_ensure_bridge_exists(self, mock_dev_exists, mock_exec):
+    def test_ensure_bridge(self, mock_filtering, mock_priv):
         linux_net.ensure_bridge("br0", None, filtering=False)
 
-        mock_exec.assert_not_called()
+        mock_priv.assert_called_once_with("br0", None, None, True,
-        mock_dev_exists.assert_called_once_with("br0")
+                                          filtering=False, mtu=None)
+        mock_filtering.assert_not_called()
+
+        linux_net.ensure_bridge("br0", None, filtering=True)
+        mock_filtering.assert_called_once_with("br0", True)
 
     @mock.patch.object(ip_lib, "exists", return_value=False)
-    @mock.patch.object(processutils, "execute")
+    @mock.patch.object(ip_lib, "add")
-    def test_ensure_bridge_addbr_exception(self, mock_exec, mock_dev_exists):
+    def test_ensure_bridge_addbr_exception(self, mock_add, mock_dev_exists):
-        mock_exec.side_effect = ValueError()
+        mock_add.side_effect = ValueError()
         with testtools.ExpectedException(ValueError):
             linux_net.ensure_bridge("br0", None, filtering=False)
 
-    @mock.patch.object(ip_lib, "set")
+    @mock.patch.object(ip_lib, "add")
-    @mock.patch.object(processutils, "execute")
-    @mock.patch.object(ip_lib, "exists", side_effect=[False, True])
-    def test_ensure_bridge_concurrent_add(self, mock_dev_exists, mock_exec,
-                                          mock_ip_set):
-        mock_exec.side_effect = [ValueError(), 0, 0, 0]
-        linux_net.ensure_bridge("br0", None, filtering=False)
-
-        calls = [mock.call('brctl', 'addbr', 'br0'),
-                 mock.call('brctl', 'setfd', 'br0', 0),
-                 mock.call('brctl', 'stp', 'br0', "off")]
-        mock_exec.assert_has_calls(calls)
-        mock_dev_exists.assert_has_calls([mock.call("br0"), mock.call("br0")])
-        mock_ip_set.assert_called_once_with('br0', state='up')
-
     @mock.patch.object(ip_lib, "set")
     @mock.patch.object(linux_net, "_set_device_mtu")
-    @mock.patch.object(os.path, "exists", return_value=False)
+    @mock.patch.object(linux_net, "_disable_ipv6")
-    @mock.patch.object(processutils, "execute")
+    @mock.patch.object(linux_net, "_update_bridge_routes")
-    @mock.patch.object(ip_lib, "exists", return_value=False)
+    @mock.patch.object(ip_lib, "exists")
-    def test_ensure_bridge_mtu_not_called(self, mock_dev_exists, mock_exec,
+    def test_ensure_bridge_priv_mtu_not_called(self, mock_dev_exists,
-            mock_path_exists, mock_set_mtu, mock_ip_set):
+        mock_routes, mock_disable_ipv6, mock_set_mtu, mock_ip_set, mock_add):
         """This test validates that mtus are updated only if an interface
            is added to the bridge
         """
+        mock_dev_exists.return_value = False
         linux_net._ensure_bridge_privileged("fake-bridge", None,
                                             None, False, mtu=1500)
         mock_set_mtu.assert_not_called()
         mock_ip_set.assert_called_once_with('fake-bridge', state='up')
 
+    @mock.patch.object(ip_lib, "add")
     @mock.patch.object(ip_lib, "set")
     @mock.patch.object(linux_net, "_set_device_mtu")
-    @mock.patch.object(os.path, "exists", return_value=False)
+    @mock.patch.object(linux_net, "_disable_ipv6")
-    @mock.patch.object(processutils, "execute", return_value=("", ""))
+    @mock.patch.object(linux_net, "_update_bridge_routes")
-    @mock.patch.object(ip_lib, "exists", return_value=False)
+    @mock.patch.object(ip_lib, "exists")
-    def test_ensure_bridge_mtu_order(self, mock_dev_exists, mock_exec,
+    def test_ensure_bridge_priv_mtu_order(self, mock_dev_exists, mock_routes,
-            mock_path_exists, mock_set_mtu, mock_ip_set):
+        mock_disable_ipv6, mock_set_mtu, mock_ip_set, mock_add):
         """This test validates that when adding an interface
            to a bridge, the interface mtu is updated first
            followed by the bridge. This is required to work around
            https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1399064
         """
+        mock_dev_exists.return_value = next(lambda: (yield False),
+                                            (yield True))
         linux_net._ensure_bridge_privileged("fake-bridge", "fake-interface",
                                             None, False, mtu=1500)
         calls = [mock.call('fake-interface', 1500),
                  mock.call('fake-bridge', 1500)]
         mock_set_mtu.assert_has_calls(calls)
         calls = [mock.call('fake-bridge', state = 'up'),
-                 mock.call('fake-interface', state='up')]
+                 mock.call('fake-interface', master='fake-bridge', state='up',
+                 check_exit_code=[0, 2, 254])]
         mock_ip_set.assert_has_calls(calls)
 
-    @mock.patch.object(ip_lib, "set")
+    @mock.patch('six.moves.builtins.open')
-    @mock.patch.object(os.path, "exists", return_value=False)
+    @mock.patch("os.path.exists")
-    @mock.patch.object(processutils, "execute")
+    def test__disable_ipv6(self, mock_exists, mock_open):
-    @mock.patch.object(ip_lib, "exists", return_value=False)
-    def test_ensure_bridge_new_ipv4(self, mock_dev_exists, mock_exec,
-                                    mock_path_exists, mock_ip_set):
-        linux_net.ensure_bridge("br0", None, filtering=False)
 
-        calls = [mock.call('brctl', 'addbr', 'br0'),
+        exists_path = "/proc/sys/net/ipv6/conf/br0/disable_ipv6"
-                 mock.call('brctl', 'setfd', 'br0', 0),
+        mock_exists.return_value = False
-                 mock.call('brctl', 'stp', 'br0', "off")]
-        mock_exec.assert_has_calls(calls)
-        mock_dev_exists.assert_called_once_with("br0")
-        mock_ip_set.assert_called_once_with('br0', state='up')
 
-    @mock.patch.object(ip_lib, "set")
+        linux_net._disable_ipv6("br0")
-    @mock.patch.object(os.path, "exists", return_value=True)
+        mock_exists.assert_called_once_with(exists_path)
-    @mock.patch.object(processutils, "execute")
+        mock_open.assert_not_called()
-    @mock.patch.object(ip_lib, "exists", return_value=False)
-    def test_ensure_bridge_new_ipv6(self, mock_dev_exists, mock_exec,
-                                    mock_path_exists, mock_ip_set):
-        linux_net.ensure_bridge("br0", None, filtering=False)
 
-        calls = [mock.call('brctl', 'addbr', 'br0'),
+        mock_exists.reset_mock()
-                 mock.call('brctl', 'setfd', 'br0', 0),
+        mock_exists.return_value = True
-                 mock.call('brctl', 'stp', 'br0', "off"),
+        linux_net._disable_ipv6("br0")
-                 mock.call('tee', '/proc/sys/net/ipv6/conf/br0/disable_ipv6',
+        mock_exists.assert_called_once_with(exists_path)
-                           check_exit_code=[0, 1], process_input='1')]
+        mock_open.assert_called_once_with(exists_path, 'w')
-        mock_exec.assert_has_calls(calls)
-        mock_dev_exists.assert_called_once_with("br0")
-        mock_ip_set.assert_called_once_with('br0', state='up')