diff mbox series

[ovs-dev,RFC,1/2] northd: rename table datapath_group in ls_datapath_group in load_balancer sb db table

Message ID fdb9c74a4b401ce532f3f3f1a78ac2f4aec0621b.1686326027.git.lorenzo.bianconi@redhat.com
State Superseded
Headers show
Series sync lb applied to logical routers | expand

Checks

Context Check Description
ovsrobot/apply-robot fail apply and check: fail

Commit Message

Lorenzo Bianconi June 9, 2023, 4:07 p.m. UTC 
This is a preliminary patch to sync load_balancers applied to
logical_routers to Load_Balancer table in the SouthBound database.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2193323
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 controller/lflow.c          |  7 ++++---
 controller/local_data.c     |  2 +-
 controller/ovn-controller.c |  6 +++---
 northd/northd.c             |  6 +++---
 ovn-sb.ovsschema            |  4 ++--
 ovn-sb.xml                  |  7 ++++---
 tests/ovn-northd.at         | 14 +++++++-------
 utilities/ovn-sbctl.c       | 15 ++++++++-------
 8 files changed, 32 insertions(+), 29 deletions(-)

Comments

Ilya Maximets June 9, 2023, 4:12 p.m. UTC | #1 
On 6/9/23 18:07, Lorenzo Bianconi wrote:
> This is a preliminary patch to sync load_balancers applied to
> logical_routers to Load_Balancer table in the SouthBound database.
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2193323

The patch doesn't seem have anything in common with the BZ.

But anyway, the change propesed here is backward incompatible
and will break upgrades.  Column rename is equivalent to
a column removal and addition of a new one.  Existing clients
will not be able to handle that.  Basically, columns can never
be removed from a schema.

Best regards, Ilya Maximets.
Lorenzo Bianconi June 9, 2023, 4:38 p.m. UTC | #2 
> On 6/9/23 18:07, Lorenzo Bianconi wrote:
> > This is a preliminary patch to sync load_balancers applied to
> > logical_routers to Load_Balancer table in the SouthBound database.
> > 
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2193323
> 
> The patch doesn't seem have anything in common with the BZ.

The issue reported in the bz is due to missing sync of lb applied to logical
router in the load_balancer table in the sb db.

> 
> But anyway, the change propesed here is backward incompatible
> and will break upgrades.  Column rename is equivalent to
> a column removal and addition of a new one.  Existing clients
> will not be able to handle that.  Basically, columns can never
> be removed from a schema.

ops, right. I will fix it.

Regards,
Lorenzo

> 
> Best regards, Ilya Maximets.
>
Ilya Maximets June 9, 2023, 5:52 p.m. UTC | #3 
On 6/9/23 18:38, Lorenzo Bianconi wrote:
>> On 6/9/23 18:07, Lorenzo Bianconi wrote:
>>> This is a preliminary patch to sync load_balancers applied to
>>> logical_routers to Load_Balancer table in the SouthBound database.
>>>
>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2193323
>>
>> The patch doesn't seem have anything in common with the BZ.
> 
> The issue reported in the bz is due to missing sync of lb applied to logical
> router in the load_balancer table in the sb db.

True, but this particular patch doesn't fix that issue.
And the change implemented in this patch is not mentioned
in the linked bugzilla issue.  So, the reported-at tag is
not really appropriate here.

> 
>>
>> But anyway, the change propesed here is backward incompatible
>> and will break upgrades.  Column rename is equivalent to
>> a column removal and addition of a new one.  Existing clients
>> will not be able to handle that.  Basically, columns can never
>> be removed from a schema.
> 
> ops, right. I will fix it.
> 
> Regards,
> Lorenzo
> 
>>
>> Best regards, Ilya Maximets.
>>
Lorenzo Bianconi June 10, 2023, 10:26 a.m. UTC | #4 
> On 6/9/23 18:38, Lorenzo Bianconi wrote:
> >> On 6/9/23 18:07, Lorenzo Bianconi wrote:
> >>> This is a preliminary patch to sync load_balancers applied to
> >>> logical_routers to Load_Balancer table in the SouthBound database.
> >>>
> >>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2193323
> >>
> >> The patch doesn't seem have anything in common with the BZ.
> > 
> > The issue reported in the bz is due to missing sync of lb applied to logical
> > router in the load_balancer table in the sb db.
> 
> True, but this particular patch doesn't fix that issue.
> And the change implemented in this patch is not mentioned
> in the linked bugzilla issue.  So, the reported-at tag is
> not really appropriate here.

ack, I will drop it.

Regards,
Lorenzo

> 
> > 
> >>
> >> But anyway, the change propesed here is backward incompatible
> >> and will break upgrades.  Column rename is equivalent to
> >> a column removal and addition of a new one.  Existing clients
> >> will not be able to handle that.  Basically, columns can never
> >> be removed from a schema.
> > 
> > ops, right. I will fix it.
> > 
> > Regards,
> > Lorenzo
> > 
> >>
> >> Best regards, Ilya Maximets.
> >>
>
diff mbox series

Patch

diff --git a/controller/lflow.c b/controller/lflow.c
index 22faaf013..9a4e3006b 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -1870,11 +1870,12 @@  add_lb_ct_snat_hairpin_vip_flow(const struct ovn_controller_lb *lb,
                                           local_datapaths, &match,
                                           &ofpacts, flow_table);
         }
