Segment Routing bug fix and enhancement

Bugfix:
- Add MPLS BOS matching
- Fix NPE caused by race between filter objective and broadcast next objective

Enhancement:
- Move group handler out from OFDPA pipeline
- Move ARP request from rule populator to packet request

Change-Id: I0ba40e10f7cb7f97277df86725fbd2546a62e890
diff --git a/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java b/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
index b3e432c..1a37efd 100644
--- a/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
+++ b/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
@@ -513,7 +513,6 @@
     public void populatePortAddressingRules(DeviceId deviceId) {
         rulePopulator.populateRouterMacVlanFilters(deviceId);
         rulePopulator.populateRouterIpPunts(deviceId);
-        rulePopulator.populateArpPunts(deviceId);
     }
 
     /**
diff --git a/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java b/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
index d4aa770..8543c86 100644
--- a/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
+++ b/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
@@ -303,6 +303,7 @@
         // TODO Handle the case of Bos == false
         sbuilder.matchEthType(Ethernet.MPLS_UNICAST);
         sbuilder.matchMplsLabel(MplsLabel.mplsLabel(segmentId));
+        sbuilder.matchMplsBos(true);
         TrafficSelector selector = sbuilder.build();
 
         // setup metadata to pass to nextObjective - indicate the vlan on egress
@@ -525,39 +526,6 @@
     }
 
     /**
-     * Creates a forwarding objective to punt all IP packets, destined to the
-     * router's port IP addresses, to the controller. Note that the input
-     * port should not be matched on, as these packets can come from any input.
-     * Furthermore, these are applied only by the master instance.
-     *
-     * @param deviceId the switch dpid for the router
-     */
-    public void populateArpPunts(DeviceId deviceId) {
-        if (!srManager.mastershipService.isLocalMaster(deviceId)) {
-            log.debug("Not installing port-IP punts - not the master for dev:{} ",
-                    deviceId);
-            return;
-        }
-
-        ForwardingObjective.Builder puntArp = DefaultForwardingObjective.builder();
-        TrafficSelector.Builder sbuilder = DefaultTrafficSelector.builder();
-        TrafficTreatment.Builder tbuilder = DefaultTrafficTreatment.builder();
-        sbuilder.matchEthType(Ethernet.TYPE_ARP);
-        tbuilder.setOutput(PortNumber.CONTROLLER);
-        puntArp.withSelector(sbuilder.build());
-        puntArp.withTreatment(tbuilder.build());
-        puntArp.withFlag(Flag.VERSATILE)
-                .withPriority(HIGHEST_PRIORITY)
-                .makePermanent()
-                .fromApp(srManager.appId);
-        log.debug("Installing forwarding objective to punt ARPs");
-        srManager.flowObjectiveService.
-                forward(deviceId,
-                        puntArp.add(new SRObjectiveContext(deviceId,
-                                SRObjectiveContext.ObjectiveType.FORWARDING)));
-    }
-
-    /**
      * Populates a forwarding objective to send packets that miss other high
      * priority Bridging Table entries to a group that contains all ports of
      * its subnet.
diff --git a/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java b/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
index 7bcdfeb..0f20451 100644
--- a/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
+++ b/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
@@ -51,6 +51,7 @@
 import org.onosproject.net.flowobjective.ObjectiveError;
 import org.onosproject.net.host.HostEvent;
 import org.onosproject.net.host.HostListener;
+import org.onosproject.net.packet.PacketPriority;
 import org.onosproject.segmentrouting.config.DeviceConfigNotFoundException;
 import org.onosproject.segmentrouting.config.DeviceConfiguration;
 import org.onosproject.segmentrouting.config.SegmentRoutingConfig;
@@ -90,6 +91,7 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentLinkedQueue;
@@ -297,6 +299,11 @@
         linkService.addListener(linkListener);
         deviceService.addListener(deviceListener);
 
+        // Request ARP packet-in
+        TrafficSelector.Builder selector = DefaultTrafficSelector.builder();
+        selector.matchEthType(Ethernet.TYPE_ARP);
+        packetService.requestPackets(selector.build(), PacketPriority.CONTROL, appId, Optional.empty());
+
         cfgListener.configureNetwork();
 
         log.info("Started");
@@ -307,6 +314,11 @@
         cfgService.removeListener(cfgListener);
         cfgService.unregisterConfigFactory(cfgFactory);
 
+        // Withdraw ARP packet-in
+        TrafficSelector.Builder selector = DefaultTrafficSelector.builder();
+        selector.matchEthType(Ethernet.TYPE_ARP);
+        packetService.cancelPackets(selector.build(), PacketPriority.CONTROL, appId, Optional.empty());
+
         packetService.removeProcessor(processor);
         linkService.removeListener(linkListener);
         deviceService.removeListener(deviceListener);
@@ -697,7 +709,8 @@
                                            flowObjectiveService,
                                            nsNextObjStore,
                                            subnetNextObjStore,
-                                           portNextObjStore);
+                                           portNextObjStore,
+                                           this);
             } catch (DeviceConfigNotFoundException e) {
                 log.warn(e.getMessage() + " Aborting processDeviceAdded.");
                 return;
@@ -766,7 +779,8 @@
                                                    flowObjectiveService,
                                                    nsNextObjStore,
                                                    subnetNextObjStore,
-                                                   portNextObjStore);
+                                                   portNextObjStore,
+                                                   segmentRoutingManager);
                     } catch (DeviceConfigNotFoundException e) {
                         log.warn(e.getMessage() + " Aborting configureNetwork.");
                         return;
@@ -836,16 +850,7 @@
         private ForwardingObjective.Builder getForwardingObjectiveBuilder(
                      DeviceId deviceId, MacAddress mac, VlanId vlanId,
                      PortNumber outport) {
-            // match rule
-            TrafficSelector.Builder sbuilder = DefaultTrafficSelector.builder();
-            sbuilder.matchEthDst(mac);
-            sbuilder.matchVlanId(vlanId);
-
-            TrafficTreatment.Builder tbuilder = DefaultTrafficTreatment.builder();
-            tbuilder.immediate().popVlan();
-            tbuilder.immediate().setOutput(outport);
-
-            // for switch pipelines that need it, provide outgoing vlan as metadata
+            // Get assigned VLAN for the subnet
             VlanId outvlan = null;
             Ip4Prefix subnet = deviceConfiguration.getPortSubnet(deviceId, outport);
             if (subnet == null) {
@@ -853,6 +858,25 @@
             } else {
                 outvlan = getSubnetAssignedVlanId(deviceId, subnet);
             }
+
+            // match rule
+            TrafficSelector.Builder sbuilder = DefaultTrafficSelector.builder();
+            sbuilder.matchEthDst(mac);
+            /*
+             * Note: for untagged packets, match on the assigned VLAN.
+             *       for tagged packets, match on its incoming VLAN.
+             */
+            if (vlanId.equals(VlanId.NONE)) {
+                sbuilder.matchVlanId(outvlan);
+            } else {
+                sbuilder.matchVlanId(vlanId);
+            }
+
+            TrafficTreatment.Builder tbuilder = DefaultTrafficTreatment.builder();
+            tbuilder.immediate().popVlan();
+            tbuilder.immediate().setOutput(outport);
+
+            // for switch pipelines that need it, provide outgoing vlan as metadata
             TrafficSelector meta = DefaultTrafficSelector.builder()
                                         .matchVlanId(outvlan).build();
 
