diff mbox series

[ovs-dev,v8,2/4] northd: Handle load balancer group changes for a logical switch.

Message ID 20230912174707.258687-1-numans@ovn.org
State Accepted
Headers show
Series northd: I-P for load balancer and lb groups | 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

Numan Siddique Sept. 12, 2023, 5:47 p.m. UTC 
From: Numan Siddique <numans@ovn.org>

For a given load balancer group 'A', northd engine data maintains
a bitmap of datapaths associated to this lb group.  So when lb group 'A'
gets associated to a logical switch 's1', the bitmap index of 's1' is set
in its bitmap.

In order to handle the load balancer group changes incrementally for a
logical switch, we need to set and clear the bitmap bits accordingly.
And this patch does it.

Reviewed-by: Ales Musil <amusil@redhat.com>
Acked-by: Mark Michelson <mmichels@redhat.com>
Signed-off-by: Numan Siddique <numans@ovn.org>
---
 northd/en-lb-data.c | 102 ++++++++++++++++++++++++++++++++------------
 northd/en-lb-data.h |   4 ++
 northd/northd.c     |  63 +++++++++++++++++++++++----
 tests/ovn-northd.at |  30 +++++++++----
 4 files changed, 155 insertions(+), 44 deletions(-)
diff mbox series

Patch

diff --git a/northd/en-lb-data.c b/northd/en-lb-data.c
index d854042feb..fd09e719d8 100644
--- a/northd/en-lb-data.c
+++ b/northd/en-lb-data.c
@@ -63,6 +63,7 @@  static struct crupdated_lbgrp *
 static void add_deleted_lbgrp_to_tracked_data(
     struct ovn_lb_group *, struct tracked_lb_data *);
 static bool is_ls_lbs_changed(const struct nbrec_logical_switch *nbs);
+static bool is_ls_lbgrps_changed(const struct nbrec_logical_switch *nbs);
 
 /* 'lb_data' engine node manages the NB load balancers and load balancer
  * groups.  For each NB LB, it creates 'struct ovn_northd_lb' and
@@ -264,12 +265,15 @@  lb_data_logical_switch_handler(struct engine_node *node, void *data)
                 hmapx_add(&trk_lb_data->deleted_od_lb_data, od_lb_data);
             }
         } else {
-            if (!is_ls_lbs_changed(nbs)) {
+            bool ls_lbs_changed = is_ls_lbs_changed(nbs);
+            bool ls_lbgrps_changed = is_ls_lbgrps_changed(nbs);
+            if (!ls_lbs_changed && !ls_lbgrps_changed) {
                 continue;
             }
             struct crupdated_od_lb_data *codlb = xzalloc(sizeof *codlb);
             codlb->od_uuid = nbs->header_.uuid;
             uuidset_init(&codlb->assoc_lbs);
+            uuidset_init(&codlb->assoc_lbgrps);
 
             struct od_lb_data *od_lb_data =
                 find_od_lb_data(&lb_data->ls_lb_map, &nbs->header_.uuid);
@@ -278,38 +282,66 @@  lb_data_logical_switch_handler(struct engine_node *node, void *data)
                                                 &nbs->header_.uuid);
             }
 
-            struct uuidset *pre_lb_uuids = od_lb_data->lbs;
-            od_lb_data->lbs = xzalloc(sizeof *od_lb_data->lbs);
-            uuidset_init(od_lb_data->lbs);
-
-            for (size_t i = 0; i < nbs->n_load_balancer; i++) {
-                const struct uuid *lb_uuid =
-                    &nbs->load_balancer[i]->header_.uuid;
-                uuidset_insert(od_lb_data->lbs, lb_uuid);
+            if (ls_lbs_changed) {
+                struct uuidset *pre_lb_uuids = od_lb_data->lbs;
+                od_lb_data->lbs = xzalloc(sizeof *od_lb_data->lbs);
+                uuidset_init(od_lb_data->lbs);
+
+                for (size_t i = 0; i < nbs->n_load_balancer; i++) {
+                    const struct uuid *lb_uuid =
+                        &nbs->load_balancer[i]->header_.uuid;
+                    uuidset_insert(od_lb_data->lbs, lb_uuid);
+
+                    struct uuidset_node *unode = uuidset_find(pre_lb_uuids,
+                                                            lb_uuid);
+
+                    if (!unode || (nbrec_load_balancer_row_get_seqno(
+                            nbs->load_balancer[i],
+                            OVSDB_IDL_CHANGE_MODIFY) > 0)) {
+                        /* Add this lb to the tracked data. */
+                        uuidset_insert(&codlb->assoc_lbs, lb_uuid);
+                        changed = true;
+                    }
+
+                    if (unode) {
+                        uuidset_delete(pre_lb_uuids, unode);
+                    }
+                }
+                if (!uuidset_is_empty(pre_lb_uuids)) {
+                    trk_lb_data->has_dissassoc_lbs_from_od = true;
+                    changed = true;
+                }
 
