On Thu, Nov 25, 2021 at 12:05 PM Lorenzo Bianconi
<[email protected]> 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]>
Hi Lorenzo,
Thanks for the fix. Please see below for a few comments.
Numan
> ---
> 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);
What is the reason to add a new meter engine node ?
> 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);
Instead of adding a new engine node, you could have just added a
handler here i.e en_lflow_output_meter_handler().
What is the reason ? Is it because when a sb meter is updated along
with some other sb change ( which results in a full
recompute) then the OF flow for the updated meter is not MODIFIED ?
If that is so, then I think your approach makes sense.
Can you please add a test case scenario where in ovn-controller has to
process a sb meter update and an another update (which
results in full recompute, for example a chassis is added) in the
same loop and the meter flow is updated accordingly ?
Numan
>
> 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
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
