Permalink
Browse files

Cleanup stale OVS flows for physical bridges

Perform deletion of the stale flows in physical bridges consistently with
br-int and br-tun, respecting drop_flows_on_start configuration option.
Added tests for auxiliary bridge and functional tests for the physical
bridge using VLAN/flat external network. Fixes part of the bug 1514056;
together with [1] and [2], the bug should be considered fixed.

The commit also fixes inconsistency between netmask of allocated IP
addresses assigned in _create_test_port_dict and ip_len in _plug_ports
of base.py.

Further, this commit sets agent UUID to physical bridges similarly to
tun and int bridges. This is necessary for stale flows cleanup to work
correctly. In upstream, it is treated using OVSBridgeCookieMixin.

[1] https://review.openstack.org/#/c/297211/
[2] https://review.openstack.org/#/c/297818/

Conflicts:
	neutron/tests/functional/agent/l2/base.py
	neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_tunnel.py

Co-Authored-By: Jian Wen <wenjianhn@gmail.com>
Co-Authored-By: Clayton O'Neill <clayton@oneill.net>
Partial-Bug: 1514056
Change-Id: I9801b76829021c9a0e6358982e1136637634a521
(cherry picked from commit cacde30)
  • Loading branch information...
hmlnarik authored and booxter committed Feb 25, 2016
1 parent 260cbe1 commit 8d29f38356fc5b840fa8b5c31fcd9d76c0fdd336
@@ -30,7 +30,6 @@ class OVSPhysicalBridge(ovs_bridge.OVSAgentBridge,
dvr_process_next_table_id = constants.LOCAL_VLAN_TRANSLATION

def setup_default_table(self):
self.delete_flows()
self.install_normal()

@staticmethod
@@ -30,7 +30,6 @@ class OVSPhysicalBridge(ovs_bridge.OVSAgentBridge,
dvr_process_next_table_id = constants.LOCAL_VLAN_TRANSLATION

def setup_default_table(self):
self.delete_flows()
self.install_normal()

def provision_local_vlan(self, port, lvid, segmentation_id, distributed):
@@ -1132,7 +1132,10 @@ def setup_physical_bridges(self, bridge_mappings):
'bridge': bridge})
sys.exit(1)
br = self.br_phys_cls(bridge)
br.set_agent_uuid_stamp(self.agent_uuid_stamp)
br.setup_controllers(self.conf)
if cfg.CONF.AGENT.drop_flows_on_start:
br.delete_flows()
br.setup_default_table()
self.phys_brs[physical_network] = br

@@ -1694,6 +1697,7 @@ def get_port_stats(self, port_info, ancillary_port_info):

def cleanup_stale_flows(self):
bridges = [self.int_br]
bridges.extend(self.phys_brs.values())
if self.enable_tunneling:
bridges.append(self.tun_br)
for bridge in bridges:
@@ -61,6 +61,8 @@ def setUp(self):
prefix='br-int')
self.br_tun = base.get_rand_name(n_const.DEVICE_NAME_MAX_LEN,
prefix='br-tun')
self.br_phys = base.get_rand_name(n_const.DEVICE_NAME_MAX_LEN,
prefix='br-phys')
patch_name_len = n_const.DEVICE_NAME_MAX_LEN - len("-patch-tun")
self.patch_tun = "%s-patch-tun" % self.br_int[patch_name_len:]
self.patch_int = "%s-patch-int" % self.br_tun[patch_name_len:]
@@ -106,7 +108,9 @@ def create_agent(self, create_tunnels=True):
else:
tunnel_types = None
local_ip = '192.168.10.1'
bridge_mappings = {'physnet': self.br_int}
bridge_mappings = {'physnet': self.br_phys}
# Physical bridges should be created prior to running
self._bridge_classes()['br_phys'](self.br_phys).create()
agent = ovs_agent.OVSNeutronAgent(self._bridge_classes(),
self.br_int, self.br_tun,
local_ip, bridge_mappings,
@@ -155,16 +159,20 @@ def _create_test_network_dict(self):
return {'id': uuidutils.generate_uuid(),
'tenant_id': uuidutils.generate_uuid()}

def _plug_ports(self, network, ports, agent, ip_len=24):
def _plug_ports(self, network, ports, agent,
bridge=None, namespace=None):
if namespace is None:
namespace = self.namespace
for port in ports:
bridge = bridge or agent.int_br
self.driver.plug(
network.get('id'), port.get('id'), port.get('vif_name'),
port.get('mac_address'),
agent.int_br.br_name, namespace=self.namespace)
ip_cidrs = ["%s/%s" % (port.get('fixed_ips')[0][
'ip_address'], ip_len)]
bridge.br_name, namespace=namespace)
ip_cidrs = ["%s/8" % (port.get('fixed_ips')[0][
'ip_address'])]
self.driver.init_l3(port.get('vif_name'), ip_cidrs,
namespace=self.namespace)
namespace=namespace)

