Message ID | 1498068412-89824-4-git-send-email-yihung.wei@gmail.com |
---|---|
State | Changes Requested |
Headers | show |
On 21 June 2017 at 11:06, Yi-Hung Wei <yihung.wei@gmail.com> 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 <yihung.wei@gmail.com> > --- > 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 ofproto/trace. > + 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); It wouldn't hurt to expand this a bit for improved readability, declaring the variables first, then getting the ovs_list_pop_front() pointer, then assigning the container, getting state and freeing. > + > + 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; I guess these could just be return xasprintf(...) ? > + } > + > + uint32_t ct_state = parse_ct_state(argv[++k], 0); I'm a bit concerned that parse_oftrace_options() may be called from ovs-vswitchd, and parse_ct_state() may, on parse failure, exit in a fatal manner. If this is the case, perhaps we should refactor parse_ct_state() into a version that should exit fatally for commandline execution, and the version run from the main vswitchd thread. > + 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); You might consider refactoring the above into a function like oftrace_push_ct_state(next_ct_states, ct_state). Then the malloc/free operations would be matched between this new function and the above oftrace_next_ct_state().. which you may also consider renaming something like oftrace_pop_ct_state(...). > + } else { > + error = xasprintf("Invalid option %s", argv[k]); > + return error; Same as above xasprintf comment. > + } > + } > + > + 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"; Could you expand on this a bit? It can be confusing for users to get stonewalled with "Syntax error" and not know why the syntax was wrong. (I realise there's other examples later in the function where this error is used, it'd be nice to improve that as well). Thanks!
On 6/21/17, 11:06 AM, "ovs-dev-bounces@openvswitch.org on behalf of Yi-Hung Wei" <ovs-dev-bounces@openvswitch.org on behalf of yihung.wei@gmail.com> 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 <yihung.wei@gmail.com>
---
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.
+.
+.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
dev@openvswitch.org
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=
On Wed, Jun 21, 2017 at 1:37 PM, Darrell Ball <dball@vmware.com> wrote: > > > On 6/21/17, 11:06 AM, "ovs-dev-bounces@openvswitch.org on behalf of Yi-Hung Wei" <ovs-dev-bounces@openvswitch.org on behalf of yihung.wei@gmail.com> 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 <yihung.wei@gmail.com> > --- > 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 > dev@openvswitch.org > 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= > >
On Wed, Jun 21, 2017 at 1:02 PM, Joe Stringer <joe@ovn.org> wrote: > On 21 June 2017 at 11:06, Yi-Hung Wei <yihung.wei@gmail.com> 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 <yihung.wei@gmail.com> >> --- >> 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 > > ofproto/trace. Fixed in v3. > >> + 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); > > It wouldn't hurt to expand this a bit for improved readability, > declaring the variables first, then getting the ovs_list_pop_front() > pointer, then assigning the container, getting state and freeing. Sure. > >> + >> + 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; > > I guess these could just be return xasprintf(...) ? Sure. > >> + } >> + >> + uint32_t ct_state = parse_ct_state(argv[++k], 0); > > I'm a bit concerned that parse_oftrace_options() may be called from > ovs-vswitchd, and parse_ct_state() may, on parse failure, exit in a > fatal manner. If this is the case, perhaps we should refactor > parse_ct_state() into a version that should exit fatally for > commandline execution, and the version run from the main vswitchd > thread. This is a good point. I break parse_ct_state() into parse_ct_state() and validate_ct_sate(), and move the ovs_fatal() logic into the caller of parse_ct_state(). Later on if ovs-vswitchd needs to use parse_ct_state() it will not necessarily trigger ovs_fatal(). > >> + 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); > > You might consider refactoring the above into a function like > oftrace_push_ct_state(next_ct_states, ct_state). Then the malloc/free > operations would be matched between this new function and the above > oftrace_next_ct_state().. which you may also consider renaming > something like oftrace_pop_ct_state(...). Thanks, that will be more clean. I refactor that in v2. > >> + } else { >> + error = xasprintf("Invalid option %s", argv[k]); >> + return error; > > Same as above xasprintf comment. Yes. > >> + } >> + } >> + >> + 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"; > > Could you expand on this a bit? It can be confusing for users to get > stonewalled with "Syntax error" and not know why the syntax was wrong. > (I realise there's other examples later in the function where this > error is used, it'd be nice to improve that as well). I replace it with "Syntax error: invalid option" > > Thanks!
On 6/26/17, 10:18 AM, "Yi-Hung Wei" <yihung.wei@gmail.com> wrote: On Wed, Jun 21, 2017 at 1:37 PM, Darrell Ball <dball@vmware.com> wrote: > > > On 6/21/17, 11:06 AM, "ovs-dev-bounces@openvswitch.org on behalf of Yi-Hung Wei" <ovs-dev-bounces@openvswitch.org on behalf of yihung.wei@gmail.com> 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 <yihung.wei@gmail.com> > --- > 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 > dev@openvswitch.org > 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= > >
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 <dball@vmware.com> wrote: > > > On 6/26/17, 10:18 AM, "Yi-Hung Wei" <yihung.wei@gmail.com> wrote: > > On Wed, Jun 21, 2017 at 1:37 PM, Darrell Ball <dball@vmware.com> wrote: > > > > > > On 6/21/17, 11:06 AM, "ovs-dev-bounces@openvswitch.org on behalf of Yi-Hung Wei" <ovs-dev-bounces@openvswitch.org on behalf of yihung.wei@gmail.com> 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 <yihung.wei@gmail.com> > > --- > > 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 > > dev@openvswitch.org > > 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= > > > > > > > >
Thanks Yi-hung That should partially address point ‘2’ as well. Maybe we can also describe the iterative process of debugging ct-actions, so the user has the expectation, basically describing that debugging would probably start with one ct-next and then add another one by one on each subsequent run as more information becomes available. Darrell On 6/26/17, 5:30 PM, "Yi-Hung Wei" <yihung.wei@gmail.com> wrote: 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 <dball@vmware.com> wrote: > > > On 6/26/17, 10:18 AM, "Yi-Hung Wei" <yihung.wei@gmail.com> wrote: > > On Wed, Jun 21, 2017 at 1:37 PM, Darrell Ball <dball@vmware.com> wrote: > > > > > > On 6/21/17, 11:06 AM, "ovs-dev-bounces@openvswitch.org on behalf of Yi-Hung Wei" <ovs-dev-bounces@openvswitch.org on behalf of yihung.wei@gmail.com> 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 <yihung.wei@gmail.com> > > --- > > 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 > > dev@openvswitch.org > > 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= > > > > > > > >
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. +. +.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
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 <yihung.wei@gmail.com> --- 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(-)