| Message ID | 20230124192130.706143-1-amorenoz@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2,1/2] ofproto-ipfix: use per-domain template timeouts | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
Bleep bloop. Greetings Adrián Moreno, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Comment with 'xxx' marker
#271 FILE: ofproto/ofproto-dpif-ipfix.c:2925:
/* XXX: Make frequency of the (Options) Template and Exporter Process
Lines checked: 379, Warnings: 1, Errors: 0
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On Tue, Jan 24, 2023 at 2:21 PM Adrián Moreno <amorenoz@redhat.com> wrote: > > From: Adrian Moreno <amorenoz@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 <amorenoz@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 >
On Tue, Jan 24, 2023 at 08:21:28PM +0100, Adrián Moreno wrote: > From: Adrian Moreno <amorenoz@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 <amorenoz@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. */ I'm not sure if it is important, but this change bumps the cache_flow_key_map from the 1st to 2nd cacheline (on x86_64). This can be avoided by moving seq_number above collectors, as there is a 32-bit hole there. Possibly there is further scope for making this structure more cache-fiendly. But again, I'm not sure if it is important. Before: $ pahole ofproto/libofproto_la-ofproto-dpif-ipfix.o ... struct dpif_ipfix_exporter { uint32_t exporter_id; /* 0 4 */ /* XXX 4 bytes hole, try to pack */ struct collectors * collectors; /* 8 8 */ uint32_t seq_number; /* 16 4 */ /* XXX 4 bytes hole, try to pack */ time_t last_template_set_time; /* 24 8 */ struct hmap cache_flow_key_map; /* 32 32 */ /* --- cacheline 1 boundary (64 bytes) --- */ struct ovs_list cache_flow_start_timestamp_list; /* 64 16 */ uint32_t cache_active_timeout; /* 80 4 */ uint32_t cache_max_flows; /* 84 4 */ char * virtual_obs_id; /* 88 8 */ uint8_t virtual_obs_len; /* 96 1 */ /* XXX 7 bytes hole, try to pack */ ofproto_ipfix_stats ofproto_stats; /* 104 88 */ /* --- cacheline 3 boundary (192 bytes) --- */ struct dpif_ipfix_global_stats ipfix_global_stats; /* 192 152 */ /* size: 344, cachelines: 6, members: 12 */ /* sum members: 329, holes: 3, sum holes: 15 */ /* last cacheline: 24 bytes */ } ... After: $ pahole ofproto/libofproto_la-ofproto-dpif-ipfix.o ... struct dpif_ipfix_exporter { uint32_t exporter_id; /* 0 4 */ /* XXX 4 bytes hole, try to pack */ struct collectors * collectors; /* 8 8 */ uint32_t seq_number; /* 16 4 */ /* XXX 4 bytes hole, try to pack */ struct hmap domains; /* 24 32 */ time_t last_stats_sent_time; /* 56 8 */ /* --- cacheline 1 boundary (64 bytes) --- */ struct hmap cache_flow_key_map; /* 64 32 */ struct ovs_list cache_flow_start_timestamp_list; /* 96 16 */ uint32_t cache_active_timeout; /* 112 4 */ uint32_t cache_max_flows; /* 116 4 */ char * virtual_obs_id; /* 120 8 */ /* --- cacheline 2 boundary (128 bytes) --- */ uint8_t virtual_obs_len; /* 128 1 */ /* XXX 7 bytes hole, try to pack */ ofproto_ipfix_stats ofproto_stats; /* 136 88 */ /* --- cacheline 3 boundary (192 bytes) was 32 bytes ago --- */ struct dpif_ipfix_global_stats ipfix_global_stats; /* 224 152 */ /* size: 376, cachelines: 6, members: 13 */ /* sum members: 361, holes: 3, sum holes: 15 */ /* last cacheline: 56 bytes */ }; ... Proposal: $ git diff diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c index fa71fda0f276..1701d91e8a72 100644 --- a/ofproto/ofproto-dpif-ipfix.c +++ b/ofproto/ofproto-dpif-ipfix.c @@ -131,8 +131,8 @@ struct dpif_ipfix_domain { struct dpif_ipfix_exporter { uint32_t exporter_id; /* Exporting Process identifier */ - struct collectors *collectors; uint32_t seq_number; + struct collectors *collectors; struct hmap domains; /* Contains struct dpif_ipfix_domain indexed by observation domain id. */ time_t last_stats_sent_time; $ pahole ofproto/libofproto_la-ofproto-dpif-ipfix.o ... struct dpif_ipfix_exporter { uint32_t exporter_id; /* 0 4 */ uint32_t seq_number; /* 4 4 */ struct collectors * collectors; /* 8 8 */ struct hmap domains; /* 16 32 */ time_t last_stats_sent_time; /* 48 8 */ struct hmap cache_flow_key_map; /* 56 32 */ /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */ struct ovs_list cache_flow_start_timestamp_list; /* 88 16 */ uint32_t cache_active_timeout; /* 104 4 */ uint32_t cache_max_flows; /* 108 4 */ char * virtual_obs_id; /* 112 8 */ uint8_t virtual_obs_len; /* 120 1 */ /* XXX 7 bytes hole, try to pack */ /* --- cacheline 2 boundary (128 bytes) --- */ ofproto_ipfix_stats ofproto_stats; /* 128 88 */ /* --- cacheline 3 boundary (192 bytes) was 24 bytes ago --- */ struct dpif_ipfix_global_stats ipfix_global_stats; /* 216 152 */ /* size: 368, cachelines: 6, members: 13 */ /* sum members: 361, holes: 1, sum holes: 7 */ /* last cacheline: 48 bytes */ }; ... > 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]) > +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 > + nit: git complains, I think it is about the trailing blank line above. $ git am a.patch Applying: ofproto-ipfix: use per-domain template timeouts .git/rebase-apply/patch:366: new blank line at EOF. + warning: 1 line adds whitespace errors.
On 1/25/23 06:34, Mike Pattrick wrote: > On Tue, Jan 24, 2023 at 2:21 PM Adrián Moreno <amorenoz@redhat.com> wrote: >> >> From: Adrian Moreno <amorenoz@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 <amorenoz@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. Yeah, I can't reproduce the failure happening on Intel CI as well. Michael, do you know what is happening with this test? OTOH, it might be better to take the road similar to how we test sflow and netflow. I just found specific test binaries test-sflow and test-netflow which are just listening and printing out packtes in human-readable format. If we had something like this for ipfix, we could write similar unit tests that do not require any external tools or root privileges, as we do for sflow and netflow. Thoughts? We might split this bug fix from the implementation of the unit test though, since it will require a bit more code. Best regards, Ilya Maximets.
> -----Original Message----- > From: Ilya Maximets <i.maximets@ovn.org> > Sent: Friday 27 January 2023 20:03 > To: Mike Pattrick <mkp@redhat.com>; Adrián Moreno > <amorenoz@redhat.com>; Phelan, Michael <michael.phelan@intel.com> > Cc: dev@openvswitch.org; i.maximets@ovn.org; Stokes, Ian > <ian.stokes@intel.com> > Subject: Re: [ovs-dev] [PATCH v2 1/2] ofproto-ipfix: use per-domain template > timeouts > > On 1/25/23 06:34, Mike Pattrick wrote: > > On Tue, Jan 24, 2023 at 2:21 PM Adrián Moreno <amorenoz@redhat.com> > wrote: > >> > >> From: Adrian Moreno <amorenoz@redhat.com> <snip> > >> +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. > > Yeah, I can't reproduce the failure happening on Intel CI as well. > > Michael, do you know what is happening with this test? Hi Ilya, I have reran the test and received the same failure as reported on the mailing list, I'm not sure what exactly is going on with this test, maybe there is some prerequisite missing from our machine? Let me know if you need any more information. Thanks, Michael. > > OTOH, it might be better to take the road similar to how we test sflow and > netflow. I just found specific test binaries test-sflow and test-netflow which are > just listening and printing out packtes in human-readable format. If we had > something like this for ipfix, we could write similar unit tests that do not require > any external tools or root privileges, as we do for sflow and netflow. > > Thoughts? > > We might split this bug fix from the implementation of the unit test though, > since it will require a bit more code. > > Best regards, Ilya Maximets.
On 1/31/23 13:15, Phelan, Michael wrote: >> -----Original Message----- >> From: Ilya Maximets <i.maximets@ovn.org> >> Sent: Friday 27 January 2023 20:03 >> To: Mike Pattrick <mkp@redhat.com>; Adrián Moreno >> <amorenoz@redhat.com>; Phelan, Michael <michael.phelan@intel.com> >> Cc: dev@openvswitch.org; i.maximets@ovn.org; Stokes, Ian >> <ian.stokes@intel.com> >> Subject: Re: [ovs-dev] [PATCH v2 1/2] ofproto-ipfix: use per-domain template >> timeouts >> >> On 1/25/23 06:34, Mike Pattrick wrote: >>> On Tue, Jan 24, 2023 at 2:21 PM Adrián Moreno <amorenoz@redhat.com> >> wrote: >>>> >>>> From: Adrian Moreno <amorenoz@redhat.com> > <snip> >>>> +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. >> >> Yeah, I can't reproduce the failure happening on Intel CI as well. >> >> Michael, do you know what is happening with this test? > > Hi Ilya, > I have reran the test and received the same failure as reported on the mailing list, I'm not sure what exactly is going on with this test, maybe there is some prerequisite missing from our machine? Thanks for checking. It doesn't look like something is missing... Could you provide the content of testsuite directory after the test failure? i.e. the ipfix.pcap file and other stuff. This may shed some light. Best regards, Ilya Maximets. > > Let me know if you need any more information. > > Thanks, > Michael. >> >> OTOH, it might be better to take the road similar to how we test sflow and >> netflow. I just found specific test binaries test-sflow and test-netflow which are >> just listening and printing out packtes in human-readable format. If we had >> something like this for ipfix, we could write similar unit tests that do not require >> any external tools or root privileges, as we do for sflow and netflow. >> >> Thoughts? >> >> We might split this bug fix from the implementation of the unit test though, >> since it will require a bit more code. >> >> Best regards, Ilya Maximets.
> -----Original Message----- > From: Ilya Maximets <i.maximets@ovn.org> > Sent: Tuesday 31 January 2023 12:23 > To: Phelan, Michael <michael.phelan@intel.com>; Mike Pattrick > <mkp@redhat.com>; Adrián Moreno <amorenoz@redhat.com> > Cc: i.maximets@ovn.org; dev@openvswitch.org; Stokes, Ian > <ian.stokes@intel.com> > Subject: Re: [ovs-dev] [PATCH v2 1/2] ofproto-ipfix: use per-domain template > timeouts > > On 1/31/23 13:15, Phelan, Michael wrote: > >> -----Original Message----- > >> From: Ilya Maximets <i.maximets@ovn.org> > >> Sent: Friday 27 January 2023 20:03 > >> To: Mike Pattrick <mkp@redhat.com>; Adrián Moreno > >> <amorenoz@redhat.com>; Phelan, Michael <michael.phelan@intel.com> > >> Cc: dev@openvswitch.org; i.maximets@ovn.org; Stokes, Ian > >> <ian.stokes@intel.com> > >> Subject: Re: [ovs-dev] [PATCH v2 1/2] ofproto-ipfix: use per-domain > >> template timeouts > >> > >> On 1/25/23 06:34, Mike Pattrick wrote: > >>> On Tue, Jan 24, 2023 at 2:21 PM Adrián Moreno <amorenoz@redhat.com> > >> wrote: > >>>> > >>>> From: Adrian Moreno <amorenoz@redhat.com> > > <snip> > >>>> +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. > >> > >> Yeah, I can't reproduce the failure happening on Intel CI as well. > >> > >> Michael, do you know what is happening with this test? > > > > Hi Ilya, > > I have reran the test and received the same failure as reported on the mailing > list, I'm not sure what exactly is going on with this test, maybe there is some > prerequisite missing from our machine? > > Thanks for checking. > It doesn't look like something is missing... > > Could you provide the content of testsuite directory after the test failure? > i.e. the ipfix.pcap file and other stuff. This may shed some light. > > Best regards, Ilya Maximets. Sure, I've attached a zipped folder containing the contents of the testsuite directory after the test failure. Let me know if you have any more questions. Kind regards, Michael. > > > > > Let me know if you need any more information. > > > > Thanks, > > Michael. > >> > >> OTOH, it might be better to take the road similar to how we test > >> sflow and netflow. I just found specific test binaries test-sflow > >> and test-netflow which are just listening and printing out packtes in > >> human-readable format. If we had something like this for ipfix, we > >> could write similar unit tests that do not require any external tools or root > privileges, as we do for sflow and netflow. > >> > >> Thoughts? > >> > >> We might split this bug fix from the implementation of the unit test > >> though, since it will require a bit more code. > >> > >> Best regards, Ilya Maximets.
On Tue, Jan 31, 2023 at 10:45 AM Phelan, Michael <michael.phelan@intel.com> wrote: > > > -----Original Message----- > > From: Ilya Maximets <i.maximets@ovn.org> > > Sent: Tuesday 31 January 2023 12:23 > > To: Phelan, Michael <michael.phelan@intel.com>; Mike Pattrick > > <mkp@redhat.com>; Adrián Moreno <amorenoz@redhat.com> > > Cc: i.maximets@ovn.org; dev@openvswitch.org; Stokes, Ian > > <ian.stokes@intel.com> > > Subject: Re: [ovs-dev] [PATCH v2 1/2] ofproto-ipfix: use per-domain template > > timeouts > > > > On 1/31/23 13:15, Phelan, Michael wrote: > > >> -----Original Message----- > > >> From: Ilya Maximets <i.maximets@ovn.org> > > >> Sent: Friday 27 January 2023 20:03 > > >> To: Mike Pattrick <mkp@redhat.com>; Adrián Moreno > > >> <amorenoz@redhat.com>; Phelan, Michael <michael.phelan@intel.com> > > >> Cc: dev@openvswitch.org; i.maximets@ovn.org; Stokes, Ian > > >> <ian.stokes@intel.com> > > >> Subject: Re: [ovs-dev] [PATCH v2 1/2] ofproto-ipfix: use per-domain > > >> template timeouts > > >> > > >> On 1/25/23 06:34, Mike Pattrick wrote: > > >>> On Tue, Jan 24, 2023 at 2:21 PM Adrián Moreno <amorenoz@redhat.com> > > >> wrote: > > >>>> > > >>>> From: Adrian Moreno <amorenoz@redhat.com> > > > <snip> > > >>>> +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. > > >> > > >> Yeah, I can't reproduce the failure happening on Intel CI as well. > > >> > > >> Michael, do you know what is happening with this test? > > > > > > Hi Ilya, > > > I have reran the test and received the same failure as reported on the mailing > > list, I'm not sure what exactly is going on with this test, maybe there is some > > prerequisite missing from our machine? > > > > Thanks for checking. > > It doesn't look like something is missing... > > > > Could you provide the content of testsuite directory after the test failure? > > i.e. the ipfix.pcap file and other stuff. This may shed some light. > > > > Best regards, Ilya Maximets. > Sure, I've attached a zipped folder containing the contents of the testsuite directory after the test failure. > > Let me know if you have any more questions. It looks like --count is a recent feature, only added in 2020. So any copy of tcpdump preceding that will error out. This could be why the test fails on CI? -M > > Kind regards, > Michael. > > > > > > > > Let me know if you need any more information. > > > > > > Thanks, > > > Michael. > > >> > > >> OTOH, it might be better to take the road similar to how we test > > >> sflow and netflow. I just found specific test binaries test-sflow > > >> and test-netflow which are just listening and printing out packtes in > > >> human-readable format. If we had something like this for ipfix, we > > >> could write similar unit tests that do not require any external tools or root > > privileges, as we do for sflow and netflow. > > >> > > >> Thoughts? > > >> > > >> We might split this bug fix from the implementation of the unit test > > >> though, since it will require a bit more code. > > >> > > >> Best regards, Ilya Maximets. >
On 1/31/23 17:30, Mike Pattrick wrote: > On Tue, Jan 31, 2023 at 10:45 AM Phelan, Michael > <michael.phelan@intel.com> wrote: >> >>> -----Original Message----- >>> From: Ilya Maximets <i.maximets@ovn.org> >>> Sent: Tuesday 31 January 2023 12:23 >>> To: Phelan, Michael <michael.phelan@intel.com>; Mike Pattrick >>> <mkp@redhat.com>; Adrián Moreno <amorenoz@redhat.com> >>> Cc: i.maximets@ovn.org; dev@openvswitch.org; Stokes, Ian >>> <ian.stokes@intel.com> >>> Subject: Re: [ovs-dev] [PATCH v2 1/2] ofproto-ipfix: use per-domain template >>> timeouts >>> >>> On 1/31/23 13:15, Phelan, Michael wrote: >>>>> -----Original Message----- >>>>> From: Ilya Maximets <i.maximets@ovn.org> >>>>> Sent: Friday 27 January 2023 20:03 >>>>> To: Mike Pattrick <mkp@redhat.com>; Adrián Moreno >>>>> <amorenoz@redhat.com>; Phelan, Michael <michael.phelan@intel.com> >>>>> Cc: dev@openvswitch.org; i.maximets@ovn.org; Stokes, Ian >>>>> <ian.stokes@intel.com> >>>>> Subject: Re: [ovs-dev] [PATCH v2 1/2] ofproto-ipfix: use per-domain >>>>> template timeouts >>>>> >>>>> On 1/25/23 06:34, Mike Pattrick wrote: >>>>>> On Tue, Jan 24, 2023 at 2:21 PM Adrián Moreno <amorenoz@redhat.com> >>>>> wrote: >>>>>>> >>>>>>> From: Adrian Moreno <amorenoz@redhat.com> >>>> <snip> >>>>>>> +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. >>>>> >>>>> Yeah, I can't reproduce the failure happening on Intel CI as well. >>>>> >>>>> Michael, do you know what is happening with this test? >>>> >>>> Hi Ilya, >>>> I have reran the test and received the same failure as reported on the mailing >>> list, I'm not sure what exactly is going on with this test, maybe there is some >>> prerequisite missing from our machine? >>> >>> Thanks for checking. >>> It doesn't look like something is missing... >>> >>> Could you provide the content of testsuite directory after the test failure? >>> i.e. the ipfix.pcap file and other stuff. This may shed some light. >>> >>> Best regards, Ilya Maximets. >> Sure, I've attached a zipped folder containing the contents of the testsuite directory after the test failure. >> >> Let me know if you have any more questions. > > It looks like --count is a recent feature, only added in 2020. So any > copy of tcpdump preceding that will error out. This could be why the > test fails on CI? > Thanks Mike, Ilya and Michael for following up on this while I was on a sick leave. I think we can follow Ilya's suggestion and remove the unit testing from the first patch so it can be backported and then add a small program that parses IPFIX packets and prints some information. Maybe we can even reuse something from test-netflow.c. But this will be a bigger patch (currently header definitions are private) so it's better to do it separately. Thanks.
On 1/25/23 16:35, Simon Horman wrote: > On Tue, Jan 24, 2023 at 08:21:28PM +0100, Adrián Moreno wrote: >> From: Adrian Moreno <amorenoz@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 <amorenoz@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. */ > > I'm not sure if it is important, but this change bumps the > cache_flow_key_map from the 1st to 2nd cacheline (on x86_64). > > This can be avoided by moving seq_number above collectors, > as there is a 32-bit hole there. > Thanks very much for looking at this Simon. Your proposal makes total sense. We should not degrade cache efficiency when it's so easily avoidable. I'll add it to the patch. > Possibly there is further scope for making this structure more > cache-fiendly. But again, I'm not sure if it is important. > Looking further optimizing this structure, I don't think there's huge space for improvement. The operation that we'd like to optimize would be ipfix_cache_update() which also uses "ofproto_stats", that spans over 2 cache lines. I think the best we can do is: struct dpif_ipfix_exporter { struct hmap cache_flow_key_map; /* 0 32 */ struct ovs_list cache_flow_start_timestamp_list; /* 32 16 */ uint32_t cache_max_flows; /* 48 4 */ uint32_t cache_active_timeout; /* 52 4 */ struct hmap domains; /* 56 32 */ /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */ time_t last_stats_sent_time; /* 88 8 */ char * virtual_obs_id; /* 96 8 */ struct collectors * collectors; /* 104 8 */ uint32_t exporter_id; /* 112 4 */ uint32_t seq_number; /* 116 4 */ uint8_t virtual_obs_len; /* 120 1 */ /* XXX 7 bytes hole, try to pack */ /* --- cacheline 2 boundary (128 bytes) --- */ ofproto_ipfix_stats ofproto_stats; /* 128 88 */ /* --- cacheline 3 boundary (192 bytes) was 24 bytes ago --- */ struct dpif_ipfix_global_stats ipfix_global_stats; /* 216 152 */ /* size: 368, cachelines: 6, members: 13 */ /* sum members: 361, holes: 1, sum holes: 7 */ /* last cacheline: 48 bytes */ }; If we're lucky we can not make use of cacheline 2 during this operation. Not sure that'll give us any significant benefit. My plan is to do some performance testing of the IPFIX collection. I will take this into account. > Before: > > $ pahole ofproto/libofproto_la-ofproto-dpif-ipfix.o > ... > struct dpif_ipfix_exporter { > uint32_t exporter_id; /* 0 4 */ > > /* XXX 4 bytes hole, try to pack */ > > struct collectors * collectors; /* 8 8 */ > uint32_t seq_number; /* 16 4 */ > > /* XXX 4 bytes hole, try to pack */ > > time_t last_template_set_time; /* 24 8 */ > struct hmap cache_flow_key_map; /* 32 32 */ > /* --- cacheline 1 boundary (64 bytes) --- */ > struct ovs_list cache_flow_start_timestamp_list; /* 64 > 16 */ > uint32_t cache_active_timeout; /* 80 4 */ > uint32_t cache_max_flows; /* 84 4 */ > char * virtual_obs_id; /* 88 8 */ > uint8_t virtual_obs_len; /* 96 1 */ > > /* XXX 7 bytes hole, try to pack */ > > ofproto_ipfix_stats ofproto_stats; /* 104 88 */ > /* --- cacheline 3 boundary (192 bytes) --- */ > struct dpif_ipfix_global_stats ipfix_global_stats; /* 192 152 */ > > /* size: 344, cachelines: 6, members: 12 */ > /* sum members: 329, holes: 3, sum holes: 15 */ > /* last cacheline: 24 bytes */ > } > ... > > After: > > $ pahole ofproto/libofproto_la-ofproto-dpif-ipfix.o > ... > struct dpif_ipfix_exporter { > uint32_t exporter_id; /* 0 4 */ > > /* XXX 4 bytes hole, try to pack */ > > struct collectors * collectors; /* 8 8 */ > uint32_t seq_number; /* 16 4 */ > > /* XXX 4 bytes hole, try to pack */ > > struct hmap domains; /* 24 32 */ > time_t last_stats_sent_time; /* 56 8 */ > /* --- cacheline 1 boundary (64 bytes) --- */ > struct hmap cache_flow_key_map; /* 64 32 */ > struct ovs_list cache_flow_start_timestamp_list; /* 96 16 */ > uint32_t cache_active_timeout; /* 112 4 */ > uint32_t cache_max_flows; /* 116 4 */ > char * virtual_obs_id; /* 120 8 */ > /* --- cacheline 2 boundary (128 bytes) --- */ > uint8_t virtual_obs_len; /* 128 1 */ > > /* XXX 7 bytes hole, try to pack */ > > ofproto_ipfix_stats ofproto_stats; /* 136 88 */ > /* --- cacheline 3 boundary (192 bytes) was 32 bytes ago --- */ > struct dpif_ipfix_global_stats ipfix_global_stats; /* 224 152 */ > > /* size: 376, cachelines: 6, members: 13 */ > /* sum members: 361, holes: 3, sum holes: 15 */ > /* last cacheline: 56 bytes */ > }; > ... > > Proposal: > > > $ git diff > diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c > index fa71fda0f276..1701d91e8a72 100644 > --- a/ofproto/ofproto-dpif-ipfix.c > +++ b/ofproto/ofproto-dpif-ipfix.c > @@ -131,8 +131,8 @@ struct dpif_ipfix_domain { > > struct dpif_ipfix_exporter { > uint32_t exporter_id; /* Exporting Process identifier */ > - struct collectors *collectors; > uint32_t seq_number; > + struct collectors *collectors; > struct hmap domains; /* Contains struct dpif_ipfix_domain indexed by > observation domain id. */ > time_t last_stats_sent_time; > > $ pahole ofproto/libofproto_la-ofproto-dpif-ipfix.o > ... > struct dpif_ipfix_exporter { > uint32_t exporter_id; /* 0 4 */ > uint32_t seq_number; /* 4 4 */ > struct collectors * collectors; /* 8 8 */ > struct hmap domains; /* 16 32 */ > time_t last_stats_sent_time; /* 48 8 */ > struct hmap cache_flow_key_map; /* 56 32 */ > /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */ > struct ovs_list cache_flow_start_timestamp_list; /* 88 16 */ > uint32_t cache_active_timeout; /* 104 4 */ > uint32_t cache_max_flows; /* 108 4 */ > char * virtual_obs_id; /* 112 8 */ > uint8_t virtual_obs_len; /* 120 1 */ > > /* XXX 7 bytes hole, try to pack */ > > /* --- cacheline 2 boundary (128 bytes) --- */ > ofproto_ipfix_stats ofproto_stats; /* 128 88 */ > /* --- cacheline 3 boundary (192 bytes) was 24 bytes ago --- */ > struct dpif_ipfix_global_stats ipfix_global_stats; /* 216 152 */ > > /* size: 368, cachelines: 6, members: 13 */ > /* sum members: 361, holes: 1, sum holes: 7 */ > /* last cacheline: 48 bytes */ > }; > ... > >> 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]) >> +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 >> + > > nit: git complains, I think it is about the trailing blank line above. > > $ git am a.patch > Applying: ofproto-ipfix: use per-domain template timeouts > .git/rebase-apply/patch:366: new blank line at EOF. > + > warning: 1 line adds whitespace errors. >
On Fri, Feb 10, 2023 at 12:30:58PM +0100, Adrian Moreno wrote: > > > On 1/25/23 16:35, Simon Horman wrote: > > On Tue, Jan 24, 2023 at 08:21:28PM +0100, Adrián Moreno wrote: > > > From: Adrian Moreno <amorenoz@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 <amorenoz@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. */ > > > > I'm not sure if it is important, but this change bumps the > > cache_flow_key_map from the 1st to 2nd cacheline (on x86_64). > > > > This can be avoided by moving seq_number above collectors, > > as there is a 32-bit hole there. > > > > Thanks very much for looking at this Simon. > > Your proposal makes total sense. We should not degrade cache efficiency when > it's so easily avoidable. I'll add it to the patch. Likewise, thanks. > > Possibly there is further scope for making this structure more > > cache-fiendly. But again, I'm not sure if it is important. > > > Looking further optimizing this structure, I don't think there's huge space > for improvement. That may well be the case. TBH, I didn't look for further optimisations particularly closely. > The operation that we'd like to optimize would be > ipfix_cache_update() which also uses "ofproto_stats", that spans over 2 > cache lines. > I think the best we can do is: > > struct dpif_ipfix_exporter { > struct hmap cache_flow_key_map; /* 0 32 */ > struct ovs_list cache_flow_start_timestamp_list; /* 32 > 16 */ > uint32_t cache_max_flows; /* 48 4 */ > uint32_t cache_active_timeout; /* 52 4 */ > struct hmap domains; /* 56 32 */ > /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */ > time_t last_stats_sent_time; /* 88 8 */ > char * virtual_obs_id; /* 96 8 */ > struct collectors * collectors; /* 104 8 */ > uint32_t exporter_id; /* 112 4 */ > uint32_t seq_number; /* 116 4 */ > uint8_t virtual_obs_len; /* 120 1 */ > > /* XXX 7 bytes hole, try to pack */ > > /* --- cacheline 2 boundary (128 bytes) --- */ > ofproto_ipfix_stats ofproto_stats; /* 128 88 */ > /* --- cacheline 3 boundary (192 bytes) was 24 bytes ago --- */ > struct dpif_ipfix_global_stats ipfix_global_stats; /* 216 152 */ > > /* size: 368, cachelines: 6, members: 13 */ > /* sum members: 361, holes: 1, sum holes: 7 */ > /* last cacheline: 48 bytes */ > }; > > If we're lucky we can not make use of cacheline 2 during this operation. Not > sure that'll give us any significant benefit. > > My plan is to do some performance testing of the IPFIX collection. I will > take this into account. Yes, good plan. I agree it is a good idea to make such a change data driven.
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]) +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 +