From d2b69af321ad8064d42aad2fd3d15857334e2d63 Mon Sep 17 00:00:00 2001

From: Dumitru Ceara <dceara@redhat.com>

Date: Fri, 15 Jan 2021 14:41:19 +0100

Subject: [PATCH] northd: Fix ACL fair log meters for Port_Group ACLs.

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 <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>

(cherry picked from master commit bf4f75f90c3309dbcfac8e098a2c1ff2d822e77d)

Change-Id: If6f19df6fe0b84abc2fbb7356bf59c2d5eb496e1

---

 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 fa2bd73..49afc2f 100644

--- a/northd/ovn-northd.c

+++ b/northd/ovn-northd.c

@@ -12250,17 +12250,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;

@@ -12282,6 +12285,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

@@ -12289,9 +12310,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) {

@@ -12301,7 +12323,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);

     }

     /*

@@ -12315,19 +12337,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);

@@ -12825,7 +12856,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