On Wed, May 3, 2017 at 8:45 AM, Ben Pfaff <b...@ovn.org> wrote:
> Without this support, ovn-trace is not very useful with OpenStack, which
> uses connection tracking extensively.
>
> Signed-off-by: Ben Pfaff <b...@ovn.org>
>
Acked-by: Mickey Spiegel <mickeys....@gmail.com>
A couple of minor comments below.
---
> ovn/utilities/ovn-trace.8.xml | 50 ++++++++++++++++++++++++++++++
> +++++++++++
> ovn/utilities/ovn-trace.c | 52 ++++++++++++++++++++++++++++++
> ++++++++++---
> 2 files changed, 99 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>
>
The flow is a little funny. At the beginning of the output section:
<code>ovn-trace</code> supports the three different forms of output,
each
described in a separate section below.
Now there is another h2 section in between the three sections describing
the different forms of output.
Perhaps just use <h3>?
+
> + <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
>
s/action/actions
> + 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 3a0780eb931e..860fd4b26be0 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 as local for our purposes. */
> + dp->has_local_l3gateway = true;
> }
> }
>
> @@ -1522,6 +1527,46 @@ 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;
> + 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";
> +
> + /* 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,
> @@ -1585,10 +1630,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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
