On Tue, Mar 8, 2022 at 4:14 AM Lorenzo Bianconi <[email protected]> wrote: > > 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. > > Acked-by: Han Zhou <[email protected]> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1939524 > Signed-off-by: Lorenzo Bianconi <[email protected]> > --- > Changes since v9: > - clear meter_bands shahs in run_S_CLEAR_FLOWS > - avoid ovn_extend_table_info lookup in ofctrl_meter_bands_sync > - reintroduce ovn_extend_table_info lookup by name in > lflow_output_sb_meter_handler in order to avoid unnecessary processing > > Changes since v8: > - rename ofctrl_meter_bands_check() to ofctrl_meter_bands_is_equal() > - move ofctrl_meter_bands_sync() in add_meter loop in order to avoid a > dedicated loop over desired extend_table meter hash > > Changes since v7: > - implement full-compare support in ofctrl module > - drop SB_meter IP management > > 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 | 214 +++++++++++++++++++++++++++++++----- > controller/ovn-controller.c | 25 ++++- > lib/extend-table.c | 14 +++ > lib/extend-table.h | 4 + > tests/ovn-performance.at | 22 ++++ > tests/ovn.at | 74 +++++++++++++ > tests/system-ovn.at | 17 +++ > 7 files changed, 340 insertions(+), 30 deletions(-) > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > index 19aa787f9..18071788b 100644 > --- a/controller/ofctrl.c > +++ b/controller/ofctrl.c > @@ -360,6 +360,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. */ > @@ -397,6 +413,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. > @@ -640,6 +657,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. */ > @@ -814,6 +832,7 @@ ofctrl_destroy(void) > rconn_packet_counter_destroy(tx_counter); > expr_symtab_destroy(&symtab); > shash_destroy(&symtab); > + ofctrl_meter_bands_destroy(); > } > > uint64_t > @@ -1979,26 +1998,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")) { > @@ -2031,6 +2038,153 @@ 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_desired, > + 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_desired->name, sb_meter->name)) {
Hi Lorenzo, This can be m_existing->name, and so the parameter m_desired is not needed in this function. Another issue I encountered with this patch is that whenever I use -j20 to run the tests, the below cases always fail (90% chance failure): ovn-controller - check meters update -- ovn-northd -- dp-groups=yes ovn-controller - check meters update -- ovn-northd -- dp-groups=no While when executed individually they always pass (100%) When they fail, the error is: ../../tests/ovn.at:29978: as hv1 ovs-ofctl dump-flows br-int | grep -q meter_id=1 --- /dev/null 2022-02-02 10:59:06.536711015 -0800 +++ /home/hanzhou/src/ovn/_build/tests/testsuite.dir/at-groups/736/stderr 2022-03-08 23:30:43.362680171 -0800 @@ -0,0 +1 @@ +ovs-ofctl: write to stdout failed 736. ovn.at:29978: 736. ovn-controller - check meters update -- ovn-northd -- dp-groups=no (ovn.at:29978): FAILED (ovn.at:29978) This looks weird to me. If I add the same command before the check, the failure is less likely to be reproduced. And also when executed with -j20, all the other test cases are ok. I haven't figured out yet what's wrong here. Did you encounter the same failures? @Mark Michelson <[email protected]> I gave my ACK, but do you want to take a look as well? Thanks, Han > + 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, > @@ -2409,13 +2563,20 @@ 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_desired, m_existing, meter_table, > + &msgs); > } > } > > @@ -2505,12 +2666,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 c09018243..ea5e9df41 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,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; > @@ -3316,6 +3337,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..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 c81890ccc..09d3db626 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -29974,3 +29974,77 @@ 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]) > + > +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 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
