[ovs-dev,v2,3/8] northd: Add initial I-P for load balancer and load balancer groups

From: Numan Siddique <numans@ovn.org>

Any changes to load balancers and load balancer groups
are handled incrementally in the newly added 'northd_lb_data'
engine node.  'northd_lb_data' is input to 'northd' node
and the handler - northd_northd_lb_data_handler in 'northd'
node handles the changes.

If a load balancer or load balancer group is associated to
a logical switch or router then 'northd' node falls back
to full recompute.

Signed-off-by: Numan Siddique <numans@ovn.org>
---
 lib/lb.c                   |  85 ++++++++++++----
 lib/lb.h                   |   9 ++
 northd/en-northd-lb-data.c | 202 +++++++++++++++++++++++++++++++++++--
 northd/en-northd-lb-data.h |  37 +++++++
 northd/en-northd.c         |  24 +++++
 northd/en-northd.h         |   1 +
 northd/inc-proc-northd.c   |  13 ++-
 northd/northd.c            |  77 ++++++++++++++
 northd/northd.h            |   7 ++
 tests/ovn-northd.at        | 123 ++++++++++++++++++++++
 10 files changed, 545 insertions(+), 33 deletions(-)

Hi Numan,

I have one small nit below.

On 7/7/23 01:53, numans@ovn.org wrote:
> From: Numan Siddique <numans@ovn.org>
> 
> Any changes to load balancers and load balancer groups
> are handled incrementally in the newly added 'northd_lb_data'
> engine node.  'northd_lb_data' is input to 'northd' node
> and the handler - northd_northd_lb_data_handler in 'northd'
> node handles the changes.
> 
> If a load balancer or load balancer group is associated to
> a logical switch or router then 'northd' node falls back
> to full recompute.
> 
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>   lib/lb.c                   |  85 ++++++++++++----
>   lib/lb.h                   |   9 ++
>   northd/en-northd-lb-data.c | 202 +++++++++++++++++++++++++++++++++++--
>   northd/en-northd-lb-data.h |  37 +++++++
>   northd/en-northd.c         |  24 +++++
>   northd/en-northd.h         |   1 +
>   northd/inc-proc-northd.c   |  13 ++-
>   northd/northd.c            |  77 ++++++++++++++
>   northd/northd.h            |   7 ++
>   tests/ovn-northd.at        | 123 ++++++++++++++++++++++
>   10 files changed, 545 insertions(+), 33 deletions(-)
> 
> diff --git a/lib/lb.c b/lib/lb.c
> index 429dbf15af..a51fe225fa 100644
> --- a/lib/lb.c
> +++ b/lib/lb.c
> @@ -606,13 +606,13 @@ ovn_lb_get_health_check(const struct nbrec_load_balancer *nbrec_lb,
>       return NULL;
>   }
>   
> -struct ovn_northd_lb *
> -ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
> +static void
> +ovn_northd_lb_init(struct ovn_northd_lb *lb,
> +                   const struct nbrec_load_balancer *nbrec_lb)
>   {
>       bool template = smap_get_bool(&nbrec_lb->options, "template", false);
>       bool is_udp = nullable_string_is_equal(nbrec_lb->protocol, "udp");
>       bool is_sctp = nullable_string_is_equal(nbrec_lb->protocol, "sctp");
> -    struct ovn_northd_lb *lb = xzalloc(sizeof *lb);
>       int address_family = !strcmp(smap_get_def(&nbrec_lb->options,
>                                                 "address-family", "ipv4"),
>                                    "ipv4")
> @@ -668,6 +668,10 @@ ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
>                                                     "reject", false);
>           ovn_northd_lb_vip_init(lb_vip_nb, lb_vip, nbrec_lb,
>                                  node->key, node->value, template);
> +        if (lb_vip_nb->lb_health_check) {
> +            lb->health_checks = true;
> +        }
> +
>           if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
>               sset_add(&lb->ips_v4, lb_vip->vip_str);
>           } else {
> @@ -713,6 +717,13 @@ ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
>           ds_chomp(&sel_fields, ',');
>           lb->selection_fields = ds_steal_cstr(&sel_fields);
>       }
> +}
> +
> +struct ovn_northd_lb *
> +ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
> +{
> +    struct ovn_northd_lb *lb = xzalloc(sizeof *lb);
> +    ovn_northd_lb_init(lb, nbrec_lb);
>       return lb;
>   }
>   
> @@ -738,8 +749,8 @@ ovn_northd_lb_get_vips(const struct ovn_northd_lb *lb)
>       return &lb->nlb->vips;
>   }
>   
> -void
> -ovn_northd_lb_destroy(struct ovn_northd_lb *lb)
> +static void
> +ovn_northd_lb_cleanup(struct ovn_northd_lb *lb)
>   {
>       for (size_t i = 0; i < lb->n_vips; i++) {
>           ovn_lb_vip_destroy(&lb->vips[i]);
> @@ -747,26 +758,36 @@ ovn_northd_lb_destroy(struct ovn_northd_lb *lb)
>       }
>       free(lb->vips);
>       free(lb->vips_nb);
> +    lb->vips = NULL;
> +    lb->vips_nb = NULL;
>       smap_destroy(&lb->template_vips);
>       sset_destroy(&lb->ips_v4);
>       sset_destroy(&lb->ips_v6);
>       free(lb->selection_fields);
> +    lb->selection_fields = NULL;
> +    lb->health_checks = false;
> +}
> +
> +void
> +ovn_northd_lb_destroy(struct ovn_northd_lb *lb)
> +{
> +    ovn_northd_lb_cleanup(lb);
>       free(lb);
>   }
>   
> -/* Constructs a new 'struct ovn_lb_group' object from the Nb LB Group record
> - * and a hash map of all existing 'struct ovn_northd_lb' objects.  Space will
> - * be allocated for 'max_ls_datapaths' logical switches and 'max_lr_datapaths'
> - * logical routers to which this LB Group is applied.  Can be filled later
> - * with ovn_lb_group_add_ls() and ovn_lb_group_add_lr() respectively. */
> -struct ovn_lb_group *
> -ovn_lb_group_create(const struct nbrec_load_balancer_group *nbrec_lb_group,
> -                    const struct hmap *lbs)
> +void
> +ovn_northd_lb_reinit(struct ovn_northd_lb *lb,
> +                     const struct nbrec_load_balancer *nbrec_lb)
>   {
> -    struct ovn_lb_group *lb_group;
> +    ovn_northd_lb_cleanup(lb);
> +    ovn_northd_lb_init(lb, nbrec_lb);
> +}
>   
> -    lb_group = xzalloc(sizeof *lb_group);
> -    lb_group->uuid = nbrec_lb_group->header_.uuid;
> +static void
> +ovn_lb_group_init(struct ovn_lb_group *lb_group,
> +                  const struct nbrec_load_balancer_group *nbrec_lb_group,
> +                  const struct hmap *lbs)
> +{
>       lb_group->n_lbs = nbrec_lb_group->n_load_balancer;
>       lb_group->lbs = xmalloc(lb_group->n_lbs * sizeof *lb_group->lbs);
>       lb_group->lb_ips = ovn_lb_ip_set_create();
> @@ -776,10 +797,30 @@ ovn_lb_group_create(const struct nbrec_load_balancer_group *nbrec_lb_group,
>               &nbrec_lb_group->load_balancer[i]->header_.uuid;
>           lb_group->lbs[i] = ovn_northd_lb_find(lbs, lb_uuid);
>       }
> +}
>   
> +/* Constructs a new 'struct ovn_lb_group' object from the Nb LB Group record
> + * and a hash map of all existing 'struct ovn_northd_lb' objects.  Space will
> + * be allocated for 'max_ls_datapaths' logical switches and 'max_lr_datapaths'
> + * logical routers to which this LB Group is applied.  Can be filled later
> + * with ovn_lb_group_add_ls() and ovn_lb_group_add_lr() respectively. */
> +struct ovn_lb_group *
> +ovn_lb_group_create(const struct nbrec_load_balancer_group *nbrec_lb_group,
> +                    const struct hmap *lbs)
> +{
> +    struct ovn_lb_group *lb_group = xzalloc(sizeof *lb_group);
> +    lb_group->uuid = nbrec_lb_group->header_.uuid;
> +    ovn_lb_group_init(lb_group, nbrec_lb_group, lbs);
>       return lb_group;
>   }
>   
> +static void
> +ovn_lb_group_cleanup(struct ovn_lb_group *lb_group)
> +{
> +    ovn_lb_ip_set_destroy(lb_group->lb_ips);
> +    free(lb_group->lbs);
> +}
> +
>   void
>   ovn_lb_group_destroy(struct ovn_lb_group *lb_group)
>   {
> @@ -787,11 +828,19 @@ ovn_lb_group_destroy(struct ovn_lb_group *lb_group)
>           return;
>       }
>   
> -    ovn_lb_ip_set_destroy(lb_group->lb_ips);
> -    free(lb_group->lbs);
> +    ovn_lb_group_cleanup(lb_group);
>       free(lb_group);
>   }
>   
> +void
> +ovn_lb_group_reinit(struct ovn_lb_group *lb_group,
> +                    const struct nbrec_load_balancer_group *nbrec_lb_group,
> +                    const struct hmap *lbs)
> +{
> +    ovn_lb_group_cleanup(lb_group);
> +    ovn_lb_group_init(lb_group, nbrec_lb_group, lbs);
> +}
> +
>   struct ovn_lb_group *
>   ovn_lb_group_find(const struct hmap *lb_groups, const struct uuid *uuid)
>   {
> diff --git a/lib/lb.h b/lib/lb.h
> index 0339050cba..74905c73b7 100644
> --- a/lib/lb.h
> +++ b/lib/lb.h
> @@ -77,6 +77,9 @@ struct ovn_northd_lb {
>   
>       struct sset ips_v4;
>       struct sset ips_v6;
> +
> +    /* Indicates if the load balancer has health checks configured. */
> +    bool health_checks;
>   };
>   
>   struct ovn_lb_vip {
> @@ -130,6 +133,8 @@ struct ovn_northd_lb *ovn_northd_lb_find(const struct hmap *,
>                                            const struct uuid *);
>   const struct smap *ovn_northd_lb_get_vips(const struct ovn_northd_lb *);
>   void ovn_northd_lb_destroy(struct ovn_northd_lb *);
> +void ovn_northd_lb_reinit(struct ovn_northd_lb *,
> +                          const struct nbrec_load_balancer *);
>   
>   void build_lrouter_lb_ips(struct ovn_lb_ip_set *,
>                             const struct ovn_northd_lb *);
> @@ -148,6 +153,10 @@ struct ovn_lb_group *ovn_lb_group_create(
>   void ovn_lb_group_destroy(struct ovn_lb_group *lb_group);
>   struct ovn_lb_group *ovn_lb_group_find(const struct hmap *lb_groups,
>                                          const struct uuid *);
> +void ovn_lb_group_reinit(
> +    struct ovn_lb_group *lb_group,
> +    const struct nbrec_load_balancer_group *,
> +    const struct hmap *lbs);
>   
>   struct ovn_lb_datapaths {
>       struct hmap_node hmap_node;
> diff --git a/northd/en-northd-lb-data.c b/northd/en-northd-lb-data.c
> index d46c3c27ed..e52535b4a9 100644
> --- a/northd/en-northd-lb-data.c
> +++ b/northd/en-northd-lb-data.c
> @@ -37,6 +37,15 @@ static void northd_lb_data_destroy(struct northd_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 struct ovn_lb_group *create_lb_group(
> +    const struct nbrec_load_balancer_group *, struct hmap *lbs,
> +    struct hmap *lb_groups);
> +static void destroy_tracked_data(struct northd_lb_data *);
> +static void add_lb_to_tracked_data(struct ovn_northd_lb *,
> +                                   struct ovs_list *tracked_list,
> +                                   bool health_checks);
> +static void add_lb_group_to_tracked_data(struct ovn_lb_group *,
> +                                         struct ovs_list *tracked_list);
>   
>   void *
>   en_northd_lb_data_init(struct engine_node *node OVS_UNUSED,
> @@ -61,6 +70,7 @@ en_northd_lb_data_run(struct engine_node *node, void *data)
>       const struct nbrec_load_balancer_group_table *nb_lbg_table =
>           EN_OVSDB_GET(engine_get_input("NB_load_balancer_group", node));
>   
> +    lb_data->tracked = false;
>       build_lbs(nb_lb_table, nb_lbg_table, &lb_data->lbs, &lb_data->lb_groups);
>       engine_set_node_state(node, EN_UPDATED);
>   }
> @@ -72,12 +82,122 @@ en_northd_lb_data_cleanup(void *data)
>       northd_lb_data_destroy(lb_data);
>   }
>   
> +void
> +en_northd_lb_data_clear_tracked_data(void *data)
> +{
> +    struct northd_lb_data *lb_data = (struct northd_lb_data *) data;
> +    destroy_tracked_data(lb_data);
> +}
> +
> +
> +/* Handler functions. */
> +bool
> +northd_lb_data_load_balancer_handler(struct engine_node *node,
> +                                     void *data OVS_UNUSED)
> +{
> +    const struct nbrec_load_balancer_table *nb_lb_table =
> +        EN_OVSDB_GET(engine_get_input("NB_load_balancer", node));
> +
> +    struct northd_lb_data *lb_data = (struct northd_lb_data *) data;
> +
> +    lb_data->tracked = true;
> +    struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> +
> +    const struct nbrec_load_balancer *tracked_lb;
> +    NBREC_LOAD_BALANCER_TABLE_FOR_EACH_TRACKED (tracked_lb, nb_lb_table) {
> +        struct ovn_northd_lb *lb;
> +        if (nbrec_load_balancer_is_new(tracked_lb)) {
> +            /* New load balancer. */
> +            lb = ovn_northd_lb_create(tracked_lb);
> +            hmap_insert(&lb_data->lbs, &lb->hmap_node,
> +                        uuid_hash(&tracked_lb->header_.uuid));
> +            add_lb_to_tracked_data(
> +                lb, &trk_lb_data->tracked_updated_lbs.updated,
> +                lb->health_checks);
> +        } else if (nbrec_load_balancer_is_deleted(tracked_lb)) {
> +            lb = ovn_northd_lb_find(&lb_data->lbs,
> +                                    &tracked_lb->header_.uuid);
> +            ovs_assert(lb);
> +            hmap_remove(&lb_data->lbs, &lb->hmap_node);
> +            add_lb_to_tracked_data(
> +                lb, &trk_lb_data->tracked_deleted_lbs.updated,
> +                lb->health_checks);
> +        } else {
> +            /* Load balancer updated. */
> +            lb = ovn_northd_lb_find(&lb_data->lbs,
> +                                    &tracked_lb->header_.uuid);
> +            ovs_assert(lb);
> +            bool health_checks = lb->health_checks;
> +            ovn_northd_lb_reinit(lb, tracked_lb);
> +            health_checks |= lb->health_checks;
> +            add_lb_to_tracked_data(
> +                lb, &trk_lb_data->tracked_updated_lbs.updated, health_checks);
> +        }
> +    }
> +
> +    engine_set_node_state(node, EN_UPDATED);
> +    return true;
> +}
> +
> +bool
> +northd_lb_data_load_balancer_group_handler(struct engine_node *node,
> +                                           void *data OVS_UNUSED)
> +{
> +    struct northd_lb_data *lb_data = (struct northd_lb_data *) data;
> +    const struct nbrec_load_balancer_group_table *nb_lbg_table =
> +        EN_OVSDB_GET(engine_get_input("NB_load_balancer_group", node));
> +
> +    lb_data->tracked = true;
> +    struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> +    const struct nbrec_load_balancer_group *tracked_lb_group;
> +    NBREC_LOAD_BALANCER_GROUP_TABLE_FOR_EACH_TRACKED (tracked_lb_group,
> +                                                      nb_lbg_table) {
> +        if (nbrec_load_balancer_group_is_new(tracked_lb_group)) {
> +            struct ovn_lb_group *lb_group =
> +                create_lb_group(tracked_lb_group, &lb_data->lbs,
> +                                &lb_data->lb_groups);
> +            add_lb_group_to_tracked_data(
> +                lb_group, &trk_lb_data->tracked_updated_lb_groups.updated);
> +        } else if (nbrec_load_balancer_group_is_deleted(tracked_lb_group)) {
> +            struct ovn_lb_group *lb_group;
> +            lb_group = ovn_lb_group_find(&lb_data->lb_groups,
> +                                         &tracked_lb_group->header_.uuid);
> +            ovs_assert(lb_group);
> +            hmap_remove(&lb_data->lb_groups, &lb_group->hmap_node);
> +            add_lb_group_to_tracked_data(
> +                lb_group, &trk_lb_data->tracked_deleted_lb_groups.updated);
> +        } else if (nbrec_load_balancer_group_is_updated(tracked_lb_group,
> +                                NBREC_LOAD_BALANCER_GROUP_COL_LOAD_BALANCER)) {
> +
> +            struct ovn_lb_group *lb_group;
> +            lb_group = ovn_lb_group_find(&lb_data->lb_groups,
> +                                         &tracked_lb_group->header_.uuid);
> +            ovs_assert(lb_group);
> +            ovn_lb_group_reinit(lb_group, tracked_lb_group, &lb_data->lbs);
> +            for (size_t i = 0; i < lb_group->n_lbs; i++) {
> +                build_lrouter_lb_ips(lb_group->lb_ips, lb_group->lbs[i]);
> +            }
> +            add_lb_group_to_tracked_data(
> +                lb_group, &trk_lb_data->tracked_updated_lb_groups.updated);
> +        }
> +    }
> +
> +    engine_set_node_state(node, EN_UPDATED);
> +    return true;
> +}
> +
>   /* static functions. */
>   static void
>   northd_lb_data_init(struct northd_lb_data *lb_data)
>   {
>       hmap_init(&lb_data->lbs);
>       hmap_init(&lb_data->lb_groups);
> +
> +    struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> +    ovs_list_init(&trk_lb_data->tracked_updated_lbs.updated);
> +    ovs_list_init(&trk_lb_data->tracked_deleted_lbs.updated);
> +    ovs_list_init(&trk_lb_data->tracked_updated_lb_groups.updated);
> +    ovs_list_init(&trk_lb_data->tracked_deleted_lb_groups.updated);
>   }
>   
>   static void
> @@ -94,6 +214,8 @@ northd_lb_data_destroy(struct northd_lb_data *lb_data)
>           ovn_lb_group_destroy(lb_group);
>       }
>       hmap_destroy(&lb_data->lb_groups);
> +
> +    destroy_tracked_data(lb_data);
>   }
>   
>   static void
> @@ -101,12 +223,9 @@ build_lbs(const struct nbrec_load_balancer_table *nbrec_load_balancer_table,
>             const struct nbrec_load_balancer_group_table *nbrec_lb_group_table,
>             struct hmap *lbs, struct hmap *lb_groups)
>   {
> -    struct ovn_lb_group *lb_group;
> -    struct ovn_northd_lb *lb_nb;
> -
>       const struct nbrec_load_balancer *nbrec_lb;
>       NBREC_LOAD_BALANCER_TABLE_FOR_EACH (nbrec_lb, nbrec_load_balancer_table) {
> -        lb_nb = ovn_northd_lb_create(nbrec_lb);
> +        struct ovn_northd_lb *lb_nb = ovn_northd_lb_create(nbrec_lb);
>           hmap_insert(lbs, &lb_nb->hmap_node,
>                       uuid_hash(&nbrec_lb->header_.uuid));
>       }
> @@ -114,13 +233,76 @@ build_lbs(const struct nbrec_load_balancer_table *nbrec_load_balancer_table,
>       const struct nbrec_load_balancer_group *nbrec_lb_group;
>       NBREC_LOAD_BALANCER_GROUP_TABLE_FOR_EACH (nbrec_lb_group,
>                                                 nbrec_lb_group_table) {
> -        lb_group = ovn_lb_group_create(nbrec_lb_group, lbs);
> +        create_lb_group(nbrec_lb_group, lbs, lb_groups);
> +    }
> +}
>   
> -        for (size_t i = 0; i < lb_group->n_lbs; i++) {
> -            build_lrouter_lb_ips(lb_group->lb_ips, lb_group->lbs[i]);
> -        }
> +static struct ovn_lb_group *
> +create_lb_group(const struct nbrec_load_balancer_group *nbrec_lb_group,
> +                struct hmap *lbs, struct hmap *lb_groups)
> +{
> +    struct ovn_lb_group *lb_group = ovn_lb_group_create(nbrec_lb_group, lbs);
> +
> +    for (size_t i = 0; i < lb_group->n_lbs; i++) {
> +        build_lrouter_lb_ips(lb_group->lb_ips, lb_group->lbs[i]);
> +    }
> +
> +    hmap_insert(lb_groups, &lb_group->hmap_node,
> +                uuid_hash(&lb_group->uuid));
> +
> +    return lb_group;
> +}
> +
> +static void
> +destroy_tracked_data(struct northd_lb_data *lb_data)
> +{
> +    lb_data->tracked = false;
> +
> +    struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> +    struct tracked_lb *tracked_lb;
> +    LIST_FOR_EACH_SAFE (tracked_lb, list_node,
> +                        &trk_lb_data->tracked_updated_lbs.updated) {
> +        ovs_list_remove(&tracked_lb->list_node);
> +        free(tracked_lb);
> +    }
> +
> +    LIST_FOR_EACH_SAFE (tracked_lb, list_node,
> +                        &trk_lb_data->tracked_deleted_lbs.updated) {
> +        ovs_list_remove(&tracked_lb->list_node);
> +        ovn_northd_lb_destroy(tracked_lb->lb);
> +        free(tracked_lb);
> +    }
> +
> +    struct tracked_lb_group *tracked_lb_group;
> +    LIST_FOR_EACH_SAFE (tracked_lb_group, list_node,
> +                        &trk_lb_data->tracked_updated_lb_groups.updated) {
> +        ovs_list_remove(&tracked_lb_group->list_node);
> +        free(tracked_lb_group);
> +    }
>   
> -        hmap_insert(lb_groups, &lb_group->hmap_node,
> -                    uuid_hash(&lb_group->uuid));
> +    LIST_FOR_EACH_SAFE (tracked_lb_group, list_node,
> +                        &trk_lb_data->tracked_deleted_lb_groups.updated) {
> +        ovs_list_remove(&tracked_lb_group->list_node);
> +        ovn_lb_group_destroy(tracked_lb_group->lb_group);
> +        free(tracked_lb_group);
>       }
>   }
> +
> +static void
> +add_lb_to_tracked_data(struct ovn_northd_lb *lb, struct ovs_list *tracked_list,
> +                       bool health_checks)
> +{
> +    struct tracked_lb *t_lb = xzalloc(sizeof *t_lb);
> +    t_lb->lb = lb;
> +    t_lb->health_checks = health_checks;
> +    ovs_list_push_back(tracked_list, &t_lb->list_node);
> +}
> +
> +static void
> +add_lb_group_to_tracked_data(struct ovn_lb_group *lb_group,
> +                             struct ovs_list *tracked_list)
> +{
> +    struct tracked_lb_group *t_lb_group = xzalloc(sizeof *t_lb_group);
> +    t_lb_group->lb_group = lb_group;
> +    ovs_list_push_back(tracked_list, &t_lb_group->list_node);
> +}
> diff --git a/northd/en-northd-lb-data.h b/northd/en-northd-lb-data.h
> index eb297e376d..bb07fcf686 100644
> --- a/northd/en-northd-lb-data.h
> +++ b/northd/en-northd-lb-data.h
> @@ -4,16 +4,53 @@
>   #include <config.h>
>   
>   #include "openvswitch/hmap.h"
> +#include "include/openvswitch/list.h"
>   
>   #include "lib/inc-proc-eng.h"
>   
> +struct ovn_northd_lb;
> +struct ovn_lb_group;
> +
> +struct tracked_lb {
> +    struct ovs_list list_node;
> +    struct ovn_northd_lb *lb;
> +    bool health_checks;
> +};
> +
> +struct tracked_lb_group {
> +    struct ovs_list list_node;
> +    struct ovn_lb_group *lb_group;
> +};
> +
> +struct tracked_lb_changes {
> +    struct ovs_list updated; /* contains list of 'struct tracked_lb ' or
> +                                list of 'struct tracked_lb_group' */
> +};
> +
> +struct tracked_lb_data {
> +    struct tracked_lb_changes tracked_updated_lbs;
> +    struct tracked_lb_changes tracked_deleted_lbs;
> +    struct tracked_lb_changes tracked_updated_lb_groups;
> +    struct tracked_lb_changes tracked_deleted_lb_groups;
> +};

The typing here is a bit odd. The problem is that struct 
tracked_lb_changes is being used for two distinct types of lists. I 
think you should do one of two things here.

1) Get rid of the trakced_lb_changes struct and use ovs_list in-line.

struct tracked_lb_data {
     struct ovs_list tracked_updated_lbs;
     struct ovs_list tracked_deleted_lbs;
     struct ovs_list tracked_updated_lb_groups;
     struct ovs_list tracked_deleted_lb_groups;
};

2) Make a separate type for tracked lbs and tracked lb groups.

struct tracked_lb_changes {
     struct ovs_list updated;
};

struct tracked_lb_group_changes {
     struct ovs_list updated;
};

struct tracked_lb_data {
     struct tracked_lb_changes tracked_updated_lbs;
     struct tracked_lb_changes tracked_deleted_lbs;
     struct tracked_lb_group_changes tracked_updated_lb_groups;
     struct tracked_lb_group_changes tracked_deleted_lb_groups;
};

I think option (2) is my preference.

> +
>   struct northd_lb_data {
>       struct hmap lbs;
>       struct hmap lb_groups;
> +
> +    /* tracked data*/
> +    bool tracked;
> +    struct tracked_lb_data tracked_lb_data;
>   };
>   
>   void *en_northd_lb_data_init(struct engine_node *, struct engine_arg *);
>   void en_northd_lb_data_run(struct engine_node *, void *data);
>   void en_northd_lb_data_cleanup(void *data);
> +void en_northd_lb_data_clear_tracked_data(void *data);
> +
> +bool northd_lb_data_load_balancer_handler(struct engine_node *,
> +                                          void *data);
> +bool northd_lb_data_load_balancer_group_handler(struct engine_node *,
> +                                                void *data);
>   
>   #endif /* end of EN_NORTHD_LB_DATA_H */
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index cc7d838451..b3c03c54bd 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -206,6 +206,30 @@ northd_sb_port_binding_handler(struct engine_node *node,
>       return true;
>   }
>   
> +bool
> +northd_northd_lb_data_handler(struct engine_node *node, void *data)
> +{
> +    struct northd_lb_data *lb_data =
> +        engine_get_input_data("northd_lb_data", node);
> +
> +    if (!lb_data->tracked) {
> +        return false;
> +    }
> +
> +    struct northd_data *nd = data;
> +
> +    if (!northd_handle_lb_data_changes(&lb_data->tracked_lb_data,
> +                                       &nd->ls_datapaths,
> +                                       &nd->lr_datapaths,
> +                                       &nd->lb_datapaths_map,
> +                                       &nd->lb_group_datapaths_map)) {
> +        return false;
> +    }
> +
> +    engine_set_node_state(node, EN_UPDATED);
> +    return true;
> +}
> +
>   void
>   *en_northd_init(struct engine_node *node OVS_UNUSED,
>                   struct engine_arg *arg OVS_UNUSED)
> diff --git a/northd/en-northd.h b/northd/en-northd.h
> index 20cc77f108..5674f4390c 100644
> --- a/northd/en-northd.h
> +++ b/northd/en-northd.h
> @@ -17,5 +17,6 @@ void en_northd_clear_tracked_data(void *data);
>   bool northd_nb_nb_global_handler(struct engine_node *, void *data OVS_UNUSED);
>   bool northd_nb_logical_switch_handler(struct engine_node *, void *data);
>   bool northd_sb_port_binding_handler(struct engine_node *, void *data);
> +bool northd_northd_lb_data_handler(struct engine_node *, void *data);
>   
>   #endif /* EN_NORTHD_H */
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> index b2e884962f..b444a488db 100644
> --- a/northd/inc-proc-northd.c
> +++ b/northd/inc-proc-northd.c
> @@ -141,13 +141,18 @@ static ENGINE_NODE(sync_to_sb_addr_set, "sync_to_sb_addr_set");
>   static ENGINE_NODE(fdb_aging, "fdb_aging");
>   static ENGINE_NODE(fdb_aging_waker, "fdb_aging_waker");
>   static ENGINE_NODE(sync_to_sb_lb, "sync_to_sb_lb");
> -static ENGINE_NODE(northd_lb_data, "northd_lb_data");
> +static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(northd_lb_data, "northd_lb_data");
>   
>   void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>                             struct ovsdb_idl_loop *sb)
>   {
>       /* Define relationships between nodes where first argument is dependent
>        * on the second argument */
> +    engine_add_input(&en_northd_lb_data, &en_nb_load_balancer,
> +                     northd_lb_data_load_balancer_handler);
> +    engine_add_input(&en_northd_lb_data, &en_nb_load_balancer_group,
> +                     northd_lb_data_load_balancer_group_handler);
> +
>       engine_add_input(&en_northd, &en_nb_port_group, NULL);
>       engine_add_input(&en_northd, &en_nb_acl, NULL);
>       engine_add_input(&en_northd, &en_nb_logical_router, NULL);
> @@ -171,6 +176,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>       engine_add_input(&en_northd, &en_sb_static_mac_binding, NULL);
>       engine_add_input(&en_northd, &en_sb_chassis_template_var, NULL);
>   
> +    engine_add_input(&en_northd, &en_northd_lb_data,
> +                     northd_northd_lb_data_handler);
>       engine_add_input(&en_northd, &en_sb_port_binding,
>                        northd_sb_port_binding_handler);
>       engine_add_input(&en_northd, &en_nb_nb_global,
> @@ -178,10 +185,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>       engine_add_input(&en_northd, &en_nb_logical_switch,
>                        northd_nb_logical_switch_handler);
>   
> -    engine_add_input(&en_northd_lb_data, &en_nb_load_balancer, NULL);
> -    engine_add_input(&en_northd_lb_data, &en_nb_load_balancer_group, NULL);
> -    engine_add_input(&en_northd, &en_northd_lb_data, NULL);
> -
>       engine_add_input(&en_mac_binding_aging, &en_nb_nb_global, NULL);
>       engine_add_input(&en_mac_binding_aging, &en_sb_mac_binding, NULL);
>       engine_add_input(&en_mac_binding_aging, &en_northd, NULL);
> diff --git a/northd/northd.c b/northd/northd.c
> index 890186b29c..f27f0de49b 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -40,6 +40,7 @@
>   #include "lib/lb.h"
>   #include "memory.h"
>   #include "northd.h"
> +#include "en-northd-lb-data.h"
>   #include "lib/ovn-parallel-hmap.h"
>   #include "ovn/actions.h"
>   #include "ovn/features.h"
> @@ -5323,6 +5324,82 @@ northd_handle_sb_port_binding_changes(
>       return true;
>   }
>   
> +bool
> +northd_handle_lb_data_changes(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 tracked_lb *trk_lb;
> +
> +    struct ovn_lb_datapaths *lb_dps;
> +    LIST_FOR_EACH (trk_lb, list_node,
> +                   &trk_lb_data->tracked_deleted_lbs.updated) {
> +        if (trk_lb->health_checks) {
> +            /* Fall back to recompute if the deleted load balancer has
> +             * health checks configured. */
> +            return false;
> +        }
> +
> +        const struct uuid *lb_uuid =
> +                &trk_lb->lb->nlb->header_.uuid;
> +
> +        lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> +        ovs_assert(lb_dps);
> +        hmap_remove(lb_datapaths_map, &lb_dps->hmap_node);
> +        ovn_lb_datapaths_destroy(lb_dps);
> +    }
> +
> +    LIST_FOR_EACH (trk_lb, list_node,
> +                   &trk_lb_data->tracked_updated_lbs.updated) {
> +        if (trk_lb->health_checks) {
> +            /* Fall back to recompute if the created/updated load balancer has
> +             * health checks configured. */
> +            return false;
> +        }
> +
> +        const struct uuid *lb_uuid =
> +                &trk_lb->lb->nlb->header_.uuid;
> +        lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> +        if (!lb_dps) {
> +            lb_dps = ovn_lb_datapaths_create(trk_lb->lb,
> +                                             ods_size(ls_datapaths),
> +                                             ods_size(lr_datapaths));
> +            hmap_insert(lb_datapaths_map, &lb_dps->hmap_node,
> +                        uuid_hash(lb_uuid));
> +        }
> +    }
> +
> +    struct ovn_lb_group_datapaths *lb_group_dps;
> +    struct tracked_lb_group *trk_lb_group;
> +    LIST_FOR_EACH_SAFE (trk_lb_group, list_node,
> +                        &trk_lb_data->tracked_deleted_lb_groups.updated) {
> +        const struct uuid *lb_uuid = &trk_lb_group->lb_group->uuid;
> +        lb_group_dps = ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> +                                                   lb_uuid);
> +        ovs_assert(lb_group_dps);
> +        hmap_remove(lb_group_datapaths_map, &lb_group_dps->hmap_node);
> +        ovn_lb_group_datapaths_destroy(lb_group_dps);
> +    }
> +
> +    LIST_FOR_EACH_SAFE (trk_lb_group, list_node,
> +                        &trk_lb_data->tracked_updated_lb_groups.updated) {
> +        const struct uuid *lb_uuid = &trk_lb_group->lb_group->uuid;
> +        lb_group_dps = ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> +                                                   lb_uuid);
> +        if (!lb_group_dps) {
> +            lb_group_dps = ovn_lb_group_datapaths_create(
> +                trk_lb_group->lb_group, ods_size(ls_datapaths),
> +                ods_size(lr_datapaths));
> +            hmap_insert(lb_group_datapaths_map, &lb_group_dps->hmap_node,
> +                        uuid_hash(lb_uuid));
> +        }
> +    }
> +
> +    return true;
> +}
> +
>   struct multicast_group {
>       const char *name;
>       uint16_t key;               /* OVN_MIN_MULTICAST...OVN_MAX_MULTICAST. */
> diff --git a/northd/northd.h b/northd/northd.h
> index 7d92028c7d..7d17921fa2 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -347,6 +347,13 @@ bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn,
>   bool northd_handle_sb_port_binding_changes(
>       const struct sbrec_port_binding_table *, struct hmap *ls_ports);
>   
> +struct tracked_lb_data;
> +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);
> +
>   void build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
>                        const struct nbrec_bfd_table *,
>                        const struct sbrec_bfd_table *,
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index e79d33b2ae..dd0bd8b36a 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -9681,3 +9681,126 @@ AT_CHECK([grep "lr_in_gw_redirect" R1flows |sed s'/table=../table=??/' |sort], [
>   
>   AT_CLEANUP
>   ])
> +
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([Load balancer incremental processing])
> +ovn_start
> +
> +check_engine_stats() {
> +  northd_comp=$1
> +  lflow_comp=$2
> +
> +  AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd_lb_data recompute], [0], [0
> +])
> +
> +  northd_recompute_ct=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute)
> +  if [[ "$northd_comp" == "norecompute" ]]; then
> +    check test "$northd_recompute_ct" -eq "0"
> +  else
> +    check test "$northd_recompute_ct" -ne "0"
> +  fi
> +
> +  lflow_recompute_ct=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow recompute)
> +  if [[ "$lflow_comp" == "norecompute" ]]; then
> +    check test "$lflow_recompute_ct" -eq "0"
> +  else
> +    check test "$lflow_recompute_ct" -ne "0"
> +  fi
> +}
> +
> +# Test I-P for load balancers.
> +# Presently ovn-northd handles I-P for NB LBs in northd_lb_data engine node
> +# only.
> +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 norecompute recompute
> +
> +check ovn-nbctl --wait=sb set load_balancer . ip_port_mappings:10.0.0.3=sw0-p1:10.0.0.2
> +check_engine_stats norecompute recompute
> +
> +check ovn-nbctl --wait=sb set load_balancer . options:foo=bar
> +check_engine_stats norecompute recompute
> +
> +check ovn-nbctl --wait=sb -- lb-add lb2 20.0.0.10:80 20.0.0.20:80 -- lb-add lb3 30.0.0.10:80 30.0.0.20:80
> +check_engine_stats norecompute recompute
> +
> +check ovn-nbctl --wait=sb -- lb-del lb2 -- lb-del lb3
> +check_engine_stats norecompute recompute
> +
> +AT_CHECK([ovn-nbctl --wait=sb \
> +          -- --id=@hc create Load_Balancer_Health_Check vip="10.0.0.10\:80" \
> +             options:failure_count=100 \
> +          -- add Load_Balancer . health_check @hc | uuidfilt], [0], [<0>
> +])
> +check_engine_stats recompute recompute
> +
> +# Any change to load balancer health check should also result in full recompute
> +# of northd node (but not northd_lb_data node)
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +check ovn-nbctl --wait=sb set load_balancer_health_check . options:foo=bar1
> +check_engine_stats recompute recompute
> +
> +# Delete the health check from the load balancer.  northd engine node should do a full recompute.
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +check ovn-nbctl --wait=sb clear Load_Balancer . health_check
> +check_engine_stats recompute recompute
> +
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +check ovn-nbctl ls-add sw0
> +check ovn-nbctl --wait=sb lr-add lr0
> +check_engine_stats recompute recompute
> +
> +# Associate lb1 to 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-add sw0 lb1
> +check_engine_stats recompute 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 recompute recompute
> +
> +# Test load balancer group now
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +lbg1_uuid=$(ovn-nbctl create load_balancer_group name=lbg1)
> +check_engine_stats norecompute recompute
> +
> +lb1_uuid=$(fetch_column nb:Load_Balancer _uuid)
> +
> +# Add lb to the lbg1 group
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +check ovn-nbctl add load_balancer_group . load_Balancer $lb1_uuid
> +check_engine_stats norecompute recompute
> +
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +check ovn-nbctl clear load_balancer_group . load_Balancer
> +check_engine_stats norecompute recompute
> +
> +# Add back lb to the lbg1 group
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +check ovn-nbctl add load_balancer_group . load_Balancer $lb1_uuid
> +check_engine_stats norecompute 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 recompute recompute
> +
> +# Update lb and this should result in 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 recompute 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 recompute recompute
> +
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +check ovn-nbctl add logical_router lr0 load_balancer_group $lbg1_uuid
> +check_engine_stats recompute recompute
> +
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +check ovn-nbctl clear logical_router lr0 load_balancer_group
> +check_engine_stats recompute recompute
> +
> +AT_CLEANUP
> +])

