In this commit:

Removing dependence on hashing for unique groupkeys in ofdpa driver. Group-store no longer removes groups from store if a group-operation fails due to GROUP_EXISTS. Group-store also checks for unique group-id when given by app. Group-provider now logs warning before making call to core. Change-Id: I4a1dcb887cb74cd6e245df0c82c90a50d8f3898a

In this commit:
Removing dependence on hashing for unique groupkeys in ofdpa driver. Group-store no longer removes groups from store if a group-operation fails due to GROUP_EXISTS. Group-store also checks for unique group-id when given by app. Group-provider now logs warning before making call to core. Change-Id: I4a1dcb887cb74cd6e245df0c82c90a50d8f3898a
Saurav Das · Gerrit Code Review
Commit 8be4e3a4cb53553ea748f0501a5d64e251f7171e 8be4e3a4 1 parent 49e03a34
Showing 4 changed files with 112 additions and 55 deletions
core/store/dist/src/main/java/org/onosproject/store/group/impl/DistributedGroupStore.java
drivers/default/src/main/java/org/onosproject/driver/pipeline/OFDPA2GroupHandler.java
drivers/default/src/main/java/org/onosproject/driver/pipeline/OFDPA2Pipeline.java
providers/openflow/group/src/main/java/org/onosproject/provider/of/group/impl/OpenFlowGroupProvider.java
--- a/core/store/dist/src/main/java/org/onosproject/store/group/impl/DistributedGroupStore.java
View file @8be4e3a
+++ b/core/store/dist/src/main/java/org/onosproject/store/group/impl/DistributedGroupStore.java
View file @8be4e3a
@@ -537,6 +537,19 @@ public class DistributedGroupStore
             // Get a new group identifier
             id = new DefaultGroupId(getFreeGroupIdValue(groupDesc.deviceId()));
         } else {
+            // we need to use the identifier passed in by caller, but check if
+            // already used
+            Group existing = getGroup(groupDesc.deviceId(),
+                                      new DefaultGroupId(groupDesc.givenGroupId()));
+            if (existing != null) {
+                log.warn("Group already exists with the same id: 0x{} in dev:{} "
+                        + "but with different key: {} (request gkey: {})",
+                        Integer.toHexString(groupDesc.givenGroupId()),
+                        groupDesc.deviceId(),
+                        existing.appCookie(),
+                        groupDesc.appCookie());
+                return;
+            }
             id = new DefaultGroupId(groupDesc.givenGroupId());
         }
         // Create a group entry object
@@ -621,7 +634,8 @@ public class DistributedGroupStore
         // Check if a group is existing with the provided key
         Group oldGroup = getGroup(deviceId, oldAppCookie);
         if (oldGroup == null) {
-            log.warn("updateGroupDescriptionInternal: Group not found...strange");
+            log.warn("updateGroupDescriptionInternal: Group not found...strange. "
+                    + "GroupKey:{} DeviceId:{}", oldAppCookie, deviceId);
             return;
         }
@@ -940,6 +954,19 @@ public class DistributedGroupStore
             log.warn("Current extraneous groups in device:{} are: {}",
                      deviceId,
                      getExtraneousGroups(deviceId));
