[ONOS-8086] Groups entries are not deleted
Introduces a new method to update the stats, instead of using addOrUpdate.
In this way, we can handle properly the PENDING_DELETE state.
Change-Id: Iee8eb2398d8d8b4a1b6105ad19b9d733056eb1bb
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 8db3385..2734b8b 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
@@ -908,6 +908,7 @@
existing.id(),
existing.deviceId(),
existing.state());
+ // TODO is this really safe ?
synchronized (existing) {
existing.setState(GroupState.PENDING_DELETE);
getGroupStoreKeyMap().
@@ -920,6 +921,36 @@
}
/**
+ * Updates the stats of an existing group entry.
+ *
+ * @param group the new stats
+ * @param existing the existing group
+ */
+ private void updateGroupEntryStatsInternal(Group group, StoredGroupEntry existing) {
+ for (GroupBucket bucket : group.buckets().buckets()) {
+ Optional<GroupBucket> matchingBucket =
+ existing.buckets().buckets()
+ .stream()
+ .filter((existingBucket) -> (existingBucket.equals(bucket)))
+ .findFirst();
+ if (matchingBucket.isPresent()) {
+ ((StoredGroupBucketEntry) matchingBucket.
+ get()).setPackets(bucket.packets());
+ ((StoredGroupBucketEntry) matchingBucket.
+ get()).setBytes(bucket.bytes());
+ } else {
+ log.warn("updateGroupEntryStatsInternal: No matching bucket {}" +
+ " to update stats", bucket);
+ }
+ }
+ existing.setLife(group.life());
+ existing.setPackets(group.packets());
+ existing.setBytes(group.bytes());
+ existing.setReferenceCount(group.referenceCount());
+ existing.setFailedRetryCount(0);
+ }
+
+ /**
* Stores a new group entry, or updates an existing entry.
*
* @param group group entry
@@ -935,28 +966,10 @@
log.trace("addOrUpdateGroupEntry: updating group entry {} in device {}",
group.id(),
group.deviceId());
+ // TODO is this really safe ?
synchronized (existing) {
- for (GroupBucket bucket : group.buckets().buckets()) {
- Optional<GroupBucket> matchingBucket =
- existing.buckets().buckets()
- .stream()
- .filter((existingBucket) -> (existingBucket.equals(bucket)))
- .findFirst();
- if (matchingBucket.isPresent()) {
- ((StoredGroupBucketEntry) matchingBucket.
- get()).setPackets(bucket.packets());
- ((StoredGroupBucketEntry) matchingBucket.
- get()).setBytes(bucket.bytes());
- } else {
- log.warn("addOrUpdateGroupEntry: No matching "
- + "buckets to update stats");
- }
- }
- existing.setLife(group.life());
- existing.setPackets(group.packets());
- existing.setBytes(group.bytes());
- existing.setReferenceCount(group.referenceCount());
- existing.setFailedRetryCount(0);
+ // Update stats
+ updateGroupEntryStatsInternal(group, existing);
if ((existing.state() == GroupState.PENDING_ADD) ||
(existing.state() == GroupState.PENDING_ADD_RETRY)) {
log.trace("addOrUpdateGroupEntry: group entry {} in device {} moving from {} to ADDED",
@@ -981,17 +994,69 @@
existing.appCookie()), existing);
}
} else {
- log.warn("addOrUpdateGroupEntry: Group update "
- + "happening for a non-existing entry in the map");
+ log.warn("addOrUpdateGroupEntry: Group update {} " +
+ "happening for a non-existing entry in the map", group);
}
- // XXX if map is going to trigger event, is this one needed?
+ // TODO if map is going to trigger event, is this one needed?
if (event != null) {
notifyDelegate(event);
}
}
/**
+ * Updates stats of an existing entry.
+ *
+ * @param group group entry
+ */
+ private void updateGroupEntryStats(Group group) {
+ // check if this new entry is an update to an existing entry
+ StoredGroupEntry existing = getStoredGroupEntry(group.deviceId(),
+ group.id());
+ if (existing != null) {
+ log.trace("updateStatsGroupEntry: updating group entry {} in device {}",
+ group.id(),
+ group.deviceId());
+ // TODO is this really safe ?
+ synchronized (existing) {
+ // We don't make further update - it will be gone after the next update
+ if (existing.state() == GroupState.PENDING_DELETE) {
+ log.trace("updateStatsGroupEntry: group entry {} in device {} is in {} not updated",
+ existing.id(),
+ existing.deviceId(),
+ existing.state());
+ return;
+ }
+ // Update stats
+ updateGroupEntryStatsInternal(group, existing);
+ if ((existing.state() == GroupState.PENDING_ADD) ||
+ (existing.state() == GroupState.PENDING_ADD_RETRY)) {
+ log.trace("updateStatsGroupEntry: group entry {} in device {} moving from {} to ADDED",
+ existing.id(),
+ existing.deviceId(),
+ existing.state());
+ existing.setState(GroupState.ADDED);
+ existing.setIsGroupStateAddedFirstTime(true);
+ } else {
+ log.trace("updateStatsGroupEntry: group entry {} in device {} moving from {} to ADDED",
+ existing.id(),
+ existing.deviceId(),
+ GroupState.PENDING_UPDATE);
+ existing.setState(GroupState.ADDED);
+ existing.setIsGroupStateAddedFirstTime(false);
+ }
+ //Re-PUT map entries to trigger map update events
+ getGroupStoreKeyMap().
+ put(new GroupStoreKeyMapKey(existing.deviceId(),
+ existing.appCookie()), existing);
+ }
+ } else {
+ log.warn("updateStatsGroupEntry: Group update {} "
+ + "happening for a non-existing entry in the map", group);
+ }
+ }
+
+ /**
* Removes the group entry from store.
*
* @param group group entry
@@ -1580,6 +1645,6 @@
private void groupAdded(Group group) {
log.trace("Group {} Added or Updated in device {}",
group, group.deviceId());
- addOrUpdateGroupEntry(group);
+ updateGroupEntryStats(group);
}
}