Repository: faucetsdn/faucet
Issue: When multi DP routing on stacking, FAUCET router can't resolve hosts on non-root DPs (because route resolver doesn't flood to stack ports)

See also.

https://github.com/faucetsdn/faucet/pull/3226

Changed files:
faucet/valve_flood.py, tests/unit/faucet/test_valve_stack.py, faucet/valve_route.py, faucet/valve.py

diff --git a/faucet/valve.py b/faucet/valve.py
index 9b3fae66..8b979097 100644
--- a/faucet/valve.py
+++ b/faucet/valve.py
@@ -170,29 +170,6 @@ class Valve:
             self._port_highwater[vlan_vid] = {}
             for port_number in self.dp.ports.keys():
                 self._port_highwater[vlan_vid][port_number] = 0
-        for ipv, route_manager_class, neighbor_timeout in (
-                (4, valve_route.ValveIPv4RouteManager, self.dp.arp_neighbor_timeout),
-                (6, valve_route.ValveIPv6RouteManager, self.dp.nd_neighbor_timeout)):
-            fib_table_name = 'ipv%u_fib' % ipv
-            if not fib_table_name in self.dp.tables:
-                continue
-            fib_table = self.dp.tables[fib_table_name]
-            proactive_learn = getattr(self.dp, 'proactive_learn_v%u' % ipv)
-            route_manager = route_manager_class(
-                self.logger, self.notify, self.dp.global_vlan, neighbor_timeout,
-                self.dp.max_hosts_per_resolve_cycle,
-                self.dp.max_host_fib_retry_count,
-                self.dp.max_resolve_backoff_time, proactive_learn,
-                self.DEC_TTL, self.dp.multi_out, fib_table,
-                self.dp.tables['vip'], self.pipeline, self.dp.routers)
-            self._route_manager_by_ipv[route_manager.IPV] = route_manager
-            for vlan in self.dp.vlans.values():
-                if vlan.faucet_vips_by_ipv(route_manager.IPV):
-                    route_manager.active = True
-                    self.logger.info('IPv%u routing is active on %s with VIPs %s' % (
-                        route_manager.IPV, vlan, vlan.faucet_vips_by_ipv(route_manager.IPV)))
-            for eth_type in route_manager.CONTROL_ETH_TYPES:
-                self._route_manager_by_eth_type[eth_type] = route_manager
         restricted_bcast_arpnd = bool(self.dp.restricted_bcast_arpnd_ports())
         if self.dp.stack:
             flood_class = valve_flood.ValveFloodStackManagerNoReflection
@@ -216,6 +193,32 @@ class Valve:
                 self.dp.group_table, self.dp.groups,
                 self.dp.combinatorial_port_flood, self.dp.canonical_port_order,
                 restricted_bcast_arpnd)
+        for ipv, route_manager_class, neighbor_timeout in (
+                (4, valve_route.ValveIPv4RouteManager, self.dp.arp_neighbor_timeout),
+                (6, valve_route.ValveIPv6RouteManager, self.dp.nd_neighbor_timeout)):
+            fib_table_name = 'ipv%u_fib' % ipv
+            if fib_table_name not in self.dp.tables:
+                continue
+            fib_table = self.dp.tables[fib_table_name]
+            proactive_learn = getattr(self.dp, 'proactive_learn_v%u' % ipv)
+            valve_flood_manager = None
+            if self.dp.stack:
+                valve_flood_manager = self.flood_manager
+            route_manager = route_manager_class(
+                self.logger, self.notify, self.dp.global_vlan, neighbor_timeout,
+                self.dp.max_hosts_per_resolve_cycle,
+                self.dp.max_host_fib_retry_count,
+                self.dp.max_resolve_backoff_time, proactive_learn,
+                self.DEC_TTL, self.dp.multi_out, fib_table,
+                self.dp.tables['vip'], self.pipeline, self.dp.routers, valve_flood_manager)
+            self._route_manager_by_ipv[route_manager.IPV] = route_manager
+            for vlan in self.dp.vlans.values():
+                if vlan.faucet_vips_by_ipv(route_manager.IPV):
+                    route_manager.active = True
+                    self.logger.info('IPv%u routing is active on %s with VIPs %s' % (
+                        route_manager.IPV, vlan, vlan.faucet_vips_by_ipv(route_manager.IPV)))
+            for eth_type in route_manager.CONTROL_ETH_TYPES:
+                self._route_manager_by_eth_type[eth_type] = route_manager
         eth_dst_hairpin_table = self.dp.tables.get('eth_dst_hairpin', None)
         host_manager_cl = valve_host.ValveHostManager
         if self.dp.use_idle_timeout:
