ONOS-973: Implemented a unit test for OpenFlowGroupProvider implmentation.

 - Fixed a bug in OpenFlowGroupProvider of wrong handling of pending group operations.

Change-Id: I70b80629f4eed000110d242f3558abe49b6b13bc

@@ -28,7 +28,6 @@ import org.projectfloodlight.openflow.types.OFGroup;
 import org.slf4j.Logger;
 
 import java.util.concurrent.TimeUnit;
-import java.util.concurrent.atomic.AtomicLong;
 
 import static org.slf4j.LoggerFactory.getLogger;
 
@@ -43,7 +42,6 @@ public class GroupStatsCollector implements TimerTask {
     private final int refreshInterval;
 
     private Timeout timeout;
-    private final AtomicLong xidCounter = new AtomicLong(1);
 
     private boolean stopTimer = false;
 
@@ -79,7 +77,7 @@ public class GroupStatsCollector implements TimerTask {
         if (sw.getRole() != RoleState.MASTER) {
             return;
         }
-        Long statsXid = xidCounter.getAndAdd(2);
+        Long statsXid = OpenFlowGroupProvider.getXidAndAdd(2);
         OFGroupStatsRequest statsRequest = sw.factory().buildGroupStatsRequest()
                 .setGroup(OFGroup.ALL)
                 .setXid(statsXid)

@@ -83,12 +83,15 @@ public class OpenFlowGroupProvider extends AbstractProvider implements GroupProv
 
     private final InternalGroupProvider listener = new InternalGroupProvider();
 
-    private final AtomicLong xidCounter = new AtomicLong(1);
+    private static final AtomicLong XID_COUNTER = new AtomicLong(1);
     private final Map<Dpid, GroupStatsCollector> collectors = Maps.newHashMap();
-    private final Map<Long, OFStatsReply> groupStats = Maps.newHashMap();
-    private final Map<Long, GroupOperation> pendingGroupOperations =
+    private final Map<Long, OFStatsReply> groupStats = Maps.newConcurrentMap();
+    private final Map<Integer, GroupOperation> pendingGroupOperations =
             Maps.newConcurrentMap();
 
+    /* Map<Group ID, Transaction ID> */
+    private final Map<Integer, Long> pendingXidMaps = Maps.newConcurrentMap();
+
     /**
      * Creates a OpenFlow group provider.
      */
@@ -126,10 +129,10 @@ public class OpenFlowGroupProvider extends AbstractProvider implements GroupProv
         OpenFlowSwitch sw = controller.getSwitch(dpid);
         for (GroupOperation groupOperation: groupOps.operations()) {
             if (sw == null) {
-                log.error("SW {} is not found", sw.getStringId());
+                log.error("SW {} is not found", dpid);
                 return;
             }
-            final Long groupModXid = xidCounter.getAndIncrement();
+            final Long groupModXid = XID_COUNTER.getAndIncrement();
             GroupModBuilder builder =
                     GroupModBuilder.builder(groupOperation.buckets(),
                             groupOperation.groupId(),
@@ -151,7 +154,9 @@ public class OpenFlowGroupProvider extends AbstractProvider implements GroupProv
                     log.error("Unsupported Group operation");
             }
             sw.sendMsg(groupMod);
-            pendingGroupOperations.put(groupModXid, groupOperation);
+            pendingGroupOperations.put(groupMod.getGroup().getGroupNumber(),
+                    groupOperation);
+            pendingXidMaps.put(groupMod.getGroup().getGroupNumber(), groupModXid);
         }
      }
 
@@ -161,6 +166,7 @@ public class OpenFlowGroupProvider extends AbstractProvider implements GroupProv
         OFGroupStatsReply groupStatsReply = null;
         OFGroupDescStatsReply groupDescStatsReply = null;
 
+        synchronized (groupStats) {
             if (statsReply.getStatsType() == OFStatsType.GROUP) {
                 OFStatsReply reply = groupStats.get(statsReply.getXid() + 1);
                 if (reply != null) {
@@ -180,6 +186,7 @@ public class OpenFlowGroupProvider extends AbstractProvider implements GroupProv
                     groupStats.put(statsReply.getXid(), statsReply);
                 }
             }
+        }
 
         if (groupStatsReply != null && groupDescStatsReply != null) {
             Collection<Group> groups = buildGroupMetrics(deviceId,
@@ -187,6 +194,7 @@ public class OpenFlowGroupProvider extends AbstractProvider implements GroupProv
             providerService.pushGroupMetrics(deviceId, groups);
             for (Group group: groups) {
                 pendingGroupOperations.remove(group.id());
+                pendingXidMaps.remove(group.id());
             }
         }
     }
@@ -238,6 +246,17 @@ public class OpenFlowGroupProvider extends AbstractProvider implements GroupProv
         return null;
     }
 
