Merge "remove brctl from vif_plug_ovs"

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
         """
 

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)
 

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,

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'))

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:

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)
 
 
-@privsep.vif_plug.entrypoint
+def _disable_ipv6(bridge):
-def ensure_bridge(bridge):
+    """Disable ipv6 if available for bridge. Must be called from
-    if not ip_lib.exists(bridge):
+       privsep context.
-        processutils.execute('brctl', 'addbr', bridge)
+    """
-        processutils.execute('brctl', 'setfd', bridge, 0)
+    # NOTE(sean-k-mooney): os-vif disables ipv6 to ensure the Bridge
-        processutils.execute('brctl', 'stp', bridge, 'off')
+    # does not aquire an ipv6 auto config or link local adress.
-        processutils.execute('brctl', 'setageing', bridge, 0)
+    # This is required to prevent bug 1302080.
-        syspath = '/sys/class/net/%s/bridge/multicast_snooping'
+    # https://bugs.launchpad.net/neutron/+bug/1302080
-        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',
+        with open(disv6, 'w') as f:
-                                 disv6,
+            f.write('1')
-                                 process_input='1',
+
-                                 check_exit_code=[0, 1])
+
+@privsep.vif_plug.entrypoint
+def ensure_bridge(bridge):
+    if not ip_lib.exists(bridge):
+        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):
+        # Note(sean-k-mooney): this will detach all ports on
-            processutils.execute('brctl', 'delif', bridge, dev)
+        # the bridge before deleting the bridge.
-
+        ip_lib.delete(bridge, check_exit_code=[0, 2, 254])
-        ip_lib.set(bridge, state='down')
+        # howver it will not set the detached interface down
-        processutils.execute('brctl', 'delbr', bridge)
+        # 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

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(linux_net, "set_interface_state")
-    @mock.patch.object(ip_lib, "exists", return_value=True)
+    @mock.patch.object(linux_net, "_disable_ipv6")
-    def test_ensure_bridge_exists(self, mock_dev_exists, mock_ip_set):
+    @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',
+        mock_dev_exists.assert_called_once_with("br0")
-                                            check_exit_code=[0, 2, 254])
+        mock_add.assert_called_once_with("br0", "bridge")
-        mock_dev_exists.assert_has_calls([mock.call("br0")])
+        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(linux_net, "set_interface_state")
-    @mock.patch.object(os.path, "exists", return_value=False)
+    @mock.patch.object(linux_net, "_disable_ipv6")
-    @mock.patch.object(processutils, "execute")
+    @mock.patch.object(ip_lib, "add")
-    @mock.patch.object(ip_lib, "exists", return_value=False)
+    @mock.patch.object(ip_lib, "exists", return_value=True)
-    def test_ensure_bridge_new_ipv4(self, mock_dev_exists, mock_execute,
+    def test_ensure_bridge_exists(self, mock_dev_exists, mock_add,
-                                    mock_path_exists, mock_ip_set):
+                           mock_disable_ipv6, mock_set_state):
         linux_net.ensure_bridge("br0")
 
-        calls = [
+        mock_dev_exists.assert_called_once_with("br0")
-            mock.call('brctl', 'addbr', 'br0'),
+        mock_add.assert_not_called()
-            mock.call('brctl', 'setfd', 'br0', 0),
+        mock_disable_ipv6.assert_called_once_with("br0")
-            mock.call('brctl', 'stp', 'br0', "off"),
+        mock_set_state.assert_called_once_with("br0", "up")
-            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.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)
+    def test_delete_bridge_none(self, mock_dev_exists, mock_delete):
-    @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,):
         linux_net.delete_bridge("br0", "vnet1")
 
-        mock_execute.assert_not_called()
+        mock_delete.assert_not_called()
-        mock_dev_exists.assert_has_calls([mock.call("br0")])
+        mock_dev_exists.assert_called_once_with("br0")
-        mock_interface_br.assert_not_called()
+
+    @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")
+    def test_add_bridge_port(self, mock_set):
-    @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):
         linux_net.add_bridge_port("br0", "vnet1")
-
+        mock_set.assert_called_once_with("vnet1", master="br0")
-        mock_execute.assert_has_calls([
-            mock.call('brctl', 'addif', 'br0', 'vnet1')])
 
     @mock.patch('six.moves.builtins.open')
     @mock.patch.object(os.path, 'isfile')