diff --git a/os_vif/internal/ip/ip_command.py b/os_vif/internal/ip/ip_command.py index 01ba5f9c..d7a1dbfe 100644 --- a/os_vif/internal/ip/ip_command.py +++ b/os_vif/internal/ip/ip_command.py @@ -39,7 +39,7 @@ class IpCommand(object): @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 @@ class IpCommand(object): :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 """ diff --git a/os_vif/internal/ip/linux/impl_pyroute2.py b/os_vif/internal/ip/linux/impl_pyroute2.py index 2745223c..da9e849c 100644 --- a/os_vif/internal/ip/linux/impl_pyroute2.py +++ b/os_vif/internal/ip/linux/impl_pyroute2.py @@ -67,7 +67,7 @@ class PyRoute2(ip_command.IpCommand): 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 @@ class PyRoute2(ip_command.IpCommand): # 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) diff --git a/os_vif/tests/functional/internal/command/ip/test_impl_pyroute2.py b/os_vif/tests/functional/internal/command/ip/test_impl_pyroute2.py index 5cf6e3fb..fdefc421 100644 --- a/os_vif/tests/functional/internal/command/ip/test_impl_pyroute2.py +++ b/os_vif/tests/functional/internal/command/ip/test_impl_pyroute2.py @@ -213,6 +213,28 @@ class TestIpCommand(ShellIpCommands, base.BaseFunctionalTestCase): _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 @@ class TestIpCommand(ShellIpCommands, base.BaseFunctionalTestCase): 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') diff --git a/os_vif/tests/unit/internal/ip/linux/test_impl_pyroute2.py b/os_vif/tests/unit/internal/ip/linux/test_impl_pyroute2.py index 9066c36e..603afaca 100644 --- a/os_vif/tests/unit/internal/ip/linux/test_impl_pyroute2.py +++ b/os_vif/tests/unit/internal/ip/linux/test_impl_pyroute2.py @@ -95,6 +95,15 @@ class TestIpCommand(base.TestCase): 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, diff --git a/releasenotes/notes/do-not-force-mac-ageing-c6e8d750130c5740.yaml b/releasenotes/notes/do-not-force-mac-ageing-c6e8d750130c5740.yaml new file mode 100644 index 00000000..149f22dc --- /dev/null +++ b/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 \ No newline at end of file diff --git a/vif_plug_ovs/linux_net.py b/vif_plug_ovs/linux_net.py index e83fcdd5..20baab31 100644 --- a/vif_plug_ovs/linux_net.py +++ b/vif_plug_ovs/linux_net.py @@ -129,7 +129,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 diff --git a/vif_plug_ovs/tests/unit/test_linux_net.py b/vif_plug_ovs/tests/unit/test_linux_net.py index c4227831..4eaa1f73 100644 --- a/vif_plug_ovs/tests/unit/test_linux_net.py +++ b/vif_plug_ovs/tests/unit/test_linux_net.py @@ -41,7 +41,7 @@ class LinuxNetTest(testtools.TestCase): 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")