On Tue, Feb 27, 2024 at 5:04 PM Ilya Maximets <[email protected]> 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 <[email protected]>
> ---
> 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}
> --
> 2.43.0
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Looks good to me, thanks.
Acked-by: Ales Musil <[email protected]>
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
[email protected]
<https://red.ht/sig>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
