diff options
| -rw-r--r-- | ofproto/ofproto-dpif-ipfix.c | 51 | ||||
| -rw-r--r-- | ofproto/ofproto-dpif-ipfix.h | 2 | ||||
| -rw-r--r-- | ofproto/ofproto-dpif.c | 7 | ||||
| -rw-r--r-- | tests/ofproto-dpif.at | 17 |
4 files changed, 54 insertions, 23 deletions
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c index fc927fe86..742eed399 100644 --- a/ofproto/ofproto-dpif-ipfix.c +++ b/ofproto/ofproto-dpif-ipfix.c @@ -926,17 +926,21 @@ dpif_ipfix_bridge_exporter_destroy(struct dpif_ipfix_bridge_exporter *exporter) static void dpif_ipfix_bridge_exporter_set_options( struct dpif_ipfix_bridge_exporter *exporter, - const struct ofproto_ipfix_bridge_exporter_options *options) + const struct ofproto_ipfix_bridge_exporter_options *options, + bool *options_changed) { - bool options_changed; - if (!options || sset_is_empty(&options->targets)) { /* No point in doing any work if there are no targets. */ - dpif_ipfix_bridge_exporter_clear(exporter); + if (exporter->options) { + dpif_ipfix_bridge_exporter_clear(exporter); + *options_changed = true; + } else { + *options_changed = false; + } return; } - options_changed = ( + *options_changed = ( !exporter->options || !ofproto_ipfix_bridge_exporter_options_equal( options, exporter->options)); @@ -945,7 +949,7 @@ dpif_ipfix_bridge_exporter_set_options( * shortchanged in collectors (which indicates that opening one or * more of the configured collectors failed, so that we should * retry). */ - if (options_changed + if (*options_changed || collectors_count(exporter->exporter.collectors) < sset_count(&options->targets)) { if (!dpif_ipfix_exporter_set_options( @@ -957,7 +961,7 @@ dpif_ipfix_bridge_exporter_set_options( } /* Avoid reconfiguring if options didn't change. */ - if (!options_changed) { + if (!*options_changed) { return; } @@ -1015,17 +1019,21 @@ dpif_ipfix_flow_exporter_destroy(struct dpif_ipfix_flow_exporter *exporter) static bool dpif_ipfix_flow_exporter_set_options( struct dpif_ipfix_flow_exporter *exporter, - const struct ofproto_ipfix_flow_exporter_options *options) + const struct ofproto_ipfix_flow_exporter_options *options, + bool *options_changed) { - bool options_changed; - if (sset_is_empty(&options->targets)) { /* No point in doing any work if there are no targets. */ - dpif_ipfix_flow_exporter_clear(exporter); + if (exporter->options) { + dpif_ipfix_flow_exporter_clear(exporter); + *options_changed = true; + } else { + *options_changed = false; + } return true; } - options_changed = ( + *options_changed = ( !exporter->options || !ofproto_ipfix_flow_exporter_options_equal( options, exporter->options)); @@ -1034,7 +1042,7 @@ dpif_ipfix_flow_exporter_set_options( * shortchanged in collectors (which indicates that opening one or * more of the configured collectors failed, so that we should * retry). */ - if (options_changed + if (*options_changed || collectors_count(exporter->exporter.collectors) < sset_count(&options->targets)) { if (!dpif_ipfix_exporter_set_options( @@ -1046,7 +1054,7 @@ dpif_ipfix_flow_exporter_set_options( } /* Avoid reconfiguring if options didn't change. */ - if (!options_changed) { + if (!*options_changed) { return true; } @@ -1069,7 +1077,7 @@ remove_flow_exporter(struct dpif_ipfix *di, free(node); } -void +bool dpif_ipfix_set_options( struct dpif_ipfix *di, const struct ofproto_ipfix_bridge_exporter_options *bridge_exporter_options, @@ -1077,16 +1085,19 @@ dpif_ipfix_set_options( size_t n_flow_exporters_options) OVS_EXCLUDED(mutex) { int i; + bool beo_changed, feo_changed, entry_changed; struct ofproto_ipfix_flow_exporter_options *options; struct dpif_ipfix_flow_exporter_map_node *node; ovs_mutex_lock(&mutex); dpif_ipfix_bridge_exporter_set_options(&di->bridge_exporter, - bridge_exporter_options); + bridge_exporter_options, + &beo_changed); /* Add new flow exporters and update current flow exporters. */ options = (struct ofproto_ipfix_flow_exporter_options *) flow_exporters_options; + feo_changed = false; for (i = 0; i < n_flow_exporters_options; i++) { node = dpif_ipfix_find_flow_exporter_map_node( di, options->collector_set_id); @@ -1095,10 +1106,14 @@ dpif_ipfix_set_options( dpif_ipfix_flow_exporter_init(&node->exporter); hmap_insert(&di->flow_exporter_map, &node->node, hash_int(options->collector_set_id, 0)); + feo_changed = true; } - if (!dpif_ipfix_flow_exporter_set_options(&node->exporter, options)) { + if (!dpif_ipfix_flow_exporter_set_options(&node->exporter, + options, + &entry_changed)) { remove_flow_exporter(di, node); } + feo_changed = entry_changed ? true : feo_changed; options++; } @@ -1117,10 +1132,12 @@ dpif_ipfix_set_options( } if (i == n_flow_exporters_options) { /* Not found. */ remove_flow_exporter(di, node); + feo_changed = true; } } ovs_mutex_unlock(&mutex); + return beo_changed || feo_changed; } struct dpif_ipfix * diff --git a/ofproto/ofproto-dpif-ipfix.h b/ofproto/ofproto-dpif-ipfix.h index 1f42cd527..75c0ab81a 100644 --- a/ofproto/ofproto-dpif-ipfix.h +++ b/ofproto/ofproto-dpif-ipfix.h @@ -48,7 +48,7 @@ bool dpif_ipfix_get_bridge_exporter_output_sampling(const struct dpif_ipfix *); bool dpif_ipfix_get_flow_exporter_tunnel_sampling(const struct dpif_ipfix *, const uint32_t); bool dpif_ipfix_is_tunnel_port(const struct dpif_ipfix *, odp_port_t); -void dpif_ipfix_set_options( +bool dpif_ipfix_set_options( struct dpif_ipfix *, const struct ofproto_ipfix_bridge_exporter_options *, const struct ofproto_ipfix_flow_exporter_options *, size_t); diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index d7544ef69..28dcb8f86 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -2339,6 +2339,7 @@ set_ipfix( struct dpif_ipfix *di = ofproto->ipfix; bool has_options = bridge_exporter_options || flow_exporters_options; bool new_di = false; + bool options_changed = false; if (has_options && !di) { di = ofproto->ipfix = dpif_ipfix_create(); @@ -2348,7 +2349,7 @@ set_ipfix( if (di) { /* Call set_options in any case to cleanly flush the flow * caches in the last exporters that are to be destroyed. */ - dpif_ipfix_set_options( + options_changed = dpif_ipfix_set_options( di, bridge_exporter_options, flow_exporters_options, n_flow_exporters_options); @@ -2365,9 +2366,7 @@ set_ipfix( ofproto->ipfix = NULL; } - /* TODO: need to consider ipfix option changes more than - * enable/disable */ - if (new_di || !ofproto->ipfix) { + if (new_di || options_changed) { ofproto->backer->need_revalidate = REV_RECONFIGURE; } } diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index dbb3b6dda..8504c21b1 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -7629,13 +7629,28 @@ dnl configure bridge IPFIX and ensure that sample action generation works at the dnl datapath level. AT_SETUP([ofproto-dpif - Bridge IPFIX sanity check]) OVS_VSWITCHD_START +dnl first revalidation triggered by add interface +AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl +1 +]) + add_of_ports br0 1 2 3 +AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl +2 +]) dnl Sample every packet using bridge-based sampling. AT_CHECK([ovs-vsctl -- set bridge br0 ipfix=@fix -- \ --id=@fix create ipfix targets=\"127.0.0.1:4739\" \ - sampling=1], [0], [ignore]) + sampling=2], [0], [ignore]) +AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl +3 +]) +AT_CHECK([ovs-vsctl set ipfix `ovs-vsctl get bridge br0 ipfix` sampling=1], [0]) +AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl +4 +]) dnl Send some packets that should be sampled. for i in `seq 1 3`; do AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)']) |