ONOS-973: Implemented a unit test for OpenFlowGroupProvider implmentation.
- Fixed a bug in OpenFlowGroupProvider of wrong handling of pending group operations.
Change-Id: I70b80629f4eed000110d242f3558abe49b6b13bc
diff --git a/providers/openflow/group/src/main/java/org/onosproject/provider/of/group/impl/GroupStatsCollector.java b/providers/openflow/group/src/main/java/org/onosproject/provider/of/group/impl/GroupStatsCollector.java
index 90c700e..dca42a9 100644
--- a/providers/openflow/group/src/main/java/org/onosproject/provider/of/group/impl/GroupStatsCollector.java
+++ b/providers/openflow/group/src/main/java/org/onosproject/provider/of/group/impl/GroupStatsCollector.java
@@ -28,7 +28,6 @@
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 @@
private final int refreshInterval;
private Timeout timeout;
- private final AtomicLong xidCounter = new AtomicLong(1);
private boolean stopTimer = false;
@@ -79,7 +77,7 @@
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)
diff --git a/providers/openflow/group/src/main/java/org/onosproject/provider/of/group/impl/OpenFlowGroupProvider.java b/providers/openflow/group/src/main/java/org/onosproject/provider/of/group/impl/OpenFlowGroupProvider.java
index a477c39..1c9f801 100644
--- a/providers/openflow/group/src/main/java/org/onosproject/provider/of/group/impl/OpenFlowGroupProvider.java
+++ b/providers/openflow/group/src/main/java/org/onosproject/provider/of/group/impl/OpenFlowGroupProvider.java
@@ -83,12 +83,15 @@
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 @@
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 @@
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,23 +166,25 @@
OFGroupStatsReply groupStatsReply = null;
OFGroupDescStatsReply groupDescStatsReply = null;
- if (statsReply.getStatsType() == OFStatsType.GROUP) {
- OFStatsReply reply = groupStats.get(statsReply.getXid() + 1);
- if (reply != null) {
- groupStatsReply = (OFGroupStatsReply) statsReply;
- groupDescStatsReply = (OFGroupDescStatsReply) reply;
- groupStats.remove(statsReply.getXid() + 1);
- } else {
- groupStats.put(statsReply.getXid(), statsReply);
- }
- } else if (statsReply.getStatsType() == OFStatsType.GROUP_DESC) {
- OFStatsReply reply = groupStats.get(statsReply.getXid() - 1);
- if (reply != null) {
- groupStatsReply = (OFGroupStatsReply) reply;
- groupDescStatsReply = (OFGroupDescStatsReply) statsReply;
- groupStats.remove(statsReply.getXid() - 1);
- } else {
- groupStats.put(statsReply.getXid(), statsReply);
+ synchronized (groupStats) {
+ if (statsReply.getStatsType() == OFStatsType.GROUP) {
+ OFStatsReply reply = groupStats.get(statsReply.getXid() + 1);
+ if (reply != null) {
+ groupStatsReply = (OFGroupStatsReply) statsReply;
+ groupDescStatsReply = (OFGroupDescStatsReply) reply;
+ groupStats.remove(statsReply.getXid() + 1);
+ } else {
+ groupStats.put(statsReply.getXid(), statsReply);
+ }
+ } else if (statsReply.getStatsType() == OFStatsType.GROUP_DESC) {
+ OFStatsReply reply = groupStats.get(statsReply.getXid() - 1);
+ if (reply != null) {
+ groupStatsReply = (OFGroupStatsReply) reply;
+ groupDescStatsReply = (OFGroupDescStatsReply) statsReply;
+ groupStats.remove(statsReply.getXid() - 1);
+ } else {
+ groupStats.put(statsReply.getXid(), statsReply);
+ }
}
}
@@ -187,6 +194,7 @@
providerService.pushGroupMetrics(deviceId, groups);
for (Group group: groups) {
pendingGroupOperations.remove(group.id());
+ pendingXidMaps.remove(group.id());
}
}
}
@@ -238,6 +246,17 @@
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 @@
case ERROR:
OFErrorMsg errorMsg = (OFErrorMsg) msg;
if (errorMsg.getErrType() == OFErrorType.GROUP_MOD_FAILED) {
- GroupOperation operation =
- pendingGroupOperations.get(errorMsg.getXid());
- if (operation != null) {
- providerService.groupOperationFailed(operation);
- log.warn("received Error message {} from {}", msg, dpid);
+ 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(pendingGroupId);
+ if (operation != null) {
+ providerService.groupOperationFailed(operation);
+ 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;
}
diff --git a/providers/openflow/group/src/test/java/org/onosproject/provider/of/group/impl/OpenFlowGroupProviderTest.java b/providers/openflow/group/src/test/java/org/onosproject/provider/of/group/impl/OpenFlowGroupProviderTest.java
new file mode 100644
index 0000000..dd6d9ff
--- /dev/null
+++ b/providers/openflow/group/src/test/java/org/onosproject/provider/of/group/impl/OpenFlowGroupProviderTest.java
@@ -0,0 +1,402 @@
+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