[ovs-dev] [PATCH 3/3] ofproto/trace: Add --ct-next option to ofproto/trace
Darrell Ball
dball at vmware.com
Wed Jun 21 20:37:53 UTC 2017
On 6/21/17, 11:06 AM, "ovs-dev-bounces at openvswitch.org on behalf of Yi-Hung Wei" <ovs-dev-bounces at openvswitch.org on behalf of yihung.wei at 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 at 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 at 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=
More information about the dev
mailing list