On Thu, Sep 10, 2026 at 8:13 PM Jacob Tanenbaum <[email protected]> wrote:
> > > On Tue, Sep 8, 2026 at 6:32 AM Ales Musil via dev <[email protected]> > wrote: > >> We create meters only if both log and meter is set for given >> ACL. Do not recompute if the ACL is created with only one >> of those. This should make it more efficient especially for >> ovn-kubernetes which always creates ACLs with meters set, but >> enables logging only when needed. >> >> Fixes: b2ac717fcec2 ("northd: Avoid recompute of lflow from ACLs without >> meters.") >> Reported-at: https://redhat.atlassian.net/browse/FDP-3976 >> Signed-off-by: Ales Musil <[email protected]> >> --- >> northd/en-meters.c | 11 +++++++---- >> tests/ovn-northd.at | 30 ++++++++++++++++++++++++------ >> 2 files changed, 31 insertions(+), 10 deletions(-) >> >> diff --git a/northd/en-meters.c b/northd/en-meters.c >> index ef0da25c9..18db9a1fc 100644 >> --- a/northd/en-meters.c >> +++ b/northd/en-meters.c >> @@ -81,10 +81,13 @@ sync_meters_nb_acl_handler(struct engine_node *node, >> void *data OVS_UNUSED) >> >> const struct nbrec_acl *nb_acl; >> NBREC_ACL_TABLE_FOR_EACH_TRACKED (nb_acl, acl_table) { >> - /* New or deleted ACL with meter needs to be recomputed. */ >> - if ((nbrec_acl_is_new(nb_acl) || nbrec_acl_is_deleted(nb_acl)) && >> - (nb_acl->log || nb_acl->meter)) { >> - return EN_UNHANDLED; >> + /* New or deleted ACL with meter and log needs to be recomputed. >> */ >> + if (nbrec_acl_is_new(nb_acl) || nbrec_acl_is_deleted(nb_acl)) { >> + if (nb_acl->log && nb_acl->meter) { >> + return EN_UNHANDLED; >> + } >> + >> + continue; >> } >> >> /* Addition or removal of meter requires recompute. */ >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >> index 1e567704d..fd94b7c8d 100644 >> --- a/tests/ovn-northd.at >> +++ b/tests/ovn-northd.at >> @@ -20553,12 +20553,12 @@ check ovn-nbctl meter-add meter2 drop 20 kbps >> check ovn-nbctl --wait=sb sync >> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> >> -AS_BOX([ACL with log]) >> +AS_BOX([ACL with log only]) >> check ovn-nbctl --wait=sb --log acl-add ls from-lport 100 tcp drop >> acl_id=$(fetch_column nb:Acl _uuid action=drop) >> check_engine_stats northd norecompute compute >> -check_engine_stats lflow recompute nocompute >> -check_engine_stats sync_meters recompute nocompute >> +check_engine_stats lflow norecompute compute >> +check_engine_stats sync_meters norecompute compute >> CHECK_NO_CHANGE_AFTER_RECOMPUTE >> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> >> @@ -20579,12 +20579,12 @@ check as northd ovn-appctl -t ovn-northd >> inc-engine/clear-stats >> >> check ovn-nbctl --wait=sb acl-del ls >> check_engine_stats northd norecompute compute >> -check_engine_stats lflow recompute nocompute >> -check_engine_stats sync_meters recompute nocompute >> +check_engine_stats lflow norecompute compute >> +check_engine_stats sync_meters norecompute compute >> CHECK_NO_CHANGE_AFTER_RECOMPUTE >> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> >> -AS_BOX([ACL with meter]) >> +AS_BOX([ACL with log and meter]) >> check ovn-nbctl --wait=sb --meter=meter1 acl-add ls from-lport 100 tcp >> drop >> acl_id=$(fetch_column nb:Acl _uuid action=drop) >> check_engine_stats northd norecompute compute >> @@ -20614,6 +20614,24 @@ check_engine_stats sync_meters recompute >> nocompute >> CHECK_NO_CHANGE_AFTER_RECOMPUTE >> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> >> +AS_BOX([ACL with meter]) >> +m1=$(fetch_column nb:Meter _uuid name=meter1) >> +check_uuid ovn-nbctl --wait=sb --id=@id create ACL action=drop \ >> + direction=from-lport match=tcp priority=1001 meter=$m1 \ >> > > This might be pedantic but in the ACL table the meter column is the name > of the meter not the UUID, in this case, since log=false, the meter isn't > resolved, but it would be clearer if "meter=meter1" here > > + -- set logical_switch ls acls=@id >> +check_engine_stats northd norecompute compute >> +check_engine_stats lflow norecompute compute >> +check_engine_stats sync_meters norecompute compute >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> + >> +check ovn-nbctl --wait=sb acl-del ls >> +check_engine_stats northd norecompute compute >> +check_engine_stats lflow norecompute compute >> +check_engine_stats sync_meters norecompute compute >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> + >> AS_BOX([ACLs attached to LS]) >> check ovn-nbctl --wait=sb acl-add ls from-lport 100 tcp drop >> acl_id=$(fetch_column nb:Acl _uuid match=tcp action=drop) >> -- >> 2.55.0 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > This is a good patch with just a minor change suggestion > Jacob > Hi Jacob, thank you for the review. I have fixed the meter column in v2. Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
