From 24b379ad4d6230d65c1cb044a66937e3e7c11465 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Thu, 4 Jun 2020 17:58:45 +0000 Subject: [PATCH] Implement "BridgeDevice" with Pyroute2 Change-Id: I9e64a4d4b931a132d25434eaeb9dcec6ebf0e6f8 Story: #2007686 Task: #39975 --- neutron/agent/linux/bridge_lib.py | 48 +++++++++++++------ neutron/agent/linux/ip_lib.py | 4 ++ neutron/privileged/agent/linux/ip_lib.py | 19 ++++++++ neutron/tests/common/net_helpers.py | 4 +- .../functional/agent/linux/test_bridge_lib.py | 38 +++++++++++++-- .../tests/unit/agent/linux/test_bridge_lib.py | 44 ++--------------- .../agent/test_linuxbridge_neutron_agent.py | 9 ++-- 7 files changed, 101 insertions(+), 65 deletions(-) diff --git a/neutron/agent/linux/bridge_lib.py b/neutron/agent/linux/bridge_lib.py index 2482e6d7478..a1abf512382 100644 --- a/neutron/agent/linux/bridge_lib.py +++ b/neutron/agent/linux/bridge_lib.py @@ -16,11 +16,13 @@ # License for the specific language governing permissions and limitations # under the License. +import functools import os -from oslo_utils import excutils +from pyroute2.netlink import exceptions as netlink_exceptions from neutron.agent.linux import ip_lib +from neutron.privileged.agent.linux import ip_lib as priv_ip_lib # NOTE(toabctl): Don't use /sys/devices/virtual/net here because not all tap # devices are listed here (i.e. when using Xen) @@ -31,6 +33,20 @@ BRIDGE_PORT_FS_FOR_DEVICE = BRIDGE_FS + "%s/brport" BRIDGE_PATH_FOR_DEVICE = BRIDGE_PORT_FS_FOR_DEVICE + '/bridge' +def catch_exceptions(function): + """Catch bridge command exceptions and mimic $? output""" + + @functools.wraps(function) + def decorated_function(self, *args, **kwargs): + try: + function(self, *args, **kwargs) + return 0 + except (RuntimeError, OSError, netlink_exceptions.NetlinkError): + return 1 + + return decorated_function + + def is_bridged_interface(interface): if not interface: return False @@ -51,19 +67,14 @@ def get_bridge_names(): class BridgeDevice(ip_lib.IPDevice): - def _ip_link(self, cmd): - cmd = ['ip', 'link'] + cmd - ip_wrapper = ip_lib.IPWrapper(self.namespace) - return ip_wrapper.netns.execute(cmd, run_as_root=True) @classmethod def addbr(cls, name, namespace=None): bridge = cls(name, namespace, 'bridge') try: bridge.link.create() - except RuntimeError: - with excutils.save_and_reraise_exception() as ectx: - ectx.reraise = not bridge.exists() + except priv_ip_lib.InterfaceAlreadyExists: + pass return bridge @classmethod @@ -79,19 +90,28 @@ class BridgeDevice(ip_lib.IPDevice): def delbr(self): return self.link.delete() + @catch_exceptions def addif(self, interface): - return self._ip_link(['set', 'dev', interface, 'master', self.name]) + priv_ip_lib.set_link_bridge_master(interface, self.name, + namespace=self.namespace) + @catch_exceptions def delif(self, interface): - return self._ip_link(['set', 'dev', interface, 'nomaster']) + priv_ip_lib.set_link_bridge_master(interface, None, + namespace=self.namespace) + @catch_exceptions def setfd(self, fd): - return self._ip_link(['set', 'dev', self.name, 'type', 'bridge', - 'forward_delay', str(fd)]) + priv_ip_lib.set_link_bridge_forward_delay(self.name, fd, + namespace=self.namespace) + @catch_exceptions def disable_stp(self): - return self._ip_link(['set', 'dev', self.name, 'type', 'bridge', - 'stp_state', 0]) + priv_ip_lib.set_link_bridge_stp(self.name, 0, namespace=self.namespace) + + @catch_exceptions + def enable_stp(self): + priv_ip_lib.set_link_bridge_stp(self.name, 1, namespace=self.namespace) def owns_interface(self, interface): return os.path.exists( diff --git a/neutron/agent/linux/ip_lib.py b/neutron/agent/linux/ip_lib.py index 414e57d4950..7d992ff4374 100644 --- a/neutron/agent/linux/ip_lib.py +++ b/neutron/agent/linux/ip_lib.py @@ -1395,6 +1395,10 @@ def get_devices_info(namespace, **kwargs): 'IFLA_VXLAN_LINK') elif ret['kind'] == 'vlan': ret['vlan_id'] = get_attr(ifla_data, 'IFLA_VLAN_ID') + elif ret['kind'] == 'bridge': + ret['stp'] = get_attr(ifla_data, 'IFLA_BR_STP_STATE') + ret['forward_delay'] = get_attr(ifla_data, + 'IFLA_BR_FORWARD_DELAY') retval[device['index']] = ret for device in retval.values(): diff --git a/neutron/privileged/agent/linux/ip_lib.py b/neutron/privileged/agent/linux/ip_lib.py index 5cc5462b56f..57e8a53c8b4 100644 --- a/neutron/privileged/agent/linux/ip_lib.py +++ b/neutron/privileged/agent/linux/ip_lib.py @@ -415,6 +415,25 @@ def set_link_vf_feature(device, namespace, vf_config): return _run_iproute_link("set", device, namespace=namespace, vf=vf_config) +@privileged.default.entrypoint +def set_link_bridge_forward_delay(device, forward_delay, namespace=None): + return _run_iproute_link('set', device, namespace=namespace, + kind='bridge', br_forward_delay=forward_delay) + + +@privileged.default.entrypoint +def set_link_bridge_stp(device, stp, namespace=None): + return _run_iproute_link('set', device, namespace=namespace, + kind='bridge', br_stp_state=stp) + + +@privileged.default.entrypoint +def set_link_bridge_master(device, bridge, namespace=None): + bridge_idx = get_link_id(bridge, namespace) if bridge else 0 + return _run_iproute_link('set', device, namespace=namespace, + master=bridge_idx) + + @privileged.default.entrypoint def get_link_attributes(device, namespace): link = _run_iproute_link("get", device, namespace)[0] diff --git a/neutron/tests/common/net_helpers.py b/neutron/tests/common/net_helpers.py index b5899456235..55cd2b3df36 100644 --- a/neutron/tests/common/net_helpers.py +++ b/neutron/tests/common/net_helpers.py @@ -864,7 +864,7 @@ class OVSPortFixture(PortFixture): self.addCleanup(self.port.link.delete) ip_wrapper.add_device_to_namespace(self.port) bridge_port.link.set_up() - self.qbr.addif(bridge_port) + self.qbr.addif(bridge_port.name) self.port.link.set_address(self.mac) self.port.link.set_up() @@ -963,7 +963,7 @@ class LinuxBridgePortFixture(PortFixture): # bridge side br_ip_wrapper = ip_lib.IPWrapper(self.bridge.namespace) br_ip_wrapper.add_device_to_namespace(self.br_port) - self.bridge.addif(self.br_port) + self.bridge.addif(self.br_port.name) self.br_port.link.set_up() # port side diff --git a/neutron/tests/functional/agent/linux/test_bridge_lib.py b/neutron/tests/functional/agent/linux/test_bridge_lib.py index f9887e45262..6091fc586f3 100644 --- a/neutron/tests/functional/agent/linux/test_bridge_lib.py +++ b/neutron/tests/functional/agent/linux/test_bridge_lib.py @@ -41,14 +41,23 @@ class BridgeLibTestCase(base.BaseSudoTestCase): bridge, port_id=uuidutils.generate_uuid())) return bridge, port_fixture - def test_is_bridged_interface(self): - self.assertTrue( - bridge_lib.is_bridged_interface(self.port_fixture.br_port.name)) + def test_is_bridged_interface_and_remove(self): + bridge = bridge_lib.BridgeDevice(self.bridge.name) + bridge_port = self.port_fixture.br_port.name + self.assertTrue(bridge_lib.is_bridged_interface(bridge_port)) + bridge.delif(bridge_port) + self.assertFalse(bridge_lib.is_bridged_interface(bridge_port)) def test_is_not_bridged_interface(self): self.assertFalse( bridge_lib.is_bridged_interface(self.port_fixture.port.name)) + def test_delete_bridge(self): + bridge = bridge_lib.BridgeDevice(self.bridge.name) + self.assertTrue(bridge.exists()) + bridge.delbr() + self.assertFalse(bridge.exists()) + def test_get_bridge_names(self): self.assertIn(self.bridge.name, bridge_lib.get_bridge_names()) @@ -94,6 +103,29 @@ class BridgeLibTestCase(base.BaseSudoTestCase): sysfs_disable_ipv6 = sysfs_disable_ipv6_file.read() self.assertEqual("1\n", sysfs_disable_ipv6) + def _get_bridge_info(self): + for device in (device for device in ip_lib.get_devices_info( + self.bridge.namespace) if device['name'] == self.bridge.name): + return device + self.fail('Bridge %s not present' % self.bridge.name) + + def test_set_forward_delay(self): + bridge = bridge_lib.BridgeDevice(self.bridge.name) + for fd in (10, 200, 3000, 40000): + bridge.setfd(fd) + br_info = self._get_bridge_info() + self.assertEqual(fd, br_info['forward_delay']) + + def test_enable_and_disable_stp(self): + bridge = bridge_lib.BridgeDevice(self.bridge.name) + bridge.disable_stp() + br_info = self._get_bridge_info() + self.assertEqual(0, br_info['stp']) + + bridge.enable_stp() + br_info = self._get_bridge_info() + self.assertEqual(1, br_info['stp']) + class FdbInterfaceTestCase(testscenarios.WithScenarios, base.BaseSudoTestCase): diff --git a/neutron/tests/unit/agent/linux/test_bridge_lib.py b/neutron/tests/unit/agent/linux/test_bridge_lib.py index 0763a872328..979720ea8f1 100644 --- a/neutron/tests/unit/agent/linux/test_bridge_lib.py +++ b/neutron/tests/unit/agent/linux/test_bridge_lib.py @@ -65,48 +65,10 @@ class BridgeLibTest(base.BaseTestCase): br = bridge_lib.BridgeDevice.get_interface_bridge('tap0') self.assertIsNone(br) - def _test_br(self, namespace=None): - br = bridge_lib.BridgeDevice.addbr(self._BR_NAME, namespace) - self.assertEqual(namespace, br.namespace) - self.create.assert_called_once_with(self._BR_NAME, br.namespace, - 'bridge') - - br.setfd(0) - self._verify_bridge_mock(['ip', 'link', 'set', 'dev', self._BR_NAME, - 'type', 'bridge', 'forward_delay', '0']) - - br.disable_stp() - self._verify_bridge_mock(['ip', 'link', 'set', 'dev', self._BR_NAME, - 'type', 'bridge', 'stp_state', 0]) - - br.disable_ipv6() - cmd = 'net.ipv6.conf.%s.disable_ipv6=1' % self._BR_NAME - self._verify_bridge_sysctl_mock(['sysctl', '-w', cmd]) - - br.addif(self._IF_NAME) - self._verify_bridge_mock( - ['ip', 'link', 'set', 'dev', self._IF_NAME, - 'master', self._BR_NAME]) - - br.delif(self._IF_NAME) - self._verify_bridge_mock( - ['ip', 'link', 'set', 'dev', self._IF_NAME, 'nomaster']) - - br.delbr() - self.delete.assert_called_once_with(self._BR_NAME, br.namespace) - - def test_addbr_with_namespace(self): - self._test_br(self._NAMESPACE) - - def test_addbr_without_namespace(self): - self._test_br() - def test_addbr_exists(self): - self.create.side_effect = RuntimeError() - with mock.patch.object(bridge_lib.BridgeDevice, 'exists', - return_value=True): - bridge_lib.BridgeDevice.addbr(self._BR_NAME) - bridge_lib.BridgeDevice.addbr(self._BR_NAME) + self.create.side_effect = priv_lib.InterfaceAlreadyExists + bridge_lib.BridgeDevice.addbr(self._BR_NAME) + bridge_lib.BridgeDevice.addbr(self._BR_NAME) def test_owns_interface(self): br = bridge_lib.BridgeDevice('br-int') diff --git a/neutron/tests/unit/plugins/ml2/drivers/linuxbridge/agent/test_linuxbridge_neutron_agent.py b/neutron/tests/unit/plugins/ml2/drivers/linuxbridge/agent/test_linuxbridge_neutron_agent.py index 4addd8fce12..6630abeafe2 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/linuxbridge/agent/test_linuxbridge_neutron_agent.py +++ b/neutron/tests/unit/plugins/ml2/drivers/linuxbridge/agent/test_linuxbridge_neutron_agent.py @@ -521,10 +521,10 @@ class TestLinuxBridgeManager(base.BaseTestCase): 'get_interface_bridge') as get_if_br_fn: de_fn.return_value = False br_fn.addbr.return_value = bridge_device - bridge_device.setfd.return_value = False - bridge_device.disable_stp.return_value = False + bridge_device.setfd.return_value = 0 + bridge_device.disable_stp.return_value = 0 bridge_device.disable_ipv6.return_value = False - bridge_device.link.set_up.return_value = False + bridge_device.link.set_up.return_value = 0 self.assertEqual("br0", self.lbm.ensure_bridge("br0", None)) bridge_device.owns_interface.return_value = False @@ -617,7 +617,6 @@ class TestLinuxBridgeManager(base.BaseTestCase): mock.patch.object(self.lbm, '_set_tap_mtu') as set_tap, \ mock.patch.object(bridge_lib.BridgeDevice, "get_interface_bridge") as get_br: - bridge_device.addif.retun_value = False get_br.return_value = True self.assertTrue(self.lbm.add_tap_interface( "123", constants.TYPE_LOCAL, "physnet1", None, @@ -632,7 +631,7 @@ class TestLinuxBridgeManager(base.BaseTestCase): en_fn.assert_called_with("123", "brq999") get_br.return_value = False - bridge_device.addif.retun_value = True + bridge_device.addif.retun_value = 1 self.assertFalse(self.lbm.add_tap_interface( "123", constants.TYPE_LOCAL, "physnet1", None, "tap1", dev_owner_prefix, None))