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

[Ben Pfaff]

On Mon, Dec 05, 2016 at 12:58:24PM -0800, Mickey Spiegel wrote:
> On Sun, Dec 4, 2016 at 11:17 PM, 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.
> >
> 
> I like it. It took me a bit to change my mindset from most physical.c flows
> based on local ports with ofport values, splitting into some local ports
> with ofport values and other local ports accessing the same refactored
> code directly from a walk of sbrec_port_binding, based on port type.
> 
> This will allow me to cut a bunch of code from the first chassisredirect
> patch in my distributed NAT patch set, similar to the way you cut
> add_logical_path_ports in patch.c. More importantly for the distributed
> NAT patch set, this solves most of my egress loopback open issue.
> The local_datapath changes earlier in the patch set probably solve
> my localnet issue. I need to look at that a little more carefully.
> 
> A few comments below, mostly about code organization. One real
> technical issue.
> 
> After this, I will go back to the first patch and work my way through
> patch by patch.

Thanks for the comments, and sorry it's taken me a while to get back to
them.

> > +    /* 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) {
> > +        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->name)
> > +                    && binding->chassis != chassis) {
> > +                    sbrec_port_binding_set_chassis(binding, chassis);
> >
> 
> I prefer that sbrec_port_binding_set_chassis be handled in binding.c
> rather than patch.c. For l2gateway, even though patch port creation
> is handled in patch.c, sbrec_port_binding_set_chassis is handled in
> binding.c. There is already l3gateway code in binding.c to trigger
> add_local_datapath. This code can go in right alongside that.

You're right that this code doesn't fit in here very well.  I kept it
here only because it was previously handled in patch.c, but that's not a
good justification.

I'll move it.

> > +        struct zone_ids zone_ids = { .ct = 0 };
> > +        if (!strcmp(binding->type, "l3gateway")) {
> > +            char *dnat = alloc_nat_zone_key(&binding->
> > datapath->header_.uuid,
> > +                                            "dnat");
> > +            zone_ids.dnat = simap_get(ct_zones, dnat);
> > +            free(dnat);
> > +
> > +            char *snat = alloc_nat_zone_key(&binding->
> > datapath->header_.uuid,
> > +                                            "snat");
> > +            zone_ids.snat = simap_get(ct_zones, snat);
> > +            free(snat);
> > +        }
> >
> 
> For distributed NAT, most of the optimization above compared to
> get_zone_ids will go away. I will need dnat and snat zones for
> distributed routers as well as gateway routers.

...

> When I implement egress loopback for distributed NAT, while I will
> only need egress loopback for patch ports, I should probably code
> it up generically for all port types. In order to do that, I will need to
> access the code starting here, down to ofpact_finish_CLONE, from
> another place. I can take care of refactoring this chunk of code in
> that patch if you do not do it here. The difference will be that you
> are calling load_logical_ingress_metadata based on peer,
> whereas for egress loopback this will be based on binding itself.
> Alternatively, I could create my own copy just to optimize since I
> do not need to reset metadata (datapath) or any of the three
> zones.

I think that it's best to make these changes at the time they're
needed.

> 
> > +        size_t clone_ofs = ofpacts_p->size;
> > +        struct ofpact_clone *clone = ofpact_put_CLONE(ofpacts_p);
> > +        put_load(0, MFF_LOG_DNAT_ZONE, 0, 32, ofpacts_p);
> > +        put_load(0, MFF_LOG_SNAT_ZONE, 0, 32, ofpacts_p);
> > +        put_load(0, MFF_LOG_CT_ZONE, 0, 32, ofpacts_p);
> > +        load_logical_ingress_metadata(peer, &zone_ids, ofpacts_p);
> >
> 
> Don't you need to recompute the zone_ids based on the peer rather
> than based on binding?
> The peer is in a different datapath.

Thanks.  I think that the following incremental fixes that:

diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index 678b62d..f33750e 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -321,12 +321,12 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve,
 
         struct zone_ids zone_ids = { .ct = 0 };
         if (!strcmp(binding->type, "l3gateway")) {
-            char *dnat = alloc_nat_zone_key(&binding->datapath->header_.uuid,
+            char *dnat = alloc_nat_zone_key(&peer->datapath->header_.uuid,
                                             "dnat");
             zone_ids.dnat = simap_get(ct_zones, dnat);
             free(dnat);
 
-            char *snat = alloc_nat_zone_key(&binding->datapath->header_.uuid,
+            char *snat = alloc_nat_zone_key(&peer->datapath->header_.uuid,
                                             "snat");
             zone_ids.snat = simap_get(ct_zones, snat);
             free(snat);