+    /**
+     * Returns a transaction ID for entire group operations and increases
+     * the counter by the number given.
+     *
+     * @param increase the amount to increase the counter by
+     * @return a transaction ID
+     */
+    public static long getXidAndAdd(int increase) {
+        return XID_COUNTER.getAndAdd(increase);
+    }
+
     private class InternalGroupProvider
             implements OpenFlowSwitchListener, OpenFlowEventListener {
 
@@ -250,11 +269,28 @@ public class OpenFlowGroupProvider extends AbstractProvider implements GroupProv
                 case ERROR:
                     OFErrorMsg errorMsg = (OFErrorMsg) msg;
                     if (errorMsg.getErrType() == OFErrorType.GROUP_MOD_FAILED) {
+                        int pendingGroupId = -1;
+                        for (Map.Entry<Integer, Long> entry: pendingXidMaps.entrySet()) {
+                            if (entry.getValue() == errorMsg.getXid()) {
+                                pendingGroupId = entry.getKey();
+                                break;
+                            }
+                        }
+                        if (pendingGroupId == -1) {
+                            log.warn("Error for unknown group operation: {}",
+                                    errorMsg.getXid());
+                        } else {
                             GroupOperation operation =
-                                pendingGroupOperations.get(errorMsg.getXid());
+                                    pendingGroupOperations.get(pendingGroupId);
                             if (operation != null) {
                                 providerService.groupOperationFailed(operation);
-                            log.warn("received Error message {} from {}", msg, dpid);
+                                pendingGroupOperations.remove(pendingGroupId);
+                                pendingXidMaps.remove(pendingGroupId);
+                                log.warn("Received an group mod error {}", msg);
+                            } else {
+                                log.error("Cannot find pending group operation with group ID: {}",
+                                        pendingGroupId);
+                            }
                         }
                         break;
                     }

+package org.onosproject.provider.of.group.impl;
+
+import com.google.common.collect.Lists;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.onosproject.core.DefaultGroupId;
+import org.onosproject.core.GroupId;
+import org.onosproject.net.DeviceId;
+import org.onosproject.net.PortNumber;
+import org.onosproject.net.flow.DefaultTrafficTreatment;
+import org.onosproject.net.flow.TrafficTreatment;
+import org.onosproject.net.group.DefaultGroupBucket;
+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.GroupOperation;
+import org.onosproject.net.group.GroupOperations;
+import org.onosproject.net.group.GroupProvider;
+import org.onosproject.net.group.GroupProviderRegistry;
+import org.onosproject.net.group.GroupProviderService;
+import org.onosproject.net.provider.AbstractProviderService;
+import org.onosproject.net.provider.ProviderId;
+import org.onosproject.openflow.controller.Dpid;
+import org.onosproject.openflow.controller.OpenFlowController;
+import org.onosproject.openflow.controller.OpenFlowEventListener;
+import org.onosproject.openflow.controller.OpenFlowSwitch;
+import org.onosproject.openflow.controller.OpenFlowSwitchListener;
+import org.onosproject.openflow.controller.PacketListener;
+import org.onosproject.openflow.controller.RoleState;
+import org.projectfloodlight.openflow.protocol.OFFactories;
+import org.projectfloodlight.openflow.protocol.OFFactory;
+import org.projectfloodlight.openflow.protocol.OFGroupDescStatsReply;
+import org.projectfloodlight.openflow.protocol.OFGroupMod;
+import org.projectfloodlight.openflow.protocol.OFGroupModFailedCode;
+import org.projectfloodlight.openflow.protocol.OFGroupStatsReply;
+import org.projectfloodlight.openflow.protocol.OFGroupType;
+import org.projectfloodlight.openflow.protocol.OFMessage;
+import org.projectfloodlight.openflow.protocol.OFPortDesc;
+import org.projectfloodlight.openflow.protocol.OFVersion;
+import org.projectfloodlight.openflow.protocol.errormsg.OFGroupModFailedErrorMsg;
+import org.projectfloodlight.openflow.types.OFGroup;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Set;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+
+public class OpenFlowGroupProviderTest {
+
+    OpenFlowGroupProvider provider = new OpenFlowGroupProvider();
+    private final OpenFlowController controller = new TestController();
+    GroupProviderRegistry providerRegistry = new TestGroupProviderRegistry();
+    GroupProviderService providerService;
+
+    private DeviceId deviceId = DeviceId.deviceId("of:0000000000000001");
+    private Dpid dpid1 = Dpid.dpid(deviceId.uri());
+
+    @Before
+    public void setUp() {
+        provider.controller = controller;
+        provider.providerRegistry = providerRegistry;
+        provider.activate();
+    }
+
+    @Test
+    public void basics() {
+        assertNotNull("registration expected", providerService);
+        assertEquals("incorrect provider", provider, providerService.provider());
+    }
+
+    @Test
+    public void addGroup() {
+
+        GroupId groupId = new DefaultGroupId(1);
+
+        List<GroupBucket> bucketList = Lists.newArrayList();
+        TrafficTreatment.Builder builder = DefaultTrafficTreatment.builder();
+        builder.setOutput(PortNumber.portNumber(1));
+        GroupBucket bucket =
+                DefaultGroupBucket.createSelectGroupBucket(builder.build());
+        bucketList.add(bucket);
+        GroupBuckets buckets = new GroupBuckets(bucketList);
+
+        List<GroupOperation> operationList = Lists.newArrayList();
+        GroupOperation operation = GroupOperation.createAddGroupOperation(groupId,
+                GroupDescription.Type.SELECT, buckets);
+        operationList.add(operation);
+        GroupOperations operations = new GroupOperations(operationList);
+
+        provider.performGroupOperation(deviceId, operations);
+
+        final Dpid dpid = Dpid.dpid(deviceId.uri());
+        TestOpenFlowSwitch sw = (TestOpenFlowSwitch) controller.getSwitch(dpid);
+        assertNotNull("Switch should not be nul", sw);
+        assertNotNull("OFGroupMsg should not be null", sw.msg);
+
+    }
+
+
+    @Test
+    public void groupModFailure() {
+        TestOpenFlowGroupProviderService testProviderService =
+                (TestOpenFlowGroupProviderService) providerService;
+
+        GroupId groupId = new DefaultGroupId(1);
+        List<GroupBucket> bucketList = Lists.newArrayList();
+        TrafficTreatment.Builder builder = DefaultTrafficTreatment.builder();
+        builder.setOutput(PortNumber.portNumber(1));
+        GroupBucket bucket =
+                DefaultGroupBucket.createSelectGroupBucket(builder.build());
+        bucketList.add(bucket);
+        GroupBuckets buckets = new GroupBuckets(bucketList);
+        List<GroupOperation> operationList = Lists.newArrayList();
+        GroupOperation operation = GroupOperation.createAddGroupOperation(groupId,
+                GroupDescription.Type.SELECT, buckets);
+        operationList.add(operation);
+        GroupOperations operations = new GroupOperations(operationList);
+
+        provider.performGroupOperation(deviceId, operations);
+
+        OFGroupModFailedErrorMsg.Builder errorBuilder =
+                OFFactories.getFactory(OFVersion.OF_13).errorMsgs().buildGroupModFailedErrorMsg();
+        OFGroupMod.Builder groupBuilder = OFFactories.getFactory(OFVersion.OF_13).buildGroupModify();
+        groupBuilder.setGroupType(OFGroupType.ALL);
+        groupBuilder.setGroup(OFGroup.of(1));
+        errorBuilder.setCode(OFGroupModFailedCode.GROUP_EXISTS);
+        errorBuilder.setXid(provider.getXidAndAdd(0) - 1);
+
+        controller.processPacket(dpid1, errorBuilder.build());
+
+        assertNotNull("Operation failed should not be null",
+                testProviderService.failedOperation);
+    }
+
+
+    @Test
+    public void groupStatsEvent() {
+        TestOpenFlowGroupProviderService testProviderService =
+                (TestOpenFlowGroupProviderService) providerService;
+
+        OFGroupStatsReply.Builder rep1 =
+                OFFactories.getFactory(OFVersion.OF_13).buildGroupStatsReply();
+        rep1.setXid(1);
+        controller.processPacket(dpid1, rep1.build());
+        OFGroupDescStatsReply.Builder rep2 =
+                OFFactories.getFactory(OFVersion.OF_13).buildGroupDescStatsReply();
+        assertNull("group entries is not set yet", testProviderService.getGroupEntries());
+
+        rep2.setXid(2);
+        controller.processPacket(dpid1, rep2.build());
+        assertNotNull("group entries should be set", testProviderService.getGroupEntries());
+    }
+
+
+
+    @After
+    public void tearDown() {
+        provider.deactivate();
+        provider.providerRegistry = null;
+        provider.controller = null;
+    }
+
+    private class TestOpenFlowGroupProviderService
+            extends AbstractProviderService<GroupProvider>
+            implements GroupProviderService {
+
+        Collection<Group> groups = null;
+        GroupOperation failedOperation = null;
+
+        protected TestOpenFlowGroupProviderService(GroupProvider provider) {
+            super(provider);
+        }
+
+        @Override
+        public void groupOperationFailed(GroupOperation operation) {
+            this.failedOperation = operation;
+        }
+
+        @Override
+        public void pushGroupMetrics(DeviceId deviceId, Collection<Group> groupEntries) {
+            this.groups = groupEntries;
+        }
+
+        public Collection<Group> getGroupEntries() {
+            return groups;
+        }
+    }
+
+    private class TestController implements OpenFlowController {
+
+        OpenFlowEventListener eventListener = null;
+        List<OpenFlowSwitch> switches = Lists.newArrayList();
+
+        public TestController() {
+            OpenFlowSwitch testSwitch = new TestOpenFlowSwitch();
+            switches.add(testSwitch);
+        }
+
+        @Override
+        public void addListener(OpenFlowSwitchListener listener) {
+        }
+
+        @Override
+        public void removeListener(OpenFlowSwitchListener listener) {
+
+        }
+
+        @Override
+        public void addPacketListener(int priority, PacketListener listener) {
+
+        }
+
+        @Override
+        public void removePacketListener(PacketListener listener) {
+
+        }
+
+        @Override
+        public void addEventListener(OpenFlowEventListener listener) {
+            this.eventListener = listener;
+        }
+
+        @Override
+        public void removeEventListener(OpenFlowEventListener listener) {
+
+        }
+
+        @Override
+        public void write(Dpid dpid, OFMessage msg) {
+
+        }
+
+        @Override
+        public void processPacket(Dpid dpid, OFMessage msg) {
+            eventListener.handleMessage(dpid, msg);
+        }
+
+        @Override
+        public void setRole(Dpid dpid, RoleState role) {
+
+        }
+
+        @Override
+        public Iterable<OpenFlowSwitch> getSwitches() {
+            return switches;
+        }
+
+        @Override
+        public Iterable<OpenFlowSwitch> getMasterSwitches() {
+            return null;
+        }
+
+        @Override
+        public Iterable<OpenFlowSwitch> getEqualSwitches() {
+            return null;
+        }
+
+        @Override
+        public OpenFlowSwitch getSwitch(Dpid dpid) {
+            return switches.get(0);
+        }
+
+        @Override
+        public OpenFlowSwitch getMasterSwitch(Dpid dpid) {
+            return null;
+        }
+
+        @Override
+        public OpenFlowSwitch getEqualSwitch(Dpid dpid) {
+            return null;
+        }
+
+    }
+
+    private class TestGroupProviderRegistry implements GroupProviderRegistry {
+
+        @Override
+        public GroupProviderService register(GroupProvider provider) {
+            providerService = new TestOpenFlowGroupProviderService(provider);
+            return providerService;
+        }
+
+        @Override
+        public void unregister(GroupProvider provider) {
+        }
+
+        @Override
+        public Set<ProviderId> getProviders() {
+            return null;
+        }
+    }
+
+    private class TestOpenFlowSwitch implements OpenFlowSwitch {
+
+        OFMessage msg = null;
+
+        @Override
+        public void sendMsg(OFMessage msg) {
+            this.msg = msg;
+        }
+
+        @Override
+        public void sendMsg(List<OFMessage> msgs) {
+
+        }
+
+        @Override
+        public void sendMsg(OFMessage msg, TableType tableType) {
+
+        }
+
+        @Override
+        public void handleMessage(OFMessage fromSwitch) {
+
+        }
+
+        @Override
+        public void setRole(RoleState role) {
+
+        }
+
+        @Override
+        public RoleState getRole() {
+            return null;
+        }
+
+        @Override
+        public List<OFPortDesc> getPorts() {
+            return null;
+        }
+
+        @Override
+        public OFFactory factory() {
+            return OFFactories.getFactory(OFVersion.OF_13);
+        }
+
+        @Override
+        public String getStringId() {
+            return null;
+        }
+
+        @Override
+        public long getId() {
+            return 0;
+        }
+
+        @Override
+        public String manufacturerDescription() {
+            return null;
+        }
+
+        @Override
+        public String datapathDescription() {
+            return null;
+        }
+
+        @Override
+        public String hardwareDescription() {
+            return null;
+        }
+
+        @Override
+        public String softwareDescription() {
+            return null;
+        }
+
+        @Override
+        public String serialNumber() {
+            return null;
+        }
+
+        @Override
+        public boolean isConnected() {
+            return false;
+        }
+
+        @Override
+        public void disconnectSwitch() {
+
+        }
+
+        @Override
+        public void returnRoleReply(RoleState requested, RoleState response) {
+
+        }
+
+        @Override
+        public boolean isOptical() {
+            return false;
+        }
+
+        @Override
+        public String channelId() {
+            return null;
+        }
+    }
+}
\ No newline at end of file