Thanks Josh, Both the patches are now merged. On Sun, Jul 10, 2016 at 1:42 AM, Josh Hershberg <[email protected]> wrote:
> The cherry is picked. > > On Fri, Jul 8, 2016 at 10:49 PM, Anil Vishnoi <[email protected]> > wrote: > >> Josh, can you cherry-pick it to master as well. Once you cherry-pick i >> will merge the master patch as jozef did +1 on that. >> >> On Fri, Jul 8, 2016 at 12:48 AM, Josh Hershberg <[email protected]> >> wrote: >> >>> done >>> >>> On Fri, Jul 8, 2016 at 9:31 AM, Anil Vishnoi <[email protected]> >>> wrote: >>> >>>> Hi Josh, >>>> >>>> Can you please address comment from Jozef on this patch, so we can >>>> merge it asap. >>>> >>>> On Wed, Jul 6, 2016 at 1:07 AM, Anil Vishnoi <[email protected]> >>>> wrote: >>>> >>>>> I will have a look at it sometime tomorrow. >>>>> >>>>> On Sun, Jul 3, 2016 at 8:53 AM, Josh Hershberg <[email protected]> >>>>> wrote: >>>>> >>>>>> 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] >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>>> >>>>> -- >>>>> Thanks >>>>> Anil >>>>> >>>> >>>> >>>> >>>> -- >>>> Thanks >>>> Anil >>>> >>> >>> >> >> >> -- >> Thanks >> Anil >> > > -- Thanks Anil
_______________________________________________ openflowplugin-dev mailing list [email protected] https://lists.opendaylight.org/mailman/listinfo/openflowplugin-dev
