Several fixes for Segment Routing

1. In getSubnetNextObjectiveId, do not create a broadcast group if it does not exist
       The creation is done when device is up and when config is added
2. Check if broadcast group exists before creating one
3. Put subnet in a Set instead of a List to avoid duplicated elements

Change-Id: Ifafd8e783a9d98aa62b5e299f560897458b90bb0

@@ -29,7 +29,6 @@ import org.slf4j.LoggerFactory;
 import java.util.ArrayList;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.HashSet;
-import java.util.List;
 import java.util.Set;
 import java.util.Set;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
 import java.util.concurrent.locks.ReentrantLock;
@@ -451,7 +450,7 @@ public class DefaultRoutingHandler {
         // If both target switch and dest switch are edge routers, then set IP
         // If both target switch and dest switch are edge routers, then set IP
         // rule for both subnet and router IP.
         // rule for both subnet and router IP.
         if (config.isEdgeDevice(targetSw) && config.isEdgeDevice(destSw)) {
         if (config.isEdgeDevice(targetSw) && config.isEdgeDevice(destSw)) {
-            List<Ip4Prefix> subnets = config.getSubnets(destSw);
+            Set<Ip4Prefix> subnets = config.getSubnets(destSw);
             log.debug("* populateEcmpRoutingRulePartial in device {} towards {} for subnets {}",
             log.debug("* populateEcmpRoutingRulePartial in device {} towards {} for subnets {}",
                     targetSw, destSw, subnets);
                     targetSw, destSw, subnets);
             result = rulePopulator.populateIpRuleForSubnet(targetSw,
             result = rulePopulator.populateIpRuleForSubnet(targetSw,

@@ -15,6 +15,7 @@
  */
  */
 package org.onosproject.segmentrouting;
 package org.onosproject.segmentrouting;
 
 
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Lists;
 import org.onlab.packet.Ip4Address;
 import org.onlab.packet.Ip4Address;
 import org.onlab.packet.Ip4Prefix;
 import org.onlab.packet.Ip4Prefix;
@@ -344,12 +345,12 @@ public class DeviceConfiguration implements DeviceProperties {
      * @param deviceId device identifier
      * @param deviceId device identifier
      * @return list of ip prefixes or null if not found
      * @return list of ip prefixes or null if not found
      */
      */
-    public List<Ip4Prefix> getSubnets(DeviceId deviceId) {
+    public Set<Ip4Prefix> getSubnets(DeviceId deviceId) {
         SegmentRouterInfo srinfo = deviceConfigMap.get(deviceId);
         SegmentRouterInfo srinfo = deviceConfigMap.get(deviceId);
         if (srinfo != null) {
         if (srinfo != null) {
             log.trace("getSubnets for device{} is {}", deviceId,
             log.trace("getSubnets for device{} is {}", deviceId,
                       srinfo.subnets.values());
                       srinfo.subnets.values());
-            return new ArrayList<>(srinfo.subnets.values());
+            return ImmutableSet.copyOf(srinfo.subnets.values());
         }
         }
         return null;
         return null;
     }
     }
@@ -424,7 +425,7 @@ public class DeviceConfiguration implements DeviceProperties {
      */
      */
     public boolean inSameSubnet(DeviceId deviceId, Ip4Address hostIp) {
     public boolean inSameSubnet(DeviceId deviceId, Ip4Address hostIp) {
 
 
-        List<Ip4Prefix> subnets = getSubnets(deviceId);
+        Set<Ip4Prefix> subnets = getSubnets(deviceId);
         if (subnets == null) {
         if (subnets == null) {
             return false;
             return false;
         }
         }

@@ -141,7 +141,7 @@ public class RoutingRulePopulator {
      * @return true if all rules are set successfully, false otherwise
      * @return true if all rules are set successfully, false otherwise
      */
      */
     public boolean populateIpRuleForSubnet(DeviceId deviceId,
     public boolean populateIpRuleForSubnet(DeviceId deviceId,
-                                           List<Ip4Prefix> subnets,
+                                           Set<Ip4Prefix> subnets,
                                            DeviceId destSw,
                                            DeviceId destSw,
                                            Set<DeviceId> nextHops) {
                                            Set<DeviceId> nextHops) {
 
 

@@ -375,7 +375,7 @@ public class SegmentRoutingManager implements SegmentRoutingService {
             return null;
             return null;
         }
         }
         // vlan assignment is expensive but done only once
         // vlan assignment is expensive but done only once
-        List<Ip4Prefix> configuredSubnets = deviceConfiguration.getSubnets(deviceId);
+        Set<Ip4Prefix> configuredSubnets = deviceConfiguration.getSubnets(deviceId);
         Set<Short> assignedVlans = new HashSet<>();
         Set<Short> assignedVlans = new HashSet<>();
         Set<Ip4Prefix> unassignedSubnets = new HashSet<>();
         Set<Ip4Prefix> unassignedSubnets = new HashSet<>();
         for (Ip4Prefix sub : configuredSubnets) {
         for (Ip4Prefix sub : configuredSubnets) {

@@ -334,8 +334,8 @@ public class DefaultGroupHandler {
     }
     }
 
 
     /**
     /**
-     * Returns the next objective associated with the neighborset.
-     * If there is no next objective for this neighborset, this API
+     * Returns the next objective associated with the subnet.
+     * If there is no next objective for this subnet, this API
      * would create a next objective and return.
      * would create a next objective and return.
      *
      *
      * @param prefix subnet information
      * @param prefix subnet information
@@ -344,31 +344,8 @@ public class DefaultGroupHandler {
     public int getSubnetNextObjectiveId(IpPrefix prefix) {
     public int getSubnetNextObjectiveId(IpPrefix prefix) {
         Integer nextId = subnetNextObjStore.
         Integer nextId = subnetNextObjStore.
                 get(new SubnetNextObjectiveStoreKey(deviceId, prefix));
                 get(new SubnetNextObjectiveStoreKey(deviceId, prefix));
-        if (nextId == null) {
-            log.trace("getSubnetNextObjectiveId in device{}: Next objective id "
-                              + "not found for {} and creating", deviceId, prefix);
-            log.trace("getSubnetNextObjectiveId: subnetNextObjStore contents for device {}: {}",
-                      deviceId,
-                      subnetNextObjStore.entrySet()
-                              .stream()
-                              .filter((subnetStoreEntry) ->
-                                              (subnetStoreEntry.getKey().deviceId().equals(deviceId)))
-                              .collect(Collectors.toList()));
-            createGroupsFromSubnetConfig();
-            nextId = subnetNextObjStore.
-                    get(new SubnetNextObjectiveStoreKey(deviceId, prefix));
-            if (nextId == null) {
-                log.warn("subnetNextObjStore: unable to create next objective");
-                return -1;
-            } else {
-                log.debug("subnetNextObjStore in device{}: Next objective id {} "
-                                  + "created for {}", deviceId, nextId, prefix);
-            }
-        } else {
-            log.trace("subnetNextObjStore in device{}: Next objective id {} "
-                              + "found for {}", deviceId, nextId, prefix);
-        }
-        return nextId;
+
+        return (nextId != null) ? nextId : -1;
     }
     }
 
 
     /**
     /**
@@ -541,6 +518,15 @@ public class DefaultGroupHandler {
 
 
         // Construct a broadcast group for each subnet
         // Construct a broadcast group for each subnet
         subnetPortMap.forEach((subnet, ports) -> {
         subnetPortMap.forEach((subnet, ports) -> {
+            SubnetNextObjectiveStoreKey key =
+                    new SubnetNextObjectiveStoreKey(deviceId, subnet);
+
+            if (subnetNextObjStore.containsKey(key)) {
+                log.debug("Broadcast group for device {} and subnet {} exists",
+                          deviceId, subnet);
+                return;
+            }
+
             int nextId = flowObjectiveService.allocateNextId();
             int nextId = flowObjectiveService.allocateNextId();
 
 
             NextObjective.Builder nextObjBuilder = DefaultNextObjective
             NextObjective.Builder nextObjBuilder = DefaultNextObjective
@@ -558,8 +544,7 @@ public class DefaultGroupHandler {
             log.debug("createGroupFromSubnetConfig: Submited "
             log.debug("createGroupFromSubnetConfig: Submited "
                               + "next objective {} in device {}",
                               + "next objective {} in device {}",
                       nextId, deviceId);
                       nextId, deviceId);
-            SubnetNextObjectiveStoreKey key =
-                    new SubnetNextObjectiveStoreKey(deviceId, subnet);
+
             subnetNextObjStore.put(key, nextId);
             subnetNextObjStore.put(key, nextId);
         });
         });
     }
     }