Save developers from wasting hours on wrong bucket type
Enforce bucket type to be consistent with group type
Update unit tests accordingly
Change-Id: Ia5f56ca4b5445268ebee09e321c34ed3f3f39827
(cherry picked from commit 5c00be8b143eb73d42b98820641da5c20e458579)
diff --git a/core/api/src/main/java/org/onosproject/net/group/DefaultGroupDescription.java b/core/api/src/main/java/org/onosproject/net/group/DefaultGroupDescription.java
index e047062..4322482 100644
--- a/core/api/src/main/java/org/onosproject/net/group/DefaultGroupDescription.java
+++ b/core/api/src/main/java/org/onosproject/net/group/DefaultGroupDescription.java
@@ -65,7 +65,8 @@
if (this.type == GroupDescription.Type.INDIRECT) {
checkArgument(buckets.buckets().size() == 1, "Indirect group " +
"should have only one action bucket");
- }
+ }
+ checkArgument(buckets.buckets().stream().allMatch(b -> b.type() == type), "Inconsistent bucket type");
this.appCookie = appCookie;
this.givenGroupId = groupId;
this.appId = appId;
diff --git a/core/api/src/test/java/org/onosproject/net/group/DefaultGroupDescriptionTest.java b/core/api/src/test/java/org/onosproject/net/group/DefaultGroupDescriptionTest.java
index 0a0a093..3d2d368 100644
--- a/core/api/src/test/java/org/onosproject/net/group/DefaultGroupDescriptionTest.java
+++ b/core/api/src/test/java/org/onosproject/net/group/DefaultGroupDescriptionTest.java
@@ -16,6 +16,8 @@
package org.onosproject.net.group;
import org.junit.Test;
+import org.onosproject.core.GroupId;
+import org.onosproject.net.PortNumber;
import org.onosproject.net.flow.DefaultTrafficTreatment;
import org.onosproject.net.flow.TrafficTreatment;
@@ -32,28 +34,31 @@
* Default group description unit tests.
*/
public class DefaultGroupDescriptionTest {
- byte[] keyData = "abcdefg".getBytes();
+ private final byte[] keyData = "abcdefg".getBytes();
private final GroupKey key = new DefaultGroupKey(keyData);
- private final TrafficTreatment treatment =
- DefaultTrafficTreatment.emptyTreatment();
- private final GroupBucket bucket =
- DefaultGroupBucket.createSelectGroupBucket(treatment);
- private final GroupBuckets groupBuckets =
- new GroupBuckets(ImmutableList.of(bucket));
+ private final GroupId groupId1 = new GroupId(1);
+ private final TrafficTreatment treatment = DefaultTrafficTreatment.emptyTreatment();
+
+ private final GroupBucket failoverGroupBucket =
+ DefaultGroupBucket.createFailoverGroupBucket(treatment, PortNumber.IN_PORT, groupId1);
+ private final GroupBuckets failoverGroupBuckets = new GroupBuckets(ImmutableList.of(failoverGroupBucket));
+ private final GroupBucket indirectGroupBucket =
+ DefaultGroupBucket.createIndirectGroupBucket(treatment);
+ private final GroupBuckets indirectGroupBuckets = new GroupBuckets(ImmutableList.of(indirectGroupBucket));
+
private final DefaultGroupDescription d1 =
new DefaultGroupDescription(did("2"),
GroupDescription.Type.FAILOVER,
- groupBuckets);
- private final DefaultGroupDescription sameAsD1 =
- new DefaultGroupDescription(d1);
+ failoverGroupBuckets);
+ private final DefaultGroupDescription sameAsD1 = new DefaultGroupDescription(d1);
private final DefaultGroupDescription d2 =
new DefaultGroupDescription(did("2"),
GroupDescription.Type.INDIRECT,
- groupBuckets);
+ indirectGroupBuckets);
private final DefaultGroupDescription d3 =
new DefaultGroupDescription(did("3"),
GroupDescription.Type.FAILOVER,
- groupBuckets,
+ failoverGroupBuckets,
key,
711,
APP_ID);
@@ -86,7 +91,7 @@
public void testConstruction() {
assertThat(d3.deviceId(), is(did("3")));
assertThat(d3.type(), is(GroupDescription.Type.FAILOVER));
- assertThat(d3.buckets(), is(groupBuckets));
+ assertThat(d3.buckets(), is(failoverGroupBuckets));
assertThat(d3.appId(), is(APP_ID));
assertThat(d3.givenGroupId(), is(711));
assertThat(key.key(), is(keyData));
diff --git a/core/api/src/test/java/org/onosproject/net/group/DefaultGroupTest.java b/core/api/src/test/java/org/onosproject/net/group/DefaultGroupTest.java
index e8fb7b3..c84ac87 100644
--- a/core/api/src/test/java/org/onosproject/net/group/DefaultGroupTest.java
+++ b/core/api/src/test/java/org/onosproject/net/group/DefaultGroupTest.java
@@ -18,6 +18,7 @@
import org.junit.Test;
import org.onosproject.core.GroupId;
import org.onosproject.net.NetTestTools;
+import org.onosproject.net.PortNumber;
import org.onosproject.net.flow.DefaultTrafficTreatment;
import com.google.common.collect.ImmutableList;
@@ -35,24 +36,20 @@
private final GroupId id2 = new GroupId(7);
private final GroupId id3 = new GroupId(1234);
- private final GroupBucket bucket =
- DefaultGroupBucket.createSelectGroupBucket(
- DefaultTrafficTreatment.emptyTreatment());
- private final GroupBuckets groupBuckets =
- new GroupBuckets(ImmutableList.of(bucket));
- private final GroupDescription groupDesc1 =
- new DefaultGroupDescription(did("1"),
- GroupDescription.Type.FAILOVER,
- groupBuckets);
- private final GroupDescription groupDesc2 =
- new DefaultGroupDescription(did("2"),
- GroupDescription.Type.FAILOVER,
- groupBuckets);
+ private final GroupBucket failoverBucket = DefaultGroupBucket.createFailoverGroupBucket(
+ DefaultTrafficTreatment.emptyTreatment(), PortNumber.IN_PORT, id1);
+ private final GroupBuckets failoverGroupBuckets = new GroupBuckets(ImmutableList.of(failoverBucket));
- private final GroupDescription groupDesc3 =
- new DefaultGroupDescription(did("3"),
- GroupDescription.Type.INDIRECT,
- groupBuckets);
+ private final GroupBucket indirectBucket =
+ DefaultGroupBucket.createIndirectGroupBucket(DefaultTrafficTreatment.emptyTreatment());
+ private final GroupBuckets indirectGroupBuckets = new GroupBuckets(ImmutableList.of(indirectBucket));
+
+ private final GroupDescription groupDesc1 =
+ new DefaultGroupDescription(did("1"), GroupDescription.Type.FAILOVER, failoverGroupBuckets);
+ private final GroupDescription groupDesc2 =
+ new DefaultGroupDescription(did("2"), GroupDescription.Type.FAILOVER, failoverGroupBuckets);
+ private final GroupDescription groupDesc3 =
+ new DefaultGroupDescription(did("3"), GroupDescription.Type.INDIRECT, indirectGroupBuckets);
DefaultGroup group1 = new DefaultGroup(id1, groupDesc1);
DefaultGroup sameAsGroup1 = new DefaultGroup(id1, groupDesc1);
@@ -83,7 +80,7 @@
assertThat(group1.life(), is(0L));
assertThat(group1.packets(), is(0L));
assertThat(group1.referenceCount(), is(0L));
- assertThat(group1.buckets(), is(groupBuckets));
+ assertThat(group1.buckets(), is(failoverGroupBuckets));
assertThat(group1.state(), is(Group.GroupState.PENDING_ADD));
assertThat(group1.failedRetryCount(), is(0));
}
@@ -94,14 +91,14 @@
@Test
public void checkConstructionWithDid() {
DefaultGroup group = new DefaultGroup(id2, NetTestTools.did("1"),
- GroupDescription.Type.ALL, groupBuckets);
+ GroupDescription.Type.FAILOVER, failoverGroupBuckets);
assertThat(group.id(), is(id2));
assertThat(group.bytes(), is(0L));
assertThat(group.life(), is(0L));
assertThat(group.packets(), is(0L));
assertThat(group.referenceCount(), is(0L));
assertThat(group.deviceId(), is(NetTestTools.did("1")));
- assertThat(group.buckets(), is(groupBuckets));
+ assertThat(group.buckets(), is(failoverGroupBuckets));
assertThat(group.failedRetryCount(), is(0));
}
diff --git a/core/common/src/test/java/org/onosproject/codec/impl/GroupCodecTest.java b/core/common/src/test/java/org/onosproject/codec/impl/GroupCodecTest.java
index 0867213..19c9671 100644
--- a/core/common/src/test/java/org/onosproject/codec/impl/GroupCodecTest.java
+++ b/core/common/src/test/java/org/onosproject/codec/impl/GroupCodecTest.java
@@ -76,23 +76,22 @@
@Test
public void codecEncodeTest() {
- GroupBucket bucket1 = DefaultGroupBucket
- .createSelectGroupBucket(DefaultTrafficTreatment.emptyTreatment());
- GroupBucket bucket2 = DefaultGroupBucket
- .createIndirectGroupBucket(DefaultTrafficTreatment.emptyTreatment());
- GroupBuckets buckets = new GroupBuckets(ImmutableList.of(bucket1, bucket2));
- GroupBuckets bucketsIndirect = new GroupBuckets(ImmutableList.of(bucket2));
+ GroupBucket bucket1 = DefaultGroupBucket.createAllGroupBucket(DefaultTrafficTreatment.emptyTreatment());
+ GroupBucket bucket2 = DefaultGroupBucket.createAllGroupBucket(DefaultTrafficTreatment.emptyTreatment());
+ GroupBucket bucket3 = DefaultGroupBucket.createIndirectGroupBucket(DefaultTrafficTreatment.emptyTreatment());
+ GroupBuckets allBuckets = new GroupBuckets(ImmutableList.of(bucket1, bucket2));
+ GroupBuckets indirectBuckets = new GroupBuckets(ImmutableList.of(bucket3));
DefaultGroup group = new DefaultGroup(
new GroupId(1),
NetTestTools.did("d1"),
ALL,
- buckets);
+ allBuckets);
DefaultGroup group1 = new DefaultGroup(
new GroupId(2),
NetTestTools.did("d2"),
INDIRECT,
- bucketsIndirect);
+ indirectBuckets);
MockCodecContext context = new MockCodecContext();
GroupCodec codec = new GroupCodec();
diff --git a/core/common/src/test/java/org/onosproject/utils/ComparatorsTest.java b/core/common/src/test/java/org/onosproject/utils/ComparatorsTest.java
index 6047719..d616e71 100644
--- a/core/common/src/test/java/org/onosproject/utils/ComparatorsTest.java
+++ b/core/common/src/test/java/org/onosproject/utils/ComparatorsTest.java
@@ -129,8 +129,7 @@
private final ConnectPoint cp =
new ConnectPoint(DeviceId.deviceId("of:00001"), PortNumber.portNumber(100));
private final GroupBucket testBucket =
- DefaultGroupBucket.createSelectGroupBucket(
- DefaultTrafficTreatment.emptyTreatment());
+ DefaultGroupBucket.createAllGroupBucket(DefaultTrafficTreatment.emptyTreatment());
private final GroupBuckets groupBuckets =
new GroupBuckets(ImmutableList.of(testBucket));
private final GroupDescription groupDesc1 =
diff --git a/core/store/dist/src/test/java/org/onosproject/store/group/impl/DistributedGroupStoreTest.java b/core/store/dist/src/test/java/org/onosproject/store/group/impl/DistributedGroupStoreTest.java
index 0a44359..087bd80 100644
--- a/core/store/dist/src/test/java/org/onosproject/store/group/impl/DistributedGroupStoreTest.java
+++ b/core/store/dist/src/test/java/org/onosproject/store/group/impl/DistributedGroupStoreTest.java
@@ -61,11 +61,11 @@
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.nullValue;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
import static org.onosproject.net.NetTestTools.APP_ID;
import static org.onosproject.net.NetTestTools.did;
import static org.onosproject.net.group.GroupDescription.Type.ALL;
import static org.onosproject.net.group.GroupDescription.Type.INDIRECT;
-import static org.onosproject.net.group.GroupDescription.Type.SELECT;
import static org.onosproject.net.group.GroupStore.UpdateType.ADD;
import static org.onosproject.net.group.GroupStore.UpdateType.SET;
/**
@@ -73,49 +73,49 @@
*/
public class DistributedGroupStoreTest {
- DeviceId deviceId1 = did("dev1");
- DeviceId deviceId2 = did("dev2");
- GroupId groupId1 = new GroupId(1);
- GroupId groupId2 = new GroupId(2);
- GroupId groupId3 = new GroupId(3);
- GroupKey groupKey1 = new DefaultGroupKey("abc".getBytes());
- GroupKey groupKey2 = new DefaultGroupKey("def".getBytes());
- GroupKey groupKey3 = new DefaultGroupKey("ghi".getBytes());
+ private final DeviceId deviceId1 = did("dev1");
+ private final DeviceId deviceId2 = did("dev2");
+ private final GroupId groupId1 = new GroupId(1);
+ private final GroupId groupId2 = new GroupId(2);
+ private final GroupId groupId3 = new GroupId(3);
+ private final GroupKey groupKey1 = new DefaultGroupKey("abc".getBytes());
+ private final GroupKey groupKey2 = new DefaultGroupKey("def".getBytes());
+ private final GroupKey groupKey3 = new DefaultGroupKey("ghi".getBytes());
- TrafficTreatment treatment =
- DefaultTrafficTreatment.emptyTreatment();
- GroupBucket selectGroupBucket =
- DefaultGroupBucket.createSelectGroupBucket(treatment);
- GroupBucket failoverGroupBucket =
- DefaultGroupBucket.createFailoverGroupBucket(treatment,
- PortNumber.IN_PORT, groupId1);
+ private final TrafficTreatment treatment = DefaultTrafficTreatment.emptyTreatment();
+ private final TrafficTreatment treatment2 = DefaultTrafficTreatment.builder()
+ .setOutput(PortNumber.portNumber(2)).build();
+ private final GroupBucket allGroupBucket = DefaultGroupBucket.createAllGroupBucket(treatment);
+ private final GroupBucket allGroupBucket2 = DefaultGroupBucket.createAllGroupBucket(treatment2);
+ private final GroupBuckets allGroupBuckets = new GroupBuckets(ImmutableList.of(allGroupBucket));
+ private final GroupBucket indirectGroupBucket = DefaultGroupBucket.createIndirectGroupBucket(treatment);
+ private final GroupBuckets indirectGroupBuckets = new GroupBuckets(ImmutableList.of(indirectGroupBucket));
- GroupBuckets buckets = new GroupBuckets(ImmutableList.of(selectGroupBucket));
- GroupDescription groupDescription1 = new DefaultGroupDescription(
+ private final GroupDescription groupDescription1 = new DefaultGroupDescription(
deviceId1,
ALL,
- buckets,
+ allGroupBuckets,
groupKey1,
groupId1.id(),
APP_ID);
- GroupDescription groupDescription2 = new DefaultGroupDescription(
+ private final GroupDescription groupDescription2 = new DefaultGroupDescription(
deviceId2,
INDIRECT,
- buckets,
+ indirectGroupBuckets,
groupKey2,
groupId2.id(),
APP_ID);
- GroupDescription groupDescription3 = new DefaultGroupDescription(
+ private final GroupDescription groupDescription3 = new DefaultGroupDescription(
deviceId2,
INDIRECT,
- buckets,
+ indirectGroupBuckets,
groupKey3,
groupId3.id(),
APP_ID);
- DistributedGroupStore groupStoreImpl;
- GroupStore groupStore;
- ConsistentMap auditPendingReqQueue;
+ private DistributedGroupStore groupStoreImpl;
+ private GroupStore groupStore;
+ private ConsistentMap auditPendingReqQueue;
static class MasterOfAll extends MastershipServiceAdapter {
@Override
@@ -285,8 +285,8 @@
GroupDescription groupDescription3 = new DefaultGroupDescription(
deviceId1,
- SELECT,
- buckets,
+ ALL,
+ allGroupBuckets,
new DefaultGroupKey("aaa".getBytes()),
null,
APP_ID);
@@ -347,7 +347,7 @@
GroupOperation opAdd =
GroupOperation.createAddGroupOperation(groupId1,
INDIRECT,
- buckets);
+ indirectGroupBuckets);
groupStore.groupOperationFailed(deviceId1, opAdd);
List<GroupEvent> eventsAfterAddFailed = delegate.eventsSeen();
@@ -361,7 +361,7 @@
GroupOperation opModify =
GroupOperation.createModifyGroupOperation(groupId2,
INDIRECT,
- buckets);
+ indirectGroupBuckets);
groupStore.groupOperationFailed(deviceId2, opModify);
List<GroupEvent> eventsAfterModifyFailed = delegate.eventsSeen();
assertThat(eventsAfterModifyFailed, hasSize(1));
@@ -400,7 +400,7 @@
// test group exists
GroupOperation opAdd = GroupOperation
- .createAddGroupOperation(groupId1, INDIRECT, buckets);
+ .createAddGroupOperation(groupId1, ALL, allGroupBuckets);
GroupOperation addFailedExists = GroupOperation
.createFailedGroupOperation(opAdd, GroupMsgErrorCode.GROUP_EXISTS);
groupStore.groupOperationFailed(deviceId1, addFailedExists);
@@ -420,7 +420,7 @@
assertEquals(0, g2.failedRetryCount());
assertEquals(GroupState.PENDING_ADD, g2.state());
GroupOperation opAdd1 = GroupOperation
- .createAddGroupOperation(groupId2, INDIRECT, buckets);
+ .createAddGroupOperation(groupId2, INDIRECT, indirectGroupBuckets);
GroupOperation addFailedInvalid = GroupOperation
.createFailedGroupOperation(opAdd1, GroupMsgErrorCode.INVALID_GROUP);
@@ -487,9 +487,7 @@
*/
@Test
public void testUpdateGroupDescription() {
-
- GroupBuckets buckets =
- new GroupBuckets(ImmutableList.of(failoverGroupBucket, selectGroupBucket));
+ GroupBuckets buckets = new GroupBuckets(ImmutableList.of(allGroupBucket2));
groupStore.deviceInitialAuditCompleted(deviceId1, true);
groupStore.storeGroupDescription(groupDescription1);
@@ -504,40 +502,31 @@
assertThat(group1.appCookie(), is(newKey));
assertThat(group1.buckets().buckets(), hasSize(2));
- short weight = 5;
- GroupBucket selectGroupBucketWithWeight =
- DefaultGroupBucket.createSelectGroupBucket(treatment, weight);
- buckets = new GroupBuckets(ImmutableList.of(failoverGroupBucket,
- selectGroupBucketWithWeight));
-
+ buckets = new GroupBuckets(ImmutableList.of(allGroupBucket, allGroupBucket2));
groupStore.updateGroupDescription(deviceId1,
newKey,
ADD,
buckets,
newKey);
-
group1 = groupStore.getGroup(deviceId1, groupId1);
assertThat(group1.appCookie(), is(newKey));
assertThat(group1.buckets().buckets(), hasSize(2));
for (GroupBucket bucket : group1.buckets().buckets()) {
- if (bucket.type() == SELECT) {
- assertEquals(weight, bucket.weight());
- }
+ assertTrue(bucket.treatment().equals(treatment) ||
+ bucket.treatment().equals(treatment2));
}
- buckets = new GroupBuckets(ImmutableList.of(selectGroupBucketWithWeight));
-
+ buckets = new GroupBuckets(ImmutableList.of(allGroupBucket2));
groupStore.updateGroupDescription(deviceId1,
newKey,
SET,
buckets,
newKey);
-
group1 = groupStore.getGroup(deviceId1, groupId1);
assertThat(group1.appCookie(), is(newKey));
assertThat(group1.buckets().buckets(), hasSize(1));
GroupBucket onlyBucket = group1.buckets().buckets().iterator().next();
- assertEquals(weight, onlyBucket.weight());
+ assertEquals(treatment2, onlyBucket.treatment());
}
@Test