Hi Lorenzo, I have one note below

On 6/9/23 12:07, Lorenzo Bianconi wrote:
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.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2193323
Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com>
---
  controller/local_data.c     |   8 +++
  controller/ovn-controller.c |   6 ++
  lib/lb.h                    |   3 +-
  northd/northd.c             |  78 ++++++++++++++++++------
  ovn-sb.ovsschema            |   6 +-
  ovn-sb.xml                  |   6 ++
  tests/ovn-northd.at         |  17 +++++-
  tests/system-ovn.at         | 117 ++++++++++++++++++++++++++++++++++++
  utilities/ovn-sbctl.c       |   1 +
  9 files changed, 219 insertions(+), 23 deletions(-)

diff --git a/controller/local_data.c b/controller/local_data.c
index 2950434ac..c8aebcf50 100644
--- 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
index ab4896b91..93eaf6d9a 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -2705,6 +2705,12 @@ load_balancers_by_dp_init(const struct hmap 
*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,
+                                         lb->lr_datapath_group->datapaths[i],
+                                         lb, lbs);
+        }
      }
      return lbs;
  }
diff --git a/lib/lb.h b/lib/lb.h
index 23d8fc9e9..2456423cc 100644
--- a/lib/lb.h
+++ b/lib/lb.h
@@ -85,7 +85,8 @@ struct ovn_northd_lb {
      size_t n_nb_lr;
      unsigned long *nb_lr_map;
- struct ovn_dp_group *dpg;
+    struct ovn_dp_group *ls_dpg;
+    struct ovn_dp_group *lr_dpg;
  };
