[ONOS-5169] GroupStore: properly add updated buckets
Adding buckets to group was ignoring the addition of groups
with different weights because they had the same treatment
and type. We'll now update such groupbuckets with the desired new
parameters.
Change-Id: I5f102c5fd78912844883c897bd858ee282f3cc12
diff --git a/core/api/src/main/java/org/onosproject/net/group/DefaultGroupBucket.java b/core/api/src/main/java/org/onosproject/net/group/DefaultGroupBucket.java
index 0cdc290..7e5efb8 100644
--- a/core/api/src/main/java/org/onosproject/net/group/DefaultGroupBucket.java
+++ b/core/api/src/main/java/org/onosproject/net/group/DefaultGroupBucket.java
@@ -235,6 +235,13 @@
}
@Override
+ public boolean hasSameParameters(GroupBucket other) {
+ return weight == other.weight() &&
+ Objects.equals(watchPort, other.watchPort()) &&
+ Objects.equals(watchGroup, other.watchGroup());
+ }
+
+ @Override
public String toString() {
return toStringHelper(this)
.add("type", type)
diff --git a/core/api/src/main/java/org/onosproject/net/group/GroupBucket.java b/core/api/src/main/java/org/onosproject/net/group/GroupBucket.java
index 015f3c3..54ef862 100644
--- a/core/api/src/main/java/org/onosproject/net/group/GroupBucket.java
+++ b/core/api/src/main/java/org/onosproject/net/group/GroupBucket.java
@@ -77,4 +77,12 @@
* @return number of bytes
*/
long bytes();
+
+ /**
+ * Returns whether the given GroupBucket has the same parameters (weight,
+ * watchPort and watchGroup) as this.
+ *
+ * @param other GroupBucket to compare
+ */
+ boolean hasSameParameters(GroupBucket other);
}
diff --git a/core/common/src/test/java/org/onosproject/store/trivial/SimpleGroupStore.java b/core/common/src/test/java/org/onosproject/store/trivial/SimpleGroupStore.java
index 9727c90..62c3e30 100644
--- a/core/common/src/test/java/org/onosproject/store/trivial/SimpleGroupStore.java
+++ b/core/common/src/test/java/org/onosproject/store/trivial/SimpleGroupStore.java
@@ -319,34 +319,52 @@
}
private List<GroupBucket> getUpdatedBucketList(Group oldGroup,
- UpdateType type,
- GroupBuckets buckets) {
- GroupBuckets oldBuckets = oldGroup.buckets();
- List<GroupBucket> newBucketList = new ArrayList<>(oldBuckets.buckets());
+ UpdateType type,
+ GroupBuckets buckets) {
+ List<GroupBucket> oldBuckets = oldGroup.buckets().buckets();
+ List<GroupBucket> updatedBucketList = new ArrayList<>();
boolean groupDescUpdated = false;
if (type == UpdateType.ADD) {
- // Check if the any of the new buckets are part of
- // the old bucket list
- for (GroupBucket addBucket:buckets.buckets()) {
- if (!newBucketList.contains(addBucket)) {
- newBucketList.add(addBucket);
- groupDescUpdated = true;
+ List<GroupBucket> newBuckets = buckets.buckets();
+
+ // Add old buckets that will not be updated and check if any will be updated.
+ for (GroupBucket oldBucket : oldBuckets) {
+ int newBucketIndex = newBuckets.indexOf(oldBucket);
+
+ if (newBucketIndex != -1) {
+ GroupBucket newBucket = newBuckets.get(newBucketIndex);
+ if (!newBucket.hasSameParameters(oldBucket)) {
+ // Bucket will be updated
+ groupDescUpdated = true;
+ }
+ } else {
+ // Old bucket will remain the same - add it.
+ updatedBucketList.add(oldBucket);
}
}
+
+ // Add all new buckets
+ updatedBucketList.addAll(newBuckets);
+ if (!oldBuckets.containsAll(newBuckets)) {
+ groupDescUpdated = true;
+ }
+
} else if (type == UpdateType.REMOVE) {
- // Check if the to be removed buckets are part of the
- // old bucket list
- for (GroupBucket removeBucket:buckets.buckets()) {
- if (newBucketList.contains(removeBucket)) {
- newBucketList.remove(removeBucket);
+ List<GroupBucket> bucketsToRemove = buckets.buckets();
+
+ // Check which old buckets should remain
+ for (GroupBucket oldBucket : oldBuckets) {
+ if (!bucketsToRemove.contains(oldBucket)) {
+ updatedBucketList.add(oldBucket);
+ } else {
groupDescUpdated = true;
}
}
}
if (groupDescUpdated) {
- return newBucketList;
+ return updatedBucketList;
} else {
return null;
}
diff --git a/core/common/src/test/java/org/onosproject/store/trivial/SimpleGroupStoreTest.java b/core/common/src/test/java/org/onosproject/store/trivial/SimpleGroupStoreTest.java
index 11ec795..834c8ee 100644
--- a/core/common/src/test/java/org/onosproject/store/trivial/SimpleGroupStoreTest.java
+++ b/core/common/src/test/java/org/onosproject/store/trivial/SimpleGroupStoreTest.java
@@ -130,6 +130,18 @@
} else if (expectedEvent == GroupEvent.Type.GROUP_UPDATE_REQUESTED) {
assertEquals(Group.GroupState.PENDING_UPDATE,
event.subject().state());
+ for (GroupBucket bucket:event.subject().buckets().buckets()) {
+ Optional<GroupBucket> matched = createdBuckets.buckets()
+ .stream()
+ .filter((expected) -> expected.equals(bucket))
+ .findFirst();
+ assertEquals(matched.get().weight(),
+ bucket.weight());
+ assertEquals(matched.get().watchGroup(),
+ bucket.watchGroup());
+ assertEquals(matched.get().watchPort(),
+ bucket.watchPort());
+ }
} else if (expectedEvent == GroupEvent.Type.GROUP_REMOVE_REQUESTED) {
assertEquals(Group.GroupState.PENDING_DELETE,
event.subject().state());
@@ -342,6 +354,36 @@
toAddGroupBuckets,
addKey);
simpleGroupStore.unsetDelegate(updateGroupDescDelegate);
+
+ short weight = 5;
+ toAddBuckets = new ArrayList<>();
+ for (PortNumber portNumber: newOutPorts) {
+ TrafficTreatment.Builder tBuilder = DefaultTrafficTreatment.builder();
+ tBuilder.setOutput(portNumber)
+ .setEthDst(MacAddress.valueOf("00:00:00:00:00:03"))
+ .setEthSrc(MacAddress.valueOf("00:00:00:00:00:01"))
+ .pushMpls()
+ .setMpls(MplsLabel.mplsLabel(106));
+ toAddBuckets.add(DefaultGroupBucket.createSelectGroupBucket(
+ tBuilder.build(), weight));
+ }
+
+ toAddGroupBuckets = new GroupBuckets(toAddBuckets);
+ buckets = new ArrayList<>();
+ buckets.addAll(existingGroup.buckets().buckets());
+ buckets.addAll(toAddBuckets);
+ updatedGroupBuckets = new GroupBuckets(buckets);
+ updateGroupDescDelegate =
+ new InternalGroupStoreDelegate(addKey,
+ updatedGroupBuckets,
+ GroupEvent.Type.GROUP_UPDATE_REQUESTED);
+ simpleGroupStore.setDelegate(updateGroupDescDelegate);
+ simpleGroupStore.updateGroupDescription(D1,
+ addKey,
+ UpdateType.ADD,
+ toAddGroupBuckets,
+ addKey);
+ simpleGroupStore.unsetDelegate(updateGroupDescDelegate);
}
// Testing updateGroupDescription for REMOVE operation from northbound
diff --git a/core/store/dist/src/main/java/org/onosproject/store/group/impl/DistributedGroupStore.java b/core/store/dist/src/main/java/org/onosproject/store/group/impl/DistributedGroupStore.java
index 1b4faa8..ab66bd4 100644
--- a/core/store/dist/src/main/java/org/onosproject/store/group/impl/DistributedGroupStore.java
+++ b/core/store/dist/src/main/java/org/onosproject/store/group/impl/DistributedGroupStore.java
@@ -719,32 +719,50 @@
private List<GroupBucket> getUpdatedBucketList(Group oldGroup,
UpdateType type,
GroupBuckets buckets) {
- GroupBuckets oldBuckets = oldGroup.buckets();
- List<GroupBucket> newBucketList = new ArrayList<>(oldBuckets.buckets());
+ List<GroupBucket> oldBuckets = oldGroup.buckets().buckets();
+ List<GroupBucket> updatedBucketList = new ArrayList<>();
boolean groupDescUpdated = false;
if (type == UpdateType.ADD) {
- // Check if the any of the new buckets are part of
- // the old bucket list
- for (GroupBucket addBucket : buckets.buckets()) {
- if (!newBucketList.contains(addBucket)) {
- newBucketList.add(addBucket);
- groupDescUpdated = true;
+ List<GroupBucket> newBuckets = buckets.buckets();
+
+ // Add old buckets that will not be updated and check if any will be updated.
+ for (GroupBucket oldBucket : oldBuckets) {
+ int newBucketIndex = newBuckets.indexOf(oldBucket);
+
+ if (newBucketIndex != -1) {
+ GroupBucket newBucket = newBuckets.get(newBucketIndex);
+ if (!newBucket.hasSameParameters(oldBucket)) {
+ // Bucket will be updated
+ groupDescUpdated = true;
+ }
+ } else {
+ // Old bucket will remain the same - add it.
+ updatedBucketList.add(oldBucket);
}
}
+
+ // Add all new buckets
+ updatedBucketList.addAll(newBuckets);
+ if (!oldBuckets.containsAll(newBuckets)) {
+ groupDescUpdated = true;
+ }
+
} else if (type == UpdateType.REMOVE) {
- // Check if the to be removed buckets are part of the
- // old bucket list
- for (GroupBucket removeBucket : buckets.buckets()) {
- if (newBucketList.contains(removeBucket)) {
- newBucketList.remove(removeBucket);
+ List<GroupBucket> bucketsToRemove = buckets.buckets();
+
+ // Check which old buckets should remain
+ for (GroupBucket oldBucket : oldBuckets) {
+ if (!bucketsToRemove.contains(oldBucket)) {
+ updatedBucketList.add(oldBucket);
+ } else {
groupDescUpdated = true;
}
}
}
if (groupDescUpdated) {
- return newBucketList;
+ return updatedBucketList;
} else {
return null;
}
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 9a951a6..288d8de 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
@@ -60,6 +60,7 @@
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.nullValue;
+import static org.junit.Assert.assertEquals;
import static org.onosproject.net.NetTestTools.APP_ID;
import static org.onosproject.net.NetTestTools.did;
import static org.onosproject.net.group.GroupDescription.Type.*;
@@ -390,7 +391,7 @@
public void testUpdateGroupDescription() {
GroupBuckets buckets =
- new GroupBuckets(ImmutableList.of(failoverGroupBucket));
+ new GroupBuckets(ImmutableList.of(failoverGroupBucket, selectGroupBucket));
groupStore.deviceInitialAuditCompleted(deviceId1, true);
groupStore.storeGroupDescription(groupDescription1);
@@ -404,6 +405,27 @@
Group group1 = groupStore.getGroup(deviceId1, groupId1);
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));
+
+ 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());
+ }
+ }
}
@Test