Merge pull request #3371 from anarkiwi/reflectlearn

Fix missing VLAN learn rule after warm start, don't learn host on external port if another stack DP has learned it locally.

Author: anarkiwi

faucet/valve.py

@@ -345,31 +345,30 @@ def _add_default_flows(self):
         ofmsgs.extend(self._add_default_drop_flows())
         return ofmsgs
 
-    def _add_vlan(self, vlan):
+    def add_vlan(self, vlan):
         """Configure a VLAN."""
         self.logger.info('Configuring %s' % vlan)
         ofmsgs = []
         for manager in self._get_managers():
             ofmsgs.extend(manager.add_vlan(vlan))
-        vlan.reset_caches()
         return ofmsgs
 
-    def _add_vlans(self, vlans):
+    def add_vlans(self, vlans):
         ofmsgs = []
         for vlan in vlans:
-            ofmsgs.extend(self._add_vlan(vlan))
+            ofmsgs.extend(self.add_vlan(vlan))
         return ofmsgs
 
-    def _del_vlan(self, vlan):
+    def del_vlan(self, vlan):
         """Delete a configured VLAN."""
         self.logger.info('Delete VLAN %s' % vlan)
         table = valve_table.wildcard_table
         return [table.flowdel(match=table.match(vlan=vlan))]
 
-    def _del_vlans(self, vlans):
+    def del_vlans(self, vlans):
         ofmsgs = []
         for vlan in vlans:
-            ofmsgs.extend(self._del_vlan(vlan))
+            ofmsgs.extend(self.del_vlan(vlan))
         return ofmsgs
 
     def _get_all_configured_port_nos(self):
@@ -403,7 +402,7 @@ def _add_ports_and_vlans(self, discovered_up_port_nos):
         ofmsgs = []
         ofmsgs.extend(self.ports_add(
             all_up_port_nos, cold_start=True, log_msg='configured'))
-        ofmsgs.extend(self._add_vlans(self.dp.vlans.values()))
+        ofmsgs.extend(self.add_vlans(self.dp.vlans.values()))
         return ofmsgs
 
     def ofdescstats_handler(self, body):
@@ -612,8 +611,7 @@ def _update_stack_link_state(self, ports, now, other_valves):
                 if not valve.dp.dyn_running:
                     continue
                 ofmsgs_by_valve[valve].extend(valve.get_tunnel_flowmods())
-                for vlan in valve.dp.vlans.values():
-                    ofmsgs_by_valve[valve].extend(valve.flood_manager.add_vlan(vlan))
+                ofmsgs_by_valve[valve].extend(valve.add_vlans(valve.dp.vlans.values()))
                 for port in valve.dp.stack_ports:
                     ofmsgs_by_valve[valve].extend(valve.host_manager.del_port(port))
                 path_port = valve.dp.shortest_path_port(valve.dp.stack_root_name)
@@ -871,8 +869,7 @@ def ports_add(self, port_nums, cold_start=False, log_msg='up'):
 
         # Only update flooding rules if not cold starting.
         if not cold_start:
-            for vlan in vlans_with_ports_added:
-                ofmsgs.extend(self.flood_manager.add_vlan(vlan))
+            ofmsgs.extend(self.add_vlans(vlans_with_ports_added))
         return ofmsgs
 
     def port_add(self, port_num):
@@ -947,8 +944,7 @@ def lacp_down(self, port, cold_start=False, lacp_pkt=None):
         port.lacp_update(False, lacp_pkt=lacp_pkt)
         if not cold_start:
             ofmsgs.extend(self.host_manager.del_port(port))