+            if (operation.buckets().equals(existing.buckets())) {
+                if (existing.state() == GroupState.PENDING_ADD) {
+                    log.info("GROUP_EXISTS: GroupID and Buckets match for group in pending "
+                            + "add state - moving to ADDED for group {} in device {}",
+                            existing.id(), deviceId);
+                    addOrUpdateGroupEntry(existing);
+                    return;
+                } else {
+                    log.warn("GROUP EXISTS: Group ID matched but buckets did not. "
+                            + "Operation: {} Existing: {}", operation.buckets(),
+                            existing.buckets());
+                }
+            }
         }
         switch (operation.opType()) {
             case ADD:
--- a/drivers/default/src/main/java/org/onosproject/driver/pipeline/OFDPA2GroupHandler.java
View file @8be4e3a
+++ b/drivers/default/src/main/java/org/onosproject/driver/pipeline/OFDPA2GroupHandler.java
View file @8be4e3a
@@ -36,6 +36,8 @@ 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.store.service.AtomicCounter;
+import org.onosproject.store.service.StorageService;
 import org.slf4j.Logger;
 import java.util.ArrayDeque;
@@ -87,6 +89,7 @@ public class OFDPA2GroupHandler {
     private final Logger log = getLogger(getClass());
     private ServiceDirectory serviceDirectory;
     protected GroupService groupService;
+    protected StorageService storageService;
     private DeviceId deviceId;
     private FlowObjectiveStore flowObjectiveStore;
@@ -96,7 +99,7 @@ public class OFDPA2GroupHandler {
             Executors.newScheduledThreadPool(2, groupedThreads("onos/pipeliner", "ofdpa2-%d"));
     // index number for group creation
-    private AtomicInteger l3VpnIndex = new AtomicInteger(0);
+    private AtomicCounter nextIndex;
     // local stores for port-vlan mapping
     protected Map<PortNumber, VlanId> port2Vlan = new ConcurrentHashMap<>();
@@ -111,6 +114,11 @@ public class OFDPA2GroupHandler {
         this.flowObjectiveStore = context.store();
         this.serviceDirectory = context.directory();
         this.groupService = serviceDirectory.get(GroupService.class);
+        this.storageService = serviceDirectory.get(StorageService.class);
+        this.nextIndex = storageService.atomicCounterBuilder()
+                            .withName("group-id-index-counter")
+                            .build()
+                            .asAtomicCounter();
         pendingNextObjectives = CacheBuilder.newBuilder()
                 .expireAfterWrite(20, TimeUnit.SECONDS)
@@ -129,6 +137,10 @@ public class OFDPA2GroupHandler {
         groupService.addListener(new InnerGroupListener());
     }
+    //////////////////////////////////////
+    //  Group Creation
+    //////////////////////////////////////
+
     protected void addGroup(NextObjective nextObjective) {
         switch (nextObjective.type()) {
             case SIMPLE:
@@ -213,26 +225,6 @@ public class OFDPA2GroupHandler {
         groupService.addGroup(groupInfo.innerGrpDesc);
     }
-    private void updatePendingNextObjective(GroupKey key, OfdpaNextGroup value) {
-        List<OfdpaNextGroup> nextList = new CopyOnWriteArrayList<OfdpaNextGroup>();
-        nextList.add(value);
-        List<OfdpaNextGroup> ret = pendingNextObjectives.asMap()
-                .putIfAbsent(key, nextList);
-        if (ret != null) {
-            ret.add(value);
-        }
-    }
-
-    private void updatePendingGroups(GroupKey gkey, GroupChainElem gce) {
-        Set<GroupChainElem> gceSet = Collections.newSetFromMap(
-                new ConcurrentHashMap<GroupChainElem, Boolean>());
-        gceSet.add(gce);
-        Set<GroupChainElem> retval = pendingGroups.putIfAbsent(gkey, gceSet);
-        if (retval != null) {
-            retval.add(gce);
-        }
-    }
-
     /**
      * Creates a simple L2 Interface Group.
      *
@@ -403,7 +395,7 @@ public class OFDPA2GroupHandler {
         // assemble information for ofdpa l2interface group
         int l2groupId = L2_INTERFACE_TYPE | (vlanid.toShort() << 16) | (int) portNum;
-        // a globally unique groupkey that is different for ports in the same devices
+        // a globally unique groupkey that is different for ports in the same device,
         // but different for the same portnumber on different devices. Also different
         // for the various group-types created out of the same next objective.
         int l2gk = l2InterfaceGroupKey(deviceId, vlanid, portNum);
@@ -413,10 +405,10 @@ public class OFDPA2GroupHandler {
         GroupDescription outerGrpDesc = null;
         if (mpls) {
             // outer group is MPLSInteface
-            int mplsgroupId = MPLS_INTERFACE_TYPE | (int) portNum;
+            int mplsInterfaceIndex = getNextAvailableIndex();
-            // using mplsinterfacemask in groupkey to differentiate from l2interface
+            int mplsgroupId = MPLS_INTERFACE_TYPE | (SUBTYPE_MASK & mplsInterfaceIndex);
-            int mplsgk = MPLS_INTERFACE_TYPE | (SUBTYPE_MASK & (deviceId.hashCode() << 8 | (int) portNum));
+            final GroupKey mplsgroupkey = new DefaultGroupKey(
-            final GroupKey mplsgroupkey = new DefaultGroupKey(OFDPA2Pipeline.appKryo.serialize(mplsgk));
+                               OFDPA2Pipeline.appKryo.serialize(mplsInterfaceIndex));
             outerTtb.group(new DefaultGroupId(l2groupId));
             // create the mpls-interface group description to wait for the
             // l2 interface group to be processed
@@ -435,11 +427,10 @@ public class OFDPA2GroupHandler {
                     mplsgroupkey, nextId);
         } else {
             // outer group is L3Unicast
-            int l3GroupIdHash = Objects.hash(srcMac, dstMac, portNum);
+            int l3unicastIndex = getNextAvailableIndex();
-            int l3groupId = L3_UNICAST_TYPE | (TYPE_MASK & l3GroupIdHash);
+            int l3groupId = L3_UNICAST_TYPE | (TYPE_MASK & l3unicastIndex);
-            int l3GroupKeyHash = Objects.hash(deviceId, srcMac, dstMac, portNum);
+            final GroupKey l3groupkey = new DefaultGroupKey(
-            int l3gk = L3_UNICAST_TYPE | (TYPE_MASK & l3GroupKeyHash);
+                               OFDPA2Pipeline.appKryo.serialize(l3unicastIndex));
-            final GroupKey l3groupkey = new DefaultGroupKey(OFDPA2Pipeline.appKryo.serialize(l3gk));
             outerTtb.group(new DefaultGroupId(l2groupId));
             // create the l3unicast group description to wait for the
             // l2 interface group to be processed
@@ -558,7 +549,7 @@ public class OFDPA2GroupHandler {
         // assemble info for l2 flood group
         // since there can be only one flood group for a vlan, its index is always the same - 0
         Integer l2floodgroupId = L2_FLOOD_TYPE | (vlanId.toShort() << 16);
-        int l2floodgk = L2_FLOOD_TYPE | nextObj.id() << 12;
+        int l2floodgk = getNextAvailableIndex();
         final GroupKey l2floodgroupkey = new DefaultGroupKey(OFDPA2Pipeline.appKryo.serialize(l2floodgk));
         // collection of group buckets pointing to all the l2 interface groups
         List<GroupBucket> l2floodBuckets = new ArrayList<>();
@@ -633,8 +624,10 @@ public class OFDPA2GroupHandler {
                     .createSelectGroupBucket(ttb.build());
             l3ecmpGroupBuckets.add(sbucket);
         }
-        int l3ecmpGroupId = L3_ECMP_TYPE | nextObj.id() << 12;
+        int l3ecmpIndex = getNextAvailableIndex();
-        GroupKey l3ecmpGroupKey = new DefaultGroupKey(OFDPA2Pipeline.appKryo.serialize(l3ecmpGroupId));
+        int l3ecmpGroupId = L3_ECMP_TYPE | (TYPE_MASK & l3ecmpIndex);
+        GroupKey l3ecmpGroupKey = new DefaultGroupKey(
+                                          OFDPA2Pipeline.appKryo.serialize(l3ecmpIndex));
         GroupDescription l3ecmpGroupDesc =
                 new DefaultGroupDescription(
                         deviceId,
@@ -742,9 +735,10 @@ public class OFDPA2GroupHandler {
                                 onelabelGroupInfo.outerGrpDesc.givenGroupId()));
                 GroupBucket l3vpnGrpBkt  =
                         DefaultGroupBucket.createIndirectGroupBucket(l3vpnTtb.build());
-                int l3vpngroupId = MPLS_L3VPN_SUBTYPE | l3VpnIndex.incrementAndGet();
+                int l3vpnIndex = getNextAvailableIndex();
-                int l3vpngk = MPLS_L3VPN_SUBTYPE | nextObj.id() << 12 | l3VpnIndex.get();
+                int l3vpngroupId = MPLS_L3VPN_SUBTYPE | (SUBTYPE_MASK & l3vpnIndex);
-                GroupKey l3vpngroupkey = new DefaultGroupKey(OFDPA2Pipeline.appKryo.serialize(l3vpngk));
+                GroupKey l3vpngroupkey = new DefaultGroupKey(
+                             OFDPA2Pipeline.appKryo.serialize(l3vpnIndex));
                 GroupDescription l3vpnGroupDesc =
                         new DefaultGroupDescription(
                                 deviceId,
@@ -783,6 +777,10 @@ public class OFDPA2GroupHandler {
         }
     }
+    //////////////////////////////////////
+    //  Group Editing
+    //////////////////////////////////////
+
     /**
      *  Adds a bucket to the top level group of a group-chain, and creates the chain.
      *
@@ -954,6 +952,30 @@ public class OFDPA2GroupHandler {
         flowObjectiveStore.removeNextGroup(nextObjective.id());
     }
+    //////////////////////////////////////
+    //  Helper Methods and Classes
+    //////////////////////////////////////
+
+    private void updatePendingNextObjective(GroupKey key, OfdpaNextGroup value) {
+        List<OfdpaNextGroup> nextList = new CopyOnWriteArrayList<OfdpaNextGroup>();
+        nextList.add(value);
+        List<OfdpaNextGroup> ret = pendingNextObjectives.asMap()
+                .putIfAbsent(key, nextList);
+        if (ret != null) {
+            ret.add(value);
+        }
+    }
+
+    private void updatePendingGroups(GroupKey gkey, GroupChainElem gce) {
+        Set<GroupChainElem> gceSet = Collections.newSetFromMap(
+                new ConcurrentHashMap<GroupChainElem, Boolean>());
+        gceSet.add(gce);
+        Set<GroupChainElem> retval = pendingGroups.putIfAbsent(gkey, gceSet);
+        if (retval != null) {
+            retval.add(gce);
+        }
+    }
+
     /**
      * Processes next element of a group chain. Assumption is that if this
      * group points to another group, the latter has already been created
@@ -1002,6 +1024,17 @@ public class OFDPA2GroupHandler {
         }
     }
+    private class InnerGroupListener implements GroupListener {
+        @Override
+        public void event(GroupEvent event) {
+            log.trace("received group event of type {}", event.type());
+            if (event.type() == GroupEvent.Type.GROUP_ADDED) {
+                GroupKey key = event.subject().appCookie();
+                processPendingGroupsOrNextObjectives(key, true);
+            }
+        }
+    }
+
     private void processPendingGroupsOrNextObjectives(GroupKey key, boolean added) {
         //first check for group chain
         Set<GroupChainElem> gceSet = pendingGroups.remove(key);
@@ -1046,6 +1079,10 @@ public class OFDPA2GroupHandler {
                 ? null : ((VlanIdCriterion) criterion).vlanId();
     }
+    private int getNextAvailableIndex() {
+        return (int) nextIndex.incrementAndGet();
+    }
+
     /**
      * Returns a hash as the L2 Interface Group Key.
      *
@@ -1065,17 +1102,6 @@ public class OFDPA2GroupHandler {
         return L2_INTERFACE_TYPE | (TYPE_MASK & hash << 6) | portLowerBits;
     }
-    private class InnerGroupListener implements GroupListener {
-        @Override
-        public void event(GroupEvent event) {
-            log.trace("received group event of type {}", event.type());
-            if (event.type() == GroupEvent.Type.GROUP_ADDED) {
-                GroupKey key = event.subject().appCookie();
-                processPendingGroupsOrNextObjectives(key, true);
-            }
-        }
-    }
-
     /**
      * Utility class for moving group information around.
      */
--- a/drivers/default/src/main/java/org/onosproject/driver/pipeline/OFDPA2Pipeline.java
View file @8be4e3a
+++ b/drivers/default/src/main/java/org/onosproject/driver/pipeline/OFDPA2Pipeline.java
View file @8be4e3a
@@ -1030,26 +1030,30 @@ public class OFDPA2Pipeline extends AbstractHandlerBehaviour implements Pipeline
         obj.context().ifPresent(context -> context.onError(obj, error));
     }
-
     @Override
     public List<String> getNextMappings(NextGroup nextGroup) {
         List<String> mappings = new ArrayList<>();
         List<Deque<GroupKey>> gkeys = appKryo.deserialize(nextGroup.data());
         for (Deque<GroupKey> gkd : gkeys) {
             Group lastGroup = null;
-            String gchain = "";
+            StringBuffer gchain = new StringBuffer();
             for (GroupKey gk : gkd) {
                 Group g = groupService.getGroup(deviceId, gk);
-                gchain += "  0x" + Integer.toHexString(g.id().id()) + " -->";
+                if (g == null) {
+                    gchain.append("  ERROR").append(" -->");
+                    continue;
+                }
+                gchain.append("  0x").append(Integer.toHexString(g.id().id()))
+                    .append(" -->");
                 lastGroup = g;
             }
             // add port information for last group in group-chain
             for (Instruction i: lastGroup.buckets().buckets().get(0).treatment().allInstructions()) {
                 if (i instanceof OutputInstruction) {
-                    gchain += " port:" + ((OutputInstruction) i).port();
+                    gchain.append(" port:").append(((OutputInstruction) i).port());
                 }
             }
-            mappings.add(gchain);
+            mappings.add(gchain.toString());
         }
         return mappings;
     }
--- a/providers/openflow/group/src/main/java/org/onosproject/provider/of/group/impl/OpenFlowGroupProvider.java
View file @8be4e3a
+++ b/providers/openflow/group/src/main/java/org/onosproject/provider/of/group/impl/OpenFlowGroupProvider.java
View file @8be4e3a
@@ -337,11 +337,11 @@ public class OpenFlowGroupProvider extends AbstractProvider implements GroupProv
                                         GroupMsgErrorCode.values()[(code.ordinal())];
                                 GroupOperation failedOperation = GroupOperation
                                         .createFailedGroupOperation(operation, failureCode);
+                                log.warn("Received a group mod error {}", msg);
                                 providerService.groupOperationFailed(deviceId,
                                         failedOperation);
                                 pendingGroupOperations.remove(pendingGroupId);
                                 pendingXidMaps.remove(pendingGroupId);
-                                log.warn("Received a group mod error {}", msg);
                             } else {
                                 log.error("Cannot find pending group operation with group ID: {}",
                                         pendingGroupId);