Skip to content

Commit

Permalink
northd: Fix ACL fair log meters for Port_Group ACLs.
Browse files Browse the repository at this point in the history 
Commit 880dca9 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: 880dca9 ("northd: Enhance the implementation of ACL log meters (pre-ddlog merge).")
Reported-by: Dmitry Yusupov <dyusupov@nvidia.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Acked-by: Flavio Fernandes <flavio@flaviof.com>
Signed-off-by: Numan Siddique <numans@ovn.org>
  • Loading branch information
@dceara @numansiddique
dceara authored and numansiddique committed Jan 18, 2021
1 parent 8952ae2 commit bf4f75f
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 29 deletions.
61 changes: 46 additions & 15 deletions northd/ovn-northd.c
View file
Expand Up @@ -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;
Expand All @@ -12074,16 +12077,35 @@ 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
* a private copy of its meter in the SB table.
*/
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) {
Expand All @@ -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);
}

/*
Expand All @@ -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);
Expand Down Expand Up @@ -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;
Expand Down
42 changes: 28 additions & 14 deletions tests/ovn-northd.at
View file
Expand Up @@ -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
Expand All @@ -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
])
Expand All @@ -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
Expand Down

0 comments on commit bf4f75f

Please sign in to comment.