On Sun, Dec 18, 2016 at 10:59:18PM -0800, Mickey Spiegel wrote: > On Sun, Dec 18, 2016 at 12:18 AM, Ben Pfaff <b...@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 <b...@ovn.org> > > > > Acked-by: Mickey Spiegel <mickeys....@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. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev