From 5027ce833c6fccaa80b5ddc8544d262c0bf99dbd Mon Sep 17 00:00:00 2001 From: Sean Mooney Date: Thu, 14 Feb 2019 03:08:53 +0000 Subject: [PATCH] remove brctl from vif_plug_ovs - This change extends the ip_command interface set function to accept a master as a parent device for a given interface. - This change extends the impl_pyroute2 add function to support creating linux bridges. - This change replaces calls to brctl with calls to the ip_command api. - This change removes the use of tee to disable ipv6 in the ovs plugin. Change-Id: I8308e8840e20b0a72d00880c1a7996b4c73f6a83 Partial-Bug: #1801919 --- os_vif/internal/command/ip/ip_command.py | 4 +- .../command/ip/linux/impl_pyroute2.py | 14 +- .../command/ip/windows/impl_netifaces.py | 2 +- .../internal/command/ip/test_impl_pyroute2.py | 15 ++ .../command/ip/linux/test_impl_pyroute2.py | 11 ++ vif_plug_ovs/linux_net.py | 52 +++--- vif_plug_ovs/tests/unit/test_linux_net.py | 158 ++++++++---------- 7 files changed, 138 insertions(+), 118 deletions(-) diff --git a/os_vif/internal/command/ip/ip_command.py b/os_vif/internal/command/ip/ip_command.py index 70605622..01ba5f9c 100644 --- a/os_vif/internal/command/ip/ip_command.py +++ b/os_vif/internal/command/ip/ip_command.py @@ -19,10 +19,11 @@ class IpCommand(object): TYPE_VETH = 'veth' TYPE_VLAN = 'vlan' + TYPE_BRIDGE = 'bridge' @abc.abstractmethod def set(self, device, check_exit_code=None, state=None, mtu=None, - address=None, promisc=None): + address=None, promisc=None, master=None): """Method to set a parameter in an interface. :param device: A network device (string) @@ -32,6 +33,7 @@ class IpCommand(object): :param mtu: Integer MTU value :param address: String MAC address :param promisc: Boolean promiscuous mode + :param master: String the master device that this device belongs to :return: status of the command execution """ diff --git a/os_vif/internal/command/ip/linux/impl_pyroute2.py b/os_vif/internal/command/ip/linux/impl_pyroute2.py index 2174177a..696663d3 100644 --- a/os_vif/internal/command/ip/linux/impl_pyroute2.py +++ b/os_vif/internal/command/ip/linux/impl_pyroute2.py @@ -38,7 +38,7 @@ class PyRoute2(ip_command.IpCommand): ctx.reraise = False def set(self, device, check_exit_code=None, state=None, mtu=None, - address=None, promisc=None): + address=None, promisc=None, master=None): check_exit_code = check_exit_code or [] with iproute.IPRoute() as ip: idx = ip.link_lookup(ifname=device) @@ -58,6 +58,8 @@ class PyRoute2(ip_command.IpCommand): args['flags'] = (utils.set_mask(flags, ifinfmsg.IFF_PROMISC) if promisc is True else utils.unset_mask(flags, ifinfmsg.IFF_PROMISC)) + if master: + args['master'] = master if isinstance(check_exit_code, int): check_exit_code = [check_exit_code] @@ -78,6 +80,16 @@ class PyRoute2(ip_command.IpCommand): args['link'] = idx[0] elif self.TYPE_VETH == dev_type: args['peer'] = peer + elif self.TYPE_BRIDGE == dev_type: + # NOTE(sean-k-mooney): the keys are defined in the pyroute2 + # codebase but are not documented. see the nla_map field + # in the bridge_data class located in the + # pyroute2.netlink.rtnl.ifinfmsg module for mode details + # https://github.com/svinota/pyroute2/blob/3ba9cdde34b2346ef8c2f8ba17cef5dbeb4c6d52/pyroute2/netlink/rtnl/ifinfmsg/__init__.py#L776-L820 + args['IFLA_BR_AGEING_TIME'] = 0 # disable mac learning ageing + args['IFLA_BR_FORWARD_DELAY'] = 0 # set no delay + args['IFLA_BR_STP_STATE'] = 0 # disable spanning tree + args['IFLA_BR_MCAST_SNOOPING'] = 0 # disable snooping else: raise exception.NetworkInterfaceTypeNotDefined(type=dev_type) diff --git a/os_vif/internal/command/ip/windows/impl_netifaces.py b/os_vif/internal/command/ip/windows/impl_netifaces.py index a17c1755..9b67114b 100644 --- a/os_vif/internal/command/ip/windows/impl_netifaces.py +++ b/os_vif/internal/command/ip/windows/impl_netifaces.py @@ -35,7 +35,7 @@ class Netifaces(ip_command.IpCommand): return False def set(self, device, check_exit_code=None, state=None, mtu=None, - address=None, promisc=None): + address=None, promisc=None, master=None): exception.NotImplementedForOS(function='ip.set', os='Windows') def add(self, device, dev_type, check_exit_code=None, peer=None, link=None, diff --git a/os_vif/tests/functional/internal/command/ip/test_impl_pyroute2.py b/os_vif/tests/functional/internal/command/ip/test_impl_pyroute2.py index 9539c238..ed4dcbe2 100644 --- a/os_vif/tests/functional/internal/command/ip/test_impl_pyroute2.py +++ b/os_vif/tests/functional/internal/command/ip/test_impl_pyroute2.py @@ -205,3 +205,18 @@ class TestIpCommand(ShellIpCommands, base.BaseFunctionalTestCase): self.assertTrue(_ip_cmd_exists(device)) self.del_device(device) self.assertFalse(_ip_cmd_exists(device)) + + def test_add_bridge(self): + device = "test_dev_11" + self.addCleanup(self.del_device, device) + _ip_cmd_add(device, 'bridge') + self.assertTrue(self.exist_device(device)) + base_path = "/sys/class/net/test_dev_11/bridge/%s" + with open(base_path % "forward_delay", "r") as f: + self.assertEqual("0", f.readline().rstrip('\n')) + with open(base_path % "stp_state", "r") as f: + self.assertEqual("0", f.readline().rstrip('\n')) + with open(base_path % "ageing_time", "r") as f: + self.assertEqual("0", f.readline().rstrip('\n')) + with open(base_path % "multicast_snooping", "r") as f: + self.assertEqual("0", f.readline().rstrip('\n')) diff --git a/os_vif/tests/unit/internal/command/ip/linux/test_impl_pyroute2.py b/os_vif/tests/unit/internal/command/ip/linux/test_impl_pyroute2.py index b28b0594..50a70b86 100644 --- a/os_vif/tests/unit/internal/command/ip/linux/test_impl_pyroute2.py +++ b/os_vif/tests/unit/internal/command/ip/linux/test_impl_pyroute2.py @@ -30,6 +30,7 @@ class TestIpCommand(base.TestCase): UP = 'up' TYPE_VETH = 'veth' TYPE_VLAN = 'vlan' + TYPE_BRIDGE = 'bridge' LINK = 'device2' VLAN_ID = 14 @@ -92,6 +93,16 @@ class TestIpCommand(base.TestCase): 'link': 1} self.ip_link.assert_called_once_with('add', **args) + def test_add_bridge(self): + self.ip.add(self.DEVICE, self.TYPE_BRIDGE) + args = {'ifname': self.DEVICE, + 'kind': self.TYPE_BRIDGE, + 'IFLA_BR_AGEING_TIME': 0, + 'IFLA_BR_FORWARD_DELAY': 0, + 'IFLA_BR_STP_STATE': 0, + 'IFLA_BR_MCAST_SNOOPING': 0} + self.ip_link.assert_called_once_with('add', **args) + def test_add_vlan_no_interface_found(self): with mock.patch.object(iproute.IPRoute, 'link_lookup', return_value=[]) as mock_link_lookup: diff --git a/vif_plug_ovs/linux_net.py b/vif_plug_ovs/linux_net.py index 689a4e1b..e09ba42e 100644 --- a/vif_plug_ovs/linux_net.py +++ b/vif_plug_ovs/linux_net.py @@ -60,12 +60,6 @@ def _update_device_mtu(dev, mtu): set_device_mtu(dev, mtu) -def interface_in_bridge(bridge, device): - """Check if an ethernet device belongs to a Linux Bridge.""" - return os.path.exists('/sys/class/net/%(bridge)s/brif/%(device)s' % - {'bridge': bridge, 'device': device}) - - @privsep.vif_plug.entrypoint def delete_net_dev(dev): """Delete a network device only if it exists.""" @@ -100,24 +94,26 @@ def update_veth_pair(dev1_name, dev2_name, mtu): _update_device_mtu(dev, mtu) +def _disable_ipv6(bridge): + """Disable ipv6 if available for bridge. Must be called from + privsep context. + """ + # NOTE(sean-k-mooney): os-vif disables ipv6 to ensure the Bridge + # does not aquire an ipv6 auto config or link local adress. + # This is required to prevent bug 1302080. + # https://bugs.launchpad.net/neutron/+bug/1302080 + disv6 = ('/proc/sys/net/ipv6/conf/%s/disable_ipv6' % + bridge) + if os.path.exists(disv6): + with open(disv6, 'w') as f: + f.write('1') + + @privsep.vif_plug.entrypoint def ensure_bridge(bridge): if not ip_lib.exists(bridge): - processutils.execute('brctl', 'addbr', bridge) - processutils.execute('brctl', 'setfd', bridge, 0) - processutils.execute('brctl', 'stp', bridge, 'off') - processutils.execute('brctl', 'setageing', bridge, 0) - syspath = '/sys/class/net/%s/bridge/multicast_snooping' - syspath = syspath % bridge - processutils.execute('tee', syspath, process_input='0', - check_exit_code=[0, 1]) - 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]) + ip_lib.add(bridge, 'bridge') + _disable_ipv6(bridge) # we bring up the bridge to allow it to switch packets set_interface_state(bridge, 'up') @@ -125,16 +121,18 @@ def ensure_bridge(bridge): @privsep.vif_plug.entrypoint def delete_bridge(bridge, dev): if ip_lib.exists(bridge): - if interface_in_bridge(bridge, dev): - processutils.execute('brctl', 'delif', bridge, dev) - - ip_lib.set(bridge, state='down') - processutils.execute('brctl', 'delbr', bridge) + # Note(sean-k-mooney): this will detach all ports on + # the bridge before deleting the bridge. + ip_lib.delete(bridge, check_exit_code=[0, 2, 254]) + # howver it will not set the detached interface down + # so we set the dev down if dev is not None and exists. + if dev and ip_lib.exists(dev): + set_interface_state(dev, "down") @privsep.vif_plug.entrypoint def add_bridge_port(bridge, dev): - processutils.execute('brctl', 'addif', bridge, dev) + ip_lib.set(dev, master=bridge) @privsep.vif_plug.entrypoint diff --git a/vif_plug_ovs/tests/unit/test_linux_net.py b/vif_plug_ovs/tests/unit/test_linux_net.py index b375338b..b4fa4925 100644 --- a/vif_plug_ovs/tests/unit/test_linux_net.py +++ b/vif_plug_ovs/tests/unit/test_linux_net.py @@ -16,7 +16,6 @@ import os.path import testtools from os_vif.internal.command import ip as ip_lib -from oslo_concurrency import processutils from vif_plug_ovs import exception from vif_plug_ovs import linux_net @@ -30,105 +29,88 @@ class LinuxNetTest(testtools.TestCase): privsep.vif_plug.set_client_mode(False) - @mock.patch.object(ip_lib, "set") - @mock.patch.object(ip_lib, "exists", return_value=True) - def test_ensure_bridge_exists(self, mock_dev_exists, mock_ip_set): + @mock.patch.object(linux_net, "set_interface_state") + @mock.patch.object(linux_net, "_disable_ipv6") + @mock.patch.object(ip_lib, "add") + @mock.patch.object(ip_lib, "exists", return_value=False) + def test_ensure_bridge(self, mock_dev_exists, mock_add, + mock_disable_ipv6, mock_set_state): linux_net.ensure_bridge("br0") - mock_ip_set.assert_called_once_with('br0', state='up', - check_exit_code=[0, 2, 254]) - mock_dev_exists.assert_has_calls([mock.call("br0")]) + mock_dev_exists.assert_called_once_with("br0") + mock_add.assert_called_once_with("br0", "bridge") + mock_disable_ipv6.assert_called_once_with("br0") + mock_set_state.assert_called_once_with("br0", "up") - @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_execute, - mock_path_exists, mock_ip_set): + @mock.patch.object(linux_net, "set_interface_state") + @mock.patch.object(linux_net, "_disable_ipv6") + @mock.patch.object(ip_lib, "add") + @mock.patch.object(ip_lib, "exists", return_value=True) + def test_ensure_bridge_exists(self, mock_dev_exists, mock_add, + mock_disable_ipv6, mock_set_state): linux_net.ensure_bridge("br0") - calls = [ - mock.call('brctl', 'addbr', 'br0'), - mock.call('brctl', 'setfd', 'br0', 0), - mock.call('brctl', 'stp', 'br0', "off"), - mock.call('brctl', 'setageing', 'br0', 0), - mock.call('tee', '/sys/class/net/br0/bridge/multicast_snooping', - check_exit_code=[0, 1], process_input='0'), - ] - mock_execute.assert_has_calls(calls) - mock_dev_exists.assert_has_calls([mock.call("br0")]) - mock_ip_set.assert_called_once_with('br0', state='up', - check_exit_code=[0, 2, 254]) + mock_dev_exists.assert_called_once_with("br0") + mock_add.assert_not_called() + mock_disable_ipv6.assert_called_once_with("br0") + mock_set_state.assert_called_once_with("br0", "up") - @mock.patch.object(ip_lib, "set") + @mock.patch('six.moves.builtins.open') + @mock.patch("os.path.exists") + def test__disable_ipv6(self, mock_exists, mock_open): + + exists_path = "/proc/sys/net/ipv6/conf/br0/disable_ipv6" + mock_exists.return_value = False + + linux_net._disable_ipv6("br0") + mock_exists.assert_called_once_with(exists_path) + mock_open.assert_not_called() + + 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') + + @mock.patch.object(ip_lib, "delete") @mock.patch.object(ip_lib, "exists", return_value=False) - @mock.patch.object(os.path, "exists", return_value=True) - @mock.patch.object(processutils, "execute") - def test_ensure_bridge_new_ipv6(self, mock_execute, mock_path_exists, - mock_dev_exists, mock_ip_set): - linux_net.ensure_bridge("br0") - - calls = [ - mock.call('brctl', 'addbr', 'br0'), - mock.call('brctl', 'setfd', 'br0', 0), - mock.call('brctl', 'stp', 'br0', "off"), - mock.call('brctl', 'setageing', 'br0', 0), - mock.call('tee', '/sys/class/net/br0/bridge/multicast_snooping', - check_exit_code=[0, 1], process_input='0'), - mock.call('tee', '/proc/sys/net/ipv6/conf/br0/disable_ipv6', - check_exit_code=[0, 1], process_input='1'), - ] - mock_execute.assert_has_calls(calls) - mock_dev_exists.assert_has_calls([mock.call("br0")]) - mock_ip_set.assert_called_once_with('br0', state='up', - check_exit_code=[0, 2, 254]) - - @mock.patch.object(processutils, "execute") - @mock.patch.object(ip_lib, "exists", return_value=False) - @mock.patch.object(linux_net, "interface_in_bridge", return_value=False) - def test_delete_bridge_none(self, mock_interface_br, mock_dev_exists, - mock_execute,): + def test_delete_bridge_none(self, mock_dev_exists, mock_delete): linux_net.delete_bridge("br0", "vnet1") - mock_execute.assert_not_called() - mock_dev_exists.assert_has_calls([mock.call("br0")]) - mock_interface_br.assert_not_called() + mock_delete.assert_not_called() + mock_dev_exists.assert_called_once_with("br0") + + @mock.patch.object(linux_net, "set_interface_state") + @mock.patch.object(ip_lib, "delete") + @mock.patch.object(ip_lib, "exists", return_value=True) + def test_delete_bridge_exists(self, mock_dev_exists, mock_delete, + mock_set_state): + linux_net.delete_bridge("br0", "vnet1") + + mock_dev_exists.assert_has_calls([mock.call("br0"), + mock.call("vnet1")]) + mock_delete.assert_called_once_with("br0", check_exit_code=[0, 2, 254]) + mock_set_state.assert_called_once_with("vnet1", "down") + + @mock.patch.object(linux_net, "set_interface_state") + @mock.patch.object(ip_lib, "delete") + @mock.patch.object(ip_lib, "exists") + def test_delete_interface_not_present(self, mock_dev_exists, mock_delete, + mock_set_state): + mock_dev_exists.return_value = next(lambda: (yield True), + (yield False)) + + linux_net.delete_bridge("br0", "vnet1") + + mock_dev_exists.assert_has_calls([mock.call("br0"), + mock.call("vnet1")]) + mock_delete.assert_called_once_with("br0", check_exit_code=[0, 2, 254]) + mock_set_state.assert_not_called() @mock.patch.object(ip_lib, "set") - @mock.patch.object(processutils, "execute") - @mock.patch.object(ip_lib, "exists", return_value=True) - @mock.patch.object(linux_net, "interface_in_bridge", return_value=True) - def test_delete_bridge_exists(self, mock_interface_br, mock_dev_exists, - mock_execute, mock_ip_set): - linux_net.delete_bridge("br0", "vnet1") - - calls = [ - mock.call('brctl', 'delif', 'br0', 'vnet1'), - mock.call('brctl', 'delbr', 'br0')] - mock_execute.assert_has_calls(calls) - mock_dev_exists.assert_has_calls([mock.call("br0")]) - mock_interface_br.assert_called_once_with("br0", "vnet1") - mock_ip_set.assert_called_once_with('br0', state='down') - - @mock.patch.object(ip_lib, "set") - @mock.patch.object(processutils, "execute") - @mock.patch.object(ip_lib, "exists", return_value=True) - @mock.patch.object(linux_net, "interface_in_bridge", return_value=False) - def test_delete_interface_not_present(self, - mock_interface_br, mock_dev_exists, mock_execute, mock_ip_set): - linux_net.delete_bridge("br0", "vnet1") - - mock_execute.assert_called_once_with('brctl', 'delbr', 'br0') - mock_dev_exists.assert_has_calls([mock.call("br0")]) - mock_interface_br.assert_called_once_with("br0", "vnet1") - mock_ip_set.assert_called_once_with('br0', state='down') - - @mock.patch.object(processutils, "execute") - def test_add_bridge_port(self, mock_execute): + def test_add_bridge_port(self, mock_set): linux_net.add_bridge_port("br0", "vnet1") - - mock_execute.assert_has_calls([ - mock.call('brctl', 'addif', 'br0', 'vnet1')]) + mock_set.assert_called_once_with("vnet1", master="br0") @mock.patch('six.moves.builtins.open') @mock.patch.object(os.path, 'isfile')