On Sat, Jul 30, 2016 at 10:58 AM, Liran Schour <lir...@il.ibm.com> wrote:
> "dev" <dev-boun...@openvswitch.org> wrote on 29/07/2016 11:46:09 AM: > > > From: Darrell Ball <dlu...@gmail.com> > > To: dlu...@gmail.com, d...@openvswitch.com, b...@ovn.org > > Date: 29/07/2016 11:46 AM > > Subject: [ovs-dev] [patch_v1] ovn: Add datapaths of interest filtering > (RFC). > > 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. This > > allows monitoring to adapt quickly with a single new monitor setting for > > all datapaths of interest locally. > > > > Presently, the code does not cleanup logical port monitor contexts > > as some refactoring may need to be done given several recent > > changes in this area. > > > > Signed-off-by: Darrell Ball <dlu...@gmail.com> > > First I would mention that this code is based on the patch: "ovn: > implementation of conditional monitor usage" > http://openvswitch.org/pipermail/dev/2016-July/076532.html I commented on your design earlier and voiced my concerns with the approach. See your previous e-mails. I told you I thought your design had too long a latency to be practically useful. Thats why I have had a approach. 1) My change supports flood fill calculation in ovn-controller that was based on a variation of https://patchwork.ozlabs.org/patch/646962/ which is an approach that I have had in one form or another for some time. You patch is not doing a flood fill in ovn-controller and this is the core algorithm I am using. 2) My patch introduces a SB database datapath_binding table change to support flood fill in "1" above. So northd is doing some of the work here to make flood fill in "1" easier. This was suggested by Ben. This is a very different approach than your implementation where northd has no role at all. 3) My code calls existing conditional monitoring APIs which are an enhancement over previously existing conditional monitoring support in ovsdb client. I believe you provided an implementation that enhanced the existing conditional monitoring in ovsdb where more granularity was added for conditional monitoring. I called the provided ovsdb client monitoring APIs in my patch. Calling ovsdb client conditional monitoring APIs is one approach. However, I have also used flood fill to determine which openflow rules are generated locally in ovn-controller after allowing all flows to be downloaded. eg) https://patchwork.ozlabs.org/patch/646962/ > > This new patch introduce an optimization to the base patch by exposing a > new column in the datapath_binding table called related_datapaths. Again, my implementation is based on flood fill in ovn-controller ("1" above) and uses northd to provide a compressed dataset to facilitate the flood fill in ovn-controller ("2" above). Your patch does not do either. Both patches call ovsdb client conditional monitoring APIs. > > I think that we should agree on applying the basic conditional monitor > usage to OVN first before we start to add any optimization to it. Then if we will see that we need to cut the latency of datapaths and ports > in the ovn-controller, we can easily add this functionality to the code. Since conditional monitoring in ovsdb has existed for some time and was recently enhanced to add more granularity to the monitoring APIs, I think it is safe to assume that ovsdb conditional monitoring is an accepted approach. Also, many other databases use conditional monitoring in one form (and name) or another. The question is not whether to use conditional monitoring, but how to use it ...... Liran My patch introduces 1) A datapath_binding table change 2) The change supports flood fill in ovn-controller that was based on https://patchwork.ozlabs.org/patch/646962/ 3) Use conditional monitoring APIs, which was agreed Your patch does not use a flood fill and I commented on your patch earlier and received no response > > > BTW, in this patch I do not see any calls for add_clause_false so this > code will retrieve the whole SB DB in the first iteration. > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
