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
