| Message ID | 4f96ea3987719de0ac415eb04ae2dd908af17600.1644534146.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v3] controller: reconfigure ovs meters for ovn meters updates | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
Hi Lorenzo, I think this is an improvement over the previous version, but there are still issues that need addressing. The problem with adding meter incremental processing is that there doesn't exist any unconditional processing of the SB meter data. In other words, there is no place in the code that loops over all SB meters and programs those into OVS. Instead, our lflow processing code (which is incrementally processed) determines which meters are referenced by lflows and then later uses this to determine which meters are "relevant" and only reads the SB meters that are referred to by lflows. In each iteration of the patch, adding incremental processing to SB meters has had oddities since we're not replacing an existing stateless SB meter process. At a high level, the oddities I see here are: 1) Rather than using engine node data to store SB meter data, this change adds priv_data and priv_size to the existing extend_table_info structure. 2) The en_meter_run() function doesn't do anything. 3) The meter_sb_meter_handler() function only handles updated meters, not new or deleted meters. In all of these cases, the reason is that we are still relying on lflow processing to do a lot of the heavy lifting (i.e. discovering new meters, deleting removed meters, etc). The new engine node is tightly coupled with the lflow engine node and is only designed to patch some shortcomings, rather than having a fully-functioning incremental processing node for meters. This could fall apart spectacularly if lflow processing changes. I think your previous attempts to make a separate engine to handle meter updates was an attempt to decouple the meter processing from the lflow processing, but unfortunately the coupling was still present in those versions since lflow handling was still creating the extend_table_info structures and needed to add their references to the extend_table_info. I think that in order to add incremental processing for SB meters, it's a two-step process. Step 1: Write non-incremental code that centers meter programming around the SB meter database instead of based on lflows that refer to meters. Step 2: Take the code written in step 1 and make it incremental. This patch is trying to do step 2 without step 1. The thing is, to fix the issue, you could submit a patch that just does step 1 without step 2, and it may be just fine, depending on how common it is for meters to change. I hope what I'm saying makes sense. I have some additional comments down below On 2/10/22 18:11, 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 <lorenzo.bianconi@redhat.com> > --- > 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; > + } Why is the band's action not checked here? If the rate and burst_rate remain the same but the action changes, doesn't that mean something has changed and the cache should be re-built? > + } > + > + 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); I think using sbrec_meter_band in the cache is not a good choice. This messes with the ownership principles of the IDL by allowing a structure to be borrowed. The IDL can free pointers within sbrec_* structures. In this case, band->action could be left dangling if the IDL frees the memory pointed to by band->action. I suggest using a custom structure that stores the information you care about. > + } > + 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); > + } This is nearly a copy-paste of how the cache is re-created in meter_update(). This should probably be factored into its own function. > +} > + > 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)) { This is an example of what I was talking about where this leans heavily on the existing lflow processing behavior. Incremental engine nodes are expected to handle all changed rows, not just updated rows. The reason things work is that the lflow processing code handles the new and deleted meters. > + 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; > + } Since priv_data can be copied like this, it's important that priv_data doesn't have pointers to allocated data. Otherwise, it becomes murky about who is the owner and should free the data. As I mentioned at the top, having generic priv_data on the extend_table_info is likely not a good idea anyway, and traps like this just enforce that idea further. > 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. */ This comment is incorrect. It is not the number of elements, it's the allocated size. > }; > > /* 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 > >
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
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 <lorenzo.bianconi@redhat.com> --- 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(-)