From 8d29f38356fc5b840fa8b5c31fcd9d76c0fdd336 Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Thu, 25 Feb 2016 11:34:15 +0100 Subject: [PATCH] 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 Co-Authored-By: Clayton O'Neill Partial-Bug: 1514056 Change-Id: I9801b76829021c9a0e6358982e1136637634a521 (cherry picked from commit cacde308eef6f1d7005e555b4521332da95d3cf4) --- .../agent/openflow/native/br_phys.py | 1 - .../agent/openflow/ovs_ofctl/br_phys.py | 1 - .../openvswitch/agent/ovs_neutron_agent.py | 4 ++ neutron/tests/functional/agent/l2/base.py | 45 ++++++++++++---- .../functional/agent/test_l2_ovs_agent.py | 47 ++++++++++++++++ .../agent/openflow/native/test_br_phys.py | 1 - .../agent/openflow/ovs_ofctl/test_br_phys.py | 1 - .../agent/test_ovs_neutron_agent.py | 2 + .../openvswitch/agent/test_ovs_tunnel.py | 54 ++++++++++++++++--- 9 files changed, 135 insertions(+), 21 deletions(-) diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/br_phys.py b/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/br_phys.py index a3aad0fc7ed..9b5d5754760 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/br_phys.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/br_phys.py @@ -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 diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/ovs_ofctl/br_phys.py b/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/ovs_ofctl/br_phys.py index e76b7ddaf7f..ff9d87e5405 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/ovs_ofctl/br_phys.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/ovs_ofctl/br_phys.py @@ -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): diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py index 2a1f3498a38..c1a8917a31a 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py @@ -1132,7 +1132,10 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, '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 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, 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: diff --git a/neutron/tests/functional/agent/l2/base.py b/neutron/tests/functional/agent/l2/base.py index 39e511b11df..672b3122ab9 100644 --- a/neutron/tests/functional/agent/l2/base.py +++ b/neutron/tests/functional/agent/l2/base.py @@ -61,6 +61,8 @@ class OVSAgentTestFramework(base.BaseOVSLinuxTestCase): 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 @@ class OVSAgentTestFramework(base.BaseOVSLinuxTestCase): 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 @@ class OVSAgentTestFramework(base.BaseOVSLinuxTestCase): 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 @@ class OVSAgentTestFramework(base.BaseOVSLinuxTestCase): 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 @@ class OVSAgentTestFramework(base.BaseOVSLinuxTestCase): 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) diff --git a/neutron/tests/functional/agent/test_l2_ovs_agent.py b/neutron/tests/functional/agent/test_l2_ovs_agent.py index bf8682bd08b..92e85247d37 100644 --- a/neutron/tests/functional/agent/test_l2_ovs_agent.py +++ b/neutron/tests/functional/agent/test_l2_ovs_agent.py @@ -130,7 +130,54 @@ class TestOVSAgent(base.OVSAgentTestFramework): 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. ''' diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/native/test_br_phys.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/native/test_br_phys.py index a478f9a8b61..9ddb72987e6 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/native/test_br_phys.py +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/native/test_br_phys.py @@ -38,7 +38,6 @@ class OVSPhysicalBridgeTest(ovs_bridge_test_base.OVSBridgeTestBase, self.br.setup_default_table() (dp, ofp, ofpp) = self._get_dp() expected = [ - call.delete_flows(), call._send_msg(ofpp.OFPFlowMod(dp, cookie=0, instructions=[ diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/ovs_ofctl/test_br_phys.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/ovs_ofctl/test_br_phys.py index 47ca3177dc9..ab372977bff 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/ovs_ofctl/test_br_phys.py +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/ovs_ofctl/test_br_phys.py @@ -37,7 +37,6 @@ class OVSPhysicalBridgeTest(ovs_bridge_test_base.OVSBridgeTestBase, 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) diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py index 2076c41fd52..4e881f56e18 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py @@ -880,6 +880,7 @@ class TestOvsNeutronAgent(object): 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 @@ class TestOvsNeutronAgent(object): 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'), diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_tunnel.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_tunnel.py index 5c6f749e34a..cd1d03527a5 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_tunnel.py +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_tunnel.py @@ -81,6 +81,7 @@ class TunnelTest(object): 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 @@ class TunnelTest(object): 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 @@ class TunnelTest(object): 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 @@ class TunnelTest(object): 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 @@ class TunnelTest(object): '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 @@ class TunnelTest(object): ] 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 @@ class TunnelTest(object): '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 @@ class TunnelTest(object): 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,8 +539,16 @@ class TunnelTest(object): 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 @@ -526,24 +556,31 @@ class TunnelTest(object): '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 @@ class TunnelTest(object): 'added': set([])}, False) ]) - cleanup.assert_called_once_with() self.assertTrue(update_stale.called) self._verify_mock_calls() @@ -607,10 +643,12 @@ class TunnelTestUseVethInterco(TunnelTest): 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 @@ class TunnelTestUseVethInterco(TunnelTest): 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),