Re: [ovs-dev] [PATCH ovn] northd: Don't create fair Sb meters for ACLs with logging disabled.
On 2/28/24 02:21, Ales Musil wrote:
On Tue, Feb 27, 2024 at 5:04 PM 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
---
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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Looks good to me, thanks.
Acked-by: Ales Musil
Thanks Ilya and Ales.
I merged this to main and all branches back to branch-23.03.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] northd: Don't create fair Sb meters for ACLs with logging disabled.
On Tue, Feb 27, 2024 at 5:04 PM 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
> ---
> 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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Looks good to me, thanks.
Acked-by: Ales Musil
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA
amu...@redhat.com
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] northd: Don't create fair Sb meters for ACLs with logging disabled.
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
> ---
FWIW, CI failed due to crun issues. See:
https://patchwork.ozlabs.org/project/ovn/patch/20240227162801.1908669-2-mh...@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.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] northd: Don't create fair Sb meters for ACLs with logging disabled.
Thanks, Ilya.
Acked-by: Mark Michelson
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
---
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
