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

[Ben Pfaff]

On Mon, Dec 05, 2016 at 05:45:15PM -0800, Mickey Spiegel wrote:
> 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.

You bring up reasonable questions.

I need to review the latest version of the "datapaths of interest"
patch, because I'm not very familiar with it anymore.  I recognize that
it probably conflicts with this series.

> > @@ -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.

This was a mistake.  I've now moved this from patch 9 into patch 6.

> 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.

Thanks, I've now fixed this too.