Merge pull request #3761 from anarkiwi/tunfix

Fix flap of port with tunnel ACL not handled, fix unnecessary cold start for soft pipeline switches.

clib/mininet_test_base.py

@@ -2228,30 +2228,32 @@ def verify_faucet_reconf(self, timeout=20,
         for i, controller in enumerate(self.faucet_controllers):
             cont_name = controller.name
             start_configure_count = start_configure_counts[i]
-            old_count = old_counts[i]
             for _ in range(timeout):
                 configure_count = self.get_configure_count(controller=cont_name)
                 if configure_count > start_configure_count:
                     break
                 time.sleep(1)
             self.assertNotEqual(
                 start_configure_count, configure_count, 'FAUCET %s did not reconfigure' % cont_name)
-            if change_expected:
-                for _ in range(timeout):
+            if cold_start is not None:
+                old_count = old_counts[i]
+                if change_expected:
+                    old_count = old_counts[i]
+                    for _ in range(timeout):
+                        new_count = int(
+                            self.scrape_prometheus_var(var, controller=cont_name, dpid=dpid, default=0))
+                        if new_count > old_count:
+                            break
+                        time.sleep(1)
+                    self.assertTrue(
+                        new_count > old_count,
+                        msg='FAUCET %s %s did not increment: %u' % (cont_name, var, new_count))
+                else:
                     new_count = int(
                         self.scrape_prometheus_var(var, controller=cont_name, dpid=dpid, default=0))
-                    if new_count > old_count:
-                        break
-                    time.sleep(1)
-                self.assertTrue(
-                    new_count > old_count,
-                    msg='FAUCET %s %s did not increment: %u' % (cont_name, var, new_count))
-            else:
-                new_count = int(
-                    self.scrape_prometheus_var(var, controller=cont_name, dpid=dpid, default=0))
-                self.assertEqual(
-                    old_count, new_count,
-                    msg='FAUCET %s %s incremented: %u' % (cont_name, var, new_count))
+                    self.assertEqual(
+                        old_count, new_count,
+                        msg='FAUCET %s %s incremented: %u' % (cont_name, var, new_count))
             self.wait_for_prometheus_var('faucet_config_applied', 1, controller=cont_name, dpid=None, timeout=30)
             self.wait_dp_status(1, controller=cont_name)
 

faucet/dp.py

@@ -461,6 +461,19 @@ def _generate_acl_tables(self):
         # TODO: dynamically configure output attribute
         return table_config
 
+    def pipeline_str(self):
+        """Text description of pipeline."""
+        table_configs = sorted([
+            (table.table_id, str(table.table_config))
+            for table in self.tables.values()])
+        return '\n'.join([
+            'table ID %u %s' % (table_id, table_config)
+            for table_id, table_config in table_configs])
+
+    def pipeline_tableids(self):
+        """Return pipeline table IDs."""
+        return {table.table_id for table in self.tables.values()}
+
     def _configure_tables(self):
         """Configure FAUCET pipeline with tables."""
         valve_cl = SUPPORTED_HARDWARE.get(self.hardware, None)
@@ -763,7 +776,6 @@ def resolve_stack_topology(self, dps, meta_dp_state):
                 if dp.stack and dp.has_externals:
                     self.has_externals = True
                     break
-        if self.tunnel_acls:
             self.finalize_tunnel_acls(dps)
 
     def finalize_tunnel_acls(self, dps):
@@ -1453,9 +1465,6 @@ def _get_meter_config_changes(self, logger, new_dp):
 
         return (all_meters_changed, deleted_meters, added_meters, changed_meters)
 
-    def _table_configs(self):
-        return frozenset([table.table_config for table in self.tables.values()])
-
     def get_config_changes(self, logger, new_dp):
         """Detect any config changes.
 
@@ -1477,9 +1486,7 @@ def get_config_changes(self, logger, new_dp):
                 added_meters (set): Added meter numbers
                 changed_meters (set): changed/added meter numbers
         """
-        if new_dp._table_configs() != self._table_configs():
-            logger.info('pipeline table config change - requires cold start')
-        elif new_dp.stack and self.stack and new_dp.stack.root_name != self.stack.root_name:
+        if new_dp.stack and self.stack and new_dp.stack.root_name != self.stack.root_name:
             logger.info('Stack root change - requires cold start')
         elif new_dp.routers != self.routers:
             logger.info('DP routers config changed - requires cold start')

faucet/port.py

@@ -491,16 +491,21 @@ def non_stack_forwarding(self):
             return False
         return True
 
+    def tunnel_acls(self):
+        """Return any tunnel ACLs on this port."""
+        acls = []
+        if self.acls_in:
+            acls = [acl for acl in self.acls_in if acl.is_tunnel_acl()]
+        return acls
+
     def contains_tunnel_acl(self, tunnel_id=None):
         """Searches through acls_in for a tunnel ACL with a matching tunnel_id"""
-        if self.acls_in:
-            for acl in self.acls_in:
-                if acl.is_tunnel_acl():
-                    if tunnel_id:
-                        if acl.get_tunnel_rules(tunnel_id):
-                            return True
-                    else:
-                        return True
+        acls = self.tunnel_acls()
+        if tunnel_id is None:
+            return bool(acls)
+        for acl in self.tunnel_acls():
+            if acl.get_tunnel_rules(tunnel_id):
+                return True
         return False
 
     # LACP functions

faucet/valve.py

@@ -18,6 +18,7 @@
 # limitations under the License.
 
 import copy
+import difflib
 import logging
 
 from collections import defaultdict, deque
@@ -109,7 +110,6 @@ class Valve:
         '_last_fast_advertise_sec',
         '_last_lldp_advertise_sec',
         '_last_packet_in_sec',
-        '_last_pipeline_flows',
         '_packet_in_count_sec',
         '_port_highwater',
         '_route_manager_by_eth_type',
@@ -148,7 +148,6 @@ def __init__(self, dp, logname, metrics, notifier, dot1x):
         self.logger = None
         self.recent_ofmsgs = deque(maxlen=32)
         self.stale_root = False
-        self._last_pipeline_flows = []
         self._packet_in_count_sec = None
         self._last_packet_in_sec = None
         self._last_advertise_sec = None
@@ -274,7 +273,7 @@ def dp_init(self, new_dp=None, valves=None):
                 self._route_manager_by_eth_type[eth_type] = route_manager
         self._managers = tuple(
             manager for manager in (
-                self.pipeline, self.switch_manager, self.stack_manager, self.acl_manager,
+                self.pipeline, self.switch_manager, self.acl_manager, self.stack_manager,
                 self._lldp_manager, self._route_manager_by_ipv.get(4),
                 self._route_manager_by_ipv.get(6), self._coprocessor_manager,
                 self._output_only_manager, self._dot1x_manager) if manager is not None)
@@ -637,10 +636,7 @@ def datapath_connect(self, now, discovered_up_ports):
         self.dp.cold_start(now)
         self._inc_var('of_dp_connections')
         self._reset_dp_status()
-        table_configs = sorted([
-            (table.table_id, str(table.table_config)) for table in self.dp.tables.values()])
-        for table_id, table_config in table_configs:
-            self.logger.info('table ID %u %s' % (table_id, table_config))
+        self.logger.info(self.dp.pipeline_str())
         return ofmsgs
 
     def datapath_disconnect(self, now):
@@ -1229,16 +1225,22 @@ def _update_expired_host(self, entry, vlan):
                 'vid': vlan.vid,
                 'eth_src': entry.eth_src}})
 
