northd: introduce ls_datapath_group column in lb sb db table

Introduce ls_datapath_group column in the load_balancer table of the SB
sb and deprecate datapath_group one. This patch make the table symmetric
with lr_datapath_group column in the load_balancer table of SB db.

Reviewed-by: Ales Musil <amusil@redhat.com>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>

controller/chassis.c

@@ -369,6 +369,7 @@ chassis_build_other_config(const struct ovs_chassis_cfg *ovs_cfg,
     smap_replace(config, OVN_FEATURE_MAC_BINDING_TIMESTAMP, "true");
     smap_replace(config, OVN_FEATURE_CT_LB_RELATED, "true");
     smap_replace(config, OVN_FEATURE_FDB_TIMESTAMP, "true");
+    smap_replace(config, OVN_FEATURE_LS_DPG_COLUMN, "true");
 }
 
 /*
@@ -502,6 +503,12 @@ chassis_other_config_changed(const struct ovs_chassis_cfg *ovs_cfg,
         return true;
     }
 
+    if (!smap_get_bool(&chassis_rec->other_config,
+                       OVN_FEATURE_LS_DPG_COLUMN,
+                       false)) {
+        return true;
+    }
+
     return false;
 }
 
@@ -632,6 +639,7 @@ update_supported_sset(struct sset *supported)
     sset_add(supported, OVN_FEATURE_MAC_BINDING_TIMESTAMP);
     sset_add(supported, OVN_FEATURE_CT_LB_RELATED);
     sset_add(supported, OVN_FEATURE_FDB_TIMESTAMP);
+    sset_add(supported, OVN_FEATURE_LS_DPG_COLUMN);
 }
 
 static void

controller/lflow.c

@@ -1892,6 +1892,7 @@ add_lb_ct_snat_hairpin_vip_flow(const struct ovn_controller_lb *lb,
                                           local_datapaths, &match,
                                           &ofpacts, flow_table);
         }
+        /* datapath_group column is deprecated. */
         if (lb->slb->datapath_group) {
             for (size_t i = 0; i < lb->slb->datapath_group->n_datapaths; i++) {
                 add_lb_ct_snat_hairpin_for_dp(
@@ -1900,6 +1901,15 @@ add_lb_ct_snat_hairpin_vip_flow(const struct ovn_controller_lb *lb,
                     local_datapaths, &match, &ofpacts, flow_table);
             }
         }
+        if (lb->slb->ls_datapath_group) {
+            for (size_t i = 0;
+                 i < lb->slb->ls_datapath_group->n_datapaths; i++) {
+                add_lb_ct_snat_hairpin_for_dp(
+                    lb, !!lb_vip->vip_port,
+                    lb->slb->ls_datapath_group->datapaths[i],
+                    local_datapaths, &match, &ofpacts, flow_table);
+            }
+        }
     }
 
     ofpbuf_uninit(&ofpacts);

controller/local_data.c

@@ -661,8 +661,16 @@ lb_is_local(const struct sbrec_load_balancer *sbrec_lb,
         }
     }
 
+    /* datapath_group column is deprecated. */
     struct sbrec_logical_dp_group *dp_group = sbrec_lb->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;
+        }
+    }
 
