only disable mac ageing for ovs hybrid plug

The mac ageing configuration on linux bridges is now
conditional and caller controlled. By default mac ageing
is unspecified and will use the kernel's default of 300
seconds. For ovs with hybrid plug we override this to
0 to prevent packet loss issue during some migration
edgecases. This change reverts disabling mac ageing
for the linux bridge plugin which was accidentally
introduced during the brctl removal via inheriting the
ovs plugin's default behavior when the bridge create
code became shared.

Backport Changes:
  In the train cycle we removed the os_vif.internal.command
  module in Id8b71172fb06b435cf169a7e55c11233f22fa65b to eliminate
  one layer of indirection. As a result we need to addtionally
  update the add method in os_vif/internal/command/ip/__init__.py
  which was not required in the train patch.

Change-Id: I95612352de6cdb47de98eb80c208dd1a74499d41
Closes-bug: #1837252
(cherry picked from commit 655c83d)

os_vif/internal/command/ip/__init__.py

@@ -21,10 +21,10 @@ def set(device, check_exit_code=None, state=None, mtu=None, address=None,
 
 
 def add(device, dev_type, check_exit_code=None, peer=None, link=None,
-        vlan_id=None):
+        vlan_id=None, ageing=None):
     """Method to add an interface."""
     return api.ip.add(device, dev_type, check_exit_code=check_exit_code,
-                      peer=peer, link=link, vlan_id=vlan_id)
+                      peer=peer, link=link, vlan_id=vlan_id, ageing=ageing)
 
 
 def delete(device, check_exit_code=None):

os_vif/internal/command/ip/ip_command.py

@@ -39,7 +39,7 @@ def set(self, device, check_exit_code=None, state=None, mtu=None,
 
     @abc.abstractmethod
     def add(self, device, dev_type, check_exit_code=None, peer=None, link=None,
-            vlan_id=None):
+            vlan_id=None, ageing=None):
         """Method to add an interface.
 
         :param   device: A network device (string)
@@ -50,6 +50,8 @@ def add(self, device, dev_type, check_exit_code=None, peer=None, link=None,
         :param   link: String root network interface name, 'device' will be a
                        VLAN tagged virtual interface
         :param   vlan_id: Integer VLAN ID for VLAN devices
+        :param   ageing: integer value in seconds before learned
+                         mac addresses are forgotten.
         :return: status of the command execution
         """
 

os_vif/internal/command/ip/linux/impl_pyroute2.py

