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 2bbc9f2cefaea6c32935f2d1746f04ad27fde4b6..e8176510f8f03fdc3b6911675e46033fcd71036a 100644 (file)
--- 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 36d2b09523b5dc1458a1d582a3d93866d60a64f9..6c6319ded414f060be8c8e74eae4add994bc4ff8 100644 (file)
--- 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 61627ebe35725e77385963fd5da5c41053a8cf90..b9747a4ec04073945d4667c9f86bc91f806a26ab 100644 (file)
--- 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 999cc6762dc8951f219a68388be57869306b29b2..c7728d602995f43e94eb59cdfca12638153989e4 100644 (file)
--- 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 768c276b2983c23b422bc3f1afce03298a8c7ce2..3b9701d08059442552435e75b9890fabb7c75a1e 100644 (file)
--- 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 f28232cdf4c504343b1aaa8cff2dba2d6f483df2..273e33c3a6eec4c27a091e4bdf4b37c0ba87d0df 100644 (file)
--- 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 3d5fa6cf4c40a91ab4d0a90fe00ab3b7003ed13b..55b9bd821b4ce6f0ddb41c6652a5733a8e23cb0d 100644 (file)
--- 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)