> I'm pretty certain that this code can miss meter updates in the southbound
> database. I have additional notes below.
>
> On 1/14/22 13:01, Lorenzo Bianconi 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]>
> > ---
> > controller/ofctrl.c | 77 ++++++++++++++++++++++---------------
> > controller/ofctrl.h | 4 +-
> > controller/ovn-controller.c | 72 +++++++++++++++++++++++++++++++++-
> > tests/ovn.at | 53 ++++++++++++++++++++++++-
> > tests/system-ovn.at | 17 ++++++++
> > 5 files changed, 188 insertions(+), 35 deletions(-)
> >
> > diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> > index bf715787e..14ca08e94 100644
> > --- a/controller/ofctrl.c
> > +++ b/controller/ofctrl.c
> > @@ -1810,40 +1810,26 @@ uint32_t ofctrl_get_meter_id(const char *name, bool
> > new_id)
> > return id;
> > }
> > -static void
> > -add_meter(struct ovn_extend_table_info *m_desired,
> > - const struct sbrec_meter_table *meter_table,
> > - struct ovs_list *msgs)
> > +void
> > +set_meter(const struct sbrec_meter *meter, uint32_t id, int cmd)
> > {
> > - 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.meter.flags = OFPMF13_STATS;
> > + struct ovs_list msgs = OVS_LIST_INITIALIZER(&msgs);
> > + struct ofputil_meter_mod mm = {
> > + .command = cmd,
> > + .meter.meter_id = id,
> > + .meter.flags = OFPMF13_STATS,
> > + };
> > - if (!strcmp(sb_meter->unit, "pktps")) {
> > + if (!strcmp(meter->unit, "pktps")) {
> > mm.meter.flags |= OFPMF13_PKTPS;
> > } else {
> > mm.meter.flags |= OFPMF13_KBPS;
> > }
> > -
> > - mm.meter.n_bands = sb_meter->n_bands;
> > + mm.meter.n_bands = meter->n_bands;
> > mm.meter.bands = xcalloc(mm.meter.n_bands, sizeof *mm.meter.bands);
> > - for (size_t i = 0; i < sb_meter->n_bands; i++) {
> > - struct sbrec_meter_band *sb_band = sb_meter->bands[i];
> > + for (size_t i = 0; i < meter->n_bands; i++) {
> > + struct sbrec_meter_band *sb_band = meter->bands[i];
> > struct ofputil_meter_band *mm_band = &mm.meter.bands[i];
> > if (!strcmp(sb_band->action, "drop")) {
> > @@ -1858,9 +1844,41 @@ add_meter(struct ovn_extend_table_info *m_desired,
> > mm.meter.flags |= OFPMF13_BURST;
> > }
> > }
> > -
> > - add_meter_mod(&mm, msgs);
> > + add_meter_mod(&mm, &msgs);
> > free(mm.meter.bands);
> > +
> > + 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);
> > + }
> > + }
> > +}
> > +
> > +void
> > +remove_meter(uint32_t id)
> > +{
> > + struct ovs_list msgs = OVS_LIST_INITIALIZER(&msgs);
> > + /* Delete the meter. */
> > + struct ofputil_meter_mod mm = {
> > + .command = OFPMC13_DELETE,
> > + .meter = { .meter_id = id },
> > + };
> > + add_meter_mod(&mm, &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
> > @@ -2161,7 +2179,6 @@ void
> > ofctrl_put(struct ovn_desired_flow_table *lflow_table,
> > struct ovn_desired_flow_table *pflow_table,
> > struct shash *pending_ct_zones,
> > - const struct sbrec_meter_table *meter_table,
> > uint64_t req_cfg,
> > bool lflows_changed,
> > bool pflows_changed)
> > @@ -2246,8 +2263,6 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
> > /* 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);
> > }
> > }
> > diff --git a/controller/ofctrl.h b/controller/ofctrl.h
> > index 0efcb2ef5..53af92cfb 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 {
> > @@ -55,7 +56,6 @@ enum mf_field_id ofctrl_get_mf_field_id(void);
> > void ofctrl_put(struct ovn_desired_flow_table *lflow_table,
> > struct ovn_desired_flow_table *pflow_table,
> > struct shash *pending_ct_zones,
> > - const struct sbrec_meter_table *,
> > uint64_t nb_cfg,
> > bool lflow_changed,
> > bool pflow_changed);
> > @@ -130,5 +130,7 @@ bool ofctrl_is_connected(void);
> > void ofctrl_set_probe_interval(int probe_interval);
> > void ofctrl_get_memory_usage(struct simap *usage);
> > uint32_t ofctrl_get_meter_id(const char *name, bool new_id);
> > +void set_meter(const struct sbrec_meter *meter, uint32_t id, int cmd);
> > +void remove_meter(uint32_t id);
> > #endif /* controller/ofctrl.h */
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 86cb6f769..ac332ff94 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);
> > @@ -115,6 +116,7 @@ static unixctl_cb_func debug_delay_nb_cfg_report;
> > #define OVS_STARTUP_TS_NAME "ovn-startup-ts"
> > static struct engine *flow_engine;
> > +static struct engine *meter_engine;
> > static char *parse_options(int argc, char *argv[]);
> > OVS_NO_RETURN static void usage(void);
> > @@ -964,7 +966,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,
> > @@ -1509,6 +1512,65 @@ addr_sets_sb_address_set_handler(struct engine_node
> > *node, void *data)
> > return true;
> > }
> > +struct ed_type_meter {
> > + bool change_tracked;
>
> This field isn't used for anything as far as I can tell.
>
> > +};
> > +
> > +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;
>
> The engine_node's run() function is intended to perform a full computation
> of the data. It is called when the engine has encountered a condition where
> a recompute is required. It then should set relevant data on the
> ed_type_meter structure so that the change handler can detect what has
> changed when processing SB record changes in the future.
>
> This run() function doesn't do anything with the meter data. This means that
> an engine recompute will not process the southbound meter data at all.
>
> > +}
> > +
> > +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));
> > +
> > + uint32_t id;
> > + const struct sbrec_meter *iter;
> > + SBREC_METER_TABLE_FOR_EACH_TRACKED (iter, m_table) {
> > + id = ofctrl_get_meter_id(iter->name,
> > !sbrec_meter_is_deleted(iter));
> > + if (id == EXT_TABLE_ID_INVALID) {
> > + return false;
> > + }
> > +
> > + if (sbrec_meter_is_deleted(iter)) {
> > + remove_meter(id);
>
> Performing openflow changes directly in this loop is not a great idea.
> Consider that at some point mid-loop, an error is encountered. If you have
> been queuing OF changes directly, then the result can be a partially-set
> (and possibly invalid) state in OVS.
>
> Instead, it is a good idea to wait until after this loop is complete to do
> so. This way, if an error is encountered mid-loop, you can fall back to a
> full recompute instead without having performed any changes in OVS.
ack, I will fix it.
>
> > + } else {
> > + int cmd = sbrec_meter_is_new(iter) ? OFPMC13_ADD :
> > OFPMC13_MODIFY;
> > +
> > + set_meter(iter, id, cmd);
> > + }
> > + }
> > + 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. */
> > @@ -3223,6 +3285,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);
> > @@ -3347,11 +3410,14 @@ main(int argc, char *argv[])
> > engine_add_input(&en_flow_output, &en_pflow_output,
> > flow_output_pflow_output_handler);
> > + engine_add_input(&en_meter, &en_sb_meter, meter_sb_meter_handler);
> > +
> > struct engine_arg engine_arg = {
> > .sb_idl = ovnsb_idl_loop.idl,
> > .ovs_idl = ovs_idl_loop.idl,
> > };
> > flow_engine = engine_new(&en_flow_output, &engine_arg, "flow_engine");
> > + meter_engine = engine_new(&en_meter, &engine_arg, "meter_engine");
> > engine_ovsdb_node_add_index(&en_sb_chassis, "name",
> > sbrec_chassis_by_name);
> > engine_ovsdb_node_add_index(&en_sb_multicast_group, "name_datapath",
> > @@ -3488,6 +3554,7 @@ main(int argc, char *argv[])
> > }
> > engine_init_run(flow_engine);
> > + engine_init_run(meter_engine);
> > struct ovsdb_idl_txn *ovs_idl_txn =
> > ovsdb_idl_loop_run(&ovs_idl_loop);
> > unsigned int new_ovs_cond_seqno
> > @@ -3594,6 +3661,7 @@ main(int argc, char *argv[])
> > engine_set_force_recompute(flow_engine, true);
> > }
> > + engine_run(meter_engine, true);
> > if (br_int) {
> > ct_zones_data = engine_get_data(&en_ct_zones);
> > if (ct_zones_data) {
> > @@ -3761,7 +3829,6 @@ main(int argc, char *argv[])
> > ofctrl_put(&lflow_output_data->flow_table,
> > &pflow_output_data->flow_table,
> > &ct_zones_data->pending,
> > -
> > sbrec_meter_table_get(ovnsb_idl_loop.idl),
> > ofctrl_seqno_get_req_cfg(),
> > engine_node_changed(&en_lflow_output),
> > engine_node_changed(&en_pflow_output));
> > @@ -3905,6 +3972,7 @@ loop_done:
> > engine_set_context(flow_engine, NULL);
> > engine_cleanup(flow_engine);
> > + engine_cleanup(meter_engine);
> > /* It's time to exit. Clean up the databases if we are not
> > restarting */
> > if (!restart) {
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 92e284e8a..9a5e6b196 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -9181,7 +9181,7 @@ ovn-nbctl acl-del lsw0 to-lport 1000 'tcp.dst==80'
> > ovn-nbctl --wait=hv sync
> > AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br-int | grep meter], [0],
> > [ignore], [ignore])
> > -# Delete acl2, meter should be deleted in OVS
> > +# Delete acl2, meter should be kept in OVS
> > ovn-nbctl acl-del lsw0 to-lport 1000 'tcp.dst==81'
> > ovn-nbctl --wait=hv sync
> > AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br-int | grep meter], [1])
> > @@ -29704,3 +29704,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])
>
> This is a good idea for a test, since it ensures that changes to a meter are
> reflected in OVS.
>
> But I also think test cases should be added to the "ovn-controller
> incremental processing" test in ovn-performance.at to ensure that changes
> are happening incrementally as expected.
>
> > +
> > +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
>
> I don't think this properly tests a recompute. Since the SB Meter table and
> SB Chassis table are handled by different incremental engines, adding a
> chassis doesn't result in a recompute of the meter incremental engine.
> Instead, the change handler in the meter incremental engine should be able
> to handle the change incrementally, just like it did with previous changes.
>
> My worry is that since the run() function for the en_meter node does not
> actually compute meters, a recompute will not result in any reading of the
> SB data.
>
> I think a good test for this would be to pre-populate the SB database with
> meter information, then start ovn-controller. I suspect that OVS will not
> have the meters installed. Even if you then performed `ovn-appctl -t
> ovn-controller inc-engine/recompute` I think the meters would still not be
> installed. If you then added or changed a meter, you would see the changes
> happen on the added/changed meters, but any unchanged meters in the SB
> database would still not be installed in OVS.
ack, I will fix it.
Regards,
Lorenzo
>
> > +
> > +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 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
> >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
