| Message ID | 20170430232231.15151-24-blp@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
On Sun, Apr 30, 2017 at 4:22 PM, Ben Pfaff <blp@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. > > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > ovn/utilities/ovn-trace.8.xml | 50 ++++++++++++++++++++++++++++++ > +++++++++++++ > ovn/utilities/ovn-trace.c | 43 ++++++++++++++++++++++++++++++++++--- > 2 files changed, 90 insertions(+), 3 deletions(-) > > diff --git a/ovn/utilities/ovn-trace.8.xml b/ovn/utilities/ovn-trace.8.xml > index 8bb329bfbd71..b2d46ac3d50b 100644 > --- a/ovn/utilities/ovn-trace.8.xml > +++ b/ovn/utilities/ovn-trace.8.xml > @@ -166,6 +166,56 @@ > output; > </pre> > > + <h2>Stateful Actions</h2> > + > + <p> > + Some OVN logical actions use or update state that is not available in > the > + southbound database. <code>ovn-trace</code> handles these actions as > + described below: > + </p> > + > + <dl> > + <dt>ct_next</dt> > + <dd> > + By default <code>ovn-trace</code> treats flows as ``tracked'' and > + ``established.'' The <code>--ct</code> option overrides this > behavior; > + refer to its description for more information. > + </dd> > + > + <dt>ct_commit</dt> > + <dd> > + This action is treated as a no-op. > + </dd> > + > + <dt>ct_dnat</dt> > + <dt>ct_snat</dt> > + <dd> > + <p> > + When one of these action is used without arguments, to ``un-NAT'' > a > + packet, <code>ovn-trace</code> assumes that no NAT state was > available > + and treats it as a no-op. > + </p> > + > + <p> > + With an argument, <code>ovn-trace</code> sets the IP destination > or > + source, as appropriate, to the given address. It also sets > + <code>ct.dnat</code> or <code>ct.snat</code> to 1 to indicate > that NAT > + has taken place. > + </p> > + </dd> > + > + <dt>ct_lb</dt> > + <dd> > + Not yet implemented; currently implemented as a no-op. > + </dd> > + > + <dt>put_arp</dt> > + <dt>put_nd</dt> > + <dd> > + This action is treated as a no-op. > + </dd> > + </dl> > + > <h2>Summary Output</h2> > > <p> > diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c > index 647c8a51cd0d..4346fb39268a 100644 > --- a/ovn/utilities/ovn-trace.c > +++ b/ovn/utilities/ovn-trace.c > @@ -1510,6 +1510,42 @@ execute_ct_next(const struct ovnact_ct_next > *ct_next, > } > > static void > +execute_ct_nat(const struct ovnact_ct_nat *ct_nat, > + const struct ovntrace_datapath *dp, struct flow *uflow, > + enum ovnact_pipeline pipeline, struct ovs_list *super) > +{ > + bool is_dst = ct_nat->ovnact.type == OVNACT_CT_DNAT; > + const char *direction = is_dst ? "dst" : "src"; > + > + /* Make a sub-node for attaching the next table, > + * and figure out the changes if any. */ > + struct flow ct_flow = *uflow; > + struct ds s = DS_EMPTY_INITIALIZER; > + ds_put_format(&s, "ct_%cnat", direction[0]); > + if (ct_nat->ip) { > + ds_put_format(&s, "(ip4.%s="IP_FMT")", direction, > IP_ARGS(ct_nat->ip)); > + ovs_be32 *ip = is_dst ? &ct_flow.nw_dst : &ct_flow.nw_src; > + *ip = ct_nat->ip; > + > + uint8_t state = is_dst ? CS_DST_NAT : CS_SRC_NAT; > + ct_flow.ct_state |= state; > + } else { > + ds_put_format(&s, " /* assuming no un-%cnat entry, so no change > */", > + direction[0]); > + } > + 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. Mickey + > + /* Upon return, we will trace the actions following the ct action in > the > + * original table. The pipeline forked, so we're using the original > + * flow, not ct_flow. */ > +} > + > +static void > trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len, > const struct ovntrace_datapath *dp, struct flow *uflow, > uint8_t table_id, enum ovnact_pipeline pipeline, > @@ -1573,10 +1609,11 @@ trace_actions(const struct ovnact *ovnacts, size_t > ovnacts_len, > break; > > case OVNACT_CT_DNAT: > + execute_ct_nat(ovnact_get_CT_DNAT(a), dp, uflow, pipeline, > super); > + break; > + > case OVNACT_CT_SNAT: > - ovntrace_node_append(super, OVNTRACE_NODE_ERROR, > - "*** ct_dnat and ct_snat actions " > - "not implemented"); > + execute_ct_nat(ovnact_get_CT_SNAT(a), dp, uflow, pipeline, > super); > break; > > case OVNACT_CT_LB: > -- > 2.10.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Mon, May 01, 2017 at 03:39:32PM -0700, Mickey Spiegel wrote: > On Sun, Apr 30, 2017 at 4:22 PM, Ben Pfaff <blp@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? Thanks, Ben.
On Mon, May 1, 2017 at 5:12 PM, Ben Pfaff <blp@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 <blp@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. >
One minor nit and one real comment below. On Tue, May 2, 2017 at 11:07 AM, Ben Pfaff <blp@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 <blp@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 <blp@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); } Mickey const char *direction = is_dst ? "dst" : "src"; > > /* Make a sub-node for attaching the next table, >
diff --git a/ovn/utilities/ovn-trace.8.xml b/ovn/utilities/ovn-trace.8.xml index 8bb329bfbd71..b2d46ac3d50b 100644 --- a/ovn/utilities/ovn-trace.8.xml +++ b/ovn/utilities/ovn-trace.8.xml @@ -166,6 +166,56 @@ output; </pre> + <h2>Stateful Actions</h2> + + <p> + Some OVN logical actions use or update state that is not available in the + southbound database. <code>ovn-trace</code> handles these actions as + described below: + </p> + + <dl> + <dt>ct_next</dt> + <dd> + By default <code>ovn-trace</code> treats flows as ``tracked'' and + ``established.'' The <code>--ct</code> option overrides this behavior; + refer to its description for more information. + </dd> + + <dt>ct_commit</dt> + <dd> + This action is treated as a no-op. + </dd> + + <dt>ct_dnat</dt> + <dt>ct_snat</dt> + <dd> + <p> + When one of these action is used without arguments, to ``un-NAT'' a + packet, <code>ovn-trace</code> assumes that no NAT state was available + and treats it as a no-op. + </p> + + <p> + With an argument, <code>ovn-trace</code> sets the IP destination or + source, as appropriate, to the given address. It also sets + <code>ct.dnat</code> or <code>ct.snat</code> to 1 to indicate that NAT + has taken place. + </p> + </dd> + + <dt>ct_lb</dt> + <dd> + Not yet implemented; currently implemented as a no-op. + </dd> + + <dt>put_arp</dt> + <dt>put_nd</dt> + <dd> + This action is treated as a no-op. + </dd> + </dl> + <h2>Summary Output</h2> <p> diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c index 647c8a51cd0d..4346fb39268a 100644 --- a/ovn/utilities/ovn-trace.c +++ b/ovn/utilities/ovn-trace.c @@ -1510,6 +1510,42 @@ execute_ct_next(const struct ovnact_ct_next *ct_next, } static void +execute_ct_nat(const struct ovnact_ct_nat *ct_nat, + const struct ovntrace_datapath *dp, struct flow *uflow, + enum ovnact_pipeline pipeline, struct ovs_list *super) +{ + bool is_dst = ct_nat->ovnact.type == OVNACT_CT_DNAT; + const char *direction = is_dst ? "dst" : "src"; + + /* Make a sub-node for attaching the next table, + * and figure out the changes if any. */ + struct flow ct_flow = *uflow; + struct ds s = DS_EMPTY_INITIALIZER; + ds_put_format(&s, "ct_%cnat", direction[0]); + if (ct_nat->ip) { + ds_put_format(&s, "(ip4.%s="IP_FMT")", direction, IP_ARGS(ct_nat->ip)); + ovs_be32 *ip = is_dst ? &ct_flow.nw_dst : &ct_flow.nw_src; + *ip = ct_nat->ip; + + uint8_t state = is_dst ? CS_DST_NAT : CS_SRC_NAT; + ct_flow.ct_state |= state; + } else { + ds_put_format(&s, " /* assuming no un-%cnat entry, so no change */", + direction[0]); + } + 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); + + /* Upon return, we will trace the actions following the ct action in the + * original table. The pipeline forked, so we're using the original + * flow, not ct_flow. */ +} + +static void trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len, const struct ovntrace_datapath *dp, struct flow *uflow, uint8_t table_id, enum ovnact_pipeline pipeline, @@ -1573,10 +1609,11 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len, break; case OVNACT_CT_DNAT: + execute_ct_nat(ovnact_get_CT_DNAT(a), dp, uflow, pipeline, super); + break; + case OVNACT_CT_SNAT: - ovntrace_node_append(super, OVNTRACE_NODE_ERROR, - "*** ct_dnat and ct_snat actions " - "not implemented"); + execute_ct_nat(ovnact_get_CT_SNAT(a), dp, uflow, pipeline, super); break; case OVNACT_CT_LB:
Without this support, ovn-trace is not very useful with OpenStack, which uses connection tracking extensively. Signed-off-by: Ben Pfaff <blp@ovn.org> --- ovn/utilities/ovn-trace.8.xml | 50 +++++++++++++++++++++++++++++++++++++++++++ ovn/utilities/ovn-trace.c | 43 ++++++++++++++++++++++++++++++++++--- 2 files changed, 90 insertions(+), 3 deletions(-)