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]> --- 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 | 101 ++++++++++++++++++++++++++++-------- controller/ofctrl.h | 3 ++ controller/ovn-controller.c | 57 +++++++++++++++++++- lib/extend-table.c | 6 +++ lib/extend-table.h | 2 + tests/ovn-performance.at | 15 ++++++ tests/ovn.at | 51 ++++++++++++++++++ tests/system-ovn.at | 17 ++++++ 8 files changed, 230 insertions(+), 22 deletions(-) diff --git a/controller/ofctrl.c b/controller/ofctrl.c index 08fcfed8b..c001c8ad1 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -344,8 +344,6 @@ static enum mf_field_id mff_ovn_geneve; * is restarted, even if there is no change in the desired flow table. */ static bool need_reinstall_flows; -static ovs_be32 queue_msg(struct ofpbuf *); - static struct ofpbuf *encode_flow_mod(struct ofputil_flow_mod *); static struct ofpbuf *encode_group_mod(const struct ofputil_group_mod *); @@ -797,7 +795,7 @@ ofctrl_get_cur_cfg(void) return cur_cfg; } -static ovs_be32 +ovs_be32 queue_msg(struct ofpbuf *msg) { const struct ofp_header *oh = msg->data; @@ -1802,26 +1800,12 @@ 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) +meter_create_msg(const struct sbrec_meter *sb_meter, + struct ovs_list *msgs, int cmd, int id) { - 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 = id; mm.meter.flags = OFPMF13_STATS; if (!strcmp(sb_meter->unit, "pktps")) { @@ -1854,6 +1838,76 @@ add_meter(struct ovn_extend_table_info *m_desired, free(mm.meter.bands); } +void +meter_update(const struct sbrec_meter *sb_meter, struct ovs_list *msgs) +{ + struct ovn_extend_table_info *m_installed, *next_meter; + HMAP_FOR_EACH_SAFE (m_installed, next_meter, hmap_node, + &meters->existing) { + if (!strcmp(m_installed->name, sb_meter->name)) { + struct sbrec_meter_band *band; + + for (int i = 0; i < sb_meter->n_bands; i++) { + int j; + + for (j = 0; j < m_installed->priv_size / sizeof *band; j++) { + band = + (struct sbrec_meter_band *)m_installed->priv_data + j; + if (band->rate == sb_meter->bands[i]->rate && + band->burst_size == sb_meter->bands[i]->burst_size) { + break; + } + } + + if (j == m_installed->priv_size / sizeof *band) { + meter_create_msg(sb_meter, msgs, OFPMC13_MODIFY, + m_installed->table_id); + /* Recreate meter-bands cache. */ + free(m_installed->priv_data); + m_installed->priv_size = sb_meter->n_bands * sizeof *band; + m_installed->priv_data = xmalloc(m_installed->priv_size); + for (i = 0; i < sb_meter->n_bands; i++) { + band = (struct sbrec_meter_band *)m_installed->priv_data; + memcpy(band + i, sb_meter->bands[i], sizeof *band); + } + return; + } + } + } + } +} + +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; + } + + meter_create_msg(sb_meter, msgs, OFPMC13_ADD, m_desired->table_id); + + /* create private data - meter_bands */ + struct sbrec_meter_band *band; + m_desired->priv_size = sb_meter->n_bands * sizeof *band; + m_desired->priv_data = xmalloc(m_desired->priv_size); + + for (int i = 0; i < sb_meter->n_bands; i++) { + band = (struct sbrec_meter_band *)m_desired->priv_data + i; + memcpy(band, sb_meter->bands[i], sizeof *band); + } +} + static void installed_flow_add(struct ovn_flow *d, struct ofputil_bundle_ctrl_msg *bc, @@ -2340,6 +2394,11 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table, /* Sync the contents of meters->desired to meters->existing. */ ovn_extend_table_sync(meters); + const struct sbrec_meter *sb_meter; + SBREC_METER_TABLE_FOR_EACH (sb_meter, meter_table) { + meter_update(sb_meter, &msgs); + } + if (!ovs_list_is_empty(&msgs)) { /* Add a barrier to the list of messages. */ struct ofpbuf *barrier = ofputil_encode_barrier_request(OFP15_VERSION); diff --git a/controller/ofctrl.h b/controller/ofctrl.h index 014de210d..e3bc286cc 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,7 @@ 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); +ovs_be32 queue_msg(struct ofpbuf *msg); +void meter_update(const struct sbrec_meter *sb_meter, struct ovs_list *msgs); #endif /* controller/ofctrl.h */ diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 8631bccbc..5475243b3 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); @@ -962,7 +963,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, @@ -1505,6 +1507,55 @@ addr_sets_sb_address_set_handler(struct engine_node *node, void *data) return true; } +static void * +en_meter_init(struct engine_node *node OVS_UNUSED, + struct engine_arg *arg OVS_UNUSED) +{ + return NULL; +} + +static void +en_meter_cleanup(void *data OVS_UNUSED) +{ +} + +static void +en_meter_run(struct engine_node *node OVS_UNUSED, void *data OVS_UNUSED) +{ +} + +static bool +meter_sb_meter_handler(struct engine_node *node, void *data OVS_UNUSED) +{ + struct ovs_list msgs = OVS_LIST_INITIALIZER(&msgs); + + 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)) { + meter_update(iter, &msgs); + } + } + + if (!ovs_list_is_empty(&msgs)) { + 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); + } + } + + return true; +} + + struct ed_type_port_groups{ /* A copy of SB port_groups, each converted as a sset for efficient lport * lookup. */ @@ -3232,6 +3283,7 @@ main(int argc, char *argv[]) ENGINE_NODE_WITH_CLEAR_TRACK_DATA(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); @@ -3253,6 +3305,8 @@ main(int argc, char *argv[]) engine_add_input(&en_port_groups, &en_runtime_data, port_groups_runtime_data_handler); + engine_add_input(&en_meter, &en_sb_meter, meter_sb_meter_handler); + engine_add_input(&en_non_vif_data, &en_ovs_open_vswitch, NULL); engine_add_input(&en_non_vif_data, &en_ovs_bridge, NULL); engine_add_input(&en_non_vif_data, &en_sb_chassis, NULL); @@ -3289,6 +3343,7 @@ main(int argc, char *argv[]) lflow_output_runtime_data_handler); engine_add_input(&en_lflow_output, &en_non_vif_data, NULL); + engine_add_input(&en_lflow_output, &en_meter, NULL); engine_add_input(&en_lflow_output, &en_sb_multicast_group, lflow_output_sb_multicast_group_handler); diff --git a/lib/extend-table.c b/lib/extend-table.c index c708e24b9..c7d9bf939 100644 --- a/lib/extend-table.c +++ b/lib/extend-table.c @@ -48,6 +48,7 @@ ovn_extend_table_info_alloc(const char *name, uint32_t id, bool is_new_id, e->table_id = id; e->new_table_id = is_new_id; e->hmap_node.hash = hash; + e->priv_data = NULL; hmap_init(&e->references); return e; } @@ -56,6 +57,7 @@ static void ovn_extend_table_info_destroy(struct ovn_extend_table_info *e) { free(e->name); + free(e->priv_data); struct ovn_extend_table_lflow_ref *r, *r_next; HMAP_FOR_EACH_SAFE (r, r_next, hmap_node, &e->references) { hmap_remove(&e->references, &r->hmap_node); @@ -262,6 +264,10 @@ ovn_extend_info_clone(struct ovn_extend_table_info *source) source->table_id, source->new_table_id, source->hmap_node.hash); + if (source->priv_data) { /* copy private data */ + clone->priv_data = xmemdup(source->priv_data, source->priv_size); + clone->priv_size = source->priv_size; + } return clone; } diff --git a/lib/extend-table.h b/lib/extend-table.h index 4d80cfd80..45ad29144 100644 --- a/lib/extend-table.h +++ b/lib/extend-table.h @@ -53,6 +53,8 @@ struct ovn_extend_table_info { 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. */ + void *priv_data; /* Pointer to private data (e.g. meter-bands for meters). */ + int priv_size; /* Number of elements in data pointer. */ }; /* Maintains the link between a lflow and an ovn_extend_table_info item in diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at index 10341ad72..61525b27a 100644 --- a/tests/ovn-performance.at +++ b/tests/ovn-performance.at @@ -543,6 +543,21 @@ OVN_CONTROLLER_EXPECT_HIT( [as hv3 ovs-vsctl set interface vgw3 external-ids:ovn-egress-iface=true] ) + +ovn-nbctl --event lb-add lb0 192.168.1.100:80 "" +ovn-nbctl --wait=hv ls-lb-add ls1 lb0 +ovn-nbctl --wait=hv meter-add meter0 drop 100 pktps 10 + +OVN_CONTROLLER_EXPECT_NO_HIT( + [hv1 hv2], [lflow_run], + [ovn-nbctl --wait=hv ls-copp-add ls1 event-elb meter0] +) + +OVN_CONTROLLER_EXPECT_NO_HIT( + [hv1 hv2], [lflow_run], + [ovn-nbctl --wait=hv --may-exist meter-add meter0 drop 200 pktps 10] +) + for i in 1 2; do j=$((i%2 + 1)) lp=lp$i diff --git a/tests/ovn.at b/tests/ovn.at index 957eb7850..0e79210e2 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -29578,3 +29578,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]) + +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 + +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 3ae812296..e0ec2c07d 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -6660,8 +6660,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.34.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
