Saurav Das
Committed by Gerrit Code Review

Group handling changes:

      Edge case fix in group store where extraneous group buckets are updated so the group can be reused
      Bug fix for broadcast group in OF-DPA as there can only be one l2flood group per vlan
      Group operation failure codes are now reported by the provider to the core
      Group ids are displayed in hex in a couple of places

Change-Id: Ib48d21aa96e0400b77999a1b45839c28f678e524
...@@ -61,7 +61,7 @@ public class DefaultGroupId implements GroupId { ...@@ -61,7 +61,7 @@ public class DefaultGroupId implements GroupId {
61 @Override 61 @Override
62 public String toString() { 62 public String toString() {
63 return MoreObjects.toStringHelper(this) 63 return MoreObjects.toStringHelper(this)
64 - .add("id", id) 64 + .add("id", "0x" + Integer.toHexString(id))
65 .toString(); 65 .toString();
66 } 66 }
67 } 67 }
......
...@@ -576,7 +576,7 @@ public final class Instructions { ...@@ -576,7 +576,7 @@ public final class Instructions {
576 576
577 @Override 577 @Override
578 public String toString() { 578 public String toString() {
579 - return type().toString() + SEPARATOR + Integer.toHexString(groupId.id()); 579 + return type().toString() + SEPARATOR + "0x" + Integer.toHexString(groupId.id());
580 } 580 }
581 581
582 @Override 582 @Override
......
...@@ -31,6 +31,7 @@ public final class GroupOperation { ...@@ -31,6 +31,7 @@ public final class GroupOperation {
31 private final GroupId groupId; 31 private final GroupId groupId;
32 private final GroupDescription.Type groupType; 32 private final GroupDescription.Type groupType;
33 private final GroupBuckets buckets; 33 private final GroupBuckets buckets;
34 + private final GroupMsgErrorCode failureCode;
34 35
35 public enum Type { 36 public enum Type {
36 /** 37 /**
...@@ -48,6 +49,28 @@ public final class GroupOperation { ...@@ -48,6 +49,28 @@ public final class GroupOperation {
48 } 49 }
49 50
50 /** 51 /**
52 + * Possible error codes for a failure of a group operation.
53 + *
54 + */
55 + public enum GroupMsgErrorCode {
56 + GROUP_EXISTS,
57 + INVALID_GROUP,
58 + WEIGHT_UNSUPPORTED,
59 + OUT_OF_GROUPS,
60 + OUT_OF_BUCKETS,
61 + CHAINING_UNSUPPORTED,
62 + WATCH_UNSUPPORTED,
63 + LOOP,
64 + UNKNOWN_GROUP,
65 + CHAINED_GROUP,
66 + BAD_TYPE,
67 + BAD_COMMAND,
68 + BAD_BUCKET,
69 + BAD_WATCH,
70 + EPERM;
71 + }
72 +
73 + /**
51 * Group operation constructor with the parameters. 74 * Group operation constructor with the parameters.
52 * 75 *
53 * @param opType group operation type 76 * @param opType group operation type
...@@ -63,6 +86,23 @@ public final class GroupOperation { ...@@ -63,6 +86,23 @@ public final class GroupOperation {
63 this.groupId = checkNotNull(groupId); 86 this.groupId = checkNotNull(groupId);
64 this.groupType = checkNotNull(groupType); 87 this.groupType = checkNotNull(groupType);
65 this.buckets = buckets; 88 this.buckets = buckets;
89 + this.failureCode = null;
90 + }
91 +
92 + /**
93 + * Group operation copy-constructor with additional field to set failure code.
94 + * Typically used by provider to return information to the core about
95 + * the failure reason for a group operation.
96 + *
97 + * @param groupOp the original group operation
98 + * @param failureCode failure code for a failed group operation
99 + */
100 + private GroupOperation(GroupOperation groupOp, GroupMsgErrorCode failureCode) {
101 + this.opType = groupOp.opType;
102 + this.groupId = groupOp.groupId;
103 + this.groupType = groupOp.groupType;
104 + this.buckets = groupOp.buckets;
105 + this.failureCode = failureCode;
66 } 106 }
67 107
68 /** 108 /**
...@@ -109,6 +149,12 @@ public final class GroupOperation { ...@@ -109,6 +149,12 @@ public final class GroupOperation {
109 149
110 } 150 }
111 151
152 + public static GroupOperation createFailedGroupOperation(GroupOperation groupOperation,
153 + GroupMsgErrorCode failureCode) {
154 + checkNotNull(groupOperation);
155 + return new GroupOperation(groupOperation, failureCode);
156 + }
157 +
112 /** 158 /**
113 * Returns group operation type. 159 * Returns group operation type.
114 * 160 *
...@@ -145,6 +191,15 @@ public final class GroupOperation { ...@@ -145,6 +191,15 @@ public final class GroupOperation {
145 return this.buckets; 191 return this.buckets;
146 } 192 }
147 193
194 + /**
195 + * Returns the failure code representing the failure of a group operation.
196 + *
197 + * @return error code for failure of group operation
198 + */
199 + public GroupMsgErrorCode failureCode() {
200 + return failureCode;
201 + }
202 +
148 @Override 203 @Override
149 /* 204 /*
150 * The deviceId, type and buckets are used for hash. 205 * The deviceId, type and buckets are used for hash.
......
...@@ -88,7 +88,8 @@ import static org.onlab.util.Tools.groupedThreads; ...@@ -88,7 +88,8 @@ import static org.onlab.util.Tools.groupedThreads;
88 import static org.slf4j.LoggerFactory.getLogger; 88 import static org.slf4j.LoggerFactory.getLogger;
89 89
90 /** 90 /**
91 - * Manages inventory of group entries using trivial in-memory implementation. 91 + * Manages inventory of group entries using distributed group stores from the
92 + * storage service.
92 */ 93 */
93 @Component(immediate = true) 94 @Component(immediate = true)
94 @Service 95 @Service
...@@ -450,16 +451,18 @@ public class DistributedGroupStore ...@@ -450,16 +451,18 @@ public class DistributedGroupStore
450 matchingExtraneousGroup = getMatchingExtraneousGroupbyId( 451 matchingExtraneousGroup = getMatchingExtraneousGroupbyId(
451 groupDesc.deviceId(), groupDesc.givenGroupId()); 452 groupDesc.deviceId(), groupDesc.givenGroupId());
452 if (matchingExtraneousGroup != null) { 453 if (matchingExtraneousGroup != null) {
453 - log.debug("storeGroupDescriptionInternal: Matching extraneous group found in Device {} for group id {}", 454 + log.debug("storeGroupDescriptionInternal: Matching extraneous group "
455 + + "found in Device {} for group id 0x{}",
454 groupDesc.deviceId(), 456 groupDesc.deviceId(),
455 - groupDesc.givenGroupId()); 457 + Integer.toHexString(groupDesc.givenGroupId()));
456 //Check if the group buckets matches with user provided buckets 458 //Check if the group buckets matches with user provided buckets
457 if (matchingExtraneousGroup.buckets().equals(groupDesc.buckets())) { 459 if (matchingExtraneousGroup.buckets().equals(groupDesc.buckets())) {
458 //Group is already existing with the same buckets and Id 460 //Group is already existing with the same buckets and Id
459 // Create a group entry object 461 // Create a group entry object
460 - log.debug("storeGroupDescriptionInternal: Buckets also matching in Device {} for group id {}", 462 + log.debug("storeGroupDescriptionInternal: Buckets also matching "
463 + + "in Device {} for group id 0x{}",
461 groupDesc.deviceId(), 464 groupDesc.deviceId(),
462 - groupDesc.givenGroupId()); 465 + Integer.toHexString(groupDesc.givenGroupId()));
463 StoredGroupEntry group = new DefaultGroup( 466 StoredGroupEntry group = new DefaultGroup(
464 matchingExtraneousGroup.id(), groupDesc); 467 matchingExtraneousGroup.id(), groupDesc);
465 // Insert the newly created group entry into key and id maps 468 // Insert the newly created group entry into key and id maps
...@@ -476,10 +479,27 @@ public class DistributedGroupStore ...@@ -476,10 +479,27 @@ public class DistributedGroupStore
476 } else { 479 } else {
477 //Group buckets are not matching. Update group 480 //Group buckets are not matching. Update group
478 //with user provided buckets. 481 //with user provided buckets.
479 - //TODO 482 + log.debug("storeGroupDescriptionInternal: Buckets are not "
480 - log.debug("storeGroupDescriptionInternal: Buckets are not matching in Device {} for group id {}", 483 + + "matching in Device {} for group id 0x{}",
481 groupDesc.deviceId(), 484 groupDesc.deviceId(),
482 - groupDesc.givenGroupId()); 485 + Integer.toHexString(groupDesc.givenGroupId()));
486 + StoredGroupEntry modifiedGroup = new DefaultGroup(
487 + matchingExtraneousGroup.id(), groupDesc);
488 + modifiedGroup.setState(GroupState.PENDING_UPDATE);
489 + getGroupStoreKeyMap().
490 + put(new GroupStoreKeyMapKey(groupDesc.deviceId(),
491 + groupDesc.appCookie()), modifiedGroup);
492 + // Ensure it also inserted into group id based table to
493 + // avoid any chances of duplication in group id generation
494 + getGroupIdTable(groupDesc.deviceId()).
495 + put(matchingExtraneousGroup.id(), modifiedGroup);
496 + removeExtraneousGroupEntry(matchingExtraneousGroup);
497 + log.debug("storeGroupDescriptionInternal: Triggering Group "
498 + + "UPDATE request for {} in device {}",
499 + matchingExtraneousGroup.id(),
500 + groupDesc.deviceId());
501 + notifyDelegate(new GroupEvent(Type.GROUP_UPDATE_REQUESTED, modifiedGroup));
502 + return;
483 } 503 }
484 } 504 }
485 } else { 505 } else {
...@@ -754,7 +774,7 @@ public class DistributedGroupStore ...@@ -754,7 +774,7 @@ public class DistributedGroupStore
754 GroupEvent event = null; 774 GroupEvent event = null;
755 775
756 if (existing != null) { 776 if (existing != null) {
757 - log.debug("addOrUpdateGroupEntry: updating group entry {} in device {}", 777 + log.trace("addOrUpdateGroupEntry: updating group entry {} in device {}",
758 group.id(), 778 group.id(),
759 group.deviceId()); 779 group.deviceId());
760 synchronized (existing) { 780 synchronized (existing) {
...@@ -779,7 +799,7 @@ public class DistributedGroupStore ...@@ -779,7 +799,7 @@ public class DistributedGroupStore
779 existing.setBytes(group.bytes()); 799 existing.setBytes(group.bytes());
780 if ((existing.state() == GroupState.PENDING_ADD) || 800 if ((existing.state() == GroupState.PENDING_ADD) ||
781 (existing.state() == GroupState.PENDING_ADD_RETRY)) { 801 (existing.state() == GroupState.PENDING_ADD_RETRY)) {
782 - log.debug("addOrUpdateGroupEntry: group entry {} in device {} moving from {} to ADDED", 802 + log.trace("addOrUpdateGroupEntry: group entry {} in device {} moving from {} to ADDED",
783 existing.id(), 803 existing.id(),
784 existing.deviceId(), 804 existing.deviceId(),
785 existing.state()); 805 existing.state());
...@@ -787,7 +807,7 @@ public class DistributedGroupStore ...@@ -787,7 +807,7 @@ public class DistributedGroupStore
787 existing.setIsGroupStateAddedFirstTime(true); 807 existing.setIsGroupStateAddedFirstTime(true);
788 event = new GroupEvent(Type.GROUP_ADDED, existing); 808 event = new GroupEvent(Type.GROUP_ADDED, existing);
789 } else { 809 } else {
790 - log.debug("addOrUpdateGroupEntry: group entry {} in device {} moving from {} to ADDED", 810 + log.trace("addOrUpdateGroupEntry: group entry {} in device {} moving from {} to ADDED",
791 existing.id(), 811 existing.id(),
792 existing.deviceId(), 812 existing.deviceId(),
793 GroupState.PENDING_UPDATE); 813 GroupState.PENDING_UPDATE);
...@@ -911,18 +931,19 @@ public class DistributedGroupStore ...@@ -911,18 +931,19 @@ public class DistributedGroupStore
911 } 931 }
912 932
913 log.warn("groupOperationFailed: group operation {} failed" 933 log.warn("groupOperationFailed: group operation {} failed"
914 - + "for group {} in device {}", 934 + + "for group {} in device {} with code {}",
915 operation.opType(), 935 operation.opType(),
916 existing.id(), 936 existing.id(),
917 - existing.deviceId()); 937 + existing.deviceId(),
938 + operation.failureCode());
939 + if (operation.failureCode() == GroupOperation.GroupMsgErrorCode.GROUP_EXISTS) {
940 + log.warn("Current extraneous groups in device:{} are: {}",
941 + deviceId,
942 + getExtraneousGroups(deviceId));
943 + }
918 switch (operation.opType()) { 944 switch (operation.opType()) {
919 case ADD: 945 case ADD:
920 if (existing.state() == GroupState.PENDING_ADD) { 946 if (existing.state() == GroupState.PENDING_ADD) {
921 - //TODO: Need to add support for passing the group
922 - //operation failure reason from group provider.
923 - //If the error type is anything other than GROUP_EXISTS,
924 - //then the GROUP_ADD_FAILED event should be raised even
925 - //in PENDING_ADD_RETRY state also.
926 notifyDelegate(new GroupEvent(Type.GROUP_ADD_FAILED, existing)); 947 notifyDelegate(new GroupEvent(Type.GROUP_ADD_FAILED, existing));
927 log.warn("groupOperationFailed: cleaningup " 948 log.warn("groupOperationFailed: cleaningup "
928 + "group {} from store in device {}....", 949 + "group {} from store in device {}....",
...@@ -1220,13 +1241,13 @@ public class DistributedGroupStore ...@@ -1220,13 +1241,13 @@ public class DistributedGroupStore
1220 for (Group group : extraneousStoredEntries) { 1241 for (Group group : extraneousStoredEntries) {
1221 // there are groups in the extraneous store that 1242 // there are groups in the extraneous store that
1222 // aren't in the switch 1243 // aren't in the switch
1223 - log.debug("Group AUDIT: clearing extransoeus group {} from store for device {}", 1244 + log.debug("Group AUDIT: clearing extraneous group {} from store for device {}",
1224 group.id(), deviceId); 1245 group.id(), deviceId);
1225 removeExtraneousGroupEntry(group); 1246 removeExtraneousGroupEntry(group);
1226 } 1247 }
1227 1248
1228 if (!deviceInitialAuditStatus) { 1249 if (!deviceInitialAuditStatus) {
1229 - log.debug("Group AUDIT: Setting device {} initial AUDIT completed", 1250 + log.info("Group AUDIT: Setting device {} initial AUDIT completed",
1230 deviceId); 1251 deviceId);
1231 deviceInitialAuditCompleted(deviceId, true); 1252 deviceInitialAuditCompleted(deviceId, true);
1232 } 1253 }
...@@ -1266,7 +1287,7 @@ public class DistributedGroupStore ...@@ -1266,7 +1287,7 @@ public class DistributedGroupStore
1266 } 1287 }
1267 1288
1268 private void extraneousGroup(Group group) { 1289 private void extraneousGroup(Group group) {
1269 - log.debug("Group {} is on device {} but not in store.", 1290 + log.trace("Group {} is on device {} but not in store.",
1270 group, group.deviceId()); 1291 group, group.deviceId());
1271 addOrUpdateExtraneousGroupEntry(group); 1292 addOrUpdateExtraneousGroupEntry(group);
1272 } 1293 }
......
...@@ -556,7 +556,8 @@ public class OFDPA2GroupHandler { ...@@ -556,7 +556,8 @@ public class OFDPA2GroupHandler {
556 } 556 }
557 557
558 // assemble info for l2 flood group 558 // assemble info for l2 flood group
559 - Integer l2floodgroupId = L2_FLOOD_TYPE | (vlanId.toShort() << 16) | nextObj.id(); 559 + // since there can be only one flood group for a vlan, its index is always the same - 0
560 + Integer l2floodgroupId = L2_FLOOD_TYPE | (vlanId.toShort() << 16);
560 int l2floodgk = L2_FLOOD_TYPE | nextObj.id() << 12; 561 int l2floodgk = L2_FLOOD_TYPE | nextObj.id() << 12;
561 final GroupKey l2floodgroupkey = new DefaultGroupKey(OFDPA2Pipeline.appKryo.serialize(l2floodgk)); 562 final GroupKey l2floodgroupkey = new DefaultGroupKey(OFDPA2Pipeline.appKryo.serialize(l2floodgk));
562 // collection of group buckets pointing to all the l2 interface groups 563 // collection of group buckets pointing to all the l2 interface groups
...@@ -1007,7 +1008,7 @@ public class OFDPA2GroupHandler { ...@@ -1007,7 +1008,7 @@ public class OFDPA2GroupHandler {
1007 if (gceSet != null) { 1008 if (gceSet != null) {
1008 for (GroupChainElem gce : gceSet) { 1009 for (GroupChainElem gce : gceSet) {
1009 log.info("Group service {} group key {} in device {}. " 1010 log.info("Group service {} group key {} in device {}. "
1010 - + "Processing next group in group chain with group id {}", 1011 + + "Processing next group in group chain with group id 0x{}",
1011 (added) ? "ADDED" : "processed", 1012 (added) ? "ADDED" : "processed",
1012 key, deviceId, 1013 key, deviceId,
1013 Integer.toHexString(gce.groupDescription.givenGroupId())); 1014 Integer.toHexString(gce.groupDescription.givenGroupId()));
...@@ -1020,7 +1021,7 @@ public class OFDPA2GroupHandler { ...@@ -1020,7 +1021,7 @@ public class OFDPA2GroupHandler {
1020 pendingNextObjectives.invalidate(key); 1021 pendingNextObjectives.invalidate(key);
1021 nextGrpList.forEach(nextGrp -> { 1022 nextGrpList.forEach(nextGrp -> {
1022 log.info("Group service {} group key {} in device:{}. " 1023 log.info("Group service {} group key {} in device:{}. "
1023 - + "Done implementing next objective: {} <<-->> gid:{}", 1024 + + "Done implementing next objective: {} <<-->> gid:0x{}",
1024 (added) ? "ADDED" : "processed", 1025 (added) ? "ADDED" : "processed",
1025 key, deviceId, nextGrp.nextObjective().id(), 1026 key, deviceId, nextGrp.nextObjective().id(),
1026 Integer.toHexString(groupService.getGroup(deviceId, key) 1027 Integer.toHexString(groupService.getGroup(deviceId, key)
...@@ -1116,7 +1117,6 @@ public class OFDPA2GroupHandler { ...@@ -1116,7 +1117,6 @@ public class OFDPA2GroupHandler {
1116 this.nextObj = nextObj; 1117 this.nextObj = nextObj;
1117 } 1118 }
1118 1119
1119 - @SuppressWarnings("unused")
1120 public List<Deque<GroupKey>> groupKey() { 1120 public List<Deque<GroupKey>> groupKey() {
1121 return gkeys; 1121 return gkeys;
1122 } 1122 }
......
...@@ -37,6 +37,7 @@ import org.onosproject.net.group.Group; ...@@ -37,6 +37,7 @@ import org.onosproject.net.group.Group;
37 import org.onosproject.net.group.GroupBuckets; 37 import org.onosproject.net.group.GroupBuckets;
38 import org.onosproject.net.group.GroupDescription; 38 import org.onosproject.net.group.GroupDescription;
39 import org.onosproject.net.group.GroupOperation; 39 import org.onosproject.net.group.GroupOperation;
40 +import org.onosproject.net.group.GroupOperation.GroupMsgErrorCode;
40 import org.onosproject.net.group.GroupOperations; 41 import org.onosproject.net.group.GroupOperations;
41 import org.onosproject.net.group.GroupProvider; 42 import org.onosproject.net.group.GroupProvider;
42 import org.onosproject.net.group.GroupProviderRegistry; 43 import org.onosproject.net.group.GroupProviderRegistry;
...@@ -56,6 +57,7 @@ import org.projectfloodlight.openflow.protocol.OFErrorType; ...@@ -56,6 +57,7 @@ import org.projectfloodlight.openflow.protocol.OFErrorType;
56 import org.projectfloodlight.openflow.protocol.OFGroupDescStatsEntry; 57 import org.projectfloodlight.openflow.protocol.OFGroupDescStatsEntry;
57 import org.projectfloodlight.openflow.protocol.OFGroupDescStatsReply; 58 import org.projectfloodlight.openflow.protocol.OFGroupDescStatsReply;
58 import org.projectfloodlight.openflow.protocol.OFGroupMod; 59 import org.projectfloodlight.openflow.protocol.OFGroupMod;
60 +import org.projectfloodlight.openflow.protocol.OFGroupModFailedCode;
59 import org.projectfloodlight.openflow.protocol.OFGroupStatsEntry; 61 import org.projectfloodlight.openflow.protocol.OFGroupStatsEntry;
60 import org.projectfloodlight.openflow.protocol.OFGroupStatsReply; 62 import org.projectfloodlight.openflow.protocol.OFGroupStatsReply;
61 import org.projectfloodlight.openflow.protocol.OFGroupType; 63 import org.projectfloodlight.openflow.protocol.OFGroupType;
...@@ -64,6 +66,7 @@ import org.projectfloodlight.openflow.protocol.OFPortStatus; ...@@ -64,6 +66,7 @@ import org.projectfloodlight.openflow.protocol.OFPortStatus;
64 import org.projectfloodlight.openflow.protocol.OFStatsReply; 66 import org.projectfloodlight.openflow.protocol.OFStatsReply;
65 import org.projectfloodlight.openflow.protocol.OFStatsType; 67 import org.projectfloodlight.openflow.protocol.OFStatsType;
66 import org.projectfloodlight.openflow.protocol.OFVersion; 68 import org.projectfloodlight.openflow.protocol.OFVersion;
69 +import org.projectfloodlight.openflow.protocol.errormsg.OFGroupModFailedErrorMsg;
67 import org.slf4j.Logger; 70 import org.slf4j.Logger;
68 71
69 import com.google.common.collect.Maps; 72 import com.google.common.collect.Maps;
...@@ -135,7 +138,6 @@ public class OpenFlowGroupProvider extends AbstractProvider implements GroupProv ...@@ -135,7 +138,6 @@ public class OpenFlowGroupProvider extends AbstractProvider implements GroupProv
135 138
136 @Override 139 @Override
137 public void performGroupOperation(DeviceId deviceId, GroupOperations groupOps) { 140 public void performGroupOperation(DeviceId deviceId, GroupOperations groupOps) {
138 - Map<OFGroupMod, OpenFlowSwitch> mods = Maps.newIdentityHashMap();
139 final Dpid dpid = Dpid.dpid(deviceId.uri()); 141 final Dpid dpid = Dpid.dpid(deviceId.uri());
140 OpenFlowSwitch sw = controller.getSwitch(dpid); 142 OpenFlowSwitch sw = controller.getSwitch(dpid);
141 for (GroupOperation groupOperation: groupOps.operations()) { 143 for (GroupOperation groupOperation: groupOps.operations()) {
...@@ -329,11 +331,17 @@ public class OpenFlowGroupProvider extends AbstractProvider implements GroupProv ...@@ -329,11 +331,17 @@ public class OpenFlowGroupProvider extends AbstractProvider implements GroupProv
329 pendingGroupOperations.get(pendingGroupId); 331 pendingGroupOperations.get(pendingGroupId);
330 DeviceId deviceId = DeviceId.deviceId(Dpid.uri(dpid)); 332 DeviceId deviceId = DeviceId.deviceId(Dpid.uri(dpid));
331 if (operation != null) { 333 if (operation != null) {
334 + OFGroupModFailedCode code =
335 + ((OFGroupModFailedErrorMsg) errorMsg).getCode();
336 + GroupMsgErrorCode failureCode =
337 + GroupMsgErrorCode.values()[(code.ordinal())];
338 + GroupOperation failedOperation = GroupOperation
339 + .createFailedGroupOperation(operation, failureCode);
332 providerService.groupOperationFailed(deviceId, 340 providerService.groupOperationFailed(deviceId,
333 - operation); 341 + failedOperation);
334 pendingGroupOperations.remove(pendingGroupId); 342 pendingGroupOperations.remove(pendingGroupId);
335 pendingXidMaps.remove(pendingGroupId); 343 pendingXidMaps.remove(pendingGroupId);
336 - log.warn("Received an group mod error {}", msg); 344 + log.warn("Received a group mod error {}", msg);
337 } else { 345 } else {
338 log.error("Cannot find pending group operation with group ID: {}", 346 log.error("Cannot find pending group operation with group ID: {}",
339 pendingGroupId); 347 pendingGroupId);
......