[ovs-dev] [PATCH monitor_cond V6 11/11] RFC OVN: Quick implementation of conditional monitoring

[Ryan Moats]

Liran Schour/Haifa/IBM wrote on 05/24/2016 03:34:36 AM:

> From: Liran Schour/Haifa/IBM
> To: Ryan Moats/Omaha/IBM at IBMUS
> Cc: "Ben Pfaff" <blp at ovn.org>, ovs-dev <dev at openvswitch.org>
> Date: 05/24/2016 03:34 AM
> Subject: Re: [ovs-dev] [PATCH monitor_cond V6 11/11] RFC OVN: Quick
> implementation of conditional monitoring
>
> Ryan Moats/Omaha/IBM wrote on 23/05/2016 04:53:00 PM:
>
> > "dev" <dev-bounces at openvswitch.org> wrote on 05/23/2016 03:48:39 AM:
> >
> > > From: "Liran Schour" <LIRANS at il.ibm.com>
> > > To: "Ben Pfaff" <blp at ovn.org>
> > > Cc: ovs-dev <dev at openvswitch.org>
> > > Date: 05/23/2016 03:49 AM
> > > Subject: Re: [ovs-dev] [PATCH monitor_cond V6 11/11] RFC OVN: Quick
> > > implementation of conditional monitoring
> > > Sent by: "dev" <dev-bounces at openvswitch.org>
> > >
> > > Liran Schour/Haifa/IBM at IBMIL wrote on 17/05/2016 05:22:33 PM:
> > >
> > > > Conditional monitor of: Port_Binding, Logical_Flow, Multicast_Group
> > > > MAC_Binding tables. As a result ovn-controller will be notified
only
> > > about
> > > > records belongs to a datapath that is being served by this
hypervisor.
> > > >
> > > > Hack: Ideally, logical datapath ID should be retrieved from
> Port_Binding
> > > table
> > > > and by that conditions should be composed only from logical
datapath
> > > IDs.
> > > > In the meantime we first add the logical port to the conditions and

> > > > on retrieval
> > > > of port binding record, we add the logical datapath to the
conditions.
> > > >
> > > > ovn-bridge-mappings unit test fails due to the fact that patch
ports
> > > will not
> > > > be created on local bridge if there is no port binded to the
logical
> > > > datapath on
> > > > the local host.
> > > >
> > > > Signed-off-by: Liran Schour <lirans at il.ibm.com>
> > >
> > > Pushed a tiny fix to this patch to pass ovn-bridge-mappings unit
test.
> > > Updated code is pushed to:
https://github.com/liranschour/ovs.gitbranch:
> > > monitor_cond_ovn.
> >
> > That patch *almost* gets us the rest of the way there - that test case
> > switches between the current database and an empty database in lines
134-136
> > as a test of the database switching code - unfortunately, it looks like
> > the patch ports aren't being recreated.  That has the potential to
cause
> > headaches later on...
>
> At my environment it passes this unit test.
> Look at the following lines at ovn-controller.c:
>
> @@ -337,6 +354,12 @@ main(int argc, char *argv[])
>              free(ovnsb_remote);
>              ovnsb_remote = new_ovnsb_remote;
>              ovsdb_idl_set_remote(ovnsb_idl_loop.idl, ovnsb_remote,
true);
> +
> +            /* Update existing conditions */
> +            ovsdb_idl_cond_update(ovnsb_idl_loop.idl, &binding_cond);
> +            ovsdb_idl_cond_update(ovnsb_idl_loop.idl, &lflow_cond);
> +            ovsdb_idl_cond_update(ovnsb_idl_loop.idl, &mgroup_cond);
> +            ovsdb_idl_cond_update(ovnsb_idl_loop.idl,
&mac_binding_cond);
>          } else {
>              free(new_ovnsb_remote);
>          }

Ok, now I see - somehow, the version of code I have is missing those four
lines.
Once I put these in then the test case passes (and I no longer feel like
I'm
going crazy... or at least concerning this)

>
> These lines re-update the existing conditions on the new monitor
> session with the new database and ensure that we will get the right
updates.
>
> However, on a second look it seems that it is a bug in ovsdb-idl.c
> in ovsdb_idl_cond_update() and the above lines are just a work a round.
>
> I pushed the proper change to the next patch series. It is available
here:
> https://github.com/liranschour/ovs.git Branch: monitor_cond_v7_ovn

I'll let Ben weigh in on this, but I think I'd prefer to get the current
patch set applied and address this as a followon optimization.

Ryan