On Mon, May 1, 2017 at 5:12 PM, Ben Pfaff <b...@ovn.org> wrote:
> On Mon, May 01, 2017 at 03:39:32PM -0700, Mickey Spiegel wrote:
> > On Sun, Apr 30, 2017 at 4:22 PM, Ben Pfaff <b...@ovn.org> wrote:
> >
> > > Without this support, ovn-trace is not very useful with OpenStack,
> which
> > > uses connection tracking extensively.
> > >
> >
> > I scanned the patch set briefly, not what I would call a full review but
> > quick sanity checking. The only issue that I saw is described inline
> below.
>
> Thanks!
>
> > > + struct ovntrace_node *node = ovntrace_node_append(
> > > + super, OVNTRACE_NODE_TRANSFORMATION, "%s", ds_cstr(&s));
> > > + ds_destroy(&s);
> > > +
> > > + /* Trace the actions in the next table. */
> > > + trace__(dp, &ct_flow, ct_nat->ltable, pipeline, &node->subs);
> > >
> >
> > Since OpenStack uses NAT on distributed routers, moving on to the next
> > table is the right thing to do.
> >
> > However, in case gateway routers are used, ct_snat without an IP address
> > does not do recirc.
> > Lines 832 to 842 of ovn/lib/actions.c:
> >
> > } else if (snat && ep->is_gateway_router) {
> > /* For performance reasons, we try to prevent additional
> > * recirculations. ct_snat which is used in a gateway router
> > * does not need a recirculation. ct_snat(IP) does need a
> > * recirculation. ct_snat in a distributed router needs
> > * recirculation regardless of whether an IP address is
> > * specified.
> > * XXX Should we consider a method to let the actions specify
> > * whether an action needs recirculation if there are more use
> > * cases?. */
> > ct->recirc_table = NX_CT_RECIRC_NONE;
> >
> > Lines 4548, 4549 of ovn/northd/ovn-northd.c for a gateway router:
> >
> > ovn_lflow_add(lflows, od, S_ROUTER_IN_UNSNAT, 90,
> > ds_cstr(&match), "ct_snat; next;");
> >
> > The corresponding lines 4565, 4566 of ovn/northd/ovn-northd.c for a
> > distributed router:
> >
> > ovn_lflow_add(lflows, od, S_ROUTER_IN_UNSNAT, 100,
> > ds_cstr(&match), "ct_snat;");
> >
> > I think with this code you would be seeing double on a gateway router,
> > since both "ct_snat" and "next" would trace the actions in the next
> table.
>
> Oh, that's a good point.
>
> From lflow.c, a given router is a gateway router if its datapath is
> present on the local hypervisor and it has a local L3 gateway:
>
> static bool
> is_gateway_router(const struct sbrec_datapath_binding *ldp,
> const struct hmap *local_datapaths)
> {
> struct local_datapath *ld =
> get_local_datapath(local_datapaths, ldp->tunnel_key);
> return ld ? ld->has_local_l3gateway : false;
> }
>
> Therefore, this is another bit of context that ovn-trace would need to
> be provided via command-line options. I guess it would have to be
> something like "--gateway-router no,yes" to indicate, for example, that
> the first snat is not for a gateway router and that the second one is
> (or whatever). And I'd tend to assume that the default is "no" since
> that makes the OpenStack case work OK. Mickey and Guru, does this
> concept and syntax make sense? If not, can you suggest a way?
>
Two ways to figure out if a router is a gateway router or not:
1. If you have access to nb, if the logical router has options:chassis then
it is a gateway router.
2. From sb, while processing read_ports in ovn/utilities/ovn-trace.c, any
ports with type "l3gateway" on a datapath representing a router indicate
that the router is a gateway router. That is more or less what
ovn-controller does in "add_local_datapath" in ovn/controller/binding.c to
set "has_local_l3gateway", which ends up triggering no recirc in
ovn/lib/actions.c.
The next question is whether a specific gateway router should be treated as
local. Since ovn-trace has no knowledge of topology and hypervisors, it
seems like the consistent approach would be to treat all gateway routers as
local for the purposes of ovn-trace.
Mickey
> Thanks,
>
> Ben.
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
