From 655c83d706f5de8a8cf23430782e065219297aef Mon Sep 17 00:00:00 2001 From: Sean Mooney Date: Thu, 25 Jul 2019 22:16:42 +0000 Subject: [PATCH] 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. Change-Id: I95612352de6cdb47de98eb80c208dd1a74499d41 Closes-bug: #1837252 --- os_vif/internal/ip/ip_command.py | 4 ++- os_vif/internal/ip/linux/impl_pyroute2.py | 12 +++++++-- .../internal/command/ip/test_impl_pyroute2.py | 26 +++++++++++++++++-- .../internal/ip/linux/test_impl_pyroute2.py | 9 +++++++ ...not-force-mac-ageing-c6e8d750130c5740.yaml | 13 ++++++++++ vif_plug_ovs/linux_net.py | 5 +++- vif_plug_ovs/tests/unit/test_linux_net.py | 2 +- 7 files changed, 64 insertions(+), 7 deletions(-) create mode 100644 releasenotes/notes/do-not-force-mac-ageing-c6e8d750130c5740.yaml 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")