def _unplug_ports(self, ports, agent):
for port in ports:
@@ -175,9 +183,9 @@ def _get_device_details(self, port, network):
dev = {'device': port['id'],
'port_id': port['id'],
'network_id': network['id'],
'network_type': 'vlan',
'physical_network': 'physnet',
'segmentation_id': 1,
'network_type': network.get('network_type', 'vlan'),
'physical_network': network.get('physical_network', 'physnet'),
'segmentation_id': network.get('segmentation_id', 1),
'fixed_ips': port['fixed_ips'],
'device_owner': 'compute',
'port_security_enabled': True,
@@ -279,11 +287,26 @@ def wait_until_ports_state(self, ports, up, timeout=60):
timeout=timeout)

def setup_agent_and_ports(self, port_dicts, create_tunnels=True,
trigger_resync=False):
trigger_resync=False, network=None):
self.agent = self.create_agent(create_tunnels=create_tunnels)
self.start_agent(self.agent)
self.network = self._create_test_network_dict()
self.network = network or self._create_test_network_dict()
self.ports = port_dicts
if trigger_resync:
self._prepare_resync_trigger(self.agent)
self._plug_ports(self.network, self.ports, self.agent)

def plug_ports_to_phys_br(self, network, ports, namespace=None):
physical_network = network.get('physical_network', 'physnet')
phys_segmentation_id = network.get('segmentation_id', None)
network_type = network.get('network_type', 'flat')

phys_br = self.agent.phys_brs[physical_network]

self._plug_ports(network, ports, self.agent, bridge=phys_br,
namespace=namespace)

if phys_segmentation_id and network_type == 'vlan':
for port in ports:
phys_br.set_db_attribute(
"Port", port['vif_name'], "tag", phys_segmentation_id)
@@ -130,7 +130,54 @@ def test_assert_br_phys_patch_port_ofports_dont_change(self):
self.assertEqual(patch_phys_ofport_before,
self.agent.phys_ofports['physnet'])

def test_assert_pings_during_br_phys_setup_not_lost_in_vlan_to_flat(self):
provider_net = self._create_test_network_dict()
provider_net['network_type'] = 'flat'

self._test_assert_pings_during_br_phys_setup_not_lost(provider_net)

def test_assert_pings_during_br_phys_setup_not_lost_in_vlan_to_vlan(self):
provider_net = self._create_test_network_dict()
provider_net['network_type'] = 'vlan'
provider_net['segmentation_id'] = 876

self._test_assert_pings_during_br_phys_setup_not_lost(provider_net)

def _test_assert_pings_during_br_phys_setup_not_lost(self, provider_net):
# Separate namespace is needed when pinging from one port to another,
# otherwise Linux ping uses loopback instead for sending and receiving
# ping, hence ignoring flow setup.
ns_phys = self.useFixture(net_helpers.NamespaceFixture()).name

ports = self.create_test_ports(amount=2)
port_int = ports[0]
port_phys = ports[1]
ip_int = port_int['fixed_ips'][0]['ip_address']
ip_phys = port_phys['fixed_ips'][0]['ip_address']

self.setup_agent_and_ports(port_dicts=[port_int], create_tunnels=False,
network=provider_net)

self.plug_ports_to_phys_br(provider_net, [port_phys],
namespace=ns_phys)

# The OVS agent doesn't monitor the physical bridges, no notification
# is sent when a port is up on a physical bridge, hence waiting only
# for the ports connected to br-int
self.wait_until_ports_state([port_int], up=True)

with net_helpers.async_ping(ns_phys, [ip_int]) as done:
while not done():
self.agent.setup_physical_bridges(self.agent.bridge_mappings)
time.sleep(0.25)

with net_helpers.async_ping(self.namespace, [ip_phys]) as done:
while not done():
self.agent.setup_physical_bridges(self.agent.bridge_mappings)
time.sleep(0.25)

def test_noresync_after_port_gone(self):

