From 1f6fed6a69e9fd386e421f3cacae97c11cdd7c75 Mon Sep 17 00:00:00 2001 From: Sean Mooney Date: Thu, 14 Feb 2019 03:59:50 +0000 Subject: [PATCH] remove use of brctl from vif_plug_linux_bridge - This change replaces calls to brctl with calls to the ip_command interface. - This change adds a release note for the brctl removal. - This change removes the use of tee to disable ipv6 in the linux bridge plugin. Change-Id: Id03be72e22302a0954f3e47c116f389cb4304c03 Closes-Bug: #1801919 --- .../notes/brctl-removal-a5b0e69b865afa57.yaml | 10 ++ vif_plug_linux_bridge/linux_net.py | 118 +++++++++--------- .../tests/unit/test_linux_net.py | 105 +++++++--------- 3 files changed, 111 insertions(+), 122 deletions(-) create mode 100644 releasenotes/notes/brctl-removal-a5b0e69b865afa57.yaml diff --git a/releasenotes/notes/brctl-removal-a5b0e69b865afa57.yaml b/releasenotes/notes/brctl-removal-a5b0e69b865afa57.yaml new file mode 100644 index 00000000..b093b5e1 --- /dev/null +++ b/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. diff --git a/vif_plug_linux_bridge/linux_net.py b/vif_plug_linux_bridge/linux_net.py index 5b3a3d32..66217c34 100644 --- a/vif_plug_linux_bridge/linux_net.py +++ b/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: - processutils.execute('brctl', 'addbr', 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.add(bridge, 'bridge') + _disable_ipv6(bridge) 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, - interface, check_exit_code=False) - 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') + ip_lib.set(interface, master=bridge, state='up', + check_exit_code=[0, 2, 254]) _set_device_mtu(interface, mtu) - - # 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) - + _update_bridge_routes(interface, bridge) # NOTE(sean-k-mooney): # The bridge mtu cannont be set until after an # interface is added due to bug: diff --git a/vif_plug_linux_bridge/tests/unit/test_linux_net.py b/vif_plug_linux_bridge/tests/unit/test_linux_net.py index 214153b8..e909c5ac 100644 --- a/vif_plug_linux_bridge/tests/unit/test_linux_net.py +++ b/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(ip_lib, "exists", return_value=True) - def test_ensure_bridge_exists(self, mock_dev_exists, mock_exec): + @mock.patch.object(linux_net, "_ensure_bridge_privileged") + @mock.patch.object(linux_net, "_ensure_bridge_filtering") + def test_ensure_bridge(self, mock_filtering, mock_priv): linux_net.ensure_bridge("br0", None, filtering=False) - mock_exec.assert_not_called() - mock_dev_exists.assert_called_once_with("br0") + mock_priv.assert_called_once_with("br0", None, None, True, + 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") - def test_ensure_bridge_addbr_exception(self, mock_exec, mock_dev_exists): - mock_exec.side_effect = ValueError() + @mock.patch.object(ip_lib, "add") + def test_ensure_bridge_addbr_exception(self, mock_add, mock_dev_exists): + 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(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, "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(processutils, "execute") - @mock.patch.object(ip_lib, "exists", return_value=False) - def test_ensure_bridge_mtu_not_called(self, mock_dev_exists, mock_exec, - mock_path_exists, mock_set_mtu, mock_ip_set): + @mock.patch.object(linux_net, "_disable_ipv6") + @mock.patch.object(linux_net, "_update_bridge_routes") + @mock.patch.object(ip_lib, "exists") + def test_ensure_bridge_priv_mtu_not_called(self, mock_dev_exists, + 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(processutils, "execute", return_value=("", "")) - @mock.patch.object(ip_lib, "exists", return_value=False) - def test_ensure_bridge_mtu_order(self, mock_dev_exists, mock_exec, - mock_path_exists, mock_set_mtu, mock_ip_set): + @mock.patch.object(linux_net, "_disable_ipv6") + @mock.patch.object(linux_net, "_update_bridge_routes") + @mock.patch.object(ip_lib, "exists") + def test_ensure_bridge_priv_mtu_order(self, mock_dev_exists, mock_routes, + 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.object(os.path, "exists", return_value=False) - @mock.patch.object(processutils, "execute") - @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) + @mock.patch('six.moves.builtins.open') + @mock.patch("os.path.exists") + def test__disable_ipv6(self, mock_exists, mock_open): - 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_called_once_with("br0") - mock_ip_set.assert_called_once_with('br0', state='up') + exists_path = "/proc/sys/net/ipv6/conf/br0/disable_ipv6" + mock_exists.return_value = False - @mock.patch.object(ip_lib, "set") - @mock.patch.object(os.path, "exists", return_value=True) - @mock.patch.object(processutils, "execute") - @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) + linux_net._disable_ipv6("br0") + mock_exists.assert_called_once_with(exists_path) + mock_open.assert_not_called() - calls = [mock.call('brctl', 'addbr', 'br0'), - mock.call('brctl', 'setfd', 'br0', 0), - mock.call('brctl', 'stp', 'br0', "off"), - mock.call('tee', '/proc/sys/net/ipv6/conf/br0/disable_ipv6', - check_exit_code=[0, 1], process_input='1')] - 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_exists.reset_mock() + mock_exists.return_value = True + linux_net._disable_ipv6("br0") + mock_exists.assert_called_once_with(exists_path) + mock_open.assert_called_once_with(exists_path, 'w')