openstack/os-vif
Code Issues Proposed changes

only disable mac ageing for ovs hybrid plug

Browse Source 
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
This commit is contained in:
Sean Mooney 2019-07-25 22:16:42 +00:00
parent 719a8fd9bc
commit 655c83d706
7 changed files with 64 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
os_vif/internal/ip/ip_command.py
View File

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

12
os_vif/internal/ip/linux/impl_pyroute2.py
View File

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

26
os_vif/tests/functional/internal/command/ip/test_impl_pyroute2.py
View File

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

9
os_vif/tests/unit/internal/ip/linux/test_impl_pyroute2.py
View File

@ -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,

13
releasenotes/notes/do-not-force-mac-ageing-c6e8d750130c5740.yaml Normal file
View File

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

5
vif_plug_ovs/linux_net.py
View File

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

2
vif_plug_ovs/tests/unit/test_linux_net.py
View File

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