BGP router now handles the case where groups don't exists right away.

Also reworked some logic to make delete routes work. Change-Id: I1f65279284b85144a847f1295fcbd7695cb59167

BGP router now handles the case where groups don't exists right away.
Also reworked some logic to make delete routes work. Change-Id: I1f65279284b85144a847f1295fcbd7695cb59167
Jonathan Hart
Commit 7baba078721f29e50eef67eda16048c5ae76fdd1 7baba078 1 parent 3a7c98e7
Showing 6 changed files with 182 additions and 66 deletions
apps/bgprouter/src/main/java/org/onosproject/bgprouter/BgpRouter.java
apps/bgprouter/src/main/java/org/onosproject/bgprouter/NextHop.java
apps/bgprouter/src/main/java/org/onosproject/bgprouter/NextHopGroupKey.java
core/store/dist/src/main/java/org/onosproject/store/statistic/impl/DistributedStatisticStore.java
core/store/trivial/src/main/java/org/onosproject/store/trivial/impl/SimpleStatisticStore.java
providers/openflow/group/src/main/java/org/onosproject/provider/of/group/impl/OpenFlowGroupProvider.java
--- a/apps/bgprouter/src/main/java/org/onosproject/bgprouter/BgpRouter.java
View file @7baba07
+++ b/apps/bgprouter/src/main/java/org/onosproject/bgprouter/BgpRouter.java
View file @7baba07
@@ -16,6 +16,9 @@
 package org.onosproject.bgprouter;
 import com.google.common.collect.ConcurrentHashMultiset;
+import com.google.common.collect.HashMultimap;
+import com.google.common.collect.Maps;
+import com.google.common.collect.Multimap;
 import com.google.common.collect.Multiset;
 import org.apache.felix.scr.annotations.Activate;
 import org.apache.felix.scr.annotations.Component;
@@ -24,6 +27,8 @@ import org.apache.felix.scr.annotations.Reference;
 import org.apache.felix.scr.annotations.ReferenceCardinality;
 import org.onlab.packet.Ethernet;
 import org.onlab.packet.MacAddress;
+import org.onlab.packet.IpAddress;
+import org.onlab.packet.IpPrefix;
 import org.onosproject.core.ApplicationId;
 import org.onosproject.core.CoreService;
 import org.onosproject.net.DeviceId;
@@ -42,9 +47,12 @@ import org.onosproject.net.group.Group;
 import org.onosproject.net.group.GroupBucket;
 import org.onosproject.net.group.GroupBuckets;
 import org.onosproject.net.group.GroupDescription;
+import org.onosproject.net.group.GroupEvent;
 import org.onosproject.net.group.GroupKey;
+import org.onosproject.net.group.GroupListener;
 import org.onosproject.net.group.GroupService;
 import org.onosproject.net.packet.PacketService;
+import org.onosproject.routing.FibEntry;
 import org.onosproject.routing.FibListener;
 import org.onosproject.routing.FibUpdate;
 import org.onosproject.routing.RoutingService;
