northd: Don't create fair Sb meters for ACLs with logging disabled.

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: 880dca9 ("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.maximets@ovn.org>
Acked-by: Mark Michelson <mmichels@redhat.com>
Acked-by: Ales Musil <amusil@redhat.com>
Signed-off-by: Mark Michelson <mmichels@redhat.com>

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;
     }

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}