On Sat, Jul 8, 2023 at 3:57 AM Mark Michelson <mmichels@redhat.com> wrote:
>
> Hi Numan,
>
> I have one small nit below.
>
> On 7/7/23 01:53, numans@ovn.org wrote:
> > From: Numan Siddique <numans@ovn.org>
> >
> > Any changes to load balancers and load balancer groups
> > are handled incrementally in the newly added 'northd_lb_data'
> > engine node.  'northd_lb_data' is input to 'northd' node
> > and the handler - northd_northd_lb_data_handler in 'northd'
> > node handles the changes.
> >
> > If a load balancer or load balancer group is associated to
> > a logical switch or router then 'northd' node falls back
> > to full recompute.
> >
> > Signed-off-by: Numan Siddique <numans@ovn.org>
> > ---
> >   lib/lb.c                   |  85 ++++++++++++----
> >   lib/lb.h                   |   9 ++
> >   northd/en-northd-lb-data.c | 202 +++++++++++++++++++++++++++++++++++--
> >   northd/en-northd-lb-data.h |  37 +++++++
> >   northd/en-northd.c         |  24 +++++
> >   northd/en-northd.h         |   1 +
> >   northd/inc-proc-northd.c   |  13 ++-
> >   northd/northd.c            |  77 ++++++++++++++
> >   northd/northd.h            |   7 ++
> >   tests/ovn-northd.at        | 123 ++++++++++++++++++++++
> >   10 files changed, 545 insertions(+), 33 deletions(-)
> >
> > diff --git a/lib/lb.c b/lib/lb.c
> > index 429dbf15af..a51fe225fa 100644
> > --- a/lib/lb.c
> > +++ b/lib/lb.c
> > @@ -606,13 +606,13 @@ ovn_lb_get_health_check(const struct
nbrec_load_balancer *nbrec_lb,
> >       return NULL;
> >   }
> >
> > -struct ovn_northd_lb *
> > -ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
> > +static void
> > +ovn_northd_lb_init(struct ovn_northd_lb *lb,
> > +                   const struct nbrec_load_balancer *nbrec_lb)
> >   {
> >       bool template = smap_get_bool(&nbrec_lb->options, "template",
false);
> >       bool is_udp = nullable_string_is_equal(nbrec_lb->protocol, "udp");
> >       bool is_sctp = nullable_string_is_equal(nbrec_lb->protocol,
"sctp");
> > -    struct ovn_northd_lb *lb = xzalloc(sizeof *lb);
> >       int address_family = !strcmp(smap_get_def(&nbrec_lb->options,
> >                                                 "address-family",
"ipv4"),
> >                                    "ipv4")
> > @@ -668,6 +668,10 @@ ovn_northd_lb_create(const struct
nbrec_load_balancer *nbrec_lb)
> >                                                     "reject", false);
> >           ovn_northd_lb_vip_init(lb_vip_nb, lb_vip, nbrec_lb,
> >                                  node->key, node->value, template);
> > +        if (lb_vip_nb->lb_health_check) {
> > +            lb->health_checks = true;
> > +        }
> > +
> >           if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
> >               sset_add(&lb->ips_v4, lb_vip->vip_str);
> >           } else {
> > @@ -713,6 +717,13 @@ ovn_northd_lb_create(const struct
nbrec_load_balancer *nbrec_lb)
> >           ds_chomp(&sel_fields, ',');
> >           lb->selection_fields = ds_steal_cstr(&sel_fields);
> >       }
> > +}
> > +
> > +struct ovn_northd_lb *
> > +ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
> > +{
> > +    struct ovn_northd_lb *lb = xzalloc(sizeof *lb);
> > +    ovn_northd_lb_init(lb, nbrec_lb);
> >       return lb;
> >   }
> >
> > @@ -738,8 +749,8 @@ ovn_northd_lb_get_vips(const struct ovn_northd_lb
*lb)
> >       return &lb->nlb->vips;
> >   }
> >
> > -void
> > -ovn_northd_lb_destroy(struct ovn_northd_lb *lb)
> > +static void
> > +ovn_northd_lb_cleanup(struct ovn_northd_lb *lb)
> >   {
> >       for (size_t i = 0; i < lb->n_vips; i++) {
> >           ovn_lb_vip_destroy(&lb->vips[i]);
> > @@ -747,26 +758,36 @@ ovn_northd_lb_destroy(struct ovn_northd_lb *lb)
> >       }
> >       free(lb->vips);
> >       free(lb->vips_nb);
> > +    lb->vips = NULL;
> > +    lb->vips_nb = NULL;
> >       smap_destroy(&lb->template_vips);
> >       sset_destroy(&lb->ips_v4);
> >       sset_destroy(&lb->ips_v6);
> >       free(lb->selection_fields);
> > +    lb->selection_fields = NULL;
> > +    lb->health_checks = false;
> > +}
> > +
> > +void
> > +ovn_northd_lb_destroy(struct ovn_northd_lb *lb)
> > +{
> > +    ovn_northd_lb_cleanup(lb);
> >       free(lb);
> >   }
> >
> > -/* Constructs a new 'struct ovn_lb_group' object from the Nb LB Group
record
> > - * and a hash map of all existing 'struct ovn_northd_lb' objects.
Space will
> > - * be allocated for 'max_ls_datapaths' logical switches and
'max_lr_datapaths'
> > - * logical routers to which this LB Group is applied.  Can be filled
later
> > - * with ovn_lb_group_add_ls() and ovn_lb_group_add_lr() respectively.
*/
> > -struct ovn_lb_group *
> > -ovn_lb_group_create(const struct nbrec_load_balancer_group
*nbrec_lb_group,
> > -                    const struct hmap *lbs)
> > +void
> > +ovn_northd_lb_reinit(struct ovn_northd_lb *lb,
> > +                     const struct nbrec_load_balancer *nbrec_lb)
> >   {
> > -    struct ovn_lb_group *lb_group;
> > +    ovn_northd_lb_cleanup(lb);
> > +    ovn_northd_lb_init(lb, nbrec_lb);
> > +}
> >
> > -    lb_group = xzalloc(sizeof *lb_group);
> > -    lb_group->uuid = nbrec_lb_group->header_.uuid;
> > +static void
> > +ovn_lb_group_init(struct ovn_lb_group *lb_group,
> > +                  const struct nbrec_load_balancer_group
*nbrec_lb_group,
> > +                  const struct hmap *lbs)
> > +{
> >       lb_group->n_lbs = nbrec_lb_group->n_load_balancer;
> >       lb_group->lbs = xmalloc(lb_group->n_lbs * sizeof *lb_group->lbs);
> >       lb_group->lb_ips = ovn_lb_ip_set_create();
> > @@ -776,10 +797,30 @@ ovn_lb_group_create(const struct
nbrec_load_balancer_group *nbrec_lb_group,
> >               &nbrec_lb_group->load_balancer[i]->header_.uuid;
> >           lb_group->lbs[i] = ovn_northd_lb_find(lbs, lb_uuid);
> >       }
> > +}
> >
> > +/* Constructs a new 'struct ovn_lb_group' object from the Nb LB Group
record
> > + * and a hash map of all existing 'struct ovn_northd_lb' objects.
Space will
> > + * be allocated for 'max_ls_datapaths' logical switches and
'max_lr_datapaths'
> > + * logical routers to which this LB Group is applied.  Can be filled
later
> > + * with ovn_lb_group_add_ls() and ovn_lb_group_add_lr() respectively.
*/
> > +struct ovn_lb_group *
> > +ovn_lb_group_create(const struct nbrec_load_balancer_group
*nbrec_lb_group,
> > +                    const struct hmap *lbs)
> > +{
> > +    struct ovn_lb_group *lb_group = xzalloc(sizeof *lb_group);
> > +    lb_group->uuid = nbrec_lb_group->header_.uuid;
> > +    ovn_lb_group_init(lb_group, nbrec_lb_group, lbs);
> >       return lb_group;
> >   }
> >
> > +static void
> > +ovn_lb_group_cleanup(struct ovn_lb_group *lb_group)
> > +{
> > +    ovn_lb_ip_set_destroy(lb_group->lb_ips);
> > +    free(lb_group->lbs);
> > +}
> > +
> >   void
> >   ovn_lb_group_destroy(struct ovn_lb_group *lb_group)
> >   {
> > @@ -787,11 +828,19 @@ ovn_lb_group_destroy(struct ovn_lb_group
*lb_group)
> >           return;
> >       }
> >
> > -    ovn_lb_ip_set_destroy(lb_group->lb_ips);
> > -    free(lb_group->lbs);
> > +    ovn_lb_group_cleanup(lb_group);
> >       free(lb_group);
> >   }
> >
> > +void
> > +ovn_lb_group_reinit(struct ovn_lb_group *lb_group,
> > +                    const struct nbrec_load_balancer_group
*nbrec_lb_group,
> > +                    const struct hmap *lbs)
> > +{
> > +    ovn_lb_group_cleanup(lb_group);
> > +    ovn_lb_group_init(lb_group, nbrec_lb_group, lbs);
> > +}
> > +
> >   struct ovn_lb_group *
> >   ovn_lb_group_find(const struct hmap *lb_groups, const struct uuid
*uuid)
> >   {
> > diff --git a/lib/lb.h b/lib/lb.h
> > index 0339050cba..74905c73b7 100644
> > --- a/lib/lb.h
> > +++ b/lib/lb.h
> > @@ -77,6 +77,9 @@ struct ovn_northd_lb {
> >
> >       struct sset ips_v4;
> >       struct sset ips_v6;
> > +
> > +    /* Indicates if the load balancer has health checks configured. */
> > +    bool health_checks;
> >   };
> >
> >   struct ovn_lb_vip {
> > @@ -130,6 +133,8 @@ struct ovn_northd_lb *ovn_northd_lb_find(const
struct hmap *,
> >                                            const struct uuid *);
> >   const struct smap *ovn_northd_lb_get_vips(const struct ovn_northd_lb
*);
> >   void ovn_northd_lb_destroy(struct ovn_northd_lb *);
> > +void ovn_northd_lb_reinit(struct ovn_northd_lb *,
> > +                          const struct nbrec_load_balancer *);
> >
> >   void build_lrouter_lb_ips(struct ovn_lb_ip_set *,
> >                             const struct ovn_northd_lb *);
> > @@ -148,6 +153,10 @@ struct ovn_lb_group *ovn_lb_group_create(
> >   void ovn_lb_group_destroy(struct ovn_lb_group *lb_group);
> >   struct ovn_lb_group *ovn_lb_group_find(const struct hmap *lb_groups,
> >                                          const struct uuid *);
> > +void ovn_lb_group_reinit(
> > +    struct ovn_lb_group *lb_group,
> > +    const struct nbrec_load_balancer_group *,
> > +    const struct hmap *lbs);
> >
> >   struct ovn_lb_datapaths {
> >       struct hmap_node hmap_node;
> > diff --git a/northd/en-northd-lb-data.c b/northd/en-northd-lb-data.c
> > index d46c3c27ed..e52535b4a9 100644
> > --- a/northd/en-northd-lb-data.c
> > +++ b/northd/en-northd-lb-data.c
> > @@ -37,6 +37,15 @@ static void northd_lb_data_destroy(struct
northd_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 struct ovn_lb_group *create_lb_group(
> > +    const struct nbrec_load_balancer_group *, struct hmap *lbs,
> > +    struct hmap *lb_groups);
> > +static void destroy_tracked_data(struct northd_lb_data *);
> > +static void add_lb_to_tracked_data(struct ovn_northd_lb *,
> > +                                   struct ovs_list *tracked_list,
> > +                                   bool health_checks);
> > +static void add_lb_group_to_tracked_data(struct ovn_lb_group *,
> > +                                         struct ovs_list
*tracked_list);
> >
> >   void *
> >   en_northd_lb_data_init(struct engine_node *node OVS_UNUSED,
> > @@ -61,6 +70,7 @@ en_northd_lb_data_run(struct engine_node *node, void
*data)
> >       const struct nbrec_load_balancer_group_table *nb_lbg_table =
> >           EN_OVSDB_GET(engine_get_input("NB_load_balancer_group",
node));
> >
> > +    lb_data->tracked = false;
> >       build_lbs(nb_lb_table, nb_lbg_table, &lb_data->lbs,
&lb_data->lb_groups);
> >       engine_set_node_state(node, EN_UPDATED);
> >   }
> > @@ -72,12 +82,122 @@ en_northd_lb_data_cleanup(void *data)
> >       northd_lb_data_destroy(lb_data);
> >   }
> >
> > +void
> > +en_northd_lb_data_clear_tracked_data(void *data)
> > +{
> > +    struct northd_lb_data *lb_data = (struct northd_lb_data *) data;
> > +    destroy_tracked_data(lb_data);
> > +}
> > +
> > +
> > +/* Handler functions. */
> > +bool
> > +northd_lb_data_load_balancer_handler(struct engine_node *node,
> > +                                     void *data OVS_UNUSED)
> > +{
> > +    const struct nbrec_load_balancer_table *nb_lb_table =
> > +        EN_OVSDB_GET(engine_get_input("NB_load_balancer", node));
> > +
> > +    struct northd_lb_data *lb_data = (struct northd_lb_data *) data;
> > +
> > +    lb_data->tracked = true;
> > +    struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> > +
> > +    const struct nbrec_load_balancer *tracked_lb;
> > +    NBREC_LOAD_BALANCER_TABLE_FOR_EACH_TRACKED (tracked_lb,
nb_lb_table) {
> > +        struct ovn_northd_lb *lb;
> > +        if (nbrec_load_balancer_is_new(tracked_lb)) {
> > +            /* New load balancer. */
> > +            lb = ovn_northd_lb_create(tracked_lb);
> > +            hmap_insert(&lb_data->lbs, &lb->hmap_node,
> > +                        uuid_hash(&tracked_lb->header_.uuid));
> > +            add_lb_to_tracked_data(
> > +                lb, &trk_lb_data->tracked_updated_lbs.updated,
> > +                lb->health_checks);
> > +        } else if (nbrec_load_balancer_is_deleted(tracked_lb)) {
> > +            lb = ovn_northd_lb_find(&lb_data->lbs,
> > +                                    &tracked_lb->header_.uuid);
> > +            ovs_assert(lb);
> > +            hmap_remove(&lb_data->lbs, &lb->hmap_node);
> > +            add_lb_to_tracked_data(
> > +                lb, &trk_lb_data->tracked_deleted_lbs.updated,
> > +                lb->health_checks);
> > +        } else {
> > +            /* Load balancer updated. */
> > +            lb = ovn_northd_lb_find(&lb_data->lbs,
> > +                                    &tracked_lb->header_.uuid);
> > +            ovs_assert(lb);
> > +            bool health_checks = lb->health_checks;
> > +            ovn_northd_lb_reinit(lb, tracked_lb);
> > +            health_checks |= lb->health_checks;
> > +            add_lb_to_tracked_data(
> > +                lb, &trk_lb_data->tracked_updated_lbs.updated,
health_checks);
> > +        }
> > +    }
> > +
> > +    engine_set_node_state(node, EN_UPDATED);
> > +    return true;
> > +}
> > +
> > +bool
> > +northd_lb_data_load_balancer_group_handler(struct engine_node *node,
> > +                                           void *data OVS_UNUSED)
> > +{
> > +    struct northd_lb_data *lb_data = (struct northd_lb_data *) data;
> > +    const struct nbrec_load_balancer_group_table *nb_lbg_table =
> > +        EN_OVSDB_GET(engine_get_input("NB_load_balancer_group", node));
> > +
> > +    lb_data->tracked = true;
> > +    struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> > +    const struct nbrec_load_balancer_group *tracked_lb_group;
> > +    NBREC_LOAD_BALANCER_GROUP_TABLE_FOR_EACH_TRACKED (tracked_lb_group,
> > +                                                      nb_lbg_table) {
> > +        if (nbrec_load_balancer_group_is_new(tracked_lb_group)) {
> > +            struct ovn_lb_group *lb_group =
> > +                create_lb_group(tracked_lb_group, &lb_data->lbs,
> > +                                &lb_data->lb_groups);
> > +            add_lb_group_to_tracked_data(
> > +                lb_group,
&trk_lb_data->tracked_updated_lb_groups.updated);
> > +        } else if
(nbrec_load_balancer_group_is_deleted(tracked_lb_group)) {
> > +            struct ovn_lb_group *lb_group;
> > +            lb_group = ovn_lb_group_find(&lb_data->lb_groups,
> > +
&tracked_lb_group->header_.uuid);
> > +            ovs_assert(lb_group);
> > +            hmap_remove(&lb_data->lb_groups, &lb_group->hmap_node);
> > +            add_lb_group_to_tracked_data(
> > +                lb_group,
&trk_lb_data->tracked_deleted_lb_groups.updated);
> > +        } else if
(nbrec_load_balancer_group_is_updated(tracked_lb_group,
> > +
 NBREC_LOAD_BALANCER_GROUP_COL_LOAD_BALANCER)) {
> > +
> > +            struct ovn_lb_group *lb_group;
> > +            lb_group = ovn_lb_group_find(&lb_data->lb_groups,
> > +
&tracked_lb_group->header_.uuid);
> > +            ovs_assert(lb_group);
> > +            ovn_lb_group_reinit(lb_group, tracked_lb_group,
&lb_data->lbs);
> > +            for (size_t i = 0; i < lb_group->n_lbs; i++) {
> > +                build_lrouter_lb_ips(lb_group->lb_ips,
lb_group->lbs[i]);
> > +            }
> > +            add_lb_group_to_tracked_data(
> > +                lb_group,
&trk_lb_data->tracked_updated_lb_groups.updated);
> > +        }
> > +    }
> > +
> > +    engine_set_node_state(node, EN_UPDATED);
> > +    return true;
> > +}
> > +
> >   /* static functions. */
> >   static void
> >   northd_lb_data_init(struct northd_lb_data *lb_data)
> >   {
> >       hmap_init(&lb_data->lbs);
> >       hmap_init(&lb_data->lb_groups);
> > +
> > +    struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> > +    ovs_list_init(&trk_lb_data->tracked_updated_lbs.updated);
> > +    ovs_list_init(&trk_lb_data->tracked_deleted_lbs.updated);
> > +    ovs_list_init(&trk_lb_data->tracked_updated_lb_groups.updated);
> > +    ovs_list_init(&trk_lb_data->tracked_deleted_lb_groups.updated);
> >   }
> >
> >   static void
> > @@ -94,6 +214,8 @@ northd_lb_data_destroy(struct northd_lb_data
*lb_data)
> >           ovn_lb_group_destroy(lb_group);
> >       }
> >       hmap_destroy(&lb_data->lb_groups);
> > +
> > +    destroy_tracked_data(lb_data);
> >   }
> >
> >   static void
> > @@ -101,12 +223,9 @@ build_lbs(const struct nbrec_load_balancer_table
*nbrec_load_balancer_table,
> >             const struct nbrec_load_balancer_group_table
*nbrec_lb_group_table,
> >             struct hmap *lbs, struct hmap *lb_groups)
> >   {
> > -    struct ovn_lb_group *lb_group;
> > -    struct ovn_northd_lb *lb_nb;
> > -
> >       const struct nbrec_load_balancer *nbrec_lb;
> >       NBREC_LOAD_BALANCER_TABLE_FOR_EACH (nbrec_lb,
nbrec_load_balancer_table) {
> > -        lb_nb = ovn_northd_lb_create(nbrec_lb);
> > +        struct ovn_northd_lb *lb_nb = ovn_northd_lb_create(nbrec_lb);
> >           hmap_insert(lbs, &lb_nb->hmap_node,
> >                       uuid_hash(&nbrec_lb->header_.uuid));
> >       }
> > @@ -114,13 +233,76 @@ build_lbs(const struct nbrec_load_balancer_table
*nbrec_load_balancer_table,
> >       const struct nbrec_load_balancer_group *nbrec_lb_group;
> >       NBREC_LOAD_BALANCER_GROUP_TABLE_FOR_EACH (nbrec_lb_group,
> >                                                 nbrec_lb_group_table) {
> > -        lb_group = ovn_lb_group_create(nbrec_lb_group, lbs);
> > +        create_lb_group(nbrec_lb_group, lbs, lb_groups);
> > +    }
> > +}
> >
> > -        for (size_t i = 0; i < lb_group->n_lbs; i++) {
> > -            build_lrouter_lb_ips(lb_group->lb_ips, lb_group->lbs[i]);
> > -        }
> > +static struct ovn_lb_group *
> > +create_lb_group(const struct nbrec_load_balancer_group *nbrec_lb_group,
> > +                struct hmap *lbs, struct hmap *lb_groups)
> > +{
> > +    struct ovn_lb_group *lb_group =
ovn_lb_group_create(nbrec_lb_group, lbs);
> > +
> > +    for (size_t i = 0; i < lb_group->n_lbs; i++) {
> > +        build_lrouter_lb_ips(lb_group->lb_ips, lb_group->lbs[i]);
> > +    }
> > +
> > +    hmap_insert(lb_groups, &lb_group->hmap_node,
> > +                uuid_hash(&lb_group->uuid));
> > +
> > +    return lb_group;
> > +}
> > +
> > +static void
> > +destroy_tracked_data(struct northd_lb_data *lb_data)
> > +{
> > +    lb_data->tracked = false;
> > +
> > +    struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> > +    struct tracked_lb *tracked_lb;
> > +    LIST_FOR_EACH_SAFE (tracked_lb, list_node,
> > +                        &trk_lb_data->tracked_updated_lbs.updated) {
> > +        ovs_list_remove(&tracked_lb->list_node);
> > +        free(tracked_lb);
> > +    }
> > +
> > +    LIST_FOR_EACH_SAFE (tracked_lb, list_node,
> > +                        &trk_lb_data->tracked_deleted_lbs.updated) {
> > +        ovs_list_remove(&tracked_lb->list_node);
> > +        ovn_northd_lb_destroy(tracked_lb->lb);
> > +        free(tracked_lb);
> > +    }
> > +
> > +    struct tracked_lb_group *tracked_lb_group;
> > +    LIST_FOR_EACH_SAFE (tracked_lb_group, list_node,
> > +
 &trk_lb_data->tracked_updated_lb_groups.updated) {
> > +        ovs_list_remove(&tracked_lb_group->list_node);
> > +        free(tracked_lb_group);
> > +    }
> >
> > -        hmap_insert(lb_groups, &lb_group->hmap_node,
> > -                    uuid_hash(&lb_group->uuid));
> > +    LIST_FOR_EACH_SAFE (tracked_lb_group, list_node,
> > +
 &trk_lb_data->tracked_deleted_lb_groups.updated) {
> > +        ovs_list_remove(&tracked_lb_group->list_node);
> > +        ovn_lb_group_destroy(tracked_lb_group->lb_group);
> > +        free(tracked_lb_group);
> >       }
> >   }
> > +
> > +static void
> > +add_lb_to_tracked_data(struct ovn_northd_lb *lb, struct ovs_list
*tracked_list,
> > +                       bool health_checks)
> > +{
> > +    struct tracked_lb *t_lb = xzalloc(sizeof *t_lb);
> > +    t_lb->lb = lb;
> > +    t_lb->health_checks = health_checks;
> > +    ovs_list_push_back(tracked_list, &t_lb->list_node);
> > +}
> > +
> > +static void
> > +add_lb_group_to_tracked_data(struct ovn_lb_group *lb_group,
> > +                             struct ovs_list *tracked_list)
> > +{
> > +    struct tracked_lb_group *t_lb_group = xzalloc(sizeof *t_lb_group);
> > +    t_lb_group->lb_group = lb_group;
> > +    ovs_list_push_back(tracked_list, &t_lb_group->list_node);
> > +}
> > diff --git a/northd/en-northd-lb-data.h b/northd/en-northd-lb-data.h
> > index eb297e376d..bb07fcf686 100644
> > --- a/northd/en-northd-lb-data.h
> > +++ b/northd/en-northd-lb-data.h
> > @@ -4,16 +4,53 @@
> >   #include <config.h>
> >
> >   #include "openvswitch/hmap.h"
> > +#include "include/openvswitch/list.h"
> >
> >   #include "lib/inc-proc-eng.h"
> >
> > +struct ovn_northd_lb;
> > +struct ovn_lb_group;
> > +
> > +struct tracked_lb {
> > +    struct ovs_list list_node;
> > +    struct ovn_northd_lb *lb;
> > +    bool health_checks;
> > +};
> > +
> > +struct tracked_lb_group {
> > +    struct ovs_list list_node;
> > +    struct ovn_lb_group *lb_group;
> > +};
> > +
> > +struct tracked_lb_changes {
> > +    struct ovs_list updated; /* contains list of 'struct tracked_lb '
or
> > +                                list of 'struct tracked_lb_group' */
> > +};
> > +
> > +struct tracked_lb_data {
> > +    struct tracked_lb_changes tracked_updated_lbs;
> > +    struct tracked_lb_changes tracked_deleted_lbs;
> > +    struct tracked_lb_changes tracked_updated_lb_groups;
> > +    struct tracked_lb_changes tracked_deleted_lb_groups;
> > +};
>
> The typing here is a bit odd. The problem is that struct
> tracked_lb_changes is being used for two distinct types of lists. I
> think you should do one of two things here.
>
> 1) Get rid of the trakced_lb_changes struct and use ovs_list in-line.
>
> struct tracked_lb_data {
>      struct ovs_list tracked_updated_lbs;
>      struct ovs_list tracked_deleted_lbs;
>      struct ovs_list tracked_updated_lb_groups;
>      struct ovs_list tracked_deleted_lb_groups;
> };
>
> 2) Make a separate type for tracked lbs and tracked lb groups.
>
> struct tracked_lb_changes {
>      struct ovs_list updated;
> };
>
> struct tracked_lb_group_changes {
>      struct ovs_list updated;
> };
>
> struct tracked_lb_data {
>      struct tracked_lb_changes tracked_updated_lbs;
>      struct tracked_lb_changes tracked_deleted_lbs;
>      struct tracked_lb_group_changes tracked_updated_lb_groups;
>      struct tracked_lb_group_changes tracked_deleted_lb_groups;
> };
>
> I think option (2) is my preference.

I prefer option (1), because the names of the fields in struct
tracked_lb_data already tells the purpose, and the only member "update" in
the struct tracked_lb_changes is unnecessary. In addition, the name
"update" is confusing, because it is in fact used for both "updated"
(added/updated) and "deleted".


>
> > +
> >   struct northd_lb_data {
> >       struct hmap lbs;
> >       struct hmap lb_groups;
> > +
> > +    /* tracked data*/
> > +    bool tracked;
> > +    struct tracked_lb_data tracked_lb_data;
> >   };
> >
> >   void *en_northd_lb_data_init(struct engine_node *, struct engine_arg
*);
> >   void en_northd_lb_data_run(struct engine_node *, void *data);
> >   void en_northd_lb_data_cleanup(void *data);
> > +void en_northd_lb_data_clear_tracked_data(void *data);
> > +
> > +bool northd_lb_data_load_balancer_handler(struct engine_node *,
> > +                                          void *data);
> > +bool northd_lb_data_load_balancer_group_handler(struct engine_node *,
> > +                                                void *data);
> >
> >   #endif /* end of EN_NORTHD_LB_DATA_H */
> > diff --git a/northd/en-northd.c b/northd/en-northd.c
> > index cc7d838451..b3c03c54bd 100644
> > --- a/northd/en-northd.c
> > +++ b/northd/en-northd.c
> > @@ -206,6 +206,30 @@ northd_sb_port_binding_handler(struct engine_node
*node,
> >       return true;
> >   }
> >
> > +bool
> > +northd_northd_lb_data_handler(struct engine_node *node, void *data)
> > +{
> > +    struct northd_lb_data *lb_data =
> > +        engine_get_input_data("northd_lb_data", node);
> > +
> > +    if (!lb_data->tracked) {
> > +        return false;
> > +    }
> > +
> > +    struct northd_data *nd = data;
> > +
> > +    if (!northd_handle_lb_data_changes(&lb_data->tracked_lb_data,
> > +                                       &nd->ls_datapaths,
> > +                                       &nd->lr_datapaths,
> > +                                       &nd->lb_datapaths_map,
> > +                                       &nd->lb_group_datapaths_map)) {
> > +        return false;
> > +    }
> > +
> > +    engine_set_node_state(node, EN_UPDATED);
> > +    return true;
> > +}
> > +
> >   void
> >   *en_northd_init(struct engine_node *node OVS_UNUSED,
> >                   struct engine_arg *arg OVS_UNUSED)
> > diff --git a/northd/en-northd.h b/northd/en-northd.h
> > index 20cc77f108..5674f4390c 100644
> > --- a/northd/en-northd.h
> > +++ b/northd/en-northd.h
> > @@ -17,5 +17,6 @@ void en_northd_clear_tracked_data(void *data);
> >   bool northd_nb_nb_global_handler(struct engine_node *, void *data
OVS_UNUSED);
> >   bool northd_nb_logical_switch_handler(struct engine_node *, void
*data);
> >   bool northd_sb_port_binding_handler(struct engine_node *, void *data);
> > +bool northd_northd_lb_data_handler(struct engine_node *, void *data);
> >
> >   #endif /* EN_NORTHD_H */
> > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> > index b2e884962f..b444a488db 100644
> > --- a/northd/inc-proc-northd.c
> > +++ b/northd/inc-proc-northd.c
> > @@ -141,13 +141,18 @@ static ENGINE_NODE(sync_to_sb_addr_set,
"sync_to_sb_addr_set");
> >   static ENGINE_NODE(fdb_aging, "fdb_aging");
> >   static ENGINE_NODE(fdb_aging_waker, "fdb_aging_waker");
> >   static ENGINE_NODE(sync_to_sb_lb, "sync_to_sb_lb");
> > -static ENGINE_NODE(northd_lb_data, "northd_lb_data");
> > +static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(northd_lb_data,
"northd_lb_data");
> >
> >   void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> >                             struct ovsdb_idl_loop *sb)
> >   {
> >       /* Define relationships between nodes where first argument is
dependent
> >        * on the second argument */
> > +    engine_add_input(&en_northd_lb_data, &en_nb_load_balancer,
> > +                     northd_lb_data_load_balancer_handler);
> > +    engine_add_input(&en_northd_lb_data, &en_nb_load_balancer_group,
> > +                     northd_lb_data_load_balancer_group_handler);
> > +

Maybe it is not a big deal, but I think it is better to move the
dependencies that have handlers after the ones that don't, so that any
changes that will trigger recompute are handled first, to avoid wasting
time handling other changes and then fallback to recompute.

With these minor comments addressed:
Acked-by: Han Zhou <hzhou@ovn.org>

> >       engine_add_input(&en_northd, &en_nb_port_group, NULL);
> >       engine_add_input(&en_northd, &en_nb_acl, NULL);
> >       engine_add_input(&en_northd, &en_nb_logical_router, NULL);
> > @@ -171,6 +176,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> >       engine_add_input(&en_northd, &en_sb_static_mac_binding, NULL);
> >       engine_add_input(&en_northd, &en_sb_chassis_template_var, NULL);
> >
> > +    engine_add_input(&en_northd, &en_northd_lb_data,
> > +                     northd_northd_lb_data_handler);
> >       engine_add_input(&en_northd, &en_sb_port_binding,
> >                        northd_sb_port_binding_handler);
> >       engine_add_input(&en_northd, &en_nb_nb_global,
> > @@ -178,10 +185,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop
*nb,
> >       engine_add_input(&en_northd, &en_nb_logical_switch,
> >                        northd_nb_logical_switch_handler);
> >
> > -    engine_add_input(&en_northd_lb_data, &en_nb_load_balancer, NULL);
> > -    engine_add_input(&en_northd_lb_data, &en_nb_load_balancer_group,
NULL);
> > -    engine_add_input(&en_northd, &en_northd_lb_data, NULL);
> > -
> >       engine_add_input(&en_mac_binding_aging, &en_nb_nb_global, NULL);
> >       engine_add_input(&en_mac_binding_aging, &en_sb_mac_binding, NULL);
> >       engine_add_input(&en_mac_binding_aging, &en_northd, NULL);
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 890186b29c..f27f0de49b 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -40,6 +40,7 @@
> >   #include "lib/lb.h"
> >   #include "memory.h"
> >   #include "northd.h"
> > +#include "en-northd-lb-data.h"
> >   #include "lib/ovn-parallel-hmap.h"
> >   #include "ovn/actions.h"
> >   #include "ovn/features.h"
> > @@ -5323,6 +5324,82 @@ northd_handle_sb_port_binding_changes(
> >       return true;
> >   }
> >
> > +bool
> > +northd_handle_lb_data_changes(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 tracked_lb *trk_lb;
> > +
> > +    struct ovn_lb_datapaths *lb_dps;
> > +    LIST_FOR_EACH (trk_lb, list_node,
> > +                   &trk_lb_data->tracked_deleted_lbs.updated) {
> > +        if (trk_lb->health_checks) {
> > +            /* Fall back to recompute if the deleted load balancer has
> > +             * health checks configured. */
> > +            return false;
> > +        }
> > +
> > +        const struct uuid *lb_uuid =
> > +                &trk_lb->lb->nlb->header_.uuid;
> > +
> > +        lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> > +        ovs_assert(lb_dps);
> > +        hmap_remove(lb_datapaths_map, &lb_dps->hmap_node);
> > +        ovn_lb_datapaths_destroy(lb_dps);
> > +    }
> > +
> > +    LIST_FOR_EACH (trk_lb, list_node,
> > +                   &trk_lb_data->tracked_updated_lbs.updated) {
> > +        if (trk_lb->health_checks) {
> > +            /* Fall back to recompute if the created/updated load
balancer has
> > +             * health checks configured. */
> > +            return false;
> > +        }
> > +
> > +        const struct uuid *lb_uuid =
> > +                &trk_lb->lb->nlb->header_.uuid;
> > +        lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> > +        if (!lb_dps) {
> > +            lb_dps = ovn_lb_datapaths_create(trk_lb->lb,
> > +                                             ods_size(ls_datapaths),
> > +                                             ods_size(lr_datapaths));
> > +            hmap_insert(lb_datapaths_map, &lb_dps->hmap_node,
> > +                        uuid_hash(lb_uuid));
> > +        }
> > +    }
> > +
> > +    struct ovn_lb_group_datapaths *lb_group_dps;
> > +    struct tracked_lb_group *trk_lb_group;
> > +    LIST_FOR_EACH_SAFE (trk_lb_group, list_node,
> > +
 &trk_lb_data->tracked_deleted_lb_groups.updated) {
> > +        const struct uuid *lb_uuid = &trk_lb_group->lb_group->uuid;
> > +        lb_group_dps =
ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > +                                                   lb_uuid);
> > +        ovs_assert(lb_group_dps);
> > +        hmap_remove(lb_group_datapaths_map, &lb_group_dps->hmap_node);
> > +        ovn_lb_group_datapaths_destroy(lb_group_dps);
> > +    }
> > +
> > +    LIST_FOR_EACH_SAFE (trk_lb_group, list_node,
> > +
 &trk_lb_data->tracked_updated_lb_groups.updated) {
> > +        const struct uuid *lb_uuid = &trk_lb_group->lb_group->uuid;
> > +        lb_group_dps =
ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > +                                                   lb_uuid);
> > +        if (!lb_group_dps) {
> > +            lb_group_dps = ovn_lb_group_datapaths_create(
> > +                trk_lb_group->lb_group, ods_size(ls_datapaths),
> > +                ods_size(lr_datapaths));
> > +            hmap_insert(lb_group_datapaths_map,
&lb_group_dps->hmap_node,
> > +                        uuid_hash(lb_uuid));
> > +        }
> > +    }
> > +
> > +    return true;
> > +}
> > +
> >   struct multicast_group {
> >       const char *name;
> >       uint16_t key;               /*
OVN_MIN_MULTICAST...OVN_MAX_MULTICAST. */
> > diff --git a/northd/northd.h b/northd/northd.h
> > index 7d92028c7d..7d17921fa2 100644
> > --- a/northd/northd.h
> > +++ b/northd/northd.h
> > @@ -347,6 +347,13 @@ bool lflow_handle_northd_ls_changes(struct
ovsdb_idl_txn *ovnsb_txn,
> >   bool northd_handle_sb_port_binding_changes(
> >       const struct sbrec_port_binding_table *, struct hmap *ls_ports);
> >
> > +struct tracked_lb_data;
> > +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);
> > +
> >   void build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
> >                        const struct nbrec_bfd_table *,
> >                        const struct sbrec_bfd_table *,
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index e79d33b2ae..dd0bd8b36a 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -9681,3 +9681,126 @@ AT_CHECK([grep "lr_in_gw_redirect" R1flows |sed
s'/table=../table=??/' |sort], [
> >
> >   AT_CLEANUP
> >   ])
> > +
> > +OVN_FOR_EACH_NORTHD_NO_HV([
> > +AT_SETUP([Load balancer incremental processing])
> > +ovn_start
> > +
> > +check_engine_stats() {
> > +  northd_comp=$1
> > +  lflow_comp=$2
> > +
> > +  AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats
northd_lb_data recompute], [0], [0
> > +])
> > +
> > +  northd_recompute_ct=$(as northd ovn-appctl -t NORTHD_TYPE
inc-engine/show-stats northd recompute)
> > +  if [[ "$northd_comp" == "norecompute" ]]; then
> > +    check test "$northd_recompute_ct" -eq "0"
> > +  else
> > +    check test "$northd_recompute_ct" -ne "0"
> > +  fi
> > +
> > +  lflow_recompute_ct=$(as northd ovn-appctl -t NORTHD_TYPE
inc-engine/show-stats lflow recompute)
> > +  if [[ "$lflow_comp" == "norecompute" ]]; then
> > +    check test "$lflow_recompute_ct" -eq "0"
> > +  else
> > +    check test "$lflow_recompute_ct" -ne "0"
> > +  fi
> > +}
> > +
> > +# Test I-P for load balancers.
> > +# Presently ovn-northd handles I-P for NB LBs in northd_lb_data engine
node
> > +# only.
> > +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 norecompute recompute
> > +
> > +check ovn-nbctl --wait=sb set load_balancer .
ip_port_mappings:10.0.0.3=sw0-p1:10.0.0.2
> > +check_engine_stats norecompute recompute
> > +
> > +check ovn-nbctl --wait=sb set load_balancer . options:foo=bar
> > +check_engine_stats norecompute recompute
> > +
> > +check ovn-nbctl --wait=sb -- lb-add lb2 20.0.0.10:80 20.0.0.20:80 --
lb-add lb3 30.0.0.10:80 30.0.0.20:80
> > +check_engine_stats norecompute recompute
> > +
> > +check ovn-nbctl --wait=sb -- lb-del lb2 -- lb-del lb3
> > +check_engine_stats norecompute recompute
> > +
> > +AT_CHECK([ovn-nbctl --wait=sb \
> > +          -- --id=@hc create Load_Balancer_Health_Check
vip="10.0.0.10\:80" \
> > +             options:failure_count=100 \
> > +          -- add Load_Balancer . health_check @hc | uuidfilt], [0],
[<0>
> > +])
> > +check_engine_stats recompute recompute
> > +
> > +# Any change to load balancer health check should also result in full
recompute
> > +# of northd node (but not northd_lb_data node)
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl --wait=sb set load_balancer_health_check .
options:foo=bar1
> > +check_engine_stats recompute recompute
> > +
> > +# Delete the health check from the load balancer.  northd engine node
should do a full recompute.
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl --wait=sb clear Load_Balancer . health_check
> > +check_engine_stats recompute recompute
> > +
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl ls-add sw0
> > +check ovn-nbctl --wait=sb lr-add lr0
> > +check_engine_stats recompute recompute
> > +
> > +# Associate lb1 to 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-add sw0 lb1
> > +check_engine_stats recompute 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 recompute recompute
> > +
> > +# Test load balancer group now
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +lbg1_uuid=$(ovn-nbctl create load_balancer_group name=lbg1)
> > +check_engine_stats norecompute recompute
> > +
> > +lb1_uuid=$(fetch_column nb:Load_Balancer _uuid)
> > +
> > +# Add lb to the lbg1 group
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl add load_balancer_group . load_Balancer $lb1_uuid
> > +check_engine_stats norecompute recompute
> > +
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl clear load_balancer_group . load_Balancer
> > +check_engine_stats norecompute recompute
> > +
> > +# Add back lb to the lbg1 group
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl add load_balancer_group . load_Balancer $lb1_uuid
> > +check_engine_stats norecompute 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 recompute recompute
> > +
> > +# Update lb and this should result in 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 recompute 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 recompute recompute
> > +
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl add logical_router lr0 load_balancer_group $lbg1_uuid
> > +check_engine_stats recompute recompute
> > +
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl clear logical_router lr0 load_balancer_group
> > +check_engine_stats recompute recompute
> > +
> > +AT_CLEANUP
> > +])
>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On Wed, Jul 12, 2023 at 9:26 PM Han Zhou <zhouhan@gmail.com> wrote:
>
> On Sat, Jul 8, 2023 at 3:57 AM Mark Michelson <mmichels@redhat.com> wrote:
> >
> > Hi Numan,
> >
> > I have one small nit below.
> >
> > On 7/7/23 01:53, numans@ovn.org wrote:
> > > From: Numan Siddique <numans@ovn.org>
> > >
> > > Any changes to load balancers and load balancer groups
> > > are handled incrementally in the newly added 'northd_lb_data'
> > > engine node.  'northd_lb_data' is input to 'northd' node
> > > and the handler - northd_northd_lb_data_handler in 'northd'
> > > node handles the changes.
> > >
> > > If a load balancer or load balancer group is associated to
> > > a logical switch or router then 'northd' node falls back
> > > to full recompute.
> > >
> > > Signed-off-by: Numan Siddique <numans@ovn.org>
> > > ---
> > >   lib/lb.c                   |  85 ++++++++++++----
> > >   lib/lb.h                   |   9 ++
> > >   northd/en-northd-lb-data.c | 202 +++++++++++++++++++++++++++++++++++--
> > >   northd/en-northd-lb-data.h |  37 +++++++
> > >   northd/en-northd.c         |  24 +++++
> > >   northd/en-northd.h         |   1 +
> > >   northd/inc-proc-northd.c   |  13 ++-
> > >   northd/northd.c            |  77 ++++++++++++++
> > >   northd/northd.h            |   7 ++
> > >   tests/ovn-northd.at        | 123 ++++++++++++++++++++++
> > >   10 files changed, 545 insertions(+), 33 deletions(-)
> > >
> > > diff --git a/lib/lb.c b/lib/lb.c
> > > index 429dbf15af..a51fe225fa 100644
> > > --- a/lib/lb.c
> > > +++ b/lib/lb.c
> > > @@ -606,13 +606,13 @@ ovn_lb_get_health_check(const struct
> nbrec_load_balancer *nbrec_lb,
> > >       return NULL;
> > >   }
> > >
> > > -struct ovn_northd_lb *
> > > -ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
> > > +static void
> > > +ovn_northd_lb_init(struct ovn_northd_lb *lb,
> > > +                   const struct nbrec_load_balancer *nbrec_lb)
> > >   {
> > >       bool template = smap_get_bool(&nbrec_lb->options, "template",
> false);
> > >       bool is_udp = nullable_string_is_equal(nbrec_lb->protocol, "udp");
> > >       bool is_sctp = nullable_string_is_equal(nbrec_lb->protocol,
> "sctp");
> > > -    struct ovn_northd_lb *lb = xzalloc(sizeof *lb);
> > >       int address_family = !strcmp(smap_get_def(&nbrec_lb->options,
> > >                                                 "address-family",
> "ipv4"),
> > >                                    "ipv4")
> > > @@ -668,6 +668,10 @@ ovn_northd_lb_create(const struct
> nbrec_load_balancer *nbrec_lb)
> > >                                                     "reject", false);
> > >           ovn_northd_lb_vip_init(lb_vip_nb, lb_vip, nbrec_lb,
> > >                                  node->key, node->value, template);
> > > +        if (lb_vip_nb->lb_health_check) {
> > > +            lb->health_checks = true;
> > > +        }
> > > +
> > >           if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
> > >               sset_add(&lb->ips_v4, lb_vip->vip_str);
> > >           } else {
> > > @@ -713,6 +717,13 @@ ovn_northd_lb_create(const struct
> nbrec_load_balancer *nbrec_lb)
> > >           ds_chomp(&sel_fields, ',');
> > >           lb->selection_fields = ds_steal_cstr(&sel_fields);
> > >       }
> > > +}
> > > +
> > > +struct ovn_northd_lb *
> > > +ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
> > > +{
> > > +    struct ovn_northd_lb *lb = xzalloc(sizeof *lb);
> > > +    ovn_northd_lb_init(lb, nbrec_lb);
> > >       return lb;
> > >   }
> > >
> > > @@ -738,8 +749,8 @@ ovn_northd_lb_get_vips(const struct ovn_northd_lb
> *lb)
> > >       return &lb->nlb->vips;
> > >   }
> > >
> > > -void
> > > -ovn_northd_lb_destroy(struct ovn_northd_lb *lb)
> > > +static void
> > > +ovn_northd_lb_cleanup(struct ovn_northd_lb *lb)
> > >   {
> > >       for (size_t i = 0; i < lb->n_vips; i++) {
> > >           ovn_lb_vip_destroy(&lb->vips[i]);
> > > @@ -747,26 +758,36 @@ ovn_northd_lb_destroy(struct ovn_northd_lb *lb)
> > >       }
> > >       free(lb->vips);
> > >       free(lb->vips_nb);
> > > +    lb->vips = NULL;
> > > +    lb->vips_nb = NULL;
> > >       smap_destroy(&lb->template_vips);
> > >       sset_destroy(&lb->ips_v4);
> > >       sset_destroy(&lb->ips_v6);
> > >       free(lb->selection_fields);
> > > +    lb->selection_fields = NULL;
> > > +    lb->health_checks = false;
> > > +}
> > > +
> > > +void
> > > +ovn_northd_lb_destroy(struct ovn_northd_lb *lb)
> > > +{
> > > +    ovn_northd_lb_cleanup(lb);
> > >       free(lb);
> > >   }
> > >
> > > -/* Constructs a new 'struct ovn_lb_group' object from the Nb LB Group
> record
> > > - * and a hash map of all existing 'struct ovn_northd_lb' objects.
> Space will
> > > - * be allocated for 'max_ls_datapaths' logical switches and
> 'max_lr_datapaths'
> > > - * logical routers to which this LB Group is applied.  Can be filled
> later
> > > - * with ovn_lb_group_add_ls() and ovn_lb_group_add_lr() respectively.
> */
> > > -struct ovn_lb_group *
> > > -ovn_lb_group_create(const struct nbrec_load_balancer_group
> *nbrec_lb_group,
> > > -                    const struct hmap *lbs)
> > > +void
> > > +ovn_northd_lb_reinit(struct ovn_northd_lb *lb,
> > > +                     const struct nbrec_load_balancer *nbrec_lb)
> > >   {
> > > -    struct ovn_lb_group *lb_group;
> > > +    ovn_northd_lb_cleanup(lb);
> > > +    ovn_northd_lb_init(lb, nbrec_lb);
> > > +}
> > >
> > > -    lb_group = xzalloc(sizeof *lb_group);
> > > -    lb_group->uuid = nbrec_lb_group->header_.uuid;
> > > +static void
> > > +ovn_lb_group_init(struct ovn_lb_group *lb_group,
> > > +                  const struct nbrec_load_balancer_group
> *nbrec_lb_group,
> > > +                  const struct hmap *lbs)
> > > +{
> > >       lb_group->n_lbs = nbrec_lb_group->n_load_balancer;
> > >       lb_group->lbs = xmalloc(lb_group->n_lbs * sizeof *lb_group->lbs);
> > >       lb_group->lb_ips = ovn_lb_ip_set_create();
> > > @@ -776,10 +797,30 @@ ovn_lb_group_create(const struct
> nbrec_load_balancer_group *nbrec_lb_group,
> > >               &nbrec_lb_group->load_balancer[i]->header_.uuid;
> > >           lb_group->lbs[i] = ovn_northd_lb_find(lbs, lb_uuid);
> > >       }
> > > +}
> > >
> > > +/* Constructs a new 'struct ovn_lb_group' object from the Nb LB Group
> record
> > > + * and a hash map of all existing 'struct ovn_northd_lb' objects.
> Space will
> > > + * be allocated for 'max_ls_datapaths' logical switches and
> 'max_lr_datapaths'
> > > + * logical routers to which this LB Group is applied.  Can be filled
> later
> > > + * with ovn_lb_group_add_ls() and ovn_lb_group_add_lr() respectively.
> */
> > > +struct ovn_lb_group *
> > > +ovn_lb_group_create(const struct nbrec_load_balancer_group
> *nbrec_lb_group,
> > > +                    const struct hmap *lbs)
> > > +{
> > > +    struct ovn_lb_group *lb_group = xzalloc(sizeof *lb_group);
> > > +    lb_group->uuid = nbrec_lb_group->header_.uuid;
> > > +    ovn_lb_group_init(lb_group, nbrec_lb_group, lbs);
> > >       return lb_group;
> > >   }
> > >
> > > +static void
> > > +ovn_lb_group_cleanup(struct ovn_lb_group *lb_group)
> > > +{
> > > +    ovn_lb_ip_set_destroy(lb_group->lb_ips);
> > > +    free(lb_group->lbs);
> > > +}
> > > +
> > >   void
> > >   ovn_lb_group_destroy(struct ovn_lb_group *lb_group)
> > >   {
> > > @@ -787,11 +828,19 @@ ovn_lb_group_destroy(struct ovn_lb_group
> *lb_group)
> > >           return;
> > >       }
> > >
> > > -    ovn_lb_ip_set_destroy(lb_group->lb_ips);
> > > -    free(lb_group->lbs);
> > > +    ovn_lb_group_cleanup(lb_group);
> > >       free(lb_group);
> > >   }
> > >
> > > +void
> > > +ovn_lb_group_reinit(struct ovn_lb_group *lb_group,
> > > +                    const struct nbrec_load_balancer_group
> *nbrec_lb_group,
> > > +                    const struct hmap *lbs)
> > > +{
> > > +    ovn_lb_group_cleanup(lb_group);
> > > +    ovn_lb_group_init(lb_group, nbrec_lb_group, lbs);
> > > +}
> > > +
> > >   struct ovn_lb_group *
> > >   ovn_lb_group_find(const struct hmap *lb_groups, const struct uuid
> *uuid)
> > >   {
> > > diff --git a/lib/lb.h b/lib/lb.h
> > > index 0339050cba..74905c73b7 100644
> > > --- a/lib/lb.h
> > > +++ b/lib/lb.h
> > > @@ -77,6 +77,9 @@ struct ovn_northd_lb {
> > >
> > >       struct sset ips_v4;
> > >       struct sset ips_v6;
> > > +
> > > +    /* Indicates if the load balancer has health checks configured. */
> > > +    bool health_checks;
> > >   };
> > >
> > >   struct ovn_lb_vip {
> > > @@ -130,6 +133,8 @@ struct ovn_northd_lb *ovn_northd_lb_find(const
> struct hmap *,
> > >                                            const struct uuid *);
> > >   const struct smap *ovn_northd_lb_get_vips(const struct ovn_northd_lb
> *);
> > >   void ovn_northd_lb_destroy(struct ovn_northd_lb *);
> > > +void ovn_northd_lb_reinit(struct ovn_northd_lb *,
> > > +                          const struct nbrec_load_balancer *);
> > >
> > >   void build_lrouter_lb_ips(struct ovn_lb_ip_set *,
> > >                             const struct ovn_northd_lb *);
> > > @@ -148,6 +153,10 @@ struct ovn_lb_group *ovn_lb_group_create(
> > >   void ovn_lb_group_destroy(struct ovn_lb_group *lb_group);
> > >   struct ovn_lb_group *ovn_lb_group_find(const struct hmap *lb_groups,
> > >                                          const struct uuid *);
> > > +void ovn_lb_group_reinit(
> > > +    struct ovn_lb_group *lb_group,
> > > +    const struct nbrec_load_balancer_group *,
> > > +    const struct hmap *lbs);
> > >
> > >   struct ovn_lb_datapaths {
> > >       struct hmap_node hmap_node;
> > > diff --git a/northd/en-northd-lb-data.c b/northd/en-northd-lb-data.c
> > > index d46c3c27ed..e52535b4a9 100644
> > > --- a/northd/en-northd-lb-data.c
> > > +++ b/northd/en-northd-lb-data.c
> > > @@ -37,6 +37,15 @@ static void northd_lb_data_destroy(struct
> northd_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 struct ovn_lb_group *create_lb_group(
> > > +    const struct nbrec_load_balancer_group *, struct hmap *lbs,
> > > +    struct hmap *lb_groups);
> > > +static void destroy_tracked_data(struct northd_lb_data *);
> > > +static void add_lb_to_tracked_data(struct ovn_northd_lb *,
> > > +                                   struct ovs_list *tracked_list,
> > > +                                   bool health_checks);
> > > +static void add_lb_group_to_tracked_data(struct ovn_lb_group *,
> > > +                                         struct ovs_list
> *tracked_list);
> > >
> > >   void *
> > >   en_northd_lb_data_init(struct engine_node *node OVS_UNUSED,
> > > @@ -61,6 +70,7 @@ en_northd_lb_data_run(struct engine_node *node, void
> *data)
> > >       const struct nbrec_load_balancer_group_table *nb_lbg_table =
> > >           EN_OVSDB_GET(engine_get_input("NB_load_balancer_group",
> node));
> > >
> > > +    lb_data->tracked = false;
> > >       build_lbs(nb_lb_table, nb_lbg_table, &lb_data->lbs,
> &lb_data->lb_groups);
> > >       engine_set_node_state(node, EN_UPDATED);
> > >   }
> > > @@ -72,12 +82,122 @@ en_northd_lb_data_cleanup(void *data)
> > >       northd_lb_data_destroy(lb_data);
> > >   }
> > >
> > > +void
> > > +en_northd_lb_data_clear_tracked_data(void *data)
> > > +{
> > > +    struct northd_lb_data *lb_data = (struct northd_lb_data *) data;
> > > +    destroy_tracked_data(lb_data);
> > > +}
> > > +
> > > +
> > > +/* Handler functions. */
> > > +bool
> > > +northd_lb_data_load_balancer_handler(struct engine_node *node,
> > > +                                     void *data OVS_UNUSED)
> > > +{
> > > +    const struct nbrec_load_balancer_table *nb_lb_table =
> > > +        EN_OVSDB_GET(engine_get_input("NB_load_balancer", node));
> > > +
> > > +    struct northd_lb_data *lb_data = (struct northd_lb_data *) data;
> > > +
> > > +    lb_data->tracked = true;
> > > +    struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> > > +
> > > +    const struct nbrec_load_balancer *tracked_lb;
> > > +    NBREC_LOAD_BALANCER_TABLE_FOR_EACH_TRACKED (tracked_lb,
> nb_lb_table) {
> > > +        struct ovn_northd_lb *lb;
> > > +        if (nbrec_load_balancer_is_new(tracked_lb)) {
> > > +            /* New load balancer. */
> > > +            lb = ovn_northd_lb_create(tracked_lb);
> > > +            hmap_insert(&lb_data->lbs, &lb->hmap_node,
> > > +                        uuid_hash(&tracked_lb->header_.uuid));
> > > +            add_lb_to_tracked_data(
> > > +                lb, &trk_lb_data->tracked_updated_lbs.updated,
> > > +                lb->health_checks);
> > > +        } else if (nbrec_load_balancer_is_deleted(tracked_lb)) {
> > > +            lb = ovn_northd_lb_find(&lb_data->lbs,
> > > +                                    &tracked_lb->header_.uuid);
> > > +            ovs_assert(lb);
> > > +            hmap_remove(&lb_data->lbs, &lb->hmap_node);
> > > +            add_lb_to_tracked_data(
> > > +                lb, &trk_lb_data->tracked_deleted_lbs.updated,
> > > +                lb->health_checks);
> > > +        } else {
> > > +            /* Load balancer updated. */
> > > +            lb = ovn_northd_lb_find(&lb_data->lbs,
> > > +                                    &tracked_lb->header_.uuid);
> > > +            ovs_assert(lb);
> > > +            bool health_checks = lb->health_checks;
> > > +            ovn_northd_lb_reinit(lb, tracked_lb);
> > > +            health_checks |= lb->health_checks;
> > > +            add_lb_to_tracked_data(
> > > +                lb, &trk_lb_data->tracked_updated_lbs.updated,
> health_checks);
> > > +        }
> > > +    }
> > > +
> > > +    engine_set_node_state(node, EN_UPDATED);
> > > +    return true;
> > > +}
> > > +
> > > +bool
> > > +northd_lb_data_load_balancer_group_handler(struct engine_node *node,
> > > +                                           void *data OVS_UNUSED)
> > > +{
> > > +    struct northd_lb_data *lb_data = (struct northd_lb_data *) data;
> > > +    const struct nbrec_load_balancer_group_table *nb_lbg_table =
> > > +        EN_OVSDB_GET(engine_get_input("NB_load_balancer_group", node));
> > > +
> > > +    lb_data->tracked = true;
> > > +    struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> > > +    const struct nbrec_load_balancer_group *tracked_lb_group;
> > > +    NBREC_LOAD_BALANCER_GROUP_TABLE_FOR_EACH_TRACKED (tracked_lb_group,
> > > +                                                      nb_lbg_table) {
> > > +        if (nbrec_load_balancer_group_is_new(tracked_lb_group)) {
> > > +            struct ovn_lb_group *lb_group =
> > > +                create_lb_group(tracked_lb_group, &lb_data->lbs,
> > > +                                &lb_data->lb_groups);
> > > +            add_lb_group_to_tracked_data(
> > > +                lb_group,
> &trk_lb_data->tracked_updated_lb_groups.updated);
> > > +        } else if
> (nbrec_load_balancer_group_is_deleted(tracked_lb_group)) {
> > > +            struct ovn_lb_group *lb_group;
> > > +            lb_group = ovn_lb_group_find(&lb_data->lb_groups,
> > > +
> &tracked_lb_group->header_.uuid);
> > > +            ovs_assert(lb_group);
> > > +            hmap_remove(&lb_data->lb_groups, &lb_group->hmap_node);
> > > +            add_lb_group_to_tracked_data(
> > > +                lb_group,
> &trk_lb_data->tracked_deleted_lb_groups.updated);
> > > +        } else if
> (nbrec_load_balancer_group_is_updated(tracked_lb_group,
> > > +
>  NBREC_LOAD_BALANCER_GROUP_COL_LOAD_BALANCER)) {
> > > +
> > > +            struct ovn_lb_group *lb_group;
> > > +            lb_group = ovn_lb_group_find(&lb_data->lb_groups,
> > > +
> &tracked_lb_group->header_.uuid);
> > > +            ovs_assert(lb_group);
> > > +            ovn_lb_group_reinit(lb_group, tracked_lb_group,
> &lb_data->lbs);
> > > +            for (size_t i = 0; i < lb_group->n_lbs; i++) {
> > > +                build_lrouter_lb_ips(lb_group->lb_ips,
> lb_group->lbs[i]);
> > > +            }
> > > +            add_lb_group_to_tracked_data(
> > > +                lb_group,
> &trk_lb_data->tracked_updated_lb_groups.updated);
> > > +        }
> > > +    }
> > > +
> > > +    engine_set_node_state(node, EN_UPDATED);
> > > +    return true;
> > > +}
> > > +
> > >   /* static functions. */
> > >   static void
> > >   northd_lb_data_init(struct northd_lb_data *lb_data)
> > >   {
> > >       hmap_init(&lb_data->lbs);
> > >       hmap_init(&lb_data->lb_groups);
> > > +
> > > +    struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> > > +    ovs_list_init(&trk_lb_data->tracked_updated_lbs.updated);
> > > +    ovs_list_init(&trk_lb_data->tracked_deleted_lbs.updated);
> > > +    ovs_list_init(&trk_lb_data->tracked_updated_lb_groups.updated);
> > > +    ovs_list_init(&trk_lb_data->tracked_deleted_lb_groups.updated);
> > >   }
> > >
> > >   static void
> > > @@ -94,6 +214,8 @@ northd_lb_data_destroy(struct northd_lb_data
> *lb_data)
> > >           ovn_lb_group_destroy(lb_group);
> > >       }
> > >       hmap_destroy(&lb_data->lb_groups);
> > > +
> > > +    destroy_tracked_data(lb_data);
> > >   }
> > >
> > >   static void
> > > @@ -101,12 +223,9 @@ build_lbs(const struct nbrec_load_balancer_table
> *nbrec_load_balancer_table,
> > >             const struct nbrec_load_balancer_group_table
> *nbrec_lb_group_table,
> > >             struct hmap *lbs, struct hmap *lb_groups)
> > >   {
> > > -    struct ovn_lb_group *lb_group;
> > > -    struct ovn_northd_lb *lb_nb;
> > > -
> > >       const struct nbrec_load_balancer *nbrec_lb;
> > >       NBREC_LOAD_BALANCER_TABLE_FOR_EACH (nbrec_lb,
> nbrec_load_balancer_table) {
> > > -        lb_nb = ovn_northd_lb_create(nbrec_lb);
> > > +        struct ovn_northd_lb *lb_nb = ovn_northd_lb_create(nbrec_lb);
> > >           hmap_insert(lbs, &lb_nb->hmap_node,
> > >                       uuid_hash(&nbrec_lb->header_.uuid));
> > >       }
> > > @@ -114,13 +233,76 @@ build_lbs(const struct nbrec_load_balancer_table
> *nbrec_load_balancer_table,
> > >       const struct nbrec_load_balancer_group *nbrec_lb_group;
> > >       NBREC_LOAD_BALANCER_GROUP_TABLE_FOR_EACH (nbrec_lb_group,
> > >                                                 nbrec_lb_group_table) {
> > > -        lb_group = ovn_lb_group_create(nbrec_lb_group, lbs);
> > > +        create_lb_group(nbrec_lb_group, lbs, lb_groups);
> > > +    }
> > > +}
> > >
> > > -        for (size_t i = 0; i < lb_group->n_lbs; i++) {
> > > -            build_lrouter_lb_ips(lb_group->lb_ips, lb_group->lbs[i]);
> > > -        }
> > > +static struct ovn_lb_group *
> > > +create_lb_group(const struct nbrec_load_balancer_group *nbrec_lb_group,
> > > +                struct hmap *lbs, struct hmap *lb_groups)
> > > +{
> > > +    struct ovn_lb_group *lb_group =
> ovn_lb_group_create(nbrec_lb_group, lbs);
> > > +
> > > +    for (size_t i = 0; i < lb_group->n_lbs; i++) {
> > > +        build_lrouter_lb_ips(lb_group->lb_ips, lb_group->lbs[i]);
> > > +    }
> > > +
> > > +    hmap_insert(lb_groups, &lb_group->hmap_node,
> > > +                uuid_hash(&lb_group->uuid));
> > > +
> > > +    return lb_group;
> > > +}
> > > +
> > > +static void
> > > +destroy_tracked_data(struct northd_lb_data *lb_data)
> > > +{
> > > +    lb_data->tracked = false;
> > > +
> > > +    struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> > > +    struct tracked_lb *tracked_lb;
> > > +    LIST_FOR_EACH_SAFE (tracked_lb, list_node,
> > > +                        &trk_lb_data->tracked_updated_lbs.updated) {
> > > +        ovs_list_remove(&tracked_lb->list_node);
> > > +        free(tracked_lb);
> > > +    }
> > > +
> > > +    LIST_FOR_EACH_SAFE (tracked_lb, list_node,
> > > +                        &trk_lb_data->tracked_deleted_lbs.updated) {
> > > +        ovs_list_remove(&tracked_lb->list_node);
> > > +        ovn_northd_lb_destroy(tracked_lb->lb);
> > > +        free(tracked_lb);
> > > +    }
> > > +
> > > +    struct tracked_lb_group *tracked_lb_group;
> > > +    LIST_FOR_EACH_SAFE (tracked_lb_group, list_node,
> > > +
>  &trk_lb_data->tracked_updated_lb_groups.updated) {
> > > +        ovs_list_remove(&tracked_lb_group->list_node);
> > > +        free(tracked_lb_group);
> > > +    }
> > >
> > > -        hmap_insert(lb_groups, &lb_group->hmap_node,
> > > -                    uuid_hash(&lb_group->uuid));
> > > +    LIST_FOR_EACH_SAFE (tracked_lb_group, list_node,
> > > +
>  &trk_lb_data->tracked_deleted_lb_groups.updated) {
> > > +        ovs_list_remove(&tracked_lb_group->list_node);
> > > +        ovn_lb_group_destroy(tracked_lb_group->lb_group);
> > > +        free(tracked_lb_group);
> > >       }
> > >   }
> > > +
> > > +static void
> > > +add_lb_to_tracked_data(struct ovn_northd_lb *lb, struct ovs_list
> *tracked_list,
> > > +                       bool health_checks)
> > > +{
> > > +    struct tracked_lb *t_lb = xzalloc(sizeof *t_lb);
> > > +    t_lb->lb = lb;
> > > +    t_lb->health_checks = health_checks;
> > > +    ovs_list_push_back(tracked_list, &t_lb->list_node);
> > > +}
> > > +
> > > +static void
> > > +add_lb_group_to_tracked_data(struct ovn_lb_group *lb_group,
> > > +                             struct ovs_list *tracked_list)
> > > +{
> > > +    struct tracked_lb_group *t_lb_group = xzalloc(sizeof *t_lb_group);
> > > +    t_lb_group->lb_group = lb_group;
> > > +    ovs_list_push_back(tracked_list, &t_lb_group->list_node);
> > > +}
> > > diff --git a/northd/en-northd-lb-data.h b/northd/en-northd-lb-data.h
> > > index eb297e376d..bb07fcf686 100644
> > > --- a/northd/en-northd-lb-data.h
> > > +++ b/northd/en-northd-lb-data.h
> > > @@ -4,16 +4,53 @@
> > >   #include <config.h>
> > >
> > >   #include "openvswitch/hmap.h"
> > > +#include "include/openvswitch/list.h"
> > >
> > >   #include "lib/inc-proc-eng.h"
> > >
> > > +struct ovn_northd_lb;
> > > +struct ovn_lb_group;
> > > +
> > > +struct tracked_lb {
> > > +    struct ovs_list list_node;
> > > +    struct ovn_northd_lb *lb;
> > > +    bool health_checks;
> > > +};
> > > +
> > > +struct tracked_lb_group {
> > > +    struct ovs_list list_node;
> > > +    struct ovn_lb_group *lb_group;
> > > +};
> > > +
> > > +struct tracked_lb_changes {
> > > +    struct ovs_list updated; /* contains list of 'struct tracked_lb '
> or
> > > +                                list of 'struct tracked_lb_group' */
> > > +};
> > > +
> > > +struct tracked_lb_data {
> > > +    struct tracked_lb_changes tracked_updated_lbs;
> > > +    struct tracked_lb_changes tracked_deleted_lbs;
> > > +    struct tracked_lb_changes tracked_updated_lb_groups;
> > > +    struct tracked_lb_changes tracked_deleted_lb_groups;
> > > +};
> >
> > The typing here is a bit odd. The problem is that struct
> > tracked_lb_changes is being used for two distinct types of lists. I
> > think you should do one of two things here.
> >
> > 1) Get rid of the trakced_lb_changes struct and use ovs_list in-line.
> >
> > struct tracked_lb_data {
> >      struct ovs_list tracked_updated_lbs;
> >      struct ovs_list tracked_deleted_lbs;
> >      struct ovs_list tracked_updated_lb_groups;
> >      struct ovs_list tracked_deleted_lb_groups;
> > };
> >
> > 2) Make a separate type for tracked lbs and tracked lb groups.
> >
> > struct tracked_lb_changes {
> >      struct ovs_list updated;
> > };
> >
> > struct tracked_lb_group_changes {
> >      struct ovs_list updated;
> > };
> >
> > struct tracked_lb_data {
> >      struct tracked_lb_changes tracked_updated_lbs;
> >      struct tracked_lb_changes tracked_deleted_lbs;
> >      struct tracked_lb_group_changes tracked_updated_lb_groups;
> >      struct tracked_lb_group_changes tracked_deleted_lb_groups;
> > };
> >
> > I think option (2) is my preference.
>
> I prefer option (1), because the names of the fields in struct
> tracked_lb_data already tells the purpose, and the only member "update" in
> the struct tracked_lb_changes is unnecessary. In addition, the name
> "update" is confusing, because it is in fact used for both "updated"
> (added/updated) and "deleted".

How about the below one ?

-----------------------------------------
struct tracked_lb_data {
/* Both created and updated lbs. Contains hmapx of 'struct ovn_northd_lb' */
struct hmapx crupdated_lbs;
struct hmapx deleted_lbs;

/* Both created and updated lb groups. Contains hmapx of
* 'struct ovn_lb_group' */
struct hmapx crupdated_lb_groups; /* Both created and updated lb groups. */
struct hmapx deleted_lb_groups;

bool has_health_checks;
};
-----------------------------------------

I just googled a proper word for create and update and came across
"upsert" and "crupdate" in the database context.
I prefer the latter.  So went with it.

Since northd engine will be handling both the created and updated
lbs/lb groups, I think having just one is sufficient.

Also I'm changing from a list to hmapx because in handling load
balancer groups I want to also update the "crupdated_lbs"
with the load balancers in the load balancer groups as this is
required later when handling lb group changes for lflow_engine.


Please let me know if there are any comments/objections.

Thanks
Numan


>
>
> >
> > > +
> > >   struct northd_lb_data {
> > >       struct hmap lbs;
> > >       struct hmap lb_groups;
> > > +
> > > +    /* tracked data*/
> > > +    bool tracked;
> > > +    struct tracked_lb_data tracked_lb_data;
> > >   };
> > >
> > >   void *en_northd_lb_data_init(struct engine_node *, struct engine_arg
> *);
> > >   void en_northd_lb_data_run(struct engine_node *, void *data);
> > >   void en_northd_lb_data_cleanup(void *data);
> > > +void en_northd_lb_data_clear_tracked_data(void *data);
> > > +
> > > +bool northd_lb_data_load_balancer_handler(struct engine_node *,
> > > +                                          void *data);
> > > +bool northd_lb_data_load_balancer_group_handler(struct engine_node *,
> > > +                                                void *data);
> > >
> > >   #endif /* end of EN_NORTHD_LB_DATA_H */
> > > diff --git a/northd/en-northd.c b/northd/en-northd.c
> > > index cc7d838451..b3c03c54bd 100644
> > > --- a/northd/en-northd.c
> > > +++ b/northd/en-northd.c
> > > @@ -206,6 +206,30 @@ northd_sb_port_binding_handler(struct engine_node
> *node,
> > >       return true;
> > >   }
> > >
> > > +bool
> > > +northd_northd_lb_data_handler(struct engine_node *node, void *data)
> > > +{
> > > +    struct northd_lb_data *lb_data =
> > > +        engine_get_input_data("northd_lb_data", node);
> > > +
> > > +    if (!lb_data->tracked) {
> > > +        return false;
> > > +    }
> > > +
> > > +    struct northd_data *nd = data;
> > > +
> > > +    if (!northd_handle_lb_data_changes(&lb_data->tracked_lb_data,
> > > +                                       &nd->ls_datapaths,
> > > +                                       &nd->lr_datapaths,
> > > +                                       &nd->lb_datapaths_map,
> > > +                                       &nd->lb_group_datapaths_map)) {
> > > +        return false;
> > > +    }
> > > +
> > > +    engine_set_node_state(node, EN_UPDATED);
> > > +    return true;
> > > +}
> > > +
> > >   void
> > >   *en_northd_init(struct engine_node *node OVS_UNUSED,
> > >                   struct engine_arg *arg OVS_UNUSED)
> > > diff --git a/northd/en-northd.h b/northd/en-northd.h
> > > index 20cc77f108..5674f4390c 100644
> > > --- a/northd/en-northd.h
> > > +++ b/northd/en-northd.h
> > > @@ -17,5 +17,6 @@ void en_northd_clear_tracked_data(void *data);
> > >   bool northd_nb_nb_global_handler(struct engine_node *, void *data
> OVS_UNUSED);
> > >   bool northd_nb_logical_switch_handler(struct engine_node *, void
> *data);
> > >   bool northd_sb_port_binding_handler(struct engine_node *, void *data);
> > > +bool northd_northd_lb_data_handler(struct engine_node *, void *data);
> > >
> > >   #endif /* EN_NORTHD_H */
> > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> > > index b2e884962f..b444a488db 100644
> > > --- a/northd/inc-proc-northd.c
> > > +++ b/northd/inc-proc-northd.c
> > > @@ -141,13 +141,18 @@ static ENGINE_NODE(sync_to_sb_addr_set,
> "sync_to_sb_addr_set");
> > >   static ENGINE_NODE(fdb_aging, "fdb_aging");
> > >   static ENGINE_NODE(fdb_aging_waker, "fdb_aging_waker");
> > >   static ENGINE_NODE(sync_to_sb_lb, "sync_to_sb_lb");
> > > -static ENGINE_NODE(northd_lb_data, "northd_lb_data");
> > > +static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(northd_lb_data,
> "northd_lb_data");
> > >
> > >   void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> > >                             struct ovsdb_idl_loop *sb)
> > >   {
> > >       /* Define relationships between nodes where first argument is
> dependent
> > >        * on the second argument */
> > > +    engine_add_input(&en_northd_lb_data, &en_nb_load_balancer,
> > > +                     northd_lb_data_load_balancer_handler);
> > > +    engine_add_input(&en_northd_lb_data, &en_nb_load_balancer_group,
> > > +                     northd_lb_data_load_balancer_group_handler);
> > > +
>
> Maybe it is not a big deal, but I think it is better to move the
> dependencies that have handlers after the ones that don't, so that any
> changes that will trigger recompute are handled first, to avoid wasting
> time handling other changes and then fallback to recompute.
>
> With these minor comments addressed:
> Acked-by: Han Zhou <hzhou@ovn.org>
>
> > >       engine_add_input(&en_northd, &en_nb_port_group, NULL);
> > >       engine_add_input(&en_northd, &en_nb_acl, NULL);
> > >       engine_add_input(&en_northd, &en_nb_logical_router, NULL);
> > > @@ -171,6 +176,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> > >       engine_add_input(&en_northd, &en_sb_static_mac_binding, NULL);
> > >       engine_add_input(&en_northd, &en_sb_chassis_template_var, NULL);
> > >
> > > +    engine_add_input(&en_northd, &en_northd_lb_data,
> > > +                     northd_northd_lb_data_handler);
> > >       engine_add_input(&en_northd, &en_sb_port_binding,
> > >                        northd_sb_port_binding_handler);
> > >       engine_add_input(&en_northd, &en_nb_nb_global,
> > > @@ -178,10 +185,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop
> *nb,
> > >       engine_add_input(&en_northd, &en_nb_logical_switch,
> > >                        northd_nb_logical_switch_handler);
> > >
> > > -    engine_add_input(&en_northd_lb_data, &en_nb_load_balancer, NULL);
> > > -    engine_add_input(&en_northd_lb_data, &en_nb_load_balancer_group,
> NULL);
> > > -    engine_add_input(&en_northd, &en_northd_lb_data, NULL);
> > > -
> > >       engine_add_input(&en_mac_binding_aging, &en_nb_nb_global, NULL);
> > >       engine_add_input(&en_mac_binding_aging, &en_sb_mac_binding, NULL);
> > >       engine_add_input(&en_mac_binding_aging, &en_northd, NULL);
> > > diff --git a/northd/northd.c b/northd/northd.c
> > > index 890186b29c..f27f0de49b 100644
> > > --- a/northd/northd.c
> > > +++ b/northd/northd.c
> > > @@ -40,6 +40,7 @@
> > >   #include "lib/lb.h"
> > >   #include "memory.h"
> > >   #include "northd.h"
> > > +#include "en-northd-lb-data.h"
> > >   #include "lib/ovn-parallel-hmap.h"
> > >   #include "ovn/actions.h"
> > >   #include "ovn/features.h"
> > > @@ -5323,6 +5324,82 @@ northd_handle_sb_port_binding_changes(
> > >       return true;
> > >   }
> > >
> > > +bool
> > > +northd_handle_lb_data_changes(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 tracked_lb *trk_lb;
> > > +
> > > +    struct ovn_lb_datapaths *lb_dps;
> > > +    LIST_FOR_EACH (trk_lb, list_node,
> > > +                   &trk_lb_data->tracked_deleted_lbs.updated) {
> > > +        if (trk_lb->health_checks) {
> > > +            /* Fall back to recompute if the deleted load balancer has
> > > +             * health checks configured. */
> > > +            return false;
> > > +        }
> > > +
> > > +        const struct uuid *lb_uuid =
> > > +                &trk_lb->lb->nlb->header_.uuid;
> > > +
> > > +        lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> > > +        ovs_assert(lb_dps);
> > > +        hmap_remove(lb_datapaths_map, &lb_dps->hmap_node);
> > > +        ovn_lb_datapaths_destroy(lb_dps);
> > > +    }
> > > +
> > > +    LIST_FOR_EACH (trk_lb, list_node,
> > > +                   &trk_lb_data->tracked_updated_lbs.updated) {
> > > +        if (trk_lb->health_checks) {
> > > +            /* Fall back to recompute if the created/updated load
> balancer has
> > > +             * health checks configured. */
> > > +            return false;
> > > +        }
> > > +
> > > +        const struct uuid *lb_uuid =
> > > +                &trk_lb->lb->nlb->header_.uuid;
> > > +        lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> > > +        if (!lb_dps) {
> > > +            lb_dps = ovn_lb_datapaths_create(trk_lb->lb,
> > > +                                             ods_size(ls_datapaths),
> > > +                                             ods_size(lr_datapaths));
> > > +            hmap_insert(lb_datapaths_map, &lb_dps->hmap_node,
> > > +                        uuid_hash(lb_uuid));
> > > +        }
> > > +    }
> > > +
> > > +    struct ovn_lb_group_datapaths *lb_group_dps;
> > > +    struct tracked_lb_group *trk_lb_group;
> > > +    LIST_FOR_EACH_SAFE (trk_lb_group, list_node,
> > > +
>  &trk_lb_data->tracked_deleted_lb_groups.updated) {
> > > +        const struct uuid *lb_uuid = &trk_lb_group->lb_group->uuid;
> > > +        lb_group_dps =
> ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > > +                                                   lb_uuid);
> > > +        ovs_assert(lb_group_dps);
> > > +        hmap_remove(lb_group_datapaths_map, &lb_group_dps->hmap_node);
> > > +        ovn_lb_group_datapaths_destroy(lb_group_dps);
> > > +    }
> > > +
> > > +    LIST_FOR_EACH_SAFE (trk_lb_group, list_node,
> > > +
>  &trk_lb_data->tracked_updated_lb_groups.updated) {
> > > +        const struct uuid *lb_uuid = &trk_lb_group->lb_group->uuid;
> > > +        lb_group_dps =
> ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > > +                                                   lb_uuid);
> > > +        if (!lb_group_dps) {
> > > +            lb_group_dps = ovn_lb_group_datapaths_create(
> > > +                trk_lb_group->lb_group, ods_size(ls_datapaths),
> > > +                ods_size(lr_datapaths));
> > > +            hmap_insert(lb_group_datapaths_map,
> &lb_group_dps->hmap_node,
> > > +                        uuid_hash(lb_uuid));
> > > +        }
> > > +    }
> > > +
> > > +    return true;
> > > +}
> > > +
> > >   struct multicast_group {
> > >       const char *name;
> > >       uint16_t key;               /*
> OVN_MIN_MULTICAST...OVN_MAX_MULTICAST. */
> > > diff --git a/northd/northd.h b/northd/northd.h
> > > index 7d92028c7d..7d17921fa2 100644
> > > --- a/northd/northd.h
> > > +++ b/northd/northd.h
> > > @@ -347,6 +347,13 @@ bool lflow_handle_northd_ls_changes(struct
> ovsdb_idl_txn *ovnsb_txn,
> > >   bool northd_handle_sb_port_binding_changes(
> > >       const struct sbrec_port_binding_table *, struct hmap *ls_ports);
> > >
> > > +struct tracked_lb_data;
> > > +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);
> > > +
> > >   void build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
> > >                        const struct nbrec_bfd_table *,
> > >                        const struct sbrec_bfd_table *,
> > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > > index e79d33b2ae..dd0bd8b36a 100644
> > > --- a/tests/ovn-northd.at
> > > +++ b/tests/ovn-northd.at
> > > @@ -9681,3 +9681,126 @@ AT_CHECK([grep "lr_in_gw_redirect" R1flows |sed
> s'/table=../table=??/' |sort], [
> > >
> > >   AT_CLEANUP
> > >   ])
> > > +
> > > +OVN_FOR_EACH_NORTHD_NO_HV([
> > > +AT_SETUP([Load balancer incremental processing])
> > > +ovn_start
> > > +
> > > +check_engine_stats() {
> > > +  northd_comp=$1
> > > +  lflow_comp=$2
> > > +
> > > +  AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats
> northd_lb_data recompute], [0], [0
> > > +])
> > > +
> > > +  northd_recompute_ct=$(as northd ovn-appctl -t NORTHD_TYPE
> inc-engine/show-stats northd recompute)
> > > +  if [[ "$northd_comp" == "norecompute" ]]; then
> > > +    check test "$northd_recompute_ct" -eq "0"
> > > +  else
> > > +    check test "$northd_recompute_ct" -ne "0"
> > > +  fi
> > > +
> > > +  lflow_recompute_ct=$(as northd ovn-appctl -t NORTHD_TYPE
> inc-engine/show-stats lflow recompute)
> > > +  if [[ "$lflow_comp" == "norecompute" ]]; then
> > > +    check test "$lflow_recompute_ct" -eq "0"
> > > +  else
> > > +    check test "$lflow_recompute_ct" -ne "0"
> > > +  fi
> > > +}
> > > +
> > > +# Test I-P for load balancers.
> > > +# Presently ovn-northd handles I-P for NB LBs in northd_lb_data engine
> node
> > > +# only.
> > > +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 norecompute recompute
> > > +
> > > +check ovn-nbctl --wait=sb set load_balancer .
> ip_port_mappings:10.0.0.3=sw0-p1:10.0.0.2
> > > +check_engine_stats norecompute recompute
> > > +
> > > +check ovn-nbctl --wait=sb set load_balancer . options:foo=bar
> > > +check_engine_stats norecompute recompute
> > > +
> > > +check ovn-nbctl --wait=sb -- lb-add lb2 20.0.0.10:80 20.0.0.20:80 --
> lb-add lb3 30.0.0.10:80 30.0.0.20:80
> > > +check_engine_stats norecompute recompute
> > > +
> > > +check ovn-nbctl --wait=sb -- lb-del lb2 -- lb-del lb3
> > > +check_engine_stats norecompute recompute
> > > +
> > > +AT_CHECK([ovn-nbctl --wait=sb \
> > > +          -- --id=@hc create Load_Balancer_Health_Check
> vip="10.0.0.10\:80" \
> > > +             options:failure_count=100 \
> > > +          -- add Load_Balancer . health_check @hc | uuidfilt], [0],
> [<0>
> > > +])
> > > +check_engine_stats recompute recompute
> > > +
> > > +# Any change to load balancer health check should also result in full
> recompute
> > > +# of northd node (but not northd_lb_data node)
> > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > +check ovn-nbctl --wait=sb set load_balancer_health_check .
> options:foo=bar1
> > > +check_engine_stats recompute recompute
> > > +
> > > +# Delete the health check from the load balancer.  northd engine node
> should do a full recompute.
> > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > +check ovn-nbctl --wait=sb clear Load_Balancer . health_check
> > > +check_engine_stats recompute recompute
> > > +
> > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > +check ovn-nbctl ls-add sw0
> > > +check ovn-nbctl --wait=sb lr-add lr0
> > > +check_engine_stats recompute recompute
> > > +
> > > +# Associate lb1 to 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-add sw0 lb1
> > > +check_engine_stats recompute 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 recompute recompute
> > > +
> > > +# Test load balancer group now
> > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > +lbg1_uuid=$(ovn-nbctl create load_balancer_group name=lbg1)
> > > +check_engine_stats norecompute recompute
> > > +
> > > +lb1_uuid=$(fetch_column nb:Load_Balancer _uuid)
> > > +
> > > +# Add lb to the lbg1 group
> > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > +check ovn-nbctl add load_balancer_group . load_Balancer $lb1_uuid
> > > +check_engine_stats norecompute recompute
> > > +
> > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > +check ovn-nbctl clear load_balancer_group . load_Balancer
> > > +check_engine_stats norecompute recompute
> > > +
> > > +# Add back lb to the lbg1 group
> > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > +check ovn-nbctl add load_balancer_group . load_Balancer $lb1_uuid
> > > +check_engine_stats norecompute 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 recompute recompute
> > > +
> > > +# Update lb and this should result in 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 recompute 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 recompute recompute
> > > +
> > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > +check ovn-nbctl add logical_router lr0 load_balancer_group $lbg1_uuid
> > > +check_engine_stats recompute recompute
> > > +
> > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > +check ovn-nbctl clear logical_router lr0 load_balancer_group
> > > +check_engine_stats recompute recompute
> > > +
> > > +AT_CLEANUP
> > > +])
> >
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On Fri, Jul 14, 2023 at 6:54 PM Numan Siddique <numans@ovn.org> wrote:
>
> On Wed, Jul 12, 2023 at 9:26 PM Han Zhou <zhouhan@gmail.com> wrote:
> >
> > On Sat, Jul 8, 2023 at 3:57 AM Mark Michelson <mmichels@redhat.com> wrote:
> > >
> > > Hi Numan,
> > >
> > > I have one small nit below.
> > >
> > > On 7/7/23 01:53, numans@ovn.org wrote:
> > > > From: Numan Siddique <numans@ovn.org>
> > > >
> > > > Any changes to load balancers and load balancer groups
> > > > are handled incrementally in the newly added 'northd_lb_data'
> > > > engine node.  'northd_lb_data' is input to 'northd' node
> > > > and the handler - northd_northd_lb_data_handler in 'northd'
> > > > node handles the changes.
> > > >
> > > > If a load balancer or load balancer group is associated to
> > > > a logical switch or router then 'northd' node falls back
> > > > to full recompute.
> > > >
> > > > Signed-off-by: Numan Siddique <numans@ovn.org>
> > > > ---
> > > >   lib/lb.c                   |  85 ++++++++++++----
> > > >   lib/lb.h                   |   9 ++
> > > >   northd/en-northd-lb-data.c | 202 +++++++++++++++++++++++++++++++++++--
> > > >   northd/en-northd-lb-data.h |  37 +++++++
> > > >   northd/en-northd.c         |  24 +++++
> > > >   northd/en-northd.h         |   1 +
> > > >   northd/inc-proc-northd.c   |  13 ++-
> > > >   northd/northd.c            |  77 ++++++++++++++
> > > >   northd/northd.h            |   7 ++
> > > >   tests/ovn-northd.at        | 123 ++++++++++++++++++++++
> > > >   10 files changed, 545 insertions(+), 33 deletions(-)
> > > >
> > > > diff --git a/lib/lb.c b/lib/lb.c
> > > > index 429dbf15af..a51fe225fa 100644
> > > > --- a/lib/lb.c
> > > > +++ b/lib/lb.c
> > > > @@ -606,13 +606,13 @@ ovn_lb_get_health_check(const struct
> > nbrec_load_balancer *nbrec_lb,
> > > >       return NULL;
> > > >   }
> > > >
> > > > -struct ovn_northd_lb *
> > > > -ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
> > > > +static void
> > > > +ovn_northd_lb_init(struct ovn_northd_lb *lb,
> > > > +                   const struct nbrec_load_balancer *nbrec_lb)
> > > >   {
> > > >       bool template = smap_get_bool(&nbrec_lb->options, "template",
> > false);
> > > >       bool is_udp = nullable_string_is_equal(nbrec_lb->protocol, "udp");
> > > >       bool is_sctp = nullable_string_is_equal(nbrec_lb->protocol,
> > "sctp");
> > > > -    struct ovn_northd_lb *lb = xzalloc(sizeof *lb);
> > > >       int address_family = !strcmp(smap_get_def(&nbrec_lb->options,
> > > >                                                 "address-family",
> > "ipv4"),
> > > >                                    "ipv4")
> > > > @@ -668,6 +668,10 @@ ovn_northd_lb_create(const struct
> > nbrec_load_balancer *nbrec_lb)
> > > >                                                     "reject", false);
> > > >           ovn_northd_lb_vip_init(lb_vip_nb, lb_vip, nbrec_lb,
> > > >                                  node->key, node->value, template);
> > > > +        if (lb_vip_nb->lb_health_check) {
> > > > +            lb->health_checks = true;
> > > > +        }
> > > > +
> > > >           if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
> > > >               sset_add(&lb->ips_v4, lb_vip->vip_str);
> > > >           } else {
> > > > @@ -713,6 +717,13 @@ ovn_northd_lb_create(const struct
> > nbrec_load_balancer *nbrec_lb)
> > > >           ds_chomp(&sel_fields, ',');
> > > >           lb->selection_fields = ds_steal_cstr(&sel_fields);
> > > >       }
> > > > +}
> > > > +
> > > > +struct ovn_northd_lb *
> > > > +ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
> > > > +{
> > > > +    struct ovn_northd_lb *lb = xzalloc(sizeof *lb);
> > > > +    ovn_northd_lb_init(lb, nbrec_lb);
> > > >       return lb;
> > > >   }
> > > >
> > > > @@ -738,8 +749,8 @@ ovn_northd_lb_get_vips(const struct ovn_northd_lb
> > *lb)
> > > >       return &lb->nlb->vips;
> > > >   }
> > > >
> > > > -void
> > > > -ovn_northd_lb_destroy(struct ovn_northd_lb *lb)
> > > > +static void
> > > > +ovn_northd_lb_cleanup(struct ovn_northd_lb *lb)
> > > >   {
> > > >       for (size_t i = 0; i < lb->n_vips; i++) {
> > > >           ovn_lb_vip_destroy(&lb->vips[i]);
> > > > @@ -747,26 +758,36 @@ ovn_northd_lb_destroy(struct ovn_northd_lb *lb)
> > > >       }
> > > >       free(lb->vips);
> > > >       free(lb->vips_nb);
> > > > +    lb->vips = NULL;
> > > > +    lb->vips_nb = NULL;
> > > >       smap_destroy(&lb->template_vips);
> > > >       sset_destroy(&lb->ips_v4);
> > > >       sset_destroy(&lb->ips_v6);
> > > >       free(lb->selection_fields);
> > > > +    lb->selection_fields = NULL;
> > > > +    lb->health_checks = false;
> > > > +}
> > > > +
> > > > +void
> > > > +ovn_northd_lb_destroy(struct ovn_northd_lb *lb)
> > > > +{
> > > > +    ovn_northd_lb_cleanup(lb);
> > > >       free(lb);
> > > >   }
> > > >
> > > > -/* Constructs a new 'struct ovn_lb_group' object from the Nb LB Group
> > record
> > > > - * and a hash map of all existing 'struct ovn_northd_lb' objects.
> > Space will
> > > > - * be allocated for 'max_ls_datapaths' logical switches and
> > 'max_lr_datapaths'
> > > > - * logical routers to which this LB Group is applied.  Can be filled
> > later
> > > > - * with ovn_lb_group_add_ls() and ovn_lb_group_add_lr() respectively.
> > */
> > > > -struct ovn_lb_group *
> > > > -ovn_lb_group_create(const struct nbrec_load_balancer_group
> > *nbrec_lb_group,
> > > > -                    const struct hmap *lbs)
> > > > +void
> > > > +ovn_northd_lb_reinit(struct ovn_northd_lb *lb,
> > > > +                     const struct nbrec_load_balancer *nbrec_lb)
> > > >   {
> > > > -    struct ovn_lb_group *lb_group;
> > > > +    ovn_northd_lb_cleanup(lb);
> > > > +    ovn_northd_lb_init(lb, nbrec_lb);
> > > > +}
> > > >
> > > > -    lb_group = xzalloc(sizeof *lb_group);
> > > > -    lb_group->uuid = nbrec_lb_group->header_.uuid;
> > > > +static void
> > > > +ovn_lb_group_init(struct ovn_lb_group *lb_group,
> > > > +                  const struct nbrec_load_balancer_group
> > *nbrec_lb_group,
> > > > +                  const struct hmap *lbs)
> > > > +{
> > > >       lb_group->n_lbs = nbrec_lb_group->n_load_balancer;
> > > >       lb_group->lbs = xmalloc(lb_group->n_lbs * sizeof *lb_group->lbs);
> > > >       lb_group->lb_ips = ovn_lb_ip_set_create();
> > > > @@ -776,10 +797,30 @@ ovn_lb_group_create(const struct
> > nbrec_load_balancer_group *nbrec_lb_group,
> > > >               &nbrec_lb_group->load_balancer[i]->header_.uuid;
> > > >           lb_group->lbs[i] = ovn_northd_lb_find(lbs, lb_uuid);
> > > >       }
> > > > +}
> > > >
> > > > +/* Constructs a new 'struct ovn_lb_group' object from the Nb LB Group
> > record
> > > > + * and a hash map of all existing 'struct ovn_northd_lb' objects.
> > Space will
> > > > + * be allocated for 'max_ls_datapaths' logical switches and
> > 'max_lr_datapaths'
> > > > + * logical routers to which this LB Group is applied.  Can be filled
> > later
> > > > + * with ovn_lb_group_add_ls() and ovn_lb_group_add_lr() respectively.
> > */
> > > > +struct ovn_lb_group *
> > > > +ovn_lb_group_create(const struct nbrec_load_balancer_group
> > *nbrec_lb_group,
> > > > +                    const struct hmap *lbs)
> > > > +{
> > > > +    struct ovn_lb_group *lb_group = xzalloc(sizeof *lb_group);
> > > > +    lb_group->uuid = nbrec_lb_group->header_.uuid;
> > > > +    ovn_lb_group_init(lb_group, nbrec_lb_group, lbs);
> > > >       return lb_group;
> > > >   }
> > > >
> > > > +static void
> > > > +ovn_lb_group_cleanup(struct ovn_lb_group *lb_group)
> > > > +{
> > > > +    ovn_lb_ip_set_destroy(lb_group->lb_ips);
> > > > +    free(lb_group->lbs);
> > > > +}
> > > > +
> > > >   void
> > > >   ovn_lb_group_destroy(struct ovn_lb_group *lb_group)
> > > >   {
> > > > @@ -787,11 +828,19 @@ ovn_lb_group_destroy(struct ovn_lb_group
> > *lb_group)
> > > >           return;
> > > >       }
> > > >
> > > > -    ovn_lb_ip_set_destroy(lb_group->lb_ips);
> > > > -    free(lb_group->lbs);
> > > > +    ovn_lb_group_cleanup(lb_group);
> > > >       free(lb_group);
> > > >   }
> > > >
> > > > +void
> > > > +ovn_lb_group_reinit(struct ovn_lb_group *lb_group,
> > > > +                    const struct nbrec_load_balancer_group
> > *nbrec_lb_group,
> > > > +                    const struct hmap *lbs)
> > > > +{
> > > > +    ovn_lb_group_cleanup(lb_group);
> > > > +    ovn_lb_group_init(lb_group, nbrec_lb_group, lbs);
> > > > +}
> > > > +
> > > >   struct ovn_lb_group *
> > > >   ovn_lb_group_find(const struct hmap *lb_groups, const struct uuid
> > *uuid)
> > > >   {
> > > > diff --git a/lib/lb.h b/lib/lb.h
> > > > index 0339050cba..74905c73b7 100644
> > > > --- a/lib/lb.h
> > > > +++ b/lib/lb.h
> > > > @@ -77,6 +77,9 @@ struct ovn_northd_lb {
> > > >
> > > >       struct sset ips_v4;
> > > >       struct sset ips_v6;
> > > > +
> > > > +    /* Indicates if the load balancer has health checks configured. */
> > > > +    bool health_checks;
> > > >   };
> > > >
> > > >   struct ovn_lb_vip {
> > > > @@ -130,6 +133,8 @@ struct ovn_northd_lb *ovn_northd_lb_find(const
> > struct hmap *,
> > > >                                            const struct uuid *);
> > > >   const struct smap *ovn_northd_lb_get_vips(const struct ovn_northd_lb
> > *);
> > > >   void ovn_northd_lb_destroy(struct ovn_northd_lb *);
> > > > +void ovn_northd_lb_reinit(struct ovn_northd_lb *,
> > > > +                          const struct nbrec_load_balancer *);
> > > >
> > > >   void build_lrouter_lb_ips(struct ovn_lb_ip_set *,
> > > >                             const struct ovn_northd_lb *);
> > > > @@ -148,6 +153,10 @@ struct ovn_lb_group *ovn_lb_group_create(
> > > >   void ovn_lb_group_destroy(struct ovn_lb_group *lb_group);
> > > >   struct ovn_lb_group *ovn_lb_group_find(const struct hmap *lb_groups,
> > > >                                          const struct uuid *);
> > > > +void ovn_lb_group_reinit(
> > > > +    struct ovn_lb_group *lb_group,
> > > > +    const struct nbrec_load_balancer_group *,
> > > > +    const struct hmap *lbs);
> > > >
> > > >   struct ovn_lb_datapaths {
> > > >       struct hmap_node hmap_node;
> > > > diff --git a/northd/en-northd-lb-data.c b/northd/en-northd-lb-data.c
> > > > index d46c3c27ed..e52535b4a9 100644
> > > > --- a/northd/en-northd-lb-data.c
> > > > +++ b/northd/en-northd-lb-data.c
> > > > @@ -37,6 +37,15 @@ static void northd_lb_data_destroy(struct
> > northd_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 struct ovn_lb_group *create_lb_group(
> > > > +    const struct nbrec_load_balancer_group *, struct hmap *lbs,
> > > > +    struct hmap *lb_groups);
> > > > +static void destroy_tracked_data(struct northd_lb_data *);
> > > > +static void add_lb_to_tracked_data(struct ovn_northd_lb *,
> > > > +                                   struct ovs_list *tracked_list,
> > > > +                                   bool health_checks);
> > > > +static void add_lb_group_to_tracked_data(struct ovn_lb_group *,
> > > > +                                         struct ovs_list
> > *tracked_list);
> > > >
> > > >   void *
> > > >   en_northd_lb_data_init(struct engine_node *node OVS_UNUSED,
> > > > @@ -61,6 +70,7 @@ en_northd_lb_data_run(struct engine_node *node, void
> > *data)
> > > >       const struct nbrec_load_balancer_group_table *nb_lbg_table =
> > > >           EN_OVSDB_GET(engine_get_input("NB_load_balancer_group",
> > node));
> > > >
> > > > +    lb_data->tracked = false;
> > > >       build_lbs(nb_lb_table, nb_lbg_table, &lb_data->lbs,
> > &lb_data->lb_groups);
> > > >       engine_set_node_state(node, EN_UPDATED);
> > > >   }
> > > > @@ -72,12 +82,122 @@ en_northd_lb_data_cleanup(void *data)
> > > >       northd_lb_data_destroy(lb_data);
> > > >   }
> > > >
> > > > +void
> > > > +en_northd_lb_data_clear_tracked_data(void *data)
> > > > +{
> > > > +    struct northd_lb_data *lb_data = (struct northd_lb_data *) data;
> > > > +    destroy_tracked_data(lb_data);
> > > > +}
> > > > +
> > > > +
> > > > +/* Handler functions. */
> > > > +bool
> > > > +northd_lb_data_load_balancer_handler(struct engine_node *node,
> > > > +                                     void *data OVS_UNUSED)
> > > > +{
> > > > +    const struct nbrec_load_balancer_table *nb_lb_table =
> > > > +        EN_OVSDB_GET(engine_get_input("NB_load_balancer", node));
> > > > +
> > > > +    struct northd_lb_data *lb_data = (struct northd_lb_data *) data;
> > > > +
> > > > +    lb_data->tracked = true;
> > > > +    struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> > > > +
> > > > +    const struct nbrec_load_balancer *tracked_lb;
> > > > +    NBREC_LOAD_BALANCER_TABLE_FOR_EACH_TRACKED (tracked_lb,
> > nb_lb_table) {
> > > > +        struct ovn_northd_lb *lb;
> > > > +        if (nbrec_load_balancer_is_new(tracked_lb)) {
> > > > +            /* New load balancer. */
> > > > +            lb = ovn_northd_lb_create(tracked_lb);
> > > > +            hmap_insert(&lb_data->lbs, &lb->hmap_node,
> > > > +                        uuid_hash(&tracked_lb->header_.uuid));
> > > > +            add_lb_to_tracked_data(
> > > > +                lb, &trk_lb_data->tracked_updated_lbs.updated,
> > > > +                lb->health_checks);
> > > > +        } else if (nbrec_load_balancer_is_deleted(tracked_lb)) {
> > > > +            lb = ovn_northd_lb_find(&lb_data->lbs,
> > > > +                                    &tracked_lb->header_.uuid);
> > > > +            ovs_assert(lb);
> > > > +            hmap_remove(&lb_data->lbs, &lb->hmap_node);
> > > > +            add_lb_to_tracked_data(
> > > > +                lb, &trk_lb_data->tracked_deleted_lbs.updated,
> > > > +                lb->health_checks);
> > > > +        } else {
> > > > +            /* Load balancer updated. */
> > > > +            lb = ovn_northd_lb_find(&lb_data->lbs,
> > > > +                                    &tracked_lb->header_.uuid);
> > > > +            ovs_assert(lb);
> > > > +            bool health_checks = lb->health_checks;
> > > > +            ovn_northd_lb_reinit(lb, tracked_lb);
> > > > +            health_checks |= lb->health_checks;
> > > > +            add_lb_to_tracked_data(
> > > > +                lb, &trk_lb_data->tracked_updated_lbs.updated,
> > health_checks);
> > > > +        }
> > > > +    }
> > > > +
> > > > +    engine_set_node_state(node, EN_UPDATED);
> > > > +    return true;
> > > > +}
> > > > +
> > > > +bool
> > > > +northd_lb_data_load_balancer_group_handler(struct engine_node *node,
> > > > +                                           void *data OVS_UNUSED)
> > > > +{
> > > > +    struct northd_lb_data *lb_data = (struct northd_lb_data *) data;
> > > > +    const struct nbrec_load_balancer_group_table *nb_lbg_table =
> > > > +        EN_OVSDB_GET(engine_get_input("NB_load_balancer_group", node));
> > > > +
> > > > +    lb_data->tracked = true;
> > > > +    struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> > > > +    const struct nbrec_load_balancer_group *tracked_lb_group;
> > > > +    NBREC_LOAD_BALANCER_GROUP_TABLE_FOR_EACH_TRACKED (tracked_lb_group,
> > > > +                                                      nb_lbg_table) {
> > > > +        if (nbrec_load_balancer_group_is_new(tracked_lb_group)) {
> > > > +            struct ovn_lb_group *lb_group =
> > > > +                create_lb_group(tracked_lb_group, &lb_data->lbs,
> > > > +                                &lb_data->lb_groups);
> > > > +            add_lb_group_to_tracked_data(
> > > > +                lb_group,
> > &trk_lb_data->tracked_updated_lb_groups.updated);
> > > > +        } else if
> > (nbrec_load_balancer_group_is_deleted(tracked_lb_group)) {
> > > > +            struct ovn_lb_group *lb_group;
> > > > +            lb_group = ovn_lb_group_find(&lb_data->lb_groups,
> > > > +
> > &tracked_lb_group->header_.uuid);
> > > > +            ovs_assert(lb_group);
> > > > +            hmap_remove(&lb_data->lb_groups, &lb_group->hmap_node);
> > > > +            add_lb_group_to_tracked_data(
> > > > +                lb_group,
> > &trk_lb_data->tracked_deleted_lb_groups.updated);
> > > > +        } else if
> > (nbrec_load_balancer_group_is_updated(tracked_lb_group,
> > > > +
> >  NBREC_LOAD_BALANCER_GROUP_COL_LOAD_BALANCER)) {
> > > > +
> > > > +            struct ovn_lb_group *lb_group;
> > > > +            lb_group = ovn_lb_group_find(&lb_data->lb_groups,
> > > > +
> > &tracked_lb_group->header_.uuid);
> > > > +            ovs_assert(lb_group);
> > > > +            ovn_lb_group_reinit(lb_group, tracked_lb_group,
> > &lb_data->lbs);
> > > > +            for (size_t i = 0; i < lb_group->n_lbs; i++) {
> > > > +                build_lrouter_lb_ips(lb_group->lb_ips,
> > lb_group->lbs[i]);
> > > > +            }
> > > > +            add_lb_group_to_tracked_data(
> > > > +                lb_group,
> > &trk_lb_data->tracked_updated_lb_groups.updated);
> > > > +        }
> > > > +    }
> > > > +
> > > > +    engine_set_node_state(node, EN_UPDATED);
> > > > +    return true;
> > > > +}
> > > > +
> > > >   /* static functions. */
> > > >   static void
> > > >   northd_lb_data_init(struct northd_lb_data *lb_data)
> > > >   {
> > > >       hmap_init(&lb_data->lbs);
> > > >       hmap_init(&lb_data->lb_groups);
> > > > +
> > > > +    struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> > > > +    ovs_list_init(&trk_lb_data->tracked_updated_lbs.updated);
> > > > +    ovs_list_init(&trk_lb_data->tracked_deleted_lbs.updated);
> > > > +    ovs_list_init(&trk_lb_data->tracked_updated_lb_groups.updated);
> > > > +    ovs_list_init(&trk_lb_data->tracked_deleted_lb_groups.updated);
> > > >   }
> > > >
> > > >   static void
> > > > @@ -94,6 +214,8 @@ northd_lb_data_destroy(struct northd_lb_data
> > *lb_data)
> > > >           ovn_lb_group_destroy(lb_group);
> > > >       }
> > > >       hmap_destroy(&lb_data->lb_groups);
> > > > +
> > > > +    destroy_tracked_data(lb_data);
> > > >   }
> > > >
> > > >   static void
> > > > @@ -101,12 +223,9 @@ build_lbs(const struct nbrec_load_balancer_table
> > *nbrec_load_balancer_table,
> > > >             const struct nbrec_load_balancer_group_table
> > *nbrec_lb_group_table,
> > > >             struct hmap *lbs, struct hmap *lb_groups)
> > > >   {
> > > > -    struct ovn_lb_group *lb_group;
> > > > -    struct ovn_northd_lb *lb_nb;
> > > > -
> > > >       const struct nbrec_load_balancer *nbrec_lb;
> > > >       NBREC_LOAD_BALANCER_TABLE_FOR_EACH (nbrec_lb,
> > nbrec_load_balancer_table) {
> > > > -        lb_nb = ovn_northd_lb_create(nbrec_lb);
> > > > +        struct ovn_northd_lb *lb_nb = ovn_northd_lb_create(nbrec_lb);
> > > >           hmap_insert(lbs, &lb_nb->hmap_node,
> > > >                       uuid_hash(&nbrec_lb->header_.uuid));
> > > >       }
> > > > @@ -114,13 +233,76 @@ build_lbs(const struct nbrec_load_balancer_table
> > *nbrec_load_balancer_table,
> > > >       const struct nbrec_load_balancer_group *nbrec_lb_group;
> > > >       NBREC_LOAD_BALANCER_GROUP_TABLE_FOR_EACH (nbrec_lb_group,
> > > >                                                 nbrec_lb_group_table) {
> > > > -        lb_group = ovn_lb_group_create(nbrec_lb_group, lbs);
> > > > +        create_lb_group(nbrec_lb_group, lbs, lb_groups);
> > > > +    }
> > > > +}
> > > >
> > > > -        for (size_t i = 0; i < lb_group->n_lbs; i++) {
> > > > -            build_lrouter_lb_ips(lb_group->lb_ips, lb_group->lbs[i]);
> > > > -        }
> > > > +static struct ovn_lb_group *
> > > > +create_lb_group(const struct nbrec_load_balancer_group *nbrec_lb_group,
> > > > +                struct hmap *lbs, struct hmap *lb_groups)
> > > > +{
> > > > +    struct ovn_lb_group *lb_group =
> > ovn_lb_group_create(nbrec_lb_group, lbs);
> > > > +
> > > > +    for (size_t i = 0; i < lb_group->n_lbs; i++) {
> > > > +        build_lrouter_lb_ips(lb_group->lb_ips, lb_group->lbs[i]);
> > > > +    }
> > > > +
> > > > +    hmap_insert(lb_groups, &lb_group->hmap_node,
> > > > +                uuid_hash(&lb_group->uuid));
> > > > +
> > > > +    return lb_group;
> > > > +}
> > > > +
> > > > +static void
> > > > +destroy_tracked_data(struct northd_lb_data *lb_data)
> > > > +{
> > > > +    lb_data->tracked = false;
> > > > +
> > > > +    struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> > > > +    struct tracked_lb *tracked_lb;
> > > > +    LIST_FOR_EACH_SAFE (tracked_lb, list_node,
> > > > +                        &trk_lb_data->tracked_updated_lbs.updated) {
> > > > +        ovs_list_remove(&tracked_lb->list_node);
> > > > +        free(tracked_lb);
> > > > +    }
> > > > +
> > > > +    LIST_FOR_EACH_SAFE (tracked_lb, list_node,
> > > > +                        &trk_lb_data->tracked_deleted_lbs.updated) {
> > > > +        ovs_list_remove(&tracked_lb->list_node);
> > > > +        ovn_northd_lb_destroy(tracked_lb->lb);
> > > > +        free(tracked_lb);
> > > > +    }
> > > > +
> > > > +    struct tracked_lb_group *tracked_lb_group;
> > > > +    LIST_FOR_EACH_SAFE (tracked_lb_group, list_node,
> > > > +
> >  &trk_lb_data->tracked_updated_lb_groups.updated) {
> > > > +        ovs_list_remove(&tracked_lb_group->list_node);
> > > > +        free(tracked_lb_group);
> > > > +    }
> > > >
> > > > -        hmap_insert(lb_groups, &lb_group->hmap_node,
> > > > -                    uuid_hash(&lb_group->uuid));
> > > > +    LIST_FOR_EACH_SAFE (tracked_lb_group, list_node,
> > > > +
> >  &trk_lb_data->tracked_deleted_lb_groups.updated) {
> > > > +        ovs_list_remove(&tracked_lb_group->list_node);
> > > > +        ovn_lb_group_destroy(tracked_lb_group->lb_group);
> > > > +        free(tracked_lb_group);
> > > >       }
> > > >   }
> > > > +
> > > > +static void
> > > > +add_lb_to_tracked_data(struct ovn_northd_lb *lb, struct ovs_list
> > *tracked_list,
> > > > +                       bool health_checks)
> > > > +{
> > > > +    struct tracked_lb *t_lb = xzalloc(sizeof *t_lb);
> > > > +    t_lb->lb = lb;
> > > > +    t_lb->health_checks = health_checks;
> > > > +    ovs_list_push_back(tracked_list, &t_lb->list_node);
> > > > +}
> > > > +
> > > > +static void
> > > > +add_lb_group_to_tracked_data(struct ovn_lb_group *lb_group,
> > > > +                             struct ovs_list *tracked_list)
> > > > +{
> > > > +    struct tracked_lb_group *t_lb_group = xzalloc(sizeof *t_lb_group);
> > > > +    t_lb_group->lb_group = lb_group;
> > > > +    ovs_list_push_back(tracked_list, &t_lb_group->list_node);
> > > > +}
> > > > diff --git a/northd/en-northd-lb-data.h b/northd/en-northd-lb-data.h
> > > > index eb297e376d..bb07fcf686 100644
> > > > --- a/northd/en-northd-lb-data.h
> > > > +++ b/northd/en-northd-lb-data.h
> > > > @@ -4,16 +4,53 @@
> > > >   #include <config.h>
> > > >
> > > >   #include "openvswitch/hmap.h"
> > > > +#include "include/openvswitch/list.h"
> > > >
> > > >   #include "lib/inc-proc-eng.h"
> > > >
> > > > +struct ovn_northd_lb;
> > > > +struct ovn_lb_group;
> > > > +
> > > > +struct tracked_lb {
> > > > +    struct ovs_list list_node;
> > > > +    struct ovn_northd_lb *lb;
> > > > +    bool health_checks;
> > > > +};
> > > > +
> > > > +struct tracked_lb_group {
> > > > +    struct ovs_list list_node;
> > > > +    struct ovn_lb_group *lb_group;
> > > > +};
> > > > +
> > > > +struct tracked_lb_changes {
> > > > +    struct ovs_list updated; /* contains list of 'struct tracked_lb '
> > or
> > > > +                                list of 'struct tracked_lb_group' */
> > > > +};
> > > > +
> > > > +struct tracked_lb_data {
> > > > +    struct tracked_lb_changes tracked_updated_lbs;
> > > > +    struct tracked_lb_changes tracked_deleted_lbs;
> > > > +    struct tracked_lb_changes tracked_updated_lb_groups;
> > > > +    struct tracked_lb_changes tracked_deleted_lb_groups;
> > > > +};
> > >
> > > The typing here is a bit odd. The problem is that struct
> > > tracked_lb_changes is being used for two distinct types of lists. I
> > > think you should do one of two things here.
> > >
> > > 1) Get rid of the trakced_lb_changes struct and use ovs_list in-line.
> > >
> > > struct tracked_lb_data {
> > >      struct ovs_list tracked_updated_lbs;
> > >      struct ovs_list tracked_deleted_lbs;
> > >      struct ovs_list tracked_updated_lb_groups;
> > >      struct ovs_list tracked_deleted_lb_groups;
> > > };
> > >
> > > 2) Make a separate type for tracked lbs and tracked lb groups.
> > >
> > > struct tracked_lb_changes {
> > >      struct ovs_list updated;
> > > };
> > >
> > > struct tracked_lb_group_changes {
> > >      struct ovs_list updated;
> > > };
> > >
> > > struct tracked_lb_data {
> > >      struct tracked_lb_changes tracked_updated_lbs;
> > >      struct tracked_lb_changes tracked_deleted_lbs;
> > >      struct tracked_lb_group_changes tracked_updated_lb_groups;
> > >      struct tracked_lb_group_changes tracked_deleted_lb_groups;
> > > };
> > >
> > > I think option (2) is my preference.
> >
> > I prefer option (1), because the names of the fields in struct
> > tracked_lb_data already tells the purpose, and the only member "update" in
> > the struct tracked_lb_changes is unnecessary. In addition, the name
> > "update" is confusing, because it is in fact used for both "updated"
> > (added/updated) and "deleted".
>
> How about the below one ?
>
> -----------------------------------------
> struct tracked_lb_data {
> /* Both created and updated lbs. Contains hmapx of 'struct ovn_northd_lb' */
> struct hmapx crupdated_lbs;
> struct hmapx deleted_lbs;
>
> /* Both created and updated lb groups. Contains hmapx of
> * 'struct ovn_lb_group' */
> struct hmapx crupdated_lb_groups; /* Both created and updated lb groups. */
> struct hmapx deleted_lb_groups;
>
> bool has_health_checks;
> };
> -----------------------------------------
>
> I just googled a proper word for create and update and came across
> "upsert" and "crupdate" in the database context.
> I prefer the latter.  So went with it.
>
> Since northd engine will be handling both the created and updated
> lbs/lb groups, I think having just one is sufficient.
>
> Also I'm changing from a list to hmapx because in handling load
> balancer groups I want to also update the "crupdated_lbs"
> with the load balancers in the load balancer groups as this is
> required later when handling lb group changes for lflow_engine.
>
>
> Please let me know if there are any comments/objections.
>
> Thanks
> Numan
>
>
> >
> >
> > >
> > > > +
> > > >   struct northd_lb_data {
> > > >       struct hmap lbs;
> > > >       struct hmap lb_groups;
> > > > +
> > > > +    /* tracked data*/
> > > > +    bool tracked;
> > > > +    struct tracked_lb_data tracked_lb_data;
> > > >   };
> > > >
> > > >   void *en_northd_lb_data_init(struct engine_node *, struct engine_arg
> > *);
> > > >   void en_northd_lb_data_run(struct engine_node *, void *data);
> > > >   void en_northd_lb_data_cleanup(void *data);
> > > > +void en_northd_lb_data_clear_tracked_data(void *data);
> > > > +
> > > > +bool northd_lb_data_load_balancer_handler(struct engine_node *,
> > > > +                                          void *data);
> > > > +bool northd_lb_data_load_balancer_group_handler(struct engine_node *,
> > > > +                                                void *data);
> > > >
> > > >   #endif /* end of EN_NORTHD_LB_DATA_H */
> > > > diff --git a/northd/en-northd.c b/northd/en-northd.c
> > > > index cc7d838451..b3c03c54bd 100644
> > > > --- a/northd/en-northd.c
> > > > +++ b/northd/en-northd.c
> > > > @@ -206,6 +206,30 @@ northd_sb_port_binding_handler(struct engine_node
> > *node,
> > > >       return true;
> > > >   }
> > > >
> > > > +bool
> > > > +northd_northd_lb_data_handler(struct engine_node *node, void *data)
> > > > +{
> > > > +    struct northd_lb_data *lb_data =
> > > > +        engine_get_input_data("northd_lb_data", node);
> > > > +
> > > > +    if (!lb_data->tracked) {
> > > > +        return false;
> > > > +    }
> > > > +
> > > > +    struct northd_data *nd = data;
> > > > +
> > > > +    if (!northd_handle_lb_data_changes(&lb_data->tracked_lb_data,
> > > > +                                       &nd->ls_datapaths,
> > > > +                                       &nd->lr_datapaths,
> > > > +                                       &nd->lb_datapaths_map,
> > > > +                                       &nd->lb_group_datapaths_map)) {
> > > > +        return false;
> > > > +    }
> > > > +
> > > > +    engine_set_node_state(node, EN_UPDATED);
> > > > +    return true;
> > > > +}
> > > > +
> > > >   void
> > > >   *en_northd_init(struct engine_node *node OVS_UNUSED,
> > > >                   struct engine_arg *arg OVS_UNUSED)
> > > > diff --git a/northd/en-northd.h b/northd/en-northd.h
> > > > index 20cc77f108..5674f4390c 100644
> > > > --- a/northd/en-northd.h
> > > > +++ b/northd/en-northd.h
> > > > @@ -17,5 +17,6 @@ void en_northd_clear_tracked_data(void *data);
> > > >   bool northd_nb_nb_global_handler(struct engine_node *, void *data
> > OVS_UNUSED);
> > > >   bool northd_nb_logical_switch_handler(struct engine_node *, void
> > *data);
> > > >   bool northd_sb_port_binding_handler(struct engine_node *, void *data);
> > > > +bool northd_northd_lb_data_handler(struct engine_node *, void *data);
> > > >
> > > >   #endif /* EN_NORTHD_H */
> > > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> > > > index b2e884962f..b444a488db 100644
> > > > --- a/northd/inc-proc-northd.c
> > > > +++ b/northd/inc-proc-northd.c
> > > > @@ -141,13 +141,18 @@ static ENGINE_NODE(sync_to_sb_addr_set,
> > "sync_to_sb_addr_set");
> > > >   static ENGINE_NODE(fdb_aging, "fdb_aging");
> > > >   static ENGINE_NODE(fdb_aging_waker, "fdb_aging_waker");
> > > >   static ENGINE_NODE(sync_to_sb_lb, "sync_to_sb_lb");
> > > > -static ENGINE_NODE(northd_lb_data, "northd_lb_data");
> > > > +static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(northd_lb_data,
> > "northd_lb_data");
> > > >
> > > >   void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> > > >                             struct ovsdb_idl_loop *sb)
> > > >   {
> > > >       /* Define relationships between nodes where first argument is
> > dependent
> > > >        * on the second argument */
> > > > +    engine_add_input(&en_northd_lb_data, &en_nb_load_balancer,
> > > > +                     northd_lb_data_load_balancer_handler);
> > > > +    engine_add_input(&en_northd_lb_data, &en_nb_load_balancer_group,
> > > > +                     northd_lb_data_load_balancer_group_handler);
> > > > +
> >
> > Maybe it is not a big deal, but I think it is better to move the
> > dependencies that have handlers after the ones that don't, so that any
> > changes that will trigger recompute are handled first, to avoid wasting
> > time handling other changes and then fallback to recompute.

I didn't get this comment.  The above handlers are for the "lb_data"
engine node.
Since "lb_data" engine node is an input to "northd" engine node,
"lb_data" engine node
handlers will be evaluated first anyway.

Correct me if I'm wrong.

Thanks
Numan

> >
> > With these minor comments addressed:
> > Acked-by: Han Zhou <hzhou@ovn.org>
> >
> > > >       engine_add_input(&en_northd, &en_nb_port_group, NULL);
> > > >       engine_add_input(&en_northd, &en_nb_acl, NULL);
> > > >       engine_add_input(&en_northd, &en_nb_logical_router, NULL);
> > > > @@ -171,6 +176,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> > > >       engine_add_input(&en_northd, &en_sb_static_mac_binding, NULL);
> > > >       engine_add_input(&en_northd, &en_sb_chassis_template_var, NULL);
> > > >
> > > > +    engine_add_input(&en_northd, &en_northd_lb_data,
> > > > +                     northd_northd_lb_data_handler);
> > > >       engine_add_input(&en_northd, &en_sb_port_binding,
> > > >                        northd_sb_port_binding_handler);
> > > >       engine_add_input(&en_northd, &en_nb_nb_global,
> > > > @@ -178,10 +185,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop
> > *nb,
> > > >       engine_add_input(&en_northd, &en_nb_logical_switch,
> > > >                        northd_nb_logical_switch_handler);
> > > >
> > > > -    engine_add_input(&en_northd_lb_data, &en_nb_load_balancer, NULL);
> > > > -    engine_add_input(&en_northd_lb_data, &en_nb_load_balancer_group,
> > NULL);
> > > > -    engine_add_input(&en_northd, &en_northd_lb_data, NULL);
> > > > -
> > > >       engine_add_input(&en_mac_binding_aging, &en_nb_nb_global, NULL);
> > > >       engine_add_input(&en_mac_binding_aging, &en_sb_mac_binding, NULL);
> > > >       engine_add_input(&en_mac_binding_aging, &en_northd, NULL);
> > > > diff --git a/northd/northd.c b/northd/northd.c
> > > > index 890186b29c..f27f0de49b 100644
> > > > --- a/northd/northd.c
> > > > +++ b/northd/northd.c
> > > > @@ -40,6 +40,7 @@
> > > >   #include "lib/lb.h"
> > > >   #include "memory.h"
> > > >   #include "northd.h"
> > > > +#include "en-northd-lb-data.h"
> > > >   #include "lib/ovn-parallel-hmap.h"
> > > >   #include "ovn/actions.h"
> > > >   #include "ovn/features.h"
> > > > @@ -5323,6 +5324,82 @@ northd_handle_sb_port_binding_changes(
> > > >       return true;
> > > >   }
> > > >
> > > > +bool
> > > > +northd_handle_lb_data_changes(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 tracked_lb *trk_lb;
> > > > +
> > > > +    struct ovn_lb_datapaths *lb_dps;
> > > > +    LIST_FOR_EACH (trk_lb, list_node,
> > > > +                   &trk_lb_data->tracked_deleted_lbs.updated) {
> > > > +        if (trk_lb->health_checks) {
> > > > +            /* Fall back to recompute if the deleted load balancer has
> > > > +             * health checks configured. */
> > > > +            return false;
> > > > +        }
> > > > +
> > > > +        const struct uuid *lb_uuid =
> > > > +                &trk_lb->lb->nlb->header_.uuid;
> > > > +
> > > > +        lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> > > > +        ovs_assert(lb_dps);
> > > > +        hmap_remove(lb_datapaths_map, &lb_dps->hmap_node);
> > > > +        ovn_lb_datapaths_destroy(lb_dps);
> > > > +    }
> > > > +
> > > > +    LIST_FOR_EACH (trk_lb, list_node,
> > > > +                   &trk_lb_data->tracked_updated_lbs.updated) {
> > > > +        if (trk_lb->health_checks) {
> > > > +            /* Fall back to recompute if the created/updated load
> > balancer has
> > > > +             * health checks configured. */
> > > > +            return false;
> > > > +        }
> > > > +
> > > > +        const struct uuid *lb_uuid =
> > > > +                &trk_lb->lb->nlb->header_.uuid;
> > > > +        lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> > > > +        if (!lb_dps) {
> > > > +            lb_dps = ovn_lb_datapaths_create(trk_lb->lb,
> > > > +                                             ods_size(ls_datapaths),
> > > > +                                             ods_size(lr_datapaths));
> > > > +            hmap_insert(lb_datapaths_map, &lb_dps->hmap_node,
> > > > +                        uuid_hash(lb_uuid));
> > > > +        }
> > > > +    }
> > > > +
> > > > +    struct ovn_lb_group_datapaths *lb_group_dps;
> > > > +    struct tracked_lb_group *trk_lb_group;
> > > > +    LIST_FOR_EACH_SAFE (trk_lb_group, list_node,
> > > > +
> >  &trk_lb_data->tracked_deleted_lb_groups.updated) {
> > > > +        const struct uuid *lb_uuid = &trk_lb_group->lb_group->uuid;
> > > > +        lb_group_dps =
> > ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > > > +                                                   lb_uuid);
> > > > +        ovs_assert(lb_group_dps);
> > > > +        hmap_remove(lb_group_datapaths_map, &lb_group_dps->hmap_node);
> > > > +        ovn_lb_group_datapaths_destroy(lb_group_dps);
> > > > +    }
> > > > +
> > > > +    LIST_FOR_EACH_SAFE (trk_lb_group, list_node,
> > > > +
> >  &trk_lb_data->tracked_updated_lb_groups.updated) {
> > > > +        const struct uuid *lb_uuid = &trk_lb_group->lb_group->uuid;
> > > > +        lb_group_dps =
> > ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > > > +                                                   lb_uuid);
> > > > +        if (!lb_group_dps) {
> > > > +            lb_group_dps = ovn_lb_group_datapaths_create(
> > > > +                trk_lb_group->lb_group, ods_size(ls_datapaths),
> > > > +                ods_size(lr_datapaths));
> > > > +            hmap_insert(lb_group_datapaths_map,
> > &lb_group_dps->hmap_node,
> > > > +                        uuid_hash(lb_uuid));
> > > > +        }
> > > > +    }
> > > > +
> > > > +    return true;
> > > > +}
> > > > +
> > > >   struct multicast_group {
> > > >       const char *name;
> > > >       uint16_t key;               /*
> > OVN_MIN_MULTICAST...OVN_MAX_MULTICAST. */
> > > > diff --git a/northd/northd.h b/northd/northd.h
> > > > index 7d92028c7d..7d17921fa2 100644
> > > > --- a/northd/northd.h
> > > > +++ b/northd/northd.h
> > > > @@ -347,6 +347,13 @@ bool lflow_handle_northd_ls_changes(struct
> > ovsdb_idl_txn *ovnsb_txn,
> > > >   bool northd_handle_sb_port_binding_changes(
> > > >       const struct sbrec_port_binding_table *, struct hmap *ls_ports);
> > > >
> > > > +struct tracked_lb_data;
> > > > +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);
> > > > +
> > > >   void build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
> > > >                        const struct nbrec_bfd_table *,
> > > >                        const struct sbrec_bfd_table *,
> > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > > > index e79d33b2ae..dd0bd8b36a 100644
> > > > --- a/tests/ovn-northd.at
> > > > +++ b/tests/ovn-northd.at
> > > > @@ -9681,3 +9681,126 @@ AT_CHECK([grep "lr_in_gw_redirect" R1flows |sed
> > s'/table=../table=??/' |sort], [
> > > >
> > > >   AT_CLEANUP
> > > >   ])
> > > > +
> > > > +OVN_FOR_EACH_NORTHD_NO_HV([
> > > > +AT_SETUP([Load balancer incremental processing])
> > > > +ovn_start
> > > > +
> > > > +check_engine_stats() {
> > > > +  northd_comp=$1
> > > > +  lflow_comp=$2
> > > > +
> > > > +  AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats
> > northd_lb_data recompute], [0], [0
> > > > +])
> > > > +
> > > > +  northd_recompute_ct=$(as northd ovn-appctl -t NORTHD_TYPE
> > inc-engine/show-stats northd recompute)
> > > > +  if [[ "$northd_comp" == "norecompute" ]]; then
> > > > +    check test "$northd_recompute_ct" -eq "0"
> > > > +  else
> > > > +    check test "$northd_recompute_ct" -ne "0"
> > > > +  fi
> > > > +
> > > > +  lflow_recompute_ct=$(as northd ovn-appctl -t NORTHD_TYPE
> > inc-engine/show-stats lflow recompute)
> > > > +  if [[ "$lflow_comp" == "norecompute" ]]; then
> > > > +    check test "$lflow_recompute_ct" -eq "0"
> > > > +  else
> > > > +    check test "$lflow_recompute_ct" -ne "0"
> > > > +  fi
> > > > +}
> > > > +
> > > > +# Test I-P for load balancers.
> > > > +# Presently ovn-northd handles I-P for NB LBs in northd_lb_data engine
> > node
> > > > +# only.
> > > > +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 norecompute recompute
> > > > +
> > > > +check ovn-nbctl --wait=sb set load_balancer .
> > ip_port_mappings:10.0.0.3=sw0-p1:10.0.0.2
> > > > +check_engine_stats norecompute recompute
> > > > +
> > > > +check ovn-nbctl --wait=sb set load_balancer . options:foo=bar
> > > > +check_engine_stats norecompute recompute
> > > > +
> > > > +check ovn-nbctl --wait=sb -- lb-add lb2 20.0.0.10:80 20.0.0.20:80 --
> > lb-add lb3 30.0.0.10:80 30.0.0.20:80
> > > > +check_engine_stats norecompute recompute
> > > > +
> > > > +check ovn-nbctl --wait=sb -- lb-del lb2 -- lb-del lb3
> > > > +check_engine_stats norecompute recompute
> > > > +
> > > > +AT_CHECK([ovn-nbctl --wait=sb \
> > > > +          -- --id=@hc create Load_Balancer_Health_Check
> > vip="10.0.0.10\:80" \
> > > > +             options:failure_count=100 \
> > > > +          -- add Load_Balancer . health_check @hc | uuidfilt], [0],
> > [<0>
> > > > +])
> > > > +check_engine_stats recompute recompute
> > > > +
> > > > +# Any change to load balancer health check should also result in full
> > recompute
> > > > +# of northd node (but not northd_lb_data node)
> > > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > +check ovn-nbctl --wait=sb set load_balancer_health_check .
> > options:foo=bar1
> > > > +check_engine_stats recompute recompute
> > > > +
> > > > +# Delete the health check from the load balancer.  northd engine node
> > should do a full recompute.
> > > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > +check ovn-nbctl --wait=sb clear Load_Balancer . health_check
> > > > +check_engine_stats recompute recompute
> > > > +
> > > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > +check ovn-nbctl ls-add sw0
> > > > +check ovn-nbctl --wait=sb lr-add lr0
> > > > +check_engine_stats recompute recompute
> > > > +
> > > > +# Associate lb1 to 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-add sw0 lb1
> > > > +check_engine_stats recompute 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 recompute recompute
> > > > +
> > > > +# Test load balancer group now
> > > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > +lbg1_uuid=$(ovn-nbctl create load_balancer_group name=lbg1)
> > > > +check_engine_stats norecompute recompute
> > > > +
> > > > +lb1_uuid=$(fetch_column nb:Load_Balancer _uuid)
> > > > +
> > > > +# Add lb to the lbg1 group
> > > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > +check ovn-nbctl add load_balancer_group . load_Balancer $lb1_uuid
> > > > +check_engine_stats norecompute recompute
> > > > +
> > > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > +check ovn-nbctl clear load_balancer_group . load_Balancer
> > > > +check_engine_stats norecompute recompute
> > > > +
> > > > +# Add back lb to the lbg1 group
> > > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > +check ovn-nbctl add load_balancer_group . load_Balancer $lb1_uuid
> > > > +check_engine_stats norecompute 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 recompute recompute
> > > > +
> > > > +# Update lb and this should result in 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 recompute 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 recompute recompute
> > > > +
> > > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > +check ovn-nbctl add logical_router lr0 load_balancer_group $lbg1_uuid
> > > > +check_engine_stats recompute recompute
> > > > +
> > > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > +check ovn-nbctl clear logical_router lr0 load_balancer_group
> > > > +check_engine_stats recompute recompute
> > > > +
> > > > +AT_CLEANUP
> > > > +])
> > >
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On Tue, Jul 25, 2023 at 1:28 AM Numan Siddique <numans@ovn.org> wrote:
>
> On Fri, Jul 14, 2023 at 6:54 PM Numan Siddique <numans@ovn.org> wrote:
> >
> > On Wed, Jul 12, 2023 at 9:26 PM Han Zhou <zhouhan@gmail.com> wrote:
> > >
> > > On Sat, Jul 8, 2023 at 3:57 AM Mark Michelson <mmichels@redhat.com>
wrote:
> > > >
> > > > Hi Numan,
> > > >
> > > > I have one small nit below.
> > > >
> > > > On 7/7/23 01:53, numans@ovn.org wrote:
> > > > > From: Numan Siddique <numans@ovn.org>
> > > > >
> > > > > Any changes to load balancers and load balancer groups
> > > > > are handled incrementally in the newly added 'northd_lb_data'
> > > > > engine node.  'northd_lb_data' is input to 'northd' node
> > > > > and the handler - northd_northd_lb_data_handler in 'northd'
> > > > > node handles the changes.
> > > > >
> > > > > If a load balancer or load balancer group is associated to
> > > > > a logical switch or router then 'northd' node falls back
> > > > > to full recompute.
> > > > >
> > > > > Signed-off-by: Numan Siddique <numans@ovn.org>
> > > > > ---
> > > > >   lib/lb.c                   |  85 ++++++++++++----
> > > > >   lib/lb.h                   |   9 ++
> > > > >   northd/en-northd-lb-data.c | 202
+++++++++++++++++++++++++++++++++++--
> > > > >   northd/en-northd-lb-data.h |  37 +++++++
> > > > >   northd/en-northd.c         |  24 +++++
> > > > >   northd/en-northd.h         |   1 +
> > > > >   northd/inc-proc-northd.c   |  13 ++-
> > > > >   northd/northd.c            |  77 ++++++++++++++
> > > > >   northd/northd.h            |   7 ++
> > > > >   tests/ovn-northd.at        | 123 ++++++++++++++++++++++
> > > > >   10 files changed, 545 insertions(+), 33 deletions(-)
> > > > >
> > > > > diff --git a/lib/lb.c b/lib/lb.c
> > > > > index 429dbf15af..a51fe225fa 100644
> > > > > --- a/lib/lb.c
> > > > > +++ b/lib/lb.c
> > > > > @@ -606,13 +606,13 @@ ovn_lb_get_health_check(const struct
> > > nbrec_load_balancer *nbrec_lb,
> > > > >       return NULL;
> > > > >   }
> > > > >
> > > > > -struct ovn_northd_lb *
> > > > > -ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
> > > > > +static void
> > > > > +ovn_northd_lb_init(struct ovn_northd_lb *lb,
> > > > > +                   const struct nbrec_load_balancer *nbrec_lb)
> > > > >   {
> > > > >       bool template = smap_get_bool(&nbrec_lb->options,
"template",
> > > false);
> > > > >       bool is_udp = nullable_string_is_equal(nbrec_lb->protocol,
"udp");
> > > > >       bool is_sctp = nullable_string_is_equal(nbrec_lb->protocol,
> > > "sctp");
> > > > > -    struct ovn_northd_lb *lb = xzalloc(sizeof *lb);
> > > > >       int address_family =
!strcmp(smap_get_def(&nbrec_lb->options,
> > > > >                                                 "address-family",
> > > "ipv4"),
> > > > >                                    "ipv4")
> > > > > @@ -668,6 +668,10 @@ ovn_northd_lb_create(const struct
> > > nbrec_load_balancer *nbrec_lb)
> > > > >                                                     "reject",
false);
> > > > >           ovn_northd_lb_vip_init(lb_vip_nb, lb_vip, nbrec_lb,
> > > > >                                  node->key, node->value,
template);
> > > > > +        if (lb_vip_nb->lb_health_check) {
> > > > > +            lb->health_checks = true;
> > > > > +        }
> > > > > +
> > > > >           if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
> > > > >               sset_add(&lb->ips_v4, lb_vip->vip_str);
> > > > >           } else {
> > > > > @@ -713,6 +717,13 @@ ovn_northd_lb_create(const struct
> > > nbrec_load_balancer *nbrec_lb)
> > > > >           ds_chomp(&sel_fields, ',');
> > > > >           lb->selection_fields = ds_steal_cstr(&sel_fields);
> > > > >       }
> > > > > +}
> > > > > +
> > > > > +struct ovn_northd_lb *
> > > > > +ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
> > > > > +{
> > > > > +    struct ovn_northd_lb *lb = xzalloc(sizeof *lb);
> > > > > +    ovn_northd_lb_init(lb, nbrec_lb);
> > > > >       return lb;
> > > > >   }
> > > > >
> > > > > @@ -738,8 +749,8 @@ ovn_northd_lb_get_vips(const struct
ovn_northd_lb
> > > *lb)
> > > > >       return &lb->nlb->vips;
> > > > >   }
> > > > >
> > > > > -void
> > > > > -ovn_northd_lb_destroy(struct ovn_northd_lb *lb)
> > > > > +static void
> > > > > +ovn_northd_lb_cleanup(struct ovn_northd_lb *lb)
> > > > >   {
> > > > >       for (size_t i = 0; i < lb->n_vips; i++) {
> > > > >           ovn_lb_vip_destroy(&lb->vips[i]);
> > > > > @@ -747,26 +758,36 @@ ovn_northd_lb_destroy(struct ovn_northd_lb
*lb)
> > > > >       }
> > > > >       free(lb->vips);
> > > > >       free(lb->vips_nb);
> > > > > +    lb->vips = NULL;
> > > > > +    lb->vips_nb = NULL;
> > > > >       smap_destroy(&lb->template_vips);
> > > > >       sset_destroy(&lb->ips_v4);
> > > > >       sset_destroy(&lb->ips_v6);
> > > > >       free(lb->selection_fields);
> > > > > +    lb->selection_fields = NULL;
> > > > > +    lb->health_checks = false;
> > > > > +}
> > > > > +
> > > > > +void
> > > > > +ovn_northd_lb_destroy(struct ovn_northd_lb *lb)
> > > > > +{
> > > > > +    ovn_northd_lb_cleanup(lb);
> > > > >       free(lb);
> > > > >   }
> > > > >
> > > > > -/* Constructs a new 'struct ovn_lb_group' object from the Nb LB
Group
> > > record
> > > > > - * and a hash map of all existing 'struct ovn_northd_lb' objects.
> > > Space will
> > > > > - * be allocated for 'max_ls_datapaths' logical switches and
> > > 'max_lr_datapaths'
> > > > > - * logical routers to which this LB Group is applied.  Can be
filled
> > > later
> > > > > - * with ovn_lb_group_add_ls() and ovn_lb_group_add_lr()
respectively.
> > > */
> > > > > -struct ovn_lb_group *
> > > > > -ovn_lb_group_create(const struct nbrec_load_balancer_group
> > > *nbrec_lb_group,
> > > > > -                    const struct hmap *lbs)
> > > > > +void
> > > > > +ovn_northd_lb_reinit(struct ovn_northd_lb *lb,
> > > > > +                     const struct nbrec_load_balancer *nbrec_lb)
> > > > >   {
> > > > > -    struct ovn_lb_group *lb_group;
> > > > > +    ovn_northd_lb_cleanup(lb);
> > > > > +    ovn_northd_lb_init(lb, nbrec_lb);
> > > > > +}
> > > > >
> > > > > -    lb_group = xzalloc(sizeof *lb_group);
> > > > > -    lb_group->uuid = nbrec_lb_group->header_.uuid;
> > > > > +static void
> > > > > +ovn_lb_group_init(struct ovn_lb_group *lb_group,
> > > > > +                  const struct nbrec_load_balancer_group
> > > *nbrec_lb_group,
> > > > > +                  const struct hmap *lbs)
> > > > > +{
> > > > >       lb_group->n_lbs = nbrec_lb_group->n_load_balancer;
> > > > >       lb_group->lbs = xmalloc(lb_group->n_lbs * sizeof
*lb_group->lbs);
> > > > >       lb_group->lb_ips = ovn_lb_ip_set_create();
> > > > > @@ -776,10 +797,30 @@ ovn_lb_group_create(const struct
> > > nbrec_load_balancer_group *nbrec_lb_group,
> > > > >               &nbrec_lb_group->load_balancer[i]->header_.uuid;
> > > > >           lb_group->lbs[i] = ovn_northd_lb_find(lbs, lb_uuid);
> > > > >       }
> > > > > +}
> > > > >
> > > > > +/* Constructs a new 'struct ovn_lb_group' object from the Nb LB
Group
> > > record
> > > > > + * and a hash map of all existing 'struct ovn_northd_lb' objects.
> > > Space will
> > > > > + * be allocated for 'max_ls_datapaths' logical switches and
> > > 'max_lr_datapaths'
> > > > > + * logical routers to which this LB Group is applied.  Can be
filled
> > > later
> > > > > + * with ovn_lb_group_add_ls() and ovn_lb_group_add_lr()
respectively.
> > > */
> > > > > +struct ovn_lb_group *
> > > > > +ovn_lb_group_create(const struct nbrec_load_balancer_group
> > > *nbrec_lb_group,
> > > > > +                    const struct hmap *lbs)
> > > > > +{
> > > > > +    struct ovn_lb_group *lb_group = xzalloc(sizeof *lb_group);
> > > > > +    lb_group->uuid = nbrec_lb_group->header_.uuid;
> > > > > +    ovn_lb_group_init(lb_group, nbrec_lb_group, lbs);
> > > > >       return lb_group;
> > > > >   }
> > > > >
> > > > > +static void
> > > > > +ovn_lb_group_cleanup(struct ovn_lb_group *lb_group)
> > > > > +{
> > > > > +    ovn_lb_ip_set_destroy(lb_group->lb_ips);
> > > > > +    free(lb_group->lbs);
> > > > > +}
> > > > > +
> > > > >   void
> > > > >   ovn_lb_group_destroy(struct ovn_lb_group *lb_group)
> > > > >   {
> > > > > @@ -787,11 +828,19 @@ ovn_lb_group_destroy(struct ovn_lb_group
> > > *lb_group)
> > > > >           return;
> > > > >       }
> > > > >
> > > > > -    ovn_lb_ip_set_destroy(lb_group->lb_ips);
> > > > > -    free(lb_group->lbs);
> > > > > +    ovn_lb_group_cleanup(lb_group);
> > > > >       free(lb_group);
> > > > >   }
> > > > >
> > > > > +void
> > > > > +ovn_lb_group_reinit(struct ovn_lb_group *lb_group,
> > > > > +                    const struct nbrec_load_balancer_group
> > > *nbrec_lb_group,
> > > > > +                    const struct hmap *lbs)
> > > > > +{
> > > > > +    ovn_lb_group_cleanup(lb_group);
> > > > > +    ovn_lb_group_init(lb_group, nbrec_lb_group, lbs);
> > > > > +}
> > > > > +
> > > > >   struct ovn_lb_group *
> > > > >   ovn_lb_group_find(const struct hmap *lb_groups, const struct
uuid
> > > *uuid)
> > > > >   {
> > > > > diff --git a/lib/lb.h b/lib/lb.h
> > > > > index 0339050cba..74905c73b7 100644
> > > > > --- a/lib/lb.h
> > > > > +++ b/lib/lb.h
> > > > > @@ -77,6 +77,9 @@ struct ovn_northd_lb {
> > > > >
> > > > >       struct sset ips_v4;
> > > > >       struct sset ips_v6;
> > > > > +
> > > > > +    /* Indicates if the load balancer has health checks
configured. */
> > > > > +    bool health_checks;
> > > > >   };
> > > > >
> > > > >   struct ovn_lb_vip {
> > > > > @@ -130,6 +133,8 @@ struct ovn_northd_lb *ovn_northd_lb_find(const
> > > struct hmap *,
> > > > >                                            const struct uuid *);
> > > > >   const struct smap *ovn_northd_lb_get_vips(const struct
ovn_northd_lb
> > > *);
> > > > >   void ovn_northd_lb_destroy(struct ovn_northd_lb *);
> > > > > +void ovn_northd_lb_reinit(struct ovn_northd_lb *,
> > > > > +                          const struct nbrec_load_balancer *);
> > > > >
> > > > >   void build_lrouter_lb_ips(struct ovn_lb_ip_set *,
> > > > >                             const struct ovn_northd_lb *);
> > > > > @@ -148,6 +153,10 @@ struct ovn_lb_group *ovn_lb_group_create(
> > > > >   void ovn_lb_group_destroy(struct ovn_lb_group *lb_group);
> > > > >   struct ovn_lb_group *ovn_lb_group_find(const struct hmap
*lb_groups,
> > > > >                                          const struct uuid *);
> > > > > +void ovn_lb_group_reinit(
> > > > > +    struct ovn_lb_group *lb_group,
> > > > > +    const struct nbrec_load_balancer_group *,
> > > > > +    const struct hmap *lbs);
> > > > >
> > > > >   struct ovn_lb_datapaths {
> > > > >       struct hmap_node hmap_node;
> > > > > diff --git a/northd/en-northd-lb-data.c
b/northd/en-northd-lb-data.c
> > > > > index d46c3c27ed..e52535b4a9 100644
> > > > > --- a/northd/en-northd-lb-data.c
> > > > > +++ b/northd/en-northd-lb-data.c
> > > > > @@ -37,6 +37,15 @@ static void northd_lb_data_destroy(struct
> > > northd_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 struct ovn_lb_group *create_lb_group(
> > > > > +    const struct nbrec_load_balancer_group *, struct hmap *lbs,
> > > > > +    struct hmap *lb_groups);
> > > > > +static void destroy_tracked_data(struct northd_lb_data *);
> > > > > +static void add_lb_to_tracked_data(struct ovn_northd_lb *,
> > > > > +                                   struct ovs_list *tracked_list,
> > > > > +                                   bool health_checks);
> > > > > +static void add_lb_group_to_tracked_data(struct ovn_lb_group *,
> > > > > +                                         struct ovs_list
> > > *tracked_list);
> > > > >
> > > > >   void *
> > > > >   en_northd_lb_data_init(struct engine_node *node OVS_UNUSED,
> > > > > @@ -61,6 +70,7 @@ en_northd_lb_data_run(struct engine_node *node,
void
> > > *data)
> > > > >       const struct nbrec_load_balancer_group_table *nb_lbg_table =
> > > > >           EN_OVSDB_GET(engine_get_input("NB_load_balancer_group",
> > > node));
> > > > >
> > > > > +    lb_data->tracked = false;
> > > > >       build_lbs(nb_lb_table, nb_lbg_table, &lb_data->lbs,
> > > &lb_data->lb_groups);
> > > > >       engine_set_node_state(node, EN_UPDATED);
> > > > >   }
> > > > > @@ -72,12 +82,122 @@ en_northd_lb_data_cleanup(void *data)
> > > > >       northd_lb_data_destroy(lb_data);
> > > > >   }
> > > > >
> > > > > +void
> > > > > +en_northd_lb_data_clear_tracked_data(void *data)
> > > > > +{
> > > > > +    struct northd_lb_data *lb_data = (struct northd_lb_data *)
data;
> > > > > +    destroy_tracked_data(lb_data);
> > > > > +}
> > > > > +
> > > > > +
> > > > > +/* Handler functions. */
> > > > > +bool
> > > > > +northd_lb_data_load_balancer_handler(struct engine_node *node,
> > > > > +                                     void *data OVS_UNUSED)
> > > > > +{
> > > > > +    const struct nbrec_load_balancer_table *nb_lb_table =
> > > > > +        EN_OVSDB_GET(engine_get_input("NB_load_balancer", node));
> > > > > +
> > > > > +    struct northd_lb_data *lb_data = (struct northd_lb_data *)
data;
> > > > > +
> > > > > +    lb_data->tracked = true;
> > > > > +    struct tracked_lb_data *trk_lb_data =
&lb_data->tracked_lb_data;
> > > > > +
> > > > > +    const struct nbrec_load_balancer *tracked_lb;
> > > > > +    NBREC_LOAD_BALANCER_TABLE_FOR_EACH_TRACKED (tracked_lb,
> > > nb_lb_table) {
> > > > > +        struct ovn_northd_lb *lb;
> > > > > +        if (nbrec_load_balancer_is_new(tracked_lb)) {
> > > > > +            /* New load balancer. */
> > > > > +            lb = ovn_northd_lb_create(tracked_lb);
> > > > > +            hmap_insert(&lb_data->lbs, &lb->hmap_node,
> > > > > +                        uuid_hash(&tracked_lb->header_.uuid));
> > > > > +            add_lb_to_tracked_data(
> > > > > +                lb, &trk_lb_data->tracked_updated_lbs.updated,
> > > > > +                lb->health_checks);
> > > > > +        } else if (nbrec_load_balancer_is_deleted(tracked_lb)) {
> > > > > +            lb = ovn_northd_lb_find(&lb_data->lbs,
> > > > > +                                    &tracked_lb->header_.uuid);
> > > > > +            ovs_assert(lb);
> > > > > +            hmap_remove(&lb_data->lbs, &lb->hmap_node);
> > > > > +            add_lb_to_tracked_data(
> > > > > +                lb, &trk_lb_data->tracked_deleted_lbs.updated,
> > > > > +                lb->health_checks);
> > > > > +        } else {
> > > > > +            /* Load balancer updated. */
> > > > > +            lb = ovn_northd_lb_find(&lb_data->lbs,
> > > > > +                                    &tracked_lb->header_.uuid);
> > > > > +            ovs_assert(lb);
> > > > > +            bool health_checks = lb->health_checks;
> > > > > +            ovn_northd_lb_reinit(lb, tracked_lb);
> > > > > +            health_checks |= lb->health_checks;
> > > > > +            add_lb_to_tracked_data(
> > > > > +                lb, &trk_lb_data->tracked_updated_lbs.updated,
> > > health_checks);
> > > > > +        }
> > > > > +    }
> > > > > +
> > > > > +    engine_set_node_state(node, EN_UPDATED);
> > > > > +    return true;
> > > > > +}
> > > > > +
> > > > > +bool
> > > > > +northd_lb_data_load_balancer_group_handler(struct engine_node
*node,
> > > > > +                                           void *data OVS_UNUSED)
> > > > > +{
> > > > > +    struct northd_lb_data *lb_data = (struct northd_lb_data *)
data;
> > > > > +    const struct nbrec_load_balancer_group_table *nb_lbg_table =
> > > > > +        EN_OVSDB_GET(engine_get_input("NB_load_balancer_group",
node));
> > > > > +
> > > > > +    lb_data->tracked = true;
> > > > > +    struct tracked_lb_data *trk_lb_data =
&lb_data->tracked_lb_data;
> > > > > +    const struct nbrec_load_balancer_group *tracked_lb_group;
> > > > > +    NBREC_LOAD_BALANCER_GROUP_TABLE_FOR_EACH_TRACKED
(tracked_lb_group,
> > > > > +
 nb_lbg_table) {
> > > > > +        if (nbrec_load_balancer_group_is_new(tracked_lb_group)) {
> > > > > +            struct ovn_lb_group *lb_group =
> > > > > +                create_lb_group(tracked_lb_group, &lb_data->lbs,
> > > > > +                                &lb_data->lb_groups);
> > > > > +            add_lb_group_to_tracked_data(
> > > > > +                lb_group,
> > > &trk_lb_data->tracked_updated_lb_groups.updated);
> > > > > +        } else if
> > > (nbrec_load_balancer_group_is_deleted(tracked_lb_group)) {
> > > > > +            struct ovn_lb_group *lb_group;
> > > > > +            lb_group = ovn_lb_group_find(&lb_data->lb_groups,
> > > > > +
> > > &tracked_lb_group->header_.uuid);
> > > > > +            ovs_assert(lb_group);
> > > > > +            hmap_remove(&lb_data->lb_groups,
&lb_group->hmap_node);
> > > > > +            add_lb_group_to_tracked_data(
> > > > > +                lb_group,
> > > &trk_lb_data->tracked_deleted_lb_groups.updated);
> > > > > +        } else if
> > > (nbrec_load_balancer_group_is_updated(tracked_lb_group,
> > > > > +
> > >  NBREC_LOAD_BALANCER_GROUP_COL_LOAD_BALANCER)) {
> > > > > +
> > > > > +            struct ovn_lb_group *lb_group;
> > > > > +            lb_group = ovn_lb_group_find(&lb_data->lb_groups,
> > > > > +
> > > &tracked_lb_group->header_.uuid);
> > > > > +            ovs_assert(lb_group);
> > > > > +            ovn_lb_group_reinit(lb_group, tracked_lb_group,
> > > &lb_data->lbs);
> > > > > +            for (size_t i = 0; i < lb_group->n_lbs; i++) {
> > > > > +                build_lrouter_lb_ips(lb_group->lb_ips,
> > > lb_group->lbs[i]);
> > > > > +            }
> > > > > +            add_lb_group_to_tracked_data(
> > > > > +                lb_group,
> > > &trk_lb_data->tracked_updated_lb_groups.updated);
> > > > > +        }
> > > > > +    }
> > > > > +
> > > > > +    engine_set_node_state(node, EN_UPDATED);
> > > > > +    return true;
> > > > > +}
> > > > > +
> > > > >   /* static functions. */
> > > > >   static void
> > > > >   northd_lb_data_init(struct northd_lb_data *lb_data)
> > > > >   {
> > > > >       hmap_init(&lb_data->lbs);
> > > > >       hmap_init(&lb_data->lb_groups);
> > > > > +
> > > > > +    struct tracked_lb_data *trk_lb_data =
&lb_data->tracked_lb_data;
> > > > > +    ovs_list_init(&trk_lb_data->tracked_updated_lbs.updated);
> > > > > +    ovs_list_init(&trk_lb_data->tracked_deleted_lbs.updated);
> > > > > +
 ovs_list_init(&trk_lb_data->tracked_updated_lb_groups.updated);
> > > > > +
 ovs_list_init(&trk_lb_data->tracked_deleted_lb_groups.updated);
> > > > >   }
> > > > >
> > > > >   static void
> > > > > @@ -94,6 +214,8 @@ northd_lb_data_destroy(struct northd_lb_data
> > > *lb_data)
> > > > >           ovn_lb_group_destroy(lb_group);
> > > > >       }
> > > > >       hmap_destroy(&lb_data->lb_groups);
> > > > > +
> > > > > +    destroy_tracked_data(lb_data);
> > > > >   }
> > > > >
> > > > >   static void
> > > > > @@ -101,12 +223,9 @@ build_lbs(const struct
nbrec_load_balancer_table
> > > *nbrec_load_balancer_table,
> > > > >             const struct nbrec_load_balancer_group_table
> > > *nbrec_lb_group_table,
> > > > >             struct hmap *lbs, struct hmap *lb_groups)
> > > > >   {
> > > > > -    struct ovn_lb_group *lb_group;
> > > > > -    struct ovn_northd_lb *lb_nb;
> > > > > -
> > > > >       const struct nbrec_load_balancer *nbrec_lb;
> > > > >       NBREC_LOAD_BALANCER_TABLE_FOR_EACH (nbrec_lb,
> > > nbrec_load_balancer_table) {
> > > > > -        lb_nb = ovn_northd_lb_create(nbrec_lb);
> > > > > +        struct ovn_northd_lb *lb_nb =
ovn_northd_lb_create(nbrec_lb);
> > > > >           hmap_insert(lbs, &lb_nb->hmap_node,
> > > > >                       uuid_hash(&nbrec_lb->header_.uuid));
> > > > >       }
> > > > > @@ -114,13 +233,76 @@ build_lbs(const struct
nbrec_load_balancer_table
> > > *nbrec_load_balancer_table,
> > > > >       const struct nbrec_load_balancer_group *nbrec_lb_group;
> > > > >       NBREC_LOAD_BALANCER_GROUP_TABLE_FOR_EACH (nbrec_lb_group,
> > > > >
nbrec_lb_group_table) {
> > > > > -        lb_group = ovn_lb_group_create(nbrec_lb_group, lbs);
> > > > > +        create_lb_group(nbrec_lb_group, lbs, lb_groups);
> > > > > +    }
> > > > > +}
> > > > >
> > > > > -        for (size_t i = 0; i < lb_group->n_lbs; i++) {
> > > > > -            build_lrouter_lb_ips(lb_group->lb_ips,
lb_group->lbs[i]);
> > > > > -        }
> > > > > +static struct ovn_lb_group *
> > > > > +create_lb_group(const struct nbrec_load_balancer_group
*nbrec_lb_group,
> > > > > +                struct hmap *lbs, struct hmap *lb_groups)
> > > > > +{
> > > > > +    struct ovn_lb_group *lb_group =
> > > ovn_lb_group_create(nbrec_lb_group, lbs);
> > > > > +
> > > > > +    for (size_t i = 0; i < lb_group->n_lbs; i++) {
> > > > > +        build_lrouter_lb_ips(lb_group->lb_ips, lb_group->lbs[i]);
> > > > > +    }
> > > > > +
> > > > > +    hmap_insert(lb_groups, &lb_group->hmap_node,
> > > > > +                uuid_hash(&lb_group->uuid));
> > > > > +
> > > > > +    return lb_group;
> > > > > +}
> > > > > +
> > > > > +static void
> > > > > +destroy_tracked_data(struct northd_lb_data *lb_data)
> > > > > +{
> > > > > +    lb_data->tracked = false;
> > > > > +
> > > > > +    struct tracked_lb_data *trk_lb_data =
&lb_data->tracked_lb_data;
> > > > > +    struct tracked_lb *tracked_lb;
> > > > > +    LIST_FOR_EACH_SAFE (tracked_lb, list_node,
> > > > > +
 &trk_lb_data->tracked_updated_lbs.updated) {
> > > > > +        ovs_list_remove(&tracked_lb->list_node);
> > > > > +        free(tracked_lb);
> > > > > +    }
> > > > > +
> > > > > +    LIST_FOR_EACH_SAFE (tracked_lb, list_node,
> > > > > +
 &trk_lb_data->tracked_deleted_lbs.updated) {
> > > > > +        ovs_list_remove(&tracked_lb->list_node);
> > > > > +        ovn_northd_lb_destroy(tracked_lb->lb);
> > > > > +        free(tracked_lb);
> > > > > +    }
> > > > > +
> > > > > +    struct tracked_lb_group *tracked_lb_group;
> > > > > +    LIST_FOR_EACH_SAFE (tracked_lb_group, list_node,
> > > > > +
> > >  &trk_lb_data->tracked_updated_lb_groups.updated) {
> > > > > +        ovs_list_remove(&tracked_lb_group->list_node);
> > > > > +        free(tracked_lb_group);
> > > > > +    }
> > > > >
> > > > > -        hmap_insert(lb_groups, &lb_group->hmap_node,
> > > > > -                    uuid_hash(&lb_group->uuid));
> > > > > +    LIST_FOR_EACH_SAFE (tracked_lb_group, list_node,
> > > > > +
> > >  &trk_lb_data->tracked_deleted_lb_groups.updated) {
> > > > > +        ovs_list_remove(&tracked_lb_group->list_node);
> > > > > +        ovn_lb_group_destroy(tracked_lb_group->lb_group);
> > > > > +        free(tracked_lb_group);
> > > > >       }
> > > > >   }
> > > > > +
> > > > > +static void
> > > > > +add_lb_to_tracked_data(struct ovn_northd_lb *lb, struct ovs_list
> > > *tracked_list,
> > > > > +                       bool health_checks)
> > > > > +{
> > > > > +    struct tracked_lb *t_lb = xzalloc(sizeof *t_lb);
> > > > > +    t_lb->lb = lb;
> > > > > +    t_lb->health_checks = health_checks;
> > > > > +    ovs_list_push_back(tracked_list, &t_lb->list_node);
> > > > > +}
> > > > > +
> > > > > +static void
> > > > > +add_lb_group_to_tracked_data(struct ovn_lb_group *lb_group,
> > > > > +                             struct ovs_list *tracked_list)
> > > > > +{
> > > > > +    struct tracked_lb_group *t_lb_group = xzalloc(sizeof
*t_lb_group);
> > > > > +    t_lb_group->lb_group = lb_group;
> > > > > +    ovs_list_push_back(tracked_list, &t_lb_group->list_node);
> > > > > +}
> > > > > diff --git a/northd/en-northd-lb-data.h
b/northd/en-northd-lb-data.h
> > > > > index eb297e376d..bb07fcf686 100644
> > > > > --- a/northd/en-northd-lb-data.h
> > > > > +++ b/northd/en-northd-lb-data.h
> > > > > @@ -4,16 +4,53 @@
> > > > >   #include <config.h>
> > > > >
> > > > >   #include "openvswitch/hmap.h"
> > > > > +#include "include/openvswitch/list.h"
> > > > >
> > > > >   #include "lib/inc-proc-eng.h"
> > > > >
> > > > > +struct ovn_northd_lb;
> > > > > +struct ovn_lb_group;
> > > > > +
> > > > > +struct tracked_lb {
> > > > > +    struct ovs_list list_node;
> > > > > +    struct ovn_northd_lb *lb;
> > > > > +    bool health_checks;
> > > > > +};
> > > > > +
> > > > > +struct tracked_lb_group {
> > > > > +    struct ovs_list list_node;
> > > > > +    struct ovn_lb_group *lb_group;
> > > > > +};
> > > > > +
> > > > > +struct tracked_lb_changes {
> > > > > +    struct ovs_list updated; /* contains list of 'struct
tracked_lb '
> > > or
> > > > > +                                list of 'struct
tracked_lb_group' */
> > > > > +};
> > > > > +
> > > > > +struct tracked_lb_data {
> > > > > +    struct tracked_lb_changes tracked_updated_lbs;
> > > > > +    struct tracked_lb_changes tracked_deleted_lbs;
> > > > > +    struct tracked_lb_changes tracked_updated_lb_groups;
> > > > > +    struct tracked_lb_changes tracked_deleted_lb_groups;
> > > > > +};
> > > >
> > > > The typing here is a bit odd. The problem is that struct
> > > > tracked_lb_changes is being used for two distinct types of lists. I
> > > > think you should do one of two things here.
> > > >
> > > > 1) Get rid of the trakced_lb_changes struct and use ovs_list
in-line.
> > > >
> > > > struct tracked_lb_data {
> > > >      struct ovs_list tracked_updated_lbs;
> > > >      struct ovs_list tracked_deleted_lbs;
> > > >      struct ovs_list tracked_updated_lb_groups;
> > > >      struct ovs_list tracked_deleted_lb_groups;
> > > > };
> > > >
> > > > 2) Make a separate type for tracked lbs and tracked lb groups.
> > > >
> > > > struct tracked_lb_changes {
> > > >      struct ovs_list updated;
> > > > };
> > > >
> > > > struct tracked_lb_group_changes {
> > > >      struct ovs_list updated;
> > > > };
> > > >
> > > > struct tracked_lb_data {
> > > >      struct tracked_lb_changes tracked_updated_lbs;
> > > >      struct tracked_lb_changes tracked_deleted_lbs;
> > > >      struct tracked_lb_group_changes tracked_updated_lb_groups;
> > > >      struct tracked_lb_group_changes tracked_deleted_lb_groups;
> > > > };
> > > >
> > > > I think option (2) is my preference.
> > >
> > > I prefer option (1), because the names of the fields in struct
> > > tracked_lb_data already tells the purpose, and the only member
"update" in
> > > the struct tracked_lb_changes is unnecessary. In addition, the name
> > > "update" is confusing, because it is in fact used for both "updated"
> > > (added/updated) and "deleted".
> >
> > How about the below one ?
> >
> > -----------------------------------------
> > struct tracked_lb_data {
> > /* Both created and updated lbs. Contains hmapx of 'struct
ovn_northd_lb' */
> > struct hmapx crupdated_lbs;
> > struct hmapx deleted_lbs;
> >
> > /* Both created and updated lb groups. Contains hmapx of
> > * 'struct ovn_lb_group' */
> > struct hmapx crupdated_lb_groups; /* Both created and updated lb
groups. */
> > struct hmapx deleted_lb_groups;
> >
> > bool has_health_checks;
> > };
> > -----------------------------------------
> >
> > I just googled a proper word for create and update and came across
> > "upsert" and "crupdate" in the database context.
> > I prefer the latter.  So went with it.
> >
> > Since northd engine will be handling both the created and updated
> > lbs/lb groups, I think having just one is sufficient.
> >
> > Also I'm changing from a list to hmapx because in handling load
> > balancer groups I want to also update the "crupdated_lbs"
> > with the load balancers in the load balancer groups as this is
> > required later when handling lb group changes for lflow_engine.
> >
> >
> > Please let me know if there are any comments/objections.
> >
> > Thanks
> > Numan
> >
> >
> > >
> > >
> > > >
> > > > > +
> > > > >   struct northd_lb_data {
> > > > >       struct hmap lbs;
> > > > >       struct hmap lb_groups;
> > > > > +
> > > > > +    /* tracked data*/
> > > > > +    bool tracked;
> > > > > +    struct tracked_lb_data tracked_lb_data;
> > > > >   };
> > > > >
> > > > >   void *en_northd_lb_data_init(struct engine_node *, struct
engine_arg
> > > *);
> > > > >   void en_northd_lb_data_run(struct engine_node *, void *data);
> > > > >   void en_northd_lb_data_cleanup(void *data);
> > > > > +void en_northd_lb_data_clear_tracked_data(void *data);
> > > > > +
> > > > > +bool northd_lb_data_load_balancer_handler(struct engine_node *,
> > > > > +                                          void *data);
> > > > > +bool northd_lb_data_load_balancer_group_handler(struct
engine_node *,
> > > > > +                                                void *data);
> > > > >
> > > > >   #endif /* end of EN_NORTHD_LB_DATA_H */
> > > > > diff --git a/northd/en-northd.c b/northd/en-northd.c
> > > > > index cc7d838451..b3c03c54bd 100644
> > > > > --- a/northd/en-northd.c
> > > > > +++ b/northd/en-northd.c
> > > > > @@ -206,6 +206,30 @@ northd_sb_port_binding_handler(struct
engine_node
> > > *node,
> > > > >       return true;
> > > > >   }
> > > > >
> > > > > +bool
> > > > > +northd_northd_lb_data_handler(struct engine_node *node, void
*data)
> > > > > +{
> > > > > +    struct northd_lb_data *lb_data =
> > > > > +        engine_get_input_data("northd_lb_data", node);
> > > > > +
> > > > > +    if (!lb_data->tracked) {
> > > > > +        return false;
> > > > > +    }
> > > > > +
> > > > > +    struct northd_data *nd = data;
> > > > > +
> > > > > +    if (!northd_handle_lb_data_changes(&lb_data->tracked_lb_data,
> > > > > +                                       &nd->ls_datapaths,
> > > > > +                                       &nd->lr_datapaths,
> > > > > +                                       &nd->lb_datapaths_map,
> > > > > +
&nd->lb_group_datapaths_map)) {
> > > > > +        return false;
> > > > > +    }
> > > > > +
> > > > > +    engine_set_node_state(node, EN_UPDATED);
> > > > > +    return true;
> > > > > +}
> > > > > +
> > > > >   void
> > > > >   *en_northd_init(struct engine_node *node OVS_UNUSED,
> > > > >                   struct engine_arg *arg OVS_UNUSED)
> > > > > diff --git a/northd/en-northd.h b/northd/en-northd.h
> > > > > index 20cc77f108..5674f4390c 100644
> > > > > --- a/northd/en-northd.h
> > > > > +++ b/northd/en-northd.h
> > > > > @@ -17,5 +17,6 @@ void en_northd_clear_tracked_data(void *data);
> > > > >   bool northd_nb_nb_global_handler(struct engine_node *, void
*data
> > > OVS_UNUSED);
> > > > >   bool northd_nb_logical_switch_handler(struct engine_node *, void
> > > *data);
> > > > >   bool northd_sb_port_binding_handler(struct engine_node *, void
*data);
> > > > > +bool northd_northd_lb_data_handler(struct engine_node *, void
*data);
> > > > >
> > > > >   #endif /* EN_NORTHD_H */
> > > > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> > > > > index b2e884962f..b444a488db 100644
> > > > > --- a/northd/inc-proc-northd.c
> > > > > +++ b/northd/inc-proc-northd.c
> > > > > @@ -141,13 +141,18 @@ static ENGINE_NODE(sync_to_sb_addr_set,
> > > "sync_to_sb_addr_set");
> > > > >   static ENGINE_NODE(fdb_aging, "fdb_aging");
> > > > >   static ENGINE_NODE(fdb_aging_waker, "fdb_aging_waker");
> > > > >   static ENGINE_NODE(sync_to_sb_lb, "sync_to_sb_lb");
> > > > > -static ENGINE_NODE(northd_lb_data, "northd_lb_data");
> > > > > +static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(northd_lb_data,
> > > "northd_lb_data");
> > > > >
> > > > >   void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> > > > >                             struct ovsdb_idl_loop *sb)
> > > > >   {
> > > > >       /* Define relationships between nodes where first argument
is
> > > dependent
> > > > >        * on the second argument */
> > > > > +    engine_add_input(&en_northd_lb_data, &en_nb_load_balancer,
> > > > > +                     northd_lb_data_load_balancer_handler);
> > > > > +    engine_add_input(&en_northd_lb_data,
&en_nb_load_balancer_group,
> > > > > +                     northd_lb_data_load_balancer_group_handler);
> > > > > +
> > >
> > > Maybe it is not a big deal, but I think it is better to move the
> > > dependencies that have handlers after the ones that don't, so that any
> > > changes that will trigger recompute are handled first, to avoid
wasting
> > > time handling other changes and then fallback to recompute.
>
> I didn't get this comment.  The above handlers are for the "lb_data"
> engine node.
> Since "lb_data" engine node is an input to "northd" engine node,
> "lb_data" engine node
> handlers will be evaluated first anyway.
>

Oops, my bad. Please ignore this comment.

> Correct me if I'm wrong.
>
> Thanks
> Numan
>
> > >
> > > With these minor comments addressed:
> > > Acked-by: Han Zhou <hzhou@ovn.org>
> > >
> > > > >       engine_add_input(&en_northd, &en_nb_port_group, NULL);
> > > > >       engine_add_input(&en_northd, &en_nb_acl, NULL);
> > > > >       engine_add_input(&en_northd, &en_nb_logical_router, NULL);
> > > > > @@ -171,6 +176,8 @@ void inc_proc_northd_init(struct
ovsdb_idl_loop *nb,
> > > > >       engine_add_input(&en_northd, &en_sb_static_mac_binding,
NULL);
> > > > >       engine_add_input(&en_northd, &en_sb_chassis_template_var,
NULL);
> > > > >
> > > > > +    engine_add_input(&en_northd, &en_northd_lb_data,
> > > > > +                     northd_northd_lb_data_handler);
> > > > >       engine_add_input(&en_northd, &en_sb_port_binding,
> > > > >                        northd_sb_port_binding_handler);
> > > > >       engine_add_input(&en_northd, &en_nb_nb_global,
> > > > > @@ -178,10 +185,6 @@ void inc_proc_northd_init(struct
ovsdb_idl_loop
> > > *nb,
> > > > >       engine_add_input(&en_northd, &en_nb_logical_switch,
> > > > >                        northd_nb_logical_switch_handler);
> > > > >
> > > > > -    engine_add_input(&en_northd_lb_data, &en_nb_load_balancer,
NULL);
> > > > > -    engine_add_input(&en_northd_lb_data,
&en_nb_load_balancer_group,
> > > NULL);
> > > > > -    engine_add_input(&en_northd, &en_northd_lb_data, NULL);
> > > > > -
> > > > >       engine_add_input(&en_mac_binding_aging, &en_nb_nb_global,
NULL);
> > > > >       engine_add_input(&en_mac_binding_aging, &en_sb_mac_binding,
NULL);
> > > > >       engine_add_input(&en_mac_binding_aging, &en_northd, NULL);
> > > > > diff --git a/northd/northd.c b/northd/northd.c
> > > > > index 890186b29c..f27f0de49b 100644
> > > > > --- a/northd/northd.c
> > > > > +++ b/northd/northd.c
> > > > > @@ -40,6 +40,7 @@
> > > > >   #include "lib/lb.h"
> > > > >   #include "memory.h"
> > > > >   #include "northd.h"
> > > > > +#include "en-northd-lb-data.h"
> > > > >   #include "lib/ovn-parallel-hmap.h"
> > > > >   #include "ovn/actions.h"
> > > > >   #include "ovn/features.h"
> > > > > @@ -5323,6 +5324,82 @@ northd_handle_sb_port_binding_changes(
> > > > >       return true;
> > > > >   }
> > > > >
> > > > > +bool
> > > > > +northd_handle_lb_data_changes(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 tracked_lb *trk_lb;
> > > > > +
> > > > > +    struct ovn_lb_datapaths *lb_dps;
> > > > > +    LIST_FOR_EACH (trk_lb, list_node,
> > > > > +                   &trk_lb_data->tracked_deleted_lbs.updated) {
> > > > > +        if (trk_lb->health_checks) {
> > > > > +            /* Fall back to recompute if the deleted load
balancer has
> > > > > +             * health checks configured. */
> > > > > +            return false;
> > > > > +        }
> > > > > +
> > > > > +        const struct uuid *lb_uuid =
> > > > > +                &trk_lb->lb->nlb->header_.uuid;
> > > > > +
> > > > > +        lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
lb_uuid);
> > > > > +        ovs_assert(lb_dps);
> > > > > +        hmap_remove(lb_datapaths_map, &lb_dps->hmap_node);
> > > > > +        ovn_lb_datapaths_destroy(lb_dps);
> > > > > +    }
> > > > > +
> > > > > +    LIST_FOR_EACH (trk_lb, list_node,
> > > > > +                   &trk_lb_data->tracked_updated_lbs.updated) {
> > > > > +        if (trk_lb->health_checks) {
> > > > > +            /* Fall back to recompute if the created/updated load
> > > balancer has
> > > > > +             * health checks configured. */
> > > > > +            return false;
> > > > > +        }
> > > > > +
> > > > > +        const struct uuid *lb_uuid =
> > > > > +                &trk_lb->lb->nlb->header_.uuid;
> > > > > +        lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
lb_uuid);
> > > > > +        if (!lb_dps) {
> > > > > +            lb_dps = ovn_lb_datapaths_create(trk_lb->lb,
> > > > > +
ods_size(ls_datapaths),
> > > > > +
ods_size(lr_datapaths));
> > > > > +            hmap_insert(lb_datapaths_map, &lb_dps->hmap_node,
> > > > > +                        uuid_hash(lb_uuid));
> > > > > +        }
> > > > > +    }
> > > > > +
> > > > > +    struct ovn_lb_group_datapaths *lb_group_dps;
> > > > > +    struct tracked_lb_group *trk_lb_group;
> > > > > +    LIST_FOR_EACH_SAFE (trk_lb_group, list_node,
> > > > > +
> > >  &trk_lb_data->tracked_deleted_lb_groups.updated) {
> > > > > +        const struct uuid *lb_uuid =
&trk_lb_group->lb_group->uuid;
> > > > > +        lb_group_dps =
> > > ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > > > > +                                                   lb_uuid);
> > > > > +        ovs_assert(lb_group_dps);
> > > > > +        hmap_remove(lb_group_datapaths_map,
&lb_group_dps->hmap_node);
> > > > > +        ovn_lb_group_datapaths_destroy(lb_group_dps);
> > > > > +    }
> > > > > +
> > > > > +    LIST_FOR_EACH_SAFE (trk_lb_group, list_node,
> > > > > +
> > >  &trk_lb_data->tracked_updated_lb_groups.updated) {
> > > > > +        const struct uuid *lb_uuid =
&trk_lb_group->lb_group->uuid;
> > > > > +        lb_group_dps =
> > > ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > > > > +                                                   lb_uuid);
> > > > > +        if (!lb_group_dps) {
> > > > > +            lb_group_dps = ovn_lb_group_datapaths_create(
> > > > > +                trk_lb_group->lb_group, ods_size(ls_datapaths),
> > > > > +                ods_size(lr_datapaths));
> > > > > +            hmap_insert(lb_group_datapaths_map,
> > > &lb_group_dps->hmap_node,
> > > > > +                        uuid_hash(lb_uuid));
> > > > > +        }
> > > > > +    }
> > > > > +
> > > > > +    return true;
> > > > > +}
> > > > > +
> > > > >   struct multicast_group {
> > > > >       const char *name;
> > > > >       uint16_t key;               /*
> > > OVN_MIN_MULTICAST...OVN_MAX_MULTICAST. */
> > > > > diff --git a/northd/northd.h b/northd/northd.h
> > > > > index 7d92028c7d..7d17921fa2 100644
> > > > > --- a/northd/northd.h
> > > > > +++ b/northd/northd.h
> > > > > @@ -347,6 +347,13 @@ bool lflow_handle_northd_ls_changes(struct
> > > ovsdb_idl_txn *ovnsb_txn,
> > > > >   bool northd_handle_sb_port_binding_changes(
> > > > >       const struct sbrec_port_binding_table *, struct hmap
*ls_ports);
> > > > >
> > > > > +struct tracked_lb_data;
> > > > > +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);
> > > > > +
> > > > >   void build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
> > > > >                        const struct nbrec_bfd_table *,
> > > > >                        const struct sbrec_bfd_table *,
> > > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > > > > index e79d33b2ae..dd0bd8b36a 100644
> > > > > --- a/tests/ovn-northd.at
> > > > > +++ b/tests/ovn-northd.at
> > > > > @@ -9681,3 +9681,126 @@ AT_CHECK([grep "lr_in_gw_redirect"
R1flows |sed
> > > s'/table=../table=??/' |sort], [
> > > > >
> > > > >   AT_CLEANUP
> > > > >   ])
> > > > > +
> > > > > +OVN_FOR_EACH_NORTHD_NO_HV([
> > > > > +AT_SETUP([Load balancer incremental processing])
> > > > > +ovn_start
> > > > > +
> > > > > +check_engine_stats() {
> > > > > +  northd_comp=$1
> > > > > +  lflow_comp=$2
> > > > > +
> > > > > +  AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE
inc-engine/show-stats
> > > northd_lb_data recompute], [0], [0
> > > > > +])
> > > > > +
> > > > > +  northd_recompute_ct=$(as northd ovn-appctl -t NORTHD_TYPE
> > > inc-engine/show-stats northd recompute)
> > > > > +  if [[ "$northd_comp" == "norecompute" ]]; then
> > > > > +    check test "$northd_recompute_ct" -eq "0"
> > > > > +  else
> > > > > +    check test "$northd_recompute_ct" -ne "0"
> > > > > +  fi
> > > > > +
> > > > > +  lflow_recompute_ct=$(as northd ovn-appctl -t NORTHD_TYPE
> > > inc-engine/show-stats lflow recompute)
> > > > > +  if [[ "$lflow_comp" == "norecompute" ]]; then
> > > > > +    check test "$lflow_recompute_ct" -eq "0"
> > > > > +  else
> > > > > +    check test "$lflow_recompute_ct" -ne "0"
> > > > > +  fi
> > > > > +}
> > > > > +
> > > > > +# Test I-P for load balancers.
> > > > > +# Presently ovn-northd handles I-P for NB LBs in northd_lb_data
engine
> > > node
> > > > > +# only.
> > > > > +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 norecompute recompute
> > > > > +
> > > > > +check ovn-nbctl --wait=sb set load_balancer .
> > > ip_port_mappings:10.0.0.3=sw0-p1:10.0.0.2
> > > > > +check_engine_stats norecompute recompute
> > > > > +
> > > > > +check ovn-nbctl --wait=sb set load_balancer . options:foo=bar
> > > > > +check_engine_stats norecompute recompute
> > > > > +
> > > > > +check ovn-nbctl --wait=sb -- lb-add lb2 20.0.0.10:80 20.0.0.20:80
--
> > > lb-add lb3 30.0.0.10:80 30.0.0.20:80
> > > > > +check_engine_stats norecompute recompute
> > > > > +
> > > > > +check ovn-nbctl --wait=sb -- lb-del lb2 -- lb-del lb3
> > > > > +check_engine_stats norecompute recompute
> > > > > +
> > > > > +AT_CHECK([ovn-nbctl --wait=sb \
> > > > > +          -- --id=@hc create Load_Balancer_Health_Check
> > > vip="10.0.0.10\:80" \
> > > > > +             options:failure_count=100 \
> > > > > +          -- add Load_Balancer . health_check @hc | uuidfilt],
[0],
> > > [<0>
> > > > > +])
> > > > > +check_engine_stats recompute recompute
> > > > > +
> > > > > +# Any change to load balancer health check should also result in
full
> > > recompute
> > > > > +# of northd node (but not northd_lb_data node)
> > > > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > > +check ovn-nbctl --wait=sb set load_balancer_health_check .
> > > options:foo=bar1
> > > > > +check_engine_stats recompute recompute
> > > > > +
> > > > > +# Delete the health check from the load balancer.  northd engine
node
> > > should do a full recompute.
> > > > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > > +check ovn-nbctl --wait=sb clear Load_Balancer . health_check
> > > > > +check_engine_stats recompute recompute
> > > > > +
> > > > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > > +check ovn-nbctl ls-add sw0
> > > > > +check ovn-nbctl --wait=sb lr-add lr0
> > > > > +check_engine_stats recompute recompute
> > > > > +
> > > > > +# Associate lb1 to 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-add sw0 lb1
> > > > > +check_engine_stats recompute 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 recompute recompute
> > > > > +
> > > > > +# Test load balancer group now
> > > > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > > +lbg1_uuid=$(ovn-nbctl create load_balancer_group name=lbg1)
> > > > > +check_engine_stats norecompute recompute
> > > > > +
> > > > > +lb1_uuid=$(fetch_column nb:Load_Balancer _uuid)
> > > > > +
> > > > > +# Add lb to the lbg1 group
> > > > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > > +check ovn-nbctl add load_balancer_group . load_Balancer $lb1_uuid
> > > > > +check_engine_stats norecompute recompute
> > > > > +
> > > > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > > +check ovn-nbctl clear load_balancer_group . load_Balancer
> > > > > +check_engine_stats norecompute recompute
> > > > > +
> > > > > +# Add back lb to the lbg1 group
> > > > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > > +check ovn-nbctl add load_balancer_group . load_Balancer $lb1_uuid
> > > > > +check_engine_stats norecompute 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 recompute recompute
> > > > > +
> > > > > +# Update lb and this should result in 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 recompute 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 recompute recompute
> > > > > +
> > > > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > > +check ovn-nbctl add logical_router lr0 load_balancer_group
$lbg1_uuid
> > > > > +check_engine_stats recompute recompute
> > > > > +
> > > > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > > +check ovn-nbctl clear logical_router lr0 load_balancer_group
> > > > > +check_engine_stats recompute recompute
> > > > > +
> > > > > +AT_CLEANUP
> > > > > +])
> > > >
> > > >
> > > > _______________________________________________
> > > > dev mailing list
> > > > dev@openvswitch.org
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev