diff mbox series

[ovs-dev,v2,5/5] northd: Use the same UUID for SB representation of Load_Balancer.

Message ID 20241114074945.1278590-6-amusil@redhat.com
State Accepted
Headers show
Series Reuse UUID for SB rows if there is 1:1 mapping | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Ales Musil Nov. 14, 2024, 7:49 a.m. UTC 
The NB Load_Balancer has exactly one corresponding row in SB database.
Use the NB UUID for the SB representation which makes the processing easier
and the link between the rows is obvious at first glance.

Signed-off-by: Ales Musil <amusil@redhat.com>
Acked-by: Mark Michelson <mmichels@redhat.com>
Acked-by: Numan Siddique <numans@ovn.org>
---
v2: Rebase on top of current main.
    Add ack from Numan and Mark.
---
 northd/en-sync-sb.c      | 80 ++++++----------------------------------
 northd/en-sync-sb.h      |  2 -
 northd/inc-proc-northd.c |  4 +-
 tests/ovn-northd.at      |  3 +-
 4 files changed, 16 insertions(+), 73 deletions(-)
diff mbox series

Patch

diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
index 9bd8a1fc6..d9dc25eb8 100644
--- a/northd/en-sync-sb.c
+++ b/northd/en-sync-sb.c
@@ -225,7 +225,6 @@  struct sb_lb_record {
     const struct sbrec_load_balancer *sbrec_lb;
     struct ovn_dp_group *ls_dpg;
     struct ovn_dp_group *lr_dpg;
-    struct uuid sb_uuid;
 };
 
 struct sb_lb_table {
@@ -268,7 +267,6 @@  static bool sync_changed_lbs(struct sb_lb_table *,
                              struct ovn_datapaths *ls_datapaths,
                              struct ovn_datapaths *lr_datapaths,
                              struct chassis_features *);
-static bool check_sb_lb_duplicates(const struct sbrec_load_balancer_table *);
 
 void *
 en_sync_to_sb_lb_init(struct engine_node *node OVS_UNUSED,
@@ -346,23 +344,6 @@  sync_to_sb_lb_northd_handler(struct engine_node *node, void *data_)
     return true;
 }
 
-bool
-sync_to_sb_lb_sb_load_balancer(struct engine_node *node, void *data OVS_UNUSED)
-{
-    const struct sbrec_load_balancer_table *sb_load_balancer_table =
-        EN_OVSDB_GET(engine_get_input("SB_load_balancer", node));
-
-    /* The only reason to handle SB.Load_Balancer updates is to detect
-     * spurious records being created in clustered databases due to
-     * lack of indexing on the SB.Load_Balancer table.  All other changes
-     * are valid and performed by northd, the only write-client for
-     * this table. */
-    if (check_sb_lb_duplicates(sb_load_balancer_table)) {
-        return false;
-    }
-    return true;
-}
-
 /* sync_to_sb_pb engine node functions.
  * This engine node syncs the SB Port Bindings (partly).
  * en_northd engine create the SB Port binding rows and
@@ -690,14 +671,7 @@  sb_lb_table_build_and_sync(
     const struct sbrec_load_balancer *sbrec_lb;
     SBREC_LOAD_BALANCER_TABLE_FOR_EACH_SAFE (sbrec_lb,
                                              sb_lb_table) {
-        const char *nb_lb_uuid = smap_get(&sbrec_lb->external_ids, "lb_id");
-        struct uuid lb_uuid;
-        if (!nb_lb_uuid || !uuid_from_string(&lb_uuid, nb_lb_uuid)) {
-            sbrec_load_balancer_delete(sbrec_lb);
-            continue;
-        }
-
-        sb_lb = sb_lb_table_find(&tmp_sb_lbs, &lb_uuid);
+        sb_lb = sb_lb_table_find(&tmp_sb_lbs, &sbrec_lb->header_.uuid);
         if (sb_lb) {
             sb_lb->sbrec_lb = sbrec_lb;
             bool success = sync_sb_lb_record(sb_lb, sbrec_lb, sb_dpgrp_table,
@@ -751,17 +725,9 @@  sync_sb_lb_record(struct sb_lb_record *sb_lb,
     pre_sync_lr_dpg = sb_lb->lr_dpg;
 
     if (!sbrec_lb) {
-        sb_lb->sb_uuid = uuid_random();
-        sbrec_lb =  sbrec_load_balancer_insert_persist_uuid(ovnsb_txn,
-                                                            &sb_lb->sb_uuid);
-        char *lb_id = xasprintf(
-            UUID_FMT, UUID_ARGS(&lb_dps->lb->nlb->header_.uuid));
-        const struct smap external_ids =
-            SMAP_CONST1(&external_ids, "lb_id", lb_id);
-        sbrec_load_balancer_set_external_ids(sbrec_lb, &external_ids);
-        free(lb_id);
+        const struct uuid *nb_uuid = &lb_dps->lb->nlb->header_.uuid;
+        sbrec_lb = sbrec_load_balancer_insert_persist_uuid(ovnsb_txn, nb_uuid);
     } else {
-        sb_lb->sb_uuid = sbrec_lb->header_.uuid;
         sbrec_ls_dp_group =
             chassis_features->ls_dpg_column
             ? sbrec_lb->ls_datapath_group
@@ -923,13 +889,11 @@  sync_changed_lbs(struct sb_lb_table *sb_lbs,
 
     HMAPX_FOR_EACH (hmapx_node, &trk_lbs->deleted) {
         lb_dps = hmapx_node->data;
-
-        sb_lb = sb_lb_table_find(&sb_lbs->entries,
-                                 &lb_dps->lb->nlb->header_.uuid);
+        const struct uuid *nb_uuid = &lb_dps->lb->nlb->header_.uuid;
+        sb_lb = sb_lb_table_find(&sb_lbs->entries, nb_uuid);
         if (sb_lb) {
             const struct sbrec_load_balancer *sbrec_lb =
-                sbrec_load_balancer_table_get_for_uuid(sb_lb_table,
-                                                       &sb_lb->sb_uuid);
+                sbrec_load_balancer_table_get_for_uuid(sb_lb_table, nb_uuid);
             if (sbrec_lb) {
                 sbrec_load_balancer_delete(sbrec_lb);
             }
@@ -941,9 +905,9 @@  sync_changed_lbs(struct sb_lb_table *sb_lbs,
 
     HMAPX_FOR_EACH (hmapx_node, &trk_lbs->crupdated) {
         lb_dps = hmapx_node->data;
+        const struct uuid *nb_uuid = &lb_dps->lb->nlb->header_.uuid;
 
-        sb_lb = sb_lb_table_find(&sb_lbs->entries,
-                                 &lb_dps->lb->nlb->header_.uuid);
+        sb_lb = sb_lb_table_find(&sb_lbs->entries, nb_uuid);
 
         if (!sb_lb && !lb_dps->n_nb_ls && !lb_dps->n_nb_lr) {
             continue;
@@ -953,17 +917,15 @@  sync_changed_lbs(struct sb_lb_table *sb_lbs,
             sb_lb = xzalloc(sizeof *sb_lb);
             sb_lb->lb_dps = lb_dps;
             hmap_insert(&sb_lbs->entries, &sb_lb->key_node,
-                        uuid_hash(&lb_dps->lb->nlb->header_.uuid));
+                        uuid_hash(nb_uuid));
         } else {
             sb_lb->sbrec_lb =
-                sbrec_load_balancer_table_get_for_uuid(sb_lb_table,
-                                                       &sb_lb->sb_uuid);
+                sbrec_load_balancer_table_get_for_uuid(sb_lb_table, nb_uuid);
         }
 
         if (sb_lb && !lb_dps->n_nb_ls && !lb_dps->n_nb_lr) {
             const struct sbrec_load_balancer *sbrec_lb =
-                sbrec_load_balancer_table_get_for_uuid(sb_lb_table,
-                                                       &sb_lb->sb_uuid);
+                sbrec_load_balancer_table_get_for_uuid(sb_lb_table, nb_uuid);
             if (sbrec_lb) {
                 sbrec_load_balancer_delete(sbrec_lb);
             }
@@ -981,23 +943,3 @@  sync_changed_lbs(struct sb_lb_table *sb_lbs,
 
     return true;
 }
-
-static bool
-check_sb_lb_duplicates(const struct sbrec_load_balancer_table *table)
-{
-    struct sset existing_nb_lb_uuids =
-        SSET_INITIALIZER(&existing_nb_lb_uuids);
-    const struct sbrec_load_balancer *sbrec_lb;
-    bool duplicates = false;
-
-    SBREC_LOAD_BALANCER_TABLE_FOR_EACH (sbrec_lb, table) {
-        const char *nb_lb_uuid = smap_get(&sbrec_lb->external_ids, "lb_id");
-        if (nb_lb_uuid && !sset_add(&existing_nb_lb_uuids, nb_lb_uuid)) {
-            duplicates = true;
-            break;
-        }
-    }
-
-    sset_destroy(&existing_nb_lb_uuids);
-    return duplicates;
-}
diff --git a/northd/en-sync-sb.h b/northd/en-sync-sb.h
index 3bcbb8259..f08565eee 100644
--- a/northd/en-sync-sb.h
+++ b/northd/en-sync-sb.h
@@ -21,8 +21,6 @@  void *en_sync_to_sb_lb_init(struct engine_node *, struct engine_arg *);
 void en_sync_to_sb_lb_run(struct engine_node *, void *data);
 void en_sync_to_sb_lb_cleanup(void *data);
 bool sync_to_sb_lb_northd_handler(struct engine_node *, void *data OVS_UNUSED);
-bool sync_to_sb_lb_sb_load_balancer(struct engine_node *,
-                                    void *data OVS_UNUSED);
 
 void *en_sync_to_sb_pb_init(struct engine_node *, struct engine_arg *);
 void en_sync_to_sb_pb_run(struct engine_node *, void *data);
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index 480100d3c..f5f414959 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -308,8 +308,10 @@  void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
                      node_global_config_handler);
     engine_add_input(&en_sync_to_sb_lb, &en_northd,
                      sync_to_sb_lb_northd_handler);
+    /* There is nothing to handle as duplicates are prevented by using
+     * UUID mapping between NB and SB. */
     engine_add_input(&en_sync_to_sb_lb, &en_sb_load_balancer,
-                     sync_to_sb_lb_sb_load_balancer);
+                     engine_noop_handler);
     engine_add_input(&en_sync_to_sb_lb, &en_sb_logical_dp_group, NULL);
 
     engine_add_input(&en_sync_to_sb_pb, &en_northd,
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 73767d11b..0a71a62fc 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -6469,7 +6469,8 @@  check ovn-nbctl --wait=sb sync
 
 dps=$(fetch_column Load_Balancer ls_datapath_group)
 nlb=$(fetch_column nb:Load_Balancer _uuid)
-AT_CHECK([ovn-sbctl create Load_Balancer name=lb1 ls_datapath_group="$dps" external_ids="lb_id=$nlb"], [0], [ignore])
+AT_CHECK([ovn-sbctl --id="$nlb" create Load_Balancer name=lb1 ls_datapath_group="$dps"], [1], [ignore], [stderr])
+check grep -q "duplicate uuid" stderr
 
 check ovn-nbctl --wait=sb sync
 check_row_count Load_Balancer 1