Thanks, Ilya. Acked-by: Mark Michelson <mmich...@redhat.com>
On 2/27/24 11:05, Ilya Maximets wrote:
If the ACL.log is false for a fair meter, but ACL.meter is set in the Northbound database, northd will create a unique meter for this ACL in a Southbound database, even though it will never be used. Normal ovn-nbctl acl-add command can't create such a record, but it is possible with a plain 'ovn-nbctl set' or a direct database transaction. And, in practice, ovn-kubernetes always sets the ACL.meter column even if the logging is not enabled in the namespace. This creates extra unnecessary load on the Southbound database and the ovn-controller that performs a linear iteration over the Southbound Meter table on every ofctrl_put(). Logging is also not a default option, so only a fraction of ACLs will actually need meters under normal circumstances. Stop generating these unnecessary meters. In an ovn-kubernetes setup with 90K ACLs 1K of which has logging enabled this saves ~20 MB of the Southbound database file size and about 30% of the RSS on ovsdb-server (with 1 ovn-controller connected). Should make ofctrl_put() in ovn-controller much faster as well. Arguably, CMS should not set ACL.meter without ACL.log, but the behavior of the ovn-northd is not correct either, so should be fixed anyway. Fixes: 880dca99eaf7 ("northd: Enhance the implementation of ACL log meters (pre-ddlog merge).") Reported-at: https://issues.redhat.com/browse/FDP-401 Signed-off-by: Ilya Maximets <i.maxim...@ovn.org> --- northd/en-meters.c | 8 ++++++-- tests/ovn-northd.at | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/northd/en-meters.c b/northd/en-meters.c index aabd002b6..793a46335 100644 --- a/northd/en-meters.c +++ b/northd/en-meters.c @@ -203,9 +203,13 @@ sync_acl_fair_meter(struct ovsdb_idl_txn *ovnsb_txn, const struct nbrec_acl *acl, struct shash *sb_meters, struct sset *used_sb_meters) { - const struct nbrec_meter *nb_meter = - fair_meter_lookup_by_name(meter_groups, acl->meter); + const struct nbrec_meter *nb_meter; + + if (!acl->log || !acl->meter) { + return; + }+ nb_meter = fair_meter_lookup_by_name(meter_groups, acl->meter);if (!nb_meter) { return; } diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 6fdd761da..0732486f3 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -2432,6 +2432,24 @@ check_acl_lflow acl_two meter_me__${acl2} sw0 check_acl_lflow acl_three meter_me__${acl3} sw0 check_acl_lflow acl_three meter_me__${acl3} sw1+AS_BOX([Disable/enable logging for acl3 while still referencing the meter])+check_row_count meter 4 +check ovn-nbctl --wait=sb set acl $acl3 log=false +check_row_count meter 3 +check_meter_by_name meter_me meter_me__${acl1} meter_me__${acl2} +check_meter_by_name NOT meter_me__${acl3} +check_acl_lflow acl_one meter_me__${acl1} sw0 +check_acl_lflow acl_two meter_me__${acl2} sw0 + +check ovn-nbctl --wait=sb set acl $acl3 log=true +check_row_count meter 4 +check_meter_by_name \ + meter_me meter_me__${acl1} meter_me__${acl2} meter_me__${acl3} +check_acl_lflow acl_one meter_me__${acl1} sw0 +check_acl_lflow acl_two meter_me__${acl2} sw0 +check_acl_lflow acl_three meter_me__${acl3} sw0 +check_acl_lflow acl_three meter_me__${acl3} sw1 + AS_BOX([Stop using meter for acl1]) check ovn-nbctl --wait=sb clear acl $acl1 meter check_meter_by_name meter_me meter_me__${acl2}
_______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