diff --git a/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultEdgeGroupHandler.java b/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultEdgeGroupHandler.java
index 32c5365..4866b82 100644
--- a/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultEdgeGroupHandler.java
+++ b/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultEdgeGroupHandler.java
@@ -24,6 +24,7 @@
 import org.onosproject.net.Link;
 import org.onosproject.net.flowobjective.FlowObjectiveService;
 import org.onosproject.net.link.LinkService;
+import org.onosproject.segmentrouting.SegmentRoutingManager;
 import org.onosproject.segmentrouting.config.DeviceProperties;
 import org.onosproject.store.service.EventuallyConsistentMap;
 
@@ -46,7 +47,7 @@
  * 8) what about ecmp no label case
  */
 public class DefaultEdgeGroupHandler extends DefaultGroupHandler {
-
+    // TODO Access stores through srManager
     protected DefaultEdgeGroupHandler(DeviceId deviceId,
                                   ApplicationId appId,
                                   DeviceProperties config,
@@ -58,9 +59,10 @@
                                   EventuallyConsistentMap<SubnetNextObjectiveStoreKey,
                                           Integer> subnetNextObjStore,
                                   EventuallyConsistentMap<PortNextObjectiveStoreKey,
-                                          Integer> portNextObjStore) {
+                                          Integer> portNextObjStore,
+                                  SegmentRoutingManager srManager) {
         super(deviceId, appId, config, linkService, flowObjService,
-              nsNextObjStore, subnetNextObjStore, portNextObjStore);
+              nsNextObjStore, subnetNextObjStore, portNextObjStore, srManager);
     }
 
     @Override
