[ovs-dev,v6,06/16] northd: Handle load balancer group changes for a logical switch.

From: Numan Siddique <numans@ovn.org>

For every 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.

Signed-off-by: Numan Siddique <numans@ovn.org>
---
 northd/en-lb-data.c | 102 ++++++++++++++++++++++++++++++++------------
 northd/en-lb-data.h |   4 ++
 northd/en-northd.c  |   9 +++-
 northd/northd.c     |  70 ++++++++++++++++++++++++------
 northd/northd.h     |   5 ++-
 tests/ovn-northd.at |  30 +++++++++----
 6 files changed, 169 insertions(+), 51 deletions(-)

On Fri, Aug 18, 2023 at 10:58 AM <numans@ovn.org> wrote:
>
> From: Numan Siddique <numans@ovn.org>
>
> For every 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.
>
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>  northd/en-lb-data.c | 102 ++++++++++++++++++++++++++++++++------------
>  northd/en-lb-data.h |   4 ++
>  northd/en-northd.c  |   9 +++-
>  northd/northd.c     |  70 ++++++++++++++++++++++++------
>  northd/northd.h     |   5 ++-
>  tests/ovn-northd.at |  30 +++++++++----
>  6 files changed, 169 insertions(+), 51 deletions(-)
>
> diff --git a/northd/en-lb-data.c b/northd/en-lb-data.c
> index e0c4db1422..8619b4cc14 100644
> --- a/northd/en-lb-data.c
> +++ b/northd/en-lb-data.c
> @@ -63,6 +63,7 @@ static struct crupdated_lb_group *
>  static void add_deleted_lb_group_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
> @@ -271,12 +272,15 @@ lb_data_logical_switch_handler(struct engine_node *node, void *data)
>                  destroy_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);
> @@ -285,38 +289,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);
>          }
>      }
> @@ -412,7 +444,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;
>          }
>
> @@ -422,6 +454,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);
> +        }
>      }
>  }
>
> @@ -431,7 +467,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));
> @@ -456,7 +494,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);
>  }
>
> @@ -467,6 +507,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_lb_grops = 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) {
> @@ -497,7 +538,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 b2f86322a2..bc09ddb7eb 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 {
> @@ -62,6 +63,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;
>  };
>
>  /* struct which maintains the data of the engine node lb_data. */
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index 9d1838a1a4..545971f76f 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -236,6 +236,10 @@ northd_lb_data_handler_pre_od(struct engine_node *node, void *data)
>          return false;
>      }
>
> +    if (lb_data->tracked_lb_data.has_dissassoc_lbgrps_from_od) {
> +        return false;
> +    }
> +
>      struct northd_data *nd = data;
>
>      if (!northd_handle_lb_data_changes_pre_od(&lb_data->tracked_lb_data,
> @@ -258,12 +262,15 @@ northd_lb_data_handler_post_od(struct engine_node *node, void *data)
>
>      ovs_assert(lb_data->tracked);
>      ovs_assert(!lb_data->tracked_lb_data.has_dissassoc_lbs_from_od);
> +    ovs_assert(!lb_data->tracked_lb_data.has_dissassoc_lbgrps_from_od);
> +    ovs_assert(!lb_data->tracked_lb_data.has_dissassoc_lbs_from_lb_grops);
>
>      struct northd_data *nd = data;
>
>      if (!northd_handle_lb_data_changes_post_od(&lb_data->tracked_lb_data,
>                                                 &nd->ls_datapaths,
> -                                               &nd->lb_datapaths_map)) {
> +                                               &nd->lb_datapaths_map,
> +                                               &nd->lb_group_datapaths_map)) {
>          return false;
>      }
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 6e8efbd496..1477b79331 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -5035,6 +5035,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(
> @@ -5045,7 +5046,8 @@ ls_changes_can_be_handled(
>      for (col = 0; col < NBREC_LOGICAL_SWITCH_N_COLUMNS; col++) {
>          if (nbrec_logical_switch_is_updated(ls, col)) {
>              if (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;
> @@ -5076,12 +5078,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) {
> @@ -5298,7 +5294,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,
> @@ -5434,7 +5434,7 @@ northd_handle_lb_data_changes_pre_od(struct tracked_lb_data *trk_lb_data,
>                                       struct ovn_datapaths *ls_datapaths,
>                                       struct ovn_datapaths *lr_datapaths,
>                                       struct hmap *lb_datapaths_map,
> -                                     struct hmap *lb_group_datapaths_map)
> +                                     struct hmap *lbgrp_datapaths_map)
>  {
>      struct ovn_lb_datapaths *lb_dps;
>      struct ovn_northd_lb *lb;
> @@ -5482,12 +5482,12 @@ northd_handle_lb_data_changes_pre_od(struct tracked_lb_data *trk_lb_data,
>          lbg = crupdated_lbg->lbg;
>          const struct uuid *lb_uuid = &lbg->uuid;
>
> -        lb_group_dps = ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> +        lb_group_dps = ovn_lb_group_datapaths_find(lbgrp_datapaths_map,
>                                                     lb_uuid);
>          if (!lb_group_dps) {
>              lb_group_dps = ovn_lb_group_datapaths_create(
>                  lbg, ods_size(ls_datapaths), ods_size(lr_datapaths));
> -            hmap_insert(lb_group_datapaths_map, &lb_group_dps->hmap_node,
> +            hmap_insert(lbgrp_datapaths_map, &lb_group_dps->hmap_node,
>                          uuid_hash(lb_uuid));
>          }
>      }
> @@ -5511,12 +5511,15 @@ northd_handle_lb_data_changes_pre_od(struct tracked_lb_data *trk_lb_data,
>  bool
>  northd_handle_lb_data_changes_post_od(struct tracked_lb_data *trk_lb_data,
>                                        struct ovn_datapaths *ls_datapaths,
> -                                      struct hmap *lb_datapaths_map)
> +                                      struct hmap *lb_datapaths_map,
> +                                      struct hmap *lbgrp_datapaths_map)
>  {
>      ovs_assert(!trk_lb_data->has_health_checks);
> +    ovs_assert(!trk_lb_data->has_dissassoc_lbs_from_lb_grops);
>
>      struct ovn_northd_lb *lb;
>      struct ovn_lb_datapaths *lb_dps;
> +    struct ovn_lb_group_datapaths *lbgrp_dps;
>      struct ovn_datapath *od;
>      struct crupdated_od_lb_data *codlb;
>
> @@ -5531,6 +5534,22 @@ northd_handle_lb_data_changes_post_od(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);
>      }
> @@ -5551,6 +5570,33 @@ northd_handle_lb_data_changes_post_od(struct tracked_lb_data *trk_lb_data,
>          }
>      }
>
> +    struct ovn_lb_group *lbgrp;
> +    struct crupdated_lb_group *crupdated_lbg;
> +    HMAP_FOR_EACH (crupdated_lbg, hmap_node,
> +                   &trk_lb_data->crupdated_lb_groups) {
> +        lbgrp = crupdated_lbg->lbg;
> +        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_lbg->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/northd/northd.h b/northd/northd.h
> index 0ed7215356..044d4ee0c0 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -354,10 +354,11 @@ bool northd_handle_lb_data_changes_pre_od(struct tracked_lb_data *,
>                                            struct ovn_datapaths *ls_datapaths,
>                                            struct ovn_datapaths *lr_datapaths,
>                                            struct hmap *lb_datapaths_map,
> -                                          struct hmap *lb_group_datapaths_map);
> +                                          struct hmap *lbgrp_datapaths_map);
>  bool northd_handle_lb_data_changes_post_od(struct tracked_lb_data *,
>                                             struct ovn_datapaths *ls_datapaths,
> -                                           struct hmap *lb_datapaths_map);
> +                                           struct hmap *lb_datapaths_map,
> +                                           struct hmap *lbgrp_datapaths_map);
>
>  void build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
>                       const struct nbrec_bfd_table *,
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 9e9b26ce09..b8b2ee390e 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -2141,6 +2141,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
>  ])
>
> @@ -10016,21 +10028,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
>
> @@ -10038,7 +10050,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
>
> @@ -10046,7 +10058,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
>
> @@ -10054,7 +10066,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
>
> @@ -10113,7 +10125,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
> @@ -10154,7 +10166,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
>
> --
> 2.40.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


Looks good to me, thanks.

Reviewed-by: Ales Musil <amusil@redhat.com>

On Fri, Aug 18, 2023 at 1:58 AM <numans@ovn.org> wrote:
>
> From: Numan Siddique <numans@ovn.org>
>
> For every 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.
>
> Signed-off-by: Numan Siddique <numans@ovn.org>

Thanks Numan. Some of my comments/questions to the previous patch also
apply to this patch because the logic is similar. Otherwise looks good to
me.

Regards,
Han

> ---
>  northd/en-lb-data.c | 102 ++++++++++++++++++++++++++++++++------------
>  northd/en-lb-data.h |   4 ++
>  northd/en-northd.c  |   9 +++-
>  northd/northd.c     |  70 ++++++++++++++++++++++++------
>  northd/northd.h     |   5 ++-
>  tests/ovn-northd.at |  30 +++++++++----
>  6 files changed, 169 insertions(+), 51 deletions(-)
>
> diff --git a/northd/en-lb-data.c b/northd/en-lb-data.c
> index e0c4db1422..8619b4cc14 100644
> --- a/northd/en-lb-data.c
> +++ b/northd/en-lb-data.c
> @@ -63,6 +63,7 @@ static struct crupdated_lb_group *
>  static void add_deleted_lb_group_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
> @@ -271,12 +272,15 @@ lb_data_logical_switch_handler(struct engine_node
*node, void *data)
>                  destroy_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);
> @@ -285,38 +289,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);
>          }
>      }
> @@ -412,7 +444,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;
>          }
>
> @@ -422,6 +454,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);
> +        }
>      }
>  }
>
> @@ -431,7 +467,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));
> @@ -456,7 +494,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);
>  }
>
> @@ -467,6 +507,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_lb_grops = 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) {
> @@ -497,7 +538,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 b2f86322a2..bc09ddb7eb 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 {
> @@ -62,6 +63,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;
>  };
>
>  /* struct which maintains the data of the engine node lb_data. */
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index 9d1838a1a4..545971f76f 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -236,6 +236,10 @@ northd_lb_data_handler_pre_od(struct engine_node
*node, void *data)
>          return false;
>      }
>
> +    if (lb_data->tracked_lb_data.has_dissassoc_lbgrps_from_od) {
> +        return false;
> +    }
> +
>      struct northd_data *nd = data;
>
>      if (!northd_handle_lb_data_changes_pre_od(&lb_data->tracked_lb_data,
> @@ -258,12 +262,15 @@ northd_lb_data_handler_post_od(struct engine_node
*node, void *data)
>
>      ovs_assert(lb_data->tracked);
>      ovs_assert(!lb_data->tracked_lb_data.has_dissassoc_lbs_from_od);
> +    ovs_assert(!lb_data->tracked_lb_data.has_dissassoc_lbgrps_from_od);
> +
 ovs_assert(!lb_data->tracked_lb_data.has_dissassoc_lbs_from_lb_grops);
>
>      struct northd_data *nd = data;
>
>      if (!northd_handle_lb_data_changes_post_od(&lb_data->tracked_lb_data,
>                                                 &nd->ls_datapaths,
> -                                               &nd->lb_datapaths_map)) {
> +                                               &nd->lb_datapaths_map,
> +
&nd->lb_group_datapaths_map)) {
>          return false;
>      }
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 6e8efbd496..1477b79331 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -5035,6 +5035,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(
> @@ -5045,7 +5046,8 @@ ls_changes_can_be_handled(
>      for (col = 0; col < NBREC_LOGICAL_SWITCH_N_COLUMNS; col++) {
>          if (nbrec_logical_switch_is_updated(ls, col)) {
>              if (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;
> @@ -5076,12 +5078,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) {
> @@ -5298,7 +5294,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,
> @@ -5434,7 +5434,7 @@ northd_handle_lb_data_changes_pre_od(struct
tracked_lb_data *trk_lb_data,
>                                       struct ovn_datapaths *ls_datapaths,
>                                       struct ovn_datapaths *lr_datapaths,
>                                       struct hmap *lb_datapaths_map,
> -                                     struct hmap *lb_group_datapaths_map)
> +                                     struct hmap *lbgrp_datapaths_map)
>  {
>      struct ovn_lb_datapaths *lb_dps;
>      struct ovn_northd_lb *lb;
> @@ -5482,12 +5482,12 @@ northd_handle_lb_data_changes_pre_od(struct
tracked_lb_data *trk_lb_data,
>          lbg = crupdated_lbg->lbg;
>          const struct uuid *lb_uuid = &lbg->uuid;
>
> -        lb_group_dps =
ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> +        lb_group_dps = ovn_lb_group_datapaths_find(lbgrp_datapaths_map,
>                                                     lb_uuid);
>          if (!lb_group_dps) {
>              lb_group_dps = ovn_lb_group_datapaths_create(
>                  lbg, ods_size(ls_datapaths), ods_size(lr_datapaths));
> -            hmap_insert(lb_group_datapaths_map, &lb_group_dps->hmap_node,
> +            hmap_insert(lbgrp_datapaths_map, &lb_group_dps->hmap_node,
>                          uuid_hash(lb_uuid));
>          }
>      }
> @@ -5511,12 +5511,15 @@ northd_handle_lb_data_changes_pre_od(struct
tracked_lb_data *trk_lb_data,
>  bool
>  northd_handle_lb_data_changes_post_od(struct tracked_lb_data
*trk_lb_data,
>                                        struct ovn_datapaths *ls_datapaths,
> -                                      struct hmap *lb_datapaths_map)
> +                                      struct hmap *lb_datapaths_map,
> +                                      struct hmap *lbgrp_datapaths_map)
>  {
>      ovs_assert(!trk_lb_data->has_health_checks);
> +    ovs_assert(!trk_lb_data->has_dissassoc_lbs_from_lb_grops);
>
>      struct ovn_northd_lb *lb;
>      struct ovn_lb_datapaths *lb_dps;
> +    struct ovn_lb_group_datapaths *lbgrp_dps;
>      struct ovn_datapath *od;
>      struct crupdated_od_lb_data *codlb;
>
> @@ -5531,6 +5534,22 @@ northd_handle_lb_data_changes_post_od(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);
>      }
> @@ -5551,6 +5570,33 @@ northd_handle_lb_data_changes_post_od(struct
tracked_lb_data *trk_lb_data,
>          }
>      }
>
> +    struct ovn_lb_group *lbgrp;
> +    struct crupdated_lb_group *crupdated_lbg;
> +    HMAP_FOR_EACH (crupdated_lbg, hmap_node,
> +                   &trk_lb_data->crupdated_lb_groups) {
> +        lbgrp = crupdated_lbg->lbg;
> +        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_lbg->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/northd/northd.h b/northd/northd.h
> index 0ed7215356..044d4ee0c0 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -354,10 +354,11 @@ bool northd_handle_lb_data_changes_pre_od(struct
tracked_lb_data *,
>                                            struct ovn_datapaths
*ls_datapaths,
>                                            struct ovn_datapaths
*lr_datapaths,
>                                            struct hmap *lb_datapaths_map,
> -                                          struct hmap
*lb_group_datapaths_map);
> +                                          struct hmap
*lbgrp_datapaths_map);
>  bool northd_handle_lb_data_changes_post_od(struct tracked_lb_data *,
>                                             struct ovn_datapaths
*ls_datapaths,
> -                                           struct hmap
*lb_datapaths_map);
> +                                           struct hmap *lb_datapaths_map,
> +                                           struct hmap
*lbgrp_datapaths_map);
>
>  void build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
>                       const struct nbrec_bfd_table *,
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 9e9b26ce09..b8b2ee390e 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -2141,6 +2141,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
>  ])
>
> @@ -10016,21 +10028,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
>
> @@ -10038,7 +10050,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
>
> @@ -10046,7 +10058,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
>
> @@ -10054,7 +10066,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
>
> @@ -10113,7 +10125,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
> @@ -10154,7 +10166,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
>
> --
> 2.40.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev