Guys, I pushed a patch for this.
https://git.opendaylight.org/gerrit/#/c/41247/ -J On Thu, Jun 30, 2016 at 1:41 PM, Josh Hershberg <[email protected]> wrote: > All, this is a separate email discussion about the problem sam raised > regarding the flow ids after the li plugin switch. There's some code > analysis below as to what is causing the issue. > > On Thu, Jun 30, 2016 at 1:29 PM, Josh Hershberg <[email protected]> > wrote: > >> Woops was rushing there so I was not being careful. Below, inline, >> corrections: >> >> On Thu, Jun 30, 2016 at 1:20 PM, Sam Hague <[email protected]> wrote: >> >>> Good stuff, Josh. Comments inline. >>> On Jun 30, 2016 7:11 AM, "Josh Hershberg" <[email protected]> wrote: >>> > >>> > Guys, >>> > >>> > I've found the following bug in openflowplugin that surfaces because >>> netvirt seems to be writing some flows multiple times. The exact same flows >>> multiple times. The following code is from SalFlowServiceImpl.updateFlow. >>> I'll annotate it to explain the problem: >>> > >>> > //STEP 1: HERE THE FLOW IS MARKED FOR DELETION BUT NOT ACTUALLY >>> DELETED. THE MARK IS BASED ON THE FLOW HASH >>> Why is it marked for deletion or the hash different? From netvirt side >>> the flow is the exact same. >>> >> Yeah. The hash is the same because all the fields are the same. So it >> gets marked for deletion and then re-added and then it gets swept up. >> >> Re: the netvirt flow: Well, it seems the update is in fact triggered by >> the southbound. Here's a stack trace I stuck in there [1] (no real >> exception, just to see the triggering event) >> >> >>> > >>> > deviceFlowRegistry.markToBeremoved(flowRegistryKey); >>> > >>> > if (itemLifecycleListener != null) { >>> > final FlowDescriptor flowDescriptor = >>> deviceContext.getDeviceFlowRegistry().retrieveIdForFlow(flowRegistryKey); >>> > if (flowDescriptor != null) { >>> > KeyedInstanceIdentifier<Flow, FlowKey> flowPath = >>> createFlowPath(flowDescriptor, >>> > >>> deviceContext.getDeviceInfo().getNodeInstanceIdentifier()); >>> > itemLifecycleListener.onRemoved(flowPath); >>> > } >>> > } >>> > //if provided, store flow id to flow registry >>> > if (flowRef != null) { >>> > final FlowId flowId = flowRef.getValue().firstKeyOf(Flow.class, >>> FlowKey.class).getId(); >>> > final FlowDescriptor flowDescriptor = >>> FlowDescriptorFactory.create(updated.getTableId(), flowId); >>> > >>> > STEP 2: HERE THE FLOW IS RE-ADDED... >>> > >>> > >>> > deviceFlowRegistry.store(updatedflowRegistryKey, flowDescriptor); >>> > >>> > STEP 3: DeviceFlowRegistryImpl.removeMarked is called resulting in the >>> removing of a flow that should not be removed because it was in fact >>> re-added right afterwards. The flow does get updated via an OFPST_FLOW but >>> by then it is missing the name which is what we use in IT tests to validate >>> that the flows get created. >>> Same question, I don't see why the flow is viewed as new. And it was >>> never deleted from config. >>> >> Right. But we validate that it's in the operational as well. The way it >> works is that when you put it in config it stores the flow in the >> DeviceFlowRegistry, including the name. Then when it gets the OFPST_FLOW it >> get's the name from the Registry and stores that in the operational as >> well. Is that what you were asking? >> >> >>> Or is it because the two writes to config happen before the first flow >>> is pushed? >>> > >>> > I can think of a few ways to solve this but I wanted to give you, >>> Anil, a chance to voice an opinion. My gut instinct it to take the safest >>> solution which is to add a method to DeviceFlowRegistry that directly >>> removes (not marks) the flow before rewriting it...or better yet, just >>> overwrite it. >>> >>> Overwrite seems accurate and matches what netvirt expects, with caveat >>> that overwrite is a noop. >>> >> Agreed. >> >> >>> > >>> > I gotta' drop for the weekend, will be back Sunday AM but checking >>> emails in the interim. >>> > >>> > -Josh >>> > >>> >> [1] >> 5321 2016-06-30 13:45:07,742 | WARN | entLoopGroup-9-1 | >> DeviceFlowRegistryImpl | 290 - >> org.opendaylight.openflowplugin.impl - 0.3.0.SNAPSHOT | JOSH DROP FILTER 3 >> REMOVED >> 5322 java.lang.Exception >> 5323 at >> org.opendaylight.openflowplugin.impl.registry.flow.DeviceFlowRegistryImpl.markToBeremoved(DeviceFlowRegistryImpl.java:170)[290:org.opendaylight.openflowplugin.impl:0.3.0.SNAPSHOT >> ] >> 5324 at >> org.opendaylight.openflowplugin.impl.services.SalFlowServiceImpl$3.onSuccess(SalFlowServiceImpl.java:200)[290:org.opendaylight.openflowplugin.impl:0.3.0.SNAPSHOT] >> 5325 at >> org.opendaylight.openflowplugin.impl.services.SalFlowServiceImpl$3.onSuccess(SalFlowServiceImpl.java:192)[290:org.opendaylight.openflowplugin.impl:0.3.0.SNAPSHOT] >> 5326 at >> com.google.common.util.concurrent.Futures$6.run(Futures.java:1319)[66:com.google.guava:18.0.0] >> 5327 at >> com.google.common.util.concurrent.MoreExecutors$DirectExecutor.execute(MoreExecutors.java:457)[66:com.google.guava:18.0.0] >> 5328 at >> com.google.common.util.concurrent.ExecutionList.executeListener(ExecutionList.java:156)[66:com.google.guava:18.0.0] >> 5329 at >> com.google.common.util.concurrent.ExecutionList.execute(ExecutionList.java:145)[66:com.google.guava:18.0.0] >> 5330 at >> com.google.common.util.concurrent.AbstractFuture.set(AbstractFuture.java:185)[66:com.google.guava:18.0.0] >> 5331 at >> com.google.common.util.concurrent.SettableFuture.set(SettableFuture.java:53)[66:com.google.guava:18.0.0] >> 5332 at >> org.opendaylight.openflowplugin.impl.services.FlowService$1.onSuccess(FlowService.java:75)[290:org.opendaylight.openflowplugin.impl:0.3.0.SNAPSHOT] >> 5333 at >> org.opendaylight.openflowplugin.impl.services.FlowService$1.onSuccess(FlowService.java:54)[290:org.opendaylight.openflowplugin.impl:0.3.0.SNAPSHOT] >> 5334 at >> com.google.common.util.concurrent.Futures$6.run(Futures.java:1319)[66:com.google.guava:18.0.0] >> 5335 at >> com.google.common.util.concurrent.MoreExecutors$DirectExecutor.execute(MoreExecutors.java:457)[66:com.google.guava:18.0.0] >> 5336 at >> com.google.common.util.concurrent.ExecutionList.executeListener(ExecutionList.java:156)[66:com.google.guava:18.0.0] >> 5337 at >> com.google.common.util.concurrent.ExecutionList.execute(ExecutionList.java:145)[66:com.google.guava:18.0.0] >> 5338 at >> com.google.common.util.concurrent.AbstractFuture.set(AbstractFuture.java:185)[66:com.google.guava:18.0.0] >> 5339 at >> com.google.common.util.concurrent.Futures$CombinedFuture.setOneValue(Futures.java:1764)[66:com.google.guava:18.0.0] >> 5340 at >> com.google.common.util.concurrent.Futures$CombinedFuture.access$400(Futures.java:1608)[66:com.google.guava:18.0.0] >> 5341 at >> com.google.common.util.concurrent.Futures$CombinedFuture$2.run(Futures.java:1686)[66:com.google.guava:18.0.0] >> 5342 at >> com.google.common.util.concurrent.MoreExecutors$DirectExecutor.execute(MoreExecutors.java:457)[66:com.google.guava:18.0.0] >> 5343 at >> com.google.common.util.concurrent.ExecutionList.executeListener(ExecutionList.java:156)[66:com.google.guava:18.0.0] >> 5344 at >> com.google.common.util.concurrent.ExecutionList.execute(ExecutionList.java:145)[66:com.google.guava:18.0.0] >> 5345 at >> com.google.common.util.concurrent.AbstractFuture.set(AbstractFuture.java:185)[66:com.google.guava:18.0.0] >> 5346 at >> com.google.common.util.concurrent.SettableFuture.set(SettableFuture.java:53)[66:com.google.guava:18.0.0] >> 5347 at >> org.opendaylight.openflowplugin.impl.rpc.AbstractRequestContext.setResult(AbstractRequestContext.java:32)[290:org.opendaylight.openflowplugin.impl:0.3.0.SNAPSHOT] >> 5348 at >> org.opendaylight.openflowplugin.impl.services.AbstractRequestCallback.setResult(AbstractRequestCallback.java:50)[290:org.opendaylight.openflowplugin.impl:0.3.0.SNAPSHOT] >> 5349 at >> org.opendaylight.openflowplugin.impl.services.SimpleRequestCallback.onSuccess(SimpleRequestCallback.java:38)[290:org.opendaylight.openflowplugin.impl:0.3.0.SNAPSHOT] >> 5350 at >> org.opendaylight.openflowplugin.impl.services.SimpleRequestCallback.onSuccess(SimpleRequestCallback.java:20)[290:org.opendaylight.openflowplugin.impl:0.3.0.SNAPSHOT] >> 5351 at >> org.opendaylight.openflowjava.protocol.impl.core.connection.OutboundQueueEntry.complete(OutboundQueueEntry.java:103)[286:org.opendaylight.openflowjava.openflow-protocol-impl:0.8. >> 0.SNAPSHOT] >> 5352 at >> org.opendaylight.openflowjava.protocol.impl.core.connection.StackedSegment.completeRequests(StackedSegment.java:163)[286:org.opendaylight.openflowjava.openflow-protocol-impl:0.8. >> 0.SNAPSHOT] >> 5353 at >> org.opendaylight.openflowjava.protocol.impl.core.connection.StackedSegment.pairRequest(StackedSegment.java:147)[286:org.opendaylight.openflowjava.openflow-protocol-impl:0.8.0.SNA >> PSHOT] >> 5354 at >> org.opendaylight.openflowjava.protocol.impl.core.connection.AbstractStackedOutboundQueue.pairRequest(AbstractStackedOutboundQueue.java:191)[286:org.opendaylight.openflowjava.open >> flow-protocol-impl:0.8.0.SNAPSHOT] >> 5355 at >> org.opendaylight.openflowjava.protocol.impl.core.connection.AbstractOutboundQueueManager.onMessage(AbstractOutboundQueueManager.java:208)[286:org.opendaylight.openflowjava.openfl >> ow-protocol-impl:0.8.0.SNAPSHOT] >> 5356 at >> org.opendaylight.openflowjava.protocol.impl.core.connection.ConnectionAdapterImpl.consumeDeviceMessage(ConnectionAdapterImpl.java:136)[286:org.opendaylight.openflowjava.openflow- >> protocol-impl:0.8.0.SNAPSHOT] >> 5357 at >> org.opendaylight.openflowjava.protocol.impl.core.connection.AbstractConnectionAdapterStatistics.consume(AbstractConnectionAdapterStatistics.java:66)[286:org.opendaylight.openflow >> java.openflow-protocol-impl:0.8.0.SNAPSHOT] >> 5358 at >> org.opendaylight.openflowjava.protocol.impl.core.connection.ConnectionAdapterImpl.consume(ConnectionAdapterImpl.java:43)[286:org.opendaylight.openflowjava.openflow-protocol-impl: >> 0.8.0.SNAPSHOT] >> >> >> >
_______________________________________________ openflowplugin-dev mailing list [email protected] https://lists.opendaylight.org/mailman/listinfo/openflowplugin-dev