struct ovn_lb_vip {
diff --git a/northd/northd.c b/northd/northd.c
index fad8ab0ec..9341823e3 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -4412,10 +4412,11 @@ ovn_dp_group_get_or_create(struct ovsdb_idl_txn 
*ovnsb_txn,
  static 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 *lbs)
+         struct ovn_datapaths *ls_datapaths,
+         struct ovn_datapaths *lr_datapaths, struct hmap *lbs)
  {
-    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_northd_lb *lb;
/* Delete any stale SB load balancer rows and create datapath
@@ -4440,7 +4441,8 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
           * are not indexed in any way.
           */
          lb = ovn_northd_lb_find(lbs, &lb_uuid);
-        if (!lb || !lb->n_nb_ls || !hmapx_add(&existing_lbs, lb)) {
+        if (!lb || (!lb->n_nb_ls && !lb->n_nb_lr) ||
+            !hmapx_add(&existing_lbs, lb)) {
              sbrec_load_balancer_delete(sbrec_lb);
              continue;
          }
@@ -4448,11 +4450,20 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
          lb->slb = sbrec_lb;
/* Find or create datapath group for this load balancer. */
-        lb->dpg = ovn_dp_group_get_or_create(ovnsb_txn, &dp_groups,
-                                             lb->slb->ls_datapath_group,
-                                             lb->n_nb_ls, lb->nb_ls_map,
-                                             bitmap_len, true,
-                                             ls_datapaths, NULL);
+        if (lb->n_nb_ls) {
+            lb->ls_dpg = ovn_dp_group_get_or_create(ovnsb_txn, &ls_dp_groups,
+                                                    lb->slb->ls_datapath_group,
+                                                    lb->n_nb_ls, lb->nb_ls_map,
+                                                    ods_size(ls_datapaths),
+                                                    true, ls_datapaths, NULL);
+        }
+        if (lb->n_nb_lr) {
+            lb->lr_dpg = ovn_dp_group_get_or_create(ovnsb_txn, &lr_dp_groups,
+                                                    lb->slb->lr_datapath_group,
+                                                    lb->n_nb_lr, lb->nb_lr_map,
+                                                    ods_size(lr_datapaths),
+                                                    false, NULL, lr_datapaths);
+        }
      }
      hmapx_destroy(&existing_lbs);
@@ -4460,7 +4471,7 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
       * the SB load balancer columns. */
      HMAP_FOR_EACH (lb, hmap_node, lbs) {
- if (!lb->n_nb_ls) {
+        if (!lb->n_nb_ls && !lb->n_nb_lr) {
              continue;
          }
@@ -4483,19 +4494,33 @@ 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->ls_datapath_group,
-                                                 lb->n_nb_ls, lb->nb_ls_map,
-                                                 bitmap_len, true,
-                                                 ls_datapaths, NULL);
+        if (!lb->ls_dpg && lb->n_nb_ls) {
+            lb->ls_dpg = ovn_dp_group_get_or_create(ovnsb_txn, &ls_dp_groups,
+                                                    lb->slb->ls_datapath_group,
+                                                    lb->n_nb_ls, lb->nb_ls_map,
+                                                    ods_size(ls_datapaths),
+                                                    true, ls_datapaths, NULL);
+        }
+        if (!lb->lr_dpg && lb->n_nb_lr) {
+            lb->lr_dpg = ovn_dp_group_get_or_create(ovnsb_txn, &lr_dp_groups,
+                                                    lb->slb->lr_datapath_group,
+                                                    lb->n_nb_lr, lb->nb_lr_map,
+                                                    ods_size(lr_datapaths),
+                                                    false, NULL, lr_datapaths);
          }
/* Update columns. */
          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_ls_datapath_group(lb->slb, lb->dpg->dp_group);
+        if (lb->ls_dpg) {
+            sbrec_load_balancer_set_ls_datapath_group(lb->slb,
+                                                      lb->ls_dpg->dp_group);
+        }
+        if (lb->lr_dpg) {
+            sbrec_load_balancer_set_lr_datapath_group(lb->slb,
+                                                      lb->lr_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);
@@ -4503,11 +4528,17 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
      }
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);
/* Datapath_Binding.load_balancers is not used anymore, it's still in the
       * schema for compatibility reasons.  Reset it to empty, just in case.
@@ -4516,6 +4547,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);
          }
@@ -16683,7 +16721,7 @@ ovnnb_db_run(struct northd_input *input_data,
      ovn_update_ipv6_prefix(&data->lr_ports);
sync_lbs(ovnsb_txn, input_data->sbrec_load_balancer_table,
-             &data->ls_datapaths, &data->lbs);
+             &data->ls_datapaths, &data->lr_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,
diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
index e02fbd884..2a81d54ac 100644
--- a/ovn-sb.ovsschema
+++ b/ovn-sb.ovsschema
@@ -1,7 +1,7 @@
  {
      "name": "OVN_Southbound",
      "version": "20.27.2",
-    "cksum": "2970847454 30465",
+    "cksum": "1977000278 30679",
      "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
index 5d11d059b..de92a63f7 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -4885,6 +4885,12 @@ tcp.flags = RST;
        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
index 63caf8d66..0a7725c37 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -2811,6 +2811,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."
@@ -2856,7 +2865,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."
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 6669c18e7..08b380d10 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -11546,3 +11546,120 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port 
patch-.*/d
AT_CLEANUP
  ])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ct_flush on logical router load balancer])
