Acked-by: Flavio Fernandes <fla...@flaviof.com <mailto:fla...@flaviof.com>>
> On Jan 15, 2021, at 8:41 AM, Dumitru Ceara <dce...@redhat.com> wrote:
>
> Commit 880dca99eaf7 added support for fair meters but didn't cover the
> case when an ACL is configured on a port group instead of a logical
> switch.
>
> Iterate over PG ACLs too when syncing fair meters to the Southbound
> database. Due to the fact that a meter might be used for ACLs that are
> applied on multiple logical datapaths (through port groups) we also need
> to change the logic of deleting stale SB Meter records.
>
> Fixes: 880dca99eaf7 ("northd: Enhance the implementation of ACL log meters
> (pre-ddlog merge).")
> Reported-by: Dmitry Yusupov <dyusu...@nvidia.com>
> Signed-off-by: Dumitru Ceara <dce...@redhat.com>
> ---
> northd/ovn-northd.c | 61 ++++++++++++++++++++++++++++++++++++++++-------------
> tests/ovn-northd.at | 42 ++++++++++++++++++++++++------------
> 2 files changed, 74 insertions(+), 29 deletions(-)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 969fbe1..27df6a3 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -12042,17 +12042,20 @@ static void
> sync_meters_iterate_nb_meter(struct northd_context *ctx,
> const char *meter_name,
> const struct nbrec_meter *nb_meter,
> - struct shash *sb_meters)
> + struct shash *sb_meters,
> + struct sset *used_sb_meters)
> {
> + const struct sbrec_meter *sb_meter;
> bool new_sb_meter = false;
>
> - const struct sbrec_meter *sb_meter = shash_find_and_delete(sb_meters,
> - meter_name);
> + sb_meter = shash_find_data(sb_meters, meter_name);
> if (!sb_meter) {
> sb_meter = sbrec_meter_insert(ctx->ovnsb_txn);
> sbrec_meter_set_name(sb_meter, meter_name);
> + shash_add(sb_meters, sb_meter->name, sb_meter);
> new_sb_meter = true;
> }
> + sset_add(used_sb_meters, meter_name);
>
> if (new_sb_meter || bands_need_update(nb_meter, sb_meter)) {
> struct sbrec_meter_band **sb_bands;
> @@ -12074,6 +12077,24 @@ sync_meters_iterate_nb_meter(struct northd_context
> *ctx,
> sbrec_meter_set_unit(sb_meter, nb_meter->unit);
> }
>
> +static void
> +sync_acl_fair_meter(struct northd_context *ctx, struct shash *meter_groups,
> + 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);
> +
> + if (!nb_meter) {
> + return;
> + }
> +
> + char *meter_name = alloc_acl_log_unique_meter_name(acl);
> + sync_meters_iterate_nb_meter(ctx, meter_name, nb_meter, sb_meters,
> + used_sb_meters);
> + free(meter_name);
> +}
> +
> /* Each entry in the Meter and Meter_Band tables in OVN_Northbound have
> * a corresponding entries in the Meter and Meter_Band tables in
> * OVN_Southbound. Additionally, ACL logs that use fair meters have
> @@ -12081,9 +12102,10 @@ sync_meters_iterate_nb_meter(struct northd_context
> *ctx,
> */
> static void
> sync_meters(struct northd_context *ctx, struct hmap *datapaths,
> - struct shash *meter_groups)
> + struct shash *meter_groups, struct hmap *port_groups)
> {
> struct shash sb_meters = SHASH_INITIALIZER(&sb_meters);
> + struct sset used_sb_meters = SSET_INITIALIZER(&used_sb_meters);
>
> const struct sbrec_meter *sb_meter;
> SBREC_METER_FOR_EACH (sb_meter, ctx->ovnsb_idl) {
> @@ -12093,7 +12115,7 @@ sync_meters(struct northd_context *ctx, struct hmap
> *datapaths,
> const struct nbrec_meter *nb_meter;
> NBREC_METER_FOR_EACH (nb_meter, ctx->ovnnb_idl) {
> sync_meters_iterate_nb_meter(ctx, nb_meter->name, nb_meter,
> - &sb_meters);
> + &sb_meters, &used_sb_meters);
> }
>
> /*
> @@ -12107,19 +12129,28 @@ sync_meters(struct northd_context *ctx, struct hmap
> *datapaths,
> continue;
> }
> for (size_t i = 0; i < od->nbs->n_acls; i++) {
> - struct nbrec_acl *acl = od->nbs->acls[i];
> - nb_meter = fair_meter_lookup_by_name(meter_groups, acl->meter);
> - if (!nb_meter) {
> - continue;
> + sync_acl_fair_meter(ctx, meter_groups, od->nbs->acls[i],
> + &sb_meters, &used_sb_meters);
> + }
> + struct ovn_port_group *pg;
> + HMAP_FOR_EACH (pg, key_node, port_groups) {
> + if (ovn_port_group_ls_find(pg, &od->nbs->header_.uuid)) {
> + for (size_t i = 0; i < pg->nb_pg->n_acls; i++) {
> + sync_acl_fair_meter(ctx, meter_groups,
> pg->nb_pg->acls[i],
> + &sb_meters, &used_sb_meters);
> + }
> }
> -
> - char *meter_name = alloc_acl_log_unique_meter_name(acl);
> - sync_meters_iterate_nb_meter(ctx, meter_name, nb_meter,
> - &sb_meters);
> - free(meter_name);
> }
> }
>
> + const char *used_meter;
> + const char *used_meter_next;
> + SSET_FOR_EACH_SAFE (used_meter, used_meter_next, &used_sb_meters) {
> + shash_find_and_delete(&sb_meters, used_meter);
> + sset_delete(&used_sb_meters, SSET_NODE_FROM_NAME(used_meter));
> + }
> + sset_destroy(&used_sb_meters);
> +
> struct shash_node *node, *next;
> SHASH_FOR_EACH_SAFE (node, next, &sb_meters) {
> sbrec_meter_delete(node->data);
> @@ -12601,7 +12632,7 @@ ovnnb_db_run(struct northd_context *ctx,
>
> sync_address_sets(ctx);
> sync_port_groups(ctx, &port_groups);
> - sync_meters(ctx, datapaths, &meter_groups);
> + sync_meters(ctx, datapaths, &meter_groups, &port_groups);
> sync_dns_entries(ctx, datapaths);
>
> struct ovn_northd_lb *lb;
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 91eb9a3..df03b6e 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -1843,20 +1843,25 @@ AT_KEYWORDS([acl log meter fair])
> ovn_start
>
> check ovn-nbctl ls-add sw0
> +check ovn-nbctl ls-add sw1
> check ovn-nbctl lsp-add sw0 sw0-p1 -- lsp-set-addresses sw0-p1
> "50:54:00:00:00:01 10.0.0.11"
> check ovn-nbctl lsp-add sw0 sw0-p2 -- lsp-set-addresses sw0-p2
> "50:54:00:00:00:02 10.0.0.12"
> -check ovn-nbctl lsp-add sw0 sw0-p3 -- lsp-set-addresses sw0-p3
> "50:54:00:00:00:03 10.0.0.13"
> +check ovn-nbctl lsp-add sw1 sw1-p3 -- lsp-set-addresses sw1-p3
> "50:54:00:00:00:03 10.0.0.13"
> +check ovn-nbctl pg-add pg0 sw0-p1 sw0-p2 sw1-p3
>
> check ovn-nbctl meter-add meter_me drop 1 pktps
> nb_meter_uuid=$(fetch_column nb:Meter _uuid name=meter_me)
>
> check ovn-nbctl acl-add sw0 to-lport 1002 'outport == "sw0-p1" && ip4.src ==
> 10.0.0.12' allow
> check ovn-nbctl acl-add sw0 to-lport 1002 'outport == "sw0-p1" && ip4.src ==
> 10.0.0.13' allow
> +check ovn-nbctl acl-add pg0 to-lport 1002 'outport == "pg0" && ip4.src ==
> 10.0.0.11' drop
>
> acl1=$(ovn-nbctl --bare --column _uuid,match find acl | grep -B1 '10.0.0.12'
> | head -1)
> acl2=$(ovn-nbctl --bare --column _uuid,match find acl | grep -B1 '10.0.0.13'
> | head -1)
> +acl3=$(ovn-nbctl --bare --column _uuid,match find acl | grep -B1 '10.0.0.11'
> | head -1)
> check ovn-nbctl set acl $acl1 log=true severity=alert meter=meter_me
> name=acl_one
> check ovn-nbctl set acl $acl2 log=true severity=info meter=meter_me
> name=acl_two
> +check ovn-nbctl set acl $acl3 log=true severity=info meter=meter_me
> name=acl_three
> check ovn-nbctl --wait=sb sync
>
> check_row_count nb:meter 1
> @@ -1865,8 +1870,9 @@ check_column meter_me nb:meter name
> check_acl_lflow() {
> acl_log_name=$1
> meter_name=$2
> + ls=$3
> # echo checking that logical flow for acl log $acl_log_name has
> $meter_name
> - AT_CHECK([ovn-sbctl lflow-list | grep ls_out_acl | \
> + AT_CHECK([ovn-sbctl lflow-list $ls | grep ls_out_acl | \
> grep "\"${acl_log_name}\"" | \
> grep -c "meter=\"${meter_name}\""], [0], [1
> ])
> @@ -1882,55 +1888,63 @@ check_meter_by_name() {
>
> # Make sure 'fair' value properly affects the Meters in SB
> check_meter_by_name meter_me
> -check_meter_by_name NOT meter_me__${acl1} meter_me__${acl2}
> +check_meter_by_name NOT meter_me__${acl1} meter_me__${acl2} meter_me__${acl3}
>
> check ovn-nbctl --wait=sb set Meter $nb_meter_uuid fair=true
> -check_meter_by_name meter_me meter_me__${acl1} meter_me__${acl2}
> +check_meter_by_name meter_me meter_me__${acl1} meter_me__${acl2}
> meter_me__${acl3}
>
> check ovn-nbctl --wait=sb set Meter $nb_meter_uuid fair=false
> check_meter_by_name meter_me
> -check_meter_by_name NOT meter_me__${acl1} meter_me__${acl2}
> +check_meter_by_name NOT meter_me__${acl1} meter_me__${acl2} meter_me__${acl3}
>
> check ovn-nbctl --wait=sb set Meter $nb_meter_uuid fair=true
> -check_meter_by_name meter_me meter_me__${acl1} meter_me__${acl2}
> +check_meter_by_name meter_me meter_me__${acl1} meter_me__${acl2}
> meter_me__${acl3}
>
> # Change template meter and make sure that is reflected on acl meters as well
> template_band=$(fetch_column nb:meter bands name=meter_me)
> check ovn-nbctl --wait=sb set meter_band $template_band rate=123
> # Make sure that every Meter_Band has the right rate. (ovn-northd
> -# creates 3 identical Meter_Band rows, all identical; ovn-northd-ddlog
> +# creates 4 identical Meter_Band rows, all identical; ovn-northd-ddlog
> # creates just 1. It doesn't matter, they work just as well.)
> n_meter_bands=$(count_rows meter_band)
> -AT_FAIL_IF([test "$n_meter_bands" != 1 && test "$n_meter_bands" != 3])
> +AT_FAIL_IF([test "$n_meter_bands" != 1 && test "$n_meter_bands" != 4])
> check_row_count meter_band $n_meter_bands rate=123
>
> # Check meter in logical flows for acl logs
> -check_acl_lflow acl_one meter_me__${acl1}
> -check_acl_lflow acl_two meter_me__${acl2}
> +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
>
> # Stop using meter for acl1
> check ovn-nbctl --wait=sb clear acl $acl1 meter
> check_meter_by_name meter_me meter_me__${acl2}
> check_meter_by_name NOT meter_me__${acl1}
> -check_acl_lflow acl_two meter_me__${acl2}
> +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
>
> # Remove template Meter should remove all others as well
> check ovn-nbctl --wait=sb meter-del meter_me
> check_row_count meter 0
> # Check that logical flow remains but uses non-unique meter since fair
> # attribute is lost by the removal of the Meter row.
> -check_acl_lflow acl_two meter_me
> +check_acl_lflow acl_two meter_me sw0
> +check_acl_lflow acl_three meter_me sw0
> +check_acl_lflow acl_three meter_me sw1
>
> # Re-add template meter and make sure acl2's meter is back in sb
> check ovn-nbctl --wait=sb --fair meter-add meter_me drop 1 pktps
> check_meter_by_name meter_me meter_me__${acl2}
> check_meter_by_name NOT meter_me__${acl1}
> -check_acl_lflow acl_two meter_me__${acl2}
> +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
>
> # Remove acl2
> sw0=$(fetch_column nb:logical_switch _uuid name=sw0)
> check ovn-nbctl --wait=sb remove logical_switch $sw0 acls $acl2
> -check_meter_by_name meter_me
> +check_meter_by_name meter_me meter_me__${acl3}
> check_meter_by_name NOT meter_me__${acl1} meter_me__${acl2}
>
> AT_CLEANUP
> --
> 1.8.3.1
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
