On Mon, Sep 11, 2023 at 9:01 AM <num...@ovn.org> wrote:
>
> From: Numan Siddique <num...@ovn.org>
>
> 'lb_data' engine node now also handles logical switch changes.
> Its data maintains ls to lb related information. i.e if a
> logical switch sw0 has lb1, lb2 and lb3 associated then
> it stores this info in its data. And when a new load balancer
> lb4 is associated to it, it stores this information in its
> tracked data so that 'northd' engine node can handle it
> accordingly. Tracked data will have information like:
> changed ls -> {sw0 : {associated_lbs: [lb4]}
>
> The first handler 'northd_lb_data_handler_pre_od' is called before the
> 'northd_nb_logical_switch_handler' handler and it just creates or
> deletes the lb_datapaths hmap for the tracked lbs.
>
> The northd handler 'northd_lb_data_handler' updates the
> ovn_lb_datapaths's 'nb_ls_map' bitmap accordingly.
>
> Eg. If the lb_data has the below tracked data:
>
> tracked_data = {'crupdated_lbs': [lb1, lb2],
> 'deleted_lbs': [lb3],
> 'crupdated_lb_groups': [lbg1, lbg2],
> 'crupdated_ls_lbs': [{ls: sw0, assoc_lbs: [lb1],
> {ls: sw1, assoc_lbs: [lb1, lb2]}
>
> The handler northd_lb_data_handler(), creates the
> ovn_lb_datapaths object for lb1 and lb2 and deletes lb3 from
> the ovn_lb_datapaths hmap. It does the same for the created or updated lb
> groups lbg1 and lbg2 in the ovn_lbgrp_datapaths map. It also updates the
> nb_ls_bitmap of lb1 for sw0 and sw1 and nb_ls_bitmap of lb2 for sw1.
>
> Reviewed-by: Ales Musil <amu...@redhat.com>
> Acked-by: Mark Michelson <mmich...@redhat.com>
> Signed-off-by: Numan Siddique <num...@ovn.org>
> ---
> lib/lb.c | 5 +-
> northd/en-lb-data.c | 176 +++++++++++++++++++++++++++++++++++++++
> northd/en-lb-data.h | 17 ++++
> northd/en-lflow.c | 6 ++
> northd/en-northd.c | 6 +-
> northd/inc-proc-northd.c | 2 +
> northd/northd.c | 83 +++++++++++++++---
> northd/northd.h | 4 +-
> tests/ovn-northd.at | 56 +++++++++----
> 9 files changed, 322 insertions(+), 33 deletions(-)
>
> diff --git a/lib/lb.c b/lib/lb.c
> index 6fd67e2218..e6c9dc2be2 100644
> --- a/lib/lb.c
> +++ b/lib/lb.c
> @@ -1088,7 +1088,10 @@ ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths
*lb_dps, size_t n,
> struct ovn_datapath **ods)
> {
> for (size_t i = 0; i < n; i++) {
> - bitmap_set1(lb_dps->nb_ls_map, ods[i]->index);
> + if (!bitmap_is_set(lb_dps->nb_ls_map, ods[i]->index)) {
> + bitmap_set1(lb_dps->nb_ls_map, ods[i]->index);
> + lb_dps->n_nb_ls++;
> + }
> }
> }
>
> diff --git a/northd/en-lb-data.c b/northd/en-lb-data.c
> index 8acd9c8cb2..02b1bfd7a4 100644
> --- a/northd/en-lb-data.c
> +++ b/northd/en-lb-data.c
> @@ -39,6 +39,14 @@ static void lb_data_destroy(struct ed_type_lb_data *);
> static void build_lbs(const struct nbrec_load_balancer_table *,
> const struct nbrec_load_balancer_group_table *,
> struct hmap *lbs, struct hmap *lb_groups);
> +static void build_od_lb_map(const struct nbrec_logical_switch_table *,
> + struct hmap *od_lb_map);
> +static struct od_lb_data *find_od_lb_data(struct hmap *od_lb_map,
> + const struct uuid *od_uuid);
> +static void destroy_od_lb_data(struct od_lb_data *od_lb_data);
> +static struct od_lb_data *create_od_lb_data(struct hmap *od_lb_map,
> + const struct uuid *od_uuid);
> +
> static struct ovn_lb_group *create_lb_group(
> const struct nbrec_load_balancer_group *, struct hmap *lbs,
> struct hmap *lb_groups);
> @@ -54,6 +62,7 @@ static struct crupdated_lbgrp *
> struct tracked_lb_data *);
> 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);
>
> /* 'lb_data' engine node manages the NB load balancers and load balancer
> * groups. For each NB LB, it creates 'struct ovn_northd_lb' and
> @@ -80,9 +89,13 @@ en_lb_data_run(struct engine_node *node, void *data)
> EN_OVSDB_GET(engine_get_input("NB_load_balancer", node));
> const struct nbrec_load_balancer_group_table *nb_lbg_table =
> EN_OVSDB_GET(engine_get_input("NB_load_balancer_group", node));
> + const struct nbrec_logical_switch_table *nb_ls_table =
> + EN_OVSDB_GET(engine_get_input("NB_logical_switch", node));
>
> lb_data->tracked = false;
> build_lbs(nb_lb_table, nb_lbg_table, &lb_data->lbs,
&lb_data->lbgrps);
> + build_od_lb_map(nb_ls_table, &lb_data->ls_lb_map);
> +
> engine_set_node_state(node, EN_UPDATED);
> }
>
> @@ -230,18 +243,104 @@ lb_data_load_balancer_group_handler(struct
engine_node *node, void *data)
> return true;
> }
>
> +struct od_lb_data {
> + struct hmap_node hmap_node;
> + struct uuid od_uuid;
> + struct uuidset *lbs;
> + struct uuidset *lbgrps;
> +};
> +
> +bool
> +lb_data_logical_switch_handler(struct engine_node *node, void *data)
> +{
> + struct ed_type_lb_data *lb_data = (struct ed_type_lb_data *) data;
> + const struct nbrec_logical_switch_table *nb_ls_table =
> + EN_OVSDB_GET(engine_get_input("NB_logical_switch", node));
> +
> + struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> + lb_data->tracked = true;
> +
> + bool changed = false;
> + const struct nbrec_logical_switch *nbs;
> + NBREC_LOGICAL_SWITCH_TABLE_FOR_EACH_TRACKED (nbs, nb_ls_table) {
> + if (nbrec_logical_switch_is_deleted(nbs)) {
> + struct od_lb_data *od_lb_data =
> + find_od_lb_data(&lb_data->ls_lb_map, &nbs->header_.uuid);
> + if (od_lb_data) {
> + hmap_remove(&lb_data->ls_lb_map, &od_lb_data->hmap_node);
> + destroy_od_lb_data(od_lb_data);
> + }
> + } else {
> + if (!is_ls_lbs_changed(nbs)) {
> + continue;
> + }
> + struct crupdated_od_lb_data *codlb = xzalloc(sizeof *codlb);
> + codlb->od_uuid = nbs->header_.uuid;
> + uuidset_init(&codlb->assoc_lbs);
> +
> + struct od_lb_data *od_lb_data =
> + find_od_lb_data(&lb_data->ls_lb_map, &nbs->header_.uuid);
> + if (!od_lb_data) {
> + od_lb_data = create_od_lb_data(&lb_data->ls_lb_map,
> + &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);
> +
> + 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;
> + }
> +
> + uuidset_destroy(pre_lb_uuids);
> + free(pre_lb_uuids);
> +
> + ovs_list_insert(&trk_lb_data->crupdated_ls_lbs,
&codlb->list_node);
> + }
> + }
> +
> + if (changed) {
> + engine_set_node_state(node, EN_UPDATED);
> + }
> + return true;
> +}
> +
> /* static functions. */
> static void
> lb_data_init(struct ed_type_lb_data *lb_data)
> {
> hmap_init(&lb_data->lbs);
> hmap_init(&lb_data->lbgrps);
> + hmap_init(&lb_data->ls_lb_map);
>
> struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> hmap_init(&trk_lb_data->crupdated_lbs);
> hmapx_init(&trk_lb_data->deleted_lbs);
> hmap_init(&trk_lb_data->crupdated_lbgrps);
> hmapx_init(&trk_lb_data->deleted_lbgrps);
> + ovs_list_init(&trk_lb_data->crupdated_ls_lbs);
> }
>
> static void
> @@ -259,6 +358,12 @@ lb_data_destroy(struct ed_type_lb_data *lb_data)
> }
> hmap_destroy(&lb_data->lbgrps);
>
> + struct od_lb_data *od_lb_data;
> + HMAP_FOR_EACH_POP (od_lb_data, hmap_node, &lb_data->ls_lb_map) {
> + destroy_od_lb_data(od_lb_data);
> + }
> + hmap_destroy(&lb_data->ls_lb_map);
> +
> destroy_tracked_data(lb_data);
> hmap_destroy(&lb_data->tracked_lb_data.crupdated_lbs);
> hmapx_destroy(&lb_data->tracked_lb_data.deleted_lbs);
> @@ -301,12 +406,67 @@ create_lb_group(const struct
nbrec_load_balancer_group *nbrec_lb_group,
> return lb_group;
> }
>
> +static void
> +build_od_lb_map(const struct nbrec_logical_switch_table *nbrec_ls_table,
> + struct hmap *od_lb_map)
> +{
> + const struct nbrec_logical_switch *nbrec_ls;
> + NBREC_LOGICAL_SWITCH_TABLE_FOR_EACH (nbrec_ls, nbrec_ls_table) {
> + if (!nbrec_ls->n_load_balancer) {
> + continue;
> + }
> +
> + struct od_lb_data *od_lb_data =
> + create_od_lb_data(od_lb_map, &nbrec_ls->header_.uuid);
> + for (size_t i = 0; i < nbrec_ls->n_load_balancer; i++) {
> + uuidset_insert(od_lb_data->lbs,
> + &nbrec_ls->load_balancer[i]->header_.uuid);
> + }
> + }
> +}
> +
> +static struct od_lb_data *
> +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);
> + uuidset_init(od_lb_data->lbs);
> +
> + hmap_insert(od_lb_map, &od_lb_data->hmap_node,
> + uuid_hash(&od_lb_data->od_uuid));
> + return od_lb_data;
> +}
> +
> +static struct od_lb_data *
> +find_od_lb_data(struct hmap *od_lb_map, const struct uuid *od_uuid)
> +{
> + struct od_lb_data *od_lb_data;
> + HMAP_FOR_EACH_WITH_HASH (od_lb_data, hmap_node, uuid_hash(od_uuid),
> + od_lb_map) {
> + if (uuid_equals(&od_lb_data->od_uuid, od_uuid)) {
> + return od_lb_data;
> + }
> + }
> +
> + return NULL;
> +}
> +
> +static void
> +destroy_od_lb_data(struct od_lb_data *od_lb_data)
> +{
> + uuidset_destroy(od_lb_data->lbs);
> + free(od_lb_data->lbs);
> + free(od_lb_data);
> +}
> +
> static void
> destroy_tracked_data(struct ed_type_lb_data *lb_data)
> {
> lb_data->tracked = false;
> 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;
>
> struct hmapx_node *node;
> HMAPX_FOR_EACH_SAFE (node, &lb_data->tracked_lb_data.deleted_lbs) {
> @@ -331,6 +491,15 @@ destroy_tracked_data(struct ed_type_lb_data *lb_data)
> hmapx_destroy(&crupdated_lbg->assoc_lbs);
> free(crupdated_lbg);
> }
> +
> + struct crupdated_od_lb_data *codlb;
> + LIST_FOR_EACH_SAFE (codlb, list_node,
> + &lb_data->tracked_lb_data.crupdated_ls_lbs) {
> + ovs_list_remove(&codlb->list_node);
> + uuidset_destroy(&codlb->assoc_lbs);
> +
> + free(codlb);
> + }
> }
>
> static void
> @@ -376,3 +545,10 @@ add_deleted_lbgrp_to_tracked_data(struct
ovn_lb_group *lbg,
> {
> hmapx_add(&tracked_lb_data->deleted_lbgrps, lbg);
> }
> +
> +static bool
> +is_ls_lbs_changed(const struct nbrec_logical_switch *nbs) {
> + return ((nbrec_logical_switch_is_new(nbs) && nbs->n_load_balancer)
> + || nbrec_logical_switch_is_updated(nbs,
> + NBREC_LOGICAL_SWITCH_COL_LOAD_BALANCER));
> +}
> diff --git a/northd/en-lb-data.h b/northd/en-lb-data.h
> index afb163dd9f..022b38523b 100644
> --- a/northd/en-lb-data.h
> +++ b/northd/en-lb-data.h
> @@ -6,6 +6,7 @@
> #include "openvswitch/hmap.h"
> #include "include/openvswitch/list.h"
> #include "lib/hmapx.h"
> +#include "lib/uuidset.h"
>
> #include "lib/inc-proc-eng.h"
>
> @@ -27,6 +28,13 @@ struct crupdated_lbgrp {
> struct hmapx assoc_lbs;
> };
>
> +struct crupdated_od_lb_data {
> + struct ovs_list list_node;
> +
> + struct uuid od_uuid;
> + struct uuidset assoc_lbs;
> +};
> +
> struct tracked_lb_data {
> /* Both created and updated lbs. hmapx node is 'struct crupdated_lb
*'. */
> struct hmap crupdated_lbs;
> @@ -41,12 +49,19 @@ struct tracked_lb_data {
> /* Deleted lb_groups. hmapx node is 'struct ovn_lb_group *'. */
> struct hmapx deleted_lbgrps;
>
> + /* List of logical switch <-> lb changes. List node is
> + * 'struct crupdated_od_lb_data' */
> + struct ovs_list crupdated_ls_lbs;
> +
> /* Indicates if any of the tracked lb has health checks enabled. */
> bool has_health_checks;
>
> /* Indicates if any lb got disassociated from a lb group
> * but not deleted. */
> bool has_dissassoc_lbs_from_lbgrps;
> +
> + /* Indicates if a lb was disassociated from a logical switch. */
> + bool has_dissassoc_lbs_from_od;
> };
>
> /* struct which maintains the data of the engine node lb_data. */
> @@ -56,6 +71,7 @@ struct ed_type_lb_data {
>
> /* hmap of load balancer groups. hmap node is 'struct ovn_lb_group
*' */
> struct hmap lbgrps;
> + struct hmap ls_lb_map;
>
> /* tracked data*/
> bool tracked;
> @@ -69,5 +85,6 @@ void en_lb_data_clear_tracked_data(void *data);
>
> bool lb_data_load_balancer_handler(struct engine_node *, void *data);
> bool lb_data_load_balancer_group_handler(struct engine_node *, void
*data);
> +bool lb_data_logical_switch_handler(struct engine_node *, void *data);
>
> #endif /* end of EN_NORTHD_LB_DATA_H */
> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> index ad8b62c735..2b84fef0ef 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -107,6 +107,12 @@ lflow_northd_handler(struct engine_node *node,
> if (!northd_data->change_tracked) {
> return false;
> }
> +
> + /* Fall back to recompute if lb related data has changed. */
> + if (northd_data->lb_changed) {
> + return false;
> + }
> +
> const struct engine_context *eng_ctx = engine_get_context();
> struct lflow_data *lflow_data = data;
>
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index 1704a18834..8487b003f7 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -1,4 +1,4 @@
> -/*
> + /*
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> * You may obtain a copy of the License at:
> @@ -214,6 +214,10 @@ northd_lb_data_handler(struct engine_node *node,
void *data)
> return false;
> }
>
> + /* Indicate the depedendant engine nodes that load balancer/group
> + * related data has changed (including association to logical
> + * switch/router). */
> + nd->lb_changed = true;
> engine_set_node_state(node, EN_UPDATED);
> return true;
> }
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> index ccc6687207..303b58d43f 100644
> --- a/northd/inc-proc-northd.c
> +++ b/northd/inc-proc-northd.c
> @@ -155,6 +155,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> lb_data_load_balancer_handler);
> engine_add_input(&en_lb_data, &en_nb_load_balancer_group,
> lb_data_load_balancer_group_handler);
> + engine_add_input(&en_lb_data, &en_nb_logical_switch,
> + lb_data_logical_switch_handler);
>
> engine_add_input(&en_northd, &en_nb_logical_router, NULL);
> engine_add_input(&en_northd, &en_nb_mirror, NULL);
> diff --git a/northd/northd.c b/northd/northd.c
> index e9cb906e2a..f767e0b39f 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -73,7 +73,7 @@ static bool install_ls_lb_from_router;
> /* Use common zone for SNAT and DNAT if this option is set to "true". */
> static bool use_common_zone = false;
>
> -/* MAC allocated for service monitor usage. Just one mac is allocated
> +/* MAC allocated for service monitor usage. Just one mac is
allocatedg5534
> * for this purpose and ovn-controller's on each chassis will make use
> * of this mac when sending out the packets to monitor the services
> * defined in Service_Monitor Southbound table. Since these packets
> @@ -852,7 +852,6 @@ ovn_datapath_destroy(struct hmap *datapaths, struct
ovn_datapath *od)
> free(od->l3dgw_ports);
> destroy_mcast_info_for_datapath(od);
> destroy_ports_for_datapath(od);
> -
> free(od);
> }
> }
> @@ -4959,6 +4958,7 @@ destroy_northd_data_tracked_changes(struct
northd_data *nd)
> }
>
> nd->change_tracked = false;
> + nd->lb_changed = false;
> }
>
> /* Check if a changed LSP can be handled incrementally within the I-P
engine
> @@ -5074,6 +5074,7 @@ ls_port_create(struct ovsdb_idl_txn *ovnsb_txn,
struct hmap *ls_ports,
> * incrementally handled.
> * Presently supports i-p for the below changes:
> * - logical switch ports.
> + * - load balancers.
> */
> static bool
> ls_changes_can_be_handled(
> @@ -5084,7 +5085,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_ACLS ||
> - col == NBREC_LOGICAL_SWITCH_COL_PORTS) {
> + col == NBREC_LOGICAL_SWITCH_COL_PORTS ||
> + col == NBREC_LOGICAL_SWITCH_COL_LOAD_BALANCER) {
> continue;
> }
> return false;
> @@ -5109,12 +5111,6 @@ ls_changes_can_be_handled(
> return false;
> }
> }
> - for (size_t i = 0; i < ls->n_load_balancer; i++) {
> - if (nbrec_load_balancer_row_get_seqno(ls->load_balancer[i],
> - OVSDB_IDL_CHANGE_MODIFY) > 0) {
> - 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) {
> @@ -5365,6 +5361,7 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
*ovnsb_idl_txn,
> if (!ovs_list_is_empty(&nd->tracked_ls_changes.updated)) {
> nd->change_tracked = true;
> }
> +
> return true;
>
> fail:
> @@ -5431,9 +5428,20 @@ northd_handle_sb_port_binding_changes(
> return true;
> }
>
> -/* Handler for lb_data engine changes. For every tracked lb_data
> - * it creates or deletes the ovn_lb_datapaths/ovn_lb_group_datapaths
> - * from the lb_datapaths hmap and lb_group_datapaths hmap. */
> +/* Handler for lb_data engine changes. It does the following
> + * For every tracked 'lb' and 'lb_group'
> + * - it creates or deletes the ovn_lb_datapaths/ovn_lb_group_datapaths
> + * from the lb_datapaths hmap and lb_group_datapaths hmap.
> + *
> + * - For any changes to a logical switch (in
'trk_lb_data->crupdated_ls_lbs')
> + * due to association of a load balancer (eg. ovn-nbctl ls-lb-add sw0
lb1),
> + * the logical switch datapath is added to the load balancer
(represented
> + * by 'struct ovn_lb_datapaths') by calling ovn_lb_datapaths_add_ls().
> + *
> + * - For every 'lb' in the tracked data (trk_lb_data->crupdated_lbs) ,
> + * it gets the associated logical switches and for each switch it
> + * re-evaluates 'od->has_lb_vip' to reflect any changes to the lb
vips.
> + * */
> bool
> northd_handle_lb_data_changes(struct tracked_lb_data *trk_lb_data,
> struct ovn_datapaths *ls_datapaths,
> @@ -5459,8 +5467,15 @@ northd_handle_lb_data_changes(struct
tracked_lb_data *trk_lb_data,
> return false;
> }
>
> + /* Fall back to recompute if any load balancer has been
disassociated from
> + * a logical switch or router. */
> + if (trk_lb_data->has_dissassoc_lbs_from_od) {
> + return false;
> + }
> +
> struct ovn_lb_datapaths *lb_dps;
> struct ovn_northd_lb *lb;
> + struct ovn_datapath *od;
> struct hmapx_node *hmapx_node;
> HMAPX_FOR_EACH (hmapx_node, &trk_lb_data->deleted_lbs) {
> lb = hmapx_node->data;
> @@ -5468,10 +5483,22 @@ northd_handle_lb_data_changes(struct
tracked_lb_data *trk_lb_data,
>
> lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> ovs_assert(lb_dps);
> +
> + /* Re-evaluate 'od->has_lb_vip for od's associated with the
> + * deleted lb. */
> + size_t index;
> + BITMAP_FOR_EACH_1 (index, ods_size(ls_datapaths),
> + lb_dps->nb_ls_map) {
> + od = ls_datapaths->array[index];
> + init_lb_for_datapath(od);
> + }
> +
> hmap_remove(lb_datapaths_map, &lb_dps->hmap_node);
> ovn_lb_datapaths_destroy(lb_dps);
> }
>
> + /* Create the 'lb_dps' if not already created for each
> + * 'lb' in the trk_lb_data->crupdated_lbs. */
> struct crupdated_lb *clb;
> HMAP_FOR_EACH (clb, hmap_node, &trk_lb_data->crupdated_lbs) {
> lb = clb->lb;
> @@ -5504,6 +5531,37 @@ northd_handle_lb_data_changes(struct
tracked_lb_data *trk_lb_data,
> }
> }
>
> + struct crupdated_od_lb_data *codlb;
> + LIST_FOR_EACH (codlb, list_node, &trk_lb_data->crupdated_ls_lbs) {
> + od = ovn_datapath_find(&ls_datapaths->datapaths,
&codlb->od_uuid);
> + ovs_assert(od);
> +
> + struct uuidset_node *uuidnode;
> + UUIDSET_FOR_EACH (uuidnode, &codlb->assoc_lbs) {
> + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
&uuidnode->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);
> + }
> +
> + HMAP_FOR_EACH (clb, hmap_node, &trk_lb_data->crupdated_lbs) {
> + lb = clb->lb;
> + const struct uuid *lb_uuid = &lb->nlb->header_.uuid;
> +
> + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> + ovs_assert(lb_dps);
> + size_t index;
> + BITMAP_FOR_EACH_1 (index, ods_size(ls_datapaths),
> + lb_dps->nb_ls_map) {
> + od = ls_datapaths->array[index];
> + /* Re-evaluate 'od->has_lb_vip' */
> + init_lb_for_datapath(od);
To continue the discussion of one of my comments in v6:
> > > And I really didn't understand the last part. If there is any change
of
> > > the relationship between a LS and a LB, it should be reflected in
> > > crupdated_ls_lbs, and handled in the first loop. So what's the
purpose of
> > > the second loop?
> > I'm not very sure If I understood your comment. Please see the v7 and
> > let me know if you still have some comments.
So here the loop on trk_lb_data->crupdated_lbs is to re-evaluate
'od->has_lb_vip' for LS that have any LB association change, right? But for
my understanding the re-evaluation performed by the previous loop on
trk_lb_data->crupdated_ls_lbs should have covered all such LS, and the
current loop is unnecessary. Is there a case that the
init_lb_for_datapath(od) would not be performed to a datapath by the
previous loop but will be covered in this loop?
Thanks,
Han
> + }
> + }
> +
> return true;
> }
>
> @@ -16529,6 +16587,7 @@ bool lflow_handle_northd_ls_changes(struct
ovsdb_idl_txn *ovnsb_txn,
> struct hmap *lflows)
> {
> struct ls_change *ls_change;
> +
> LIST_FOR_EACH (ls_change, list_node, &ls_changes->updated) {
> const struct sbrec_multicast_group *sbmc_flood =
> mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp,
> diff --git a/northd/northd.h b/northd/northd.h
> index 2bb6910945..0d206a4e52 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -115,6 +115,8 @@ struct northd_data {
> struct hmap svc_monitor_map;
> bool change_tracked;
> struct tracked_ls_changes tracked_ls_changes;
> + bool lb_changed; /* Indicates if load balancers changed or
association of
> + * load balancer to logical switch/router changed.
*/
> };
>
> struct lflow_data {
> @@ -345,7 +347,7 @@ bool northd_handle_lb_data_changes(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);
>
> 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 5c9da811fa..f0f8d1f503 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -10398,22 +10398,23 @@ ovn-nbctl lsp-set-type sw0-lr0 router
> ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01
> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> ovn-nbctl --wait=sb lsp-set-options sw0-lr0 router-port=lr0-sw0
> -check_engine_stats lb_data norecompute nocompute
> +check_engine_stats lb_data norecompute compute
> check_engine_stats northd recompute nocompute
> check_engine_stats lflow recompute nocompute
>
> -# Associate lb1 to sw0. There should be a full recompute of northd
engine node
> +# Associate lb1 to sw0. There should be no recompute of northd engine
node
> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> check ovn-nbctl --wait=sb ls-lb-add sw0 lb1
> -check_engine_stats lb_data norecompute nocompute
> -check_engine_stats northd recompute nocompute
> +check_engine_stats lb_data norecompute compute
> +check_engine_stats northd norecompute compute
> check_engine_stats lflow recompute nocompute
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
> # 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
>
> @@ -10421,7 +10422,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
>
> @@ -10429,7 +10430,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
>
> @@ -10437,14 +10438,33 @@ 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
>
> # Disassociate lb1 from sw0. There should be a full recompute of northd
engine node.
> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> check ovn-nbctl --wait=sb ls-lb-del sw0 lb1
> -check_engine_stats lb_data norecompute nocompute
> +check_engine_stats lb_data norecompute compute
> +check_engine_stats northd recompute nocompute
> +check_engine_stats lflow recompute nocompute
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +# Associate lb1 to sw0 and also create a port sw0p1. This should not
result in
> +# full recompute of northd, but should rsult in full recompute of lflow
node.
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +check ovn-nbctl --wait=sb ls-lb-add sw0 lb1 -- lsp-add sw0 sw0p1
> +check_engine_stats lb_data norecompute compute
> +check_engine_stats northd norecompute compute
> +check_engine_stats lflow recompute nocompute
> +
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +
> +# Disassociate lb1 from sw0. There should be a recompute of northd
engine node.
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +check ovn-nbctl --wait=sb ls-lb-del sw0 lb1
> +check_engine_stats lb_data norecompute compute
> check_engine_stats northd recompute nocompute
> check_engine_stats lflow recompute nocompute
> CHECK_NO_CHANGE_AFTER_RECOMPUTE
> @@ -10532,7 +10552,7 @@ 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 nocompute
> +check_engine_stats lb_data norecompute compute
> check_engine_stats northd recompute nocompute
> check_engine_stats lflow recompute nocompute
>
> @@ -10577,7 +10597,7 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> check ovn-nbctl clear logical_switch sw0 load_balancer_group
> -check_engine_stats lb_data norecompute nocompute
> +check_engine_stats lb_data norecompute compute
> check_engine_stats northd recompute nocompute
> check_engine_stats lflow recompute nocompute
>
> @@ -10629,7 +10649,7 @@ check_engine_stats lflow recompute nocompute
> # Add back lb group to logical switch and then delete it.
> 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 nocompute
> +check_engine_stats lb_data norecompute compute
> check_engine_stats northd recompute nocompute
> check_engine_stats lflow recompute nocompute
>
> @@ -10670,7 +10690,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 nocompute
> +check_engine_stats lb_data norecompute compute
> check_engine_stats northd recompute nocompute
> check_engine_stats lflow recompute nocompute
> CHECK_NO_CHANGE_AFTER_RECOMPUTE
> @@ -10684,15 +10704,15 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> check ovn-nbctl --wait=sb ls-lb-add sw0 lb2
> -check_engine_stats lb_data norecompute nocompute
> -check_engine_stats northd recompute nocompute
> +check_engine_stats lb_data norecompute compute
> +check_engine_stats northd norecompute compute
> check_engine_stats lflow recompute nocompute
> CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> check ovn-nbctl --wait=sb ls-lb-add sw0 lb3
> -check_engine_stats lb_data norecompute nocompute
> -check_engine_stats northd recompute nocompute
> +check_engine_stats lb_data norecompute compute
> +check_engine_stats northd norecompute compute
> check_engine_stats lflow recompute nocompute
> CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
> @@ -10706,7 +10726,7 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> check ovn-nbctl --wait=sb ls-lb-del sw0 lb2
> -check_engine_stats lb_data norecompute nocompute
> +check_engine_stats lb_data norecompute compute
> check_engine_stats northd recompute nocompute
> check_engine_stats lflow recompute nocompute
> CHECK_NO_CHANGE_AFTER_RECOMPUTE
> --
> 2.41.0
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