diff --git a/faucet/valve_flood.py b/faucet/valve_flood.py
index 0944c7a3..3b6b5b30 100644
--- a/faucet/valve_flood.py
+++ b/faucet/valve_flood.py
@@ -651,6 +651,59 @@ class ValveFloodStackManagerBase(ValveFloodManager):
                 return other_external_dp_entries[0]
         return None
 
+    def _stack_flood_ports(self):
+        """Return output ports of a DP that have been pruned and follow reflection rules"""
+        # TODO: Consolidate stack port selection logic,
+        #           this reuses logic from _build_mask_flood_rules()
+        away_flood_ports = []
+        towards_flood_ports = []
+        # Obtain away ports
+        away_up_ports_by_dp = defaultdict(list)
+        for port in self._canonical_stack_up_ports(self.away_from_root_stack_ports):
+            away_up_ports_by_dp[port.stack['dp']].append(port)
+        # Obtain the towards root path port (this is the designated root port)
+        towards_up_port = None
+        towards_up_ports = self._canonical_stack_up_ports(self.towards_root_stack_ports)
+        if towards_up_ports:
+            towards_up_port = towards_up_ports[0]
+        # Figure out what stack ports will need to be flooded
+        for port in self.stack_ports:
+            remote_dp = port.stack['dp']
+            away_up_port = None
+            away_up_ports = away_up_ports_by_dp.get(remote_dp, None)
+            if away_up_ports:
+                # Pick the lowest port number on the remote DP.
+                remote_away_ports = self.canonical_port_order(
+                    [away_port.stack['port'] for away_port in away_up_ports])
+                away_up_port = remote_away_ports[0].stack['port']
+            # Is the port to an away DP, (away from the stack root)
+            away_port = port in self.away_from_root_stack_ports
+            # Otherwise it is towards the stack root
+            towards_port = not away_port
+
+            # Prune == True for ports that do not need to be flooded
+            if towards_port:
+                # If towards the stack root, then if the port is not the chosen
+                #   root path port, then we do not need to flood to it
+                prune = port != towards_up_port
+                if not prune and not self.is_stack_root():
+                    # Port is chosen towards port and not the root so flood
+                    #   towards the root
+                    towards_flood_ports.append(port)
+            else:
+                # If away from stack root, then if the port is not the chosen
+                #   away port for that DP, we do not need to flood to it
+                prune = port != away_up_port
+                if not prune and self.is_stack_root():
+                    # Port is chosen away port and the root switch
+                    #   so flood away from the root
+                    away_flood_ports.append(port)
+
+        # Also need to turn off inactive away ports (for DPs that have a better way to get to root)
+        exclude_ports = self._inactive_away_stack_ports()
+        away_flood_ports = [port for port in away_flood_ports if port not in exclude_ports]
+        return towards_flood_ports + away_flood_ports
+
 
 class ValveFloodStackManagerNoReflection(ValveFloodStackManagerBase):
     """Stacks of size 2 - all switches directly connected to root.
diff --git a/faucet/valve_route.py b/faucet/valve_route.py
index 31d902ee..95d8ca14 100644
--- a/faucet/valve_route.py
+++ b/faucet/valve_route.py
@@ -110,6 +110,7 @@ class ValveRouteManager(ValveManagerBase):
         'route_priority',
         'routers',
         'vip_table',
+        'flood_manager',
     ]
 
     IPV = 0
@@ -123,7 +124,7 @@ class ValveRouteManager(ValveManagerBase):
     def __init__(self, logger, notify, global_vlan, neighbor_timeout,
                  max_hosts_per_resolve_cycle, max_host_fib_retry_count,
                  max_resolve_backoff_time, proactive_learn, dec_ttl, multi_out,
-                 fib_table, vip_table, pipeline, routers):
+                 fib_table, vip_table, pipeline, routers, flood_manager):
         self.notify = notify
         self.logger = logger
         self.global_vlan = AnonVLAN(global_vlan)
@@ -141,6 +142,7 @@ class ValveRouteManager(ValveManagerBase):
         self.routers = routers
         self.active = False
         self.global_routing = self._global_routing()
+        self.flood_manager = flood_manager
         if self.global_routing:
             self.logger.info('global routing enabled')
 
@@ -170,12 +172,37 @@ class ValveRouteManager(ValveManagerBase):
     def _gw_respond_pkt():
         return None
 
+    def _flood_stack_links(self, pkt_builder, vlan, multi_out=True, *args):
+        """Return flood packet-out actions to stack ports for gw resolving"""
+        ofmsgs = []
+        if self.flood_manager:
+            ports = self.flood_manager._stack_flood_ports()
+            if ports:
+                running_port_nos = [port.number for port in ports if port.running()]
+                pkt = pkt_builder(vlan.vid, *args)
+                if running_port_nos:
+                    random.shuffle(running_port_nos)
+                    if multi_out:
+                        ofmsgs.append(valve_of.packetouts(running_port_nos, pkt.data))
+                    else:
+                        ofmsgs.extend(
+                            [valve_of.packetout(port_no, pkt.data) for port_no in running_port_nos])
+        return ofmsgs
+
     def _resolve_gw_on_vlan(self, vlan, faucet_vip, ip_gw):
         """Return flood packet-out actions for gw resolving"""
-        # TODO: in multi DP routing, need to flood out stack ports as well
-        return vlan.flood_pkt(
+        ofmsgs = []
+        stack_ofmsgs = self._flood_stack_links(
+            self._gw_resolve_pkt(), vlan, self.multi_out,
+            vlan.faucet_mac, valve_of.mac.BROADCAST_STR, faucet_vip.ip, ip_gw)
+        if stack_ofmsgs:
+            ofmsgs.extend(stack_ofmsgs)
+        vlan_ofmsgs = vlan.flood_pkt(
             self._gw_resolve_pkt(), self.multi_out,
             vlan.faucet_mac, valve_of.mac.BROADCAST_STR, faucet_vip.ip, ip_gw)
+        if vlan_ofmsgs:
+            ofmsgs.extend(vlan_ofmsgs)
+        return ofmsgs
 
     def _resolve_gw_on_port(self, vlan, port, faucet_vip, ip_gw, eth_dst):
         """Return packet-out actions for outputting to a specific port"""
diff --git a/tests/unit/faucet/test_valve_stack.py b/tests/unit/faucet/test_valve_stack.py
index 35901d08..29a4ce98 100755
--- a/tests/unit/faucet/test_valve_stack.py
+++ b/tests/unit/faucet/test_valve_stack.py
@@ -19,6 +19,7 @@
 
 from functools import partial
 import unittest
+import ipaddress
 import yaml
 
 from ryu.lib import mac
@@ -88,6 +89,10 @@ dps:
         """Setup basic loop config"""
         self.setup_valve(self.CONFIG)
 
+    def get_other_valves(self, valve):
+        """Return other running valves"""
+        return self.valves_manager._other_running_valves(valve)  # pylint: disable=protected-access
+
     def test_dpid_nominations(self):
         """Test dpids are nominated correctly"""
         self.activate_all_ports()
@@ -99,7 +104,7 @@ dps:
                     lacp_ports[valve.dp.dp_id].append(port)
                     port.actor_up()
         valve = self.valves_manager.valves[0x1]
-        other_valves = self.valves_manager._other_running_valves(valve)
+        other_valves = self.get_other_valves(valve)
         # Equal number of LAG ports, choose root DP
         nominated_dpid = valve.get_lacp_dpid_nomination(1, other_valves)[0]
         self.assertEqual(
@@ -116,7 +121,7 @@ dps:
         """Test dpid nomination doesn't nominate when no LACP ports are up"""
         self.activate_all_ports()
         valve = self.valves_manager.valves[0x1]