'''This will test the scenario where a port is removed after listing
it but before getting vif info about it.
'''
@@ -38,7 +38,6 @@ def test_setup_default_table(self):
self.br.setup_default_table()
(dp, ofp, ofpp) = self._get_dp()
expected = [
call.delete_flows(),
call._send_msg(ofpp.OFPFlowMod(dp,
cookie=0,
instructions=[
@@ -37,7 +37,6 @@ def setUp(self):
def test_setup_default_table(self):
self.br.setup_default_table()
expected = [
call.delete_flows(),
call.add_flow(priority=0, table=0, actions='normal'),
]
self.assertEqual(expected, self.mock.mock_calls)
@@ -880,6 +880,7 @@ def test_setup_physical_bridges(self):
self.agent.setup_physical_bridges({"physnet1": "br-eth"})
expected_calls = [
mock.call.phys_br_cls('br-eth'),
mock.call.phys_br.set_agent_uuid_stamp(mock.ANY),
mock.call.phys_br.setup_controllers(mock.ANY),
mock.call.phys_br.setup_default_table(),
mock.call.int_br.db_get_val('Interface', 'int-br-eth',
@@ -964,6 +965,7 @@ def test_setup_physical_bridges_change_from_veth_to_patch_conf(self):
self.agent.setup_physical_bridges({"physnet1": "br-eth"})
expected_calls = [
mock.call.phys_br_cls('br-eth'),
mock.call.phys_br.set_agent_uuid_stamp(mock.ANY),
mock.call.phys_br.setup_controllers(mock.ANY),
mock.call.phys_br.setup_default_table(),
mock.call.int_br.delete_port('int-br-eth'),
@@ -81,6 +81,7 @@ def setUp(self):
self.INT_BRIDGE = 'integration_bridge'
self.TUN_BRIDGE = 'tunnel_bridge'
self.MAP_TUN_BRIDGE = 'tun_br_map'
self.AUX_BRIDGE = 'ancillary_bridge'
self.NET_MAPPING = {'net1': self.MAP_TUN_BRIDGE}
self.INT_OFPORT = 11111
self.TUN_OFPORT = 22222
@@ -104,6 +105,8 @@ def setUp(self):
self.br_tun_cls('br-tun')),
self.MAP_TUN_BRIDGE: mock.create_autospec(
self.br_phys_cls('br-phys')),
self.AUX_BRIDGE: mock.create_autospec(
ovs_lib.OVSBridge('br-aux')),
}
self.ovs_int_ofports = {
'patch-tun': self.TUN_OFPORT,
@@ -122,8 +125,13 @@ def lookup_br(br_name, *args, **kwargs):
self.mock_tun_bridge_cls = mock.patch(self._BR_TUN_CLASS,
autospec=True).start()
self.mock_tun_bridge_cls.side_effect = lookup_br
self.mock_aux_bridge_cls = mock.patch(
'neutron.agent.common.ovs_lib.OVSBridge',
autospec=True).start()
self.mock_aux_bridge_cls.side_effect = lookup_br

self.mock_int_bridge = self.ovs_bridges[self.INT_BRIDGE]
self.mock_int_bridge.br_name = self.INT_BRIDGE
self.mock_int_bridge.add_port.return_value = self.MAP_TUN_INT_OFPORT
self.mock_int_bridge.add_patch_port.side_effect = (
lambda tap, peer: self.ovs_int_ofports[tap])
@@ -143,6 +151,7 @@ def lookup_br(br_name, *args, **kwargs):
ovs_lib.INVALID_OFPORT)

self.mock_tun_bridge = self.ovs_bridges[self.TUN_BRIDGE]
self.mock_tun_bridge.br_name = self.TUN_BRIDGE
self.mock_tun_bridge.add_port.return_value = self.INT_OFPORT
self.mock_tun_bridge.add_patch_port.return_value = self.INT_OFPORT

@@ -159,7 +168,13 @@ def lookup_br(br_name, *args, **kwargs):
'get_bridges').start()
self.get_bridges.return_value = [self.INT_BRIDGE,
self.TUN_BRIDGE,
self.MAP_TUN_BRIDGE]
self.MAP_TUN_BRIDGE,
self.AUX_BRIDGE]
self.get_bridge_external_bridge_id = mock.patch.object(
ovs_lib.BaseOVS,
'get_bridge_external_bridge_id').start()
self.get_bridge_external_bridge_id.side_effect = (
lambda bridge: bridge if bridge in self.ovs_bridges else None)

self.execute = mock.patch('neutron.agent.common.utils.execute').start()

@@ -189,6 +204,7 @@ def _define_expected_calls(self, arp_responder=False):
]

self.mock_map_tun_bridge_expected = [
mock.call.set_agent_uuid_stamp(mock.ANY),
mock.call.setup_controllers(mock.ANY),
mock.call.setup_default_table(),
mock.call.get_port_ofport('phy-%s' % self.MAP_TUN_BRIDGE),
@@ -216,6 +232,10 @@ def _define_expected_calls(self, arp_responder=False):
'options', {'peer': 'int-%s' % self.MAP_TUN_BRIDGE}),
]

self.mock_aux_bridge = self.ovs_bridges[self.AUX_BRIDGE]
self.mock_aux_bridge_expected = [
]

self.mock_tun_bridge_expected = [
mock.call.set_agent_uuid_stamp(mock.ANY),
mock.call.bridge_exists(mock.ANY),
@@ -286,6 +306,8 @@ def _verify_mock_calls(self):
self.mock_map_tun_bridge_expected)
self._verify_mock_call(self.mock_tun_bridge,
self.mock_tun_bridge_expected)
self._verify_mock_call(self.mock_aux_bridge,
self.mock_aux_bridge_expected)
self._verify_mock_call(self.device_exists, self.device_exists_expected)
self._verify_mock_call(self.ipdevice, self.ipdevice_expected)
self._verify_mock_call(self.ipwrapper, self.ipwrapper_expected)
@@ -517,33 +539,48 @@ def test_daemon_loop(self):

self.mock_int_bridge_expected += [
mock.call.check_canary_table(),
mock.call.cleanup_flows(),
mock.call.check_canary_table()
]
self.mock_tun_bridge_expected += [
mock.call.cleanup_flows()
]
self.mock_map_tun_bridge_expected += [
mock.call.cleanup_flows()
]
# No cleanup is expected on ancillary bridge

self.ovs_bridges[self.INT_BRIDGE].check_canary_table.return_value = \
constants.OVS_NORMAL
with mock.patch.object(log.KeywordArgumentAdapter,
'exception') as log_exception,\
mock.patch.object(self.mod_agent.OVSNeutronAgent,
'scan_ports') as scan_ports,\
mock.patch.object(
self.mod_agent.OVSNeutronAgent,
'scan_ancillary_ports') as scan_ancillary_ports,\
mock.patch.object(
self.mod_agent.OVSNeutronAgent,
'process_network_ports') as process_network_ports,\
mock.patch.object(
self.mod_agent.OVSNeutronAgent,
'process_ancillary_network_ports') as process_anc_ports,\
mock.patch.object(self.mod_agent.OVSNeutronAgent,
'tunnel_sync'),\
mock.patch.object(time, 'sleep'),\
mock.patch.object(
self.mod_agent.OVSNeutronAgent,
'update_stale_ofport_rules') as update_stale,\
mock.patch.object(
self.mod_agent.OVSNeutronAgent,
'cleanup_stale_flows') as cleanup:
'update_stale_ofport_rules') as update_stale:
log_exception.side_effect = Exception(
'Fake exception to get out of the loop')
scan_ports.side_effect = [reply2, reply3]
scan_ancillary_ports.return_value = {
'current': set([]), 'added': set([]), 'removed': set([]),
}
update_stale.return_value = []
process_network_ports.side_effect = [
False, Exception('Fake exception to get out of the loop')]
process_anc_ports.return_value = False

n_agent = self._build_agent()

@@ -572,7 +609,6 @@ def test_daemon_loop(self):
'added': set([])}, False)
])

cleanup.assert_called_once_with()
self.assertTrue(update_stale.called)
self._verify_mock_calls()

@@ -607,10 +643,12 @@ def _define_expected_calls(self, arp_responder=False):
mock.call.create(),
mock.call.set_secure_mode(),
mock.call.setup_controllers(mock.ANY),

mock.call.setup_default_table(),
]

self.mock_map_tun_bridge_expected = [
mock.call.set_agent_uuid_stamp(mock.ANY),
mock.call.setup_controllers(mock.ANY),
mock.call.setup_default_table(),
mock.call.add_port(self.intb),
@@ -628,6 +666,10 @@ def _define_expected_calls(self, arp_responder=False):
mock.call.drop_port(in_port=self.MAP_TUN_PHY_OFPORT),
]

self.mock_aux_bridge = self.ovs_bridges[self.AUX_BRIDGE]
self.mock_aux_bridge_expected = [
]

self.mock_tun_bridge_expected = [
mock.call.set_agent_uuid_stamp(mock.ANY),
mock.call.bridge_exists(mock.ANY),

0 comments on commit 8d29f38

Please sign in to comment.