[ovs-dev] [PATCH] ovn-controller: Reset flow processing after (re)connection to switch

[Numan Siddique]

On Wed, Aug 10, 2016 at 6:07 AM, Numan Siddique <nusiddiq at redhat.com> wrote:

> On Aug 9, 2016 8:28 PM, "Ryan Moats" <rmoats at us.ibm.com> wrote:
> >
> > Numan Siddique <nusiddiq at redhat.com> wrote on 08/09/2016 09:39:21 AM:
> >
> > > From: Numan Siddique <nusiddiq at redhat.com>
> > > To: Ryan Moats/Omaha/IBM at IBMUS
> > > Cc: ovs dev <dev at openvswitch.org>
> > > Date: 08/09/2016 09:39 AM
> > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: Reset flow processing
> > > after (re)connection to switch
> >
> > >
> > > On Tue, Aug 9, 2016 at 7:15 PM, Ryan Moats <rmoats at us.ibm.com> wrote:
> > > "dev" <dev-bounces at openvswitch.org> wrote on 08/09/2016 07:19:27 AM:
> > >
> > > > From: Numan Siddique <nusiddiq at redhat.com>
> > > > To: ovs dev <dev at openvswitch.org>
> > > > Date: 08/09/2016 07:19 AM
> > > > Subject: [ovs-dev] [PATCH] ovn-controller: Reset flow processing
> > > > after (re)connection to switch
> > > > Sent by: "dev" <dev-bounces at openvswitch.org>
> > > >
> > > > When ovn-controller reconnects to the ovs-vswitchd, it deletes all
> the
> > > > OF flows in the switch. It doesn't install the flows again, leaving
> > > > the datapath broken unless ovn-controller is restarted or ovn-northd
> > > > updates the SB DB.
> > > >
> > > > The reason for this is
> > > >   - lflow_reset_processing() is not called after the reconnection
> > > >   - the hmap "installed_flows" is not cleared, because of which
> > > >     ofctrl_put skips adding the flows to the switch.
> > > >
> > > > This patch fixes the issue and also adds a test case to test
> > > > this scenario.
> > > >
> > > > Signed-off-by: Numan Siddique <nusiddiq at redhat.com>
> > > > ---
> > >
> > > I'm going to pick a nit on this one - is the behavior you are aiming
> > > for delete and re-add or just recalculate and leave alone?
> >
> > >
> > > ​In my testing I am seeing that all the OF flows are getting de​
> > > leted when I restart ovs-vswitchd.
> > > I am testing with the latest master of OVS. I am able to see this on
> > > 2 different machines and also in sandbox.
> > >
> > > I thought that ovn-controller is deleting the flows in the switch
> > > when it restarts (https://github.com/openvswitch/ovs/blob/master/
> > > ovn/controller/ofctrl.c#L355)
> > >
> > > Now I tested again and before restarting ovs-vswitchd, I killed ovn-
> > > controller. Looks like ovs-vswitchd is clearing the old flows when
> > > it restarts. I am not sure if this is the intended behavior. Looks
> > > like it is. Please correct me if I am wrong here.
> > >
> > > ​You can run below commands to reproduce the issue in sandbox​
> > >
> > > -----------------------------
> > >  $make sandbox SANDBOXFLAGS="--ovn"
> > >  $ovn/env1/setup.sh
> > >  $ovs-ofctl dump-flows br-int
> > >  $ovs-appctl -t ovn-controller exit
> > >  $ovs-appctl -t ovs-vswitchd exit
> > > ​ $ovs-vswitchd --detach --no-chdir --pidfile -vconsole:off --log-
> > > file --enable-dummy=override -vvconn -vnetdev_dummy
> > >  $ ovs-ofctl dump-flows br-int
> > > NXST_FLOW reply (xid=0x4):
> > > ------------------------------------
> > >
> > > You will see that the flows are deleted even if you don't run -
> > > "ovs-appctl -t ovn-controller exit".
> > >
> > > I ask because if it is "delete and re-add" aren't you still disrupting
> > > the datapath even if only momentarily?
> > >
> >
> > Ok, so we'll assume that your code is valid for when ovswitchd purges
> > the old flows and that's good.
> >
> > IIRC there is a way to restart ovswitchd via ovs_ctl so that it doesn't
> > purge the old flows.  Since I'll argue (as an operator) that is the more
> > important case, can you add a unit test for this and verify that your
> > patch doesn't leave that path broken?
> >
> >
> Sure. I will do that.


Ryan - ​I tested ​

​using ovs-ctl and I could see that ovn-controller is indeed deleting
all the flows when ever it reconnects to the vswitchd even when flows are
restored by
​ovs-ctl ​(
https://github.com/openvswitch/ovs/blob/master/ovn/controller/ofctrl.c#L350
​)
​
I also enhanced the test case as you suggested.​ The test case will fail
when run without the fix in this patch.
I will send out the v2 soon.


Thanks
> Numan
>