-        other_valves = self.valves_manager._other_running_valves(valve)
+        other_valves = self.get_other_valves(valve)
         # No actors UP so should return None
         nominated_dpid = valve.get_lacp_dpid_nomination(1, other_valves)[0]
         self.assertEqual(
@@ -143,7 +148,7 @@ dps:
                     lacp_ports[valve].append(port)
                     port.actor_up()
         for valve, ports in lacp_ports.items():
-            other_valves = self.valves_manager._other_running_valves(valve)
+            other_valves = self.get_other_valves(valve)
             for port in ports:
                 self.assertTrue(
                     valve.lacp_update_port_selection_state(port, other_valves),
@@ -161,7 +166,7 @@ dps:
         """Test flooding is allowed for UP & SELECTED LAG links only"""
         self.activate_all_ports()
         main_valve = self.valves_manager.valves[0x1]
-        main_other_valves = self.valves_manager._other_running_valves(main_valve)
+        main_other_valves = self.get_other_valves(main_valve)
         # Start with all LAG links NOACT & UNSELECTED
         self.validate_flood(2, 0, 3, False, 'Flooded out UNSELECTED & NOACT LAG port')
         self.validate_flood(2, 0, 4, False, 'Flooded out UNSELECTED & NOACT LAG port')
@@ -174,7 +179,7 @@ dps:
         self.validate_flood(2, 0, 4, True, 'Did not flood out SELECTED LAG port')
         # Set UP & SELECTED s2 LAG links
         valve = self.valves_manager.valves[0x2]
-        other_valves = self.valves_manager._other_running_valves(valve)
+        other_valves = self.get_other_valves(valve)
         for port in valve.dp.ports.values():
             if port.lacp:
                 valve.lacp_update(port, True, 1, 1, other_valves)
@@ -192,7 +197,7 @@ dps:
         """Test packets entering through UP & SELECTED LAG links"""
         self.activate_all_ports()
         main_valve = self.valves_manager.valves[0x1]
-        main_other_valves = self.valves_manager._other_running_valves(main_valve)
+        main_other_valves = self.get_other_valves(main_valve)
         # Packet initially rejected
         self.validate_flood(
             3, 0, None, False, 'Packet incoming through UNSELECTED & NOACT port was accepted')
@@ -209,7 +214,7 @@ dps:
             4, 0, None, True, 'Packet incoming through SELECTED port was not accepted')
         # Set UP & SELECTED s2 LAG links, set one s1 port down
         valve = self.valves_manager.valves[0x2]
-        other_valves = self.valves_manager._other_running_valves(valve)
+        other_valves = self.get_other_valves(valve)
         for port in valve.dp.ports.values():
             if port.lacp:
                 valve.lacp_update(port, True, 1, 1, other_valves)
@@ -362,10 +367,12 @@ class ValveStackChainTest(ValveTestBases.ValveTestSmall):
         self.assertFalse(self._learning_from_bcast(4), 'learn from access port broadcast')
 
     def test_stack_learn_edge(self):
+        """Test stack learned edge"""
         self.activate_all_ports()
         self.validate_edge_learn_ports()
 
     def test_stack_learn_root(self):
+        """Test stack learned root"""
         self.update_config(self._config_edge_learn_stack_root(True))
         self.activate_all_ports()
         self.validate_edge_learn_ports()
@@ -503,6 +510,7 @@ class ValveStackRedundantLink(ValveStackLoopTest):
 
 
 class ValveStackNonRootExtLoopProtectTestCase(ValveTestBases.ValveTestSmall):
+    """Test non-root external loop protect"""
 
     CONFIG = """
 dps:
@@ -563,6 +571,7 @@ dps:
         self.set_stack_port_up(1)
 
     def test_loop_protect(self):
+        """Test expected table outputs for external loop protect"""
         mcast_match = {
             'in_port': 2,
             'eth_dst': mac.BROADCAST_STR,
@@ -582,6 +591,7 @@ dps:
 
 
 class ValveStackAndNonStackTestCase(ValveTestBases.ValveTestSmall):
+    """Test stacked switches can exist with non-stacked switches"""
 
     CONFIG = """
 dps:
@@ -626,6 +636,7 @@ dps:
         self.setup_valve(self.CONFIG)
 
     def test_nonstack_dp_port(self):
+        """Test that finding a path from a stack swithc to a non-stack switch cannot happen"""
         self.assertEqual(None, self.valves_manager.valves[0x3].dp.shortest_path_port('s1'))
 
 
@@ -638,17 +649,20 @@ class ValveStackRedundancyTestCase(ValveTestBases.ValveTestSmall):
         self.setup_valve(self.CONFIG)
 
     def dp_by_name(self, dp_name):
+        """Get DP by DP name"""
         for valve in self.valves_manager.valves.values():
             if valve.dp.name == dp_name:
                 return valve.dp
         return None
 
     def set_stack_all_ports_status(self, dp_name, status):
+        """Set all stack ports to status on dp"""
         dp = self.dp_by_name(dp_name)
         for port in dp.stack_ports:
             port.dyn_stack_current_state = status
 
     def test_redundancy(self):
+        """Test redundant stack connections"""
         now = 1
         # All switches are down to start with.
         for dpid in self.valves_manager.valves:
@@ -731,6 +745,7 @@ class ValveRootStackTestCase(ValveTestBases.ValveTestSmall):
         self.verify_flooding(matches)
 
     def test_topo(self):
+        """Test DP is assigned appropriate edge/root states"""
         dp = self.valves_manager.valves[self.DP_ID].dp
         self.assertTrue(dp.is_stack_root())
         self.assertFalse(dp.is_stack_edge())
@@ -780,6 +795,7 @@ class ValveEdgeStackTestCase(ValveTestBases.ValveTestSmall):
             self.table.is_output(match, port=ofp.OFPP_CONTROLLER, vid=unexpressed_vid))
 
     def test_topo(self):
