On Wed, Mar 2, 2022 at 11:21 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. > Thanks Lorenzo for the revision.
> Related bz: https://bugzilla.redhat.com/show_bug.cgi?id=1939524 Suggest to use the tag: "Reported-at: " for the above. > > Signed-off-by: Lorenzo Bianconi <[email protected]> > --- > 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 | 45 ++++++++++++++++++++--- > controller/ovn-controller.c | 34 +++++++++++++++++- > lib/extend-table.c | 15 ++++++++ > lib/extend-table.h | 13 +++++++ > tests/ovn-performance.at | 22 ++++++++++++ > tests/ovn.at | 71 +++++++++++++++++++++++++++++++++++++ > tests/system-ovn.at | 17 +++++++++ > 7 files changed, 211 insertions(+), 6 deletions(-) > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > index 19aa787f9..598e39751 100644 > --- a/controller/ofctrl.c > +++ b/controller/ofctrl.c > @@ -1979,9 +1979,9 @@ 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_meter(struct ovn_extend_table_info *m_desired, > + const struct sbrec_meter_table *meter_table, > + int cmd, struct ovs_list *msgs) > { > const struct sbrec_meter *sb_meter; > SBREC_METER_TABLE_FOR_EACH (sb_meter, meter_table) { > @@ -1997,7 +1997,7 @@ add_meter(struct ovn_extend_table_info *m_desired, > } > > struct ofputil_meter_mod mm; > - mm.command = OFPMC13_ADD; > + mm.command = cmd; > mm.meter.meter_id = m_desired->table_id; > mm.meter.flags = OFPMF13_STATS; > > @@ -2029,6 +2029,7 @@ add_meter(struct ovn_extend_table_info *m_desired, > > add_meter_mod(&mm, msgs); > free(mm.meter.bands); > + m_desired->cmd = UNSPEC_CMD; > } > > static void > @@ -2314,6 +2315,38 @@ ofctrl_can_put(void) > return true; > } > > +static void > +ofctl_sync_ovs_meters(const struct sbrec_meter_table *meter_table, > + struct ovs_list *msgs) > +{ > + /* Iterate through all the desired meters to check if OVS meter needs > + * to be updated. > + */ > + struct ovn_extend_table_info *m_desired; > + HMAP_FOR_EACH (m_desired, hmap_node, &meters->desired) { > + switch (m_desired->cmd) { > + case ADD_CMD: > + update_meter(m_desired, meter_table, OFPMC13_ADD, msgs); > + break; > + case UPDATE_CMD: > + update_meter(m_desired, meter_table, OFPMC13_MODIFY, msgs); > + break; > + case DEL_CMD: { > + struct ofputil_meter_mod mm = { > + .command = OFPMC13_DELETE, > + .meter = { .meter_id = m_desired->table_id }, > + }; > + add_meter_mod(&mm, msgs); > + m_desired->cmd = UNSPEC_CMD; Is this UNSPEC_CMD supposed to mean there are no pending changes for the entry? Is it supposed to be used also for all other branches in this switch-case? I can see that you set the cmd in the I-P engine to indicate if an update is needed for the entry, so that here it can be synced to OVS incrementally. This is similar to the incremental flow installation, which is good for efficiency during incremental processing. However, I think when full recompute is triggered, the compare-and-install part is missing. For flow installation, both are supported, so that when I-P is not possible, it falls back to compare and install. In fact I thought we could start with something simple - only implement the compare-and-install approach for meters in both I-P and full recompute cases, because the number of meters shouldn't be huge and the cost should be ok. But of course it is better to implement both. It is just that we need to handle the recompute case properly, which requires both desired data and installed data to be maintained. For the meter names and IDs, the extended table already maintains the desired and installed information (so that a full compare-and-install is possible, like how it has been working), but doesn't have the meter details for the case when a meter needs to be updated (so there is nothing to compare and no way to figure out if it needs to be updated). I think we can create an extra table that maintains the meter specific details of the "installed" meters - the one similar to the shash you implemented in the previous versions, which has a one-to-one mapping to the extended table of installed meter names and IDs. For the desired meter details, they are already in the SB meter table. Now we have both desired and installed data, we can do a full compare-and-install: 1) For meters in desired but not installed: add them (no change) 2) For meters installed but not in desired: delete them (no change) 3) For meters in both desired and installed, compare the details (it is also a function you already implemented in the previous version). If they are different, update. (This is new) With the above, I think the full-recompute case can be handled properly and the meter band updates can be synced to OVS. I'd like to remind that the separate meter details table should be in the ofctrl module only and there is no need to put it to the I-P engine because the I-P engine is about the desired data only. (The existed extended table maintaining both desired and installed data is more of a historical leftover, but strictly speaking, they'd be better to be separated - this is just background information, and nothing to do with this patch). Now for efficiently installing the changed meters without doing a full comparison (again, I think it is optional for the current patch), it is smart to add the "cmd" field and set it in the I-P engine in the meter change handler, and the above function ofctl_sync_ovs_meters looks good to me, but it needs to be called only if the I-P engine has incrementally processed the changes, which needs some flags to indicate that, and also need to clear the cmds at the proper places to avoid repeated commands sending to OVS. (refer to the change_tracked flag of lflow_table/pflow_table) > + break; > + } > + case UNSPEC_CMD: > + default: > + break; > + } > + } > +} > + > /* Replaces the flow table on the switch, if possible, by the flows added > * with ofctrl_add_flow(). > * > @@ -2415,10 +2448,12 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table, > * describes the meter itself. */ > add_meter_string(m_desired, &msgs); > } else { > - add_meter(m_desired, meter_table, &msgs); > + update_meter(m_desired, meter_table, OFPMC13_ADD, &msgs); If we are taking care of this in ofctl_sync_ovs_meters, this would be redundant messages. > } > } > > + ofctl_sync_ovs_meters(meter_table, &msgs); > + > /* Add all flow updates into a bundle. */ > static int bundle_id = 0; > struct ofputil_bundle_ctrl_msg bc = { > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index c09018243..e1b6498d4 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,35 @@ 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 *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) { > + struct ovn_extend_table_info *m_desired = > + ovn_extend_table_desired_lookup_by_name(&fo->meter_table, > + iter->name); > + if (m_desired) { > + if (sbrec_meter_is_deleted(iter)) { > + m_desired->cmd = DEL_CMD; > + } else if (sbrec_meter_is_new(iter)) { > + m_desired->cmd = ADD_CMD; Here is one more trick problem to address. As mentioned in my earlier reply: 1) When a meter was referenced by a lflow but not created in SB yet, the meter won't be installed to OVS, which is reasonable. But if the meter is then added to SB, since we don't have handler for SB meter table, this is ignored, and adding the meter in SB won't correct the missing meter in OVS. I think this is the place to handle that scenario. The meter table has the information about which lflows reference the meter, and now that the meter is created in SB, we not only need to create the meter in OVS, but also re-process the lflow(s) so that the flows are properly installed to OVS. I am ok to leave a XXX (TODO) here and address in a separate patch. Please refer to lflow_handle_changed_ref, and let me know if you need any help. > + } else { > + m_desired->cmd = UPDATE_CMD; > + } > + } > + } > + > + engine_set_node_state(node, EN_UPDATED); > + > + return true; > +} > + > struct ed_type_pflow_output { > /* Desired physical flows. */ > struct ovn_desired_flow_table flow_table; > @@ -3316,6 +3346,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..20535d988 100644 > --- a/lib/extend-table.c > +++ b/lib/extend-table.c > @@ -337,3 +337,18 @@ 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) > +{ > + > + struct ovn_extend_table_info *m_desired; > + HMAP_FOR_EACH (m_desired, hmap_node, &table->desired) { This should use HMAP_FOR_EACH_WITH_HASH for efficiency. Thanks, Han > + 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..ce6ae7e7c 100644 > --- a/lib/extend-table.h > +++ b/lib/extend-table.h > @@ -44,12 +44,21 @@ struct ovn_extend_table_lflow_to_desired { > struct ovs_list desired; /* List of desired items used by the lflow. */ > }; > > +enum ovn_extend_table_sync_cmd { > + UNSPEC_CMD = 0, > + ADD_CMD, > + DEL_CMD, > + UPDATE_CMD, > +}; > + > struct ovn_extend_table_info { > struct hmap_node hmap_node; > char *name; /* Name for the table entity. */ > uint32_t table_id; > bool new_table_id; /* 'True' if 'table_id' was reserved from > * ovn_extend_table's 'table_ids' bitmap. */ > + enum ovn_extend_table_sync_cmd cmd; /* This entry needs to be synced with OVS > + * running the provided command. */ > 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. */ > @@ -94,6 +103,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 69270601a..478e26302 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -29744,3 +29744,74 @@ 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]) > + > +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
