On Sat, Jul 13, 2024 at 01:06:01AM GMT, Ilya Maximets wrote:
> On 7/12/24 19:06, Adrian Moreno wrote:
> > When the flow translation results in a datapath action list whose last
> > action is an "observational" action, i.e: one generated for IPFIX,
> > sFlow or local sampling applications, the packet is actually going to be
> > dropped (and observed).
> >
> > In that case, add an explicit drop action so that drop statistics remain
> > accurate. This behavior is controlled by a configurable boolean knob
> > called "explicit_sampled_drops"
> >
> > Combine the "optimizations" and other odp_actions "tweaks" into a single
> > function.
> >
> > Signed-off-by: Adrian Moreno <[email protected]>
> > ---
> > NEWS | 5 +
> > ofproto/ofproto-dpif-xlate.c | 64 ++++++++++---
> > ofproto/ofproto-dpif-xlate.h | 4 +
> > ofproto/ofproto-dpif.c | 6 ++
> > ofproto/ofproto-dpif.h | 2 +
> > ofproto/ofproto-provider.h | 4 +
> > ofproto/ofproto.c | 10 ++
> > ofproto/ofproto.h | 2 +
> > tests/drop-stats.at | 173 +++++++++++++++++++++++++++++++++++
> > tests/ofproto-dpif.at | 49 ++++++++++
> > vswitchd/bridge.c | 4 +-
> > vswitchd/vswitch.xml | 26 +++++-
> > 12 files changed, 333 insertions(+), 16 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index c27727c0d..4eb73aada 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -40,6 +40,11 @@ Post-v3.3.0
> > allows samples to be emitted locally (instead of via IPFIX) in a
> > datapath-specific manner. The Linux kernel datapath is the first to
> > support this feature by using the new datapath "psample" action.
> > + - OVSDB:
>
> We normally use OVSDB for changes in the database server, not the vswitch
> schema. Just remove it.
>
> > + * A new configuration knob "other_config:explicit-sampled-drops" in
> > the
> > + Open_vSwitch table controls whether an explicit drop action shall be
> > + added at the end of datapath flows whose last action is an
> > + observability-driven sample action.
> >
> >
> > v3.3.0 - 16 Feb 2024
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index 0a0964348..e5234b950 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -3415,6 +3415,7 @@ compose_sample_action(struct xlate_ctx *ctx,
> > struct ofproto *ofproto = &ctx->xin->ofproto->up;
> > uint32_t meter_id = ofproto->slowpath_meter_id;
> > size_t cookie_offset = 0;
> > + size_t observe_offset;
> >
> > /* The meter action is only used to throttle userspace actions.
> > * If they are not needed and the sampling rate is 100%, avoid
> > generating
> > @@ -3432,6 +3433,7 @@ compose_sample_action(struct xlate_ctx *ctx,
> > }
> >
> > if (args->psample) {
> > + observe_offset = ctx->odp_actions->size;
> > odp_put_psample_action(ctx->odp_actions,
> > args->psample->group_id,
> > (void *) &args->psample->cookie,
> > @@ -3443,6 +3445,7 @@ compose_sample_action(struct xlate_ctx *ctx,
> > nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER,
> > meter_id);
> > }
> >
> > + observe_offset = ctx->odp_actions->size;
> > odp_port_t odp_port = ofp_port_to_odp_port(
> > ctx->xbridge, ctx->xin->flow.in_port.ofp_port);
> > uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port);
> > @@ -3457,6 +3460,9 @@ compose_sample_action(struct xlate_ctx *ctx,
> > if (is_sample) {
> > nl_msg_end_nested(ctx->odp_actions, actions_offset);
> > nl_msg_end_nested(ctx->odp_actions, sample_offset);
> > + ctx->xout->last_observe_offset = sample_offset;
> > + } else {
> > + ctx->xout->last_observe_offset = observe_offset;
> > }
> >
> > return cookie_offset;
> > @@ -8066,12 +8072,16 @@ xlate_wc_finish(struct xlate_ctx *ctx)
> > }
> > }
> >
> > -/* This will optimize the odp actions generated. For now, it will remove
> > - * trailing clone actions that are unnecessary. */
> > +/* This will tweak the odp actions generated. For now, it will:
> > + * - Remove trailing clone actions that are unnecessary.
> > + * - Add an explicit drop action if the action list is empty.
> > + * - Add an explicit drop action if the last action is an observability
> > + * sample. This tweak is controlled by a configurable knob. */
> > static void
> > -xlate_optimize_odp_actions(struct xlate_in *xin)
> > +xlate_tweak_odp_actions(struct xlate_ctx *ctx)
> > {
> > - struct ofpbuf *actions = xin->odp_actions;
> > + uint32_t last_observe_offset = ctx->xout->last_observe_offset;
> > + struct ofpbuf *actions = ctx->xin->odp_actions;
> > struct nlattr *last_action = NULL;
> > struct nlattr *a;
> > int left;
> > @@ -8085,11 +8095,28 @@ xlate_optimize_odp_actions(struct xlate_in *xin)
> > last_action = a;
> > }
> >
> > + if (!last_action) {
> > + if (ovs_explicit_drop_action_supported(ctx->xbridge->ofproto)) {
> > + put_drop_action(actions, XLATE_OK);
> > + }
> > + return;
> > + }
> > +
> > /* Remove the trailing clone() action, by directly embedding the nested
> > * actions. */
> > - if (last_action && nl_attr_type(last_action) == OVS_ACTION_ATTR_CLONE)
> > {
> > + if (nl_attr_type(last_action) == OVS_ACTION_ATTR_CLONE) {
> > void *dest;
> >
> > + if (last_observe_offset != UINT32_MAX &&
> > + (unsigned char *) actions->data + last_observe_offset >
> > + (unsigned char *) last_action) {
> > + /* The last sample is inside the trailing clone.
> > + * Adjust its offset. */
> > + last_observe_offset -= (unsigned char *)
> > nl_attr_get(last_action) -
> > + (unsigned char *) last_action;
> > + ctx->xout->last_observe_offset = last_observe_offset;
> > + }
> > +
> > nl_msg_reset_size(actions,
> > (unsigned char *) last_action -
> > (unsigned char *) actions->data);
> > @@ -8097,6 +8124,16 @@ xlate_optimize_odp_actions(struct xlate_in *xin)
> > dest = nl_msg_put_uninit(actions, nl_attr_get_size(last_action));
> > memmove(dest, nl_attr_get(last_action),
> > nl_attr_get_size(last_action));
> > }
> > +
> > + /* If the last action of the list is an observability action, add an
> > + * explicit drop action so that drop statistics remain reliable. */
> > + if (ctx->xbridge->ofproto->explicit_sampled_drops &&
> > + ovs_explicit_drop_action_supported(ctx->xbridge->ofproto) &&
> > + last_observe_offset != UINT32_MAX &&
> > + (unsigned char *) last_action == (unsigned char *) actions->data +
> > + last_observe_offset) {
> > + put_drop_action(actions, XLATE_OK);
> > + }
> > }
> >
> > /* Translates the flow, actions, or rule in 'xin' into datapath actions in
> > @@ -8113,6 +8150,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
> > *xout)
> > *xout = (struct xlate_out) {
> > .slow = 0,
> > .recircs = RECIRC_REFS_EMPTY_INITIALIZER,
> > + .last_observe_offset = UINT32_MAX,
> > };
> >
> > struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp);
> > @@ -8541,17 +8579,15 @@ exit:
> > xout->slow = 0;
> > if (xin->odp_actions) {
> > ofpbuf_clear(xin->odp_actions);
> > + /* Make the drop explicit if the datapath supports it. */
> > + if (ovs_explicit_drop_action_supported(ctx.xbridge->ofproto)) {
> > + put_drop_action(xin->odp_actions, ctx.error);
> > + }
> > }
> > } else {
> > - /* In the non-error case, see if we can further optimize the
> > datapath
> > - * rules by removing redundant (clone) actions. */
> > - xlate_optimize_odp_actions(xin);
> > - }
> > -
> > - /* Install drop action if datapath supports explicit drop action. */
> > - if (xin->odp_actions && !xin->odp_actions->size &&
> > - ovs_explicit_drop_action_supported(ctx.xbridge->ofproto)) {
> > - put_drop_action(xin->odp_actions, ctx.error);
> > + /* In the non-error case, see if we can further optimize or tweak
> > + * datapath actions. */
> > + xlate_tweak_odp_actions(&ctx);
> > }
> >
> > /* Since congestion drop and forwarding drop are not exactly
> > diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
> > index 08f9397d8..d973a634a 100644
> > --- a/ofproto/ofproto-dpif-xlate.h
> > +++ b/ofproto/ofproto-dpif-xlate.h
> > @@ -61,6 +61,10 @@ struct xlate_out {
> >
> > /* Recirc action IDs on which references are held. */
> > struct recirc_refs recircs;
> > +
> > + /* Keep track of the last action whose purpose is purely observational.
> > + * e.g: IPFIX, sFlow, local sampling. */
> > + uint32_t last_observe_offset;
> > };
> >
> > struct xlate_in {
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 21deb7788..9e46f31e2 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -1820,6 +1820,7 @@ construct(struct ofproto *ofproto_)
> > ofproto->change_seq = 0;
> > ofproto->ams_seq = seq_create();
> > ofproto->ams_seqno = seq_read(ofproto->ams_seq);
> > + ofproto->explicit_sampled_drops = false;
> >
> >
> > SHASH_FOR_EACH_SAFE (node, &init_ofp_ports) {
> > @@ -2092,6 +2093,11 @@ run(struct ofproto *ofproto_)
> > }
> > }
> > }
> > +
> > + if (ofproto->explicit_sampled_drops != ofproto_explicit_sampled_drops)
> > {
> > + ofproto->explicit_sampled_drops = ofproto_explicit_sampled_drops;
> > + ofproto->backer->need_revalidate = REV_RECONFIGURE;
> > + }
> > return 0;
> > }
> >
> > diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> > index d7b7d5f8c..9bb82e831 100644
> > --- a/ofproto/ofproto-dpif.h
> > +++ b/ofproto/ofproto-dpif.h
> > @@ -365,6 +365,8 @@ struct ofproto_dpif {
> >
> > bool is_controller_connected; /* True if any controller admitted this
> > * switch connection. */
> > + bool explicit_sampled_drops; /* If explicit drop actions must added
> > after
> > + * trailing sample actions. */
> > };
> >
> > struct ofproto_dpif *ofproto_dpif_lookup_by_name(const char *name);
> > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> > index 85991554c..cce90066b 100644
> > --- a/ofproto/ofproto-provider.h
> > +++ b/ofproto/ofproto-provider.h
> > @@ -550,6 +550,10 @@ extern unsigned ofproto_offloaded_stats_delay;
> > * ofproto-dpif implementation. */
> > extern uint32_t n_handlers, n_revalidators;
> >
> > +/* If an explicit datapath drop action shall be added after trailing sample
> > + * actions coming from IPFIX / sFlow / local sampling. */
> > +extern bool ofproto_explicit_sampled_drops;
> > +
> > static inline struct rule *rule_from_cls_rule(const struct cls_rule *);
> >
> > void ofproto_rule_expire(struct rule *rule, uint8_t reason)
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index 8c1efe4bf..6dfd94f3e 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -312,6 +312,7 @@ unsigned ofproto_max_idle = OFPROTO_MAX_IDLE_DEFAULT;
> > unsigned ofproto_max_revalidator = OFPROTO_MAX_REVALIDATOR_DEFAULT;
> > unsigned ofproto_min_revalidate_pps = OFPROTO_MIN_REVALIDATE_PPS_DEFAULT;
> > unsigned ofproto_offloaded_stats_delay = OFPROTO_OFFLOADED_STATS_DELAY;
> > +bool ofproto_explicit_sampled_drops =
> > OFPROTO_EXPLICIT_SAMPLED_DROPS_DEFAULT;
> >
> > uint32_t n_handlers, n_revalidators;
> >
> > @@ -737,6 +738,15 @@ ofproto_set_offloaded_stats_delay(unsigned
> > offloaded_stats_delay)
> > ofproto_offloaded_stats_delay = offloaded_stats_delay;
> > }
> >
> > +/* Set if an explicit datapath drop action shall be added after trailing
> > sample
> > + * actions coming from IPFIX / sFlow / local sampling. */
> > +void
> > +ofproto_set_explicit_sampled_drops(bool explicit_sampled_drops)
> > +{
> > + ofproto_explicit_sampled_drops = explicit_sampled_drops;
> > +}
> > +
> > +
> > /* If forward_bpdu is true, the NORMAL action will forward frames with
> > * reserved (e.g. STP) destination Ethernet addresses. if forward_bpdu is
> > false,
> > * the NORMAL action will drop these frames. */
> > diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> > index f1ff80e52..fcf8e201d 100644
> > --- a/ofproto/ofproto.h
> > +++ b/ofproto/ofproto.h
> > @@ -326,6 +326,7 @@ int ofproto_port_dump_done(struct ofproto_port_dump *);
> > #define OFPROTO_MAX_REVALIDATOR_DEFAULT 500 /* ms */
> > #define OFPROTO_MIN_REVALIDATE_PPS_DEFAULT 5
> > #define OFPROTO_OFFLOADED_STATS_DELAY 2000 /* ms */
> > +#define OFPROTO_EXPLICIT_SAMPLED_DROPS_DEFAULT false
> >
> > const char *ofproto_port_open_type(const struct ofproto *,
> > const char *port_type);
> > @@ -398,6 +399,7 @@ void ofproto_ct_zone_limit_protection_update(const char
> > *datapath_type,
> > bool protected);
> > void ofproto_get_datapath_cap(const char *datapath_type,
> > struct smap *dp_cap);
> > +void ofproto_set_explicit_sampled_drops(bool explicit_sampled_drops);
> >
> > /* Configuration of ports. */
> > void ofproto_port_unregister(struct ofproto *, ofp_port_t ofp_port);
> > diff --git a/tests/drop-stats.at b/tests/drop-stats.at
> > index 1d3af98da..19e4d67a1 100644
> > --- a/tests/drop-stats.at
> > +++ b/tests/drop-stats.at
> > @@ -191,3 +191,176 @@ ovs-appctl coverage/read-counter
> > drop_action_too_many_mpls_labels
> >
> > OVS_VSWITCHD_STOP(["/|WARN|/d"])
> > AT_CLEANUP
> > +
> > +AT_SETUP([drop-stats - bridge sampling])
> > +
> > +OVS_VSWITCHD_START([dnl
> > + set bridge br0 datapath_type=dummy \
> > + protocols=OpenFlow10,OpenFlow13,OpenFlow14,OpenFlow15 -- \
> > + add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1])
> > +
> > +AT_DATA([flows.txt], [dnl
> > +table=0,in_port=1,actions=drop
> > +])
> > +
> > +AT_CHECK([
> > + ovs-ofctl del-flows br0
> > + ovs-ofctl -Oopenflow13 add-flows br0 flows.txt
> > + ovs-ofctl -Oopenflow13 dump-flows br0 | ofctl_strip | sort | grep
> > actions ], [0], [dnl
> > + in_port=1 actions=drop
> > +])
>
> Don't put multiple things into one AT_CHECK.
>
> > +
> > +AT_CHECK([ovs-vsctl -- set bridge br0 ipfix=@fix -- \
> > + --id=@fix create ipfix targets=\"127.0.0.1:4739\" \
> > + sampling=1],
> > + [0], [ignore])
> > +
> > +for i in $(seq 1 3); do
> > +AT_CHECK([
> > + ovs-appctl netdev-dummy/receive p1 \
> > +
> > 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> > +], [0], [ignore])
>
> Maybe deine this packet as a macro with a join?
>
> > +done
> > +
> > +AT_CHECK([ovs-appctl dpctl/dump-flows | sed
> > 's/used:[[0-9]].[[0-9]]*s/used:0.0/' | sort], [0], [flow-dump from the main
> > thread:
>
> maybe break the line with dnl. Also there is strip_used macro.
>
> > +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no),
> > packets:2, bytes:212, used:0.0,
> > actions:userspace(pid=0,ipfix(output_port=4294967295))
> > +])
> > +
> > +AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
> > +
> > +AT_CHECK([ovs-appctl coverage/read-counter drop_action_of_pipeline], [0],
> > [dnl
> > +0
> > +])
> > +
> > +dnl Now activate explicit sampled drops.
> > +AT_CHECK([ovs-vsctl set Open_vSwitch .
> > other_config:explicit-sampled-drops=true])
> > +AT_CHECK([ovs-appctl revalidator/purge])
>
> Can we wait instead of purge?
>
> > +
> > +for i in $(seq 1 3); do
> > +AT_CHECK([
> > + ovs-appctl netdev-dummy/receive p1 \
> > +
> > 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> > +], [0], [ignore])
> > +done
> > +
> > +AT_CHECK([ovs-appctl dpctl/dump-flows | sed
> > 's/used:[[0-9]].[[0-9]]*s/used:0.0/' | sort], [0], [flow-dump from the main
> > thread:
> > +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no),
> > packets:2, bytes:212, used:0.0,
> > actions:userspace(pid=0,ipfix(output_port=4294967295)),drop
> > +])
>
> Same here.
>
> > +
> > +AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
> > +
> > +AT_CHECK([ovs-appctl coverage/read-counter drop_action_of_pipeline], [0],
> > [dnl
> > +3
> > +])
> > +
> > +OVS_VSWITCHD_STOP(["/sending to collector failed/d"])
> > +AT_CLEANUP
> > +
> > +AT_SETUP([drop-stats - sampling action])
>
> Same for this test as well.
>
> > +
> > +OVS_VSWITCHD_START([dnl
> > + set bridge br0 datapath_type=dummy \
> > + protocols=OpenFlow10,OpenFlow13,OpenFlow14,OpenFlow15 -- \
> > + add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1 -- \
> > + add-port br0 p2 -- set Interface p2 type=dummy ofport_request=2 -- \
> > + add-port br0 p3 -- set Interface p3 type=dummy ofport_request=3])
>
> add_of_br br0 1 2 3
>
> > +
> > +AT_DATA([flows.txt], [dnl
> > +table=0,in_port=1,actions=sample(probability=65535,collector_set_id=1)
> > +table=0,in_port=2,actions=sample(probability=32767,collector_set_id=1),load:0->reg0
> > +table=0,in_port=3,actions=clone(sample(probability=65535,collector_set_id=1))
> > +])
> > +
> > +AT_CHECK([
> > + ovs-ofctl del-flows br0
> > + ovs-ofctl -Oopenflow13 add-flows br0 flows.txt
> > +])
> > +
> > +AT_CHECK([ovs-vsctl --id=@br0 get Bridge br0 \
> > + -- --id=@ipfix create IPFIX targets=\"127.0.0.1:4739\"
> > \
> > + -- create Flow_Sample_Collector_Set id=1 bridge=@br0 \
> > + ipfix=@ipfix],
> > + [0], [ignore])
> > +
> > +for i in $(seq 1 3); do
> > +AT_CHECK([
> > + ovs-appctl netdev-dummy/receive p1 \
> > +
> > 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> > +], [0], [ignore])
> > +done
> > +
> > +AT_CHECK([ovs-appctl dpctl/dump-flows | sed
> > 's/used:[[0-9]].[[0-9]]*s/used:0.0/' | sort], [0], [flow-dump from the main
> > thread:
> > +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no),
> > packets:2, bytes:212, used:0.0, dnl
> > +actions:userspace(pid=0,flow_sample(probability=65535,collector_set_id=1,obs_domain_id=0,obs_point_id=0,output_port=4294967295))
> > +])
> > +
> > +AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
> > +
> > +AT_CHECK([ovs-appctl coverage/read-counter drop_action_of_pipeline], [0],
> > [dnl
> > +0
> > +])
> > +
> > +dnl Now activate explicit sampled drops.
> > +AT_CHECK([ovs-vsctl set Open_vSwitch .
> > other_config:explicit-sampled-drops=true])
> > +AT_CHECK([ovs-appctl revalidator/purge])
>
> wait?
>
> > +
> > +for i in $(seq 1 3); do
> > +AT_CHECK([
> > + ovs-appctl netdev-dummy/receive p1 \
> > +
> > 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> > +], [0], [ignore])
> > +done
> > +
> > +AT_CHECK([ovs-appctl dpctl/dump-flows | sed
> > 's/used:[[0-9]].[[0-9]]*s/used:0.0/' | sort], [0], [flow-dump from the main
> > thread:
> > +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no),
> > packets:2, bytes:212, used:0.0, dnl
> > +actions:userspace(pid=0,flow_sample(probability=65535,collector_set_id=1,obs_domain_id=0,obs_point_id=0,output_port=4294967295)),drop
> > +])
> > +
> > +AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
> > +
> > +AT_CHECK([ovs-appctl coverage/read-counter drop_action_of_pipeline], [0],
> > [dnl
> > +3
> > +])
> > +
> > +AT_CHECK([ovs-appctl dpctl/del-flows])
> > +
> > +for i in $(seq 1 3); do
> > +AT_CHECK([
> > + ovs-appctl netdev-dummy/receive p2 \
> > +
> > 'in_port(2),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> > +], [0], [ignore])
> > +done
> > +
> > +AT_CHECK([ovs-appctl dpctl/dump-flows | sed
> > 's/used:[[0-9]].[[0-9]]*s/used:0.0/' | sort], [0], [flow-dump from the main
> > thread:
> > +recirc_id(0),in_port(2),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no),
> > packets:2, bytes:212, used:0.0, dnl
> > +actions:sample(sample=50.0%,actions(userspace(pid=0,flow_sample(probability=32767,collector_set_id=1,obs_domain_id=0,obs_point_id=0,output_port=4294967295)))),drop
> > +])
> > +
> > +AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
> > +
> > +AT_CHECK([ovs-appctl coverage/read-counter drop_action_of_pipeline], [0],
> > [dnl
> > +6
> > +])
> > +
> > +AT_CHECK([ovs-appctl dpctl/del-flows])
> > +
> > +for i in $(seq 1 3); do
> > +AT_CHECK([
> > + ovs-appctl netdev-dummy/receive p3 \
> > +
> > 'in_port(3),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> > +], [0], [ignore])
> > +done
> > +
> > +AT_CHECK([ovs-appctl dpctl/dump-flows | sed
> > 's/used:[[0-9]].[[0-9]]*s/used:0.0/' | sort], [0], [flow-dump from the main
> > thread:
> > +recirc_id(0),in_port(3),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no),
> > packets:2, bytes:212, used:0.0, dnl
> > +actions:userspace(pid=0,flow_sample(probability=65535,collector_set_id=1,obs_domain_id=0,obs_point_id=0,output_port=4294967295)),drop
> > +])
> > +
> > +AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
> > +
> > +AT_CHECK([ovs-appctl coverage/read-counter drop_action_of_pipeline], [0],
> > [dnl
> > +9
> > +])
>
> Why we need to do the same thing 3 times?
>
Flows are:
> > +table=0,in_port=1,actions=sample(probability=65535,collector_set_id=1)
> > +table=0,in_port=2,actions=sample(probability=32767,collector_set_id=1),load:0->reg0
> > +table=0,in_port=3,actions=clone(sample(probability=65535,collector_set_id=1))
We send packets on ports 1, 2 and 3 testing:
- "Normal" case.
- Having OpenFlow flows (registry restoration) following the sample.
- Having the sample inside a clone.
Flows look the same but we are testing 3 different things.
> > +
> > +OVS_VSWITCHD_STOP(["/sending to collector failed/d"])
> > +AT_CLEANUP
> > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> > index 5778e9fd3..f65ace850 100644
> > --- a/tests/ofproto-dpif.at
> > +++ b/tests/ofproto-dpif.at
> > @@ -12335,3 +12335,52 @@ Datapath actions: EXPECTED_ACT
> >
> > OVS_VSWITCHD_STOP("/Enabling an unsupported feature is very dangerous/d")
> > AT_CLEANUP
> > +
> > +AT_SETUP([ofproto-dpif - Local sampling - drop])
> > +OVS_VSWITCHD_START
> > +add_of_ports br0 1 2
> > +
> > +AT_CHECK([ovs-appctl dpif/set-dp-features --force br0 psample true], [0],
> > [ignore])
> > +
> > +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> > + -- create Flow_Sample_Collector_Set id=1 bridge=@br0
> > local_group_id=42],
> > + [0], [ignore])
> > +
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=slowpath pktps
> > stats bands=type=drop rate=2'])
> > +
> > +AT_DATA([flows.txt], [dnl
> > +in_port=1,
> > actions=sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200)
> > +in_port=2,
> > actions=sample(probability=65535,collector_set_id=1,obs_domain_id=100,obs_point_id=200)
> > +])
> > +
> > +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
> > +
> > +m4_define([TRACE_PKT], [m4_join([,],
> > + [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
> > + [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
> > + [icmp(type=8,code=0)])])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1) TRACE_PKT'], [0],
> > [stdout])
> > +AT_CHECK([tail -1 stdout], [0], [dnl
> > +Datapath actions:
> > sample(sample=50.0%,actions(psample(group=42,cookie=0x64000000c8)))
> > +])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(2) TRACE_PKT'], [0],
> > [stdout])
> > +AT_CHECK([tail -1 stdout], [0], [dnl
> > +Datapath actions: psample(group=42,cookie=0x64000000c8)
> > +])
> > +
> > +AT_CHECK([ovs-vsctl set Open_vSwitch .
> > other_config:explicit-sampled-drops=true])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1) TRACE_PKT'], [0],
> > [stdout])
> > +AT_CHECK([tail -1 stdout], [0], [dnl
> > +Datapath actions:
> > sample(sample=50.0%,actions(psample(group=42,cookie=0x64000000c8))),drop
> > +])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(2) TRACE_PKT'], [0],
> > [stdout])
> > +AT_CHECK([tail -1 stdout], [0], [dnl
> > +Datapath actions: psample(group=42,cookie=0x64000000c8),drop
> > +])
> > +
> > +OVS_VSWITCHD_STOP("/Enabling an unsupported feature is very dangerous/d")
> > +AT_CLEANUP
> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index c5399d18c..aa1de7d47 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -889,7 +889,9 @@ bridge_reconfigure(const struct ovsrec_open_vswitch
> > *ovs_cfg)
> > ofproto_set_threads(
> > smap_get_int(&ovs_cfg->other_config, "n-handler-threads", 0),
> > smap_get_int(&ovs_cfg->other_config, "n-revalidator-threads", 0));
> > -
> > + ofproto_set_explicit_sampled_drops(
> > + smap_get_bool(&ovs_cfg->other_config, "explicit-sampled-drops",
> > + OFPROTO_EXPLICIT_SAMPLED_DROPS_DEFAULT));
> > /* Destroy "struct bridge"s, "struct port"s, and "struct iface"s
> > according
> > * to 'ovs_cfg', with only very minimal configuration otherwise.
> > *
> > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> > index 56ac961fc..793d1d755 100644
> > --- a/vswitchd/vswitch.xml
> > +++ b/vswitchd/vswitch.xml
> > @@ -869,6 +869,30 @@
> > The feature is considered experimental.
> > </p>
> > </column>
> > +
> > + <column name="other_config" key="explicit-sampled-drops"
> > + type='{"type": "boolean"}'>
> > + <p>
> > + When a flow is installed in the datapath with an empty action
> > list,
> > + it indicates an implicit "drop" action. Most datapaths report this
>
> Double the space.
>
> > + for event for statistics and monitoring (in datapath-specific
> > ways).
> > + </p>
> > + <p>
> > + However, if any of the per-bridge or per-flow sampling
> > functionalities
> > + are enabled (e.g: sFlow, IPFIX, local sampling), the action list
> > might
> > + not be empty, but contain an action to implement such
> > functionality.
> > + This makes the datapaths not report the packet drop.
> > + </p>
> > + <p>
> > + This knob makes Open vSwitch detect when the last datapath action
> > + comes from these sampling features and add an explicit drop
> > action at
> > + the end to keep drop statistics accurate.
> > + </p>
> > + <p>
> > + The default value is <code>false</code>.
> > + </p>
> > + </column>
> > +
> > </group>
> > <group title="Status">
> > <column name="next_cfg">
> > @@ -904,7 +928,7 @@
> > case when statistics gathering is not useful. Set this value to
> > <code>true</code> to enable populating the <ref
> > column="statistics"/>
> > column or to <code>false</code> to explicitly disable it.
> > - </column>
> > + </column>
> >
>
> Seems unrelated.
>
> > <column name="statistics" key="cpu"
> > type='{"type": "integer", "minInteger": 1}'>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
