Darrell and I have some discussion, and we both agree on having the default option to be 'trk|new'. I will send out a v3 based on that.
-Yi-Hung On Mon, Jun 26, 2017 at 11:10 AM, Darrell Ball <[email protected]> wrote: > > > On 6/26/17, 10:18 AM, "Yi-Hung Wei" <[email protected]> wrote: > > 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. > > Yi-hung > I am not following how your response is related to my above comments. > I will ask offline and come back to the alias. > > > 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". > > The number of hits has nothing to do with my; it is quality vs quantity. > My comment is simply that people, like myself, usually want to find out > why a packet is ‘not’ EST. > > > 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. > > Who are the advanced users you are thinking about ? > > > > > > > +. > > +.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