@@ -67,7 +67,7 @@ def set(self, device, check_exit_code=None, state=None, mtu=None,
             return self._ip_link(ip, 'set', check_exit_code, **args)
 
     def add(self, device, dev_type, check_exit_code=None, peer=None, link=None,
-            vlan_id=None):
+            vlan_id=None, ageing=None):
         check_exit_code = check_exit_code or []
         with iproute.IPRoute() as ip:
             args = {'ifname': device,
@@ -86,10 +86,18 @@ def add(self, device, dev_type, check_exit_code=None, peer=None, link=None,
                 # 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
+                # NOTE(sean-k-mooney): we conditionally enable mac ageing as
+                # this code is shared between the ovs and linux bridge
+                # plugins. For linux bridge we want to allow the default
+                # ageing of 300 seconds, whereas for ovs with the ip-tables
+                # firewall we want to disable ageing. None was chosen as
+                # the default value of ageing to allow the caller to determine
+                # what policy to use and keep this code generic.
+                if ageing is not None:
+                    args['IFLA_BR_AGEING_TIME'] = ageing
             else:
                 raise exception.NetworkInterfaceTypeNotDefined(type=dev_type)
 

os_vif/tests/functional/internal/command/ip/test_impl_pyroute2.py

@@ -213,6 +213,28 @@ def test_add_bridge(self):
         _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 % "multicast_snooping", "r") as f:
+            self.assertEqual("0", f.readline().rstrip('\n'))
+        with open(base_path % "ageing_time", "r") as f:
+            value = int(f.readline().rstrip('\n'))
+            # NOTE(sean-k-mooney): IEEE 8021-Q recommends that the default
+            # ageing should be 300 and the linux kernel defaults to 300
+            # via an unconditional define. As such we expect this to be
+            # 300 however since services like network-manager could change
+            # the default on bridge creation we check that if it is not 300
+            # then the value should not be 0.
+            self.assertTrue(300 == value or value != 0)
+
+    def test_add_bridge_with_mac_ageing_0(self):
+        device = "test_dev_12"
+        self.addCleanup(self.del_device, device)
+        _ip_cmd_add(device, 'bridge', ageing=0)
+        self.assertTrue(self.exist_device(device))
+        base_path = "/sys/class/net/test_dev_12/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:
@@ -223,8 +245,8 @@ def test_add_bridge(self):
             self.assertEqual("0", f.readline().rstrip('\n'))
 
     def test_add_port_to_bridge(self):
-        device = "test_dev_12"
-        bridge = "test_dev_13"
+        device = "test_dev_13"
+        bridge = "test_dev_14"
         self.addCleanup(self.del_device, device)
         self.addCleanup(self.del_device, bridge)
         self.add_device(device, 'dummy')

os_vif/tests/unit/internal/command/ip/linux/test_impl_pyroute2.py

@@ -95,6 +95,15 @@ def test_add_vlan(self):
 
     def test_add_bridge(self):
         self.ip.add(self.DEVICE, self.TYPE_BRIDGE)
+        args = {'ifname': self.DEVICE,
+                'kind': self.TYPE_BRIDGE,
+                '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_bridge_with_ageing(self):
+        self.ip.add(self.DEVICE, self.TYPE_BRIDGE, ageing=0)
         args = {'ifname': self.DEVICE,
                 'kind': self.TYPE_BRIDGE,
                 'IFLA_BR_AGEING_TIME': 0,

releasenotes/notes/do-not-force-mac-ageing-c6e8d750130c5740.yaml

@@ -0,0 +1,13 @@
+---
+fixes:
+  - |
+    As part of a `bug #1715317`_, MAC ageing was disabled for the intermediate
+    bridge created as part of the hybrid plug mechanism. During the removal
+    of ``brctl``, this behavior was inadvertently applied to all linux bridges
+    created by os-vif including those used in the linuxbridge driver.
+    As a result this can lead to packet flooding (see bug #1837252) when
+    instances are migrated. This behavior has been reverted so that the
+    default mac ageing is determined by the kernel and is not set when using
+    the os-vif linux bridge plugin.
+
+    .. _bug #1715317: https://bugs.launchpad.net/os-vif/+bug/1837252

vif_plug_ovs/linux_net.py

@@ -128,7 +128,10 @@ def _arp_filtering(bridge):
 @privsep.vif_plug.entrypoint
 def ensure_bridge(bridge):
     if not ip_lib.exists(bridge):
-        ip_lib.add(bridge, 'bridge')
+        # NOTE(sean-k-mooney): we set mac ageing to 0 to disable mac ageing
+        # on the hybrid plug bridge to avoid packet loss during live
+        # migration. This avoids bug #1715317 and related bug #1414559
+        ip_lib.add(bridge, 'bridge', ageing=0)
     _disable_ipv6(bridge)
     _arp_filtering(bridge)
     # we bring up the bridge to allow it to switch packets

vif_plug_ovs/tests/unit/test_linux_net.py

@@ -41,7 +41,7 @@ def test_ensure_bridge(self, mock_dev_exists, mock_add,
         linux_net.ensure_bridge("br0")
 
         mock_dev_exists.assert_called_once_with("br0")
-        mock_add.assert_called_once_with("br0", "bridge")
+        mock_add.assert_called_once_with("br0", "bridge", ageing=0)
         mock_disable_ipv6.assert_called_once_with("br0")
         mock_set_state.assert_called_once_with("br0", "up")
         mock_arp_filtering.assert_called_once_with("br0")