-    def _pipeline_change(self):
-        def table_msgs(tfm_flow):
-            return {str(x) for x in tfm_flow.body}
+    def _pipeline_diff(self, new_dp):
+        old_pipeline = self.dp.pipeline_str().splitlines()
+        new_pipeline = new_dp.pipeline_str().splitlines()
+        differ = difflib.Differ()
+        diff = '\n'.join(differ.compare(old_pipeline, new_pipeline))
+        self.logger.info('pipeline change: %s' % diff)
 
-        if self._last_pipeline_flows:
-            _last_pipeline_flows = table_msgs(self._last_pipeline_flows[0])
-            _pipeline_flows = table_msgs(self._pipeline_flows()[0])
-            if _last_pipeline_flows != _pipeline_flows:
-                self.logger.info('pipeline change: %s' % str(
-                    _last_pipeline_flows.difference(_pipeline_flows)))
+    def _pipeline_change(self, new_dp):
+        if new_dp:
+            # With OVS/soft pipelines, only a change in allocated tables is significant.
+            if self.dp.hardware != new_dp.hardware:
+                return True
+            old_table_ids = self.dp.pipeline_tableids()
+            new_table_ids = new_dp.pipeline_tableids()
+            if old_table_ids != new_table_ids:
+                self.logger.info('table IDs changed, old %s new %s' % (old_table_ids, new_table_ids))
                 return True
         return False
 
@@ -1272,8 +1274,7 @@ def _apply_config_changes(self, new_dp, changes, valves=None):
         ofmsgs = []
 
         # If pipeline or all ports changed, default to cold start.
-        if self._pipeline_change():
-            self.logger.info('pipeline change')
+        if self._pipeline_change(new_dp):
             self.dp_init(new_dp, valves)
             return restart_type, ofmsgs
 
@@ -1521,9 +1522,18 @@ def _pipeline_flows(self):
                 self.dp, self, self.MAX_TABLE_ID, self.MIN_MAX_FLOWS,
                 self.USE_OXM_IDS, self.FILL_REQ))]
 
+    def _pipeline_change(self, new_dp):
+        if new_dp:
+            old_pipeline = self.dp.pipeline_str()
+            new_pipeline = new_dp.pipeline_str()
+            # TFM based pipelines, any pipeline change is significant.
+            if old_pipeline != new_pipeline:
+                self._pipeline_diff(new_dp)
+                return True
+        return False
+
     def _add_default_flows(self):
         ofmsgs = self._pipeline_flows()
-        self._last_pipeline_flows = copy.deepcopy(ofmsgs)
         ofmsgs.extend(super(TfmValve, self)._add_default_flows())
         return ofmsgs
 

