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. Related bz: https://bugzilla.redhat.com/show_bug.cgi?id=1939524 Signed-off-by: Lorenzo Bianconi <[email protected]> --- Changes since v6: - move ovs sync code back to ofctrl_put() - remove meter IP node and link SB_meter node to lflow_output one - add new unit-tests Changes since v5: - add size parameter to extend_table in order to reduce the size Changes since v4: - add offset parameter to ovn_extend_table_init - rebase on top of ovn master Changes since v3: - move full meter management in the IP engine Changes since v2: - move meter ip into lflow ip - add new test in ovn-performance.at - manage metering ip full-recompute Changes since v1: - add IP refactor to the series - rebase on top of ovn master --- controller/ofctrl.c | 45 ++++++++++++++++++++--- controller/ovn-controller.c | 34 +++++++++++++++++- lib/extend-table.c | 15 ++++++++ lib/extend-table.h | 13 +++++++ tests/ovn-performance.at | 22 ++++++++++++ tests/ovn.at | 71 +++++++++++++++++++++++++++++++++++++ tests/system-ovn.at | 17 +++++++++ 7 files changed, 211 insertions(+), 6 deletions(-) diff --git a/controller/ofctrl.c b/controller/ofctrl.c index 19aa787f9..598e39751 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -1979,9 +1979,9 @@ 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_meter(struct ovn_extend_table_info *m_desired, + const struct sbrec_meter_table *meter_table, + int cmd, struct ovs_list *msgs) { const struct sbrec_meter *sb_meter; SBREC_METER_TABLE_FOR_EACH (sb_meter, meter_table) { @@ -1997,7 +1997,7 @@ add_meter(struct ovn_extend_table_info *m_desired, } struct ofputil_meter_mod mm; - mm.command = OFPMC13_ADD; + mm.command = cmd; mm.meter.meter_id = m_desired->table_id; mm.meter.flags = OFPMF13_STATS; @@ -2029,6 +2029,7 @@ add_meter(struct ovn_extend_table_info *m_desired, add_meter_mod(&mm, msgs); free(mm.meter.bands); + m_desired->cmd = UNSPEC_CMD; } static void @@ -2314,6 +2315,38 @@ ofctrl_can_put(void) return true; } +static void +ofctl_sync_ovs_meters(const struct sbrec_meter_table *meter_table, + struct ovs_list *msgs) +{ + /* Iterate through all the desired meters to check if OVS meter needs + * to be updated. + */ + struct ovn_extend_table_info *m_desired; + HMAP_FOR_EACH (m_desired, hmap_node, &meters->desired) { + switch (m_desired->cmd) { + case ADD_CMD: + update_meter(m_desired, meter_table, OFPMC13_ADD, msgs); + break; + case UPDATE_CMD: + update_meter(m_desired, meter_table, OFPMC13_MODIFY, msgs); + break; + case DEL_CMD: { + struct ofputil_meter_mod mm = { + .command = OFPMC13_DELETE, + .meter = { .meter_id = m_desired->table_id }, + }; + add_meter_mod(&mm, msgs); + m_desired->cmd = UNSPEC_CMD; + break; + } + case UNSPEC_CMD: + default: + break; + } + } +} + /* Replaces the flow table on the switch, if possible, by the flows added * with ofctrl_add_flow(). * @@ -2415,10 +2448,12 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table, * describes the meter itself. */ add_meter_string(m_desired, &msgs); } else { - add_meter(m_desired, meter_table, &msgs); + update_meter(m_desired, meter_table, OFPMC13_ADD, &msgs); } } + ofctl_sync_ovs_meters(meter_table, &msgs); + /* Add all flow updates into a bundle. */ static int bundle_id = 0; struct ofputil_bundle_ctrl_msg bc = { diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index c09018243..e1b6498d4 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -968,7 +968,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, @@ -2718,6 +2719,35 @@ 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 *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) { + struct ovn_extend_table_info *m_desired = + ovn_extend_table_desired_lookup_by_name(&fo->meter_table, + iter->name); + if (m_desired) { + if (sbrec_meter_is_deleted(iter)) { + m_desired->cmd = DEL_CMD; + } else if (sbrec_meter_is_new(iter)) { + m_desired->cmd = ADD_CMD; + } else { + m_desired->cmd = UPDATE_CMD; + } + } + } + + engine_set_node_state(node, EN_UPDATED); + + return true; +} + struct ed_type_pflow_output { /* Desired physical flows. */ struct ovn_desired_flow_table flow_table; @@ -3316,6 +3346,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..20535d988 100644 --- a/lib/extend-table.c +++ b/lib/extend-table.c @@ -337,3 +337,18 @@ 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) +{ + + struct ovn_extend_table_info *m_desired; + HMAP_FOR_EACH (m_desired, hmap_node, &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..ce6ae7e7c 100644 --- a/lib/extend-table.h +++ b/lib/extend-table.h @@ -44,12 +44,21 @@ struct ovn_extend_table_lflow_to_desired { struct ovs_list desired; /* List of desired items used by the lflow. */ }; +enum ovn_extend_table_sync_cmd { + UNSPEC_CMD = 0, + ADD_CMD, + DEL_CMD, + UPDATE_CMD, +}; + struct ovn_extend_table_info { struct hmap_node hmap_node; char *name; /* Name for the table entity. */ uint32_t table_id; bool new_table_id; /* 'True' if 'table_id' was reserved from * ovn_extend_table's 'table_ids' bitmap. */ + enum ovn_extend_table_sync_cmd cmd; /* This entry needs to be synced with OVS + * running the provided command. */ struct hmap references; /* The lflows that are using this item, with * ovn_extend_table_lflow_ref nodes. Only useful * for items in ovn_extend_table.desired. */ @@ -94,6 +103,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..bc133f93b 100644 --- a/tests/ovn-performance.at +++ b/tests/ovn-performance.at @@ -543,6 +543,28 @@ 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 copp-add copp0 arp meter0] +) + +OVN_CONTROLLER_EXPECT_NO_HIT( + [hv1 hv2 hv3 hv4], [lflow_run], + [ovn-nbctl --wait=hv lr-copp-add copp0 lr1] +) + +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 69270601a..478e26302 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -29744,3 +29744,74 @@ 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 copp-add copp0 event-elb meter0 +ovn-nbctl --wait=hv ls-copp-add copp0 sw0 +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]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP +]) diff --git a/tests/system-ovn.at b/tests/system-ovn.at index f57d752d4..d1770408e 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -6661,8 +6661,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 copp-del copp0 reject -- 2.35.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