-        if (lb->slb->datapath_group) {
-            for (size_t i = 0; i < lb->slb->datapath_group->n_datapaths; i++) {
+        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->datapath_group->datapaths[i],
+                    lb->slb->ls_datapath_group->datapaths[i],
                     local_datapaths, &match, &ofpacts, flow_table);
             }
         }
diff --git a/controller/local_data.c b/controller/local_data.c
index cf0b21bb1..2950434ac 100644
--- a/controller/local_data.c
+++ b/controller/local_data.c
@@ -661,7 +661,7 @@  lb_is_local(const struct sbrec_load_balancer *sbrec_lb,
         }
     }
 
-    struct sbrec_logical_dp_group *dp_group = sbrec_lb->datapath_group;
+    struct sbrec_logical_dp_group *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,
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 3a81a13fb..ab4896b91 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -2699,10 +2699,10 @@  load_balancers_by_dp_init(const struct hmap *local_datapaths,
             load_balancers_by_dp_add_one(local_datapaths,
                                          lb->datapaths[i], lb, lbs);
         }
-        for (size_t i = 0; lb->datapath_group
-                           && i < lb->datapath_group->n_datapaths; i++) {
+        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->datapath_group->datapaths[i],
+                                         lb->ls_datapath_group->datapaths[i],
                                          lb, lbs);
         }
     }
diff --git a/northd/northd.c b/northd/northd.c
index 93f126aa3..fad8ab0ec 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -4449,7 +4449,7 @@  sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
 
         /* Find or create datapath group for this load balancer. */
         lb->dpg = ovn_dp_group_get_or_create(ovnsb_txn, &dp_groups,
-                                             lb->slb->datapath_group,
+                                             lb->slb->ls_datapath_group,
                                              lb->n_nb_ls, lb->nb_ls_map,
                                              bitmap_len, true,
                                              ls_datapaths, NULL);
@@ -4485,7 +4485,7 @@  sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
         /* Find or create datapath group for this load balancer. */
         if (!lb->dpg) {
             lb->dpg = ovn_dp_group_get_or_create(ovnsb_txn, &dp_groups,
-                                                 lb->slb->datapath_group,
+                                                 lb->slb->ls_datapath_group,
                                                  lb->n_nb_ls, lb->nb_ls_map,
                                                  bitmap_len, true,
                                                  ls_datapaths, NULL);
@@ -4495,7 +4495,7 @@  sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
         sbrec_load_balancer_set_name(lb->slb, lb->nlb->name);
         sbrec_load_balancer_set_vips(lb->slb, ovn_northd_lb_get_vips(lb));
         sbrec_load_balancer_set_protocol(lb->slb, lb->nlb->protocol);
-        sbrec_load_balancer_set_datapath_group(lb->slb, lb->dpg->dp_group);
+        sbrec_load_balancer_set_ls_datapath_group(lb->slb, lb->dpg->dp_group);
         sbrec_load_balancer_set_options(lb->slb, &options);
         /* Clearing 'datapaths' column, since 'dp_group' is in use. */
         sbrec_load_balancer_set_datapaths(lb->slb, NULL, 0);
diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
index f59af8cc5..e02fbd884 100644
--- a/ovn-sb.ovsschema
+++ b/ovn-sb.ovsschema
@@ -1,7 +1,7 @@ 
 {
     "name": "OVN_Southbound",
     "version": "20.27.2",
-    "cksum": "1291808617 30462",
+    "cksum": "2970847454 30465",
     "tables": {
         "SB_Global": {
             "columns": {
@@ -530,7 +530,7 @@ 
                     "type": {"key": {"type": "uuid",
                                      "refTable": "Datapath_Binding"},
                              "min": 0, "max": "unlimited"}},
-                "datapath_group":
+                "ls_datapath_group":
                     {"type": {"key": {"type": "uuid",
                                       "refTable": "Logical_DP_Group"},
                               "min": 0, "max": 1}},
diff --git a/ovn-sb.xml b/ovn-sb.xml
index 0988cb1f8..5d11d059b 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -4879,9 +4879,10 @@  tcp.flags = RST;
       Datapaths to which this load balancer applies to.
     </column>
 
-    <column name="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 name="ls_datapath_group">
+      The group of logical switches 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">
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 6736429ae..63caf8d66 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -2774,7 +2774,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
@@ -2785,7 +2785,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
@@ -2873,7 +2873,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."
@@ -2884,7 +2884,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."
@@ -5932,9 +5932,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
@@ -8603,7 +8603,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)
diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
index 3948cae3f..ddd9a9ca9 100644
--- a/utilities/ovn-sbctl.c
+++ b/utilities/ovn-sbctl.c
@@ -396,7 +396,7 @@  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);
-    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,9 +932,9 @@  cmd_lflow_list_load_balancers(struct ctl_context *ctx, struct vconn *vconn,
                     break;
                 }
             }
-            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,9 +954,10 @@  cmd_lflow_list_load_balancers(struct ctl_context *ctx, struct vconn *vconn,
                 print_vflow_datapath_name(lb->datapaths[i], true,
                                           &ctx->output);
             }
-            for (size_t i = 0; lb->datapath_group
-                               && i < lb->datapath_group->n_datapaths; i++) {
-                print_vflow_datapath_name(lb->datapath_group->datapaths[i],
+            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);
             }
         }