[ovs-dev] [PATCH 6/9] ovn-controller: Handle only relevant ports and flows.

[Mickey Spiegel]

On Sun, Dec 4, 2016 at 11:17 PM, Ben Pfaff <blp at ovn.org> wrote:

> On a particular hypervisor, ovn-controller only needs to handle ports
> and datapaths that have some relationship with it, that is, the
> ports that actually reside on the hypervisor, plus all the other ports on
> those ports' datapaths, plus all of the ports and datapaths that are
> reachable from those via logical patch ports.  Until now, ovn-controller
> has done a poor job of limiting what it deals with to this set.  This
> commit improves the situation.
>
> This commit gets rid of the concept of a "patched_datapath" which until now
> was used to represent any datapath that contained a logical patch port.
> Previously, the concept of a "local_datapath" meant a datapath with a VIF
> that resides on the local hypervisor.  This commit extends that concept to
> include any other datapath that can be reached from a VIF on the local
> hypervisor, which is a simplification that makes the code easier to
> understand in a few places.
>

I like the change to remove "patched_datapath" and consolidate into
the concept of "local_datapath". Among other issues, it solves my
localnet problem for distributed NAT.

The big question is the relationship with datapaths of interest. Both
this and datapaths of interest trigger a computation from
add_local_datapath, to find other datapaths that can be reached on
the local hypervisor. At a high level, the difference is that this
computation is based on patch ports from a datapath, while the
datapaths of interest computation is based on a list of neighbor
datapaths that is populated by northd into each datapath_binding.
Note that northd populates related_datapaths based on patch
ports, so in the end it is the same source of information, but
different parts of the computation are done in different places at
different times.

If it were only desired to do conditional monitoring of logical_flow,
then related_datapaths would not be necessary at all.
Local_datapath, as computed in this patch, could be used to
trigger the necessary logical flow clauses.

With the desire to do conditional monitoring of port_binding,
things get more complicated. The computation in this patch
depends on having the port_bindings for other datapaths,
leading to a chicken and egg problem. It seems like northd
population of related_datapaths is necessary for conditional
monitoring of port_bindings. This leads to the question
whether local_datapaths should be computed as in this
patch? or whether there should be a single computation
over related_datapaths to determine local_datapaths, which
would then be used to trigger conditional monitoring clauses
for datapaths? Is it safe for this computation to depend on
partial computation from northd? Or are there any timing
windows that might lead to corner cases?

Depending on the answer to this question, patches 2 and 5 in
this patch set may or may not be necessary.

One question and one comment inline.

>
> CC: Gurucharan Shetty <guru at ovn.org>
> Signed-off-by: Ben Pfaff <blp at ovn.org>
> ---
>  ovn/controller/binding.c        | 75 ++++++++++++++++++++++++++-----
>  ovn/controller/binding.h        |  7 ++-
>  ovn/controller/lflow.c          | 46 ++-----------------
>  ovn/controller/lflow.h          |  1 -
>  ovn/controller/ovn-controller.c | 41 ++++++-----------
>  ovn/controller/ovn-controller.h | 33 +++++++-------
>  ovn/controller/patch.c          | 98 ++++++++++++++++--------------
> -----------
>  ovn/controller/patch.h          |  5 +--
>  ovn/controller/physical.c       | 30 ++-----------
>  ovn/controller/physical.h       |  4 +-
>  tests/ovn-controller.at         | 19 +++++++-
>  11 files changed, 165 insertions(+), 194 deletions(-)
>
> diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> index fb76032..b53af98 100644
> --- a/ovn/controller/binding.c
> +++ b/ovn/controller/binding.c
>

<snip>


> @@ -342,7 +390,8 @@ consider_local_datapath(struct controller_ctx *ctx,
>          const char *chassis = smap_get(&binding_rec->options,
>                                         "l3gateway-chassis");
>          if (!strcmp(chassis, chassis_rec->name) && ctx->ovnsb_idl_txn) {
> -            add_local_datapath(local_datapaths, binding_rec);
> +            add_local_datapath(ldatapaths, lports, binding_rec->datapath,
> +                               false, local_datapaths);
>

Why did you set has_local_l3gateway to false here, then change
it to true in patch 9?
I don't see the harm in setting it to true here.


>          }
>      } else if (chassis_rec && binding_rec->chassis == chassis_rec) {
>          if (ctx->ovnsb_idl_txn) {
>

<snip>


> --- a/ovn/controller/ovn-controller.c
> +++ b/ovn/controller/ovn-controller.c
>

There is a fragment of code to destroy patched_datapaths that
was deleted at the end of patch 5. It should not have been deleted
in patch 5, but it should be deleted here.

Mickey