+    dp_group = sbrec_lb->ls_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)) {

controller/ovn-controller.c

@@ -2809,12 +2809,19 @@ load_balancers_by_dp_init(const struct hmap *local_datapaths,
             load_balancers_by_dp_add_one(local_datapaths,
                                          lb->datapaths[i], lb, lbs);
         }
+        /* datapath_group column is deprecated. */
         for (size_t i = 0; lb->datapath_group
                            && i < lb->datapath_group->n_datapaths; i++) {
             load_balancers_by_dp_add_one(local_datapaths,
                                          lb->datapath_group->datapaths[i],
                                          lb, lbs);
         }
+        for (size_t i = 0; lb->ls_datapath_group
+                           && i < lb->ls_datapath_group->n_datapaths; i++) {
+            load_balancers_by_dp_add_one(local_datapaths,
+                                         lb->ls_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,

include/ovn/features.h

@@ -26,6 +26,7 @@
 #define OVN_FEATURE_MAC_BINDING_TIMESTAMP "mac-binding-timestamp"
 #define OVN_FEATURE_CT_LB_RELATED "ovn-ct-lb-related"
 #define OVN_FEATURE_FDB_TIMESTAMP "fdb-timestamp"
+#define OVN_FEATURE_LS_DPG_COLUMN "ls-dpg-column"
 
 /* OVS datapath supported features.  Based on availability OVN might generate
  * different types of openflows.

northd/en-sync-sb.c

@@ -221,7 +221,7 @@ en_sync_to_sb_lb_run(struct engine_node *node, void *data OVS_UNUSED)
 
     sync_lbs(eng_ctx->ovnsb_idl_txn, sb_load_balancer_table,
              &northd_data->ls_datapaths, &northd_data->lr_datapaths,
-             &northd_data->lb_datapaths_map);
+             &northd_data->lb_datapaths_map, &northd_data->features);
     engine_set_node_state(node, EN_UPDATED);
 }
 

northd/northd.c

@@ -491,6 +491,15 @@ build_chassis_features(const struct sbrec_chassis_table *sbrec_chassis_table,
             chassis_features->fdb_timestamp) {
             chassis_features->fdb_timestamp = false;
         }
+
+        bool ls_dpg_column =
+            smap_get_bool(&chassis->other_config,
+                          OVN_FEATURE_LS_DPG_COLUMN,
+                          false);
+        if (!ls_dpg_column &&
+            chassis_features->ls_dpg_column) {
+            chassis_features->ls_dpg_column = false;
+        }
     }
 }
 
@@ -4535,7 +4544,8 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
          const struct sbrec_load_balancer_table *sbrec_load_balancer_table,
          struct ovn_datapaths *ls_datapaths,
          struct ovn_datapaths *lr_datapaths,
-         struct hmap *lb_dps_map)
+         struct hmap *lb_dps_map,
+         struct chassis_features *chassis_features)
 {
     struct hmap ls_dp_groups = HMAP_INITIALIZER(&ls_dp_groups);
     struct hmap lr_dp_groups = HMAP_INITIALIZER(&lr_dp_groups);
@@ -4578,9 +4588,13 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
 
         /* Find or create datapath group for this load balancer. */
         if (lb_dps->n_nb_ls) {
+            struct sbrec_logical_dp_group *ls_datapath_group
+                = chassis_features->ls_dpg_column
+                    ? sb_lb->slb->ls_datapath_group
+                    : sb_lb->slb->datapath_group; /* deprecated */
             sb_lb->dpg = ovn_dp_group_get_or_create(
                     ovnsb_txn, &ls_dp_groups,
-                    sb_lb->slb->datapath_group,
+                    ls_datapath_group,
                     lb_dps->n_nb_ls, lb_dps->nb_ls_map,
                     ods_size(ls_datapaths), true,
                     ls_datapaths, NULL);
@@ -4631,9 +4645,13 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
 
         /* Find or create datapath group for this load balancer. */
         if (!lb_dpg && lb_dps->n_nb_ls) {
+            struct sbrec_logical_dp_group *ls_datapath_group
+                = chassis_features->ls_dpg_column
+                    ? sbrec_lb->ls_datapath_group
+                    : sbrec_lb->datapath_group; /* deprecated */
             lb_dpg = ovn_dp_group_get_or_create(
                     ovnsb_txn, &ls_dp_groups,
-                    sbrec_lb->datapath_group,
+                    ls_datapath_group,
                     lb_dps->n_nb_ls, lb_dps->nb_ls_map,
                     ods_size(ls_datapaths), true,
                     ls_datapaths, NULL);
@@ -4652,9 +4670,19 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
         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 ? lb_dpg->dp_group : NULL
-        );
+
+        if (chassis_features->ls_dpg_column) {
+            sbrec_load_balancer_set_ls_datapath_group(
+                sbrec_lb, lb_dpg ? lb_dpg->dp_group : NULL
+            );
+            sbrec_load_balancer_set_datapath_group(sbrec_lb, NULL);
+        } else {
+            /* datapath_group column is deprecated. */
+            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
         );
@@ -17704,6 +17732,7 @@ northd_enable_all_features(struct northd_data *data)
         .mac_binding_timestamp = true,
         .ct_lb_related = true,
         .fdb_timestamp = true,
+        .ls_dpg_column = true,
     };
 }
 

northd/northd.h

@@ -70,6 +70,7 @@ struct chassis_features {
     bool mac_binding_timestamp;
     bool ct_lb_related;
     bool fdb_timestamp;
+    bool ls_dpg_column;
 };
 
 /* A collection of datapaths. E.g. all logical switch datapaths, or all
@@ -366,7 +367,8 @@ const struct ovn_datapath *northd_get_datapath_for_port(
 void sync_lbs(struct ovsdb_idl_txn *, const struct sbrec_load_balancer_table *,
               struct ovn_datapaths *ls_datapaths,
               struct ovn_datapaths *lr_datapaths,
-              struct hmap *lbs);
+              struct hmap *lbs,
+              struct chassis_features *chassis_features);
 
 void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports);
 bool sync_pbs_for_northd_ls_changes(struct tracked_ls_changes *);

ovn-sb.ovsschema

@@ -1,7 +1,7 @@
 {
     "name": "OVN_Southbound",
-    "version": "20.28.0",
-    "cksum": "2567491918 30745",
+    "version": "20.29.0",
+    "cksum": "3967183656 30959",
     "tables": {
         "SB_Global": {
             "columns": {
@@ -534,6 +534,10 @@
                     {"type": {"key": {"type": "uuid",
                                       "refTable": "Logical_DP_Group"},
                               "min": 0, "max": 1}},
+                "ls_datapath_group":
+                    {"type": {"key": {"type": "uuid",
+                                      "refTable": "Logical_DP_Group"},
+                              "min": 0, "max": 1}},
                 "lr_datapath_group":
                     {"type": {"key": {"type": "uuid",
                                       "refTable": "Logical_DP_Group"},

ovn-sb.xml

@@ -4888,6 +4888,12 @@ tcp.flags = RST;
     </column>
 
     <column name="datapath_group">
+      Deprecated. The group of datapaths to which this load balancer applies
+      to. This means that the same load balancer applies to all datapaths in
+      a group.
+    </column>
+
+    <column name="ls_datapath_group">
       The group of datapaths to which this load balancer applies to.  This
       means that the same load balancer applies to all datapaths in a group.
     </column>

tests/ovn-controller.at

@@ -526,6 +526,10 @@ OVS_WAIT_UNTIL([
     test "$(ovn-sbctl get chassis hv1 other_config:port-up-notif)" = '"true"'
 ])
 
+OVS_WAIT_UNTIL([
+    test "$(ovn-sbctl get chassis hv1 other_config:ls-dpg-column)" = '"true"'
+])
+
 OVN_CLEANUP([hv1])
 AT_CLEANUP
 ])

tests/ovn-northd.at

@@ -2861,7 +2861,7 @@ lbg0_uuid=$(fetch_column sb:load_balancer _uuid name=lbg0)
 echo
 echo "__file__:__line__: Check that SB lb0 has sw0 in datapaths column."
 
-lb0_dp_group=$(fetch_column sb:load_balancer datapath_group name=lb0)
+lb0_dp_group=$(fetch_column sb:load_balancer ls_datapath_group name=lb0)
 AT_CHECK_UNQUOTED([ovn-sbctl --bare --columns _uuid,datapaths find Logical_DP_Group dnl
                     | grep -A1 $lb0_dp_group | tail -1], [0], [dnl
 $sw0_sb_uuid
@@ -2872,7 +2872,7 @@ check_column "" sb:datapath_binding load_balancers external_ids:name=sw0
 echo
 echo "__file__:__line__: Check that SB lbg0 has sw0 in datapaths column."
 
-lbg0_dp_group=$(fetch_column sb:load_balancer datapath_group name=lbg0)
+lbg0_dp_group=$(fetch_column sb:load_balancer ls_datapath_group name=lbg0)
 AT_CHECK_UNQUOTED([ovn-sbctl --bare --columns _uuid,datapaths find Logical_DP_Group dnl
                     | grep -A1 $lbg0_dp_group | tail -1], [0], [dnl
 $sw0_sb_uuid
@@ -2975,7 +2975,7 @@ echo "__file__:__line__: Check that SB lbg1 has vips and protocol columns are se
 check_column "20.0.0.30:80=20.0.0.50:8080 udp" sb:load_balancer vips,protocol name=lbg1
 
 lb1_uuid=$(fetch_column sb:load_balancer _uuid name=lb1)
-lb1_dp_group=$(fetch_column sb:load_balancer datapath_group name=lb1)
+lb1_dp_group=$(fetch_column sb:load_balancer ls_datapath_group name=lb1)
 
 echo
 echo "__file__:__line__: Check that SB lb1 has sw1 in datapaths column."
@@ -2986,7 +2986,7 @@ $sw1_sb_uuid
 ])
 
 lbg1_uuid=$(fetch_column sb:load_balancer _uuid name=lbg1)
-lbg1_dp_group=$(fetch_column sb:load_balancer datapath_group name=lbg1)
+lbg1_dp_group=$(fetch_column sb:load_balancer ls_datapath_group name=lbg1)
 
 echo
 echo "__file__:__line__: Check that SB lbg1 has sw0 and sw1 in datapaths column."
@@ -6035,9 +6035,9 @@ ovn_start
 check ovn-nbctl ls-add ls -- lb-add lb1 10.0.0.1:80 10.0.0.2:80 -- ls-lb-add ls lb1
 check ovn-nbctl --wait=sb sync
 
-dps=$(fetch_column Load_Balancer datapath_group)
+dps=$(fetch_column Load_Balancer ls_datapath_group)
 nlb=$(fetch_column nb:Load_Balancer _uuid)
-AT_CHECK([ovn-sbctl create Load_Balancer name=lb1 datapath_group="$dps" external_ids="lb_id=$nlb"], [0], [ignore])
+AT_CHECK([ovn-sbctl create Load_Balancer name=lb1 ls_datapath_group="$dps" external_ids="lb_id=$nlb"], [0], [ignore])
 
 check ovn-nbctl --wait=sb sync
 check_row_count Load_Balancer 1
@@ -8718,7 +8718,7 @@ AT_CHECK([grep "ls_in_lb " S1flows | sed 's/table=../table=??/' | sort], [0], [d
 
 ovn-sbctl get datapath S0 _uuid > dp_uuids
 ovn-sbctl get datapath S1 _uuid >> dp_uuids
-lb_dp_group=$(ovn-sbctl --bare --columns datapath_group find Load_Balancer name=lb0)
+lb_dp_group=$(ovn-sbctl --bare --columns ls_datapath_group find Load_Balancer name=lb0)
 AT_CHECK_UNQUOTED([ovn-sbctl --bare --columns _uuid,datapaths find Logical_DP_Group dnl
                     | grep -A1 $lb_dp_group | tail -1 | tr ' ' '\n' | sort], [0], [dnl
 $(cat dp_uuids | sort)

utilities/ovn-sbctl.c

@@ -396,7 +396,9 @@ pre_get_info(struct ctl_context *ctx)
     ovsdb_idl_add_column(ctx->idl, &sbrec_mac_binding_col_mac);
 
     ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_datapaths);
+    /* datapath_group column is deprecated. */
     ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_datapath_group);
+    ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_ls_datapath_group);
     ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_vips);
     ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_name);
     ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_protocol);