diff --git a/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultGroupHandler.java b/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultGroupHandler.java
index 2986c50..a4b89c1 100644
--- a/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultGroupHandler.java
+++ b/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultGroupHandler.java
@@ -87,6 +87,7 @@
             SubnetNextObjectiveStoreKey, Integer> subnetNextObjStore = null;
     protected EventuallyConsistentMap<
             PortNextObjectiveStoreKey, Integer> portNextObjStore = null;
+    private SegmentRoutingManager srManager;
 
     protected KryoNamespace.Builder kryo = new KryoNamespace.Builder()
             .register(URI.class).register(HashSet.class)
@@ -96,6 +97,7 @@
             .register(GroupBucketIdentifier.class)
             .register(GroupBucketIdentifier.BucketOutputType.class);
 
+    // TODO Access stores through srManager
     protected DefaultGroupHandler(DeviceId deviceId, ApplicationId appId,
                                   DeviceProperties config,
                                   LinkService linkService,
@@ -105,7 +107,8 @@
                                   EventuallyConsistentMap<SubnetNextObjectiveStoreKey,
                                           Integer> subnetNextObjStore,
                                   EventuallyConsistentMap<PortNextObjectiveStoreKey,
-                                          Integer> portNextObjStore) {
+                                          Integer> portNextObjStore,
+                                  SegmentRoutingManager srManager) {
         this.deviceId = checkNotNull(deviceId);
         this.appId = checkNotNull(appId);
         this.deviceConfig = checkNotNull(config);
@@ -123,6 +126,7 @@
         this.nsNextObjStore = nsNextObjStore;
         this.subnetNextObjStore = subnetNextObjStore;
         this.portNextObjStore = portNextObjStore;
+        this.srManager = srManager;
 
         populateNeighborMaps();
     }
@@ -153,7 +157,8 @@
                                           EventuallyConsistentMap<SubnetNextObjectiveStoreKey,
                                           Integer> subnetNextObjStore,
                                           EventuallyConsistentMap<PortNextObjectiveStoreKey,
-                                          Integer> portNextObjStore)
+                                          Integer> portNextObjStore,
+                                          SegmentRoutingManager srManager)
                                                   throws DeviceConfigNotFoundException {
         // handle possible exception in the caller
         if (config.isEdgeDevice(deviceId)) {
@@ -162,14 +167,17 @@
                                                flowObjService,
                                                nsNextObjStore,
                                                subnetNextObjStore,
-                                               portNextObjStore);
+                                               portNextObjStore,
+                                               srManager
+                                               );
         } else {
             return new DefaultTransitGroupHandler(deviceId, appId, config,
                                                   linkService,
                                                   flowObjService,
                                                   nsNextObjStore,
                                                   subnetNextObjStore,
-                                                  portNextObjStore);
+                                                  portNextObjStore,
+                                                  srManager);
         }
     }
 
@@ -663,11 +671,17 @@
                 return;
             }
 
+            VlanId assignedVlanId =
+                    srManager.getSubnetAssignedVlanId(this.deviceId, subnet);
+            TrafficSelector metadata =
+                    DefaultTrafficSelector.builder().matchVlanId(assignedVlanId).build();
+
             int nextId = flowObjectiveService.allocateNextId();
 
             NextObjective.Builder nextObjBuilder = DefaultNextObjective
                     .builder().withId(nextId)
