On Fri, Oct 13, 2023 at 9:06 AM Ilya Maximets <i.maxim...@ovn.org> wrote:
>
> On 10/8/23 06:21, Mike Pattrick wrote:
> > Currently a bridge mirror will collect all packets and tools like
> > ovs-tcpdump can apply additional filters after they have already been
> > duplicated by vswitchd. This can result in inefficient collection.
> >
> > This patch adds support to apply pre-selection to bridge mirrors, which
> > can limit which packets are mirrored based on flow metadata. This
> > significantly improves overall vswitchd performance during mirroring if
> > only a subset of traffic is required.
> >
> > I benchmarked this with two setups. A netlink based test with two veth
> > pairs connected to a single bridge, and a netdev based test involving a
> > mix of DPDK nics, and netdev-linux interfaces. Both tests involved
> > saturating the link with iperf3 and then sending an icmp ping every
> > second. I then measured the throughput on the link with no mirroring,
> > icmp pre-selected mirroring, and full mirroring. The results, below,
> > indicate a significant reduction to impact of mirroring when only a
> > subset of the traffic on a port is selected for collection.
> >
> > Test No Mirror | No Filter | Filter | No Filter | Filter |
> > +-----------+-----------+-----------+-----------+----------+
> > netlink | 39.0 Gbps | 36.1 Gbps | 38.2 Gbps | 7% | 2% |
> > netdev | 7.39 Gbps | 4.95 Gbps | 6.24 Gbps | 33% | 15% |
> >
> > The ratios above are the percent reduction in total throughput when
> > mirroring is used either with or without a filter.
> >
> > Signed-off-by: Mike Pattrick <m...@redhat.com>
> >
>
> Hi, Mike. Thanks for the patch!
>
> Not a full review, but see some comments inline.
>
> Best regards, Ilya Maximets.
>
> > ---
> > v3:
> > - Added more tests
> > - Refactored mirror wildcard modification out of mirror_get
> > - Improved error handling
> > v4:
> > - Refactored mirror_set and mirror_get functions to reduce function
> > parameters
> > v5:
> > - Fixed formating issues
> > - Moved some code from patch 2 to patch 1 to fix compile
> > v6:
> > - Clarified filter format in documentation
> > - Moved flow/mask operations to atomic set/get
> > - Cleaned up and improved test
> >
> > ---
> > Documentation/ref/ovs-tcpdump.8.rst | 8 +-
> > NEWS | 4 +
> > lib/flow.h | 9 +++
> > ofproto/ofproto-dpif-mirror.c | 68 +++++++++++++++-
> > ofproto/ofproto-dpif-mirror.h | 8 +-
> > ofproto/ofproto-dpif-xlate.c | 16 +++-
> > ofproto/ofproto-dpif.c | 9 +--
> > ofproto/ofproto-dpif.h | 6 ++
> > ofproto/ofproto.h | 3 +
> > tests/ofproto-dpif.at | 116 ++++++++++++++++++++++++++++
> > utilities/ovs-tcpdump.in | 13 +++-
> > vswitchd/bridge.c | 13 +++-
> > vswitchd/vswitch.ovsschema | 5 +-
> > vswitchd/vswitch.xml | 13 ++++
> > 14 files changed, 273 insertions(+), 18 deletions(-)
> >
> > diff --git a/Documentation/ref/ovs-tcpdump.8.rst
> > b/Documentation/ref/ovs-tcpdump.8.rst
> > index b9f8cdf6f..e21e61211 100644
> > --- a/Documentation/ref/ovs-tcpdump.8.rst
> > +++ b/Documentation/ref/ovs-tcpdump.8.rst
> > @@ -61,8 +61,14 @@ Options
> >
> > If specified, mirror all ports (optional).
> >
> > +* ``--filter <flow>``
> > +
> > + If specified, only mirror flows that match the provided OpenFlow filter.
> > + The available fields are documented in ``ovs-fields(7)``.
> > +
> > See Also
> > ========
> >
> > ``ovs-appctl(8)``, ``ovs-vswitchd(8)``, ``ovs-pcap(1)``,
> > -``ovs-tcpundump(1)``, ``tcpdump(8)``, ``wireshark(8)``.
> > +``ovs-fields(7)``, ``ovs-tcpundump(1)``, ``tcpdump(8)``,
> > +``wireshark(8)``.
> > diff --git a/NEWS b/NEWS
> > index 6b45492f1..e95033b50 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -6,6 +6,10 @@ Post-v3.2.0
> > from older version is supported but it may trigger more leader
> > elections
> > during the process, and error logs complaining unrecognized fields
> > may
> > be observed on old nodes.
> > + * Added a new filter column in the Mirror table which can be used to
> > + apply filters to mirror ports.
> > + - ovs-tcpdump:
> > + * Added new --filter parameter to apply pre-selection on mirrored
> > flows.
> >
> >
> > v3.2.0 - 17 Aug 2023
> > diff --git a/lib/flow.h b/lib/flow.h
> > index a9d026e1c..f77d3553a 100644
> > --- a/lib/flow.h
> > +++ b/lib/flow.h
> > @@ -939,6 +939,15 @@ flow_union_with_miniflow(struct flow *dst, const
> > struct miniflow *src)
> > flow_union_with_miniflow_subset(dst, src, src->map);
> > }
> >
> > +/* Perform a bitwise OR of minimask 'src' mask data with the equivalent
> > + * fields in 'dst', storing the result in 'dst'. */
> > +static inline void
> > +flow_wildcards_union_with_minimask(struct flow_wildcards *dst,
> > + const struct minimask *src)
> > +{
> > + flow_union_with_miniflow_subset(&dst->masks, &src->masks,
> > src->masks.map);
> > +}
> > +
> > static inline bool is_ct_valid(const struct flow *flow,
> > const struct flow_wildcards *mask,
> > struct flow_wildcards *wc)
> > diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c
> > index 17ba6f799..792d3930c 100644
> > --- a/ofproto/ofproto-dpif-mirror.c
> > +++ b/ofproto/ofproto-dpif-mirror.c
> > @@ -21,6 +21,7 @@
> > #include "cmap.h"
> > #include "hmapx.h"
> > #include "ofproto.h"
> > +#include "ofproto-dpif-trace.h"
> > #include "vlan-bitmap.h"
> > #include "openvswitch/vlog.h"
> >
> > @@ -57,6 +58,11 @@ struct mirror {
> > struct hmapx srcs; /* Contains "struct mbundle*"s. */
> > struct hmapx dsts; /* Contains "struct mbundle*"s. */
> >
> > + /* Filter criteria. */
> > + OVSRCU_TYPE(struct miniflow *) filter;
> > + OVSRCU_TYPE(struct minimask *) mask;
> > + char *filter_str;
> > +
> > /* This is accessed by handler threads assuming RCU protection (see
> > * mirror_get()), but can be manipulated by mirror_set() without any
> > * explicit synchronization. */
> > @@ -207,11 +213,13 @@ mirror_bundle_dst(struct mbridge *mbridge, struct
> > ofbundle *ofbundle)
> > }
> >
> > int
> > -mirror_set(struct mbridge *mbridge, void *aux,
> > +mirror_set(const struct ofproto *ofproto_, void *aux,
> > const struct ofproto_mirror_settings *ms, struct ofbundle
> > **srcs,
> > struct ofbundle **dsts, struct ofbundle *bundle)
> >
> > {
> > + const struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
>
> This looks like a layering violation. Can we pass mbridge here instead?
>
> > + struct mbridge *mbridge = ofproto->mbridge;
> > struct mbundle *mbundle, *out;
> > mirror_mask_t mirror_bit;
> > struct mirror *mirror;
> > @@ -265,7 +273,8 @@ mirror_set(struct mbridge *mbridge, void *aux,
> > && vlan_bitmap_equal(vlans, ms->src_vlans)
> > && mirror->out == out
> > && mirror->out_vlan == out_vlan
> > - && mirror->snaplen == ms->snaplen)
> > + && mirror->snaplen == ms->snaplen
> > + && nullable_string_is_equal(mirror->filter_str, ms->filter))
> > {
> > hmapx_destroy(&srcs_map);
> > hmapx_destroy(&dsts_map);
> > @@ -289,6 +298,44 @@ mirror_set(struct mbridge *mbridge, void *aux,
> > mirror->out_vlan = out_vlan;
> > mirror->snaplen = ms->snaplen;
> >
> > + if (!nullable_string_is_equal(mirror->filter_str, ms->filter)) {
>
> THis check might not be enough. By allowing port names in the filter
> there is a chance that ports will change their numbers without changing
> their names. In this case the parsed filter flow should be invalidated.
Thanks for the review Ilya, I was going through these comments and I
noticed some discrepancy with how flows are handled when port number
changes. With the sample flow rule "in_port=p1 actions=output:p2" and
after changing the port number with "ovs-vsctl set int p1
ofport_request=8", I see that the flow is not invalidated or updated,
and instead retains the old now invalid port number: "in_port=1
actions=output:p2". Is this the intended behaviour?
I think it makes some amount of sense for this filter to act in a
similar fashion to flows.
Thanks,
Mike
>
> > + if (mirror->filter_str) {
> > + ovsrcu_postpone(free, ovsrcu_get_protected(struct miniflow *,
> > + &mirror->filter));
> > + ovsrcu_postpone(free, ovsrcu_get_protected(struct minimask *,
> > + &mirror->mask));
> > + free(mirror->filter_str);
> > + mirror->filter_str = NULL;
> > + ovsrcu_set(&mirror->filter, NULL);
> > + ovsrcu_set(&mirror->mask, NULL);
> > + }
> > +
> > + if (ms->filter && strlen(ms->filter)) {
> > + struct ofputil_port_map map =
> > OFPUTIL_PORT_MAP_INITIALIZER(&map);
> > + struct flow_wildcards wc;
> > + struct flow flow;
> > + char *err;
> > +
> > + ofproto_append_ports_to_map(&map, ofproto_->ports);
> > + err = parse_ofp_exact_flow(&flow, &wc,
> > + ofproto_get_tun_tab(ofproto_),
> > + ms->filter, &map);
> > + ofputil_port_map_destroy(&map);
> > + if (err) {
> > + VLOG_WARN("filter is invalid: %s", err);
> > + free(err);
> > + mirror_destroy(mbridge, mirror->aux);
> > + return EINVAL;
> > + }
> > +
> > + mirror->filter_str = xstrdup(ms->filter);
> > + ovsrcu_set(&mirror->mask, minimask_create(&wc));
> > + ovsrcu_set(&mirror->filter, miniflow_create(&flow));
> > + }
> > + }
> > +
> > +
> > +
>
> Too many empty lines.
>
> > /* Update mbundles. */
> > mirror_bit = MIRROR_MASK_C(1) << mirror->idx;
> > CMAP_FOR_EACH (mbundle, cmap_node, &mirror->mbridge->mbundles) {
> > @@ -344,6 +391,17 @@ mirror_destroy(struct mbridge *mbridge, void *aux)
> > ovsrcu_postpone(free, vlans);
> > }
> >
> > + if (mirror->filter_str) {
> > + ovsrcu_postpone(free, ovsrcu_get_protected(struct miniflow *,
> > + &mirror->filter));
> > + ovsrcu_postpone(free, ovsrcu_get_protected(struct minimask *,
> > + &mirror->mask));
> > + free(mirror->filter_str);
> > + mirror->filter_str = NULL;
> > + ovsrcu_set(&mirror->filter, NULL);
> > + ovsrcu_set(&mirror->mask, NULL);
> > + }
> > +
> > mbridge->mirrors[mirror->idx] = NULL;
> > /* mirror_get() might have just read the pointer, so we must postpone
> > the
> > * free. */
> > @@ -415,7 +473,9 @@ mirror_update_stats(struct mbridge *mbridge,
> > mirror_mask_t mirrors,
> > * in which a 1-bit indicates that the mirror includes a particular VLAN,
> > * 'mc->dup_mirrors' receives a bitmap of mirrors whose output duplicates
> > * mirror 'index', 'mc->out' receives the output ofbundle (if any),
> > - * and 'mc->out_vlan' receives the output VLAN (if any).
> > + * and 'mc->out_vlan' receives the output VLAN (if any). In cases where the
> > + * mirror has a filter configured 'mc->filter_flow' and 'mc->filter_mask'
> > + * receives the flow and mask that this mirror should collect.
> > *
> > * Everything returned here is assumed to be RCU protected.
> > */
> > @@ -441,6 +501,8 @@ mirror_get(struct mbridge *mbridge, int index,
> > mc->bundle = mirror->out ? mirror->out->ofbundle : NULL;
> > mc->out_vlan = mirror->out_vlan;
> > mc->snaplen = mirror->snaplen;
> > + mc->filter_flow = ovsrcu_get(struct miniflow *, &mirror->filter);
> > + mc->filter_mask = ovsrcu_get(struct minimask *, &mirror->mask);
> > return true;
> > }
> >
> > diff --git a/ofproto/ofproto-dpif-mirror.h b/ofproto/ofproto-dpif-mirror.h
> > index 9d5a6d680..faf92383e 100644
> > --- a/ofproto/ofproto-dpif-mirror.h
> > +++ b/ofproto/ofproto-dpif-mirror.h
> > @@ -25,6 +25,7 @@ typedef uint32_t mirror_mask_t;
> > struct ofproto_mirror_settings;
> > struct ofproto_dpif;
> > struct ofbundle;
> > +struct ofproto;
> >
> > struct mirror_config {
> > /* A bitmap of mirrors that duplicate the current mirror */
> > @@ -33,6 +34,11 @@ struct mirror_config {
> > /* VLANs of packets to select for mirroring. */
> > unsigned long *vlans; /* vlan_bitmap, NULL selects all VLANs. */
> >
> > + /* The flow if a filter is used, or NULL. */
> > + struct miniflow *filter_flow;
> > + /* The filter's flow mask, or NULL. */
> > + struct minimask *filter_mask;
> > +
> > /* Output (mutually exclusive). */
> > struct ofbundle *bundle; /* A registered ofbundle handle or NULL. */
> > uint16_t out_vlan; /* Output VLAN, only if out_bundle is
> > NULL. */
> > @@ -66,7 +72,7 @@ bool mbridge_need_revalidate(struct mbridge *);
> > void mbridge_register_bundle(struct mbridge *, struct ofbundle *);
> > void mbridge_unregister_bundle(struct mbridge *, struct ofbundle *);
> >
> > -int mirror_set(struct mbridge *mbridge, void *aux,
> > +int mirror_set(const struct ofproto *ofproto, void *aux,
> > const struct ofproto_mirror_settings *ms,
> > struct ofbundle **srcs, struct ofbundle **dsts,
> > struct ofbundle *bundle);
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index 3746edf06..2ea4a0c6a 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -2218,7 +2218,8 @@ lookup_input_bundle(const struct xlate_ctx *ctx,
> >
> > /* Mirrors the packet represented by 'ctx' to appropriate mirror
> > destinations,
> > * given the packet is ingressing or egressing on 'xbundle', which has
> > ingress
> > - * or egress (as appropriate) mirrors 'mirrors'. */
> > + * or egress (as appropriate) mirrors 'mirrors'. In cases where a mirror is
> > + * filtered, the current flows wildcard will be modified. */
> > static void
> > mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
> > mirror_mask_t mirrors)
> > @@ -2269,6 +2270,19 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle
> > *xbundle,
> > continue;
> > }
> >
> > + /* After the VLAN check, apply a flow mask if a filter is
> > specified */
>
> Period at the end of a comment.
>
> > + if (ctx->wc && mc.filter_flow) {
> > + if (OVS_UNLIKELY(
> > + miniflow_equal_flow_in_minimask(mc.filter_flow,
> > + &ctx->xin->flow,
> > + mc.filter_mask))) {
> > + flow_wildcards_union_with_minimask(ctx->wc,
> > mc.filter_mask);
>
> This doesn't look right. The fact that we're comparing the flow
> means we're using the fileds to make a decision. And everything that
> we use should become part of the mask. i.e. The mask should be
> updated before comparison.
>
> Otherwise, if the first packet doesn't match the filter, installed
> datapath flow will not have match on the filter fields, and subsequent
> packets that may match the filter will pass without being mirrired.
>
> A test for this case would be nice to have. Some tests with more
> cmplex filter would also be nice.
>
> This change will affect performance though significantly, because
> non-filtered traffic will start exact matching on fields present
> in the filter.
>
>
> > + } else {
> > + mirrors = zero_rightmost_1bit(mirrors);
> > + continue;
> > + }
> > + }
> > +
> > /* We sent a packet to this mirror. */
> > used_mirrors |= rightmost_1bit(mirrors);
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 5a5a8ea86..15cb948c8 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -221,13 +221,6 @@ static void ct_zone_config_init(struct dpif_backer
> > *backer);
> > static void ct_zone_config_uninit(struct dpif_backer *backer);
> > static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer);
> >
> > -static inline struct ofproto_dpif *
> > -ofproto_dpif_cast(const struct ofproto *ofproto)
> > -{
> > - ovs_assert(ofproto->ofproto_class == &ofproto_dpif_class);
> > - return CONTAINER_OF(ofproto, struct ofproto_dpif, up);
> > -}
> > -
> > /* Global variables. */
> > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> >
> > @@ -3663,7 +3656,7 @@ mirror_set__(struct ofproto *ofproto_, void *aux,
> > dsts[i] = bundle_lookup(ofproto, s->dsts[i]);
> > }
> >
> > - error = mirror_set(ofproto->mbridge, aux, s, srcs, dsts,
> > + error = mirror_set(ofproto_, aux, s, srcs, dsts,
> > bundle_lookup(ofproto, s->out_bundle));
> > free(srcs);
> > free(dsts);
> > diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> > index d8e0cd37a..9669f6827 100644
> > --- a/ofproto/ofproto-dpif.h
> > +++ b/ofproto/ofproto-dpif.h
> > @@ -357,6 +357,12 @@ struct ofproto_dpif {
> >
> > struct ofproto_dpif *ofproto_dpif_lookup_by_name(const char *name);
> > struct ofproto_dpif *ofproto_dpif_lookup_by_uuid(const struct uuid *uuid);
> > +static inline struct ofproto_dpif *
> > +ofproto_dpif_cast(const struct ofproto *ofproto)
> > +{
> > + ovs_assert(ofproto->ofproto_class == &ofproto_dpif_class);
> > + return CONTAINER_OF(ofproto, struct ofproto_dpif, up);
> > +}
> >
> > ovs_version_t ofproto_dpif_get_tables_version(struct ofproto_dpif *);
> >
> > diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> > index 8efdb20a0..372e0e3fc 100644
> > --- a/ofproto/ofproto.h
> > +++ b/ofproto/ofproto.h
> > @@ -497,6 +497,9 @@ struct ofproto_mirror_settings {
> > uint16_t out_vlan; /* Output VLAN, only if out_bundle is
> > NULL. */
> > uint16_t snaplen; /* Max packet size of a mirrored packet
> > in byte, set to 0 equals 65535. */
> > +
> > + /* Output filter */
>
> Period.
>
> > + char *filter;
> > };
> >
> > int ofproto_mirror_register(struct ofproto *, void *aux,
> > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> > index a39d0d3ae..f8d6ee52c 100644
> > --- a/tests/ofproto-dpif.at
> > +++ b/tests/ofproto-dpif.at
> > @@ -5182,6 +5182,122 @@ OVS_VSWITCHD_STOP
> > AT_CLEANUP
> >
> >
> > +AT_SETUP([ofproto-dpif - mirroring, filter])
> > +AT_KEYWORDS([mirror mirrors mirroring])
> > +OVS_VSWITCHD_START
> > +add_of_ports br0 1 2 3
> > +AT_CHECK([ovs-vsctl \
> > + set Bridge br0 mirrors=@m -- \
> > + --id=@p3 get Port p3 -- \
> > + --id=@m create Mirror name=mymirror select_all=true
> > output_port=@p3 filter="icmp"], [0], [ignore])
> > +
> > +AT_DATA([flows.txt], [dnl
> > +in_port=1 actions=output:2
> > +in_port=2 actions=output:1
> > +])
> > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> > +
> > +icmp_flow="eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
> > +tcp_flow="eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=128,frag=no),tcp()"
> > +
> > +dnl Check one direction, only icmp should mirror.
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$icmp_flow"],
> > [0], [stdout])
> > +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> > + [Datapath actions: 3,2
> > +])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$tcp_flow"], [0],
> > [stdout])
> > +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> > + [Datapath actions: 2
> > +])
> > +
> > +dnl Check other direction, only icmp should mirror.
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$icmp_flow"],
> > [0], [stdout])
> > +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> > + [Datapath actions: 3,1
> > +])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$tcp_flow"], [0],
> > [stdout])
> > +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> > + [Datapath actions: 1
> > +])
> > +
> > +dnl Change filter to tcp, only tcp should mirror.
> > +AT_CHECK([ovs-vsctl set mirror mymirror filter="tcp"], [0])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$icmp_flow"],
> > [0], [stdout])
> > +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> > + [Datapath actions: 2
> > +])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$tcp_flow"], [0],
> > [stdout])
> > +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> > + [Datapath actions: 3,2
> > +])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$icmp_flow"],
> > [0], [stdout])
> > +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> > + [Datapath actions: 1
> > +])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$tcp_flow"], [0],
> > [stdout])
> > +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> > + [Datapath actions: 3,1
> > +])
> > +
> > +dnl Invalid filter, nothing should mirror.
> > +AT_CHECK([ovs-vsctl set mirror mymirror filter="invalid"], [0])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$icmp_flow"],
> > [0], [stdout])
> > +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> > + [Datapath actions: 2
> > +])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$tcp_flow"], [0],
> > [stdout])
> > +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> > + [Datapath actions: 2
> > +])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$icmp_flow"],
> > [0], [stdout])
> > +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> > + [Datapath actions: 1
> > +])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$tcp_flow"], [0],
> > [stdout])
> > +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> > + [Datapath actions: 1
> > +])
> > +
> > +OVS_WAIT_UNTIL([test $(grep -Ec "filter is invalid|mirror mymirror
> > configuration is invalid" ovs-vswitchd.log) -eq 2])
> > +
> > +dnl Empty filter, all traffic should mirror.
> > +AT_CHECK([ovs-vsctl clear mirror mymirror filter], [0])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$icmp_flow"],
> > [0], [stdout])
> > +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> > + [Datapath actions: 3,2
> > +])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(1),$tcp_flow"], [0],
> > [stdout])
> > +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> > + [Datapath actions: 3,2
> > +])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$icmp_flow"],
> > [0], [stdout])
> > +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> > + [Datapath actions: 3,1
> > +])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "in_port(2),$tcp_flow"], [0],
> > [stdout])
> > +AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> > + [Datapath actions: 3,1
> > +])
> > +
> > +OVS_VSWITCHD_STOP(["/filter is invalid: invalid: unknown field invalid/d
> > +/mirror mymirror configuration is invalid/d"])
> > +AT_CLEANUP
> > +
> > +
> > AT_SETUP([ofproto-dpif - mirroring, select_all])
> > AT_KEYWORDS([mirror mirrors mirroring])
> > OVS_VSWITCHD_START
> > diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
> > index 4cbd9a5d3..7483b52ac 100755
> > --- a/utilities/ovs-tcpdump.in
> > +++ b/utilities/ovs-tcpdump.in
> > @@ -142,6 +142,7 @@ The following options are available:
> > --mirror-to The name for the mirror port to use
> > (optional)
> > Default 'miINTERFACE'
> > --span If specified, mirror all ports (optional)
> > + --filter Set an OpenFlow formatted preselection filter
> > """ % {'prog': sys.argv[0]})
> > sys.exit(0)
> >
> > @@ -354,7 +355,7 @@ class OVSDB(object):
> > return result
> >
> > def bridge_mirror(self, intf_name, mirror_intf_name, br_name,
> > - mirror_select_all=False):
> > + mirror_select_all=False, mirror_filter=None):
> >
> > txn = self._start_txn()
> > mirror = txn.insert(self.get_table('Mirror'))
> > @@ -362,6 +363,9 @@ class OVSDB(object):
> >
> > mirror.select_all = mirror_select_all
> >
> > + if mirror_filter is not None:
> > + mirror.filter = mirror_filter
> > +
> > mirrored_port = self._find_row_by_name('Port', intf_name)
> >
> > mirror.verify('select_dst_port')
> > @@ -445,6 +449,7 @@ def main():
> > mirror_interface = None
> > mirror_select_all = False
> > dump_cmd = 'tcpdump'
> > + mirror_filter = None
> >
> > for cur, nxt in argv_tuples(sys.argv[1:]):
> > if skip_next:
> > @@ -474,6 +479,10 @@ def main():
> > elif cur in ['--span']:
> > mirror_select_all = True
> > continue
> > + elif cur in ['--filter']:
> > + mirror_filter = nxt
> > + skip_next = True
> > + continue
> > tcpdargs.append(cur)
> >
> > if interface is None:
> > @@ -526,7 +535,7 @@ def main():
> > ovsdb.make_port(mirror_interface, ovsdb.port_bridge(interface))
> > ovsdb.bridge_mirror(interface, mirror_interface,
> > ovsdb.port_bridge(interface),
> > - mirror_select_all)
> > + mirror_select_all, mirror_filter=mirror_filter)
> > except OVSDBException as oe:
> > print("ERROR: Unable to properly setup the mirror: %s." % str(oe))
> > sys.exit(1)
> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index e9110c1d8..6e53ab42a 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -5098,6 +5098,7 @@ mirror_configure(struct mirror *m)
> > {
> > const struct ovsrec_mirror *cfg = m->cfg;
> > struct ofproto_mirror_settings s;
> > + int ret;
> >
> > /* Set name. */
> > if (strcmp(cfg->name, m->name)) {
> > @@ -5166,8 +5167,18 @@ mirror_configure(struct mirror *m)
> > /* Get VLAN selection. */
> > s.src_vlans = vlan_bitmap_from_array(cfg->select_vlan,
> > cfg->n_select_vlan);
> >
> > + /* Set the filter, mirror_set() will strdup this pointer. */
> > + s.filter = cfg->filter;
> > +
> > /* Configure. */
> > - ofproto_mirror_register(m->bridge->ofproto, m, &s);
> > + ret = ofproto_mirror_register(m->bridge->ofproto, m, &s);
> > + if (ret == EOPNOTSUPP) {
> > + VLOG_ERR("ofproto %s: does not support mirroring",
> > + m->bridge->ofproto->name);
> > + } else if (ret) {
> > + VLOG_ERR("bridge %s: mirror %s configuration is invalid",
> > + m->bridge->name, m->name);
> > + }
> >
> > /* Clean up. */
> > if (s.srcs != s.dsts) {
> > diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
> > index 2d395ff95..24c6fcdbf 100644
> > --- a/vswitchd/vswitch.ovsschema
> > +++ b/vswitchd/vswitch.ovsschema
> > @@ -1,6 +1,6 @@
> > {"name": "Open_vSwitch",
> > "version": "8.4.0",
> > - "cksum": "2738838700 27127",
> > + "cksum": "1474151405 27231",
> > "tables": {
> > "Open_vSwitch": {
> > "columns": {
> > @@ -461,6 +461,9 @@
> > "type": {"key": "string", "value": "integer",
> > "min": 0, "max": "unlimited"},
> > "ephemeral": true},
> > + "filter": {
> > + "type": {"key": {"type": "string"},
> > + "min": 0, "max": 1}},
> > "external_ids": {
> > "type": {"key": "string", "value": "string",
> > "min": 0, "max": "unlimited"}}}},
> > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> > index cfcde34ff..b5327c96c 100644
> > --- a/vswitchd/vswitch.xml
> > +++ b/vswitchd/vswitch.xml
> > @@ -5160,6 +5160,19 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
> > type=patch options:peer=p1 \
> > VLANs on which packets are selected for mirroring. An empty set
> > selects packets on all VLANs.
> > </column>
> > + <column name="filter">
> > + <p>
> > + Filter to apply to mirror. Flows that match <ref
> > column="filter"/>
> > + will flow through this mirror, flows that do not match will be
> > + ignored. The <ref column="filter"/> syntax is described in
> > + <code>ovs-fields</code>(7).
> > + </p>
> > + <p>
> > + This filter is applied after <ref column="select_all"/>, <ref
> > + column="select_dst_port"/>, <ref column="select_src_port"/>,
> > and
> > + <ref column="select_vlan"/>.
> > + </p>
> > + </column>
> > </group>
> >
> > <group title="Mirroring Destination Configuration">
> > --
> > 2.39.3
> >
> > _______________________________________________
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