@@ -932,10 +934,15 @@ cmd_lflow_list_load_balancers(struct ctl_context *ctx, struct vconn *vconn,
                     break;
                 }
             }
+            /* datapath_group column is deprecated. */
             if (lb->datapath_group && !dp_found) {
                 dp_found = datapath_group_contains_datapath(lb->datapath_group,
                                                             datapath);
             }
+            if (lb->ls_datapath_group && !dp_found) {
+                dp_found = datapath_group_contains_datapath(
+                        lb->ls_datapath_group, datapath);
+            }
             if (!dp_found) {
                 continue;
             }
@@ -954,11 +961,17 @@ cmd_lflow_list_load_balancers(struct ctl_context *ctx, struct vconn *vconn,
                 print_vflow_datapath_name(lb->datapaths[i], true,
                                           &ctx->output);
             }
+            /* datapath_group column is deprecated. */
             for (size_t i = 0; lb->datapath_group
                                && i < lb->datapath_group->n_datapaths; i++) {
                 print_vflow_datapath_name(lb->datapath_group->datapaths[i],
                                           true, &ctx->output);
             }
+            for (size_t i = 0; lb->ls_datapath_group
+                    && i < lb->ls_datapath_group->n_datapaths; i++) {
+                print_vflow_datapath_name(lb->ls_datapath_group->datapaths[i],
+                                          true, &ctx->output);
+            }
         }
 
         ds_put_cstr(&ctx->output, "\n  vips:\n");