At the moment ovs meters are reconfigured by ovn just when a
meter is allocated or removed 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
In order to fix the issue introduce SB_meter node in the incremetal
processing engine and add it as input for lflow_output one.
Sync OVS meters in ofctl_put() to push changes in SB meter/SB meter
bands tables.
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1939524
Acked-by: Mark Michelson <mmich...@redhat.com>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com>
Signed-off-by: Han Zhou <hz...@ovn.org>
---
controller/ofctrl.c | 212 +++++++++++++++++++++++++++++++-----
controller/ovn-controller.c | 25 ++++-
lib/extend-table.c | 14 +++
lib/extend-table.h | 4 +
tests/ovn-performance.at | 17 +++
tests/ovn.at | 73 +++++++++++++
tests/system-ovn.at | 17 +++
7 files changed, 332 insertions(+), 30 deletions(-)
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 08fcfed8b..22f80620a 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -335,6 +335,22 @@ static struct ovn_extend_table *groups;
/* A reference to the meter_table. */
static struct ovn_extend_table *meters;
+/* Installed meter bands. */
+struct meter_band_data {
+ int64_t burst_size;
+ int64_t rate;
+};
+
+struct meter_band_entry {
+ struct meter_band_data *bands;
+ size_t n_bands;
+};
+
+static struct shash meter_bands;
+
+static void ofctrl_meter_bands_destroy(void);
+static void ofctrl_meter_bands_clear(void);
+
/* MFF_* field ID for our Geneve option. In S_TLV_TABLE_MOD_SENT, this is
* the option we requested (we don't know whether we obtained it yet). In
* S_CLEAR_FLOWS or S_UPDATE_FLOWS, this is really the option we have. */
@@ -372,6 +388,7 @@ ofctrl_init(struct ovn_extend_table *group_table,
ovn_init_symtab(&symtab);
groups = group_table;
meters = meter_table;
+ shash_init(&meter_bands);
}
/* S_NEW, for a new connection.
@@ -615,6 +632,7 @@ run_S_CLEAR_FLOWS(void)
/* Clear existing meters, to match the state of the switch. */
if (meters) {
ovn_extend_table_clear(meters, true);
+ ofctrl_meter_bands_clear();
}
/* All flow updates are irrelevant now. */
@@ -789,6 +807,7 @@ ofctrl_destroy(void)
rconn_packet_counter_destroy(tx_counter);
expr_symtab_destroy(&symtab);
shash_destroy(&symtab);
+ ofctrl_meter_bands_destroy();
}
uint64_t
@@ -1802,26 +1821,14 @@ add_meter_string(struct ovn_extend_table_info
*m_desired,
}
static void
-add_meter(struct ovn_extend_table_info *m_desired,
- const struct sbrec_meter_table *meter_table,
- struct ovs_list *msgs)
+update_ovs_meter(struct ovn_extend_table_info *entry,
+ const struct sbrec_meter *sb_meter, int cmd,
+ struct ovs_list *msgs)
{
- 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.command = cmd;
+ mm.meter.meter_id = entry->table_id;
mm.meter.flags = OFPMF13_STATS;
if (!strcmp(sb_meter->unit, "pktps")) {
@@ -1854,6 +1861,152 @@ add_meter(struct ovn_extend_table_info *m_desired,
free(mm.meter.bands);
}
+static void
+ofctrl_meter_bands_clear(void)
+{
+ struct shash_node *node, *next;
+ SHASH_FOR_EACH_SAFE (node, next, &meter_bands) {
+ struct meter_band_entry *mb = node->data;
+ shash_delete(&meter_bands, node);
+ free(mb->bands);
+ free(mb);
+ }
+}
+
+static void
+ofctrl_meter_bands_destroy(void)
+{
+ ofctrl_meter_bands_clear();
+ shash_destroy(&meter_bands);
+}
+
+static bool
+ofctrl_meter_bands_is_equal(const struct sbrec_meter *sb_meter,
+ struct meter_band_entry *mb)
+{
+ if (mb->n_bands != sb_meter->n_bands) {
+ return false;
+ }
+
+ for (int i = 0; i < sb_meter->n_bands; i++) {
+ int j;
+ for (j = 0; j < mb->n_bands; j++) {
+ if (sb_meter->bands[i]->rate == mb->bands[j].rate &&
+ sb_meter->bands[i]->burst_size == mb->bands[j].burst_size) {
+ break;
+ }
+ }
+ if (j == mb->n_bands) {
+ return false;
+ }
+ }
+ return true;
+}
+
+static void
+ofctrl_meter_bands_alloc(const struct sbrec_meter *sb_meter,
+ struct ovn_extend_table_info *entry,
+ struct ovs_list *msgs)
+{
+ struct meter_band_entry *mb = mb = xzalloc(sizeof *mb);
+ mb->n_bands = sb_meter->n_bands;
+ mb->bands = xcalloc(mb->n_bands, sizeof *mb->bands);
+ for (int i = 0; i < sb_meter->n_bands; i++) {
+ mb->bands[i].rate = sb_meter->bands[i]->rate;
+ mb->bands[i].burst_size = sb_meter->bands[i]->burst_size;
+ }
+ shash_add(&meter_bands, entry->name, mb);
+ update_ovs_meter(entry, sb_meter, OFPMC13_ADD, msgs);
+}
+
+static void
+ofctrl_meter_bands_update(const struct sbrec_meter *sb_meter,
+ struct ovn_extend_table_info *entry,
+ struct ovs_list *msgs)
+{
+ struct meter_band_entry *mb =
+ shash_find_data(&meter_bands, entry->name);
+ if (!mb) {
+ ofctrl_meter_bands_alloc(sb_meter, entry, msgs);
+ return;
+ }
+
+ if (ofctrl_meter_bands_is_equal(sb_meter, mb)) {
+ return;
+ }
+
+ free(mb->bands);
+ mb->n_bands = sb_meter->n_bands;
+ mb->bands = xcalloc(mb->n_bands, sizeof *mb->bands);
+ for (int i = 0; i < sb_meter->n_bands; i++) {
+ mb->bands[i].rate = sb_meter->bands[i]->rate;
+ mb->bands[i].burst_size = sb_meter->bands[i]->burst_size;
+ }
+
+ update_ovs_meter(entry, sb_meter, OFPMC13_MODIFY, msgs);
+}
+
+static void
+ofctrl_meter_bands_erase(struct ovn_extend_table_info *entry,
+ struct ovs_list *msgs)
+{
+ struct meter_band_entry *mb =
+ shash_find_and_delete(&meter_bands, entry->name);
+ if (mb) {
+ /* Delete the meter. */
+ struct ofputil_meter_mod mm = {
+ .command = OFPMC13_DELETE,
+ .meter = { .meter_id = entry->table_id },
+ };
+ add_meter_mod(&mm, msgs);
+
+ free(mb->bands);
+ free(mb);
+ }
+}
+
+static void
+ofctrl_meter_bands_sync(struct ovn_extend_table_info *m_existing,
+ const struct sbrec_meter_table *meter_table,
+ struct ovs_list *msgs)
+{
+ const struct sbrec_meter *sb_meter;
+ SBREC_METER_TABLE_FOR_EACH (sb_meter, meter_table) {
+ if (!strcmp(m_existing->name, sb_meter->name)) {
+ break;
+ }
+ }
+
+ if (sb_meter) {
+ /* OFPMC13_ADD or OFPMC13_MODIFY */
+ ofctrl_meter_bands_update(sb_meter, m_existing, msgs);
+ } else {
+ /* OFPMC13_DELETE */
+ ofctrl_meter_bands_erase(m_existing, msgs);
+ }
+}
+
+static void
+add_meter(struct ovn_extend_table_info *m_desired,
+ const struct sbrec_meter_table *meter_table,
+ struct ovs_list *msgs)
+{
+ 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;
+ }
+
+ ofctrl_meter_bands_alloc(sb_meter, m_desired, msgs);
+}
+
static void
installed_flow_add(struct ovn_flow *d,
struct ofputil_bundle_ctrl_msg *bc,
@@ -2232,13 +2385,19 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
/* Iterate through all the desired meters. If there are new ones,
* add them to the switch. */
struct ovn_extend_table_info *m_desired;
- EXTEND_TABLE_FOR_EACH_UNINSTALLED (m_desired, meters) {
- if (!strncmp(m_desired->name, "__string: ", 10)) {
- /* The "set-meter" action creates a meter entry name that
- * describes the meter itself. */
- add_meter_string(m_desired, &msgs);
+ HMAP_FOR_EACH (m_desired, hmap_node, &meters->desired) {
+ struct ovn_extend_table_info *m_existing =
+ ovn_extend_table_lookup(&meters->existing, m_desired);
+ if (!m_existing) {
+ if (!strncmp(m_desired->name, "__string: ", 10)) {
+ /* 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);
+ }
} else {
- add_meter(m_desired, meter_table, &msgs);
+ ofctrl_meter_bands_sync(m_existing, meter_table, &msgs);
}
}
@@ -2328,12 +2487,7 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
struct ovn_extend_table_info *m_installed, *next_meter;
EXTEND_TABLE_FOR_EACH_INSTALLED (m_installed, next_meter, meters) {
/* Delete the meter. */
- struct ofputil_meter_mod mm = {
- .command = OFPMC13_DELETE,
- .meter = { .meter_id = m_installed->table_id },
- };
- add_meter_mod(&mm, &msgs);
-
+ ofctrl_meter_bands_erase(m_installed, &msgs);
ovn_extend_table_remove_existing(meters, m_installed);
}
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 5069aedfc..f85af9353 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -962,7 +962,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,
@@ -2713,6 +2714,26 @@ lflow_output_sb_fdb_handler(struct engine_node *node,
void *data)
return handled;
}
+static bool
+lflow_output_sb_meter_handler(struct engine_node *node, void *data)
+{
+ struct ed_type_lflow_output *fo = data;
+ struct sbrec_meter_table *meter_table =
+ (struct sbrec_meter_table *)EN_OVSDB_GET(
+ engine_get_input("SB_meter", node));
+
+ const struct sbrec_meter *iter;
+ SBREC_METER_TABLE_FOR_EACH_TRACKED (iter, meter_table) {
+ if (ovn_extend_table_desired_lookup_by_name(&fo->meter_table,
+ iter->name)) {
+ engine_set_node_state(node, EN_UPDATED);
+ break;
+ }
+ }
+
+ return true;
+}
+
struct ed_type_pflow_output {
/* Desired physical flows. */
struct ovn_desired_flow_table flow_table;
@@ -3303,6 +3324,8 @@ main(int argc, char *argv[])
lflow_output_sb_load_balancer_handler);
engine_add_input(&en_lflow_output, &en_sb_fdb,
lflow_output_sb_fdb_handler);
+ engine_add_input(&en_lflow_output, &en_sb_meter,
+ lflow_output_sb_meter_handler);
engine_add_input(&en_ct_zones, &en_ovs_open_vswitch, NULL);
engine_add_input(&en_ct_zones, &en_ovs_bridge, NULL);
diff --git a/lib/extend-table.c b/lib/extend-table.c
index c708e24b9..32d541b55 100644
--- a/lib/extend-table.c
+++ b/lib/extend-table.c
@@ -337,3 +337,17 @@ ovn_extend_table_assign_id(struct ovn_extend_table *table,
const char *name,
return table_id;
}
+
+struct ovn_extend_table_info *
+ovn_extend_table_desired_lookup_by_name(struct ovn_extend_table * table,
+ const char *name)
+{
+ uint32_t hash = hash_string(name, 0);
+ struct ovn_extend_table_info *m_desired;
+ HMAP_FOR_EACH_WITH_HASH (m_desired, hmap_node, hash, &table->desired) {
+ if (!strcmp(m_desired->name, name)) {
+ return m_desired;
+ }
+ }
+ return NULL;
+}
diff --git a/lib/extend-table.h b/lib/extend-table.h
index 4d80cfd80..6240b946e 100644
--- a/lib/extend-table.h
+++ b/lib/extend-table.h
@@ -94,6 +94,10 @@ uint32_t ovn_extend_table_assign_id(struct ovn_extend_table
*,
const char *name,
struct uuid lflow_uuid);
+struct ovn_extend_table_info *
+ovn_extend_table_desired_lookup_by_name(struct ovn_extend_table * table,
+ const char *name);
+
/* Iterates 'DESIRED' through all of the 'ovn_extend_table_info's in
* 'TABLE'->desired that are not in 'TABLE'->existing. (The loop body
* presumably adds them.) */
diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at
index 10341ad72..f92fbebf1 100644
--- a/tests/ovn-performance.at
+++ b/tests/ovn-performance.at
@@ -543,6 +543,23 @@ OVN_CONTROLLER_EXPECT_HIT(
[as hv3 ovs-vsctl set interface vgw3 external-ids:ovn-egress-iface=true]
)
+ovn-nbctl --wait=hv meter-add meter0 drop 100 pktps 10
+
+OVN_CONTROLLER_EXPECT_NO_HIT(
+ [hv1 hv2 hv3 hv4], [lflow_run],
+ [ovn-nbctl --wait=hv lr-copp-add lr1 arp meter0]
+)
+
+OVN_CONTROLLER_EXPECT_NO_HIT(
+ [hv1 hv2 hv3 hv4], [lflow_run],
+ [ovn-nbctl --wait=hv --may-exist meter-add meter0 drop 200 pktps 10]
+)
+
+OVN_CONTROLLER_EXPECT_NO_HIT(
+ [hv1 hv2 hv3 hv4], [lflow_run],
+ [ovn-nbctl --wait=hv meter-del meter0]
+)
+
for i in 1 2; do
j=$((i%2 + 1))
lp=lp$i
diff --git a/tests/ovn.at b/tests/ovn.at
index 52a1758c7..5625f7767 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -29768,3 +29768,76 @@ OVS_WAIT_UNTIL([
OVN_CLEANUP([hv1],[hv2])
AT_CLEANUP
])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn-controller - check meters update])
+AT_KEYWORDS([meters-update])
+
+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 --event lb-add lb0 192.168.1.100:80 ""
+check ovn-nbctl ls-lb-add sw0 lb0
+check ovn-nbctl meter-add meter0 drop 10 pktps
+ovn-nbctl --wait=hv ls-copp-add sw0 event-elb meter0
+check ovn-nbctl --wait=hv sync
+
+AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q rate=10],
[0])
+AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -q meter_id=1], [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])
+
+# Add a new meter
+check ovn-nbctl meter-add meter1 drop 30 pktps
+check ovn-nbctl --log --severity=alert --meter=meter1 \
+ --name=dns acl-add sw0 to-lport 1000 'udp.dst == 53' drop
+check ovn-nbctl --wait=hv sync
+AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q rate=30],
[0])
+AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -q meter_id=2], [0])
+
+# Remove meter0
+check ovn-nbctl meter-del meter0
+check ovn-nbctl --wait=hv sync
+AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q rate=10],
[1])
+AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -q meter_id=1], [1])
+
+check ovn-nbctl meter-del meter1
+check ovn-nbctl --wait=hv sync
+AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q rate=30],
[1])
+
+# create meters in the opposite order
+check ovn-nbctl --log --severity=alert --meter=meter2 \
+ --name=dns acl-add sw0 to-lport 1000 'tcp.dst == 80' drop
+check ovn-nbctl meter-add meter2 drop 100 pktps
+check ovn-nbctl --wait=hv sync
+AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q
rate=100], [0])
+AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -q meter_id=1], [0])
+
+check ovn-nbctl meter-del meter2
+AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q
rate=100], [1])
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+])
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index dd2c1800a..5f41722d9 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -6927,8 +6927,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