CORD-1583 More bug fixes in dual-ToR scenarios
- reentrant lock was not being used correctly
- fixHashGroup in group handler was not updating global store correctly
- linkUp was not being noted in seenLinks if configuration came after switches connected
- serialization error in global objective store due to missing kryo for Sets
- damaged routepath computation was not taking pair-devs into account
- switch failures were leading to improper ecmpSpg graph updates, and missed hash-group changes
- implemented more next-objective verification as group sub-system can go out-of-sync with objective-store
Change-Id: If3cfdd715e9b69820894b49def31f75ceb748863
diff --git a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
index e9517f1..50b0cd2 100644
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
@@ -64,6 +64,7 @@
private static final int RETRY_INTERVAL_MS = 250;
private static final int RETRY_INTERVAL_SCALE = 1;
private static final long STABLITY_THRESHOLD = 10; //secs
+ private static final int UPDATE_INTERVAL = 5; //secs
private static Logger log = LoggerFactory.getLogger(DefaultRoutingHandler.class);
private SegmentRoutingManager srManager;
@@ -148,7 +149,7 @@
public boolean isRoutingStable() {
long last = (long) (lastRoutingChange.getMillis() / 1000.0);
long now = (long) (DateTime.now().getMillis() / 1000.0);
- log.debug("Routing stable since {}s", now - last);
+ log.trace("Routing stable since {}s", now - last);
return (now - last) > STABLITY_THRESHOLD;
}
@@ -364,6 +365,8 @@
return;
}
lastRoutingChange = DateTime.now();
+ executorService.schedule(new UpdateMaps(), UPDATE_INTERVAL,
+ TimeUnit.SECONDS);
statusLock.lock();
try {
@@ -402,34 +405,23 @@
// comparing all routes of existing ECMP SPG to new ECMP SPG
routeChanges = computeRouteChange();
- if (routeChanges != null) {
- // deal with linkUp of a seen-before link
- if (linkUp != null && srManager.isSeenLink(linkUp)) {
- if (!srManager.isBidirectional(linkUp)) {
- log.warn("Not a bidirectional link yet .. not "
- + "processing link {}", linkUp);
- srManager.updateSeenLink(linkUp, true);
- populationStatus = Status.ABORTED;
- return;
- }
- // link previously seen before
- // do hash-bucket changes instead of a re-route
- processHashGroupChange(routeChanges, false, null);
- // clear out routesChanges so a re-route is not attempted
- routeChanges = ImmutableSet.of();
+ // deal with linkUp of a seen-before link
+ if (linkUp != null && srManager.isSeenLink(linkUp)) {
+ if (!srManager.isBidirectional(linkUp)) {
+ log.warn("Not a bidirectional link yet .. not "
+ + "processing link {}", linkUp);
+ srManager.updateSeenLink(linkUp, true);
+ populationStatus = Status.ABORTED;
+ return;
}
-
- //deal with switchDown
- if (switchDown != null) {
- processHashGroupChange(routeChanges, true, switchDown);
- // clear out routesChanges so a re-route is not attempted
- routeChanges = ImmutableSet.of();
- }
-
- // for a linkUp of a never-seen-before link
- // let it fall through to a reroute of the routeChanges
-
+ // link previously seen before
+ // do hash-bucket changes instead of a re-route
+ processHashGroupChange(routeChanges, false, null);
+ // clear out routesChanges so a re-route is not attempted
+ routeChanges = ImmutableSet.of();
}
+ // for a linkUp of a never-seen-before link
+ // let it fall through to a reroute of the routeChanges
// now that we are past the check for a previously seen link
// it is safe to update the store for the linkUp
@@ -437,6 +429,12 @@
srManager.updateSeenLink(linkUp, true);
}
+ //deal with switchDown
+ if (switchDown != null) {
+ processHashGroupChange(routeChanges, true, switchDown);
+ // clear out routesChanges so a re-route is not attempted
+ routeChanges = ImmutableSet.of();
+ }
} else {
// link has gone down
// Compare existing ECMP SPG only with the link that went down
@@ -452,7 +450,6 @@
if (routeChanges == null) {
log.info("Optimized routing failed... opting for full reroute");
populationStatus = Status.ABORTED;
- statusLock.unlock();
populateAllRoutingRules();
return;
}
@@ -482,6 +479,7 @@
}
}
+
/**
* Processes a set a route-path changes by reprogramming routing rules and
* creating new hash-groups or editing them if necessary. This method also
@@ -533,8 +531,6 @@
return false; //abort routing and fail fast
}
- //XXX should we do hashgroupchanges here?
-
// update ecmpSPG for all edge-pairs
for (EdgePair ep : edgePairs) {
currentEcmpSpgMap.put(ep.dev1, updatedEcmpSpgMap.get(ep.dev1));
@@ -847,41 +843,51 @@
private void processHashGroupChange(Set<ArrayList<DeviceId>> routeChanges,
boolean linkOrSwitchFailed,
DeviceId failedSwitch) {
+ Set<ArrayList<DeviceId>> changedRoutes = new HashSet<>();
+ // first, ensure each routeChanges entry has two elements
for (ArrayList<DeviceId> route : routeChanges) {
- DeviceId targetSw = route.get(0);
- boolean success;
- DeviceId dstSw = null;
- if (route.size() > 1) {
- dstSw = route.get(1);
+ if (route.size() == 1) {
+ // route-path changes are from everyone else to this switch
+ DeviceId dstSw = route.get(0);
+ srManager.deviceService.getAvailableDevices().forEach(sw -> {
+ if (!sw.id().equals(dstSw)) {
+ changedRoutes.add(Lists.newArrayList(sw.id(), dstSw));
+ }
+ });
+ } else {
+ changedRoutes.add(route);
}
+ }
+ for (ArrayList<DeviceId> route : changedRoutes) {
+ DeviceId targetSw = route.get(0);
+ DeviceId dstSw = route.get(1);
if (linkOrSwitchFailed) {
- fixHashGroupsForRoute(route, true);
+ boolean success = fixHashGroupsForRoute(route, true);
// it's possible that we cannot fix hash groups for a route
// if the target switch has failed. Nevertheless the ecmp graph
// for the impacted switch must still be updated.
- if (failedSwitch != null && targetSw.equals(failedSwitch)
- && dstSw != null) {
+ if (!success && failedSwitch != null && targetSw.equals(failedSwitch)) {
currentEcmpSpgMap.put(dstSw, updatedEcmpSpgMap.get(dstSw));
currentEcmpSpgMap.remove(targetSw);
- log.debug("Updating ECMPspg for dst:{} removing failed "
+ log.debug("Updating ECMPspg for dst:{} removing failed switch "
+ "target:{}", dstSw, targetSw);
- return;
+ continue;
}
//linkfailed - update both sides
- currentEcmpSpgMap.put(targetSw, updatedEcmpSpgMap.get(targetSw));
- if (dstSw != null) {
- currentEcmpSpgMap.put(dstSw, updatedEcmpSpgMap.get(dstSw));
- }
- log.debug("Updating ECMPspg for dst:{} and target:{}", dstSw, targetSw);
- } else {
- success = fixHashGroupsForRoute(route, false);
if (success) {
currentEcmpSpgMap.put(targetSw, updatedEcmpSpgMap.get(targetSw));
- if (dstSw != null) {
- currentEcmpSpgMap.put(dstSw, updatedEcmpSpgMap.get(dstSw));
- }
- log.debug("Updating ECMPspg for target:{} and dst:{}",
+ currentEcmpSpgMap.put(dstSw, updatedEcmpSpgMap.get(dstSw));
+ log.debug("Updating ECMPspg for dst:{} and target:{} for linkdown",
+ dstSw, targetSw);
+ }
+ } else {
+ //linkup of seen before link
+ boolean success = fixHashGroupsForRoute(route, false);
+ if (success) {
+ currentEcmpSpgMap.put(targetSw, updatedEcmpSpgMap.get(targetSw));
+ currentEcmpSpgMap.put(dstSw, updatedEcmpSpgMap.get(dstSw));
+ log.debug("Updating ECMPspg for target:{} and dst:{} for linkup",
targetSw, dstSw);
}
}
@@ -908,48 +914,10 @@
return false;
}
DeviceId destSw = route.get(1);
- log.debug("Processing fixHashGroupsForRoute: Target {} -> Dest {}",
+ log.debug("* processing fixHashGroupsForRoute: Target {} -> Dest {}",
targetSw, destSw);
- boolean targetIsEdge = false;
- try {
- targetIsEdge = srManager.deviceConfiguration.isEdgeDevice(targetSw);
- } catch (DeviceConfigNotFoundException e) {
- log.warn(e.getMessage() + "Cannot determine if targetIsEdge {}.. "
- + "continuing fixHash", targetSw);
- }
-
// figure out the new next hops at the targetSw towards the destSw
- Set<DeviceId> nextHops = new HashSet<>();
- EcmpShortestPathGraph ecmpSpg = updatedEcmpSpgMap.get(destSw);
- HashMap<Integer, HashMap<DeviceId, ArrayList<ArrayList<DeviceId>>>> switchVia =
- ecmpSpg.getAllLearnedSwitchesAndVia();
- for (Integer itrIdx : switchVia.keySet()) {
- HashMap<DeviceId, ArrayList<ArrayList<DeviceId>>> swViaMap =
- switchVia.get(itrIdx);
- for (DeviceId target : swViaMap.keySet()) {
- if (target.equals(targetSw)) {
- // found the iteration where targetSw is reached- get nextHops
- if (!targetIsEdge && itrIdx > 1) {
- // optimization for spines to not use other leaves to get
- // to a leaf to avoid loops
- log.debug("Avoiding {} hop path for non-edge targetSw:{}"
- + " --> dstSw:{}", itrIdx, targetSw, destSw);
- break;
- }
- for (ArrayList<DeviceId> via : swViaMap.get(target)) {
- if (via.isEmpty()) {
- // the dstSw is the next-hop from the targetSw
- nextHops.add(destSw);
- } else {
- // first elem is next-hop in each ECMP path
- nextHops.add(via.get(0));
- }
- }
- break;
- }
- }
- }
-
+ Set<DeviceId> nextHops = getNextHops(targetSw, destSw);
// call group handler to change hash group at targetSw
DefaultGroupHandler grpHandler = srManager.getGroupHandler(targetSw);
if (grpHandler == null) {
@@ -1010,7 +978,7 @@
* @param deviceId the device for which graphs need to be purged
*/
protected void purgeEcmpGraph(DeviceId deviceId) {
- currentEcmpSpgMap.remove(deviceId);
+ currentEcmpSpgMap.remove(deviceId); // XXX reconsider
if (updatedEcmpSpgMap != null) {
updatedEcmpSpgMap.remove(deviceId);
}
@@ -1030,64 +998,64 @@
* affected, or null if no previous ecmp spg was found for comparison
*/
private Set<ArrayList<DeviceId>> computeDamagedRoutes(Link linkFail) {
-
Set<ArrayList<DeviceId>> routes = new HashSet<>();
for (Device sw : srManager.deviceService.getDevices()) {
log.debug("Computing the impacted routes for device {} due to link fail",
sw.id());
- if (!srManager.mastershipService.isLocalMaster(sw.id())) {
- log.debug("No mastership for {} .. skipping route optimization",
- sw.id());
+ DeviceId retId = shouldHandleRouting(sw.id());
+ if (retId == null) {
continue;
}
- EcmpShortestPathGraph ecmpSpg = currentEcmpSpgMap.get(sw.id());
- if (ecmpSpg == null) {
- log.warn("No existing ECMP graph for switch {}. Aborting optimized"
- + " rerouting and opting for full-reroute", sw.id());
- return null;
- }
- if (log.isDebugEnabled()) {
- log.debug("Root switch: {}", sw.id());
- log.debug(" Current/Existing SPG: {}", ecmpSpg);
- log.debug(" New/Updated SPG: {}", updatedEcmpSpgMap.get(sw.id()));
- }
- HashMap<Integer, HashMap<DeviceId, ArrayList<ArrayList<DeviceId>>>> switchVia =
- ecmpSpg.getAllLearnedSwitchesAndVia();
- for (Integer itrIdx : switchVia.keySet()) {
- log.trace("Current/Exiting SPG Iterindex# {}", itrIdx);
- HashMap<DeviceId, ArrayList<ArrayList<DeviceId>>> swViaMap =
- switchVia.get(itrIdx);
- for (DeviceId targetSw : swViaMap.keySet()) {
- DeviceId rootSw = sw.id();
- if (log.isTraceEnabled()) {
- log.trace("TargetSwitch {} --> RootSwitch {}", targetSw, rootSw);
+ Set<DeviceId> devicesToProcess = Sets.newHashSet(retId, sw.id());
+ for (DeviceId rootSw : devicesToProcess) {
+ EcmpShortestPathGraph ecmpSpg = currentEcmpSpgMap.get(rootSw);
+ if (ecmpSpg == null) {
+ log.warn("No existing ECMP graph for switch {}. Aborting optimized"
+ + " rerouting and opting for full-reroute", rootSw);
+ return null;
+ }
+ if (log.isDebugEnabled()) {
+ log.debug("Root switch: {}", rootSw);
+ log.debug(" Current/Existing SPG: {}", ecmpSpg);
+ log.debug(" New/Updated SPG: {}", updatedEcmpSpgMap.get(rootSw));
+ }
+ HashMap<Integer, HashMap<DeviceId, ArrayList<ArrayList<DeviceId>>>>
+ switchVia = ecmpSpg.getAllLearnedSwitchesAndVia();
+ // figure out if the broken link affected any route-paths in this graph
+ for (Integer itrIdx : switchVia.keySet()) {
+ log.trace("Current/Exiting SPG Iterindex# {}", itrIdx);
+ HashMap<DeviceId, ArrayList<ArrayList<DeviceId>>> swViaMap =
+ switchVia.get(itrIdx);
+ for (DeviceId targetSw : swViaMap.keySet()) {
+ log.trace("TargetSwitch {} --> RootSwitch {}",
+ targetSw, rootSw);
for (ArrayList<DeviceId> via : swViaMap.get(targetSw)) {
log.trace(" Via:");
via.forEach(e -> log.trace(" {}", e));
}
- }
- Set<ArrayList<DeviceId>> subLinks =
- computeLinks(targetSw, rootSw, swViaMap);
- for (ArrayList<DeviceId> alink: subLinks) {
- if ((alink.get(0).equals(linkFail.src().deviceId()) &&
- alink.get(1).equals(linkFail.dst().deviceId()))
- ||
- (alink.get(0).equals(linkFail.dst().deviceId()) &&
- alink.get(1).equals(linkFail.src().deviceId()))) {
- log.debug("Impacted route:{}->{}", targetSw, rootSw);
- ArrayList<DeviceId> aRoute = new ArrayList<>();
- aRoute.add(targetSw); // switch with rules to populate
- aRoute.add(rootSw); // towards this destination
- routes.add(aRoute);
- break;
+ Set<ArrayList<DeviceId>> subLinks =
+ computeLinks(targetSw, rootSw, swViaMap);
+ for (ArrayList<DeviceId> alink: subLinks) {
+ if ((alink.get(0).equals(linkFail.src().deviceId()) &&
+ alink.get(1).equals(linkFail.dst().deviceId()))
+ ||
+ (alink.get(0).equals(linkFail.dst().deviceId()) &&
+ alink.get(1).equals(linkFail.src().deviceId()))) {
+ log.debug("Impacted route:{}->{}", targetSw, rootSw);
+ ArrayList<DeviceId> aRoute = new ArrayList<>();
+ aRoute.add(targetSw); // switch with rules to populate
+ aRoute.add(rootSw); // towards this destination
+ routes.add(aRoute);
+ break;
+ }
}
}
}
+
}
}
-
return routes;
}
@@ -1407,6 +1375,35 @@
}
}
+ /**
+ * Updates the currentEcmpSpgGraph for all devices.
+ */
+ private void updateEcmpSpgMaps() {
+ for (Device sw : srManager.deviceService.getDevices()) {
+ EcmpShortestPathGraph ecmpSpgUpdated =
+ new EcmpShortestPathGraph(sw.id(), srManager);
+ currentEcmpSpgMap.put(sw.id(), ecmpSpgUpdated);
+ }
+ }
+
+ /**
+ * Ensures routing is stable before updating all ECMP SPG graphs.
+ *
+ * TODO: CORD-1843 will ensure maps are updated faster, potentially while
+ * topology/routing is still unstable
+ */
+ private final class UpdateMaps implements Runnable {
+ @Override
+ public void run() {
+ if (isRoutingStable()) {
+ updateEcmpSpgMaps();
+ } else {
+ executorService.schedule(new UpdateMaps(), UPDATE_INTERVAL,
+ TimeUnit.SECONDS);
+ }
+ }
+ }
+
//////////////////////////////////////
// Filtering rule creation
//////////////////////////////////////
diff --git a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
index e00d865..39b901b 100644
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
@@ -1106,10 +1106,6 @@
private void processLinkAdded(Link link) {
log.info("** LINK ADDED {}", link.toString());
- if (!deviceConfiguration.isConfigured(link.src().deviceId())) {
- log.warn("Source device of this link is not configured..not processing");
- return;
- }
if (link.type() != Link.Type.DIRECT) {
// NOTE: A DIRECT link might be transiently marked as INDIRECT
// if BDDP is received before LLDP. We can safely ignore that
@@ -1118,6 +1114,13 @@
link.src(), link.dst(), link.type());
return;
}
+ if (!deviceConfiguration.isConfigured(link.src().deviceId())) {
+ updateSeenLink(link, true);
+ // XXX revisit - what about devicePortMap
+ log.warn("Source device of this link is not configured.. "
+ + "not processing further");
+ return;
+ }
//Irrespective of whether the local is a MASTER or not for this device,
//create group handler instance and push default TTP flow rules if needed,
@@ -1127,6 +1130,7 @@
if (groupHandler != null) {
groupHandler.portUpForLink(link);
} else {
+ // XXX revisit/cleanup
Device device = deviceService.getDevice(link.src().deviceId());
if (device != null) {
log.warn("processLinkAdded: Link Added "
@@ -1189,7 +1193,12 @@
if (groupHandler != null) {
if (mastershipService.isLocalMaster(link.src().deviceId()) &&
isParallelLink(link)) {
+ log.debug("* retrying hash for parallel link removed:{}", link);
groupHandler.retryHash(link, true, false);
+ } else {
+ log.debug("Not attempting retry-hash for link removed: {} .. {}", link,
+ (mastershipService.isLocalMaster(link.src().deviceId()))
+ ? "not parallel" : "not master");
}
// ensure local stores are updated
groupHandler.portDown(link.src().port());
diff --git a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/config/DeviceConfiguration.java b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/config/DeviceConfiguration.java
index d0dfa5e..11a8425 100644
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/config/DeviceConfiguration.java
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/config/DeviceConfiguration.java
@@ -267,7 +267,6 @@
public MacAddress getDeviceMac(DeviceId deviceId) throws DeviceConfigNotFoundException {
SegmentRouterInfo srinfo = deviceConfigMap.get(deviceId);
if (srinfo != null) {
- log.trace("getDeviceMac for device{} is {}", deviceId, srinfo.mac);
return srinfo.mac;
} else {
String message = "getDeviceMac fails for device: " + deviceId + ".";
diff --git a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultGroupHandler.java b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultGroupHandler.java
index 6f06d60..1a9a80e 100644
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultGroupHandler.java
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultGroupHandler.java
@@ -15,9 +15,9 @@
*/
package org.onosproject.segmentrouting.grouphandler;
-
-import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
import org.apache.commons.lang3.RandomUtils;
@@ -236,9 +236,9 @@
* not seen-before
*/
public void retryHash(Link link, boolean linkDown, boolean firstTime) {
- MacAddress dstMac;
+ MacAddress neighborMac;
try {
- dstMac = deviceConfig.getDeviceMac(link.dst().deviceId());
+ neighborMac = deviceConfig.getDeviceMac(link.dst().deviceId());
} catch (DeviceConfigNotFoundException e) {
log.warn(e.getMessage() + " Aborting retryHash.");
return;
@@ -264,124 +264,153 @@
int nextId = nextHops.nextId();
Set<DeviceId> dstSet = nextHops.getDstForNextHop(link.dst().deviceId());
if (!linkDown) {
- dstSet.forEach(dst -> {
- int edgeLabel = dsKey.destinationSet().getEdgeLabel(dst);
- addToHashedNextObjective(link.src().port(), dstMac,
- edgeLabel, nextId);
- });
-
+ List<PortLabel> pl = Lists.newArrayList();
if (firstTime) {
// some links may have come up before the next-objective was created
// we take this opportunity to ensure other ports to same next-hop-dst
// are part of the hash group (see CORD-1180). Duplicate additions
// to the same hash group are avoided by the driver.
for (PortNumber p : devicePortMap.get(link.dst().deviceId())) {
- if (p.equals(link.src().port())) {
- continue;
- }
dstSet.forEach(dst -> {
int edgeLabel = dsKey.destinationSet().getEdgeLabel(dst);
- addToHashedNextObjective(p, dstMac, edgeLabel, nextId);
+ pl.add(new PortLabel(p, edgeLabel));
});
}
+ addToHashedNextObjective(pl, neighborMac, nextId);
+ } else {
+ // handle only the port that came up
+ dstSet.forEach(dst -> {
+ int edgeLabel = dsKey.destinationSet().getEdgeLabel(dst);
+ pl.add(new PortLabel(link.src().port(), edgeLabel));
+ });
+ addToHashedNextObjective(pl, neighborMac, nextId);
}
} else {
+ // linkdown
+ List<PortLabel> pl = Lists.newArrayList();
dstSet.forEach(dst -> {
int edgeLabel = dsKey.destinationSet().getEdgeLabel(dst);
- removeFromHashedNextObjective(link.src().port(), dstMac,
- edgeLabel, nextId);
+ pl.add(new PortLabel(link.src().port(), edgeLabel));
});
+ removeFromHashedNextObjective(pl, neighborMac, nextId);
}
}
}
/**
- * Makes a call to the FlowObjective service to add a single bucket to
- * a hashed group.
+ * Utility class for associating output ports and the corresponding MPLS
+ * labels to push. In dual-homing, there are different labels to push
+ * corresponding to the destination switches in an edge-pair. If both
+ * destinations are reachable via the same spine, then the output-port to
+ * the spine will be associated with two labels i.e. there will be two
+ * PortLabel objects for the same port but with different labels.
+ */
+ private class PortLabel {
+ PortNumber port;
+ int edgeLabel;
+
+ PortLabel(PortNumber port, int edgeLabel) {
+ this.port = port;
+ this.edgeLabel = edgeLabel;
+ }
+
+ @Override
+ public String toString() {
+ return port.toString() + "/" + String.valueOf(edgeLabel);
+ }
+ }
+
+ /**
+ * Makes a call to the FlowObjective service to add buckets to
+ * a hashed group. User must ensure that all the ports & labels are meant
+ * same neighbor (ie. dstMac).
*
- * @param outport port to add to hash group
+ * @param portLables a collection of port & label combinations to add
+ * to the hash group identified by the nextId
* @param dstMac destination mac address of next-hop
- * @param edgeLabel the label to use in the bucket
- * @param nextId id for next-objective to which the bucket will be added
+ * @param nextId id for next-objective to which buckets will be added
*
*/
- private void addToHashedNextObjective(PortNumber outport, MacAddress dstMac,
- int edgeLabel, Integer nextId) {
- // Create the new bucket to be updated
- TrafficTreatment.Builder tBuilder =
- DefaultTrafficTreatment.builder();
- tBuilder.setOutput(outport)
- .setEthDst(dstMac)
- .setEthSrc(nodeMacAddr);
- if (edgeLabel != DestinationSet.NO_EDGE_LABEL) {
- tBuilder.pushMpls()
- .copyTtlOut()
- .setMpls(MplsLabel.mplsLabel(edgeLabel));
- }
+ private void addToHashedNextObjective(Collection<PortLabel> portLabels,
+ MacAddress dstMac, Integer nextId) {
// setup metadata to pass to nextObjective - indicate the vlan on egress
// if needed by the switch pipeline. Since hashed next-hops are always to
// other neighboring routers, there is no subnet assigned on those ports.
TrafficSelector.Builder metabuilder = DefaultTrafficSelector.builder();
metabuilder.matchVlanId(INTERNAL_VLAN);
-
NextObjective.Builder nextObjBuilder = DefaultNextObjective.builder()
.withId(nextId)
.withType(NextObjective.Type.HASHED)
- .addTreatment(tBuilder.build())
.withMeta(metabuilder.build())
.fromApp(appId);
- log.debug("addToHash in device {}: Adding Bucket with port/label {}/{} "
- + "to nextId {}", deviceId, outport, edgeLabel, nextId);
+ // Create the new buckets to be updated
+ portLabels.forEach(pl -> {
+ TrafficTreatment.Builder tBuilder = DefaultTrafficTreatment.builder();
+ tBuilder.setOutput(pl.port)
+ .setEthDst(dstMac)
+ .setEthSrc(nodeMacAddr);
+ if (pl.edgeLabel != DestinationSet.NO_EDGE_LABEL) {
+ tBuilder.pushMpls()
+ .copyTtlOut()
+ .setMpls(MplsLabel.mplsLabel(pl.edgeLabel));
+ }
+ nextObjBuilder.addTreatment(tBuilder.build());
+ });
+
+ log.debug("addToHash in device {}: Adding Bucket with port/label {} "
+ + "to nextId {}", deviceId, portLabels, nextId);
ObjectiveContext context = new DefaultObjectiveContext(
- (objective) -> log.debug("addToHash addedTo NextObj {} on {}",
- nextId, deviceId),
+ (objective) -> log.debug("addToHash port/label {} addedTo "
+ + "NextObj {} on {}", portLabels, nextId, deviceId),
(objective, error) ->
- log.warn("addToHash failed to addTo NextObj {} on {}: {}",
+ log.warn("addToHash failed to add port/label {} to"
+ + " NextObj {} on {}: {}", portLabels,
nextId, deviceId, error));
NextObjective nextObjective = nextObjBuilder.addToExisting(context);
flowObjectiveService.next(deviceId, nextObjective);
}
/**
- * Makes a call to the FlowObjective service to remove a single bucket from
- * a hashed group.
+ * Makes a call to the FlowObjective service to remove buckets from
+ * a hash group. User must ensure that all the ports & labels are meant
+ * same neighbor (ie. dstMac).
*
- * @param port port to remove from hash group
+ * @param portLables a collection of port & label combinations to remove
+ * from the hash group identified by the nextId
* @param dstMac destination mac address of next-hop
- * @param edgeLabel the label to use in the bucket
- * @param nextId id for next-objective from which the bucket will be removed
+ * @param nextId id for next-objective from which buckets will be removed
*/
- private void removeFromHashedNextObjective(PortNumber port, MacAddress dstMac,
- int edgeLabel, Integer nextId) {
- // Create the bucket to be removed
- TrafficTreatment.Builder tBuilder = DefaultTrafficTreatment
- .builder();
- tBuilder.setOutput(port)
- .setEthDst(dstMac)
- .setEthSrc(nodeMacAddr);
- if (edgeLabel != DestinationSet.NO_EDGE_LABEL) {
- tBuilder.pushMpls()
- .copyTtlOut()
- .setMpls(MplsLabel.mplsLabel(edgeLabel));
- }
- log.info("{} in device {}: Removing Bucket with Port {} to next object id {}",
- "removeFromHash", deviceId, port, nextId);
+ private void removeFromHashedNextObjective(Collection<PortLabel> portLabels,
+ MacAddress dstMac, Integer nextId) {
NextObjective.Builder nextObjBuilder = DefaultNextObjective
.builder()
.withType(NextObjective.Type.HASHED) //same as original
.withId(nextId)
- .fromApp(appId)
- .addTreatment(tBuilder.build());
- ObjectiveContext context = new DefaultObjectiveContext(
- (objective) -> log.debug("port {} removedFrom NextObj {} on {}",
- port, nextId, deviceId),
- (objective, error) ->
- log.warn("port {} failed to removeFrom NextObj {} on {}: {}",
- port, nextId, deviceId, error));
- NextObjective nextObjective = nextObjBuilder.
- removeFromExisting(context);
+ .fromApp(appId);
+ // Create the buckets to be removed
+ portLabels.forEach(pl -> {
+ TrafficTreatment.Builder tBuilder = DefaultTrafficTreatment.builder();
+ tBuilder.setOutput(pl.port)
+ .setEthDst(dstMac)
+ .setEthSrc(nodeMacAddr);
+ if (pl.edgeLabel != DestinationSet.NO_EDGE_LABEL) {
+ tBuilder.pushMpls()
+ .copyTtlOut()
+ .setMpls(MplsLabel.mplsLabel(pl.edgeLabel));
+ }
+ nextObjBuilder.addTreatment(tBuilder.build());
+ });
+ log.debug("removeFromHash in device {}: Removing Bucket with port/label"
+ + " {} from nextId {}", deviceId, portLabels, nextId);
+ ObjectiveContext context = new DefaultObjectiveContext(
+ (objective) -> log.debug("port/label {} removedFrom NextObj"
+ + " {} on {}", portLabels, nextId, deviceId),
+ (objective, error) ->
+ log.warn("port/label {} failed to removeFrom NextObj {} on "
+ + "{}: {}", portLabels, nextId, deviceId, error));
+ NextObjective nextObjective = nextObjBuilder.removeFromExisting(context);
flowObjectiveService.next(deviceId, nextObjective);
}
@@ -405,7 +434,7 @@
// temporary storage of keys to be updated
Map<DestinationSetNextObjectiveStoreKey, Set<DeviceId>> tempStore =
new HashMap<>();
- boolean foundNextObjective = false;
+ boolean foundNextObjective = false, success = true;
// retrieve hash-groups meant for destSw, which have destinationSets
// with different neighbors than the given next-hops
@@ -432,44 +461,17 @@
+ "hops:{} ..adding {}", targetSw, destSw, nextId,
currNeighbors, diff);
}
- for (DeviceId neighbor : diff) {
- MacAddress dstMac;
- try {
- dstMac = deviceConfig.getDeviceMac(neighbor);
- } catch (DeviceConfigNotFoundException e) {
- log.warn(e.getMessage() + " Aborting fixHashGroup for nextId:"
- + nextId);
- return false;
- }
- if (devicePortMap.get(neighbor) == null ||
- devicePortMap.get(neighbor).isEmpty()) {
- log.warn("No ports found in dev:{} for neighbor:{} .. cannot "
- + "fix hash group for nextId: {}",
- deviceId, neighbor, nextId);
- return false;
- }
+ boolean suc = updateAllPortsToNextHop(diff, edgeLabel, nextId,
+ revoke);
+ if (suc) {
+ // to update neighbor set with changes made
if (revoke) {
- for (PortNumber port : devicePortMap.get(neighbor)) {
- log.info("fixHashGroup in device {}: Removing Bucket "
- + "with Port {} to next object id {}",
- deviceId, port, nextId);
- removeFromHashedNextObjective(port, dstMac,
- edgeLabel,
- nextId);
- }
- // to update neighbor set with changes made
tempStore.put(dskey, Sets.difference(currNeighbors, diff));
} else {
- for (PortNumber port : devicePortMap.get(neighbor)) {
- log.info("fixHashGroup in device {}: Adding Bucket "
- + "with Port {} to next object id {}",
- deviceId, port, nextId);
- addToHashedNextObjective(port, dstMac, edgeLabel, nextId);
- }
- // to update neighbor set with changes made
tempStore.put(dskey, Sets.union(currNeighbors, diff));
}
}
+ success &= suc;
}
if (!foundNextObjective) {
@@ -480,18 +482,29 @@
// update the dsNextObjectiveStore with new destinationSet to nextId mappings
for (DestinationSetNextObjectiveStoreKey key : tempStore.keySet()) {
- NextNeighbors oldHops = dsNextObjStore.get(key);
- if (oldHops == null) {
+ NextNeighbors currentNextHops = dsNextObjStore.get(key);
+ if (currentNextHops == null) {
+ log.warn("fixHashGroups could not update global store in "
+ + "device {} .. missing nextNeighbors for key {}",
+ deviceId, key);
continue;
}
- Set<DeviceId> newNeighbors = tempStore.get(key);
- Set<DeviceId> oldNeighbors = ImmutableSet.copyOf(oldHops.nextHops(destSw));
- oldHops.dstNextHops().put(destSw, newNeighbors);
- log.debug("Updating nsNextObjStore: oldHops:{} -> newHops:{} :: nextId:{}",
- oldNeighbors, newNeighbors, oldHops.nextId());
+ Set<DeviceId> newNeighbors = new HashSet<>();
+ newNeighbors.addAll(tempStore.get(key));
+ Map<DeviceId, Set<DeviceId>> oldDstNextHops =
+ ImmutableMap.copyOf(currentNextHops.dstNextHops());
+ currentNextHops.dstNextHops().put(destSw, newNeighbors); //local change
+ log.debug("Updating nsNextObjStore target:{} -> dst:{} in key:{} nextId:{}",
+ targetSw, destSw, key, currentNextHops.nextId());
+ log.debug("Old dstNextHops: {}", oldDstNextHops);
+ log.debug("New dstNextHops: {}", currentNextHops.dstNextHops());
+ // update global store
+ dsNextObjStore.put(key,
+ new NextNeighbors(currentNextHops.dstNextHops(),
+ currentNextHops.nextId()));
}
-
- return true;
+ // even if one fails and others succeed, return false so ECMPspg not updated
+ return success;
}
/**
@@ -543,7 +556,9 @@
}
/**
- * Adds or removes buckets for all ports to a set of neighbor devices.
+ * Adds or removes buckets for all ports to a set of neighbor devices. Caller
+ * needs to ensure that the given neighbors are all next hops towards the
+ * same destination (represented by the given edgeLabel).
*
* @param neighbors set of neighbor device ids
* @param edgeLabel MPLS label to use in buckets
@@ -556,37 +571,33 @@
private boolean updateAllPortsToNextHop(Set<DeviceId> neighbors, int edgeLabel,
int nextId, boolean revoke) {
for (DeviceId neighbor : neighbors) {
- MacAddress dstMac;
+ MacAddress neighborMac;
try {
- dstMac = deviceConfig.getDeviceMac(neighbor);
+ neighborMac = deviceConfig.getDeviceMac(neighbor);
} catch (DeviceConfigNotFoundException e) {
- log.warn(e.getMessage() + " Aborting fixHashGroup for nextId:"
- + nextId);
+ log.warn(e.getMessage() + " Aborting updateAllPortsToNextHop"
+ + " for nextId:" + nextId);
return false;
}
- if (devicePortMap.get(neighbor) == null ||
- devicePortMap.get(neighbor).isEmpty()) {
+ Collection<PortNumber> portsToNeighbor = devicePortMap.get(neighbor);
+ if (portsToNeighbor == null || portsToNeighbor.isEmpty()) {
log.warn("No ports found in dev:{} for neighbor:{} .. cannot "
- + "fix hash group for nextId: {}",
+ + "updateAllPortsToNextHop for nextId: {}",
deviceId, neighbor, nextId);
return false;
}
+ List<PortLabel> pl = Lists.newArrayList();
+ portsToNeighbor.forEach(p -> pl.add(new PortLabel(p, edgeLabel)));
if (revoke) {
- for (PortNumber port : devicePortMap.get(neighbor)) {
- log.debug("fixHashGroup in device {}: Removing Bucket "
- + "with Port {} edgeLabel:{} to next object id {}",
- deviceId, port, edgeLabel, nextId);
- removeFromHashedNextObjective(port, dstMac,
- edgeLabel,
- nextId);
- }
+ log.debug("updateAllPortsToNextHops in device {}: Removing Bucket(s) "
+ + "with Port/Label:{} to next object id {}",
+ deviceId, pl, nextId);
+ removeFromHashedNextObjective(pl, neighborMac, nextId);
} else {
- for (PortNumber port : devicePortMap.get(neighbor)) {
- log.debug("fixHashGroup in device {}: Adding Bucket "
- + "with Port {} edgeLabel: {} to next object id {}",
- deviceId, port, edgeLabel, nextId);
- addToHashedNextObjective(port, dstMac, edgeLabel, nextId);
- }
+ log.debug("fixHashGroup in device {}: Adding Bucket(s) "
+ + "with Port/Label: {} to next object id {}",
+ deviceId, pl, nextId);
+ addToHashedNextObjective(pl, neighborMac, nextId);
}
}
return true;
@@ -1124,8 +1135,13 @@
/**
- *
- *
+ * Performs bucket verification operation for all hash groups in this device.
+ * Checks RouteHandler to ensure that routing is stable before attempting
+ * verification. Verification involves creating a nextObjective with
+ * operation VERIFY for existing next objectives in the store, and passing
+ * it to the driver. It is the driver that actually performs the verification
+ * by adding or removing buckets to match the verification next objective
+ * created here.
*/
protected final class BucketCorrector implements Runnable {
Integer nextId;
@@ -1152,7 +1168,7 @@
}
rh.acquireRoutingLock();
try {
- log.debug("running bucket corrector for dev: {}", deviceId);
+ log.trace("running bucket corrector for dev: {}", deviceId);
Set<DestinationSetNextObjectiveStoreKey> dsKeySet = dsNextObjStore.entrySet()
.stream()
.filter(entry -> entry.getKey().deviceId().equals(deviceId))
@@ -1167,7 +1183,7 @@
if (nextId != null && nextId != nid) {
continue;
}
- log.debug("bkt-corr: dsNextObjStore for device {}: {}",
+ log.trace("bkt-corr: dsNextObjStore for device {}: {}",
deviceId, dsKey, next);
TrafficSelector.Builder metabuilder = DefaultTrafficSelector.builder();
metabuilder.matchVlanId(INTERNAL_VLAN);
@@ -1189,7 +1205,7 @@
return;
}
devicePortMap.get(neighbor).forEach(port -> {
- log.debug("verify in device {} nextId {}: bucket with"
+ log.trace("verify in device {} nextId {}: bucket with"
+ " port/label {}/{} to dst {} via {}",
deviceId, nid, port, edgeLabel,
dstDev, neighbor);
diff --git a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/grouphandler/NextNeighbors.java b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/grouphandler/NextNeighbors.java
index 1a0507b..eaca1aa 100644
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/grouphandler/NextNeighbors.java
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/grouphandler/NextNeighbors.java
@@ -25,27 +25,60 @@
import org.onosproject.net.DeviceId;
+/**
+ * Represents the nexthop information associated with a route-path towards a
+ * set of destinations.
+ */
public class NextNeighbors {
private final Map<DeviceId, Set<DeviceId>> dstNextHops;
private final int nextId;
+ /**
+ * Constructor.
+ *
+ * @param dstNextHops map of destinations and the next-hops towards each dest
+ * @param nextId id of nextObjective that manifests the next-hop info
+ */
public NextNeighbors(Map<DeviceId, Set<DeviceId>> dstNextHops, int nextId) {
this.dstNextHops = dstNextHops;
this.nextId = nextId;
}
+ /**
+ * Returns a map of destinations and the next-hops towards them.
+ *
+ * @return map of destinations and the next-hops towards them
+ */
public Map<DeviceId, Set<DeviceId>> dstNextHops() {
return dstNextHops;
}
+ /**
+ * Set of next-hops towards the given destination.
+ *
+ * @param deviceId the destination
+ * @return set of nexthops towards the destination
+ */
public Set<DeviceId> nextHops(DeviceId deviceId) {
return dstNextHops.get(deviceId);
}
+ /**
+ * Return the nextId representing the nextObjective towards the next-hops.
+ *
+ * @return nextId representing the nextObjective towards the next-hops
+ */
public int nextId() {
return nextId;
}
+ /**
+ * Checks if the given nextHopId is a valid next hop to any one of the
+ * destinations.
+ *
+ * @param nextHopId the deviceId for the next hop
+ * @return true if given next
+ */
public boolean containsNextHop(DeviceId nextHopId) {
for (Set<DeviceId> nextHops : dstNextHops.values()) {
if (nextHops != null && nextHops.contains(nextHopId)) {
@@ -55,6 +88,14 @@
return false;
}
+ /**
+ * Returns a set of destinations which have the given nextHopId as one
+ * of the next-hops to that destination.
+ *
+ * @param nextHopId the deviceId for the next hop
+ * @return set of deviceIds that have the given nextHopId as a next-hop
+ * which could be empty if no destinations were found
+ */
public Set<DeviceId> getDstForNextHop(DeviceId nextHopId) {
Set<DeviceId> dstSet = new HashSet<>();
for (DeviceId dstKey : dstNextHops.keySet()) {
diff --git a/core/store/serializers/src/main/java/org/onosproject/store/serializers/KryoNamespaces.java b/core/store/serializers/src/main/java/org/onosproject/store/serializers/KryoNamespaces.java
index fcf6dc7..477b466 100644
--- a/core/store/serializers/src/main/java/org/onosproject/store/serializers/KryoNamespaces.java
+++ b/core/store/serializers/src/main/java/org/onosproject/store/serializers/KryoNamespaces.java
@@ -20,6 +20,8 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Maps;
+import com.google.common.collect.Sets;
+
import org.onlab.packet.ChassisId;
import org.onlab.packet.EthType;
import org.onlab.packet.Ip4Address;
@@ -272,6 +274,7 @@
LinkedHashSet.class
)
.register(HashMultiset.class)
+ .register(Sets.class)
.register(Maps.immutableEntry("a", "b").getClass())
.register(new ArraysAsListSerializer(), Arrays.asList().getClass())
.register(Collections.singletonList(1).getClass())
diff --git a/drivers/default/src/main/java/org/onosproject/driver/pipeline/ofdpa/Ofdpa2GroupHandler.java b/drivers/default/src/main/java/org/onosproject/driver/pipeline/ofdpa/Ofdpa2GroupHandler.java
index 4f16cff..24ababc 100644
--- a/drivers/default/src/main/java/org/onosproject/driver/pipeline/ofdpa/Ofdpa2GroupHandler.java
+++ b/drivers/default/src/main/java/org/onosproject/driver/pipeline/ofdpa/Ofdpa2GroupHandler.java
@@ -42,6 +42,7 @@
import org.onosproject.net.flow.criteria.VlanIdCriterion;
import org.onosproject.net.flow.instructions.Instruction;
import org.onosproject.net.flow.instructions.Instructions;
+import org.onosproject.net.flow.instructions.Instructions.GroupInstruction;
import org.onosproject.net.flow.instructions.L2ModificationInstruction;
import org.onosproject.net.flowobjective.DefaultNextObjective;
import org.onosproject.net.flowobjective.FlowObjectiveStore;
@@ -1497,7 +1498,6 @@
List<Deque<GroupKey>> allActiveKeys = appKryo.deserialize(next.data());
List<TrafficTreatment> bucketsToCreate = Lists.newArrayList();
List<Integer> indicesToRemove = Lists.newArrayList();
- // XXX verify empty group
for (TrafficTreatment bkt : nextObjective.next()) {
PortNumber portNumber = readOutPortFromTreatment(bkt);
int label = readLabelFromTreatment(bkt);
@@ -1544,6 +1544,100 @@
removeBucket(chainsToRemove, nextObjective);
}
+ if (bucketsToCreate.isEmpty() && indicesToRemove.isEmpty()) {
+ // flowObjective store record is in-sync with nextObjective passed-in
+ // Nevertheless groupStore may not be in sync due to bug in the store
+ // - see CORD-1844. XXX When this bug is fixed, the rest of this verify
+ // method will not be required.
+ GroupKey hashGroupKey = allActiveKeys.get(0).peekFirst();
+ Group hashGroup = groupService.getGroup(deviceId, hashGroupKey);
+ int actualGroupSize = hashGroup.buckets().buckets().size();
+ int objGroupSize = nextObjective.next().size();
+ if (actualGroupSize != objGroupSize) {
+ log.warn("Mismatch detected in device:{}, nextId:{}, nextObjective-size"
+ + ":{} group-size:{} .. correcting", deviceId, nextObjective.id(),
+ objGroupSize, actualGroupSize);
+ }
+ if (actualGroupSize > objGroupSize) {
+ List<GroupBucket> bucketsToRemove = Lists.newArrayList();
+ //check every bucket in the actual group
+ for (GroupBucket bucket : hashGroup.buckets().buckets()) {
+ GroupInstruction g = (GroupInstruction) bucket.treatment()
+ .allInstructions().iterator().next();
+ GroupId gidToCheck = g.groupId(); // the group pointed to
+ boolean matches = false;
+ for (Deque<GroupKey> validChain : allActiveKeys) {
+ if (validChain.size() < 2) {
+ continue;
+ }
+ GroupKey pointedGroupKey = validChain.stream()
+ .collect(Collectors.toList()).get(1);
+ Group pointedGroup = groupService.getGroup(deviceId, pointedGroupKey);
+ if (pointedGroup != null && gidToCheck.equals(pointedGroup.id())) {
+ matches = true;
+ break;
+ }
+ }
+ if (!matches) {
+ log.warn("Removing bucket pointing to groupId:{}", gidToCheck);
+ bucketsToRemove.add(bucket);
+ }
+ }
+ // remove buckets for which there was no record in the obj store
+ if (bucketsToRemove.isEmpty()) {
+ log.warn("Mismatch detected but could not determine which"
+ + "buckets to remove");
+ } else {
+ GroupBuckets removeBuckets = new GroupBuckets(bucketsToRemove);
+ groupService.removeBucketsFromGroup(deviceId, hashGroupKey,
+ removeBuckets, hashGroupKey,
+ nextObjective.appId());
+ }
+ } else if (actualGroupSize < objGroupSize) {
+ // should also add buckets not in group-store but in obj-store
+ List<GroupBucket> bucketsToAdd = Lists.newArrayList();
+ //check every bucket in the obj
+ for (Deque<GroupKey> validChain : allActiveKeys) {
+ if (validChain.size() < 2) {
+ continue;
+ }
+ GroupKey pointedGroupKey = validChain.stream()
+ .collect(Collectors.toList()).get(1);
+ Group pointedGroup = groupService.getGroup(deviceId, pointedGroupKey);
+ if (pointedGroup == null) {
+ // group should exist, otherwise cannot be added as bucket
+ continue;
+ }
+ boolean matches = false;
+ for (GroupBucket bucket : hashGroup.buckets().buckets()) {
+ GroupInstruction g = (GroupInstruction) bucket.treatment()
+ .allInstructions().iterator().next();
+ GroupId gidToCheck = g.groupId(); // the group pointed to
+ if (pointedGroup.id().equals(gidToCheck)) {
+ matches = true;
+ break;
+ }
+ }
+ if (!matches) {
+ log.warn("Adding bucket pointing to groupId:{}", pointedGroup);
+ TrafficTreatment t = DefaultTrafficTreatment.builder()
+ .group(pointedGroup.id())
+ .build();
+ bucketsToAdd.add(DefaultGroupBucket.createSelectGroupBucket(t));
+ }
+ }
+ if (bucketsToAdd.isEmpty()) {
+ log.warn("Mismatch detected but could not determine which "
+ + "buckets to add");
+ } else {
+ GroupBuckets addBuckets = new GroupBuckets(bucketsToAdd);
+ groupService.addBucketsToGroup(deviceId, hashGroupKey,
+ addBuckets, hashGroupKey,
+ nextObjective.appId());
+ }
+ }
+ }
+
pass(nextObjective);
}
@@ -1733,7 +1827,6 @@
private class InnerGroupListener implements GroupListener {
@Override
public void event(GroupEvent event) {
- log.trace("received group event of type {}", event.type());
switch (event.type()) {
case GROUP_ADDED:
processPendingAddGroupsOrNextObjs(event.subject().appCookie(), true);