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
diff --git a/core/api/src/main/java/org/onosproject/core/DefaultGroupId.java b/core/api/src/main/java/org/onosproject/core/DefaultGroupId.java
index 7467c3c..3f95559 100644
--- a/core/api/src/main/java/org/onosproject/core/DefaultGroupId.java
+++ b/core/api/src/main/java/org/onosproject/core/DefaultGroupId.java
@@ -61,7 +61,7 @@
@Override
public String toString() {
return MoreObjects.toStringHelper(this)
- .add("id", id)
+ .add("id", "0x" + Integer.toHexString(id))
.toString();
}
}
diff --git a/core/api/src/main/java/org/onosproject/net/flow/instructions/Instructions.java b/core/api/src/main/java/org/onosproject/net/flow/instructions/Instructions.java
index e1a71fc..3e2335b 100644
--- a/core/api/src/main/java/org/onosproject/net/flow/instructions/Instructions.java
+++ b/core/api/src/main/java/org/onosproject/net/flow/instructions/Instructions.java
@@ -576,7 +576,7 @@
@Override
public String toString() {
- return type().toString() + SEPARATOR + Integer.toHexString(groupId.id());
+ return type().toString() + SEPARATOR + "0x" + Integer.toHexString(groupId.id());
}
@Override
diff --git a/core/api/src/main/java/org/onosproject/net/group/GroupOperation.java b/core/api/src/main/java/org/onosproject/net/group/GroupOperation.java
index e4173b3..42ef9dd 100644
--- a/core/api/src/main/java/org/onosproject/net/group/GroupOperation.java
+++ b/core/api/src/main/java/org/onosproject/net/group/GroupOperation.java
@@ -31,6 +31,7 @@
private final GroupId groupId;
private final GroupDescription.Type groupType;
private final GroupBuckets buckets;
+ private final GroupMsgErrorCode failureCode;
public enum Type {
/**
@@ -48,6 +49,28 @@
}
/**
+ * Possible error codes for a failure of a group operation.
+ *
+ */
+ public enum GroupMsgErrorCode {
+ GROUP_EXISTS,
+ INVALID_GROUP,
+ WEIGHT_UNSUPPORTED,
+ OUT_OF_GROUPS,
+ OUT_OF_BUCKETS,
+ CHAINING_UNSUPPORTED,
+ WATCH_UNSUPPORTED,
+ LOOP,
+ UNKNOWN_GROUP,
+ CHAINED_GROUP,
+ BAD_TYPE,
+ BAD_COMMAND,
+ BAD_BUCKET,
+ BAD_WATCH,
+ EPERM;
+ }
+
+ /**
* Group operation constructor with the parameters.
*
* @param opType group operation type
@@ -63,6 +86,23 @@
this.groupId = checkNotNull(groupId);
this.groupType = checkNotNull(groupType);
this.buckets = buckets;
+ this.failureCode = null;
+ }
+
+ /**
+ * Group operation copy-constructor with additional field to set failure code.
+ * Typically used by provider to return information to the core about
+ * the failure reason for a group operation.
+ *
+ * @param groupOp the original group operation
+ * @param failureCode failure code for a failed group operation
+ */
+ private GroupOperation(GroupOperation groupOp, GroupMsgErrorCode failureCode) {
+ this.opType = groupOp.opType;
+ this.groupId = groupOp.groupId;
+ this.groupType = groupOp.groupType;
+ this.buckets = groupOp.buckets;
+ this.failureCode = failureCode;
}
/**
@@ -109,6 +149,12 @@
}
+ public static GroupOperation createFailedGroupOperation(GroupOperation groupOperation,
+ GroupMsgErrorCode failureCode) {
+ checkNotNull(groupOperation);
+ return new GroupOperation(groupOperation, failureCode);
+ }
+
/**
* Returns group operation type.
*
@@ -145,6 +191,15 @@
return this.buckets;
}
+ /**
+ * Returns the failure code representing the failure of a group operation.
+ *
+ * @return error code for failure of group operation
+ */
+ public GroupMsgErrorCode failureCode() {
+ return failureCode;
+ }
+
@Override
/*
* The deviceId, type and buckets are used for hash.
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 64b7c87..250dfb9 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
@@ -88,7 +88,8 @@
import static org.slf4j.LoggerFactory.getLogger;
/**
- * Manages inventory of group entries using trivial in-memory implementation.
+ * Manages inventory of group entries using distributed group stores from the
+ * storage service.
*/
@Component(immediate = true)
@Service
@@ -450,16 +451,18 @@
matchingExtraneousGroup = getMatchingExtraneousGroupbyId(
groupDesc.deviceId(), groupDesc.givenGroupId());
if (matchingExtraneousGroup != null) {
- log.debug("storeGroupDescriptionInternal: Matching extraneous group found in Device {} for group id {}",
+ log.debug("storeGroupDescriptionInternal: Matching extraneous group "
+ + "found in Device {} for group id 0x{}",
groupDesc.deviceId(),
- groupDesc.givenGroupId());
+ Integer.toHexString(groupDesc.givenGroupId()));
//Check if the group buckets matches with user provided buckets
if (matchingExtraneousGroup.buckets().equals(groupDesc.buckets())) {
//Group is already existing with the same buckets and Id
// Create a group entry object
- log.debug("storeGroupDescriptionInternal: Buckets also matching in Device {} for group id {}",
+ log.debug("storeGroupDescriptionInternal: Buckets also matching "
+ + "in Device {} for group id 0x{}",
groupDesc.deviceId(),
- groupDesc.givenGroupId());
+ Integer.toHexString(groupDesc.givenGroupId()));
StoredGroupEntry group = new DefaultGroup(
matchingExtraneousGroup.id(), groupDesc);
// Insert the newly created group entry into key and id maps
@@ -476,10 +479,27 @@
} else {
//Group buckets are not matching. Update group
//with user provided buckets.
- //TODO
- log.debug("storeGroupDescriptionInternal: Buckets are not matching in Device {} for group id {}",
+ log.debug("storeGroupDescriptionInternal: Buckets are not "
+ + "matching in Device {} for group id 0x{}",
groupDesc.deviceId(),
- groupDesc.givenGroupId());
+ Integer.toHexString(groupDesc.givenGroupId()));
+ StoredGroupEntry modifiedGroup = new DefaultGroup(
+ matchingExtraneousGroup.id(), groupDesc);
+ modifiedGroup.setState(GroupState.PENDING_UPDATE);
+ getGroupStoreKeyMap().
+ put(new GroupStoreKeyMapKey(groupDesc.deviceId(),
+ groupDesc.appCookie()), modifiedGroup);
+ // Ensure it also inserted into group id based table to
+ // avoid any chances of duplication in group id generation
+ getGroupIdTable(groupDesc.deviceId()).
+ put(matchingExtraneousGroup.id(), modifiedGroup);
+ removeExtraneousGroupEntry(matchingExtraneousGroup);
+ log.debug("storeGroupDescriptionInternal: Triggering Group "
+ + "UPDATE request for {} in device {}",
+ matchingExtraneousGroup.id(),
+ groupDesc.deviceId());
+ notifyDelegate(new GroupEvent(Type.GROUP_UPDATE_REQUESTED, modifiedGroup));
+ return;
}
}
} else {
@@ -754,7 +774,7 @@
GroupEvent event = null;
if (existing != null) {
- log.debug("addOrUpdateGroupEntry: updating group entry {} in device {}",
+ log.trace("addOrUpdateGroupEntry: updating group entry {} in device {}",
group.id(),
group.deviceId());
synchronized (existing) {
@@ -779,7 +799,7 @@
existing.setBytes(group.bytes());
if ((existing.state() == GroupState.PENDING_ADD) ||
(existing.state() == GroupState.PENDING_ADD_RETRY)) {
- log.debug("addOrUpdateGroupEntry: group entry {} in device {} moving from {} to ADDED",
+ log.trace("addOrUpdateGroupEntry: group entry {} in device {} moving from {} to ADDED",
existing.id(),
existing.deviceId(),
existing.state());
@@ -787,7 +807,7 @@
existing.setIsGroupStateAddedFirstTime(true);
event = new GroupEvent(Type.GROUP_ADDED, existing);
} else {
- log.debug("addOrUpdateGroupEntry: group entry {} in device {} moving from {} to ADDED",
+ log.trace("addOrUpdateGroupEntry: group entry {} in device {} moving from {} to ADDED",
existing.id(),
existing.deviceId(),
GroupState.PENDING_UPDATE);
@@ -911,18 +931,19 @@
}
log.warn("groupOperationFailed: group operation {} failed"
- + "for group {} in device {}",
+ + "for group {} in device {} with code {}",
operation.opType(),
existing.id(),
- existing.deviceId());
+ existing.deviceId(),
+ operation.failureCode());
+ if (operation.failureCode() == GroupOperation.GroupMsgErrorCode.GROUP_EXISTS) {
+ log.warn("Current extraneous groups in device:{} are: {}",
+ deviceId,
+ getExtraneousGroups(deviceId));
+ }
switch (operation.opType()) {
case ADD:
if (existing.state() == GroupState.PENDING_ADD) {
- //TODO: Need to add support for passing the group
- //operation failure reason from group provider.
- //If the error type is anything other than GROUP_EXISTS,
- //then the GROUP_ADD_FAILED event should be raised even
- //in PENDING_ADD_RETRY state also.
notifyDelegate(new GroupEvent(Type.GROUP_ADD_FAILED, existing));
log.warn("groupOperationFailed: cleaningup "
+ "group {} from store in device {}....",
@@ -1220,13 +1241,13 @@
for (Group group : extraneousStoredEntries) {
// there are groups in the extraneous store that
// aren't in the switch
- log.debug("Group AUDIT: clearing extransoeus group {} from store for device {}",
+ log.debug("Group AUDIT: clearing extraneous group {} from store for device {}",
group.id(), deviceId);
removeExtraneousGroupEntry(group);
}
if (!deviceInitialAuditStatus) {
- log.debug("Group AUDIT: Setting device {} initial AUDIT completed",
+ log.info("Group AUDIT: Setting device {} initial AUDIT completed",
deviceId);
deviceInitialAuditCompleted(deviceId, true);
}
@@ -1266,7 +1287,7 @@
}
private void extraneousGroup(Group group) {
- log.debug("Group {} is on device {} but not in store.",
+ log.trace("Group {} is on device {} but not in store.",
group, group.deviceId());
addOrUpdateExtraneousGroupEntry(group);
}
diff --git a/drivers/default/src/main/java/org/onosproject/driver/pipeline/OFDPA2GroupHandler.java b/drivers/default/src/main/java/org/onosproject/driver/pipeline/OFDPA2GroupHandler.java
index 56433cf..7a42d93 100644
--- a/drivers/default/src/main/java/org/onosproject/driver/pipeline/OFDPA2GroupHandler.java
+++ b/drivers/default/src/main/java/org/onosproject/driver/pipeline/OFDPA2GroupHandler.java
@@ -556,7 +556,8 @@
}
// assemble info for l2 flood group
- Integer l2floodgroupId = L2_FLOOD_TYPE | (vlanId.toShort() << 16) | nextObj.id();
+ // since there can be only one flood group for a vlan, its index is always the same - 0
+ Integer l2floodgroupId = L2_FLOOD_TYPE | (vlanId.toShort() << 16);
int l2floodgk = L2_FLOOD_TYPE | nextObj.id() << 12;
final GroupKey l2floodgroupkey = new DefaultGroupKey(OFDPA2Pipeline.appKryo.serialize(l2floodgk));
// collection of group buckets pointing to all the l2 interface groups
@@ -1007,7 +1008,7 @@
if (gceSet != null) {
for (GroupChainElem gce : gceSet) {
log.info("Group service {} group key {} in device {}. "
- + "Processing next group in group chain with group id {}",
+ + "Processing next group in group chain with group id 0x{}",
(added) ? "ADDED" : "processed",
key, deviceId,
Integer.toHexString(gce.groupDescription.givenGroupId()));
@@ -1020,7 +1021,7 @@
pendingNextObjectives.invalidate(key);
nextGrpList.forEach(nextGrp -> {
log.info("Group service {} group key {} in device:{}. "
- + "Done implementing next objective: {} <<-->> gid:{}",
+ + "Done implementing next objective: {} <<-->> gid:0x{}",
(added) ? "ADDED" : "processed",
key, deviceId, nextGrp.nextObjective().id(),
Integer.toHexString(groupService.getGroup(deviceId, key)
@@ -1116,7 +1117,6 @@
this.nextObj = nextObj;
}
- @SuppressWarnings("unused")
public List<Deque<GroupKey>> groupKey() {
return gkeys;
}
diff --git a/providers/openflow/group/src/main/java/org/onosproject/provider/of/group/impl/OpenFlowGroupProvider.java b/providers/openflow/group/src/main/java/org/onosproject/provider/of/group/impl/OpenFlowGroupProvider.java
index 2801f12..162c89d 100644
--- a/providers/openflow/group/src/main/java/org/onosproject/provider/of/group/impl/OpenFlowGroupProvider.java
+++ b/providers/openflow/group/src/main/java/org/onosproject/provider/of/group/impl/OpenFlowGroupProvider.java
@@ -37,6 +37,7 @@
import org.onosproject.net.group.GroupBuckets;
import org.onosproject.net.group.GroupDescription;
import org.onosproject.net.group.GroupOperation;
+import org.onosproject.net.group.GroupOperation.GroupMsgErrorCode;
import org.onosproject.net.group.GroupOperations;
import org.onosproject.net.group.GroupProvider;
import org.onosproject.net.group.GroupProviderRegistry;
@@ -56,6 +57,7 @@
import org.projectfloodlight.openflow.protocol.OFGroupDescStatsEntry;
import org.projectfloodlight.openflow.protocol.OFGroupDescStatsReply;
import org.projectfloodlight.openflow.protocol.OFGroupMod;
+import org.projectfloodlight.openflow.protocol.OFGroupModFailedCode;
import org.projectfloodlight.openflow.protocol.OFGroupStatsEntry;
import org.projectfloodlight.openflow.protocol.OFGroupStatsReply;
import org.projectfloodlight.openflow.protocol.OFGroupType;
@@ -64,6 +66,7 @@
import org.projectfloodlight.openflow.protocol.OFStatsReply;
import org.projectfloodlight.openflow.protocol.OFStatsType;
import org.projectfloodlight.openflow.protocol.OFVersion;
+import org.projectfloodlight.openflow.protocol.errormsg.OFGroupModFailedErrorMsg;
import org.slf4j.Logger;
import com.google.common.collect.Maps;
@@ -135,7 +138,6 @@
@Override
public void performGroupOperation(DeviceId deviceId, GroupOperations groupOps) {
- Map<OFGroupMod, OpenFlowSwitch> mods = Maps.newIdentityHashMap();
final Dpid dpid = Dpid.dpid(deviceId.uri());
OpenFlowSwitch sw = controller.getSwitch(dpid);
for (GroupOperation groupOperation: groupOps.operations()) {
@@ -329,11 +331,17 @@
pendingGroupOperations.get(pendingGroupId);
DeviceId deviceId = DeviceId.deviceId(Dpid.uri(dpid));
if (operation != null) {
+ OFGroupModFailedCode code =
+ ((OFGroupModFailedErrorMsg) errorMsg).getCode();
+ GroupMsgErrorCode failureCode =
+ GroupMsgErrorCode.values()[(code.ordinal())];
+ GroupOperation failedOperation = GroupOperation
+ .createFailedGroupOperation(operation, failureCode);
providerService.groupOperationFailed(deviceId,
- operation);
+ failedOperation);
pendingGroupOperations.remove(pendingGroupId);
pendingXidMaps.remove(pendingGroupId);
- log.warn("Received an group mod error {}", msg);
+ log.warn("Received a group mod error {}", msg);
} else {
log.error("Cannot find pending group operation with group ID: {}",
pendingGroupId);