-                struct uuidset_node *unode = uuidset_find(pre_lb_uuids,
-                                                          lb_uuid);
+                uuidset_destroy(pre_lb_uuids);
+                free(pre_lb_uuids);
+            }
 
-                if (!unode || (nbrec_load_balancer_row_get_seqno(
-                        nbs->load_balancer[i], OVSDB_IDL_CHANGE_MODIFY) > 0)) {
-                    /* Add this lb to the tracked data. */
-                    uuidset_insert(&codlb->assoc_lbs, lb_uuid);
-                    changed = true;
+            if (ls_lbgrps_changed) {
+                struct uuidset *pre_lbgrp_uuids = od_lb_data->lbgrps;
+                od_lb_data->lbgrps = xzalloc(sizeof *od_lb_data->lbgrps);
+                uuidset_init(od_lb_data->lbgrps);
+                for (size_t i = 0; i < nbs->n_load_balancer_group; i++) {
+                    const struct uuid *lbg_uuid =
+                        &nbs->load_balancer_group[i]->header_.uuid;
+                    uuidset_insert(od_lb_data->lbgrps, lbg_uuid);
+
+                    if (!uuidset_find_and_delete(pre_lbgrp_uuids,
+                                                 lbg_uuid)) {
+                        /* Add this lb group to the tracked data. */
+                        uuidset_insert(&codlb->assoc_lbgrps, lbg_uuid);
+                        changed = true;
+                    }
                 }
 
-                if (unode) {
-                    uuidset_delete(pre_lb_uuids, unode);
+                if (!uuidset_is_empty(pre_lbgrp_uuids)) {
+                    trk_lb_data->has_dissassoc_lbgrps_from_od = true;
+                    changed = true;
                 }
-            }
 
-            if (!uuidset_is_empty(pre_lb_uuids)) {
-                trk_lb_data->has_dissassoc_lbs_from_od = true;
-                changed = true;
+                uuidset_destroy(pre_lbgrp_uuids);
+                free(pre_lbgrp_uuids);
             }
 
-            uuidset_destroy(pre_lb_uuids);
-            free(pre_lb_uuids);
-
             ovs_list_insert(&trk_lb_data->crupdated_ls_lbs, &codlb->list_node);
         }
     }
