On Sat, Jan 16, 2021 at 4:45 AM Flavio Fernandes <[email protected]> wrote:
> Acked-by: Flavio Fernandes <[email protected] <mailto:[email protected] > >> > Thanks Dumitru and Flavio F. I applied this patch to master and backported to branch-20.12. Numan > > > > On Jan 15, 2021, at 8:41 AM, Dumitru Ceara <[email protected]> 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 <[email protected]> > > Signed-off-by: Dumitru Ceara <[email protected]> > > --- > > 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
