[ovs-dev] [patch_v1] ovn: Add datapaths of interest filtering (RFC).

[Darrell Ball]

On Sat, Jul 30, 2016 at 10:58 AM, Liran Schour <LIRANS at il.ibm.com> wrote:

> "dev" <dev-bounces at openvswitch.org> wrote on 29/07/2016 11:46:09 AM:
>
> > From: Darrell Ball <dlu998 at gmail.com>
> > To: dlu998 at gmail.com, dev at openvswitch.com, blp at 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-bounces at 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 <dlu998 at 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.
>