I'm pretty certain that this code can miss meter updates in the
southbound database. I have additional notes below.
On 1/14/22 13:01, Lorenzo Bianconi wrote:
At the moment ovs meters are reconfigured by ovn just when a
new a meter is allocated while updates for an already allocated meter are
ignored. This issue can be easily verified with the following reproducer:
$ovn-nbctl meter-add meter0 drop 10 pktps
$ovn-nbctl --log --meter=meter0 acl-add sw0 to-lport 1000 'tcp.dst == 80' drop
$ovn-nbctl --may-exist meter-add meter0 drop 20 pktps
$ovs-ofctl -O OpenFlow15 dump-meters br-int
Fix the issue reconfiguring ovs meters even for ovn meters updates.
Related bz: https://bugzilla.redhat.com/show_bug.cgi?id=1939524
Signed-off-by: Lorenzo Bianconi <[email protected]>
---
controller/ofctrl.c | 77 ++++++++++++++++++++++---------------
controller/ofctrl.h | 4 +-
controller/ovn-controller.c | 72 +++++++++++++++++++++++++++++++++-
tests/ovn.at | 53 ++++++++++++++++++++++++-
tests/system-ovn.at | 17 ++++++++
5 files changed, 188 insertions(+), 35 deletions(-)
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index bf715787e..14ca08e94 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -1810,40 +1810,26 @@ uint32_t ofctrl_get_meter_id(const char *name, bool
new_id)
return id;
}
-static void
-add_meter(struct ovn_extend_table_info *m_desired,
- const struct sbrec_meter_table *meter_table,
- struct ovs_list *msgs)
+void
+set_meter(const struct sbrec_meter *meter, uint32_t id, int cmd)
{
- const struct sbrec_meter *sb_meter;
- SBREC_METER_TABLE_FOR_EACH (sb_meter, meter_table) {
- if (!strcmp(m_desired->name, sb_meter->name)) {
- break;
- }
- }
-
- if (!sb_meter) {
- static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
- VLOG_ERR_RL(&rl, "could not find meter named \"%s\"", m_desired->name);
- return;
- }
-
- struct ofputil_meter_mod mm;
- mm.command = OFPMC13_ADD;
- mm.meter.meter_id = m_desired->table_id;
- mm.meter.flags = OFPMF13_STATS;
+ struct ovs_list msgs = OVS_LIST_INITIALIZER(&msgs);
+ struct ofputil_meter_mod mm = {
+ .command = cmd,
+ .meter.meter_id = id,
+ .meter.flags = OFPMF13_STATS,
+ };
- if (!strcmp(sb_meter->unit, "pktps")) {
+ if (!strcmp(meter->unit, "pktps")) {
mm.meter.flags |= OFPMF13_PKTPS;
} else {
mm.meter.flags |= OFPMF13_KBPS;
}
-
- mm.meter.n_bands = sb_meter->n_bands;
+ mm.meter.n_bands = meter->n_bands;
mm.meter.bands = xcalloc(mm.meter.n_bands, sizeof *mm.meter.bands);
- for (size_t i = 0; i < sb_meter->n_bands; i++) {
- struct sbrec_meter_band *sb_band = sb_meter->bands[i];
+ for (size_t i = 0; i < meter->n_bands; i++) {
+ struct sbrec_meter_band *sb_band = meter->bands[i];
struct ofputil_meter_band *mm_band = &mm.meter.bands[i];
if (!strcmp(sb_band->action, "drop")) {
@@ -1858,9 +1844,41 @@ add_meter(struct ovn_extend_table_info *m_desired,
mm.meter.flags |= OFPMF13_BURST;
}
}
-
- add_meter_mod(&mm, msgs);
+ add_meter_mod(&mm, &msgs);
free(mm.meter.bands);
+
+ if (!ovs_list_is_empty(&msgs)) {
+ /* Add a barrier to the list of messages. */
+ struct ofpbuf *barrier = ofputil_encode_barrier_request(OFP15_VERSION);
+
+ ovs_list_push_back(&msgs, &barrier->list_node);
+ /* Queue the messages. */
+ struct ofpbuf *msg;
+ LIST_FOR_EACH_POP (msg, list_node, &msgs) {
+ queue_msg(msg);
+ }
+ }
+}
+
+void
+remove_meter(uint32_t id)
+{
+ struct ovs_list msgs = OVS_LIST_INITIALIZER(&msgs);
+ /* Delete the meter. */
+ struct ofputil_meter_mod mm = {
+ .command = OFPMC13_DELETE,
+ .meter = { .meter_id = id },
+ };
+ add_meter_mod(&mm, &msgs);
+ /* Add a barrier to the list of messages. */
+ struct ofpbuf *barrier = ofputil_encode_barrier_request(OFP15_VERSION);
+
+ ovs_list_push_back(&msgs, &barrier->list_node);
+ /* Queue the messages. */
+ struct ofpbuf *msg;
+ LIST_FOR_EACH_POP (msg, list_node, &msgs) {
+ queue_msg(msg);
+ }
}
static void
@@ -2161,7 +2179,6 @@ void
ofctrl_put(struct ovn_desired_flow_table *lflow_table,
struct ovn_desired_flow_table *pflow_table,
struct shash *pending_ct_zones,
- const struct sbrec_meter_table *meter_table,
uint64_t req_cfg,
bool lflows_changed,
bool pflows_changed)
@@ -2246,8 +2263,6 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
/* The "set-meter" action creates a meter entry name that
* describes the meter itself. */
add_meter_string(m_desired, &msgs);
- } else {
- add_meter(m_desired, meter_table, &msgs);
}
}
diff --git a/controller/ofctrl.h b/controller/ofctrl.h
index 0efcb2ef5..53af92cfb 100644
--- a/controller/ofctrl.h
+++ b/controller/ofctrl.h
@@ -29,6 +29,7 @@ struct match;
struct ofpbuf;
struct ovsrec_bridge;
struct sbrec_meter_table;
+struct sbrec_meter;
struct shash;
struct ovn_desired_flow_table {
@@ -55,7 +56,6 @@ enum mf_field_id ofctrl_get_mf_field_id(void);
void ofctrl_put(struct ovn_desired_flow_table *lflow_table,
struct ovn_desired_flow_table *pflow_table,
struct shash *pending_ct_zones,
- const struct sbrec_meter_table *,
uint64_t nb_cfg,
bool lflow_changed,
bool pflow_changed);
@@ -130,5 +130,7 @@ bool ofctrl_is_connected(void);
void ofctrl_set_probe_interval(int probe_interval);
void ofctrl_get_memory_usage(struct simap *usage);
uint32_t ofctrl_get_meter_id(const char *name, bool new_id);
+void set_meter(const struct sbrec_meter *meter, uint32_t id, int cmd);
+void remove_meter(uint32_t id);
#endif /* controller/ofctrl.h */
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 86cb6f769..ac332ff94 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -76,6 +76,7 @@
#include "stopwatch.h"
#include "lib/inc-proc-eng.h"
#include "hmapx.h"
+#include "openvswitch/ofp-util.h"
VLOG_DEFINE_THIS_MODULE(main);
@@ -115,6 +116,7 @@ static unixctl_cb_func debug_delay_nb_cfg_report;
#define OVS_STARTUP_TS_NAME "ovn-startup-ts"
static struct engine *flow_engine;
+static struct engine *meter_engine;
static char *parse_options(int argc, char *argv[]);
OVS_NO_RETURN static void usage(void);
@@ -964,7 +966,8 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
SB_NODE(dhcpv6_options, "dhcpv6_options") \
SB_NODE(dns, "dns") \
SB_NODE(load_balancer, "load_balancer") \
- SB_NODE(fdb, "fdb")
+ SB_NODE(fdb, "fdb") \
+ SB_NODE(meter, "meter")
enum sb_engine_node {
#define SB_NODE(NAME, NAME_STR) SB_##NAME,
@@ -1509,6 +1512,65 @@ addr_sets_sb_address_set_handler(struct engine_node
*node, void *data)
return true;
}
+struct ed_type_meter {
+ bool change_tracked;
This field isn't used for anything as far as I can tell.
+};
+
+static void *
+en_meter_init(struct engine_node *node OVS_UNUSED,
+ struct engine_arg *arg OVS_UNUSED)
+{
+ struct ed_type_meter *m = xzalloc(sizeof *m);
+
+ m->change_tracked = false;
+ return m;
+}
+
+static void
+en_meter_cleanup(void *data OVS_UNUSED)
+{
+}
+
+static void
+en_meter_run(struct engine_node *node, void *data)
+{
+ struct ed_type_meter *m = data;
+
+ engine_set_node_state(node, EN_UPDATED);
+ m->change_tracked = false;
The engine_node's run() function is intended to perform a full
computation of the data. It is called when the engine has encountered a
condition where a recompute is required. It then should set relevant
data on the ed_type_meter structure so that the change handler can
detect what has changed when processing SB record changes in the future.
This run() function doesn't do anything with the meter data. This means
that an engine recompute will not process the southbound meter data at all.
+}
+
+static bool
+meter_sb_meter_handler(struct engine_node *node, void *data)
+{
+ struct ovs_list msgs = OVS_LIST_INITIALIZER(&msgs);
+ struct ed_type_meter *m = data;
+
+ struct sbrec_meter_table *m_table =
+ (struct sbrec_meter_table *)EN_OVSDB_GET(
+ engine_get_input("SB_meter", node));
+
+ uint32_t id;
+ const struct sbrec_meter *iter;
+ SBREC_METER_TABLE_FOR_EACH_TRACKED (iter, m_table) {
+ id = ofctrl_get_meter_id(iter->name, !sbrec_meter_is_deleted(iter));
+ if (id == EXT_TABLE_ID_INVALID) {
+ return false;
+ }
+
+ if (sbrec_meter_is_deleted(iter)) {
+ remove_meter(id);
Performing openflow changes directly in this loop is not a great idea.
Consider that at some point mid-loop, an error is encountered. If you
have been queuing OF changes directly, then the result can be a
partially-set (and possibly invalid) state in OVS.
Instead, it is a good idea to wait until after this loop is complete to
do so. This way, if an error is encountered mid-loop, you can fall back
to a full recompute instead without having performed any changes in OVS.
+ } else {
+ int cmd = sbrec_meter_is_new(iter) ? OFPMC13_ADD : OFPMC13_MODIFY;
+
+ set_meter(iter, id, cmd);
+ }
+ }
+ m->change_tracked = true;
+
+ return true;
+}
+
struct ed_type_port_groups{
/* A copy of SB port_groups, each converted as a sset for efficient lport
* lookup. */
@@ -3223,6 +3285,7 @@ main(int argc, char *argv[])
ENGINE_NODE(lflow_output, "logical_flow_output");
ENGINE_NODE(flow_output, "flow_output");
ENGINE_NODE(addr_sets, "addr_sets");
+ ENGINE_NODE(meter, "meter");
ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups");
#define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR);
@@ -3347,11 +3410,14 @@ main(int argc, char *argv[])
engine_add_input(&en_flow_output, &en_pflow_output,
flow_output_pflow_output_handler);
+ engine_add_input(&en_meter, &en_sb_meter, meter_sb_meter_handler);
+
struct engine_arg engine_arg = {
.sb_idl = ovnsb_idl_loop.idl,
.ovs_idl = ovs_idl_loop.idl,
};
flow_engine = engine_new(&en_flow_output, &engine_arg, "flow_engine");
+ meter_engine = engine_new(&en_meter, &engine_arg, "meter_engine");
engine_ovsdb_node_add_index(&en_sb_chassis, "name", sbrec_chassis_by_name);
engine_ovsdb_node_add_index(&en_sb_multicast_group, "name_datapath",
@@ -3488,6 +3554,7 @@ main(int argc, char *argv[])
}
engine_init_run(flow_engine);
+ engine_init_run(meter_engine);
struct ovsdb_idl_txn *ovs_idl_txn = ovsdb_idl_loop_run(&ovs_idl_loop);
unsigned int new_ovs_cond_seqno
@@ -3594,6 +3661,7 @@ main(int argc, char *argv[])
engine_set_force_recompute(flow_engine, true);
}
+ engine_run(meter_engine, true);
if (br_int) {
ct_zones_data = engine_get_data(&en_ct_zones);
if (ct_zones_data) {
@@ -3761,7 +3829,6 @@ main(int argc, char *argv[])
ofctrl_put(&lflow_output_data->flow_table,
&pflow_output_data->flow_table,
&ct_zones_data->pending,
- sbrec_meter_table_get(ovnsb_idl_loop.idl),
ofctrl_seqno_get_req_cfg(),
engine_node_changed(&en_lflow_output),
engine_node_changed(&en_pflow_output));
@@ -3905,6 +3972,7 @@ loop_done:
engine_set_context(flow_engine, NULL);
engine_cleanup(flow_engine);
+ engine_cleanup(meter_engine);
/* It's time to exit. Clean up the databases if we are not restarting */
if (!restart) {
diff --git a/tests/ovn.at b/tests/ovn.at
index 92e284e8a..9a5e6b196 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -9181,7 +9181,7 @@ ovn-nbctl acl-del lsw0 to-lport 1000 'tcp.dst==80'
ovn-nbctl --wait=hv sync
AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br-int | grep meter], [0],
[ignore], [ignore])
-# Delete acl2, meter should be deleted in OVS
+# Delete acl2, meter should be kept in OVS
ovn-nbctl acl-del lsw0 to-lport 1000 'tcp.dst==81'
ovn-nbctl --wait=hv sync
AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br-int | grep meter], [1])
@@ -29704,3 +29704,54 @@ OVS_WAIT_UNTIL([
OVN_CLEANUP([hv1],[hv2])
AT_CLEANUP
])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn-controller - check meters update])
+AT_KEYWORDS([meters-update])
This is a good idea for a test, since it ensures that changes to a meter
are reflected in OVS.
But I also think test cases should be added to the "ovn-controller
incremental processing" test in ovn-performance.at to ensure that
changes are happening incrementally as expected.
+
+ovn_start
+
+net_add n1
+sim_add hv1
+as hv1
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.10
+
+check ovn-nbctl ls-add sw0
+check ovn-nbctl lsp-add sw0 lsp
+
+as hv1 ovs-vsctl \
+ -- add-port br-int vif1 \
+ -- set Interface vif1 external_ids:iface-id=lsp
+
+# Wait for port to be bound.
+wait_row_count Chassis 1 name=hv1
+ch=$(fetch_column Chassis _uuid name=hv1)
+wait_row_count Port_Binding 1 logical_port=lsp chassis=$ch
+
+# Add a new meter
+check ovn-nbctl meter-add meter0 drop 10 pktps
+check ovn-nbctl --log --severity=alert --meter=meter0 --name=http acl-add sw0 \
+ to-lport 1000 'tcp.dst == 80' drop
+check ovn-nbctl --wait=hv sync
+
+AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q rate=10],
[0])
+
+# Update existing meter
+check ovn-nbctl --may-exist meter-add meter0 drop 20 pktps
+check ovn-nbctl --wait=hv sync
+
+AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q rate=20],
[0])
+
+# Update existing meter and do a full recompute
+as hv1 ovn-appctl -t ovn-controller debug/pause
+check ovn-nbctl --may-exist meter-add meter0 drop 30 pktps
+check ovn-sbctl chassis-add hv2 geneve 127.0.0.1
+check ovn-nbctl --wait=sb sync
+as hv1 ovn-appctl -t ovn-controller debug/resume
I don't think this properly tests a recompute. Since the SB Meter table
and SB Chassis table are handled by different incremental engines,
adding a chassis doesn't result in a recompute of the meter incremental
engine. Instead, the change handler in the meter incremental engine
should be able to handle the change incrementally, just like it did with
previous changes.
My worry is that since the run() function for the en_meter node does not
actually compute meters, a recompute will not result in any reading of
the SB data.
I think a good test for this would be to pre-populate the SB database
with meter information, then start ovn-controller. I suspect that OVS
will not have the meters installed. Even if you then performed
`ovn-appctl -t ovn-controller inc-engine/recompute` I think the meters
would still not be installed. If you then added or changed a meter, you
would see the changes happen on the added/changed meters, but any
unchanged meters in the SB database would still not be installed in OVS.
+
+AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q rate=30],
[0])
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+])
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 7f6cb32dc..80d5192ca 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -6667,8 +6667,25 @@ OVS_WAIT_UNTIL([
test "${n_reject}" = "2"
])
kill $(pidof tcpdump)
+rm -f reject.pcap
+
+# Let's update the meter
+NS_EXEC([sw01], [tcpdump -l -n -i sw01 icmp -Q in > reject.pcap &])
+check ovn-nbctl --may-exist meter-add acl-meter drop 10 pktps 0
+ip netns exec sw01 scapy -H <<-EOF
+p = IP(src="192.168.1.2", dst="192.168.1.1") / UDP(dport = 12345) /
Raw(b"X"*64)
+send (p, iface='sw01', loop = 0, verbose = 0, count = 100)
+EOF
+# 10pps + 1 burst size
+OVS_WAIT_UNTIL([
+ n_reject=$(grep unreachable reject.pcap | wc -l)
+ test "${n_reject}" = "20"
+])
+
+kill $(pidof tcpdump)
rm -f reject.pcap
+
NS_EXEC([sw01], [tcpdump -l -n -i sw01 icmp -Q in > reject.pcap &])
check ovn-nbctl --wait=hv ls-copp-del sw0 reject
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