@@ -55,7 +63,6 @@ import org.slf4j.LoggerFactory;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.Map;
 /**
@@ -90,20 +97,32 @@ public class BgpRouter {
     private ApplicationId appId;
-    private final Multiset<NextHop> nextHops = ConcurrentHashMultiset.create();
+    // Reference count for how many times a next hop is used by a route
-    private final Map<NextHop, NextHopGroupKey> groups = new HashMap<>();
+    private final Multiset<IpAddress> nextHopsCount = ConcurrentHashMultiset.create();
+
+    // Mapping from prefix to its current next hop
+    private final Map<IpPrefix, IpAddress> prefixToNextHop = Maps.newHashMap();
+
+    // Mapping from next hop IP to next hop object containing group info
+    private final Map<IpAddress, NextHop> nextHops = Maps.newHashMap();
+
+    // Stores FIB updates that are waiting for groups to be set up
+    private final Multimap<GroupKey, FibEntry> pendingUpdates = HashMultimap.create();
     private DeviceId deviceId = DeviceId.deviceId("of:0000000000000001"); // TODO config
+    private final GroupListener groupListener = new InternalGroupListener();
+
     private TunnellingConnectivityManager connectivityManager;
     private InternalTableHandler provisionStaticTables = new InternalTableHandler();
     @Activate
     protected void activate() {
-        log.info("Bgp1Router started");
         appId = coreService.registerApplication(BGP_ROUTER_APP);
+        groupService.addListener(groupListener);
+
         provisionStaticTables.provision(true);
         connectivityManager = new TunnellingConnectivityManager(appId,
@@ -123,29 +142,38 @@ public class BgpRouter {
         connectivityManager.stop();
         provisionStaticTables.provision(false);
+        groupService.removeListener(groupListener);
+
         log.info("BgpRouter stopped");
     }
     private void updateFibEntry(Collection<FibUpdate> updates) {
         for (FibUpdate update : updates) {
-            NextHop nextHop = new NextHop(update.entry().nextHopIp(),
+            FibEntry entry = update.entry();
-                                          update.entry().nextHopMac());
-            addNextHop(nextHop);
+            addNextHop(entry);
-            TrafficSelector selector = DefaultTrafficSelector.builder()
+            Group group;
-                    .matchEthType(Ethernet.TYPE_IPV4)
+            synchronized (pendingUpdates) {
-                    .matchIPDst(update.entry().prefix())
+                NextHop nextHop = nextHops.get(entry.nextHopIp());
-                    .build();
+                group = groupService.getGroup(deviceId, nextHop.group());
-            // TODO ensure group exists
-            NextHopGroupKey groupKey = groups.get(nextHop);
-            Group group = groupService.getGroup(deviceId, groupKey);
                 if (group == null) {
-                // TODO handle this
+                    log.debug("Adding pending flow {}", update.entry());
-                log.warn("oops, group {} wasn't there");
+                    pendingUpdates.put(nextHop.group(), update.entry());
                     continue;
                 }
+            }
+
+            installFlow(update.entry(), group);
+        }
+    }
+
+    private void installFlow(FibEntry entry, Group group) {
+        TrafficSelector selector = DefaultTrafficSelector.builder()
+                .matchEthType(Ethernet.TYPE_IPV4)
+                .matchIPDst(entry.prefix())
+                .build();
         TrafficTreatment treatment = DefaultTrafficTreatment.builder()
                 .group(group.id())
@@ -157,16 +185,15 @@ public class BgpRouter {
         flowService.applyFlowRules(flowRule);
     }
-    }
-    private void deleteFibEntry(Collection<FibUpdate> withdraws) {
+    private synchronized void deleteFibEntry(Collection<FibUpdate> withdraws) {
         for (FibUpdate update : withdraws) {
-            NextHop nextHop = new NextHop(update.entry().nextHopIp(),
+            FibEntry entry = update.entry();
-                                          update.entry().nextHopMac());
-            deleteNextHop(nextHop);
+            deleteNextHop(entry.prefix());
             TrafficSelector selector = DefaultTrafficSelector.builder()
+                    .matchEthType(Ethernet.TYPE_IPV4)
                     .matchIPDst(update.entry().prefix())
                     .build();
@@ -178,18 +205,20 @@ public class BgpRouter {
         }
     }
-    private void addNextHop(NextHop nextHop) {
+    private synchronized void addNextHop(FibEntry entry) {
-        if (nextHops.add(nextHop, 1) == 0) {
+        prefixToNextHop.put(entry.prefix(), entry.nextHopIp());
+        if (nextHopsCount.count(entry.nextHopIp()) == 0) {
             // There was no next hop in the multiset
-            Interface egressIntf = configService.getMatchingInterface(nextHop.ip());
+            Interface egressIntf = configService.getMatchingInterface(entry.nextHopIp());
             if (egressIntf == null) {
-                log.warn("no egress interface found for {}", nextHop);
+                log.warn("no egress interface found for {}", entry);
                 return;
             }
-            NextHopGroupKey groupKey = new NextHopGroupKey(nextHop.ip());
+            NextHopGroupKey groupKey = new NextHopGroupKey(entry.nextHopIp());
-            groups.put(nextHop, groupKey);
+
+            NextHop nextHop = new NextHop(entry.nextHopIp(), entry.nextHopMac(), groupKey);
             TrafficTreatment treatment = DefaultTrafficTreatment.builder()
                     .setEthSrc(egressIntf.mac())
@@ -209,17 +238,30 @@ public class BgpRouter {
                                                   appId);
             groupService.addGroup(groupDescription);
+
+            nextHops.put(nextHop.ip(), nextHop);
+
         }
+
+        nextHopsCount.add(entry.nextHopIp());
+    }
+
+    private synchronized void deleteNextHop(IpPrefix prefix) {
+        IpAddress nextHopIp = prefixToNextHop.remove(prefix);
+        NextHop nextHop = nextHops.get(nextHopIp);
+        if (nextHop == null) {
+            log.warn("No next hop found when removing prefix {}", prefix);
+            return;
         }
-    private void deleteNextHop(NextHop nextHop) {
+        if (nextHopsCount.remove(nextHopIp, 1) <= 1) {
-        if (nextHops.remove(nextHop, 1) <= 1) {
             // There was one or less next hops, so there are now none
-            log.debug("removing group");
+            log.debug("removing group for next hop {}", nextHop);
+
+            nextHops.remove(nextHopIp);
-            GroupKey groupKey = groups.remove(nextHop);
+            groupService.removeGroup(deviceId, nextHop.group(), appId);
-            groupService.removeGroup(deviceId, groupKey, appId);
         }
     }
@@ -238,7 +280,6 @@ public class BgpRouter {
         private static final int CONTROLLER_PRIORITY = 255;
         private static final int DROP_PRIORITY = 0;
-
         public void provision(boolean install) {
             processTableZero(install);
@@ -262,14 +303,14 @@ public class BgpRouter {
             treatment.transition(FlowRule.Type.VLAN_MPLS);
             FlowRule rule = new DefaultFlowRule(deviceId, selector.build(),
-                                                treatment.build(), CONTROLLER_PRIORITY,
+                                                treatment.build(),
-                                                appId, 0, true);
+                                                CONTROLLER_PRIORITY, appId, 0,
+                                                true);
             FlowRuleOperations.Builder ops = FlowRuleOperations.builder();
             ops = install ? ops.add(rule) : ops.remove(rule);
-
             //Drop rule
             selector = DefaultTrafficSelector.builder();
             treatment = DefaultTrafficTreatment.builder();
@@ -277,8 +318,8 @@ public class BgpRouter {
             treatment.drop();
             rule = new DefaultFlowRule(deviceId, selector.build(),
-                                       treatment.build(), DROP_PRIORITY,
+                                       treatment.build(), DROP_PRIORITY, appId,
-                                       appId, 0, true, FlowRule.Type.VLAN_MPLS);
+                                       0, true, FlowRule.Type.VLAN_MPLS);
             ops = install ? ops.add(rule) : ops.remove(rule);
@@ -298,15 +339,15 @@ public class BgpRouter {
         private void processTableOne(boolean install) {
             TrafficSelector.Builder selector = DefaultTrafficSelector.builder();
-            TrafficTreatment.Builder treatment = DefaultTrafficTreatment.builder();
+            TrafficTreatment.Builder treatment = DefaultTrafficTreatment
+                    .builder();
             FlowRuleOperations.Builder ops = FlowRuleOperations.builder();
             FlowRule rule;
             selector.matchEthType(Ethernet.TYPE_IPV4);
             treatment.transition(FlowRule.Type.VLAN);
-            rule = new DefaultFlowRule(deviceId, selector.build(),
+            rule = new DefaultFlowRule(deviceId, selector.build(), treatment.build(), CONTROLLER_PRIORITY,
-                                  treatment.build(), CONTROLLER_PRIORITY,
                                        appId, 0, true, FlowRule.Type.VLAN_MPLS);
             ops = install ? ops.add(rule) : ops.remove(rule);
@@ -342,8 +383,8 @@ public class BgpRouter {
             treatment.drop();
             rule = new DefaultFlowRule(deviceId, selector.build(),
-                                       treatment.build(), DROP_PRIORITY,
+                                       treatment.build(), DROP_PRIORITY, appId,
-                                       appId, 0, true, FlowRule.Type.VLAN_MPLS);
+                                       0, true, FlowRule.Type.VLAN_MPLS);
             ops = install ? ops.add(rule) : ops.remove(rule);
@@ -355,7 +396,8 @@ public class BgpRouter {
                 @Override
                 public void onError(FlowRuleOperations ops) {
-                    log.info("Failed to provision vlan/mpls table for bgp router");
+                    log.info(
+                            "Failed to provision vlan/mpls table for bgp router");
                 }
             }));
@@ -363,7 +405,8 @@ public class BgpRouter {
         private void processTableTwo(boolean install) {
             TrafficSelector.Builder selector = DefaultTrafficSelector.builder();
-            TrafficTreatment.Builder treatment = DefaultTrafficTreatment.builder();
+            TrafficTreatment.Builder treatment = DefaultTrafficTreatment
+                    .builder();
             FlowRuleOperations.Builder ops = FlowRuleOperations.builder();
             FlowRule rule;
@@ -372,8 +415,8 @@ public class BgpRouter {
             treatment.drop();
             rule = new DefaultFlowRule(deviceId, selector.build(),
-                                       treatment.build(), DROP_PRIORITY,
+                                       treatment.build(), DROP_PRIORITY, appId,
-                                       appId, 0, true, FlowRule.Type.VLAN);
+                                       0, true, FlowRule.Type.VLAN);
             ops = install ? ops.add(rule) : ops.remove(rule);
@@ -390,11 +433,10 @@ public class BgpRouter {
             }));
         }
-
-
         private void processTableThree(boolean install) {
             TrafficSelector.Builder selector = DefaultTrafficSelector.builder();
-            TrafficTreatment.Builder treatment = DefaultTrafficTreatment.builder();
+            TrafficTreatment.Builder treatment = DefaultTrafficTreatment
+                    .builder();
             FlowRuleOperations.Builder ops = FlowRuleOperations.builder();
             FlowRule rule;
@@ -426,8 +468,8 @@ public class BgpRouter {
             treatment.drop();
             rule = new DefaultFlowRule(deviceId, selector.build(),
-                                       treatment.build(), DROP_PRIORITY,
+                                       treatment.build(), DROP_PRIORITY, appId,
-                                       appId, 0, true, FlowRule.Type.VLAN_MPLS);
+                                       0, true, FlowRule.Type.VLAN_MPLS);
             ops = install ? ops.add(rule) : ops.remove(rule);
@@ -443,20 +485,20 @@ public class BgpRouter {
                 }
             }));
-
         }
         private void processTableFive(boolean install) {
             TrafficSelector.Builder selector = DefaultTrafficSelector.builder();
-            TrafficTreatment.Builder treatment = DefaultTrafficTreatment.builder();
+            TrafficTreatment.Builder treatment = DefaultTrafficTreatment
+                    .builder();
             FlowRuleOperations.Builder ops = FlowRuleOperations.builder();
             FlowRule rule;
             treatment.transition(FlowRule.Type.IP);
             rule = new DefaultFlowRule(deviceId, selector.build(),
-                                       treatment.build(), DROP_PRIORITY,
+                                       treatment.build(), DROP_PRIORITY, appId,
-                                       appId, 0, true, FlowRule.Type.COS);
+                                       0, true, FlowRule.Type.COS);
             ops = install ? ops.add(rule) : ops.remove(rule);
@@ -476,7 +518,8 @@ public class BgpRouter {
         private void processTableSix(boolean install) {
             TrafficSelector.Builder selector = DefaultTrafficSelector.builder();
-            TrafficTreatment.Builder treatment = DefaultTrafficTreatment.builder();
+            TrafficTreatment.Builder treatment = DefaultTrafficTreatment
+                    .builder();
             FlowRuleOperations.Builder ops = FlowRuleOperations.builder();
             FlowRule rule;
@@ -485,8 +528,8 @@ public class BgpRouter {
             treatment.drop();
             rule = new DefaultFlowRule(deviceId, selector.build(),
-                                       treatment.build(), DROP_PRIORITY,
+                                       treatment.build(), DROP_PRIORITY, appId,
-                                       appId, 0, true, FlowRule.Type.IP);
+                                       0, true, FlowRule.Type.IP);
             ops = install ? ops.add(rule) : ops.remove(rule);
@@ -505,7 +548,8 @@ public class BgpRouter {
         private void processTableNine(boolean install) {
             TrafficSelector.Builder selector = DefaultTrafficSelector.builder();
-            TrafficTreatment.Builder treatment = DefaultTrafficTreatment.builder();
+            TrafficTreatment.Builder treatment = DefaultTrafficTreatment
+                    .builder();
             FlowRuleOperations.Builder ops = FlowRuleOperations.builder();
             FlowRule rule;
@@ -529,6 +573,21 @@ public class BgpRouter {
                 }
             }));
         }
+    }
+
+    private class InternalGroupListener implements GroupListener {
+        @Override
+        public void event(GroupEvent event) {
+            Group group = event.subject();
+
+            if (event.type() == GroupEvent.Type.GROUP_ADDED ||
+                    event.type() == GroupEvent.Type.GROUP_UPDATED) {
+                synchronized (pendingUpdates) {
+                    pendingUpdates.removeAll(group.appCookie())
+                            .forEach((entry) -> installFlow(entry, group));
+                }
+            }
+        }
     }
 }
--- a/apps/bgprouter/src/main/java/org/onosproject/bgprouter/NextHop.java
View file @7baba07
+++ b/apps/bgprouter/src/main/java/org/onosproject/bgprouter/NextHop.java
View file @7baba07
@@ -18,30 +18,59 @@ package org.onosproject.bgprouter;
 import com.google.common.base.MoreObjects;
 import org.onlab.packet.IpAddress;
 import org.onlab.packet.MacAddress;
+import org.onosproject.net.group.GroupKey;
 import java.util.Objects;
 /**
- * Created by jono on 2/12/15.
+ * Represents a next hop for routing, whose MAC address has already been resolved.
  */
 public class NextHop {
     private final IpAddress ip;
     private final MacAddress mac;
+    private final GroupKey group;
-    public NextHop(IpAddress ip, MacAddress mac) {
+    /**
+     * Creates a new next hop.
+     *
+     * @param ip next hop's IP address
+     * @param mac next hop's MAC address
+     * @param group next hop's group
+     */
+    public NextHop(IpAddress ip, MacAddress mac, GroupKey group) {
         this.ip = ip;
         this.mac = mac;
+        this.group = group;
     }
+    /**
+     * Returns the next hop's IP address.
+     *
+     * @return next hop's IP address
+     */
     public IpAddress ip() {
         return ip;
     }
+    /**
+     * Returns the next hop's MAC address.
+     *
+     * @return next hop's MAC address
+     */
     public MacAddress mac() {
         return mac;
     }
+    /**
+     * Returns the next hop group.
+     *
+     * @return group
+     */
+    public GroupKey group() {
+        return group;
+    }
+
     @Override
     public boolean equals(Object o) {
         if (!(o instanceof NextHop)) {
@@ -51,12 +80,13 @@ public class NextHop {
         NextHop that = (NextHop) o;
         return Objects.equals(this.ip, that.ip) &&
-                Objects.equals(this.mac, that.mac);
+                Objects.equals(this.mac, that.mac) &&
+                Objects.equals(this.group, that.group);
     }
     @Override
     public int hashCode() {
-        return Objects.hash(ip, mac);
+        return Objects.hash(ip, mac, group);
     }
     @Override
@@ -64,6 +94,7 @@ public class NextHop {
         return MoreObjects.toStringHelper(getClass())
                 .add("ip", ip)
                 .add("mac", mac)
+                .add("group", group)
                 .toString();
     }
 }
--- a/apps/bgprouter/src/main/java/org/onosproject/bgprouter/NextHopGroupKey.java
View file @7baba07
+++ b/apps/bgprouter/src/main/java/org/onosproject/bgprouter/NextHopGroupKey.java
View file @7baba07
@@ -24,16 +24,26 @@ import java.util.Objects;
 import static com.google.common.base.Preconditions.checkNotNull;
 /**
- * Created by jono on 2/16/15.
+ * Identifier for a next hop group.
  */
 public class NextHopGroupKey implements GroupKey {
     private final IpAddress address;
+    /**
+     * Creates a new next hop group key.
+     *
+     * @param address next hop's IP address
+     */
     public NextHopGroupKey(IpAddress address) {
         this.address = checkNotNull(address);
     }
+    /**
+     * Returns the next hop's IP address.
+     *
+     * @return next hop's IP address
+     */
     public IpAddress address() {
         return address;
     }
--- a/core/store/dist/src/main/java/org/onosproject/store/statistic/impl/DistributedStatisticStore.java
View file @7baba07
+++ b/core/store/dist/src/main/java/org/onosproject/store/statistic/impl/DistributedStatisticStore.java
View file @7baba07
@@ -288,8 +288,16 @@ public class DistributedStatisticStore implements StatisticStore {
     private ConnectPoint buildConnectPoint(FlowRule rule) {
         PortNumber port = getOutput(rule);
+
+        boolean hasGoto = rule.treatment().instructions()
+                .stream()
+                .anyMatch(i -> (i instanceof Instructions.GroupInstruction)
+                        || (i instanceof Instructions.TableTypeTransition));
+
         if (port == null) {
+            if (!hasGoto) {
                 log.debug("Rule {} has no output.", rule);
+            }
             return null;
         }
         ConnectPoint cp = new ConnectPoint(rule.deviceId(), port);
--- a/core/store/trivial/src/main/java/org/onosproject/store/trivial/impl/SimpleStatisticStore.java
View file @7baba07
+++ b/core/store/trivial/src/main/java/org/onosproject/store/trivial/impl/SimpleStatisticStore.java
View file @7baba07
@@ -154,8 +154,16 @@ public class SimpleStatisticStore implements StatisticStore {
     private ConnectPoint buildConnectPoint(FlowRule rule) {
         PortNumber port = getOutput(rule);
+
+        boolean hasGoto = rule.treatment().instructions()
+                .stream()
+                .anyMatch(i -> (i instanceof Instructions.GroupInstruction)
+                        || (i instanceof Instructions.TableTypeTransition));
+
         if (port == null) {
+            if (!hasGoto) {
                 log.debug("Rule {} has no output.", rule);
+            }
             return null;
         }
         ConnectPoint cp = new ConnectPoint(rule.deviceId(), port);
--- a/providers/openflow/group/src/main/java/org/onosproject/provider/of/group/impl/OpenFlowGroupProvider.java
View file @7baba07
+++ b/providers/openflow/group/src/main/java/org/onosproject/provider/of/group/impl/OpenFlowGroupProvider.java
View file @7baba07
@@ -310,7 +310,7 @@ public class OpenFlowGroupProvider extends AbstractProvider implements GroupProv
                         break;
                     }
                 default:
-                    log.debug("Unhandled message type: {}", msg.getType());
+                    break;
             }
         }