[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

@@ -235,6 +235,13 @@ public final class DefaultGroupBucket implements GroupBucket, StoredGroupBucketE
     }
 
     @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)

@@ -77,4 +77,12 @@ public interface GroupBucket {
      * @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);
 }

@@ -319,34 +319,52 @@ public class SimpleGroupStore
     }
 
     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;
         }

@@ -130,6 +130,18 @@ public class SimpleGroupStoreTest {
             } 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 @@ public class SimpleGroupStoreTest {
                                                 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

@@ -719,32 +719,50 @@ public class DistributedGroupStore
     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;
         }

@@ -60,6 +60,7 @@ import static org.hamcrest.Matchers.instanceOf;
 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 class DistributedGroupStoreTest {
     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 @@ public class DistributedGroupStoreTest {
         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