> 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.
Hi Han, thx for the detailed review. > > > Related bz: https://bugzilla.redhat.com/show_bug.cgi?id=1939524 > > Suggest to use the tag: "Reported-at: " for the above. ack, I will fix it in v8. > > > > > 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. It was set already at the end of update_meter() > > 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). In the next iteration I will implement just the full-compare case since it is covering all the possible issues. We can continue the discussion according to the proposed code. > > 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. ack, right. > > > } > > } > > > > + 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 guess this case will be covered bu UPDATE_CMD cmd since we have already added the entry in the desired table and in the next ofctrl run (triggered by lflow_output_sb_fdb_handler) ofctl_sync_ovs_meters() will install the OVS meters. Am I missing something? > > 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. Why do we need to re-process the flows? the lflows just refer to the meters through the meter id that is computed by ovn_extend_table_assign_id() in the first run. ofctl_sync_ovs_meters() executed by lflow_output_sb_fdb_handler() will install the OVS meters. Moreover this case is already covered in "ovn-controller - check meters update" unit test (please take a look to meter2). Regards, Lorenzo > > > + } 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
