[ovs-dev] [PATCH v3 13/16] ovn-controller: Drop most uses of OVS patch ports.

[Ben Pfaff]

On Sun, Dec 18, 2016 at 10:59:18PM -0800, Mickey Spiegel wrote:
> On Sun, Dec 18, 2016 at 12:18 AM, Ben Pfaff <blp at ovn.org> wrote:
> 
> > Until now, ovn-controller has implemented OVN logical patch ports and
> > l3gateway ports in terms of OVS patch ports.  It is a hassle to create and
> > destroy ports, and it is also wasteful compared to what the patch ports
> > actually buy us: the ability to "save and restore" a packet around a
> > recursive trip through the flow table.  The "clone" action can do that too,
> > without the need to create a port.  This commit takes advantage of the
> > clone action for that purpose, getting rid of most of the patch ports
> > previously created by ovn-controller.
> >
> > Signed-off-by: Ben Pfaff <blp at ovn.org>
> >
> 
> Acked-by: Mickey Spiegel <mickeys.dev at gmail.com>
> 
> ---
> >  ovn/controller/binding.c        | 19 ++++++++
> >  ovn/controller/ovn-controller.c |  8 ++--
> >  ovn/controller/patch.c          | 88 +++++-------------------------
> > -------
> >  ovn/controller/patch.h          |  3 +-
> >  ovn/controller/physical.c       | 96 ++++++++++++++++++++++++++++++
> > -----------
> >  ovn/controller/physical.h       |  7 +--
> >  ovn/controller/pinctrl.c        | 36 ++++++++++------
> >  ovn/controller/pinctrl.h        |  4 +-
> >  tests/ovn-controller.at         | 57 ++----------------------
> >  9 files changed, 138 insertions(+), 180 deletions(-)
> >
> > diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> > index 4ad01b0..5f71a48 100644
> > --- a/ovn/controller/binding.c
> > +++ b/ovn/controller/binding.c
> > @@ -456,6 +456,25 @@ binding_run(struct controller_ctx *ctx, const struct
> > ovsrec_bridge *br_int,
> >      shash_destroy(&lport_to_iface);
> >      sset_destroy(&egress_ifaces);
> >      hmap_destroy(&qos_map);
> > +
> > +    /* Bind l3gateway ports to chassis.
> > +     *
> > +     * (It would make sense for ovn-northd to do this, but if it is ever
> > +     * modified to do so, ovn-controller still needs to do the binding as
> > long
> > +     * as OVN supports upgrades against older ovn-northd.) */
> > +    if (ctx->ovnsb_idl_txn && chassis_rec) {
> > +        const struct sbrec_port_binding *binding;
> > +        SBREC_PORT_BINDING_FOR_EACH (binding, ctx->ovnsb_idl) {
> > +            if (!strcmp(binding->type, "l3gateway")) {
> > +                const char *l3gw_chassis = smap_get(&binding->options,
> > +                                                    "l3gateway-chassis");
> > +                if (!strcmp(l3gw_chassis, chassis_rec->name)
> > +                    && binding->chassis != chassis_rec) {
> > +                    sbrec_port_binding_set_chassis(binding, chassis_rec);
> > +                }
> > +            }
> > +        }
> > +    }
> >  }
> >
> 
> For all other port types, the calls to sbrec_port_binding_set_chassis
> are done in consider_local_datapath. Those calls also consider cases
> where this chassis needs to release a port because the port_binding
> chassis has changed, and they post VLOG_INFOs. For example if
> someone by mistake changed the nb Logical_Router chassis option
> to a chassis name that does not match any chassis_rec, then this
> code would leave the port_binding chassis set to the old value.
> 
> IMO it would be nice if l3gateway took the same approach as
> l2gateway, by covering all the sbrec_port_binding_set_chassis
> logic around line 392 or so.

Thank you for noticing this inconsistency.  Somehow, I had not noticed
before.

This was a great opportunity to consolidate code.  I've now done so, and
it's both shorter and (I think) clearer now.

> @@ -577,7 +630,8 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve,
> >              put_resubmit(OFTABLE_CHECK_LOOPBACK, remote_ofpacts_p);
> >          } else if (simap_contains(&localvif_to_ofport,
> >                             (port->parent_port && *port->parent_port)
> > -                           ? port->parent_port : port->logical_port)) {
> > +                           ? port->parent_port : port->logical_port)
> > +                   || !strcmp(port->type, "l3gateway")) {
> >
> 
> I was a little hasty in proposing the fix. This should limit the use of
> "l3gateway" ports to this chassis:
> s/!strcmp(port->type, "l3gateway")/
> (!strcmp(port->type, "l3gateway") && port->chassis == this_chassis)
> 
> This would also require the addition of "const struct sbrec_chassis
> *this_chassis"
> to consider_mc_group.

You're right.  I made this change.

> If you take my suggestion for patch 10 to limit consider_mc_group to
> local_datapaths, then there would be some question whether the chassis
> check is redundant.

I think that it's still needed.