[ovs-dev] [PATCH] Extend sFlow agent to report tunnel and MPLS structures
Ben Pfaff
blp at nicira.com
Tue Jul 21 21:19:32 UTC 2015
On Fri, Jul 17, 2015 at 09:37:02PM -0700, Neil McKee wrote:
> Packets are still sampled at ingress only, so the egress
> tunnel and/or MPLS structures are only included when there is just 1 output
> port. The actions are either provided by the datapath in the sample upcall
> or looked up in the userspace cache. The former is preferred because it is
> more reliable and does not present any new demands or constraints on the
> userspace cache, however the code falls back on the userspace lookup so that
> this solution can work with existing kernel datapath modules. If the lookup
> fails it is not critical: the compiled user-action-cookie is still available
> and provides the essential output port and output VLAN forwarding information
> just as before.
>
> The openvswitch actions can express almost any tunneling/mangling so the only
> totally faithful representation would be to somehow encode the whole list of
> flow actions in the sFlow output. However the standard sFlow tunnel structures
> can express most common real-world scenarios, so in parsing the actions we
> look for those and skip the encoding if we see anything unusual. For example,
> a single set(tunnel()) or tnl_push() is interpreted, but if a second such
> action is encountered then the egress tunnel reporting is suppressed.
>
> The sFlow standard allows "best effort" encoding so that if a field is not
> knowable or too onerous to look up then it can be left out. This is often
> the case for the layer-4 source port or even the src ip address of a tunnel.
> The assumption is that monitoring is enabled everywhere so a missing field
> can typically be seen at ingress to the next switch in the path.
>
> This patch also adds unit tests to check the sFlow encoding of set(tunnel()),
> tnl_push() and push_mpls() actions.
>
> The netlink attribute to request that actions be included in the upcall
> from the datapath is inserted for sFlow sampling only. To make that option
> be explicit would require further changes to the printing and parsing of
> actions in lib/odp-util.c, and to scripts in the test suite.
>
> Further enhancements to report on 802.1AD QinQ, 64-bit tunnel IDs, and NAT
> transformations can follow in future patches that make only incremental
> changes.
>
> Signed-off-by: Neil McKee <neil.mckee at inmon.com>
Thanks Neil!
A lot of the new code here used tabs instead of spaces for indentation.
I fixed it up where I noticed it.
The parsing and formatting code in odp-util.c is supposed to use a very
"raw" representation of the Netlink form. I wasn't really comfortable
with having "actions" be implicit, so I folded in the following to make
it explicit:
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -257,7 +257,6 @@ format_odp_userspace_action(struct ds *ds, const struct nlattr *attr)
struct nlattr *a[ARRAY_SIZE(ovs_userspace_policy)];
const struct nlattr *userdata_attr;
const struct nlattr *tunnel_out_port_attr;
- const struct nlattr *actions_attr;
if (!nl_parse_nested(attr, ovs_userspace_policy, a, ARRAY_SIZE(a))) {
ds_put_cstr(ds, "userspace(error)");
@@ -325,22 +324,16 @@ format_odp_userspace_action(struct ds *ds, const struct nlattr *attr)
}
}
+ if (a[OVS_USERSPACE_ATTR_ACTIONS]) {
+ ds_put_cstr(ds, ",actions");
+ }
+
tunnel_out_port_attr = a[OVS_USERSPACE_ATTR_EGRESS_TUN_PORT];
if (tunnel_out_port_attr) {
ds_put_format(ds, ",tunnel_out_port=%"PRIu32,
nl_attr_get_u32(tunnel_out_port_attr));
}
- actions_attr = a[OVS_USERSPACE_ATTR_ACTIONS];
- if (actions_attr) {
- /* XXX This attribute is only ever used by sFlow, so treat
- * it as being implicit and do not print it here - at least
- * not until we parse it back again in parse_odp_userspace_action().
- * This affects unit test 367 in tests/odp.at.
- */
- /* ds_put_cstr(ds, ",actions"); */
- }
-
ds_put_char(ds, ')');
}
@@ -709,7 +702,6 @@ parse_odp_userspace_action(const char *s, struct ofpbuf *actions)
cookie.sflow.output = output;
user_data = &cookie;
user_data_size = sizeof cookie.sflow;
- include_actions = true;
} else if (ovs_scan(&s[n], ",slow_path(%n",
&n1)) {
int res;
@@ -769,6 +761,14 @@ parse_odp_userspace_action(const char *s, struct ofpbuf *actions)
{
int n1 = -1;
+ if (ovs_scan(&s[n], ",actions%n", &n1)) {
+ n += n1;
+ include_actions = true;
+ }
+ }
+
+ {
+ int n1 = -1;
if (ovs_scan(&s[n], ",tunnel_out_port=%"SCNi32")%n",
&tunnel_out_port, &n1)) {
odp_put_userspace_action(pid, user_data, user_data_size,
diff --git a/tests/odp.at b/tests/odp.at
index a50a9cd..61edde3 100644
--- a/tests/odp.at
+++ b/tests/odp.at
@@ -234,13 +234,13 @@ AT_DATA([actions.txt], [dnl
1,2,3
userspace(pid=555666777)
userspace(pid=555666777,tunnel_out_port=10)
-userspace(pid=6633,sFlow(vid=9,pcp=7,output=10))
-userspace(pid=6633,sFlow(vid=9,pcp=7,output=10),tunnel_out_port=10)
+userspace(pid=6633,sFlow(vid=9,pcp=7,output=10),actions)
+userspace(pid=6633,sFlow(vid=9,pcp=7,output=10),actions,tunnel_out_port=10)
userspace(pid=9765,slow_path(0))
userspace(pid=9765,slow_path(0),tunnel_out_port=10)
userspace(pid=9765,slow_path(cfm))
userspace(pid=9765,slow_path(cfm),tunnel_out_port=10)
-userspace(pid=1234567,userdata(0102030405060708090a0b0c0d0e0f))
+userspace(pid=1234567,userdata(0102030405060708090a0b0c0d0e0f),actions)
userspace(pid=1234567,userdata(0102030405060708090a0b0c0d0e0f),tunnel_out_port=10)
userspace(pid=6633,flow_sample(probability=123,collector_set_id=1234,obs_domain_id=2345,obs_point_id=3456))
userspace(pid=6633,flow_sample(probability=123,collector_set_id=1234,obs_domain_id=2345,obs_point_id=3456),tunnel_out_port=10)
This caused some "sparse" warnings. I fixed them.
I made coding style changes in sflow_read_tnl_push_action().
I removed the code that took a mutex in process_upcall(). The comment
on struct udpif_key, the structure being accessed, says that no mutex is
needed:
/* These elements are read only once created, and therefore aren't
* protected by a mutex. */
I added a NEWS item.
And I pushed the whole thing to master. Thank you for your contributions!
More information about the dev
mailing list