On Wed, Jun 21, 2017 at 1:37 PM, Darrell Ball <[email protected]> wrote:
>
>
> On 6/21/17, 11:06 AM, "[email protected] on behalf of Yi-Hung
> Wei" <[email protected] on behalf of [email protected]>
> wrote:
>
> Previous patch enables ofproto/trace to automatically trace a flow
> that involves multiple recirculation on conntrack. However, it always
> sets the ct_state to trk|est when it processes recirculated conntrack
> flows.
> With this patch, users can customize the expected next ct_state in the
> aforementioned use case.
>
> Signed-off-by: Yi-Hung Wei <[email protected]>
> ---
> NEWS | 2 +
> ofproto/ofproto-dpif-trace.c | 111
> ++++++++++++++++++++++++++++++++++++-------
> ofproto/ofproto-dpif-trace.h | 6 +++
> ofproto/ofproto-unixctl.man | 54 +++++++++++++++++++--
> tests/completion.at | 8 ++--
> tests/ofproto-dpif.at | 33 +++++++++----
> 6 files changed, 182 insertions(+), 32 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 4ea7e628e1fc..1824ec9de744 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -58,6 +58,8 @@ Post-v2.7.0
> - Fedora Packaging:
> * OVN services are no longer restarted automatically after upgrade.
> - Add --cleanup option to command 'ovs-appctl exit' (see
> ovs-vswitchd(8)).
> + - Add --ct-next option to command 'ovs-appctl ofprot/trace' (see
> + ovs-vswitchd(8)).
> - L3 tunneling:
> * Add "layer3" options for tunnel ports that support non-Ethernet
> (L3)
> payload (GRE, VXLAN-GPE).
> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
> index d8cdd92493cf..e75c62d464eb 100644
> --- a/ofproto/ofproto-dpif-trace.c
> +++ b/ofproto/ofproto-dpif-trace.c
> @@ -18,6 +18,7 @@
>
> #include "ofproto-dpif-trace.h"
>
> +#include "conntrack.h"
> #include "dpif.h"
> #include "ofproto-dpif-xlate.h"
> #include "openvswitch/ofp-parse.h"
> @@ -26,7 +27,7 @@
> static void ofproto_trace(struct ofproto_dpif *, struct flow *,
> const struct dp_packet *packet,
> const struct ofpact[], size_t ofpacts_len,
> - struct ds *);
> + struct ds *, struct ovs_list *next_ct_states);
> static void oftrace_node_destroy(struct oftrace_node *);
>
> /* Creates a new oftrace_node, populates it with the given 'type' and a
> copy of
> @@ -119,6 +120,17 @@ oftrace_recirc_node_destroy(struct
> oftrace_recirc_node *node)
> free(node);
> }
>
> +static uint32_t
> +oftrace_next_ct_state(struct ovs_list *next_ct_states)
> +{
> + struct oftrace_next_ct_state *next_ct_state;
> + ASSIGN_CONTAINER(next_ct_state, ovs_list_pop_front(next_ct_states),
> node);
> + uint32_t state = next_ct_state->state;
> + free(next_ct_state);
> +
> + return state;
> +}
> +
> static void
> oftrace_node_print_details(struct ds *output,
> const struct ovs_list *nodes, int level)
> @@ -160,18 +172,47 @@ oftrace_node_print_details(struct ds *output,
> }
> }
>
> +static char * OVS_WARN_UNUSED_RESULT
> +parse_oftrace_options(int argc, const char *argv[],
> + struct ovs_list *next_ct_states)
> +{
> + char *error = NULL;
> + int k;
> +
> + for (k = 0; k < argc; k++) {
> + if (!strncmp(argv[k], "--ct-next", 9)) {
> + if (k + 1 > argc) {
> + error = xasprintf("Missing argument for option %s",
> argv[k]);
> + return error;
> + }
> +
> + uint32_t ct_state = parse_ct_state(argv[++k], 0);
> + struct oftrace_next_ct_state *next_state =
> + xmalloc(sizeof *next_state);
> + next_state->state = ct_state;
> + ovs_list_push_back(next_ct_states, &next_state->node);
> + } else {
> + error = xasprintf("Invalid option %s", argv[k]);
> + return error;
> + }
> + }
> +
> + return error;
> +}
> +
> /* Parses the 'argc' elements of 'argv', ignoring argv[0]. The following
> * forms are supported:
> *
> - * - [dpname] odp_flow [-generate | packet]
> - * - bridge br_flow [-generate | packet]
> + * - [dpname] odp_flow [OPTIONS] [-generate | packet]
> + * - bridge br_flow [OPTIONS] [-generate | packet]
> *
> * On success, initializes '*ofprotop' and 'flow' and returns NULL. On
> failure
> * returns a nonnull malloced error message. */
> static char * OVS_WARN_UNUSED_RESULT
> parse_flow_and_packet(int argc, const char *argv[],
> struct ofproto_dpif **ofprotop, struct flow *flow,
> - struct dp_packet **packetp)
> + struct dp_packet **packetp,
> + struct ovs_list *next_ct_states)
> {
> const struct dpif_backer *backer = NULL;
> const char *error = NULL;
> @@ -180,6 +221,7 @@ parse_flow_and_packet(int argc, const char *argv[],
> struct dp_packet *packet;
> struct ofpbuf odp_key;
> struct ofpbuf odp_mask;
> + int first_option;
>
> ofpbuf_init(&odp_key, 0);
> ofpbuf_init(&odp_mask, 0);
> @@ -199,6 +241,25 @@ parse_flow_and_packet(int argc, const char *argv[],
> error = NULL;
> }
>
> + /* Parse options. */
> + if (argc >= 4) {
> + if (!strncmp(argv[2], "--", 2)) {
> + first_option = 2;
> + } else if (!strncmp(argv[3], "--", 2)) {
> + first_option = 3;
> + } else {
> + error = "Syntax error";
> + goto exit;
> + }
> +
> + m_err = parse_oftrace_options(argc - first_option, argv +
> first_option,
> + next_ct_states);
> + if (m_err) {
> + goto exit;
> + }
> + argc = first_option;
> + }
> +
> /* odp_flow can have its in_port specified as a name instead of port
> no.
> * We do not yet know whether a given flow is a odp_flow or a
> br_flow.
> * But, to know whether a flow is odp_flow through
> odp_flow_from_string(),
> @@ -338,13 +399,16 @@ ofproto_unixctl_trace(struct unixctl_conn *conn,
> int argc, const char *argv[],
> struct dp_packet *packet;
> char *error;
> struct flow flow;
> + struct ovs_list next_ct_states =
> OVS_LIST_INITIALIZER(&next_ct_states);
>
> - error = parse_flow_and_packet(argc, argv, &ofproto, &flow, &packet);
> + error = parse_flow_and_packet(argc, argv, &ofproto, &flow, &packet,
> + &next_ct_states);
> if (!error) {
> struct ds result;
>
> ds_init(&result);
> - ofproto_trace(ofproto, &flow, packet, NULL, 0, &result);
> + ofproto_trace(ofproto, &flow, packet, NULL, 0, &result,
> + &next_ct_states);
> unixctl_command_reply(conn, ds_cstr(&result));
> ds_destroy(&result);
> dp_packet_delete(packet);
> @@ -366,6 +430,7 @@ ofproto_unixctl_trace_actions(struct unixctl_conn
> *conn, int argc,
> struct ds result;
> struct match match;
> uint16_t in_port;
> + struct ovs_list next_ct_states =
> OVS_LIST_INITIALIZER(&next_ct_states);
>
> /* Three kinds of error return values! */
> enum ofperr retval;
> @@ -395,7 +460,8 @@ ofproto_unixctl_trace_actions(struct unixctl_conn
> *conn, int argc,
> enforce_consistency = false;
> }
>
> - error = parse_flow_and_packet(argc, argv, &ofproto, &match.flow,
> &packet);
> + error = parse_flow_and_packet(argc, argv, &ofproto, &match.flow,
> &packet,
> + &next_ct_states);
> if (error) {
> unixctl_command_reply_error(conn, error);
> free(error);
> @@ -443,7 +509,7 @@ ofproto_unixctl_trace_actions(struct unixctl_conn
> *conn, int argc,
> }
>
> ofproto_trace(ofproto, &match.flow, packet,
> - ofpacts.data, ofpacts.size, &result);
> + ofpacts.data, ofpacts.size, &result, &next_ct_states);
> unixctl_command_reply(conn, ds_cstr(&result));
>
> exit:
> @@ -541,7 +607,7 @@ static void
> ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
> const struct dp_packet *packet,
> const struct ofpact ofpacts[], size_t ofpacts_len,
> - struct ds *output)
> + struct ds *output, struct ovs_list *next_ct_states)
> {
> struct ovs_list recirc_queue = OVS_LIST_INITIALIZER(&recirc_queue);
> oftrace_add_recirc_node(&recirc_queue, OFT_INIT, flow,
> flow->recirc_id);
> @@ -553,9 +619,21 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct
> flow *flow,
> ASSIGN_CONTAINER(recirc_node, node, node);
>
> if (recirc_node->type == OFT_CONNTRACK) {
> - recirc_node->flow->ct_state = CS_ESTABLISHED | CS_TRACKED;
> - ds_put_cstr(output, "\n\nResume conntrack prcessing with "
> - "default ct_state=trk|est.\n");
> + if (ovs_list_is_empty(next_ct_states)) {
> + recirc_node->flow->ct_state = CS_ESTABLISHED |
> CS_TRACKED;
> + ds_put_cstr(output, "\n\nResume conntrack prcessing with
> "
> + "default ct_state=trk|est. Use
> --ct-next "
> + "to customize\n");
> + } else {
> + recirc_node->flow->ct_state =
> + oftrace_next_ct_state(next_ct_states);
> + struct ds s = DS_EMPTY_INITIALIZER;
> + format_flags(&s, ct_state_to_string,
> + recirc_node->flow->ct_state, '|');
> + ds_put_format(output, "\n\nResume conntrack prcessing
> with "
> + "ct_state=%s\n", ds_cstr(&s));
> + ds_destroy(&s);
> + }
> ds_put_char_multiple(output, '-', 79);
> ds_put_char(output, '\n');
> }
> @@ -576,10 +654,11 @@ ofproto_dpif_trace_init(void)
>
> unixctl_command_register(
> "ofproto/trace",
> - "{[dp_name] odp_flow | bridge br_flow} [-generate|packet]",
> - 1, 3, ofproto_unixctl_trace, NULL);
> + "{[dp_name] odp_flow | bridge br_flow} [OPTIONS...] "
> + "[-generate|packet]", 1, INT_MAX, ofproto_unixctl_trace, NULL);
> unixctl_command_register(
> "ofproto/trace-packet-out",
> - "[-consistent] {[dp_name] odp_flow | bridge br_flow}
> [-generate|packet] actions",
> - 2, 6, ofproto_unixctl_trace_actions, NULL);
> + "[-consistent] {[dp_name] odp_flow | bridge br_flow}
> [OPTIONS...] "
> + "[-generate|packet] actions",
> + 2, INT_MAX, ofproto_unixctl_trace_actions, NULL);
> }
> diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h
> index f7299fd42848..9bb080534a7a 100644
> --- a/ofproto/ofproto-dpif-trace.h
> +++ b/ofproto/ofproto-dpif-trace.h
> @@ -73,6 +73,12 @@ struct oftrace_recirc_node {
> struct flow *flow;
> };
>
> +/* A node within a next_ct_state list. */
> +struct oftrace_next_ct_state {
> + struct ovs_list node; /* In next_ct_states. */
> + uint32_t state;
> +};
> +
> void ofproto_dpif_trace_init(void);
>
> struct oftrace_node *oftrace_report(struct ovs_list *, enum
> oftrace_node_type,
> diff --git a/ofproto/ofproto-unixctl.man b/ofproto/ofproto-unixctl.man
> index 423837351910..e6c37894e980 100644
> --- a/ofproto/ofproto-unixctl.man
> +++ b/ofproto/ofproto-unixctl.man
> @@ -6,10 +6,10 @@ These commands manage the core OpenFlow switch
> implementation (called
> Lists the names of the running ofproto instances. These are the names
> that may be used on \fBofproto/trace\fR.
> .
> -.IP "\fBofproto/trace\fR [\fIdpname\fR] \fIodp_flow\fR [\fB\-generate
> \fR| \fIpacket\fR]"
> -.IQ "\fBofproto/trace\fR \fIbridge\fR \fIbr_flow\fR [\fB\-generate \fR|
> \fIpacket\fR]"
> -.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR]
> [\fIdpname\fR] \fIodp_flow\fR [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
> -.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] \fIbridge\fR
> \fIbr_flow\fR [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
> +.IP "\fBofproto/trace\fR [\fIdpname\fR] \fIodp_flow\fR [\fIOPTIONS\fR]
> [\fB\-generate \fR| \fIpacket\fR]"
> +.IQ "\fBofproto/trace\fR \fIbridge\fR \fIbr_flow\fR [\fIOPTIONS\fR]
> [\fB\-generate \fR| \fIpacket\fR]"
> +.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR]
> [\fIdpname\fR] \fIodp_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR|
> \fIpacket\fR] \fIactions\fR"
> +.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] \fIbridge\fR
> \fIbr_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
> Traces the path of an imaginary packet through \fIswitch\fR and
> reports the path that it took. The initial treatment of the packet
> varies based on the command:
> @@ -48,6 +48,52 @@ wildcards.) \fIbridge\fR names of the bridge through
> which
> .RE
> .
> .IP
> +.RS
> +\fBofproto/trace\fR supports the following options:
> +.
> +.IP "--ct-next \fIflags\fR"
> +When the traced flow triggers conntrack actions, \fBofproto/trace\fR
> +will automatically trace the forked packet processing pipeline with
> +user specified ct_state. This option sets the ct_state flags that the
> +conntrack module will report. The \fIflags\fR must be a comma- or
> +space-separated list of the following connection tracking flags:
> +.
> +.RS
> +.IP \(bu
> +\fBtrk\fR: Include to indicate connection tracking has taken place.
> +.
> +.IP \(bu
> +\fBnew\fR: Include to indicate a new flow.
> +.
> +.IP \(bu
> +\fBest\fR: Include to indicate an established flow.
> +.
> +.IP \(bu
> +\fBrel\fR: Include to indicate a related flow.
> +.
> +.IP \(bu
> +\fBrpl\fR: Include to indicate a reply flow.
> +.
> +.IP \(bu
> +\fBinv\fR: Include to indicate a connection entry in a bad state.
> +.
> +.IP \(bu
> +\fBdnat\fR: Include to indicate a packet whose destination IP address
> has been
> +changed.
> +.
> +.IP \(bu
> +\fBsnat\fR: Include to indicate a packet whose source IP address has been
> +changed.
> +.
> +.RE
> +.
> +.IP
> +When --ct-next is unspecified, or when there are fewer --ct-next options
> that
> +ct actions, the \fIflags\fR default to trk,est.
>
> 1/ I think you should drop ‘est’ as part of the default.
> Rarely, is the ‘est’ state being debugged.
> trk by itself will catch more, in case the user does not specify options,
> is not aware of
> the options or maybe does not even understand them.
>
> 2/ I think it is very surprising that if fewer –ct-next options are provided
> than
> ct actions, that the remaining ct actions are traced as the default. I
> would
> expect the last one specified to be used for any remaining ct actions,
> if any.
> I think the typical usage would be the user providing one –ct-next and
> wanting
> it applying to any/all ct actions.
Thanks for your coments. I feel like what makes more sense maybe to
query the current ct_state from conntrack module, which will be the
next thing that I plan to do later on. I choose trk|est because
ovn-trace use it as default. Actually, if we look at currently
available conntrack rules in the testcase (git grep '=+trk+est,' *.at
| wc -l), "+trk+est" hits similar (slightly more) # of rules comparing
to "+trk".
I think ofporto/trace is for advanced users to debug, and people may
use this tool to debug some tricky coner cases. Instead of speculating
the default use cases for debugging, as long as we clearly state what
is the default behavor in the trace and how can the users customize
the option in the document, it may be good for now. We can always have
another patch to chagne the defalt behavior when we get more feedback
from the users later.
>
> +.
> +.RE
> +.
> +.IP
> Most commonly, one specifies only a flow, using one of the forms
> above, but sometimes one might need to specify an actual packet
> instead of just a flow:
> diff --git a/tests/completion.at b/tests/completion.at
> index e28c75239227..00e3a46b8bfa 100644
> --- a/tests/completion.at
> +++ b/tests/completion.at
> @@ -171,7 +171,7 @@ AT_CLEANUP
>
>
> # complex completion check - ofproto/trace
> -# ofproto/trace {[dp_name] odp_flow | bridge br_flow} [-generate|packet]
> +# ofproto/trace {[dp_name] odp_flow | bridge br_flow} [OPTIONS]
> [-generate|packet]
> # test expansion on 'dp|dp_name' and 'bridge'
> AT_SETUP([appctl-bashcomp - complex completion check 3])
> AT_SKIP_IF([test -z ${BASH_VERSION+x}])
> @@ -209,7 +209,8 @@ AT_CHECK_UNQUOTED([GET_AVAIL(${INPUT})], [0])
> # set odp_flow to some random string, should go to the next level.
> INPUT="$(bash ovs-appctl-bashcomp.bash debug ovs-appctl ofproto/trace
> ovs-dummy "in_port(123),mac(),ip,tcp" TAB 2>&1)"
> MATCH="$(GET_COMP_STR([-generate], [-generate])
> -GET_COMP_STR([packet], []))"
> +GET_COMP_STR([packet], [])
> +GET_COMP_STR([OPTIONS...], []))"
> AT_CHECK_UNQUOTED([GET_EXPAN(${INPUT})],
> [0], [dnl
> ${MATCH}
> @@ -242,7 +243,8 @@ AT_CHECK_UNQUOTED([GET_AVAIL(${INPUT})], [0])
> # set argument to some random string, should go to the odp_flow path.
> INPUT="$(bash ovs-appctl-bashcomp.bash debug ovs-appctl ofproto/trace
> "in_port(123),mac(),ip,tcp" TAB 2>&1)"
> MATCH="$(GET_COMP_STR([-generate], [-generate])
> -GET_COMP_STR([packet], []))"
> +GET_COMP_STR([packet], [])
> +GET_COMP_STR([OPTIONS...], []))"
> AT_CHECK_UNQUOTED([GET_EXPAN(${INPUT})],
> [0], [dnl
> ${MATCH}
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index c51c689b9f46..142322afd777 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -5208,14 +5208,6 @@ Trailing garbage in packet data
> ovs-appctl: ovs-vswitchd: server returned an error
> ])
>
> -# Test incorrect command: ofproto/trace with 4 arguments
> -AT_CHECK([ovs-appctl ofproto/trace \
> - arg1, arg2, arg3, arg4], [2], [stdout],[stderr])
> -AT_CHECK([tail -2 stderr], [0], [dnl
> -"ofproto/trace" command takes at most 3 arguments
> -ovs-appctl: ovs-vswitchd: server returned an error
> -])
> -
> # Test incorrect command: ofproto/trace with 0 argument
> AT_CHECK([ovs-appctl ofproto/trace ], [2], [stdout],[stderr])
> AT_CHECK([tail -2 stderr], [0], [dnl
> @@ -9713,13 +9705,14 @@ AT_CLEANUP
> AT_SETUP([ofproto-dpif - conntrack - ofproto/trace])
> OVS_VSWITCHD_START
>
> -add_of_ports br0 1 2
> +add_of_ports br0 1 2 3 4
>
> AT_DATA([flows.txt], [dnl
> dnl Table 0
> dnl
> table=0,priority=100,arp,action=normal
> table=0,priority=10,udp,action=ct(table=1,zone=0)
> +table=0,priority=10,tcp,action=ct(table=2,zone=1)
> table=0,priority=1,action=drop
> dnl
> dnl Table 1
> @@ -9728,6 +9721,18 @@
> table=1,priority=10,in_port=1,ct_state=+trk+new,udp,action=ct(commit,zone=0),2
> table=1,priority=10,in_port=1,ct_state=+trk+est,udp,action=2
> table=1,priority=10,in_port=2,ct_state=+trk+est,udp,action=1
> table=1,priority=1,action=drop
> +dnl
> +dnl Table 2
> +dnl
>
> +table=2,priority=10,in_port=1,tcp,ct_state=+trk+new,tcp,action=ct(commit,zone=1),ct(table=3,zone=2)
>
> +table=2,priority=10,in_port=1,tcp,ct_state=+trk+est,tcp,action=ct(table=3,zone=2)
> +table=2,priority=1,action=drop
> +dnl
> +dnl Table 3
> +dnl
>
> +table=3,priority=10,in_port=1,tcp,ct_state=+trk+new,tcp,action=ct(commit,zone=2),4
> +table=3,priority=10,in_port=1,tcp,ct_state=+trk+est,tcp,action=3
> +table=2,priority=1,action=drop
> ])
>
> AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> @@ -9742,6 +9747,16 @@ AT_CHECK([tail -1 stdout], [0],
> [Datapath actions: 2
> ])
>
> +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,tcp'], [0], [stdout])
> +AT_CHECK([tail -1 stdout], [0],
> + [Datapath actions: 3
> +])
> +
> +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,tcp' --ct-next
> 'trk,new' --ct-next 'trk,new' ], [0], [stdout])
> +AT_CHECK([tail -1 stdout], [0],
> + [Datapath actions: ct(commit,zone=2),4
> +])
> +
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> --
> 2.7.4
>
> _______________________________________________
> dev mailing list
> [email protected]
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=JmH2jZDW_1WD9HRXGFuUOs7-4mI6AXUopn7gzbNPXo8&s=Qwjtc88CeYT74ugg8YfxZXQAjxYM_7fELmzeLlVWyVY&e=
>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