faucet/valve_stack.py

@@ -395,3 +395,9 @@ def add_tunnel_acls(self):
             for acl in self.tunnel_acls:
                 ofmsgs.extend(self.acl_update_tunnel(acl))
         return ofmsgs
+
+    def add_port(self, port):
+        """Need to add tunnel if port comes up with tunnel ACLs."""
+        if not port.stack and port.tunnel_acls():
+            return self.add_tunnel_acls()
+        return []

tests/integration/mininet_multidp_tests.py

@@ -452,7 +452,7 @@ def test_passthrough(self):
         interfaces_conf[end_port]['lacp'] = 2
 
         self.reload_conf(conf, self.faucet_config_path, restart=True,
-                         cold_start=True, change_expected=True)
+                         cold_start=None, change_expected=True)
 
         self.wait_for_all_lacp_up()
         self.verify_stack_hosts()
@@ -2273,7 +2273,7 @@ def test_del_host(self):
         del interfaces_conf[self.host_port_maps[0][0][0]]
         self.reload_conf(
             conf, self.faucet_config_path, restart=True,
-            cold_start=True, change_expected=True, dpid=self.topo.dpids_by_id[0])
+            cold_start=None, change_expected=True, dpid=self.topo.dpids_by_id[0])
         self.verify_stack_up()
         del self.host_information[0]
         self.verify_intervlan_routing()

tests/integration/mininet_tests.py

@@ -3285,7 +3285,7 @@ def test_port_down_flow_gone(self):
             second_host_dst_match, table_id=self._ETH_DST_TABLE)
         self.change_port_config(
             self.port_map['port_4'], None, None,
-            restart=True, cold_start=True)
+            restart=True, cold_start=None)
         self.wait_until_no_matching_flow(
             second_host_dst_match, table_id=self._ETH_DST_TABLE)
 

tests/unit/faucet/test_valve.py

@@ -705,6 +705,86 @@ def test_expiry(self):
             0, self.get_prom('learned_l2_port', labels=learn_labels))
 
 
+class SoftPipelineTestCase(ValveTestBases.ValveTestNetwork):
+    """Test warm starting match changes with soft pipeline."""
+
+    REQUIRE_TFM = False
+
+    CONFIG = """
+acls:
+    acl1:
+        - rule:
+            nw_dst: '224.0.0.5'
+            dl_type: 0x800
+            actions:
+                allow: 0
+    acl2:
+        - rule:
+            nw_dst: '224.0.0.5'
+            dl_type: 0x800
+            ip_proto: 6
+            actions:
+                allow: 0
+dps:
+    s1:
+        hardware: Open vSwitch
+        dp_id: 0x1
+        interfaces:
+            p1:
+                number: 1
+                native_vlan: 100
+                acls_in: [acl1]
+"""
+
+    def setUp(self):
+        self.setup_valves(self.CONFIG)
+
+    def test_soft(self):
+        config = yaml.load(self.CONFIG, Loader=yaml.SafeLoader)
+        config['dps']['s1']['interfaces']['p1']['acls_in'] = ['acl2']
+        # We changed match conditions only, so this can be a warm start.
+        self.update_config(yaml.dump(config), reload_type='warm')
+
+
+class HardPipelineTestCase(ValveTestBases.ValveTestNetwork):
+    """Test cold starting match conditions with hard pipeline."""
+
+    CONFIG = """
+acls:
+    acl1:
+        - rule:
+            nw_dst: '224.0.0.5'
+            dl_type: 0x800
+            actions:
+                allow: 0
+    acl2:
+        - rule:
+            nw_dst: '224.0.0.5'
+            dl_type: 0x800
+            ip_proto: 6
+            actions:
+                allow: 0
+dps:
+    s1:
+        hardware: GenericTFM
+        dp_id: 0x1
+        interfaces:
+            p1:
+                number: 1
+                native_vlan: 100
+                acls_in: [acl1]
+"""
+
+    def setUp(self):
+        self.setup_valves(self.CONFIG)
+
+    def test_hard(self):
+        config = yaml.load(self.CONFIG, Loader=yaml.SafeLoader)
+        config['dps']['s1']['interfaces']['p1']['acls_in'] = ['acl2']
+        # Changed match conditions require restart.
+        self.update_config(yaml.dump(config), reload_type='cold')
+
+
 class ValveMirrorTestCase(ValveTestBases.ValveTestBig):
     """Test ACL and interface mirroring."""
     # TODO: check mirror packets are present/correct

tests/unit/faucet/test_valve_stack.py

@@ -1730,6 +1730,11 @@ def test_update_src_tunnel(self):
             self.DP_ID, self.DP_ID,
             1, 0, 4, self.SRC_ID, True,
             'Did not encapsulate and forward out re-calculated port')
+        self.flap_port(1)
+        self.validate_tunnel(
+            self.DP_ID, self.DP_ID,
+            1, 0, 4, self.SRC_ID, True,
+            'Did not encapsulate and forward after port flap')
 
     def test_update_same_tunnel(self):
         """Test tunnel rules when outputting to host on the same switch as the source"""