VOL-2575 Add/RemoveMembers operations should be used to update buckets of
a group instead of SetMembers operation. However, these operations cannot
be executed in BAL 3.1 due to the situation indicated in VOL-2461.
As we have moved to 3.2, we shall use add/remove commands to update buckets
of a group.

Change-Id: I2ef603d0df5f82ab93effeb529ed04a3d614d50b
diff --git a/VERSION b/VERSION
index f90b1af..0bee604 100644
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-2.3.2
+2.3.3
diff --git a/adaptercore/openolt_flowmgr.go b/adaptercore/openolt_flowmgr.go
index 892663f..c817455 100644
--- a/adaptercore/openolt_flowmgr.go
+++ b/adaptercore/openolt_flowmgr.go
@@ -2006,56 +2006,64 @@
 		return false
 	}
 
-	var current *openoltpb2.Group
+	var current *openoltpb2.Group // represents the group on the device
 	if groupExists {
 		// group already exists
 		current = f.buildGroup(group.Desc.GroupId, val.Desc.GetBuckets())
-		log.Debugw("modify-group: group exists.", log.Fields{"current": val, "new": group})
+		log.Debugw("modify-group: group exists.", log.Fields{"group on the device": val, "new": group})
 	} else {
 		current = f.buildGroup(group.Desc.GroupId, nil)
 	}
 
-	log.Debugw("modify-group: comparing current and new.", log.Fields{"current": current, "new": new})
-	// check if the buckets are identical
-	bucketsIdentical := f.bucketsIdentical(current, new)
+	log.Debugw("modify-group: comparing current and new.", log.Fields{"group on the device": current, "new": new})
+	// get members to be added
+	membersToBeAdded := f.findDiff(current, new)
+	// get members to be removed
+	membersToBeRemoved := f.findDiff(new, current)
 
-	isSuccess := true
-	if !bucketsIdentical {
-		groupToOlt := openoltpb2.Group{
-			GroupId: group.Desc.GroupId,
-			Command: openoltpb2.Group_SET_MEMBERS,
-			Members: new.Members,
-			Action:  f.buildGroupAction(),
-		}
+	log.Infow("modify-group -> differences found", log.Fields{"membersToBeAdded": membersToBeAdded,
+		"membersToBeRemoved": membersToBeRemoved, "groupId": group.Desc.GroupId})
 
-		if err := f.callGroupAdd(&groupToOlt); err != nil {
-			log.Warnw("One of the group add/remove operations has failed. Cannot save group modifications",
-				log.Fields{"group": group})
-			isSuccess = false
-		}
+	groupToOlt := openoltpb2.Group{
+		GroupId: group.Desc.GroupId,
+	}
+	var added, removed = true, true
+	if membersToBeAdded != nil && len(membersToBeAdded) > 0 {
+		groupToOlt.Command = openoltpb2.Group_ADD_MEMBERS
+		groupToOlt.Members = membersToBeAdded
+		//execute addMembers
+		added = f.callGroupAddRemove(&groupToOlt)
+	}
+	if membersToBeRemoved != nil && len(membersToBeRemoved) > 0 {
+		groupToOlt.Command = openoltpb2.Group_REMOVE_MEMBERS
+		groupToOlt.Members = membersToBeRemoved
+		//execute removeMembers
+		removed = f.callGroupAddRemove(&groupToOlt)
 	}
 
-	if isSuccess {
+	//save the modified group
+	if added && removed {
 		if err := f.resourceMgr.AddFlowGroupToKVStore(ctx, group, false); err != nil {
 			log.Errorw("Failed to save the group into kv store", log.Fields{"groupId": group.Desc.GroupId})
 		}
 		log.Debugw("modify-group was success. Storing the group", log.Fields{"group": group, "existingGroup": current})
+	} else {
+		log.Warnw("One of the group add/remove operations has failed. Cannot save group modifications",
+			log.Fields{"group": group})
 	}
-	return isSuccess
+	return added && removed
 }
 
-//bucketsIdentical returns true if groups are identical; false otherwise
-func (f *OpenOltFlowMgr) bucketsIdentical(current *openoltpb2.Group, new *openoltpb2.Group) bool {
-	if current.GroupId == new.GroupId &&
-		len(new.Members) == len(current.Members) {
-		diff := f.findDiff(current, new)
-		if diff == nil || len(diff) < 1 {
-			log.Infow("modify-group: current and new buckets are the same. Won't send SET_MEMBERS again.",
-				log.Fields{"groupId:": current.GroupId})
-			return true
+//callGroupAddRemove performs add/remove buckets operation for the indicated group
+func (f *OpenOltFlowMgr) callGroupAddRemove(group *openoltpb2.Group) bool {
+	if err := f.performGroupOperation(group); err != nil {
+		st, _ := status.FromError(err)
+		//ignore already exists error code
+		if st.Code() != codes.AlreadyExists {
+			return false
 		}
 	}
-	return false
+	return true
 }
 
 //findDiff compares group members and finds members which only exists in groups2
@@ -2080,8 +2088,8 @@
 	return false
 }
 
-//callGroupAdd call GroupAdd operation of openolt proto
-func (f *OpenOltFlowMgr) callGroupAdd(group *openoltpb2.Group) error {
+//performGroupOperation call performGroupOperation operation of openolt proto
+func (f *OpenOltFlowMgr) performGroupOperation(group *openoltpb2.Group) error {
 	log.Debugw("Sending group to device", log.Fields{"groupToOlt": group, "command": group.Command})
 	_, err := f.deviceHandler.Client.PerformGroupOperation(context.Background(), group)
 	if err != nil {
@@ -2931,13 +2939,13 @@
 	groupEntry := ofp.OfpGroupEntry{
 		Desc: &groupDesc,
 	}
-	var acts []*ofp.OfpAction
 	for i := 0; i < len(outPorts); i++ {
+		var acts []*ofp.OfpAction
 		acts = append(acts, flows.Output(outPorts[i]))
+		bucket := ofp.OfpBucket{
+			Actions: acts,
+		}
+		groupDesc.Buckets = append(groupDesc.Buckets, &bucket)
 	}
-	bucket := ofp.OfpBucket{
-		Actions: acts,
-	}
-	groupDesc.Buckets = []*ofp.OfpBucket{&bucket}
 	return &groupEntry
 }
diff --git a/adaptercore/resourcemanager/resourcemanager_test.go b/adaptercore/resourcemanager/resourcemanager_test.go
index a5e97db..6fd3438 100644
--- a/adaptercore/resourcemanager/resourcemanager_test.go
+++ b/adaptercore/resourcemanager/resourcemanager_test.go
@@ -1073,14 +1073,14 @@
 	groupEntry := ofp.OfpGroupEntry{
 		Desc: &groupDesc,
 	}
-	var acts []*ofp.OfpAction
 	for i := 0; i < len(outPorts); i++ {
+		var acts []*ofp.OfpAction
 		acts = append(acts, fu.Output(outPorts[i]))
+		bucket := ofp.OfpBucket{
+			Actions: acts,
+		}
+		groupDesc.Buckets = append(groupDesc.Buckets, &bucket)
 	}
-	bucket := ofp.OfpBucket{
-		Actions: acts,
-	}
-	groupDesc.Buckets = []*ofp.OfpBucket{&bucket}
 	return &groupEntry
 }