On 8/29/23 04:49, Dumitru Ceara wrote:
On 8/21/23 19:33, Mark Michelson wrote:
Hi Dumitru,

I have one finding in the test code, but other than that it looks good.


Hi Mark,

Thanks for the review!

On 8/17/23 11:25, Dumitru Ceara wrote:
This is beneficial in a few ways:
- first, it reduces the number of different types of data the northd
    I-P node has to process.
- it turns out the northd I-P node (whose recompute is rather costly)
    doesn't really depend on meters or ACLs.
- prepares the ground for a pure I-P implementation for ACLs, with a
    simple/clear dependency between NB.ACL and the lflow I-P node.

  From a meters synchronization perspective this commit doesn't change any
of the existing behavior and logic.  It mostly just moves the meters
related code out of northd.c and into en-meters.c.

Signed-off-by: Dumitru Ceara <[email protected]>
---
   lib/stopwatch-names.h    |    1
   northd/automake.mk       |    2
   northd/en-lflow.c        |    5 +
   northd/en-meters.c       |  281
++++++++++++++++++++++++++++++++++++++++++++++
   northd/en-meters.h       |   44 +++++++
   northd/en-northd.c       |    6 -
   northd/inc-proc-northd.c |   14 ++
   northd/northd.c          |  235 +-------------------------------------
   northd/northd.h          |    4 -
   northd/ovn-northd.c      |    3
   tests/ovn-northd.at      |   36 ++++++
   11 files changed, 390 insertions(+), 241 deletions(-)
   create mode 100644 northd/en-meters.c
   create mode 100644 northd/en-meters.h

diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h
index de6fca4ccc..98535fff5a 100644
--- a/lib/stopwatch-names.h
+++ b/lib/stopwatch-names.h
@@ -30,5 +30,6 @@
   #define LFLOWS_IGMP_STOPWATCH_NAME "lflows_igmp"
   #define LFLOWS_DP_GROUPS_STOPWATCH_NAME "lflows_dp_groups"
   #define LFLOWS_TO_SB_STOPWATCH_NAME "lflows_to_sb"
+#define SYNC_METERS_RUN_STOPWATCH_NAME "sync_meters_run"
     #endif
diff --git a/northd/automake.mk b/northd/automake.mk
index b17f1fdb54..f52766de0c 100644
--- a/northd/automake.mk
+++ b/northd/automake.mk
@@ -12,6 +12,8 @@ northd_ovn_northd_SOURCES = \
       northd/en-northd.h \
       northd/en-lflow.c \
       northd/en-lflow.h \
+    northd/en-meters.c \
+    northd/en-meters.h \
       northd/en-northd-output.c \
       northd/en-northd-output.h \
       northd/en-sync-sb.c \
diff --git a/northd/en-lflow.c b/northd/en-lflow.c
index 28ab1c67fb..0886d4c5ca 100644
--- a/northd/en-lflow.c
+++ b/northd/en-lflow.c
@@ -20,6 +20,7 @@
     #include "en-lflow.h"
   #include "en-northd.h"
+#include "en-meters.h"
     #include "lib/inc-proc-eng.h"
   #include "northd.h"