-                    .withType(NextObjective.Type.BROADCAST).fromApp(appId);
+                    .withType(NextObjective.Type.BROADCAST).fromApp(appId)
+                    .withMeta(metadata);
 
             ports.forEach(port -> {
                 TrafficTreatment.Builder tBuilder = DefaultTrafficTreatment.builder();
diff --git a/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultTransitGroupHandler.java b/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultTransitGroupHandler.java
index 7a43e73..5bc7ede 100644
--- a/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultTransitGroupHandler.java
+++ b/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultTransitGroupHandler.java
@@ -23,6 +23,7 @@
 import org.onosproject.net.Link;
 import org.onosproject.net.flowobjective.FlowObjectiveService;
 import org.onosproject.net.link.LinkService;
+import org.onosproject.segmentrouting.SegmentRoutingManager;
 import org.onosproject.segmentrouting.config.DeviceConfigNotFoundException;
 import org.onosproject.segmentrouting.config.DeviceProperties;
 import org.onosproject.store.service.EventuallyConsistentMap;
@@ -40,7 +41,7 @@
  * 2) all ports to D3 + with no label push,
  */
 public class DefaultTransitGroupHandler extends DefaultGroupHandler {
-
+    // TODO Access stores through srManager
     protected DefaultTransitGroupHandler(DeviceId deviceId,
                                   ApplicationId appId,
                                   DeviceProperties config,
@@ -52,9 +53,10 @@
                                   EventuallyConsistentMap<SubnetNextObjectiveStoreKey,
                                         Integer> subnetNextObjStore,
                                   EventuallyConsistentMap<PortNextObjectiveStoreKey,
-                                  Integer> portNextObjStore) {
+                                  Integer> portNextObjStore,
+                                  SegmentRoutingManager srManager) {
         super(deviceId, appId, config, linkService, flowObjService,
-              nsNextObjStore, subnetNextObjStore, portNextObjStore);
+              nsNextObjStore, subnetNextObjStore, portNextObjStore, srManager);
     }
 
     @Override
diff --git a/src/main/java/org/onosproject/segmentrouting/grouphandler/PolicyGroupHandler.java b/src/main/java/org/onosproject/segmentrouting/grouphandler/PolicyGroupHandler.java
index ef143dc..4b0d518 100644
--- a/src/main/java/org/onosproject/segmentrouting/grouphandler/PolicyGroupHandler.java
+++ b/src/main/java/org/onosproject/segmentrouting/grouphandler/PolicyGroupHandler.java
@@ -27,6 +27,7 @@
 import org.onlab.packet.MacAddress;
 import org.onlab.packet.MplsLabel;
 import org.onosproject.core.ApplicationId;
+import org.onosproject.segmentrouting.SegmentRoutingManager;
 import org.onosproject.segmentrouting.config.DeviceConfigNotFoundException;
 import org.onosproject.segmentrouting.config.DeviceProperties;
 import org.onosproject.segmentrouting.grouphandler.GroupBucketIdentifier.BucketOutputType;
@@ -60,6 +61,7 @@
      * @param nsNextObjStore NeighborSet next objective store map
      * @param subnetNextObjStore subnet next objective store map
      */
+    // TODO Access stores through srManager
     public PolicyGroupHandler(DeviceId deviceId,
                               ApplicationId appId,
                               DeviceProperties config,
@@ -70,9 +72,10 @@
                               EventuallyConsistentMap<SubnetNextObjectiveStoreKey,
                                       Integer> subnetNextObjStore,
                               EventuallyConsistentMap<PortNextObjectiveStoreKey,
-                              Integer> portNextObjStore) {
+                              Integer> portNextObjStore,
+                              SegmentRoutingManager srManager) {
         super(deviceId, appId, config, linkService, flowObjService,
-              nsNextObjStore, subnetNextObjStore, portNextObjStore);
+              nsNextObjStore, subnetNextObjStore, portNextObjStore, srManager);
     }
 
     public PolicyGroupIdentifier createPolicyGroupChain(String id,