-            for vlan in port.vlans():
-                ofmsgs.extend(self.flood_manager.add_vlan(vlan))
+            ofmsgs.extend(self.add_vlans(port.vlans()))
         vlan_table = self.dp.tables['vlan']
         ofmsgs.append(vlan_table.flowdrop(
             match=vlan_table.match(in_port=port.number),
@@ -978,8 +974,7 @@ def lacp_up(self, port, now, lacp_pkt):
             ofmsgs.append(vlan_table.flowdel(
                 match=vlan_table.match(in_port=port.number),
                 priority=self.dp.high_priority, strict=True))
-            for vlan in port.vlans():
-                ofmsgs.extend(self.flood_manager.add_vlan(vlan))
+            ofmsgs.extend(self.add_vlans(port.vlans()))
         self._reset_lacp_status(port)
         return ofmsgs
 
@@ -1644,7 +1639,7 @@ def _apply_config_changes(self, new_dp, changes):
             ofmsgs.extend(self.ports_delete(deleted_ports))
         if deleted_vids:
             deleted_vlans = [self.dp.vlans[vid] for vid in deleted_vids]
-            ofmsgs.extend(self._del_vlans(deleted_vlans))
+            ofmsgs.extend(self.del_vlans(deleted_vlans))
         if changed_ports:
             ofmsgs.extend(self.ports_delete(changed_ports))
 
@@ -1653,8 +1648,10 @@ def _apply_config_changes(self, new_dp, changes):
         if changed_vids:
             changed_vlans = [self.dp.vlans[vid] for vid in changed_vids]
             # TODO: handle change versus add separately so can avoid delete first.
-            ofmsgs.extend(self._del_vlans(changed_vlans))
-            ofmsgs.extend(self._add_vlans(changed_vlans))
+            ofmsgs.extend(self.del_vlans(changed_vlans))
+            for vlan in changed_vlans:
+                vlan.reset_caches()
+            ofmsgs.extend(self.add_vlans(changed_vlans))
         if changed_ports:
             ofmsgs.extend(self.ports_add(all_up_port_nos))
         if self.acl_manager and changed_acl_ports:

faucet/valve_flood.py

@@ -375,6 +375,16 @@ def __init__(self, logger, flood_table, pipeline, # pylint: disable=too-many-arg
                 self.external_root_only = True
         self._reset_peer_distances()
 
+    def _build_flood_acts_for_port(self, vlan, exclude_unicast, port,  # pylint: disable=too-many-arguments
+                                   exclude_all_external=False,
+                                   exclude_restricted_bcast_arpnd=False):
+        if self.external_root_only:
+            exclude_all_external = True
+        return super(ValveFloodStackManagerBase, self)._build_flood_acts_for_port(
+            vlan, exclude_unicast, port,
+            exclude_all_external=exclude_all_external,
+            exclude_restricted_bcast_arpnd=exclude_restricted_bcast_arpnd)
+
     def _flood_actions(self, in_port, external_ports,
                        away_flood_actions, toward_flood_actions, local_flood_actions):
         raise NotImplementedError
@@ -521,8 +531,6 @@ def _build_mask_flood_rules(self, vlan, eth_type, eth_dst, eth_dst_mask,  # pyli
                 for ext_port_flag, exclude_all_external in (
                         (valve_of.PCP_NONEXT_PORT_FLAG, True),
                         (valve_of.PCP_EXT_PORT_FLAG, False)):
-                    if self.external_root_only:
-                        exclude_all_external = True
                     if not prune:
                         flood_acts, _, _ = self._build_flood_acts_for_port(
                             vlan, exclude_unicast, port,
@@ -589,12 +597,22 @@ def edge_learn_port(self, other_valves, pkt_meta):
         Returns:
             port to learn host on, or None.
         """
+        # Got a packet from another DP.
         if pkt_meta.port.stack:
             edge_dp = self._edge_dp_for_host(other_valves, pkt_meta)
-            # No edge DP may have learned this host yet.
-            if edge_dp is None:
-                return None
-            return self.shortest_path_port(edge_dp.name)
+            if edge_dp:
+                return self.shortest_path_port(edge_dp.name)
+            # Assuming no DP has learned this host.
+            return None
+
+        # Got a packet locally.
+        # If learning on an external port, check another DP hasn't
+        # already learned on a local/non-external port.
+        if pkt_meta.port.loop_protect_external:
+            edge_dp = self._non_stack_learned(other_valves, pkt_meta)
+            if edge_dp:
+                return self.shortest_path_port(edge_dp.name)
+        # Locally learn.
         return super(ValveFloodStackManagerBase, self).edge_learn_port(
             other_valves, pkt_meta)
 
@@ -609,6 +627,33 @@ def _edge_dp_for_host(self, other_valves, pkt_meta):
         """
         raise NotImplementedError
 
+    def _non_stack_learned(self, other_valves, pkt_meta):
+        other_local_dp_entries = []
+        other_external_dp_entries = []
+        vlan_vid = pkt_meta.vlan.vid
+        for other_valve in other_valves:
+            other_dp_vlan = other_valve.dp.vlans.get(vlan_vid, None)
+            if other_dp_vlan is not None:
+                entry = other_dp_vlan.cached_host(pkt_meta.eth_src)
+                if not entry:
+                    continue
+                if entry.port.stack:
+                    continue
+                if entry.port.loop_protect_external:
+                    other_external_dp_entries.append(other_valve.dp)
+                else:
+                    other_local_dp_entries.append(other_valve.dp)
+        # Another DP has learned locally, has priority.
+        if other_local_dp_entries:
+            return other_local_dp_entries[0]
+        # No other DP has learned locally, but at least one has learned externally.
+        if other_external_dp_entries:
+            entry = pkt_meta.vlan.cached_host(pkt_meta.eth_src)
+            # This DP has not learned the host either, use other's external.
+            if entry is None:
+                return other_external_dp_entries[0]
+        return None
+
 
 class ValveFloodStackManagerNoReflection(ValveFloodStackManagerBase):
     """Stacks of size 2 - all switches directly connected to root.
@@ -635,7 +680,11 @@ def _flood_actions(self, in_port, external_ports,
 
     def _edge_dp_for_host(self, other_valves, pkt_meta):
         """Size 2 means root shortest path is always directly connected."""
-        return pkt_meta.port.stack['dp']
+        peer_dp = pkt_meta.port.stack['dp']
+        if peer_dp.dyn_running:
+            return self._non_stack_learned(other_valves, pkt_meta)
+        # Fall back to assuming peer knows if we are not the peer's controller.
+        return peer_dp
 
 
 class ValveFloodStackManagerReflection(ValveFloodStackManagerBase):
@@ -757,14 +806,12 @@ def _edge_dp_for_host(self, other_valves, pkt_meta):
         # (for example, just default switch to a neighbor).
         # Find port that forwards closer to destination DP that
         # has already learned this host (if any).
-        vlan_vid = pkt_meta.vlan.vid
-        for other_valve in other_valves:
-            other_dp_vlan = other_valve.dp.vlans.get(vlan_vid, None)
-            if other_dp_vlan is not None:
-                entry = other_dp_vlan.cached_host(pkt_meta.eth_src)
-                if entry and not entry.port.stack:
-                    return other_valve.dp
         peer_dp = pkt_meta.port.stack['dp']
-        if peer_dp.is_stack_edge() or peer_dp.is_stack_root():
-            return peer_dp
+        if peer_dp.dyn_running:
+            return self._non_stack_learned(other_valves, pkt_meta)
+        else:
+            # Fall back to peer knows if edge or root if we are not the peer's controller.
+            if peer_dp.is_stack_edge() or peer_dp.is_stack_root():
+                return peer_dp
+        # No DP has learned this host, yet. Take no action to allow remote learning to occur.
         return None

faucet/valve_host.py

@@ -113,13 +113,12 @@ def del_port(self, port):
             vlan.clear_cache_hosts_on_port(port)
         return ofmsgs
 
-    def initialise_tables(self):
+    def add_vlan(self, vlan):
         ofmsgs = []
-        for vlan in self.vlans.values():
-            ofmsgs.append(self.eth_src_table.flowcontroller(
-                match=self.eth_src_table.match(vlan=vlan),
-                priority=self.low_priority,
-                inst=[self.eth_src_table.goto(self.output_table)]))
+        ofmsgs.append(self.eth_src_table.flowcontroller(
+            match=self.eth_src_table.match(vlan=vlan),
+            priority=self.low_priority,
+            inst=[self.eth_src_table.goto(self.output_table)]))
         return ofmsgs
 
     def _temp_ban_host_learning(self, match):

tests/integration/mininet_tests.py

@@ -3063,6 +3063,9 @@ def test_port_change_vlan(self):
         self.change_port_config(
             self.port_map['port_1'], 'native_vlan', 200,
             restart=False, cold_start=False)
+        self.wait_until_matching_flow(
+            {'vlan_vid': 200}, table_id=self._ETH_SRC_TABLE,
+            actions=['OUTPUT:CONTROLLER', 'GOTO_TABLE:%u' % self._ETH_DST_TABLE])
         self.change_port_config(
             self.port_map['port_2'], 'native_vlan', 200,
             restart=True, cold_start=True)
@@ -3087,6 +3090,9 @@ def test_port_change_acl(self):
             {'in_port': int(self.port_map['port_1']),
              'eth_type': IPV4_ETH, 'tcp_dst': 5001, 'ip_proto': 6},
             table_id=self._PORT_ACL_TABLE, cookie=self.ACL_COOKIE)
+        self.wait_until_matching_flow(
+            {'vlan_vid': 100}, table_id=self._ETH_SRC_TABLE,
+            actions=['OUTPUT:CONTROLLER', 'GOTO_TABLE:%u' % self._ETH_DST_TABLE])
         self.verify_tp_dst_blocked(5001, first_host, second_host)
         self.verify_tp_dst_notblocked(5002, first_host, second_host)
         self.reload_conf(
@@ -7029,14 +7035,13 @@ def verify_protected_connectivity(self):
             self.verify_one_broadcast(int_host, ext_hosts)
 
         for ext_host in ext_hosts:
-            # All external hosts cannot flood to each other.
-            for other_ext_host in ext_hosts - {ext_host}:
-                self.verify_broadcast(hosts=(ext_host, other_ext_host), broadcast_expected=False)
-
             # All external hosts can reach internal hosts.
             for int_host in int_hosts:
                 self.verify_broadcast(hosts=(ext_host, int_host), broadcast_expected=True)
                 self.one_ipv4_ping(ext_host, int_host.IP())
+            # All external hosts cannot flood to each other.
+            for other_ext_host in ext_hosts - {ext_host}:
+                self.verify_broadcast(hosts=(ext_host, other_ext_host), broadcast_expected=False)
 
     def set_externals_state(self, dp_name, externals_up):
         """Set the port up/down state of all external ports on a switch"""