From: Cedric Brandily Date: Fri, 8 May 2015 21:28:37 +0000 (+0200) Subject: Always use BridgeDevice to manage linuxbridges X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=59a61c351d853f465775e0f5fa3cc904a6f37c3d;p=openstack-build%2Fneutron-build.git Always use BridgeDevice to manage linuxbridges BridgeDevice[1] class provides methods to manage linuxbridges through brctl. This change adds some methods to BridgeDevice in order to always use BridgeDevice to manage linuxbridges and respect DRY principle. [1] neutron.agent.linux.bridge_lib Change-Id: I4b9d755677bba0d487a261004d9ba9b11116101f --- diff --git a/neutron/agent/linux/bridge_lib.py b/neutron/agent/linux/bridge_lib.py index 2bbc9f2ce..e8176510f 100644 --- a/neutron/agent/linux/bridge_lib.py +++ b/neutron/agent/linux/bridge_lib.py @@ -39,3 +39,9 @@ class BridgeDevice(ip_lib.IPDevice): def delif(self, interface): return self._brctl(['delif', self.name, interface]) + + def setfd(self, fd): + return self._brctl(['setfd', self.name, str(fd)]) + + def disable_stp(self): + return self._brctl(['stp', self.name, 'off']) diff --git a/neutron/agent/linux/ip_lib.py b/neutron/agent/linux/ip_lib.py index 36d2b0952..6c6319ded 100644 --- a/neutron/agent/linux/ip_lib.py +++ b/neutron/agent/linux/ip_lib.py @@ -348,10 +348,10 @@ class IpLinkCommand(IpDeviceCommandBase): self._as_root([], ('set', self.name, 'mtu', mtu_size)) def set_up(self): - self._as_root([], ('set', self.name, 'up')) + return self._as_root([], ('set', self.name, 'up')) def set_down(self): - self._as_root([], ('set', self.name, 'down')) + return self._as_root([], ('set', self.name, 'down')) def set_netns(self, namespace): self._as_root([], ('set', self.name, 'netns', namespace)) diff --git a/neutron/plugins/ml2/drivers/linuxbridge/agent/linuxbridge_neutron_agent.py b/neutron/plugins/ml2/drivers/linuxbridge/agent/linuxbridge_neutron_agent.py index 61627ebe3..b9747a4ec 100644 --- a/neutron/plugins/ml2/drivers/linuxbridge/agent/linuxbridge_neutron_agent.py +++ b/neutron/plugins/ml2/drivers/linuxbridge/agent/linuxbridge_neutron_agent.py @@ -33,6 +33,7 @@ from oslo_service import loopingcall from oslo_service import service from six import moves +from neutron.agent.linux import bridge_lib from neutron.agent.linux import ip_lib from neutron.agent.linux import utils from neutron.agent import rpc as agent_rpc @@ -303,21 +304,18 @@ class LinuxBridgeManager(object): LOG.debug("Starting bridge %(bridge_name)s for subinterface " "%(interface)s", {'bridge_name': bridge_name, 'interface': interface}) - if utils.execute(['brctl', 'addbr', bridge_name], - run_as_root=True): - return - if utils.execute(['brctl', 'setfd', bridge_name, - str(0)], run_as_root=True): + bridge_device = bridge_lib.BridgeDevice.addbr(bridge_name) + if bridge_device.setfd(0): return - if utils.execute(['brctl', 'stp', bridge_name, - 'off'], run_as_root=True): + if bridge_device.disable_stp(): return - if utils.execute(['ip', 'link', 'set', bridge_name, - 'up'], run_as_root=True): + if bridge_device.link.set_up(): return LOG.debug("Done starting bridge %(bridge_name)s for " "subinterface %(interface)s", {'bridge_name': bridge_name, 'interface': interface}) + else: + bridge_device = bridge_lib.BridgeDevice(bridge_name) if not interface: return bridge_name @@ -331,11 +329,9 @@ class LinuxBridgeManager(object): # Check if the interface is not enslaved in another bridge if self.is_device_on_bridge(interface): bridge = self.get_bridge_for_tap_device(interface) - utils.execute(['brctl', 'delif', bridge, interface], - run_as_root=True) + bridge_lib.BridgeDevice(bridge).delif(interface) - utils.execute(['brctl', 'addif', bridge_name, interface], - run_as_root=True) + bridge_device.addif(interface) except Exception as e: LOG.error(_LE("Unable to add %(interface)s to %(bridge_name)s" "! Exception: %(e)s"), @@ -401,8 +397,7 @@ class LinuxBridgeManager(object): 'bridge_name': bridge_name} LOG.debug("Adding device %(tap_device_name)s to bridge " "%(bridge_name)s", data) - if utils.execute(['brctl', 'addif', bridge_name, tap_device_name], - run_as_root=True): + if bridge_lib.BridgeDevice(bridge_name).addif(tap_device_name): return False else: data = {'tap_device_name': tap_device_name, @@ -450,11 +445,10 @@ class LinuxBridgeManager(object): self.delete_vlan(interface) LOG.debug("Deleting bridge %s", bridge_name) - if utils.execute(['ip', 'link', 'set', bridge_name, 'down'], - run_as_root=True): + bridge_device = bridge_lib.BridgeDevice(bridge_name) + if bridge_device.link.set_down(): return - if utils.execute(['brctl', 'delbr', bridge_name], - run_as_root=True): + if bridge_device.delbr(): return LOG.debug("Done deleting bridge %s", bridge_name) @@ -477,8 +471,7 @@ class LinuxBridgeManager(object): "%(bridge_name)s", {'interface_name': interface_name, 'bridge_name': bridge_name}) - if utils.execute(['brctl', 'delif', bridge_name, interface_name], - run_as_root=True): + if bridge_lib.BridgeDevice(bridge_name).delif(interface_name): return False LOG.debug("Done removing device %(interface_name)s from bridge " "%(bridge_name)s", diff --git a/neutron/tests/functional/agent/linux/test_ebtables_driver.py b/neutron/tests/functional/agent/linux/test_ebtables_driver.py index 999cc6762..c7728d602 100644 --- a/neutron/tests/functional/agent/linux/test_ebtables_driver.py +++ b/neutron/tests/functional/agent/linux/test_ebtables_driver.py @@ -13,8 +13,8 @@ # License for the specific language governing permissions and limitations # under the License. +from neutron.agent.linux import bridge_lib from neutron.agent.linux import ebtables_driver -from neutron.agent.linux import ip_lib from neutron.tests.common import machine_fixtures from neutron.tests.common import net_helpers from neutron.tests.functional import base @@ -85,14 +85,13 @@ class EbtablesLowLevelTestCase(base.BaseSudoTestCase): # Pick one of the namespaces and setup a bridge for the local ethernet # interface there, because ebtables only works on bridged interfaces. - self.source.execute(['brctl', 'addbr', 'mybridge']) - self.source.execute( - ['brctl', 'addif', 'mybridge', self.source.port.name]) + dev_mybridge = bridge_lib.BridgeDevice.addbr( + 'mybridge', self.source.namespace) + dev_mybridge.addif(self.source.port.name) # Take the IP addrss off one of the interfaces and apply it to the # bridge interface instead. self.source.port.addr.delete(self.source.ip_cidr) - dev_mybridge = ip_lib.IPDevice("mybridge", self.source.namespace) dev_mybridge.link.set_up() dev_mybridge.addr.add(self.source.ip_cidr) diff --git a/neutron/tests/unit/agent/linux/test_bridge_lib.py b/neutron/tests/unit/agent/linux/test_bridge_lib.py index 768c276b2..3b9701d08 100644 --- a/neutron/tests/unit/agent/linux/test_bridge_lib.py +++ b/neutron/tests/unit/agent/linux/test_bridge_lib.py @@ -41,6 +41,12 @@ class BridgeLibTest(base.BaseTestCase): self.assertEqual(namespace, br.namespace) self._verify_bridge_mock(['brctl', 'addbr', self._BR_NAME]) + br.setfd(0) + self._verify_bridge_mock(['brctl', 'setfd', self._BR_NAME, '0']) + + br.disable_stp() + self._verify_bridge_mock(['brctl', 'stp', self._BR_NAME, 'off']) + br.addif(self._IF_NAME) self._verify_bridge_mock( ['brctl', 'addif', self._BR_NAME, self._IF_NAME]) diff --git a/neutron/tests/unit/agent/linux/test_ip_lib.py b/neutron/tests/unit/agent/linux/test_ip_lib.py index f28232cdf..273e33c3a 100644 --- a/neutron/tests/unit/agent/linux/test_ip_lib.py +++ b/neutron/tests/unit/agent/linux/test_ip_lib.py @@ -619,11 +619,13 @@ class TestIpLinkCommand(TestIPCmdBase): self._assert_sudo([], ('set', 'eth0', 'mtu', 1500)) def test_set_up(self): - self.link_cmd.set_up() + observed = self.link_cmd.set_up() + self.assertEqual(self.parent._as_root.return_value, observed) self._assert_sudo([], ('set', 'eth0', 'up')) def test_set_down(self): - self.link_cmd.set_down() + observed = self.link_cmd.set_down() + self.assertEqual(self.parent._as_root.return_value, observed) self._assert_sudo([], ('set', 'eth0', 'down')) def test_set_netns(self): 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 3d5fa6cf4..55b9bd821 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 @@ -17,6 +17,7 @@ import os import mock from oslo_config import cfg +from neutron.agent.linux import bridge_lib from neutron.agent.linux import ip_lib from neutron.agent.linux import utils from neutron.common import constants @@ -577,9 +578,12 @@ class TestLinuxBridgeManager(base.BaseTestCase): self.assertFalse(self.lbm._bridge_exists_and_ensure_up("br0")) def test_ensure_bridge(self): + bridge_device = mock.Mock() + bridge_device_old = mock.Mock() with mock.patch.object(self.lbm, '_bridge_exists_and_ensure_up') as de_fn,\ - mock.patch.object(utils, 'execute') as exec_fn,\ + mock.patch.object(bridge_lib, "BridgeDevice", + return_value=bridge_device_old) as br_fn, \ mock.patch.object(self.lbm, 'update_interface_ip_details') as upd_fn,\ mock.patch.object(self.lbm, @@ -588,7 +592,10 @@ class TestLinuxBridgeManager(base.BaseTestCase): mock.patch.object(self.lbm, 'get_bridge_for_tap_device') as get_if_br_fn: de_fn.return_value = False - exec_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.link.set_up.return_value = False self.assertEqual(self.lbm.ensure_bridge("br0", None), "br0") ie_fn.return_Value = False self.lbm.ensure_bridge("br0", "eth0") @@ -599,24 +606,17 @@ class TestLinuxBridgeManager(base.BaseTestCase): upd_fn.assert_called_with("br0", "eth0", "ips", "gateway") ie_fn.assert_called_with("br0", "eth0") - exec_fn.side_effect = Exception() de_fn.return_value = True + bridge_device.delif.side_effect = Exception() self.lbm.ensure_bridge("br0", "eth0") ie_fn.assert_called_with("br0", "eth0") - exec_fn.reset_mock() - exec_fn.side_effect = None de_fn.return_value = True ie_fn.return_value = False get_if_br_fn.return_value = "br1" self.lbm.ensure_bridge("br0", "eth0") - expected = [ - mock.call(['brctl', 'delif', 'br1', 'eth0'], - run_as_root=True), - mock.call(['brctl', 'addif', 'br0', 'eth0'], - run_as_root=True), - ] - exec_fn.assert_has_calls(expected) + bridge_device_old.delif.assert_called_once_with('eth0') + br_fn.return_value.addif.assert_called_once_with('eth0') def test_ensure_physical_in_bridge(self): self.assertFalse( @@ -653,11 +653,13 @@ class TestLinuxBridgeManager(base.BaseTestCase): ) de_fn.return_value = True + bridge_device = mock.Mock() with mock.patch.object(self.lbm, "ensure_local_bridge") as en_fn,\ - mock.patch.object(utils, "execute") as exec_fn,\ + mock.patch.object(bridge_lib, "BridgeDevice", + return_value=bridge_device), \ mock.patch.object(self.lbm, "get_bridge_for_tap_device") as get_br: - exec_fn.return_value = False + bridge_device.addif.retun_value = False get_br.return_value = True self.assertTrue(self.lbm.add_tap_interface("123", p_const.TYPE_LOCAL, @@ -666,7 +668,7 @@ class TestLinuxBridgeManager(base.BaseTestCase): en_fn.assert_called_with("123") get_br.return_value = False - exec_fn.return_value = True + bridge_device.addif.retun_value = True self.assertFalse(self.lbm.add_tap_interface("123", p_const.TYPE_LOCAL, "physnet1", None, @@ -698,6 +700,7 @@ class TestLinuxBridgeManager(base.BaseTestCase): "1", "tap234") def test_delete_vlan_bridge(self): + bridge_device = mock.Mock() with mock.patch.object(ip_lib, "device_exists") as de_fn,\ mock.patch.object(self.lbm, "get_interfaces_on_bridge") as getif_fn,\ @@ -707,7 +710,8 @@ class TestLinuxBridgeManager(base.BaseTestCase): mock.patch.object(self.lbm, "update_interface_ip_details") as updif_fn,\ mock.patch.object(self.lbm, "delete_vxlan") as del_vxlan,\ - mock.patch.object(utils, "execute") as exec_fn: + mock.patch.object(bridge_lib, "BridgeDevice", + return_value=bridge_device): de_fn.return_value = False self.lbm.delete_vlan_bridge("br0") self.assertFalse(getif_fn.called) @@ -715,12 +719,13 @@ class TestLinuxBridgeManager(base.BaseTestCase): de_fn.return_value = True getif_fn.return_value = ["eth0", "eth1", "vxlan-1002"] if_det_fn.return_value = ("ips", "gateway") - exec_fn.return_value = False + bridge_device.link.set_down.return_value = False self.lbm.delete_vlan_bridge("br0") updif_fn.assert_called_with("eth1", "br0", "ips", "gateway") del_vxlan.assert_called_with("vxlan-1002") def test_delete_vlan_bridge_with_ip(self): + bridge_device = mock.Mock() with mock.patch.object(ip_lib, "device_exists") as de_fn,\ mock.patch.object(self.lbm, "get_interfaces_on_bridge") as getif_fn,\ @@ -730,16 +735,18 @@ class TestLinuxBridgeManager(base.BaseTestCase): mock.patch.object(self.lbm, "update_interface_ip_details") as updif_fn,\ mock.patch.object(self.lbm, "delete_vlan") as del_vlan,\ - mock.patch.object(utils, "execute") as exec_fn: + mock.patch.object(bridge_lib, "BridgeDevice", + return_value=bridge_device): de_fn.return_value = True getif_fn.return_value = ["eth0", "eth1.1"] if_det_fn.return_value = ("ips", "gateway") - exec_fn.return_value = False + bridge_device.link.set_down.return_value = False self.lbm.delete_vlan_bridge("br0") updif_fn.assert_called_with("eth1.1", "br0", "ips", "gateway") self.assertFalse(del_vlan.called) def test_delete_vlan_bridge_no_ip(self): + bridge_device = mock.Mock() with mock.patch.object(ip_lib, "device_exists") as de_fn,\ mock.patch.object(self.lbm, "get_interfaces_on_bridge") as getif_fn,\ @@ -749,10 +756,11 @@ class TestLinuxBridgeManager(base.BaseTestCase): mock.patch.object(self.lbm, "update_interface_ip_details") as updif_fn,\ mock.patch.object(self.lbm, "delete_vlan") as del_vlan,\ - mock.patch.object(utils, "execute") as exec_fn: + mock.patch.object(bridge_lib, "BridgeDevice", + return_value=bridge_device): de_fn.return_value = True getif_fn.return_value = ["eth0", "eth1.1"] - exec_fn.return_value = False + bridge_device.link.set_down.return_value = False if_det_fn.return_value = ([], None) self.lbm.delete_vlan_bridge("br0") del_vlan.assert_called_with("eth1.1") @@ -765,19 +773,21 @@ class TestLinuxBridgeManager(base.BaseTestCase): lbm = linuxbridge_neutron_agent.LinuxBridgeManager( interface_mappings) + bridge_device = mock.Mock() with mock.patch.object(ip_lib, "device_exists") as de_fn,\ mock.patch.object(lbm, "get_interfaces_on_bridge") as getif_fn,\ mock.patch.object(lbm, "remove_interface"),\ mock.patch.object(lbm, "delete_vxlan") as del_vxlan,\ - mock.patch.object(utils, "execute") as exec_fn: + mock.patch.object(bridge_lib, "BridgeDevice", + return_value=bridge_device): de_fn.return_value = False lbm.delete_vlan_bridge("br0") self.assertFalse(getif_fn.called) de_fn.return_value = True getif_fn.return_value = ["vxlan-1002"] - exec_fn.return_value = False + bridge_device.link.set_down.return_value = False lbm.delete_vlan_bridge("br0") del_vxlan.assert_called_with("vxlan-1002") @@ -795,10 +805,12 @@ class TestLinuxBridgeManager(base.BaseTestCase): del_br_fn.assert_called_once_with('brqnet1') def test_remove_interface(self): + bridge_device = mock.Mock() with mock.patch.object(ip_lib, "device_exists") as de_fn,\ mock.patch.object(self.lbm, "is_device_on_bridge") as isdev_fn,\ - mock.patch.object(utils, "execute") as exec_fn: + mock.patch.object(bridge_lib, "BridgeDevice", + return_value=bridge_device): de_fn.return_value = False self.assertFalse(self.lbm.remove_interface("br0", "eth0")) self.assertFalse(isdev_fn.called) @@ -808,10 +820,10 @@ class TestLinuxBridgeManager(base.BaseTestCase): self.assertTrue(self.lbm.remove_interface("br0", "eth0")) isdev_fn.return_value = True - exec_fn.return_value = True + bridge_device.delif.return_value = True self.assertFalse(self.lbm.remove_interface("br0", "eth0")) - exec_fn.return_value = False + bridge_device.delif.return_value = False self.assertTrue(self.lbm.remove_interface("br0", "eth0")) def test_delete_vlan(self): @@ -822,7 +834,7 @@ class TestLinuxBridgeManager(base.BaseTestCase): self.assertFalse(exec_fn.called) de_fn.return_value = True - exec_fn.return_value = False + exec_fn.return_value = True self.lbm.delete_vlan("eth1.1") self.assertTrue(exec_fn.called)