On Tue, Jan 24, 2023 at 2:21 PM Adrián Moreno <amore...@redhat.com> wrote:
>
> From: Adrian Moreno <amore...@redhat.com>
>
> IPFIX templates have to be sent for each Observation Domain ID.
> Currently, a timer is kept at each dpif_ipfix_exporter to send them.
> This works fine for per-bridge sampling where there is only one
> Observation Domain ID per exporter. However, this is does not work for
> per-flow sampling where more than one Observation Domain IDs can be
> specified by the controller. In this case, ovs-vswitchd will only send
> template information for one (arbitrary) DomainID.
>
> Fix per-flow sampling by using an hmap to keep a timer for each
> Observation Domain ID.
>
> Signed-off-by: Adrian Moreno <amore...@redhat.com>
>
> ---
> - v2:
> - Fixed a potential race condition in unit test.
> - v1:
> - Added unit test.
> - Drop domain_id from "struct dpif_ipfix_domain" since the hashmap
> already uses it as index.
> - Added OVS_REQUES(mutex) to dpif_ipfix_exporter_find_domain.
> ---
> ofproto/ofproto-dpif-ipfix.c | 127 ++++++++++++++++++++++++++++-------
> tests/system-traffic.at | 52 ++++++++++++++
> 2 files changed, 156 insertions(+), 23 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> index 742eed399..fa71fda0f 100644
> --- a/ofproto/ofproto-dpif-ipfix.c
> +++ b/ofproto/ofproto-dpif-ipfix.c
> @@ -124,11 +124,18 @@ struct dpif_ipfix_port {
> uint32_t ifindex;
> };
>
> +struct dpif_ipfix_domain {
> + struct hmap_node hmap_node; /* In struct dpif_ipfix_exporter's domains.
> */
> + time_t last_template_set_time;
> +};
> +
> struct dpif_ipfix_exporter {
> uint32_t exporter_id; /* Exporting Process identifier */
> struct collectors *collectors;
> uint32_t seq_number;
> - time_t last_template_set_time;
> + struct hmap domains; /* Contains struct dpif_ipfix_domain indexed by
> + observation domain id. */
> + time_t last_stats_sent_time;
> struct hmap cache_flow_key_map; /* ipfix_flow_cache_entry. */
> struct ovs_list cache_flow_start_timestamp_list; /*
> ipfix_flow_cache_entry. */
> uint32_t cache_active_timeout; /* In seconds. */
> @@ -617,6 +624,9 @@ static void get_export_time_now(uint64_t *, uint32_t *);
>
> static void dpif_ipfix_cache_expire_now(struct dpif_ipfix_exporter *, bool);
>
> +static void dpif_ipfix_exporter_del_domain(struct dpif_ipfix_exporter * ,
> + struct dpif_ipfix_domain * );
> +
> static bool
> ofproto_ipfix_bridge_exporter_options_equal(
> const struct ofproto_ipfix_bridge_exporter_options *a,
> @@ -697,13 +707,14 @@ dpif_ipfix_exporter_init(struct dpif_ipfix_exporter
> *exporter)
> exporter->exporter_id = ++exporter_total_count;
> exporter->collectors = NULL;
> exporter->seq_number = 1;
> - exporter->last_template_set_time = 0;
> + exporter->last_stats_sent_time = 0;
> hmap_init(&exporter->cache_flow_key_map);
> ovs_list_init(&exporter->cache_flow_start_timestamp_list);
> exporter->cache_active_timeout = 0;
> exporter->cache_max_flows = 0;
> exporter->virtual_obs_id = NULL;
> exporter->virtual_obs_len = 0;
> + hmap_init(&exporter->domains);
>
> memset(&exporter->ipfix_global_stats, 0,
> sizeof(struct dpif_ipfix_global_stats));
> @@ -711,6 +722,7 @@ dpif_ipfix_exporter_init(struct dpif_ipfix_exporter
> *exporter)
>
> static void
> dpif_ipfix_exporter_clear(struct dpif_ipfix_exporter *exporter)
> + OVS_REQUIRES(mutex)
> {
> /* Flush the cache with flow end reason "forced end." */
> dpif_ipfix_cache_expire_now(exporter, true);
> @@ -719,22 +731,29 @@ dpif_ipfix_exporter_clear(struct dpif_ipfix_exporter
> *exporter)
> exporter->exporter_id = 0;
> exporter->collectors = NULL;
> exporter->seq_number = 1;
> - exporter->last_template_set_time = 0;
> + exporter->last_stats_sent_time = 0;
> exporter->cache_active_timeout = 0;
> exporter->cache_max_flows = 0;
> free(exporter->virtual_obs_id);
> exporter->virtual_obs_id = NULL;
> exporter->virtual_obs_len = 0;
>
> + struct dpif_ipfix_domain *dom;
> + HMAP_FOR_EACH_SAFE (dom, hmap_node, &exporter->domains) {
> + dpif_ipfix_exporter_del_domain(exporter, dom);
> + }
> +
> memset(&exporter->ipfix_global_stats, 0,
> sizeof(struct dpif_ipfix_global_stats));
> }
>
> static void
> dpif_ipfix_exporter_destroy(struct dpif_ipfix_exporter *exporter)
> + OVS_REQUIRES(mutex)
> {
> dpif_ipfix_exporter_clear(exporter);
> hmap_destroy(&exporter->cache_flow_key_map);
> + hmap_destroy(&exporter->domains);
> }
>
> static bool
> @@ -742,7 +761,7 @@ dpif_ipfix_exporter_set_options(struct
> dpif_ipfix_exporter *exporter,
> const struct sset *targets,
> const uint32_t cache_active_timeout,
> const uint32_t cache_max_flows,
> - const char *virtual_obs_id)
> + const char *virtual_obs_id)
> OVS_REQUIRES(mutex)
> {
> size_t virtual_obs_len;
> collectors_destroy(exporter->collectors);
> @@ -769,6 +788,37 @@ dpif_ipfix_exporter_set_options(struct
> dpif_ipfix_exporter *exporter,
> return true;
> }
>
> +static struct dpif_ipfix_domain *
> +dpif_ipfix_exporter_find_domain(const struct dpif_ipfix_exporter *exporter,
> + uint32_t domain_id) OVS_REQUIRES(mutex)
> +{
> + struct dpif_ipfix_domain *dom;
> + HMAP_FOR_EACH_WITH_HASH (dom, hmap_node, hash_int(domain_id, 0),
> + &exporter->domains) {
> + return dom;
> + }
> + return NULL;
> +}
> +
> +static struct dpif_ipfix_domain *
> +dpif_ipfix_exporter_insert_domain(struct dpif_ipfix_exporter *exporter,
> + const uint32_t domain_id)
> OVS_REQUIRES(mutex)
> +{
> + struct dpif_ipfix_domain *dom = xmalloc(sizeof *dom);
> + dom->last_template_set_time = 0;
> + hmap_insert(&exporter->domains, &dom->hmap_node, hash_int(domain_id, 0));
> + return dom;
> +}
> +
> +static void
> +dpif_ipfix_exporter_del_domain(struct dpif_ipfix_exporter *exporter,
> + struct dpif_ipfix_domain *dom)
> + OVS_REQUIRES(mutex)
> +{
> + hmap_remove(&exporter->domains, &dom->hmap_node);
> + free(dom);
> +}
> +
> static struct dpif_ipfix_port *
> dpif_ipfix_find_port(const struct dpif_ipfix *di,
> odp_port_t odp_port) OVS_REQUIRES(mutex)
> @@ -909,6 +959,7 @@ dpif_ipfix_bridge_exporter_init(struct
> dpif_ipfix_bridge_exporter *exporter)
>
> static void
> dpif_ipfix_bridge_exporter_clear(struct dpif_ipfix_bridge_exporter *exporter)
> + OVS_REQUIRES(mutex)
> {
> dpif_ipfix_exporter_clear(&exporter->exporter);
> ofproto_ipfix_bridge_exporter_options_destroy(exporter->options);
> @@ -918,6 +969,7 @@ dpif_ipfix_bridge_exporter_clear(struct
> dpif_ipfix_bridge_exporter *exporter)
>
> static void
> dpif_ipfix_bridge_exporter_destroy(struct dpif_ipfix_bridge_exporter
> *exporter)
> + OVS_REQUIRES(mutex)
> {
> dpif_ipfix_bridge_exporter_clear(exporter);
> dpif_ipfix_exporter_destroy(&exporter->exporter);
> @@ -927,7 +979,7 @@ static void
> dpif_ipfix_bridge_exporter_set_options(
> struct dpif_ipfix_bridge_exporter *exporter,
> const struct ofproto_ipfix_bridge_exporter_options *options,
> - bool *options_changed)
> + bool *options_changed) OVS_REQUIRES(mutex)
> {
> if (!options || sset_is_empty(&options->targets)) {
> /* No point in doing any work if there are no targets. */
> @@ -1003,6 +1055,7 @@ dpif_ipfix_flow_exporter_init(struct
> dpif_ipfix_flow_exporter *exporter)
>
> static void
> dpif_ipfix_flow_exporter_clear(struct dpif_ipfix_flow_exporter *exporter)
> + OVS_REQUIRES(mutex)
> {
> dpif_ipfix_exporter_clear(&exporter->exporter);
> ofproto_ipfix_flow_exporter_options_destroy(exporter->options);
> @@ -1011,6 +1064,7 @@ dpif_ipfix_flow_exporter_clear(struct
> dpif_ipfix_flow_exporter *exporter)
>
> static void
> dpif_ipfix_flow_exporter_destroy(struct dpif_ipfix_flow_exporter *exporter)
> + OVS_REQUIRES(mutex)
> {
> dpif_ipfix_flow_exporter_clear(exporter);
> dpif_ipfix_exporter_destroy(&exporter->exporter);
> @@ -1020,7 +1074,7 @@ static bool
> dpif_ipfix_flow_exporter_set_options(
> struct dpif_ipfix_flow_exporter *exporter,
> const struct ofproto_ipfix_flow_exporter_options *options,
> - bool *options_changed)
> + bool *options_changed) OVS_REQUIRES(mutex)
> {
> if (sset_is_empty(&options->targets)) {
> /* No point in doing any work if there are no targets. */
> @@ -1071,6 +1125,7 @@ dpif_ipfix_flow_exporter_set_options(
> static void
> remove_flow_exporter(struct dpif_ipfix *di,
> struct dpif_ipfix_flow_exporter_map_node *node)
> + OVS_REQUIRES(mutex)
> {
> hmap_remove(&di->flow_exporter_map, &node->node);
> dpif_ipfix_flow_exporter_destroy(&node->exporter);
> @@ -2000,6 +2055,7 @@ static void
> ipfix_cache_update(struct dpif_ipfix_exporter *exporter,
> struct ipfix_flow_cache_entry *entry,
> enum ipfix_sampled_packet_type sampled_pkt_type)
> + OVS_REQUIRES(mutex)
> {
> struct ipfix_flow_cache_entry *old_entry;
> size_t current_flows = 0;
> @@ -2811,14 +2867,36 @@ dpif_ipfix_flow_sample(struct dpif_ipfix *di, const
> struct dp_packet *packet,
> ovs_mutex_unlock(&mutex);
> }
>
> +static bool
> +dpif_ipfix_should_send_template(struct dpif_ipfix_exporter *exporter,
> + const uint32_t observation_domain_id,
> + const uint32_t export_time_sec)
> + OVS_REQUIRES(mutex)
> +{
> + struct dpif_ipfix_domain *domain;
> + domain = dpif_ipfix_exporter_find_domain(exporter,
> + observation_domain_id);
> + if (!domain) {
> + /* First time we see this obs_domain_id. */
> + domain = dpif_ipfix_exporter_insert_domain(exporter,
> + observation_domain_id);
> + }
> +
> + if ((domain->last_template_set_time + IPFIX_TEMPLATE_INTERVAL)
> + <= export_time_sec) {
> + domain->last_template_set_time = export_time_sec;
> + return true;
> + }
> + return false;
> +}
> +
> static void
> dpif_ipfix_cache_expire(struct dpif_ipfix_exporter *exporter,
> bool forced_end, const uint64_t export_time_usec,
> - const uint32_t export_time_sec)
> + const uint32_t export_time_sec) OVS_REQUIRES(mutex)
> {
> struct ipfix_flow_cache_entry *entry;
> uint64_t max_flow_start_timestamp_usec;
> - bool template_msg_sent = false;
> enum ipfix_flow_end_reason flow_end_reason;
>
> if (ovs_list_is_empty(&exporter->cache_flow_start_timestamp_list)) {
> @@ -2844,25 +2922,28 @@ dpif_ipfix_cache_expire(struct dpif_ipfix_exporter
> *exporter,
> break;
> }
>
> - ovs_list_remove(&entry->cache_flow_start_timestamp_list_node);
> - hmap_remove(&exporter->cache_flow_key_map,
> - &entry->flow_key_map_node);
> + /* XXX: Make frequency of the (Options) Template and Exporter Process
> + * Statistics transmission configurable.
> + * Cf. IETF RFC 5101 Section 4.3. and 10.3.6. */
> + if ((exporter->last_stats_sent_time + IPFIX_TEMPLATE_INTERVAL)
> + <= export_time_sec) {
> + exporter->last_stats_sent_time = export_time_sec;
> + ipfix_send_exporter_data_msg(exporter, export_time_sec);
> + }
>
> - /* XXX: Make frequency of the (Options) Template and Exporter
> Process
> - * Statistics transmission configurable.
> - * Cf. IETF RFC 5101 Section 4.3. and 10.3.6. */
> - if (!template_msg_sent
> - && (exporter->last_template_set_time + IPFIX_TEMPLATE_INTERVAL)
> - <= export_time_sec) {
> + if (dpif_ipfix_should_send_template(exporter,
> + entry->flow_key.obs_domain_id,
> + export_time_sec)) {
> + VLOG_DBG("Sending templates for ObservationDomainID %"PRIu32,
> + entry->flow_key.obs_domain_id);
> ipfix_send_template_msgs(exporter, export_time_sec,
> entry->flow_key.obs_domain_id);
> - exporter->last_template_set_time = export_time_sec;
> - template_msg_sent = true;
> -
> - /* Send Exporter Process Statistics. */
> - ipfix_send_exporter_data_msg(exporter, export_time_sec);
> }
>
> + ovs_list_remove(&entry->cache_flow_start_timestamp_list_node);
> + hmap_remove(&exporter->cache_flow_key_map,
> + &entry->flow_key_map_node);
> +
> /* XXX: Group multiple data records for the same obs domain id
> * into the same message. */
> ipfix_send_data_msg(exporter, export_time_sec, entry,
> flow_end_reason);
> @@ -2883,7 +2964,7 @@ get_export_time_now(uint64_t *export_time_usec,
> uint32_t *export_time_sec)
>
> static void
> dpif_ipfix_cache_expire_now(struct dpif_ipfix_exporter *exporter,
> - bool forced_end)
> + bool forced_end) OVS_REQUIRES(mutex)
> {
> uint64_t export_time_usec;
> uint32_t export_time_sec;
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 08c78ff57..1421ab0ab 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -7488,3 +7488,55 @@ OVS_WAIT_UNTIL([cat p2.pcap | grep -E "0x0050: *0000
> *0000 *5002 *2000 *b85e *00
>
> OVS_TRAFFIC_VSWITCHD_STOP
> AT_CLEANUP
> +
> +AT_SETUP([ipfix - muliple domains])
> +AT_SKIP_IF([test $HAVE_TCPDUMP = no])
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
> +
> +dnl Create a dummy IPFIX collector just in localhost so that port seems open.
> +OVS_DAEMONIZE([nc -l -u 127.0.0.1 5555 > /dev/null], [nc.pid])
> +
> +OVS_DAEMONIZE([tcpdump -n -i lo udp port 5555 -w ipfix.pcap], [tcpdump.pid])
> +sleep 1
> +
> +dnl Configure a Flow Sample Collector Set with id = 1.
> +AT_CHECK([ovs-vsctl -- --id=@br get Bridge br0 \
> + -- --id=@i create IPFIX targets=\"127.0.0.1:5555\" \
> + -- create Flow_Sample_Collector_Set bridge=@br id=1
> ipfix=@i],
> + [ignore], [ignore])
> +
> +dnl Push flows that will allow communication between the network namespaces
> +dnl and sample all ICMP traffic with multiple observation domain IDs.
> +AT_DATA([flows.txt], [dnl
> +table=0,priority=100,in_port=ovs-p0,ip,icmp
> actions=sample(probability=65535,collector_set_id=1,obs_domain_id=1),NORMAL
> +table=0,priority=100,in_port=ovs-p1,ip,icmp
> actions=sample(probability=65535,collector_set_id=1,obs_domain_id=2),NORMAL
> +table=0,priority=0,actions=NORMAL
> +])
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.2 | FORMAT_PING],
> [0], [dnl
> +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> +])
> +
> +sleep 1
> +kill $(cat tcpdump.pid)
> +wait $(cat tcpdump.pid) 2> /dev/null
> +
> +dnl Check templates (Flow ID = 0x02) where sent to ObservationDomain 1.
> +AT_CHECK([test $(tcpdump -r ipfix.pcap --count "udp port 5555 and
> udp[[20:4]] = 0x00000001 and udp[[24:2]] = 0x02" 2>/dev/null | tr -d
> "packets") -gt 0])
These still seem to be getting "test: -gt: unary operator expected"
errors on Intel CI, even after the modification. I'm at a loss as to
why, the error message makes it seem like tcpdump is choking on the
pcap and not writing anything to stdout. But the pcap should be
present and readable.
-M
> +dnl Check that exactly 3 samples (Flow ID != 0x02) where sent to
> ObservationDomain 1.
> +AT_CHECK([test $(tcpdump -r ipfix.pcap --count "udp port 5555 and
> udp[[20:4]] = 0x00000001 and udp[[24:2]] != 0x02" 2>/dev/null | tr -d
> "packets") -eq 3])
> +
> +dnl Check templates (Flow ID = 0x02) where sent to ObservationDomain 2.
> +AT_CHECK([test $(tcpdump -r ipfix.pcap --count "udp port 5555 and
> udp[[20:4]] = 0x00000002 and udp[[24:2]] = 0x02" 2>/dev/null | tr -d
> "packets") -gt 0])
> +dnl Check that exactly 3 samples (Flow ID != 0x02) where sent to
> ObservationDomain 2.
> +AT_CHECK([test $(tcpdump -r ipfix.pcap --count "udp port 5555 and
> udp[[20:4]] = 0x00000002 and udp[[24:2]] != 0x02" 2>/dev/null | tr -d
> "packets") -eq 3])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
> +
> --
> 2.38.1
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