+        """Test DP is assigned appropriate edge/root states"""
         dp = self.valves_manager.valves[self.DP_ID].dp
         self.assertFalse(dp.is_stack_root())
         self.assertTrue(dp.is_stack_edge())
@@ -892,6 +908,7 @@ class ValveStackGraphBreakTestCase(ValveStackLoopTest):
         self.validate_flooding(rerouted=True)
 
     def _set_max_lldp_lost(self, new_value):
+        """Set the interface config option max_lldp_lost"""
         config = yaml.load(self.CONFIG, Loader=yaml.SafeLoader)
         for dp in config['dps'].values():
             for interface in dp['interfaces'].values():
@@ -1101,8 +1118,156 @@ class ValveTestIPV6StackedRouting(ValveTestBases.ValveTestStackedRouting):
         }
 
 
+class ValveInterVLANStackFlood(ValveTestBases.ValveTestSmall):
+    """Test that the stack ports get flooded to for interVLAN packets"""
+
+    VLAN100_FAUCET_MAC = '00:00:00:00:00:11'
+    VLAN200_FAUCET_MAC = '00:00:00:00:00:22'
+    VLAN100_FAUCET_VIPS = '10.1.0.254'
+    VLAN100_FAUCET_VIP_SPACE = '10.1.0.254/24'
+    VLAN200_FAUCET_VIPS = '10.2.0.254'
+    VLAN200_FAUCET_VIP_SPACE = '10.2.0.254/24'
+    DST_ADDRESS = ipaddress.IPv4Address('10.1.0.1')
+
+    def base_config(self):
+        """Create the base config"""
+        return """
+routers:
+    router1:
+        vlans: [vlan100, vlan200]
+dps:
+    s1:
+        hardware: 'GenericTFM'
+        dp_id: 1
+        interfaces:
+            1:
+                native_vlan: vlan100
+            2:
+                native_vlan: vlan200
+            3:
+                stack: {dp: s2, port: 3}
+    s2:
+        dp_id: 2
+        stack: {priority: 1}
+        interfaces:
+            1:
+                native_vlan: vlan100
+            2:
+                native_vlan: vlan200
+            3:
+                stack: {dp: s1, port: 3}
+            4:
+                stack: {dp: s3, port: 3}
+    s3:
+        dp_id: 3
+        interfaces:
+            1:
+                native_vlan: vlan100
+            2:
+                native_vlan: vlan200
+            3:
+                stack: {dp: s2, port: 4}
+            4:
+                stack: {dp: s4, port: 3}
+    s4:
+        dp_id: 4
+        interfaces:
+            1:
+                native_vlan: vlan100
+            2:
+                native_vlan: vlan200
+            3:
+                stack: {dp: s3, port: 4}
+"""
+
+    def create_config(self):
+        """Create the config file"""
+        self.CONFIG = """
+vlans:
+    vlan100:
+        vid: 100
+        faucet_mac: '%s'
+        faucet_vips: ['%s']
+    vlan200:
+        vid: 200
+        faucet_mac: '%s'
+        faucet_vips: ['%s']
+%s
+        """ % (self.VLAN100_FAUCET_MAC, self.VLAN100_FAUCET_VIP_SPACE,
+               self.VLAN200_FAUCET_MAC, self.VLAN200_FAUCET_VIP_SPACE,
+               self.base_config())
+
+    def setUp(self):
+        """Create a stacking config file."""
+        self.create_config()
+        self.setup_valve(self.CONFIG)
+        self.activate_all_ports()
+        for valve in self.valves_manager.valves.values():
+            for port in valve.dp.ports.values():
+                if port.stack:
+                    self.set_stack_port_up(port.number, valve)
+
+    def flood_manager_flood_ports(self, flood_manager):
+        """Return list of port numbers that will be flooded to"""
+        return [port.number for port in flood_manager._stack_flood_ports()]  # pylint: disable=protected-access
+
+    def route_manager_ofmsgs(self, route_manager, vlan):
+        """Return ofmsgs for route stack link flooding"""
+        faucet_vip = list(vlan.faucet_vips_by_ipv(4))[0].ip
+        ofmsgs = route_manager._flood_stack_links(  # pylint: disable=protected-access
+            route_manager._gw_resolve_pkt(), vlan, route_manager.multi_out,  # pylint: disable=protected-access
+            vlan.faucet_mac, valve_of.mac.BROADCAST_STR,
+            faucet_vip, self.DST_ADDRESS)
+        return ofmsgs
+
+    def test_flood_towards_root_from_s1(self):
+        """Test intervlan flooding goes towards the root"""
+        output_ports = [3]
+        valve = self.valves_manager.valves[1]
+        ports = self.flood_manager_flood_ports(valve.flood_manager)
+        self.assertEqual(output_ports, ports, 'InterVLAN flooding does not match expected')
+        route_manager = valve._route_manager_by_ipv.get(4, None)
+        vlan = valve.dp.vlans[100]
+        ofmsgs = self.route_manager_ofmsgs(route_manager, vlan)
+        self.assertTrue(self.packet_outs_from_flows(ofmsgs))
+
+    def test_flood_away_from_root(self):
+        """Test intervlan flooding goes away from the root"""
+        output_ports = [3, 4]
+        valve = self.valves_manager.valves[2]
+        ports = self.flood_manager_flood_ports(valve.flood_manager)
+        self.assertEqual(output_ports, ports, 'InterVLAN flooding does not match expected')
+        route_manager = valve._route_manager_by_ipv.get(4, None)
+        vlan = valve.dp.vlans[100]
+        ofmsgs = self.route_manager_ofmsgs(route_manager, vlan)
+        self.assertTrue(self.packet_outs_from_flows(ofmsgs))
+
+    def test_flood_towards_root_from_s3(self):
+        """Test intervlan flooding only goes towards the root (s4 will get the reflection)"""
+        output_ports = [3]
+        valve = self.valves_manager.valves[3]
+        ports = self.flood_manager_flood_ports(valve.flood_manager)
+        self.assertEqual(output_ports, ports, 'InterVLAN flooding does not match expected')
+        route_manager = valve._route_manager_by_ipv.get(4, None)
+        vlan = valve.dp.vlans[100]
+        ofmsgs = self.route_manager_ofmsgs(route_manager, vlan)
+        self.assertTrue(self.packet_outs_from_flows(ofmsgs))
+
+    def test_flood_towards_root_from_s4(self):
+        """Test intervlan flooding goes towards the root (through s3)"""
+        output_ports = [3]
+        valve = self.valves_manager.valves[4]
+        ports = self.flood_manager_flood_ports(valve.flood_manager)
+        self.assertEqual(output_ports, ports, 'InterVLAN flooding does not match expected')
+        route_manager = valve._route_manager_by_ipv.get(4, None)
+        vlan = valve.dp.vlans[100]
+        ofmsgs = self.route_manager_ofmsgs(route_manager, vlan)
+        self.assertTrue(self.packet_outs_from_flows(ofmsgs))
+
+
 class ValveTestTunnel(ValveTestBases.ValveTestSmall):
     """Test valve tunnel methods"""
+
     TUNNEL_ID = 200
     CONFIG = """
 acls: