northd: sync lb applied to logical routers in sb db lb table

Introduce lr_datapath_group column in the load_balancer table of the SB db. Sync load_balancers applied to logical_routers to Load_Balancer table in the SouthBound database. Reviewed-by: Ales Musil <amusil@redhat.com> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2193323 Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
ovn-org · Oct 10, 2023 · a349ec5 · a349ec5
1 parent a9051a1
commit a349ec5
Show file tree

Hide file tree

Showing 9 changed files with 227 additions and 28 deletions.
diff --git a/controller/local_data.c b/controller/local_data.c
@@ -670,5 +670,13 @@ lb_is_local(const struct sbrec_load_balancer *sbrec_lb,
         }
     }
 
+    dp_group = sbrec_lb->lr_datapath_group;
+    for (size_t i = 0; dp_group && i < dp_group->n_datapaths; i++) {
+        if (get_local_datapath(local_datapaths,
+                               dp_group->datapaths[i]->tunnel_key)) {
+            return true;
+        }
+    }
+
     return false;
 }
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
@@ -2815,6 +2815,12 @@ load_balancers_by_dp_init(const struct hmap *local_datapaths,
                                          lb->datapath_group->datapaths[i],
                                          lb, lbs);
         }
+        for (size_t i = 0; lb->lr_datapath_group
+                           && i < lb->lr_datapath_group->n_datapaths; i++) {
+            load_balancers_by_dp_add_one(local_datapaths,
+                                         lb->lr_datapath_group->datapaths[i],
+                                         lb, lbs);
+        }
     }
     return lbs;
 }

diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
@@ -220,7 +220,8 @@ en_sync_to_sb_lb_run(struct engine_node *node, void *data OVS_UNUSED)
     struct northd_data *northd_data = engine_get_input_data("northd", node);
 
     sync_lbs(eng_ctx->ovnsb_idl_txn, sb_load_balancer_table,
-             &northd_data->ls_datapaths, &northd_data->lb_datapaths_map);
+             &northd_data->ls_datapaths, &northd_data->lr_datapaths,
+             &northd_data->lb_datapaths_map);
     engine_set_node_state(node, EN_UPDATED);
 }
 

diff --git a/northd/northd.c b/northd/northd.c
@@ -4510,6 +4510,7 @@ struct sb_lb {
 
     const struct sbrec_load_balancer *slb;
     struct ovn_dp_group *dpg;
+    struct ovn_dp_group *lr_dpg;
     struct uuid lb_uuid;
 };
 
@@ -4532,10 +4533,12 @@ find_slb_in_sb_lbs(struct hmap *sb_lbs, const struct uuid *lb_uuid)
 void
 sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
          const struct sbrec_load_balancer_table *sbrec_load_balancer_table,
-         struct ovn_datapaths *ls_datapaths, struct hmap *lb_dps_map)
+         struct ovn_datapaths *ls_datapaths,
+         struct ovn_datapaths *lr_datapaths,
+         struct hmap *lb_dps_map)
 {
-    struct hmap dp_groups = HMAP_INITIALIZER(&dp_groups);
-    size_t bitmap_len = ods_size(ls_datapaths);
+    struct hmap ls_dp_groups = HMAP_INITIALIZER(&ls_dp_groups);
+    struct hmap lr_dp_groups = HMAP_INITIALIZER(&lr_dp_groups);
     struct ovn_lb_datapaths *lb_dps;
     struct hmap sb_lbs = HMAP_INITIALIZER(&sb_lbs);
 
@@ -4553,15 +4556,17 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
         }
 
         /* Delete any SB load balancer entries that refer to NB load balancers
-         * that don't exist anymore or are not applied to switches anymore.
+         * that don't exist anymore or are not applied to switches/routers
+         * anymore.
          *
          * There is also a special case in which duplicate LBs might be created
          * in the SB, e.g., due to the fact that OVSDB only ensures
          * "at-least-once" consistency for clustered database tables that
          * are not indexed in any way.
          */
         lb_dps = ovn_lb_datapaths_find(lb_dps_map, &lb_uuid);
-        if (!lb_dps || !lb_dps->n_nb_ls || !hmapx_add(&existing_lbs, lb_dps)) {
+        if (!lb_dps || (!lb_dps->n_nb_ls && !lb_dps->n_nb_lr) ||
+            !hmapx_add(&existing_lbs, lb_dps)) {
             sbrec_load_balancer_delete(sbrec_lb);
             continue;
         }
@@ -4572,20 +4577,30 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
         hmap_insert(&sb_lbs, &sb_lb->hmap_node, uuid_hash(&lb_uuid));
 
         /* Find or create datapath group for this load balancer. */
-        sb_lb->dpg = ovn_dp_group_get_or_create(ovnsb_txn, &dp_groups,
-                                                sb_lb->slb->datapath_group,
-                                                lb_dps->n_nb_ls,
-                                                lb_dps->nb_ls_map,
-                                                bitmap_len, true,
-                                                ls_datapaths, NULL);
+        if (lb_dps->n_nb_ls) {
+            sb_lb->dpg = ovn_dp_group_get_or_create(
+                    ovnsb_txn, &ls_dp_groups,
+                    sb_lb->slb->datapath_group,
+                    lb_dps->n_nb_ls, lb_dps->nb_ls_map,
+                    ods_size(ls_datapaths), true,
+                    ls_datapaths, NULL);
+        }
+        if (lb_dps->n_nb_lr) {
+            sb_lb->lr_dpg = ovn_dp_group_get_or_create(
+                    ovnsb_txn, &lr_dp_groups,
+                    sb_lb->slb->lr_datapath_group,
+                    lb_dps->n_nb_lr, lb_dps->nb_lr_map,
+                    ods_size(lr_datapaths), false,
+                    NULL, lr_datapaths);
+        }
     }
     hmapx_destroy(&existing_lbs);
 
     /* Create SB Load balancer records if not present and sync
      * the SB load balancer columns. */
     HMAP_FOR_EACH (lb_dps, hmap_node, lb_dps_map) {
 
-        if (!lb_dps->n_nb_ls) {
+        if (!lb_dps->n_nb_ls && !lb_dps->n_nb_lr) {
             continue;
         }
 
@@ -4598,8 +4613,8 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
 
         struct sb_lb *sb_lb = find_slb_in_sb_lbs(&sb_lbs,
                                             &lb_dps->lb->nlb->header_.uuid);
-        ovs_assert(!sb_lb || (sb_lb->slb && sb_lb->dpg));
-        struct ovn_dp_group *lb_dpg = NULL;
+        ovs_assert(!sb_lb || (sb_lb->slb && (sb_lb->dpg || sb_lb->lr_dpg)));
+        struct ovn_dp_group *lb_dpg = NULL, *lb_lr_dpg = NULL;
         if (!sb_lb) {
             sbrec_lb = sbrec_load_balancer_insert(ovnsb_txn);
             char *lb_id = xasprintf(
@@ -4611,35 +4626,56 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
         } else {
             sbrec_lb = sb_lb->slb;
             lb_dpg = sb_lb->dpg;
+            lb_lr_dpg = sb_lb->lr_dpg;
         }
 
         /* Find or create datapath group for this load balancer. */
-        if (!lb_dpg) {
-            lb_dpg = ovn_dp_group_get_or_create(ovnsb_txn, &dp_groups,
-                                                sbrec_lb->datapath_group,
-                                                lb_dps->n_nb_ls,
-                                                lb_dps->nb_ls_map, bitmap_len,
-                                                true, ls_datapaths, NULL);
+        if (!lb_dpg && lb_dps->n_nb_ls) {
+            lb_dpg = ovn_dp_group_get_or_create(
+                    ovnsb_txn, &ls_dp_groups,
+                    sbrec_lb->datapath_group,
+                    lb_dps->n_nb_ls, lb_dps->nb_ls_map,
+                    ods_size(ls_datapaths), true,
+                    ls_datapaths, NULL);
+        }
+        if (!lb_lr_dpg && lb_dps->n_nb_lr) {
+            lb_lr_dpg = ovn_dp_group_get_or_create(
+                    ovnsb_txn, &lr_dp_groups,
+                    sbrec_lb->lr_datapath_group,
+                    lb_dps->n_nb_lr, lb_dps->nb_lr_map,
+                    ods_size(lr_datapaths), false,
+                    NULL, lr_datapaths);
         }
 
         /* Update columns. */
         sbrec_load_balancer_set_name(sbrec_lb, lb_dps->lb->nlb->name);
         sbrec_load_balancer_set_vips(sbrec_lb,
                                      ovn_northd_lb_get_vips(lb_dps->lb));
         sbrec_load_balancer_set_protocol(sbrec_lb, lb_dps->lb->nlb->protocol);
-        sbrec_load_balancer_set_datapath_group(sbrec_lb, lb_dpg->dp_group);
+        sbrec_load_balancer_set_datapath_group(
+            sbrec_lb, lb_dpg ? lb_dpg->dp_group : NULL
+        );
+        sbrec_load_balancer_set_lr_datapath_group(
+            sbrec_lb, lb_lr_dpg ? lb_lr_dpg->dp_group : NULL
+        );
         sbrec_load_balancer_set_options(sbrec_lb, &options);
         /* Clearing 'datapaths' column, since 'dp_group' is in use. */
         sbrec_load_balancer_set_datapaths(sbrec_lb, NULL, 0);
         smap_destroy(&options);
     }
 
     struct ovn_dp_group *dpg;
-    HMAP_FOR_EACH_POP (dpg, node, &dp_groups) {
+    HMAP_FOR_EACH_POP (dpg, node, &ls_dp_groups) {
+        bitmap_free(dpg->bitmap);
+        free(dpg);
+    }
+    hmap_destroy(&ls_dp_groups);
+
+    HMAP_FOR_EACH_POP (dpg, node, &lr_dp_groups) {
         bitmap_free(dpg->bitmap);
         free(dpg);
     }
-    hmap_destroy(&dp_groups);
+    hmap_destroy(&lr_dp_groups);
 
     struct sb_lb *sb_lb;
     HMAP_FOR_EACH_POP (sb_lb, hmap_node, &sb_lbs) {
@@ -4654,6 +4690,13 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
     HMAP_FOR_EACH (od, key_node, &ls_datapaths->datapaths) {
         ovs_assert(od->nbs);
 
+        if (od->sb->n_load_balancers) {
+            sbrec_datapath_binding_set_load_balancers(od->sb, NULL, 0);
+        }
+    }
+    HMAP_FOR_EACH (od, key_node, &lr_datapaths->datapaths) {
+        ovs_assert(od->nbr);
+
         if (od->sb->n_load_balancers) {
             sbrec_datapath_binding_set_load_balancers(od->sb, NULL, 0);
         }

diff --git a/northd/northd.h b/northd/northd.h
@@ -364,7 +364,9 @@ const char *northd_get_svc_monitor_mac(void);
 const struct ovn_datapath *northd_get_datapath_for_port(
     const struct hmap *ls_ports, const char *port_name);
 void sync_lbs(struct ovsdb_idl_txn *, const struct sbrec_load_balancer_table *,
-              struct ovn_datapaths *ls_datapaths, struct hmap *lbs);
+              struct ovn_datapaths *ls_datapaths,
+              struct ovn_datapaths *lr_datapaths,
+              struct hmap *lbs);
 
 void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports);
 bool sync_pbs_for_northd_ls_changes(struct tracked_ls_changes *);

diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
@@ -1,7 +1,7 @@
 {
     "name": "OVN_Southbound",
-    "version": "20.27.4",
-    "cksum": "3194181501 30531",
+    "version": "20.28.0",
+    "cksum": "2567491918 30745",
     "tables": {
         "SB_Global": {
             "columns": {
@@ -534,6 +534,10 @@
                     {"type": {"key": {"type": "uuid",
                                       "refTable": "Logical_DP_Group"},
                               "min": 0, "max": 1}},
+                "lr_datapath_group":
+                    {"type": {"key": {"type": "uuid",
+                                      "refTable": "Logical_DP_Group"},
+                              "min": 0, "max": 1}},
                 "options": {
                      "type": {"key": "string",
                               "value": "string",

diff --git a/ovn-sb.xml b/ovn-sb.xml
@@ -4892,6 +4892,12 @@ tcp.flags = RST;
       means that the same load balancer applies to all datapaths in a group.
     </column>
 
+    <column name="lr_datapath_group">
+      The group of logical router datapaths to which this load balancer
+      applies to. This means that the same load balancer applies to all
+      datapaths in a group.
+    </column>
+
     <group title="Load_Balancer options">
     <column name="options" key="hairpin_snat_ip">
       IP to be used as source IP for packets that have been hair-pinned after

diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
@@ -2898,6 +2898,15 @@ sb:load_balancer vips,protocol name=lbg0
 
 check ovn-nbctl lr-add lr0 -- add logical_router lr0 load_balancer_group $lbg
 check ovn-nbctl --wait=sb lr-lb-add lr0 lb0
+check_row_count sb:load_balancer 2
+
+lr0_sb_uuid=$(fetch_column datapath_binding _uuid external_ids:name=lr0)
+lb0_lr_dp_group=$(fetch_column sb:load_balancer lr_datapath_group name=lb0)
+
+AT_CHECK_UNQUOTED([ovn-sbctl --bare --columns _uuid,datapaths find Logical_DP_Group dnl
+                    | grep -A1 $lb0_lr_dp_group | tail -1], [0], [dnl
+$lr0_sb_uuid
+])
 
 echo
 echo "__file__:__line__: Check that SB lb0 has only sw0 in datapaths column."
@@ -2943,7 +2952,13 @@ check_row_count sb:load_balancer 2
 lbg1=$(fetch_column nb:load_balancer _uuid name=lbg1)
 check ovn-nbctl add load_balancer_group $lbg load_balancer $lbg1
 check ovn-nbctl --wait=sb lr-lb-add lr0 lb1
-check_row_count sb:load_balancer 3
+check_row_count sb:load_balancer 4
+
+lb1_lr_dp_group=$(fetch_column sb:load_balancer lr_datapath_group name=lb1)
+AT_CHECK_UNQUOTED([ovn-sbctl --bare --columns _uuid,datapaths find Logical_DP_Group dnl
+                    | grep -A1 $lb1_lr_dp_group | tail -1], [0], [dnl
+$lr0_sb_uuid
+])
 
 echo
 echo "__file__:__line__: Associate lb1 to sw1 and check that lb1 is created in SB DB."