On Tue, May 02, 2017 at 11:33:27AM -0700, Mickey Spiegel wrote: > One minor nit and one real comment below. > > On Tue, May 2, 2017 at 11:07 AM, Ben Pfaff <b...@ovn.org> wrote: > > > On Mon, May 01, 2017 at 05:50:57PM -0700, Mickey Spiegel wrote: > > > 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. > > > > OK, how about folding in something like this? > > > > --8<--------------------------cut here-------------------------->8-- > > > > diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c > > index 4346fb39268a..638c21317f85 100644 > > --- a/ovn/utilities/ovn-trace.c > > +++ b/ovn/utilities/ovn-trace.c > > @@ -346,6 +346,8 @@ struct ovntrace_datapath { > > size_t n_flows, allocated_flows; > > > > struct hmap mac_bindings; /* Contains "struct > > ovntrace_mac_binding"s. */ > > + > > + bool has_local_l3gateway; > > }; > > > > struct ovntrace_port { > > @@ -570,6 +572,9 @@ read_ports(void) > > port->peer->peer = port; > > } > > } > > + } else if (!strcmp(sbpb->type, "l3gateway")) { > > + /* Treat all gateways are local for our purposes. */ > > > > Minor nit: Treat all gateways as local for our purposes. > > > > + dp->has_local_l3gateway = true; > > } > > } > > > > @@ -1515,6 +1520,10 @@ execute_ct_nat(const struct ovnact_ct_nat *ct_nat, > > enum ovnact_pipeline pipeline, struct ovs_list *super) > > { > > bool is_dst = ct_nat->ovnact.type == OVNACT_CT_DNAT; > > + if (!is_dst && dp->has_local_l3gateway) { > > + /* ct_snat has no visible effect in a gateway router. */ > > + return; > > + } > > > > If an IP address is specified with ct_snat, then you want to replace the > address, and even on a gateway router it does trigger recirc. > > Instead of this change, near the bottom, something like the following: > > if (is_dst || ct_nat->ip || !dp->has_local_l3gateway) { > /* Trace the actions in the next table, except for ct_snat > * with no IP on a gateway router since there is no recirc > * in that case. */ > trace__(dp, &ct_flow, ct_nat->ltable, pipeline, &node->subs); > }
Thanks for pointing out those problems. I recognize that this did the wrong thing for ct_snat(IP) for a gateway router. But I think we want to skip most of the function in the "ct_snat;" case for a gateway router, because if I understand correctly there can never be a visible change to the flow, or other noticeable behavior, in that case. So, I folded this in: diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c index 65623e1beb41..860fd4b26be0 100644 --- a/ovn/utilities/ovn-trace.c +++ b/ovn/utilities/ovn-trace.c @@ -573,7 +573,7 @@ read_ports(void) } } } else if (!strcmp(sbpb->type, "l3gateway")) { - /* Treat all gateways are local for our purposes. */ + /* Treat all gateways as local for our purposes. */ dp->has_local_l3gateway = true; } } @@ -1532,8 +1532,8 @@ execute_ct_nat(const struct ovnact_ct_nat *ct_nat, enum ovnact_pipeline pipeline, struct ovs_list *super) { bool is_dst = ct_nat->ovnact.type == OVNACT_CT_DNAT; - if (!is_dst && dp->has_local_l3gateway) { - /* ct_snat has no visible effect in a gateway router. */ + if (!is_dst && dp->has_local_l3gateway && !ct_nat->ip) { + /* "ct_snat;" has no visible effect in a gateway router. */ return; } const char *direction = is_dst ? "dst" : "src"; _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev