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

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

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

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:

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)

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.
         '''

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=[

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)

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

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