[ovs-dev] [PATCH ovn] controller: reconfigure ovs meters for ovn meters updates

Lorenzo Bianconi lorenzo.bianconi at redhat.com
Thu Nov 25 17:05:05 UTC 2021


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 <lorenzo.bianconi at redhat.com>
---
 controller/ofctrl.c         | 70 +++++++++++++++++++++++++++----------
 controller/ofctrl.h         |  2 ++
 controller/ovn-controller.c | 56 ++++++++++++++++++++++++++++-
 tests/system-ovn.at         | 17 +++++++++
 4 files changed, 126 insertions(+), 19 deletions(-)

diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 08fcfed8b..25c939fa3 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -1802,26 +1802,13 @@ 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)
+alloc_meter_mod(struct ovn_extend_table_info *m,
+                const struct sbrec_meter *sb_meter,
+                struct ovs_list *msgs, bool update)
 {
-    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 = update ? OFPMC13_MODIFY : OFPMC13_ADD;
+    mm.meter.meter_id = m->table_id;
     mm.meter.flags = OFPMF13_STATS;
 
     if (!strcmp(sb_meter->unit, "pktps")) {
@@ -1854,6 +1841,53 @@ add_meter(struct ovn_extend_table_info *m_desired,
     free(mm.meter.bands);
 }
 
+void
+update_meter(const struct sbrec_meter *meter)
+{
+    struct ovs_list msgs = OVS_LIST_INITIALIZER(&msgs);
+    struct ovn_extend_table_info *m_iter;
+
+    HMAP_FOR_EACH (m_iter, hmap_node, &meters->desired) {
+        if (!strcmp(meter->name, m_iter->name) &&
+            ovn_extend_table_lookup(&meters->existing, m_iter)) {
+            alloc_meter_mod(m_iter, meter, &msgs, true);
+        }
+    }
+
+    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);
+        }
+    }
+}
+
+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;
+    }
+
+    alloc_meter_mod(m_desired, sb_meter, msgs, false);
+}
+
 static void
 installed_flow_add(struct ovn_flow *d,
                    struct ofputil_bundle_ctrl_msg *bc,
diff --git a/controller/ofctrl.h b/controller/ofctrl.h
index 014de210d..60eeb9dbb 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 {
@@ -129,5 +130,6 @@ void ofctrl_check_and_add_flow_metered(struct ovn_desired_flow_table *,
 bool ofctrl_is_connected(void);
 void ofctrl_set_probe_interval(int probe_interval);
 void ofctrl_get_memory_usage(struct simap *usage);
+void update_meter(const struct sbrec_meter *meter);
 
 #endif /* controller/ofctrl.h */
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 29c1a05b2..f2fa6aee8 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);
 
@@ -957,7 +958,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,
@@ -1500,6 +1502,55 @@ addr_sets_sb_address_set_handler(struct engine_node *node, void *data)
     return true;
 }
 
+struct ed_type_meter {
+    bool change_tracked;
+};
+
+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;
+}
+
+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));
+
+    const struct sbrec_meter *iter;
+    SBREC_METER_TABLE_FOR_EACH_TRACKED (iter, m_table) {
+        if (!sbrec_meter_is_deleted(iter) && !sbrec_meter_is_new(iter)) {
+            update_meter(iter);
+        }
+    }
+    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. */
@@ -3202,6 +3253,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);
@@ -3216,6 +3268,7 @@ main(int argc, char *argv[])
 
     engine_add_input(&en_addr_sets, &en_sb_address_set,
                      addr_sets_sb_address_set_handler);
+    engine_add_input(&en_meter, &en_sb_meter, meter_sb_meter_handler);
     engine_add_input(&en_port_groups, &en_sb_port_group,
                      port_groups_sb_port_group_handler);
     /* port_groups computation requires runtime_data's lbinding_data for the
@@ -3289,6 +3342,7 @@ 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_meter, NULL);
 
     engine_add_input(&en_ct_zones, &en_ovs_open_vswitch, NULL);
     engine_add_input(&en_ct_zones, &en_ovs_bridge, NULL);
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
 
-- 
2.31.1



More information about the dev mailing list