| Message ID | 20240227160508.3530181-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] northd: Don't create fair Sb meters for ACLs with logging disabled. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
Thanks, Ilya. Acked-by: Mark Michelson <mmichels@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.maximets@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}
On 2/27/24 17: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.maximets@ovn.org> > --- FWIW, CI failed due to crun issues. See: https://patchwork.ozlabs.org/project/ovn/patch/20240227162801.1908669-2-mheib@redhat.com/ I have my own successful runs here: https://github.com/igsilya/ovn/actions/runs/8067246686 https://github.com/igsilya/ovn/actions/runs/8067246684 Best regards, Ilya Maximets.
On Tue, Feb 27, 2024 at 5:04 PM Ilya Maximets <i.maximets@ovn.org> 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.maximets@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} > -- > 2.43.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Acked-by: Ales Musil <amusil@redhat.com>
On 2/28/24 02:21, Ales Musil wrote: > On Tue, Feb 27, 2024 at 5:04 PM Ilya Maximets <i.maximets@ovn.org> 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.maximets@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} >> -- >> 2.43.0 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > Looks good to me, thanks. > > Acked-by: Ales Musil <amusil@redhat.com> Thanks Ilya and Ales. I merged this to main and all branches back to branch-23.03.
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}
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.maximets@ovn.org> --- northd/en-meters.c | 8 ++++++-- tests/ovn-northd.at | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-)