@@ -407,7 +439,7 @@  build_od_lb_map(const struct nbrec_logical_switch_table *nbrec_ls_table,
 {
     const struct nbrec_logical_switch *nbrec_ls;
     NBREC_LOGICAL_SWITCH_TABLE_FOR_EACH (nbrec_ls, nbrec_ls_table) {
-        if (!nbrec_ls->n_load_balancer) {
+        if (!nbrec_ls->n_load_balancer && !nbrec_ls->n_load_balancer_group) {
             continue;
         }
 
@@ -417,6 +449,10 @@  build_od_lb_map(const struct nbrec_logical_switch_table *nbrec_ls_table,
             uuidset_insert(od_lb_data->lbs,
                            &nbrec_ls->load_balancer[i]->header_.uuid);
         }
+        for (size_t i = 0; i < nbrec_ls->n_load_balancer_group; i++) {
+            uuidset_insert(od_lb_data->lbgrps,
+                           &nbrec_ls->load_balancer_group[i]->header_.uuid);
+        }
     }
 }
 
@@ -426,7 +462,9 @@  create_od_lb_data(struct hmap *od_lb_map, const struct uuid *od_uuid)
     struct od_lb_data *od_lb_data = xzalloc(sizeof *od_lb_data);
     od_lb_data->od_uuid = *od_uuid;
     od_lb_data->lbs = xzalloc(sizeof *od_lb_data->lbs);
+    od_lb_data->lbgrps = xzalloc(sizeof *od_lb_data->lbgrps);
     uuidset_init(od_lb_data->lbs);
+    uuidset_init(od_lb_data->lbgrps);
 
     hmap_insert(od_lb_map, &od_lb_data->hmap_node,
                 uuid_hash(&od_lb_data->od_uuid));
@@ -451,7 +489,9 @@  static void
 destroy_od_lb_data(struct od_lb_data *od_lb_data)
 {
     uuidset_destroy(od_lb_data->lbs);
+    uuidset_destroy(od_lb_data->lbgrps);
     free(od_lb_data->lbs);
+    free(od_lb_data->lbgrps);
     free(od_lb_data);
 }
 
@@ -462,6 +502,7 @@  destroy_tracked_data(struct ed_type_lb_data *lb_data)
     lb_data->tracked_lb_data.has_health_checks = false;
     lb_data->tracked_lb_data.has_dissassoc_lbs_from_lbgrps = false;
     lb_data->tracked_lb_data.has_dissassoc_lbs_from_od = false;
+    lb_data->tracked_lb_data.has_dissassoc_lbgrps_from_od = false;
 
     struct hmapx_node *node;
     HMAPX_FOR_EACH_SAFE (node, &lb_data->tracked_lb_data.deleted_lbs) {
@@ -492,7 +533,7 @@  destroy_tracked_data(struct ed_type_lb_data *lb_data)
                         &lb_data->tracked_lb_data.crupdated_ls_lbs) {
         ovs_list_remove(&codlb->list_node);
         uuidset_destroy(&codlb->assoc_lbs);
-
+        uuidset_destroy(&codlb->assoc_lbgrps);
         free(codlb);
     }
 
@@ -552,3 +593,10 @@  is_ls_lbs_changed(const struct nbrec_logical_switch *nbs) {
             ||  nbrec_logical_switch_is_updated(nbs,
                         NBREC_LOGICAL_SWITCH_COL_LOAD_BALANCER));
 }
+
+static bool
+is_ls_lbgrps_changed(const struct nbrec_logical_switch *nbs) {
+    return ((nbrec_logical_switch_is_new(nbs) && nbs->n_load_balancer_group)
+            ||  nbrec_logical_switch_is_updated(nbs,
+                        NBREC_LOGICAL_SWITCH_COL_LOAD_BALANCER_GROUP));
+}
diff --git a/northd/en-lb-data.h b/northd/en-lb-data.h
index 6b2ba11cb8..ebfd547d77 100644
--- a/northd/en-lb-data.h
+++ b/northd/en-lb-data.h
@@ -33,6 +33,7 @@  struct crupdated_od_lb_data {
 
     struct uuid od_uuid;
     struct uuidset assoc_lbs;
+    struct uuidset assoc_lbgrps;
 };
 
 struct tracked_lb_data {
@@ -66,6 +67,9 @@  struct tracked_lb_data {
 
     /* Indicates if a lb was disassociated from a logical switch. */
     bool has_dissassoc_lbs_from_od;
+
+    /* Indicates if a lb group was disassociated from a logical switch. */
+    bool has_dissassoc_lbgrps_from_od;
 };
 
 /* Datapath (logical switch) to lb/lbgrp association data. */
diff --git a/northd/northd.c b/northd/northd.c
index 492d9b4f88..d500f940b1 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -5075,6 +5075,7 @@  ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct hmap *ls_ports,
  * Presently supports i-p for the below changes:
  *    - logical switch ports.
  *    - load balancers.
+ *    - load balancer groups.
  */
 static bool
 ls_changes_can_be_handled(
@@ -5086,7 +5087,8 @@  ls_changes_can_be_handled(
         if (nbrec_logical_switch_is_updated(ls, col)) {
             if (col == NBREC_LOGICAL_SWITCH_COL_ACLS ||
                 col == NBREC_LOGICAL_SWITCH_COL_PORTS ||
-                col == NBREC_LOGICAL_SWITCH_COL_LOAD_BALANCER) {
+                col == NBREC_LOGICAL_SWITCH_COL_LOAD_BALANCER ||
+                col == NBREC_LOGICAL_SWITCH_COL_LOAD_BALANCER_GROUP) {
                 continue;
             }
             return false;
@@ -5111,12 +5113,6 @@  ls_changes_can_be_handled(
             return false;
         }
     }
-    for (size_t i = 0; i < ls->n_load_balancer_group; i++) {
-        if (nbrec_load_balancer_group_row_get_seqno(ls->load_balancer_group[i],
-                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
-            return false;
-        }
-    }
     for (size_t i = 0; i < ls->n_qos_rules; i++) {
         if (nbrec_qos_row_get_seqno(ls->qos_rules[i],
                                 OVSDB_IDL_CHANGE_MODIFY) > 0) {
@@ -5304,7 +5300,11 @@  fail:
 /* Return true if changes are handled incrementally, false otherwise.
  * When there are any changes, try to track what's exactly changed and set
  * northd_data->change_tracked accordingly: change tracked - true, otherwise,
- * false. */
+ * false.
+ *
+ * Note: Changes to load balancer and load balancer groups associated with
+ * the logical switches are handled separately in the lb_data change handlers.
+ * */
 bool
 northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn,
                          const struct northd_input *ni,
@@ -5479,6 +5479,12 @@  northd_handle_lb_data_changes(struct tracked_lb_data *trk_lb_data,
         return false;
     }
 
+    /* Fall back to recompute if any load balancer group has been disassociated
+     * from a logical switch or router. */
+    if (trk_lb_data->has_dissassoc_lbgrps_from_od) {
+        return false;
+    }
+
     struct ovn_lb_datapaths *lb_dps;
     struct ovn_northd_lb *lb;
     struct ovn_datapath *od;
@@ -5549,6 +5555,22 @@  northd_handle_lb_data_changes(struct tracked_lb_data *trk_lb_data,
             ovn_lb_datapaths_add_ls(lb_dps, 1, &od);
         }
 
+        UUIDSET_FOR_EACH (uuidnode, &codlb->assoc_lbgrps) {
+            lbgrp_dps = ovn_lb_group_datapaths_find(lbgrp_datapaths_map,
+                                                    &uuidnode->uuid);
+            ovs_assert(lbgrp_dps);
+            ovn_lb_group_datapaths_add_ls(lbgrp_dps, 1, &od);
+
+            /* Associate all the lbs of the lbgrp to the datapath 'od' */
+            for (size_t j = 0; j < lbgrp_dps->lb_group->n_lbs; j++) {
+                const struct uuid *lb_uuid
+                    = &lbgrp_dps->lb_group->lbs[j]->nlb->header_.uuid;
+                lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
+                ovs_assert(lb_dps);
+                ovn_lb_datapaths_add_ls(lb_dps, 1, &od);
+            }
+        }
+
         /* Re-evaluate 'od->has_lb_vip' */
         init_lb_for_datapath(od);
     }
@@ -5568,6 +5590,31 @@  northd_handle_lb_data_changes(struct tracked_lb_data *trk_lb_data,
         }
     }
 
+    HMAP_FOR_EACH (crupdated_lbgrp, hmap_node,
+                   &trk_lb_data->crupdated_lbgrps) {
+        lbgrp = crupdated_lbgrp->lbgrp;
+        const struct uuid *lb_uuid = &lbgrp->uuid;
+
+        lbgrp_dps = ovn_lb_group_datapaths_find(lbgrp_datapaths_map,
+                                                lb_uuid);
+        ovs_assert(lbgrp_dps);
+
+        struct hmapx_node *hnode;
+        HMAPX_FOR_EACH (hnode, &crupdated_lbgrp->assoc_lbs) {
+            lb = hnode->data;
+            lb_uuid = &lb->nlb->header_.uuid;
+            lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
+            ovs_assert(lb_dps);
+            for (size_t i = 0; i < lbgrp_dps->n_ls; i++) {
+                od = lbgrp_dps->ls[i];
+                ovn_lb_datapaths_add_ls(lb_dps, 1, &od);
+
+                /* Re-evaluate 'od->has_lb_vip' */
+                init_lb_for_datapath(od);
+            }
+        }
+    }
+
     return true;
 }
 
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index f0f8d1f503..34e10663d0 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -2191,6 +2191,18 @@  check ovn-nbctl --wait=sb sync
 AT_CHECK([ovn-sbctl lflow-list | grep "ls_out_pre_lb.*priority=100" | grep reg0 | sort], [0], [dnl
 ])
 
+# Now associate vip again to lb4 and then delete it.
+check ovn-nbctl set load_balancer $lb4 vips:"10.0.0.13"="10.0.0.6"
+check ovn-nbctl --wait=sb sync
+AT_CHECK([ovn-sbctl lflow-list | grep "ls_out_pre_lb.*priority=100" | grep reg0 | sort], [0], [dnl
+  table=1 (ls_out_pre_lb      ), priority=100  , match=(ip), action=(reg0[[2]] = 1; next;)
+])
+
+check ovn-nbctl lb-del $lb4
+check ovn-nbctl --wait=sb sync
+AT_CHECK([ovn-sbctl lflow-list | grep "ls_out_pre_lb.*priority=100" | grep reg0 | sort], [0], [dnl
+])
+
 AT_CLEANUP
 ])
 
@@ -10553,21 +10565,21 @@  CHECK_NO_CHANGE_AFTER_RECOMPUTE
 check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
 check ovn-nbctl add logical_switch sw0 load_balancer_group $lbg1_uuid
 check_engine_stats lb_data norecompute compute
-check_engine_stats northd recompute nocompute
+check_engine_stats northd norecompute compute
 check_engine_stats lflow recompute nocompute
 
-# Update lb and this should result in recompute
+# Update lb and this should not result in northd recompute
 check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
 check ovn-nbctl --wait=sb set load_balancer . options:bar=foo
 check_engine_stats lb_data norecompute compute
-check_engine_stats northd recompute nocompute
+check_engine_stats northd norecompute compute
 check_engine_stats lflow recompute nocompute
 
 # Modify the backend of the lb1 vip
 check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
 check ovn-nbctl --wait=sb set load_balancer lb1 vips:'"10.0.0.10:80"'='"10.0.0.100:80"'
 check_engine_stats lb_data norecompute compute
-check_engine_stats northd recompute nocompute
+check_engine_stats northd norecompute compute
 check_engine_stats lflow recompute nocompute
 CHECK_NO_CHANGE_AFTER_RECOMPUTE
 
@@ -10575,7 +10587,7 @@  CHECK_NO_CHANGE_AFTER_RECOMPUTE
 check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
 check ovn-nbctl --wait=sb clear load_Balancer lb1 vips
 check_engine_stats lb_data norecompute compute
-check_engine_stats northd recompute nocompute
+check_engine_stats northd norecompute compute
 check_engine_stats lflow recompute nocompute
 CHECK_NO_CHANGE_AFTER_RECOMPUTE
 
@@ -10583,7 +10595,7 @@  CHECK_NO_CHANGE_AFTER_RECOMPUTE
 check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
 check ovn-nbctl --wait=sb lb-add lb1 10.0.0.10:80 10.0.0.3:80
 check_engine_stats lb_data norecompute compute
-check_engine_stats northd recompute nocompute
+check_engine_stats northd norecompute compute
 check_engine_stats lflow recompute nocompute
 CHECK_NO_CHANGE_AFTER_RECOMPUTE
 
@@ -10591,7 +10603,7 @@  CHECK_NO_CHANGE_AFTER_RECOMPUTE
 check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
 check ovn-nbctl --wait=sb lb-add lb1 10.0.0.20:80 10.0.0.30:8080
 check_engine_stats lb_data norecompute compute
-check_engine_stats northd recompute nocompute
+check_engine_stats northd norecompute compute
 check_engine_stats lflow recompute nocompute
 CHECK_NO_CHANGE_AFTER_RECOMPUTE
 
@@ -10650,7 +10662,7 @@  check_engine_stats lflow recompute nocompute
 check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
 check ovn-nbctl add logical_switch sw0 load_balancer_group $lbg1_uuid
 check_engine_stats lb_data norecompute compute
-check_engine_stats northd recompute nocompute
+check_engine_stats northd norecompute compute
 check_engine_stats lflow recompute nocompute
 
 check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
@@ -10691,7 +10703,7 @@  check_engine_stats lflow recompute nocompute
 check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
 check ovn-nbctl set logical_switch sw0 load_balancer_group=$lbg1_uuid
 check_engine_stats lb_data norecompute compute
-check_engine_stats northd recompute nocompute
+check_engine_stats northd norecompute compute
 check_engine_stats lflow recompute nocompute
 CHECK_NO_CHANGE_AFTER_RECOMPUTE