Bug fix: Do not create groups as a side effect when revoking routes
Also changing pendingGroups to a cache that will automatically purge failed groups
Improving log messagesin several places
Change-Id: I6843a66d58e623259c7fd20ffe64d56a46d963f0
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 c93ad9b..a069c42 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
@@ -91,7 +91,7 @@
import static org.slf4j.LoggerFactory.getLogger;
/**
- * Group handler that emulates Broadcom OF-DPA TTP on CpqD.
+ * Group handler that emulates Broadcom OF-DPA TTP.
*/
public class Ofdpa2GroupHandler {
/*
@@ -134,7 +134,7 @@
private FlowObjectiveStore flowObjectiveStore;
private Cache<GroupKey, List<OfdpaNextGroup>> pendingAddNextObjectives;
private Cache<NextObjective, List<GroupKey>> pendingRemoveNextObjectives;
- private ConcurrentHashMap<GroupKey, Set<GroupChainElem>> pendingGroups;
+ private Cache<GroupKey, Set<GroupChainElem>> pendingGroups;
private ConcurrentHashMap<GroupKey, Set<NextObjective>> pendingUpdateNextObjectives;
private ScheduledExecutorService groupChecker =
Executors.newScheduledThreadPool(2, groupedThreads("onos/pipeliner", "ofdpa2-%d", log));
@@ -205,7 +205,15 @@
ObjectiveError.GROUPREMOVALFAILED);
}
}).build();
- pendingGroups = new ConcurrentHashMap<>();
+ pendingGroups = CacheBuilder.newBuilder()
+ .expireAfterWrite(20, TimeUnit.SECONDS)
+ .removalListener((
+ RemovalNotification<GroupKey, Set<GroupChainElem>> notification) -> {
+ if (notification.getCause() == RemovalCause.EXPIRED) {
+ log.error("Unable to install group with key {} and pending GCEs: {}",
+ notification.getKey(), notification.getValue());
+ }
+ }).build();
pendingUpdateNextObjectives = new ConcurrentHashMap<>();
groupChecker.scheduleAtFixedRate(new GroupChecker(), 0, 500, TimeUnit.MILLISECONDS);
@@ -875,7 +883,6 @@
updatePendingGroups(gi.nextGroupDesc.appCookie(), l3ecmpGce);
groupService.addGroup(gi.innerMostGroupDesc);
}
-
}
/**
@@ -1061,11 +1068,9 @@
nextObjective.next().forEach(trafficTreatment -> {
PortNumber portNumber = readOutPortFromTreatment(trafficTreatment);
-
if (portNumber == null) {
return;
}
-
if (existingPorts.contains(portNumber)) {
duplicateBuckets.add(trafficTreatment);
} else {
@@ -1118,7 +1123,6 @@
});
}
});
-
return existingPorts;
}
@@ -1180,7 +1184,6 @@
updatePendingGroups(groupInfo.nextGroupDesc.appCookie(), l3ecmpGce);
groupService.addGroup(groupInfo.innerMostGroupDesc);
});
-
}
private void addBucketToBroadcastGroup(NextObjective nextObj,
@@ -1418,9 +1421,6 @@
updatePendingNextObjective(l3mcastGroupKey,
new OfdpaNextGroup(allActiveKeys, nextObj));
-
-
-
}
/**
@@ -1537,8 +1537,6 @@
.map(id -> HEX_PREFIX + id)
.collect(Collectors.toList());
-
-
log.debug("Removing buckets from group id 0x{} pointing to group id(s) {} "
+ "for next id {} in device {}", Integer.toHexString(modGroup.id().id()),
pointedGroupIds, nextObjective.id(), deviceId);
@@ -1561,8 +1559,7 @@
}
/**
- * Removes all groups in multiple possible group-chains that represent the next
- * objective.
+ * Removes all groups in multiple possible group-chains that represent the next-obj.
*
* @param nextObjective the next objective to remove
* @param next the NextGroup that represents the existing group-chain for
@@ -1584,24 +1581,24 @@
// Helper Methods and Classes
//////////////////////////////////////
- protected void updatePendingNextObjective(GroupKey key, OfdpaNextGroup value) {
- List<OfdpaNextGroup> nextList = new CopyOnWriteArrayList<OfdpaNextGroup>();
- nextList.add(value);
- List<OfdpaNextGroup> ret = pendingAddNextObjectives.asMap()
- .putIfAbsent(key, nextList);
- if (ret != null) {
- ret.add(value);
- }
+ protected void updatePendingNextObjective(GroupKey gkey, OfdpaNextGroup value) {
+ pendingAddNextObjectives.asMap().compute(gkey, (k, val) -> {
+ if (val == null) {
+ val = new CopyOnWriteArrayList<OfdpaNextGroup>();
+ }
+ val.add(value);
+ return val;
+ });
}
protected void updatePendingGroups(GroupKey gkey, GroupChainElem gce) {
- Set<GroupChainElem> gceSet = Collections.newSetFromMap(
- new ConcurrentHashMap<GroupChainElem, Boolean>());
- gceSet.add(gce);
- Set<GroupChainElem> retval = pendingGroups.putIfAbsent(gkey, gceSet);
- if (retval != null) {
- retval.add(gce);
- }
+ pendingGroups.asMap().compute(gkey, (k, val) -> {
+ if (val == null) {
+ val = Sets.newConcurrentHashSet();
+ }
+ val.add(gce);
+ return val;
+ });
}
private void addPendingUpdateNextObjective(GroupKey groupKey, NextObjective nextObjective) {
@@ -1650,7 +1647,14 @@
private class GroupChecker implements Runnable {
@Override
public void run() {
- Set<GroupKey> keys = pendingGroups.keySet().stream()
+ if (pendingGroups.size() != 0) {
+ log.debug("pending groups being checked: {}", pendingGroups.asMap().keySet());
+ }
+ if (pendingAddNextObjectives.size() != 0) {
+ log.debug("pending add-next-obj being checked: {}",
+ pendingAddNextObjectives.asMap().keySet());
+ }
+ Set<GroupKey> keys = pendingGroups.asMap().keySet().stream()
.filter(key -> groupService.getGroup(deviceId, key) != null)
.collect(Collectors.toSet());
Set<GroupKey> otherkeys = pendingAddNextObjectives.asMap().keySet().stream()
@@ -1684,7 +1688,6 @@
}
private void processPendingUpdateNextObjs(GroupKey groupKey) {
-
pendingUpdateNextObjectives.compute(groupKey, (gKey, nextObjs) -> {
if (nextObjs != null) {
@@ -1695,14 +1698,13 @@
Ofdpa2Pipeline.pass(nextObj);
});
}
-
return Sets.newHashSet();
});
}
private void processPendingAddGroupsOrNextObjs(GroupKey key, boolean added) {
//first check for group chain
- Set<GroupChainElem> gceSet = pendingGroups.remove(key);
+ Set<GroupChainElem> gceSet = pendingGroups.asMap().remove(key);
if (gceSet != null) {
for (GroupChainElem gce : gceSet) {
log.debug("Group service {} group key {} in device {}. "
diff --git a/drivers/default/src/main/java/org/onosproject/driver/pipeline/Ofdpa2Pipeline.java b/drivers/default/src/main/java/org/onosproject/driver/pipeline/Ofdpa2Pipeline.java
index b937762..6edebdf 100644
--- a/drivers/default/src/main/java/org/onosproject/driver/pipeline/Ofdpa2Pipeline.java
+++ b/drivers/default/src/main/java/org/onosproject/driver/pipeline/Ofdpa2Pipeline.java
@@ -399,12 +399,12 @@
if (ethCriterion == null || ethCriterion.mac().equals(NONE)) {
// NOTE: it is possible that a filtering objective only has vidCriterion
- log.debug("filtering objective missing dstMac, cannot program TMAC table");
+ log.warn("filtering objective missing dstMac, cannot program TMAC table");
} else {
for (FlowRule tmacRule : processEthDstFilter(portCriterion, ethCriterion,
vidCriterion, assignedVlan,
applicationId)) {
- log.debug("{} MAC filtering rules in TMAC table: {} for dev: {}",
+ log.trace("{} MAC filtering rules in TMAC table: {} for dev: {}",
(install) ? "adding" : "removing", tmacRule, deviceId);
ops = install ? ops.add(tmacRule) : ops.remove(tmacRule);
}
@@ -443,7 +443,7 @@
});
for (FlowRule filteringRule : filteringRules) {
- log.debug("{} VLAN filtering rule in VLAN table: {} for dev: {}",
+ log.trace("{} VLAN filtering rule in VLAN table: {} for dev: {}",
(install) ? "adding" : "removing", filteringRule, deviceId);
ops = install ? ops.add(filteringRule) : ops.remove(filteringRule);
}
@@ -451,7 +451,7 @@
ops.newStage();
for (FlowRule assignmentRule : assignmentRules) {
- log.debug("{} VLAN assignment rule in VLAN table: {} for dev: {}",
+ log.trace("{} VLAN assignment rule in VLAN table: {} for dev: {}",
(install) ? "adding" : "removing", assignmentRule, deviceId);
ops = install ? ops.add(assignmentRule) : ops.remove(assignmentRule);
}