+AT_KEYWORDS([ct-lr-flush])
+CHECK_CONNTRACK()
+CHECK_CONNTRACK_NAT()
+ovn_start
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_BR([br-int])
+#
+# Set external-ids in br-int needed for ovn-controller
+ovs-vsctl \
+        -- set Open_vSwitch . external-ids:system-id=hv1 \
+        -- set Open_vSwitch . 
external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
+        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
+        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
+        -- set bridge br-int fail-mode=secure other-config:disable-in-band=true
+
+start_daemon ovn-controller
+
+check ovn-nbctl lr-add R1
+
+check ovn-nbctl ls-add sw0
+check ovn-nbctl ls-add public
+
+check ovn-nbctl lrp-add R1 rp-sw0 00:00:01:01:02:03 192.168.1.1/24
+check ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03 172.16.1.1/24
+
+check ovn-nbctl set logical_router R1 options:chassis=hv1
+
+check ovn-nbctl lsp-add sw0 sw0-rp -- set Logical_Switch_Port sw0-rp \
+    type=router options:router-port=rp-sw0 \
+    -- lsp-set-addresses sw0-rp router
+
+check ovn-nbctl lsp-add sw0 sw0-vm \
+    -- lsp-set-addresses sw0-vm "00:00:01:01:02:04 192.168.1.2/24"
+
+check ovn-nbctl lsp-add public public-rp -- set Logical_Switch_Port public-rp \
+    type=router options:router-port=rp-public \
+    -- lsp-set-addresses public-rp router
+
+check ovn-nbctl lsp-add public public-vm \
+   -- lsp-set-addresses public-vm "00:00:02:01:02:04 172.16.1.2/24"
+
+ADD_NAMESPACES(sw0-vm)
+ADD_VETH(sw0-vm, sw0-vm, br-int, "192.168.1.2/24", "00:00:01:01:02:04", \
+         "192.168.1.1")
+OVS_WAIT_UNTIL([test "$(ip netns exec sw0-vm ip a | grep fe80 | grep tentative)" = 
""])
+
+ADD_NAMESPACES(public-vm)
+ADD_VETH(public-vm, public-vm, br-int, "172.16.1.2/24", "00:00:02:01:02:04", \
+         "172.16.1.1")
+
+OVS_WAIT_UNTIL([test "$(ip netns exec public-vm ip a | grep fe80 | grep tentative)" = 
""])
+
+# Start webservers in 'server'.
+OVS_START_L7([sw0-vm], [http])
+
+# Create a load balancer and associate to R1
+check ovn-nbctl lb-add lb1 172.16.1.150:80 192.168.1.2:80 \
+    -- set load_balancer lb1 options:ct_flush="true"
+check ovn-nbctl lr-lb-add R1 lb1
+
+check ovn-nbctl --wait=hv sync
+
+for i in $(seq 1 5); do
+    echo Request $i
+    NS_CHECK_EXEC([public-vm], [wget 172.16.1.150 -t 5 -T 1 
--retry-connrefused -v -o wget$i.log])
+done
+
+OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.150) 
| wc -l ], [0], [dnl
+1
+])
+
+check ovn-nbctl lb-del lb1
+
+OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.150) 
| wc -l ], [0], [dnl
+0
+])
+
+check ovn-nbctl lb-add lb2 172.16.1.151:80 192.168.1.2:80
+check ovn-nbctl lr-lb-add R1 lb2
+
+check ovn-nbctl --wait=hv sync
+
+for i in $(seq 1 5); do
+    echo Request $i
+    NS_CHECK_EXEC([public-vm], [wget 172.16.1.151 -t 5 -T 1 
--retry-connrefused -v -o wget$i.log])
+done
+
+OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.151) 
| wc -l ], [0], [dnl
+1
+])
+
+check ovn-nbctl lb-del lb2
+
+OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.151) 
| wc -l ], [0], [dnl
+1
+])

The version of this test on the linked BZ expects 0 as the output when dumping conntrack at this point. Deleting the load balancer should flush the conntrack entry, so there should now be no conntrack entries for the load balancer VIP.

Is there a reason why this was changed to 1 instead? The problem is that the condition being checked before and after deleting the load balancer is the same. It's not clear what is being tested here.

I think one of two changes needs to happen with this test:

1) If the test is not actually exercising the expected conntrack-flushing behavior, then the test needs to be adjusted to properly test the conntrack flush.

2) If the test is correct, and we actually do expect a conntrack entry to still exist, then rather than counting the number of conntrack entries, we need to inspect the conntrack entry to ensure that something has actually happened as a result of deleting the load balancer.

+
+OVS_APP_EXIT_AND_WAIT([ovn-controller])
+
+as ovn-sb
+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+
+as ovn-nb
+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+
+as northd
+OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE])
+
+as
+OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
+/Failed to acquire.*/d
+/connection dropped.*/d"])
+AT_CLEANUP
+])
diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
index ddd9a9ca9..0e3afaea7 100644
--- a/utilities/ovn-sbctl.c
+++ b/utilities/ovn-sbctl.c
@@ -397,6 +397,7 @@ pre_get_info(struct ctl_context *ctx)
ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_datapaths);
      ovsdb_idl_add_column(ctx->idl, 
&sbrec_load_balancer_col_ls_datapath_group);
+    ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_lr_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);

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to