@@ -35,6 +36,8 @@ lflow_get_input_data(struct engine_node *node,
                        struct lflow_input *lflow_input)
   {
       struct northd_data *northd_data =
engine_get_input_data("northd", node);
+    struct sync_meters_data *sync_meters_data =
+        engine_get_input_data("sync_meters", node);
       lflow_input->nbrec_bfd_table =
           EN_OVSDB_GET(engine_get_input("NB_bfd", node));
       lflow_input->sbrec_bfd_table =
@@ -56,7 +59,7 @@ lflow_get_input_data(struct engine_node *node,
       lflow_input->ls_ports = &northd_data->ls_ports;
       lflow_input->lr_ports = &northd_data->lr_ports;
       lflow_input->port_groups = &northd_data->port_groups;
-    lflow_input->meter_groups = &northd_data->meter_groups;
+    lflow_input->meter_groups = &sync_meters_data->meter_groups;
       lflow_input->lbs = &northd_data->lbs;
       lflow_input->features = &northd_data->features;
       lflow_input->ovn_internal_version_changed =
diff --git a/northd/en-meters.c b/northd/en-meters.c
new file mode 100644
index 0000000000..aabd002b62
--- /dev/null
+++ b/northd/en-meters.c
@@ -0,0 +1,281 @@
+/*
+ * Copyright (c) 2023, Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <config.h>
+
+#include "openvswitch/vlog.h"
+#include "stopwatch.h"
+
+#include "en-meters.h"
+#include "lib/stopwatch-names.h"
+
+VLOG_DEFINE_THIS_MODULE(en_meters);
+
+static void build_meter_groups(struct shash *meter_group,
+                               const struct nbrec_meter_table *);
+static void sync_meters(struct ovsdb_idl_txn *ovnsb_txn,
+                        const struct nbrec_meter_table *,
+                        const struct nbrec_acl_table *,
+                        const struct sbrec_meter_table *,
+                        struct shash *meter_groups);
+
+void
+*en_sync_meters_init(struct engine_node *node OVS_UNUSED,
+                     struct engine_arg *arg OVS_UNUSED)
+{
+    struct sync_meters_data *data = xmalloc(sizeof *data);
+
+    *data = (struct sync_meters_data) {
+        .meter_groups = SHASH_INITIALIZER(&data->meter_groups),
+    };
+    return data;
+}
+
+void
+en_sync_meters_cleanup(void *data_)
+{
+    struct sync_meters_data *data = data_;
+
+    shash_destroy(&data->meter_groups);
+}
+
+void
+en_sync_meters_run(struct engine_node *node, void *data_)
+{
+    struct sync_meters_data *data = data_;
+
+    const struct nbrec_acl_table *acl_table =
+        EN_OVSDB_GET(engine_get_input("NB_acl", node));
+
+    const struct nbrec_meter_table *nb_meter_table =
+        EN_OVSDB_GET(engine_get_input("NB_meter", node));
+
+    const struct sbrec_meter_table *sb_meter_table =
+        EN_OVSDB_GET(engine_get_input("SB_meter", node));
+
+    const struct engine_context *eng_ctx = engine_get_context();
+
+    stopwatch_start(SYNC_METERS_RUN_STOPWATCH_NAME, time_msec());
+
+    build_meter_groups(&data->meter_groups, nb_meter_table);
+
+    sync_meters(eng_ctx->ovnsb_idl_txn, nb_meter_table, acl_table,
+                sb_meter_table, &data->meter_groups);
+
+    stopwatch_stop(SYNC_METERS_RUN_STOPWATCH_NAME, time_msec());
+    engine_set_node_state(node, EN_UPDATED);
+}
+
+const struct nbrec_meter*
+fair_meter_lookup_by_name(const struct shash *meter_groups,
+                          const char *meter_name)
+{
+    const struct nbrec_meter *nb_meter =
+        meter_name ? shash_find_data(meter_groups, meter_name) : NULL;
+    if (nb_meter) {
+        return (nb_meter->fair && *nb_meter->fair) ? nb_meter : NULL;
+    }
+    return NULL;
+}
+
+struct band_entry {
+    int64_t rate;
+    int64_t burst_size;
+    const char *action;
+};
+
+static int
+band_cmp(const void *band1_, const void *band2_)
+{
+    const struct band_entry *band1p = band1_;
+    const struct band_entry *band2p = band2_;
+
+    if (band1p->rate != band2p->rate) {
+        return band1p->rate - band2p->rate;
+    } else if (band1p->burst_size != band2p->burst_size) {
+        return band1p->burst_size - band2p->burst_size;
+    } else {
+        return strcmp(band1p->action, band2p->action);
+    }
+}
+
+static bool
+bands_need_update(const struct nbrec_meter *nb_meter,
+                  const struct sbrec_meter *sb_meter)
+{
+    if (nb_meter->n_bands != sb_meter->n_bands) {
+        return true;
+    }
+
+    /* Place the Northbound entries in sorted order. */
+    struct band_entry *nb_bands;
+    nb_bands = xmalloc(sizeof *nb_bands * nb_meter->n_bands);
+    for (size_t i = 0; i < nb_meter->n_bands; i++) {
+        struct nbrec_meter_band *nb_band = nb_meter->bands[i];
+
+        nb_bands[i].rate = nb_band->rate;
+        nb_bands[i].burst_size = nb_band->burst_size;
+        nb_bands[i].action = nb_band->action;
+    }
+    qsort(nb_bands, nb_meter->n_bands, sizeof *nb_bands, band_cmp);
+
+    /* Place the Southbound entries in sorted order. */
+    struct band_entry *sb_bands;
+    sb_bands = xmalloc(sizeof *sb_bands * sb_meter->n_bands);
+    for (size_t i = 0; i < sb_meter->n_bands; i++) {
+        struct sbrec_meter_band *sb_band = sb_meter->bands[i];
+
+        sb_bands[i].rate = sb_band->rate;
+        sb_bands[i].burst_size = sb_band->burst_size;
+        sb_bands[i].action = sb_band->action;
+    }
+    qsort(sb_bands, sb_meter->n_bands, sizeof *sb_bands, band_cmp);
+
+    bool need_update = false;
+    for (size_t i = 0; i < nb_meter->n_bands; i++) {
+        if (band_cmp(&nb_bands[i], &sb_bands[i])) {
+            need_update = true;
+            break;
+        }
+    }
+
+    free(nb_bands);
+    free(sb_bands);
+
+    return need_update;
+}
+
+static void
+sync_meters_iterate_nb_meter(struct ovsdb_idl_txn *ovnsb_txn,
+                             const char *meter_name,
+                             const struct nbrec_meter *nb_meter,
+                             struct shash *sb_meters,
+                             struct sset *used_sb_meters)
+{
+    const struct sbrec_meter *sb_meter;
+    bool new_sb_meter = false;
+
+    sb_meter = shash_find_data(sb_meters, meter_name);
+    if (!sb_meter) {
+        sb_meter = sbrec_meter_insert(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;
+        sb_bands = xcalloc(nb_meter->n_bands, sizeof *sb_bands);
+        for (size_t i = 0; i < nb_meter->n_bands; i++) {
+            const struct nbrec_meter_band *nb_band = nb_meter->bands[i];
+
+            sb_bands[i] = sbrec_meter_band_insert(ovnsb_txn);
+
+            sbrec_meter_band_set_action(sb_bands[i], nb_band->action);
+            sbrec_meter_band_set_rate(sb_bands[i], nb_band->rate);
+            sbrec_meter_band_set_burst_size(sb_bands[i],
+                                            nb_band->burst_size);
+        }
+        sbrec_meter_set_bands(sb_meter, sb_bands, nb_meter->n_bands);
+        free(sb_bands);
+    }
+
+    sbrec_meter_set_unit(sb_meter, nb_meter->unit);
+}
+
+static void
+sync_acl_fair_meter(struct ovsdb_idl_txn *ovnsb_txn,
+                    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(ovnsb_txn, meter_name, nb_meter,
sb_meters,
+                                 used_sb_meters);
+    free(meter_name);
+}
+
+static void
+build_meter_groups(struct shash *meter_groups,
+                   const struct nbrec_meter_table *nb_meter_table)
+{
+    const struct nbrec_meter *nb_meter;
+
+    shash_clear(meter_groups);
+    NBREC_METER_TABLE_FOR_EACH (nb_meter, nb_meter_table) {
+        shash_add(meter_groups, nb_meter->name, nb_meter);
+    }
+}
+
+/* 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 ovsdb_idl_txn *ovnsb_txn,
+            const struct nbrec_meter_table *nbrec_meter_table,
+            const struct nbrec_acl_table *nbrec_acl_table,
+            const struct sbrec_meter_table *sbrec_meter_table,
+            struct shash *meter_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_TABLE_FOR_EACH (sb_meter, sbrec_meter_table) {
+        shash_add(&sb_meters, sb_meter->name, sb_meter);
+    }
+
+    const struct nbrec_meter *nb_meter;
+    NBREC_METER_TABLE_FOR_EACH (nb_meter, nbrec_meter_table) {
+        sync_meters_iterate_nb_meter(ovnsb_txn, nb_meter->name,
nb_meter,
+                                     &sb_meters, &used_sb_meters);
+    }
+
+    /*
+     * In addition to creating Meters in the SB from the block above,
check
+     * and see if additional rows are needed to get ACLs logs
individually
+     * rate-limited.
+     */
+    const struct nbrec_acl *acl;
+    NBREC_ACL_TABLE_FOR_EACH (acl, nbrec_acl_table) {
+        sync_acl_fair_meter(ovnsb_txn, meter_groups, acl,
+                            &sb_meters, &used_sb_meters);
+    }
+
+    const char *used_meter;
+    SSET_FOR_EACH_SAFE (used_meter, &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;
+    SHASH_FOR_EACH_SAFE (node, &sb_meters) {
+        sbrec_meter_delete(node->data);
+        shash_delete(&sb_meters, node);
+    }
+    shash_destroy(&sb_meters);
+}
diff --git a/northd/en-meters.h b/northd/en-meters.h
new file mode 100644
index 0000000000..a1743282e3
--- /dev/null
+++ b/northd/en-meters.h
@@ -0,0 +1,44 @@
+/*
+ * Copyright (c) 2023, Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#ifndef EN_METERS_H
+#define EN_METERS_H 1
+
+#include "openvswitch/shash.h"
+
+#include "lib/inc-proc-eng.h"
+#include "lib/ovn-nb-idl.h"
+#include "lib/ovn-sb-idl.h"
+
+struct sync_meters_data {
+    struct shash meter_groups;
+};
+
+void *en_sync_meters_init(struct engine_node *, struct engine_arg *);
+void en_sync_meters_cleanup(void *data);
+void en_sync_meters_run(struct engine_node *, void *data);
+
+const struct nbrec_meter *fair_meter_lookup_by_name(
+    const struct shash *meter_groups,
+    const char *meter_name);
+
+static inline char*
+alloc_acl_log_unique_meter_name(const struct nbrec_acl *acl)
+{
+    return xasprintf("%s__" UUID_FMT,
+                     acl->meter, UUID_ARGS(&acl->header_.uuid));
+}
+
+#endif /* EN_ACL_H */
diff --git a/northd/en-northd.c b/northd/en-northd.c
index 044fa70190..cc05efdbbc 100644
--- a/northd/en-northd.c
+++ b/northd/en-northd.c
@@ -76,10 +76,6 @@ northd_get_input_data(struct engine_node *node,
           EN_OVSDB_GET(engine_get_input("NB_load_balancer_group", node));
       input_data->nbrec_port_group_table =
           EN_OVSDB_GET(engine_get_input("NB_port_group", node));
-    input_data->nbrec_meter_table =
-        EN_OVSDB_GET(engine_get_input("NB_meter", node));
-    input_data->nbrec_acl_table =
-        EN_OVSDB_GET(engine_get_input("NB_acl", node));
       input_data->nbrec_static_mac_binding_table =
           EN_OVSDB_GET(engine_get_input("NB_static_mac_binding", node));
       input_data->nbrec_chassis_template_var_table =
@@ -107,8 +103,6 @@ northd_get_input_data(struct engine_node *node,
           EN_OVSDB_GET(engine_get_input("SB_service_monitor", node));
       input_data->sbrec_port_group_table =
           EN_OVSDB_GET(engine_get_input("SB_port_group", node));
-    input_data->sbrec_meter_table =
-        EN_OVSDB_GET(engine_get_input("SB_meter", node));
       input_data->sbrec_dns_table =
           EN_OVSDB_GET(engine_get_input("SB_dns", node));
       input_data->sbrec_ip_multicast_table =
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index d328deb222..1de0551b3d 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -33,6 +33,7 @@
   #include "en-northd.h"
   #include "en-lflow.h"
   #include "en-northd-output.h"
+#include "en-meters.h"
   #include "en-sync-sb.h"
   #include "en-sync-from-sb.h"
   #include "unixctl.h"
@@ -135,6 +136,7 @@ static ENGINE_NODE(lflow, "lflow");
   static ENGINE_NODE(mac_binding_aging, "mac_binding_aging");
   static ENGINE_NODE(mac_binding_aging_waker, "mac_binding_aging_waker");
   static ENGINE_NODE(northd_output, "northd_output");
+static ENGINE_NODE(sync_meters, "sync_meters");
   static ENGINE_NODE(sync_to_sb, "sync_to_sb");
   static ENGINE_NODE(sync_to_sb_addr_set, "sync_to_sb_addr_set");
   static ENGINE_NODE(fdb_aging, "fdb_aging");
@@ -148,10 +150,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
       engine_add_input(&en_northd, &en_nb_port_group, NULL);
       engine_add_input(&en_northd, &en_nb_load_balancer, NULL);
       engine_add_input(&en_northd, &en_nb_load_balancer_group, NULL);
-    engine_add_input(&en_northd, &en_nb_acl, NULL);
       engine_add_input(&en_northd, &en_nb_logical_router, NULL);
       engine_add_input(&en_northd, &en_nb_mirror, NULL);
-    engine_add_input(&en_northd, &en_nb_meter, NULL);
       engine_add_input(&en_northd, &en_nb_static_mac_binding, NULL);
       engine_add_input(&en_northd, &en_nb_chassis_template_var, NULL);
   @@ -188,7 +188,13 @@ void inc_proc_northd_init(struct ovsdb_idl_loop
*nb,
       engine_add_input(&en_fdb_aging, &en_northd, NULL);
       engine_add_input(&en_fdb_aging, &en_fdb_aging_waker, NULL);
   +    engine_add_input(&en_sync_meters, &en_nb_acl, NULL);
+    engine_add_input(&en_sync_meters, &en_nb_meter, NULL);
+    engine_add_input(&en_sync_meters, &en_sb_meter, NULL);
+
       engine_add_input(&en_lflow, &en_nb_bfd, NULL);
+    engine_add_input(&en_lflow, &en_nb_acl, NULL);
+    engine_add_input(&en_lflow, &en_sync_meters, NULL);
       engine_add_input(&en_lflow, &en_sb_bfd, NULL);
       engine_add_input(&en_lflow, &en_sb_logical_flow, NULL);
       engine_add_input(&en_lflow, &en_sb_multicast_group, NULL);
@@ -204,9 +210,11 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
         /* en_sync_to_sb engine node syncs the SB database tables from
        * the NB database tables.
-     * Right now this engine only syncs the SB Address_Set table.
+     * Right now this engine syncs the SB Address_Set table and
+     * SB Meter/Meter_Band tables.
        */
       engine_add_input(&en_sync_to_sb, &en_sync_to_sb_addr_set, NULL);
+    engine_add_input(&en_sync_to_sb, &en_sync_meters, NULL);
         engine_add_input(&en_sync_from_sb, &en_northd,
                        sync_from_sb_northd_handler);
diff --git a/northd/northd.c b/northd/northd.c
index 13f2ae0565..48fb44a8be 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -21,6 +21,7 @@
   #include "bitmap.h"
   #include "coverage.h"
   #include "dirs.h"
+#include "en-meters.h"
   #include "ipam.h"
   #include "openvswitch/dynamic-string.h"
   #include "hash.h"
@@ -4980,25 +4981,22 @@ ls_port_create(struct ovsdb_idl_txn
*ovnsb_txn, struct hmap *ls_ports,
   }
     static bool
-check_ls_changes_other_than_lsp(const struct nbrec_logical_switch *ls)
+check_ls_changes_other_than_lsp_acl(const struct nbrec_logical_switch
*ls)
   {
       /* Check if the columns are changed in this row. */
       enum nbrec_logical_switch_column_id col;
       for (col = 0; col < NBREC_LOGICAL_SWITCH_N_COLUMNS; col++) {
-        if (nbrec_logical_switch_is_updated(ls, col) &&
-            col != NBREC_LOGICAL_SWITCH_COL_PORTS) {
+        if (nbrec_logical_switch_is_updated(ls, col)) {
+            if (col == NBREC_LOGICAL_SWITCH_COL_ACLS ||
+                col == NBREC_LOGICAL_SWITCH_COL_PORTS) {
+                continue;
+            }
               return true;
           }
       }
         /* Check if the referenced rows are changed.
          XXX: Need a better OVSDB IDL interface for this check. */
-    for (size_t i = 0; i < ls->n_acls; i++) {
-        if (nbrec_acl_row_get_seqno(ls->acls[i],
-                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
-            return true;
-        }
-    }
       if (ls->copp && nbrec_copp_row_get_seqno(ls->copp,
                                   OVSDB_IDL_CHANGE_MODIFY) > 0) {
           return true;
@@ -5106,7 +5104,7 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
*ovnsb_idl_txn,
           }
             /* Now only able to handle lsp changes. */
-        if (check_ls_changes_other_than_lsp(changed_ls)) {
+        if (check_ls_changes_other_than_lsp_acl(changed_ls)) {
               goto fail;
           }
   @@ -6987,25 +6985,6 @@ build_acl_hints(struct ovn_datapath *od,
       }
   }
   -static const struct nbrec_meter*
-fair_meter_lookup_by_name(const struct shash *meter_groups,
-                          const char *meter_name)
-{
-    const struct nbrec_meter *nb_meter =
-        meter_name ? shash_find_data(meter_groups, meter_name) : NULL;
-    if (nb_meter) {
-        return (nb_meter->fair && *nb_meter->fair) ? nb_meter : NULL;
-    }
-    return NULL;
-}
-
-static char*
-alloc_acl_log_unique_meter_name(const struct nbrec_acl *acl)
-{
-    return xasprintf("%s__" UUID_FMT,
-                     acl->meter, UUID_ARGS(&acl->header_.uuid));
-}
-
   static void
   build_acl_log_meter(struct ds *actions, const struct nbrec_acl *acl,
                       const struct shash *meter_groups)
@@ -16608,183 +16587,6 @@ sync_port_groups(struct ovsdb_idl_txn
*ovnsb_txn,
       shash_destroy(&sb_port_groups);
   }
   -struct band_entry {
-    int64_t rate;
-    int64_t burst_size;
-    const char *action;
-};
-
-static int
-band_cmp(const void *band1_, const void *band2_)
-{
-    const struct band_entry *band1p = band1_;
-    const struct band_entry *band2p = band2_;
-
-    if (band1p->rate != band2p->rate) {
-        return band1p->rate - band2p->rate;
-    } else if (band1p->burst_size != band2p->burst_size) {
-        return band1p->burst_size - band2p->burst_size;
-    } else {
-        return strcmp(band1p->action, band2p->action);
-    }
-}
-
-static bool
-bands_need_update(const struct nbrec_meter *nb_meter,
-                  const struct sbrec_meter *sb_meter)
-{
-    if (nb_meter->n_bands != sb_meter->n_bands) {
-        return true;
-    }
-
-    /* Place the Northbound entries in sorted order. */
-    struct band_entry *nb_bands;
-    nb_bands = xmalloc(sizeof *nb_bands * nb_meter->n_bands);
-    for (size_t i = 0; i < nb_meter->n_bands; i++) {
-        struct nbrec_meter_band *nb_band = nb_meter->bands[i];
-
-        nb_bands[i].rate = nb_band->rate;
-        nb_bands[i].burst_size = nb_band->burst_size;
-        nb_bands[i].action = nb_band->action;
-    }
-    qsort(nb_bands, nb_meter->n_bands, sizeof *nb_bands, band_cmp);
-
-    /* Place the Southbound entries in sorted order. */
-    struct band_entry *sb_bands;
-    sb_bands = xmalloc(sizeof *sb_bands * sb_meter->n_bands);
-    for (size_t i = 0; i < sb_meter->n_bands; i++) {
-        struct sbrec_meter_band *sb_band = sb_meter->bands[i];
-
-        sb_bands[i].rate = sb_band->rate;
-        sb_bands[i].burst_size = sb_band->burst_size;
-        sb_bands[i].action = sb_band->action;
-    }
-    qsort(sb_bands, sb_meter->n_bands, sizeof *sb_bands, band_cmp);
-
-    bool need_update = false;
-    for (size_t i = 0; i < nb_meter->n_bands; i++) {
-        if (band_cmp(&nb_bands[i], &sb_bands[i])) {
-            need_update = true;
-            break;
-        }
-    }
-
-    free(nb_bands);
-    free(sb_bands);
-
-    return need_update;
-}
-
-static void
-sync_meters_iterate_nb_meter(struct ovsdb_idl_txn *ovnsb_txn,
-                             const char *meter_name,
-                             const struct nbrec_meter *nb_meter,
-                             struct shash *sb_meters,
-                             struct sset *used_sb_meters)
-{
-    const struct sbrec_meter *sb_meter;
-    bool new_sb_meter = false;
-
-    sb_meter = shash_find_data(sb_meters, meter_name);
-    if (!sb_meter) {
-        sb_meter = sbrec_meter_insert(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;
-        sb_bands = xcalloc(nb_meter->n_bands, sizeof *sb_bands);
-        for (size_t i = 0; i < nb_meter->n_bands; i++) {
-            const struct nbrec_meter_band *nb_band = nb_meter->bands[i];
-
-            sb_bands[i] = sbrec_meter_band_insert(ovnsb_txn);
-
-            sbrec_meter_band_set_action(sb_bands[i], nb_band->action);
-            sbrec_meter_band_set_rate(sb_bands[i], nb_band->rate);
-            sbrec_meter_band_set_burst_size(sb_bands[i],
-                                            nb_band->burst_size);
-        }
-        sbrec_meter_set_bands(sb_meter, sb_bands, nb_meter->n_bands);
-        free(sb_bands);
-    }
-
-    sbrec_meter_set_unit(sb_meter, nb_meter->unit);
-}
-
-static void
-sync_acl_fair_meter(struct ovsdb_idl_txn *ovnsb_txn,
-                    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(ovnsb_txn, 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 ovsdb_idl_txn *ovnsb_txn,
-            const struct nbrec_meter_table *nbrec_meter_table,
-            const struct nbrec_acl_table *nbrec_acl_table,
-            const struct sbrec_meter_table *sbrec_meter_table,
-            struct shash *meter_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_TABLE_FOR_EACH (sb_meter, sbrec_meter_table) {
-        shash_add(&sb_meters, sb_meter->name, sb_meter);
-    }
-
-    const struct nbrec_meter *nb_meter;
-    NBREC_METER_TABLE_FOR_EACH (nb_meter, nbrec_meter_table) {
-        sync_meters_iterate_nb_meter(ovnsb_txn, nb_meter->name,
nb_meter,
-                                     &sb_meters, &used_sb_meters);
-    }
-
-    /*
-     * In addition to creating Meters in the SB from the block above,
check
-     * and see if additional rows are needed to get ACLs logs
individually
-     * rate-limited.
-     */
-    const struct nbrec_acl *acl;
-    NBREC_ACL_TABLE_FOR_EACH (acl, nbrec_acl_table) {
-        sync_acl_fair_meter(ovnsb_txn, meter_groups, acl,
-                            &sb_meters, &used_sb_meters);
-    }
-
-    const char *used_meter;
-    SSET_FOR_EACH_SAFE (used_meter, &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;
-    SHASH_FOR_EACH_SAFE (node, &sb_meters) {
-        sbrec_meter_delete(node->data);
-        shash_delete(&sb_meters, node);
-    }
-    shash_destroy(&sb_meters);
-}
-
   static bool
   mirror_needs_update(const struct nbrec_mirror *nb_mirror,
                       const struct sbrec_mirror *sb_mirror)
@@ -17254,16 +17056,6 @@ build_mcast_groups(const struct
sbrec_igmp_group_table *sbrec_igmp_group_table,
       }
   }
   -static void
-build_meter_groups(const struct nbrec_meter_table *nbrec_meter_table,
-                   struct shash *meter_groups)
-{
-    const struct nbrec_meter *nb_meter;
-    NBREC_METER_TABLE_FOR_EACH (nb_meter, nbrec_meter_table) {
-        shash_add(meter_groups, nb_meter->name, nb_meter);
-    }
-}
-
   static const struct nbrec_static_mac_binding *
   static_mac_binding_by_port_ip(
       const struct nbrec_static_mac_binding_table *nbrec_static_mb_table,
@@ -17405,7 +17197,6 @@ northd_init(struct northd_data *data)
       hmap_init(&data->ls_ports);
       hmap_init(&data->lr_ports);
       hmap_init(&data->port_groups);
-    shash_init(&data->meter_groups);
       hmap_init(&data->lbs);
       hmap_init(&data->lb_groups);
       ovs_list_init(&data->lr_list);
@@ -17443,12 +17234,6 @@ northd_destroy(struct northd_data *data)
         hmap_destroy(&data->port_groups);
   -    struct shash_node *node;
-    SHASH_FOR_EACH_SAFE (node, &data->meter_groups) {
-        shash_delete(&data->meter_groups, node);
-    }
-    shash_destroy(&data->meter_groups);
-
       /* XXX Having to explicitly clean up macam here
        * is a bit strange. We don't explicitly initialize
        * macam in this module, but this is the logical place
@@ -17587,7 +17372,6 @@ ovnnb_db_run(struct northd_input *input_data,
       build_ip_mcast(ovnsb_txn, input_data->sbrec_ip_multicast_table,
                      input_data->sbrec_ip_mcast_by_dp,
                      &data->ls_datapaths.datapaths);
-    build_meter_groups(input_data->nbrec_meter_table,
&data->meter_groups);
       build_static_mac_binding_table(ovnsb_txn,
           input_data->nbrec_static_mac_binding_table,
           input_data->sbrec_static_mac_binding_table,
@@ -17602,9 +17386,6 @@ ovnnb_db_run(struct northd_input *input_data,
                &data->ls_datapaths, &data->lbs);
       sync_port_groups(ovnsb_txn, input_data->sbrec_port_group_table,
                        &data->port_groups);
-    sync_meters(ovnsb_txn, input_data->nbrec_meter_table,
-                input_data->nbrec_acl_table,
input_data->sbrec_meter_table,
-                &data->meter_groups);
       sync_mirrors(ovnsb_txn, input_data->nbrec_mirror_table,
                    input_data->sbrec_mirror_table);
       sync_dns_entries(ovnsb_txn, input_data->sbrec_dns_table,
diff --git a/northd/northd.h b/northd/northd.h
index f3e63b1e1a..dcc3acabd5 100644
--- a/northd/northd.h
+++ b/northd/northd.h
@@ -31,8 +31,6 @@ struct northd_input {
       const struct nbrec_load_balancer_group_table
           *nbrec_load_balancer_group_table;
       const struct nbrec_port_group_table *nbrec_port_group_table;
-    const struct nbrec_meter_table *nbrec_meter_table;
-    const struct nbrec_acl_table *nbrec_acl_table;
       const struct nbrec_static_mac_binding_table
           *nbrec_static_mac_binding_table;
       const struct nbrec_chassis_template_var_table
@@ -50,7 +48,6 @@ struct northd_input {
       const struct sbrec_load_balancer_table *sbrec_load_balancer_table;
       const struct sbrec_service_monitor_table
*sbrec_service_monitor_table;
       const struct sbrec_port_group_table *sbrec_port_group_table;
-    const struct sbrec_meter_table *sbrec_meter_table;
       const struct sbrec_dns_table *sbrec_dns_table;
       const struct sbrec_ip_multicast_table *sbrec_ip_multicast_table;
       const struct sbrec_static_mac_binding_table
@@ -109,7 +106,6 @@ struct northd_data {
       struct hmap ls_ports;
       struct hmap lr_ports;
       struct hmap port_groups;
-    struct shash meter_groups;
       struct hmap lbs;
       struct hmap lb_groups;
       struct ovs_list lr_list;
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 4fa1b039ea..45362f4f93 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -836,6 +836,9 @@ main(int argc, char *argv[])
           ovsdb_idl_omit_alert(ovnsb_idl_loop.idl,
                                &sbrec_multicast_group_columns[i]);
       }
+    for (size_t i = 0; i < SBREC_METER_N_COLUMNS; i++) {
+        ovsdb_idl_omit_alert(ovnsb_idl_loop.idl,
&sbrec_meter_columns[i]);
+    }
         unixctl_command_register("sb-connection-status", "", 0, 0,
                                ovn_conn_show, ovnsb_idl_loop.idl);
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index d5be3be75b..dc8bf253f1 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -9,6 +9,8 @@ m4_define([_DUMP_DB_TABLES], [
       ovn-sbctl list logical_flow >> $1
       ovn-sbctl list port_binding >> $1
       ovn-sbctl list address_set >> $1
+    ovn-sbctl list meter >> $1
+    ovn-sbctl list meter_band >> $1
   ])
     # CHECK_NO_CHANGE_AFTER_RECOMPUTE
@@ -9679,6 +9681,40 @@ OVN_CLEANUP([hv1])
   AT_CLEANUP
   ])
   +OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([ACL/Meter incremental processing - no northd recompute])
+ovn_start
+
+check_recompute_counter() {
+    northd_recomp=$(as northd ovn-appctl -t NORTHD_TYPE
inc-engine/show-stats northd recompute)
+    AT_CHECK([test x$northd_recomp = x$1])
+
+    northd_recomp=$(as northd ovn-appctl -t NORTHD_TYPE
inc-engine/show-stats northd recompute)
+    AT_CHECK([test x$northd_recomp = x$1])
+
+    sync_meters_recomp=$(as northd ovn-appctl -t NORTHD_TYPE
inc-engine/show-stats sync_meters recompute)
+    AT_CHECK([test x$sync_meters_recomp = x$3])
+}

In check_recompute_counter(), the same comparison is performed twice for
the $1 parameter. Then the $3 parameter is used in the third comparison.
The $2 parameter is never used. I assume $2 was supposed to be used for
something, but it's not clear what.


Oops, it was supposed to ensure we don't recompute the lflow node.

Does the following incremental change look OK to you?
If so, I can fold it in and apply the series.

Yes, this looks good to me.

Acked-by: Mark Michelson <[email protected]>


Thanks,
Dumitru

diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index dc8bf253f1..9df02b422f 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -9689,8 +9689,8 @@ check_recompute_counter() {
      northd_recomp=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats 
northd recompute)
      AT_CHECK([test x$northd_recomp = x$1])
- northd_recomp=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute)
-    AT_CHECK([test x$northd_recomp = x$1])
+    lflow_recomp=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats 
lflow recompute)
+    AT_CHECK([test x$lflow_recomp = x$2])
sync_meters_recomp=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats sync_meters recompute)
      AT_CHECK([test x$sync_meters_recomp = x$3])
---

+
+check ovn-nbctl --wait=sb ls-add ls
+
+check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
+check ovn-nbctl --wait=sb meter-add m drop 1 pktps
+check ovn-nbctl --wait=sb acl-add ls from-lport 1 1 allow
+dnl Only triggers recompute of the sync_meters and lflow nodes.
+check_recompute_counter 0 2 2
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
+check ovn-nbctl --wait=sb meter-del m
+check ovn-nbctl --wait=sb acl-del ls
+dnl Only triggers recompute of the sync_meters and lflow nodes.
+check_recompute_counter 0 2 2
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+AT_CLEANUP
+])
+
   OVN_FOR_EACH_NORTHD_NO_HV([
   AT_SETUP([check fip and lb flows])
   AT_KEYWORDS([fip-lb-flows])

_______________________________________________
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

Reply via email to