On Wed, Aug 10, 2016 at 2:53 AM, Liran Schour <lir...@il.ibm.com> wrote:
> Darrell Ball <dlu...@gmail.com> wrote on 09/08/2016 08:02:06 PM: > > > On Thu, Aug 4, 2016 at 3:26 AM, Liran Schour <lir...@il.ibm.com> wrote: > > "dev" <dev-boun...@openvswitch.org> wrote on 03/08/2016 09:09:48 AM: > > > > > From: Darrell Ball <dlu...@gmail.com> > > > To: dlu...@gmail.com, d...@openvswitch.com, b...@ovn.org > > > Date: 03/08/2016 09:10 AM > > > Subject: [ovs-dev] [patch_v3] ovn: Add datapaths of interest filtering. > > > Sent by: "dev" <dev-boun...@openvswitch.org> > > > > > > This patch adds datapaths of interest support where only datapaths of > > > local interest are monitored by the ovn-controller ovsdb client. The > > > idea is to do a flood fill in ovn-controller of datapath associations > > > calculated by northd. A new column is added to the SB database > > > datapath_binding table - related_datapaths to facilitate this so all > > > datapaths associations are known quickly in ovn-controller. This > > > allows monitoring to adapt quickly with a single new monitor setting > > > for all datapaths of interest locally. > > > > > > Signed-off-by: Darrell Ball <dlu...@gmail.com> > > > --- > > > > > > > I still think this work is mainly based on top of the conditional > > monitor work. However it introduces a flood fill in the ovn- > > controller using the new added column, related_datapaths, in the > > datapath_bindning table which is important optimization to the origin > work. > > > > Since the origin patch has went through 12 iterations and it is much > > more stable and mature (http://openvswitch.org/pipermail/dev/2016- > > August/077008.html). > > I propose to combine the 2 patches. > > > > What do you think? > > > > I just got back from 5 days vacation yesterday. > > > > The patches are significantly different in design and implementation > aspects. > > They both call some conditional monitoring APIs. > > However, I would like to find a way to share in some way. > > > > OK, so I will propose a plan to integrate the 2 works. Both works have a > very similar usage of the conditional monitoring API, conditioning the same > tables in the SB DB on the same places in the code (in most cases). Both your patch and my patch do something to trigger adding datapath (local and/or patched) monitoring during: add_local_datapath() add_patched_datapath() which is governed by the existing code and the only place to do this. That something is however very different. I am triggering a flood fill which monitors multiple related datapaths at once. Both your patch and my patch do something to trigger removing datapath (local and/or patched) monitoring during. remove_local_datapath() add_logical_patch_ports_postprocess() where patched datapaths are removed during patched datapaths auditing near the call to hmap_remove(patched_datapaths, &pd_cur_node->hmap_node); I wrote add_logical_patch_ports_postprocess() to fix an unrelated bug. In general, the placement of add and remove datapath monitoring is governed by the existing code doing add and remove of datapath contexts in ovn-controller. This is no flexibility here. For logical port associated VIFs monitoring addition, we both add monitoring in get_local_iface_ids. This is where the existing code adds context for local VIFs, so its the only place to add logical port associated VIFs monitoring, Both patches add logical patch port monitoring in add_logical_patch_ports() where patch ports are added. Again this is the only place to do this, governed by the existing code. The approach to idempotency is different (I think the ovsdb client API should enforce this) and we handle patch port pairs differently. For logical port associated VIFs monitoring removal and patch port monitoring removal, I use the existing logical port datastructures deletion triggers in get_local_iface_ids() and patch_run(). I wrote a function to help with this. You are doing an audit of a separate conditional monitoring logical port datastructure in the ovn-controller main loop. In general, for logical port monitoring you use a separate logical port monitoring datastructure, whereas my general approach is to leverage the existing logical port datastructures. > Since my patch has gone through 12 iterations and has been tested already, > I propose to use it as the implementation of the conditional monitoring > usage and use it's exposed API filter_port() and filter_datapath() later. I am not sure what the previous testing was or how rigorous it was, but in general the design should be more important as we are not rushing anything in here. > > On top of it we can integrate your flood fill work with the changes in > files: ovn-sb.ovsschema, ovn-sb.xml, ovn-norhtd.c, and code in binding.c > and patch.c to call the above API to filter the related datapaths. Since the main flood fill code I added is in ovn-controller.c, that will not work very well. In general, the 2 patches are very different in design and implementation, except for 6 locations where some kind of triggering happens from the same existing function, which is governed by the existing code design and the only place to add those triggers. > > > What do you think? _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
