Fix a bug when processing Flow Entry notifications and updating the
locally stored FlowEntries. Previously, even if a Flow Entry
wasn't updated, it could trigger multiple writing of a FlowPath
to the Database. Now we explicitly check whether the
locally stored FlowEntry is actually updated.
As a side-effect of the above fix now we have to:
(1) Create a copy of a FlowEntry inside FlowManager.flowEntriesPushedToSwitch()
before modifying it and pushing it into the Datagrid
(2) We have to explicitly check whether all Flow Entries have been installed
into the switches before writing a FlowPath to the Graph DB.
diff --git a/src/main/java/net/onrc/onos/ofcontroller/flowmanager/FlowEventHandler.java b/src/main/java/net/onrc/onos/ofcontroller/flowmanager/FlowEventHandler.java
index 3538eb4..c36c53a 100644
--- a/src/main/java/net/onrc/onos/ofcontroller/flowmanager/FlowEventHandler.java
+++ b/src/main/java/net/onrc/onos/ofcontroller/flowmanager/FlowEventHandler.java
@@ -460,7 +460,7 @@
*/
private void processFlowEntryEvents() {
FlowPath flowPath;
- FlowEntry updatedFlowEntry;
+ FlowEntry localFlowEntry;
//
// Update Flow Entries with previously unmatched Flow Entry updates
@@ -471,13 +471,15 @@
flowPath = allFlowPaths.get(flowEntry.flowId().value());
if (flowPath == null)
continue;
- updatedFlowEntry = updateFlowEntryAdd(flowPath, flowEntry);
- if (updatedFlowEntry == null) {
+ localFlowEntry = findFlowEntryAdd(flowPath, flowEntry);
+ if (localFlowEntry == null) {
remainingUpdates.put(flowEntry.flowEntryId().value(),
flowEntry);
continue;
}
- modifiedFlowPaths.put(flowPath.flowId().value(), flowPath);
+ if (updateFlowEntryAdd(flowPath, localFlowEntry, flowEntry)) {
+ modifiedFlowPaths.put(flowPath.flowId().value(), flowPath);
+ }
}
unmatchedFlowEntryAdd = remainingUpdates;
}
@@ -505,15 +507,17 @@
flowEntry);
break;
}
- updatedFlowEntry = updateFlowEntryAdd(flowPath, flowEntry);
- if (updatedFlowEntry == null) {
+ localFlowEntry = findFlowEntryAdd(flowPath, flowEntry);
+ if (localFlowEntry == null) {
// Flow Entry not found: keep the entry for later matching
unmatchedFlowEntryAdd.put(flowEntry.flowEntryId().value(),
flowEntry);
break;
}
- // Add the updated entry to the list of updated Flow Entries
- modifiedFlowPaths.put(flowPath.flowId().value(), flowPath);
+ if (updateFlowEntryAdd(flowPath, localFlowEntry, flowEntry)) {
+ // Add the updated Flow Path to the list of updated paths
+ modifiedFlowPaths.put(flowPath.flowId().value(), flowPath);
+ }
break;
case ENTRY_REMOVE:
@@ -527,32 +531,37 @@
// Flow Path not found: ignore the update
break;
}
- updatedFlowEntry = updateFlowEntryRemove(flowPath, flowEntry);
- if (updatedFlowEntry == null) {
+ localFlowEntry = findFlowEntryRemove(flowPath, flowEntry);
+ if (localFlowEntry == null) {
// Flow Entry not found: ignore it
break;
}
- modifiedFlowPaths.put(flowPath.flowId().value(), flowPath);
+ if (updateFlowEntryRemove(flowPath, localFlowEntry,
+ flowEntry)) {
+ // Add the updated Flow Path to the list of updated paths
+ modifiedFlowPaths.put(flowPath.flowId().value(), flowPath);
+ }
break;
}
}
}
/**
- * Update a Flow Entry because of an external ENTRY_ADD event.
+ * Find a Flow Entry that should be updated because of an external
+ * ENTRY_ADD event.
*
* @param flowPath the FlowPath for the Flow Entry to update.
- * @param flowEntry the FlowEntry with the new state.
- * @return the updated Flow Entry if found, otherwise null.
+ * @param newFlowEntry the FlowEntry with the new state.
+ * @return the Flow Entry that should be updated if found, otherwise null.
*/
- private FlowEntry updateFlowEntryAdd(FlowPath flowPath,
- FlowEntry flowEntry) {
+ private FlowEntry findFlowEntryAdd(FlowPath flowPath,
+ FlowEntry newFlowEntry) {
//
// Iterate over all Flow Entries and find a match.
//
for (FlowEntry localFlowEntry : flowPath.flowEntries()) {
if (! TopologyManager.isSameFlowEntryDataPath(localFlowEntry,
- flowEntry)) {
+ newFlowEntry)) {
continue;
}
@@ -561,27 +570,89 @@
//
if (localFlowEntry.isValidFlowEntryId()) {
if (localFlowEntry.flowEntryId().value() !=
- flowEntry.flowEntryId().value()) {
+ newFlowEntry.flowEntryId().value()) {
//
// Find a local Flow Entry, but the Flow Entry ID doesn't
// match. Keep looking.
//
continue;
}
- } else {
- // Update the Flow Entry ID
- FlowEntryId flowEntryId =
- new FlowEntryId(flowEntry.flowEntryId().value());
- localFlowEntry.setFlowEntryId(flowEntryId);
}
+ return localFlowEntry;
+ }
- //
- // Update the local Flow Entry, and keep state to check
- // if the Flow Path has been installed.
- //
- localFlowEntry.setFlowEntryUserState(flowEntry.flowEntryUserState());
- localFlowEntry.setFlowEntrySwitchState(flowEntry.flowEntrySwitchState());
+ return null; // Entry not found
+ }
+
+ /**
+ * Update a Flow Entry because of an external ENTRY_ADD event.
+ *
+ * @param flowPath the FlowPath for the Flow Entry to update.
+ * @param localFlowEntry the local Flow Entry to update.
+ * @param newFlowEntry the FlowEntry with the new state.
+ * @return true if the local Flow Entry was updated, otherwise false.
+ */
+ private boolean updateFlowEntryAdd(FlowPath flowPath,
+ FlowEntry localFlowEntry,
+ FlowEntry newFlowEntry) {
+ boolean updated = false;
+
+ if (localFlowEntry.flowEntryUserState() ==
+ FlowEntryUserState.FE_USER_DELETE) {
+ // Don't add-back a Flow Entry that is already deleted
+ return false;
+ }
+
+ if (! localFlowEntry.isValidFlowEntryId()) {
+ // Update the Flow Entry ID
+ FlowEntryId flowEntryId =
+ new FlowEntryId(newFlowEntry.flowEntryId().value());
+ localFlowEntry.setFlowEntryId(flowEntryId);
+ updated = true;
+ }
+
+ //
+ // Update the local Flow Entry, and keep state to check
+ // if the Flow Path has been installed.
+ //
+ if (localFlowEntry.flowEntryUserState() !=
+ newFlowEntry.flowEntryUserState()) {
+ localFlowEntry.setFlowEntryUserState(
+ newFlowEntry.flowEntryUserState());
+ updated = true;
+ }
+ if (localFlowEntry.flowEntrySwitchState() !=
+ newFlowEntry.flowEntrySwitchState()) {
+ localFlowEntry.setFlowEntrySwitchState(
+ newFlowEntry.flowEntrySwitchState());
checkIfInstalledFlowPaths.put(flowPath.flowId().value(), flowPath);
+ updated = true;
+ }
+
+ return updated;
+ }
+
+ /**
+ * Find a Flow Entry that should be updated because of an external
+ * ENTRY_REMOVE event.
+ *
+ * @param flowPath the FlowPath for the Flow Entry to update.
+ * @param newFlowEntry the FlowEntry with the new state.
+ * @return the Flow Entry that should be updated if found, otherwise null.
+ */
+ private FlowEntry findFlowEntryRemove(FlowPath flowPath,
+ FlowEntry newFlowEntry) {
+ //
+ // Iterate over all Flow Entries and find a match based on
+ // the Flow Entry ID.
+ //
+ for (FlowEntry localFlowEntry : flowPath.flowEntries()) {
+ if (! localFlowEntry.isValidFlowEntryId())
+ continue;
+ if (localFlowEntry.flowEntryId().value() !=
+ newFlowEntry.flowEntryId().value()) {
+ continue;
+ }
return localFlowEntry;
}
@@ -592,31 +663,32 @@
* Update a Flow Entry because of an external ENTRY_REMOVE event.
*
* @param flowPath the FlowPath for the Flow Entry to update.
- * @param flowEntry the FlowEntry with the new state.
- * @return the updated Flow Entry if found, otherwise null.
+ * @param localFlowEntry the local Flow Entry to update.
+ * @param newFlowEntry the FlowEntry with the new state.
+ * @return true if the local Flow Entry was updated, otherwise false.
*/
- private FlowEntry updateFlowEntryRemove(FlowPath flowPath,
- FlowEntry flowEntry) {
+ private boolean updateFlowEntryRemove(FlowPath flowPath,
+ FlowEntry localFlowEntry,
+ FlowEntry newFlowEntry) {
+ boolean updated = false;
+
//
- // Iterate over all Flow Entries and find a match based on
- // the Flow Entry ID.
+ // Update the local Flow Entry.
//
- for (FlowEntry localFlowEntry : flowPath.flowEntries()) {
- if (! localFlowEntry.isValidFlowEntryId())
- continue;
- if (localFlowEntry.flowEntryId().value() !=
- flowEntry.flowEntryId().value()) {
- continue;
- }
- //
- // Update the local Flow Entry.
- //
- localFlowEntry.setFlowEntryUserState(flowEntry.flowEntryUserState());
- localFlowEntry.setFlowEntrySwitchState(flowEntry.flowEntrySwitchState());
- return localFlowEntry;
+ if (localFlowEntry.flowEntryUserState() !=
+ newFlowEntry.flowEntryUserState()) {
+ localFlowEntry.setFlowEntryUserState(
+ newFlowEntry.flowEntryUserState());
+ updated = true;
+ }
+ if (localFlowEntry.flowEntrySwitchState() !=
+ newFlowEntry.flowEntrySwitchState()) {
+ localFlowEntry.setFlowEntrySwitchState(
+ newFlowEntry.flowEntrySwitchState());
+ updated = true;
}
- return null; // Entry not found
+ return updated;
}
/**
diff --git a/src/main/java/net/onrc/onos/ofcontroller/flowmanager/FlowManager.java b/src/main/java/net/onrc/onos/ofcontroller/flowmanager/FlowManager.java
index 82680df..88a0cbf 100644
--- a/src/main/java/net/onrc/onos/ofcontroller/flowmanager/FlowManager.java
+++ b/src/main/java/net/onrc/onos/ofcontroller/flowmanager/FlowManager.java
@@ -405,32 +405,40 @@
//
// Process all entries
//
+ // TODO: For now we have to create an explicit FlowEntry copy so
+ // we don't modify the original FlowEntry.
+ // This should go away after we start using the OpenFlow Barrier
+ // mechnanism in the FlowPusher.
+ //
+ Kryo kryo = kryoFactory.newKryo();
for (Pair<IOFSwitch, FlowEntry> entry : entries) {
FlowEntry flowEntry = entry.second;
//
// Mark the Flow Entry that it has been pushed to the switch
//
- flowEntry.setFlowEntrySwitchState(FlowEntrySwitchState.FE_SWITCH_UPDATED);
+ FlowEntry copyFlowEntry = kryo.copy(flowEntry);
+ copyFlowEntry.setFlowEntrySwitchState(FlowEntrySwitchState.FE_SWITCH_UPDATED);
//
// Write the Flow Entry to the Datagrid
//
- switch (flowEntry.flowEntryUserState()) {
+ switch (copyFlowEntry.flowEntryUserState()) {
case FE_USER_ADD:
- datagridService.notificationSendFlowEntryAdded(flowEntry);
+ datagridService.notificationSendFlowEntryAdded(copyFlowEntry);
break;
case FE_USER_MODIFY:
- datagridService.notificationSendFlowEntryUpdated(flowEntry);
+ datagridService.notificationSendFlowEntryUpdated(copyFlowEntry);
break;
case FE_USER_DELETE:
- datagridService.notificationSendFlowEntryRemoved(flowEntry.flowEntryId());
+ datagridService.notificationSendFlowEntryRemoved(copyFlowEntry.flowEntryId());
break;
case FE_USER_UNKNOWN:
assert(false);
break;
}
}
+ kryoFactory.deleteKryo(kryo);
}
/**
@@ -687,6 +695,11 @@
allValid = false;
break;
}
+ if (flowEntry.flowEntrySwitchState() !=
+ FlowEntrySwitchState.FE_SWITCH_UPDATED) {
+ allValid = false;
+ break;
+ }
}
if (! allValid)
continue;