[ovs-dev,v2,8/8] northd: Handle load balancer changes for a logical router.

From: Numan Siddique <numans@ovn.org>

When a logical router gets updated due to load balancer
or load balancer groups changes, it is now incrementally
handled in the 'northd' engine node.  Other logical router
updates result in the full recompute of 'northd' engine node.

lflow engine node still falls back to recompute due to
logical router changes.

Signed-off-by: Numan Siddique <numans@ovn.org>
---
 lib/lb.c                 |  32 +++-
 lib/lb.h                 |   5 +
 northd/en-lflow.c        |   5 +
 northd/en-northd.c       |  20 ++
 northd/en-northd.h       |   1 +
 northd/inc-proc-northd.c |   3 +-
 northd/northd.c          | 392 ++++++++++++++++++++++++++++++++++-----
 northd/northd.h          |   5 +
 tests/ovn-northd.at      |  12 +-
 9 files changed, 422 insertions(+), 53 deletions(-)

On Fri, Jul 7, 2023 at 1:56 PM <numans@ovn.org> wrote:
>
> From: Numan Siddique <numans@ovn.org>
>
> When a logical router gets updated due to load balancer
> or load balancer groups changes, it is now incrementally
> handled in the 'northd' engine node.  Other logical router
> updates result in the full recompute of 'northd' engine node.
>
> lflow engine node still falls back to recompute due to
> logical router changes.
>
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>  lib/lb.c                 |  32 +++-
>  lib/lb.h                 |   5 +
>  northd/en-lflow.c        |   5 +
>  northd/en-northd.c       |  20 ++
>  northd/en-northd.h       |   1 +
>  northd/inc-proc-northd.c |   3 +-
>  northd/northd.c          | 392 ++++++++++++++++++++++++++++++++++-----
>  northd/northd.h          |   5 +
>  tests/ovn-northd.at      |  12 +-
>  9 files changed, 422 insertions(+), 53 deletions(-)
>
> diff --git a/lib/lb.c b/lib/lb.c
> index a0426cc42e..a8b694d0b3 100644
> --- a/lib/lb.c
> +++ b/lib/lb.c
> @@ -1082,7 +1082,10 @@ ovn_lb_datapaths_add_lr(struct ovn_lb_datapaths
*lb_dps, size_t n,
>                          struct ovn_datapath **ods)
>  {
>      for (size_t i = 0; i < n; i++) {
> -        bitmap_set1(lb_dps->nb_lr_map, ods[i]->index);
> +        if (!bitmap_is_set(lb_dps->nb_lr_map, ods[i]->index)) {
> +            bitmap_set1(lb_dps->nb_lr_map, ods[i]->index);
> +            lb_dps->n_nb_lr++;
> +        }
>      }
>  }
>
> @@ -1110,6 +1113,18 @@ ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths
*lb_dps, size_t n,
>      }
>  }
>
> +void
> +ovn_lb_datapaths_remove_lr(struct ovn_lb_datapaths *lb_dps, size_t n,
> +                           struct ovn_datapath **ods)
> +{
> +    for (size_t i = 0; i < n; i++) {
> +        if (bitmap_is_set(lb_dps->nb_lr_map, ods[i]->index)) {
> +            bitmap_set0(lb_dps->nb_lr_map, ods[i]->index);
> +            lb_dps->n_nb_lr--;
> +        }
> +    }
> +}
> +
>  struct ovn_lb_group_datapaths *
>  ovn_lb_group_datapaths_create(const struct ovn_lb_group *lb_group,
>                                size_t n_ls_datapaths,
> @@ -1175,5 +1190,18 @@ void
>  ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *lbg_dps,
>                                struct ovn_datapath *lr)
>  {
> -    bitmap_set1(lbg_dps->nb_lr_map, lr->index);
> +    if (!bitmap_is_set(lbg_dps->nb_lr_map, lr->index)) {
> +        bitmap_set1(lbg_dps->nb_lr_map, lr->index);
> +        lbg_dps->n_nb_lr++;
> +    }
> +}
> +
> +void
> +ovn_lb_group_datapaths_remove_lr(struct ovn_lb_group_datapaths *lbg_dps,
> +                                 struct ovn_datapath *lr)
> +{
> +    if (bitmap_is_set(lbg_dps->nb_lr_map, lr->index)) {
> +        bitmap_set1(lbg_dps->nb_lr_map, lr->index);
> +        lbg_dps->n_nb_lr--;
> +    }
>  }
> diff --git a/lib/lb.h b/lib/lb.h
> index 08723e8ef3..91eec0199b 100644
> --- a/lib/lb.h
> +++ b/lib/lb.h
> @@ -185,6 +185,8 @@ void ovn_lb_datapaths_set_ls(struct ovn_lb_datapaths
*, size_t n,
>
>  void ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *, size_t n,
>                                  struct ovn_datapath **);
> +void ovn_lb_datapaths_remove_lr(struct ovn_lb_datapaths *, size_t n,
> +                                struct ovn_datapath **);
>
>  struct ovn_lb_group_datapaths {
>      struct hmap_node hmap_node;
> @@ -214,6 +216,9 @@ void ovn_lb_group_datapaths_remove_ls(struct
ovn_lb_group_datapaths *,
>
>  void ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *,
>                                     struct ovn_datapath *lr);
> +void ovn_lb_group_datapaths_remove_lr(struct ovn_lb_group_datapaths *,
> +                                      struct ovn_datapath *lr);
> +
>  struct ovn_controller_lb {
>      struct hmap_node hmap_node;
>
> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> index db1bcbccd6..ea51f4e3e0 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -102,6 +102,11 @@ lflow_northd_handler(struct engine_node *node,
>      if (!northd_data->change_tracked) {
>          return false;
>      }
> +
> +    if (northd_data->lrouters_changed) {
> +        return false;
> +    }

I think lrouters_changed is not needed. If lrouters related data is
changed, we should set change_tracked to false (call the
destroy_northd_data_tracked_changes), because lrouter data is part of
northd_data nad we don't track lrouter changes yet.

> +
>      const struct engine_context *eng_ctx = engine_get_context();
>      struct lflow_data *lflow_data = data;
>
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index b3c03c54bd..a321eff323 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -188,6 +188,26 @@ northd_nb_logical_switch_handler(struct engine_node
*node,
>      return true;
>  }
>
> +bool
> +northd_nb_logical_router_handler(struct engine_node *node,
> +                                 void *data)
> +{
> +    struct northd_data *nd = data;
> +    struct northd_input input_data;
> +
> +    northd_get_input_data(node, &input_data);
> +
> +    if (!northd_handle_lr_changes(&input_data, nd)) {
> +        return false;
> +    }
> +
> +    if (nd->change_tracked) {
> +        engine_set_node_state(node, EN_UPDATED);

change_tracked is supposed to indicate if the changes in the engine node is
tracked, meaning the node depending on this one can potentailly
incrementally process the changes.
It is possible that the node is updated but the changes are not tracked. In
this case most likely the nodes depending on it will need to be recomputed.
Whether the node state is EN_UPDATED depends on if the node is updated. I
think the northd_handle_lr_changes can take a parameter "updated" to
indicate if it is updated.

> +    }
> +
> +    return true;
> +}
> +
>  bool
>  northd_sb_port_binding_handler(struct engine_node *node,
>                                 void *data)
> diff --git a/northd/en-northd.h b/northd/en-northd.h
> index 5674f4390c..739aa5e87f 100644
> --- a/northd/en-northd.h
> +++ b/northd/en-northd.h
> @@ -16,6 +16,7 @@ void en_northd_cleanup(void *data);
>  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_nb_logical_router_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);
>
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> index f5b7998f1f..362dd5e87a 100644
> --- a/northd/inc-proc-northd.c
> +++ b/northd/inc-proc-northd.c
> @@ -156,7 +156,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>
>      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);
>      engine_add_input(&en_northd, &en_nb_mirror, NULL);
>      engine_add_input(&en_northd, &en_nb_meter, NULL);
>      engine_add_input(&en_northd, &en_nb_static_mac_binding, NULL);
> @@ -185,6 +184,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>                       northd_nb_nb_global_handler);
>      engine_add_input(&en_northd, &en_nb_logical_switch,
>                       northd_nb_logical_switch_handler);
> +    engine_add_input(&en_northd, &en_nb_logical_router,
> +                     northd_nb_logical_router_handler);
>
>      engine_add_input(&en_mac_binding_aging, &en_nb_nb_global, NULL);
>      engine_add_input(&en_mac_binding_aging, &en_sb_mac_binding, NULL);
> diff --git a/northd/northd.c b/northd/northd.c
> index a7761285de..4393adc2ad 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -4021,6 +4021,90 @@ associate_ls_lb_groups(struct ovn_datapath *ls_od,
>      }
>  }
>
> +static void
> +associate_lr_lbs(struct ovn_datapath *lr_od, struct hmap
*lb_datapaths_map)
> +{
> +    struct ovn_lb_datapaths *lb_dps;
> +
> +    free(lr_od->lb_uuids);
> +    lr_od->lb_uuids = xcalloc(lr_od->nbr->n_load_balancer,
> +                              sizeof *lr_od->lb_uuids);
> +    lr_od->n_lb_uuids = lr_od->nbr->n_load_balancer;
> +
> +    if (!lr_od->lb_ips) {
> +        lr_od->lb_ips = ovn_lb_ip_set_create();
> +    }
> +
> +    for (size_t i = 0; i < lr_od->nbr->n_load_balancer; i++) {
> +        const struct uuid *lb_uuid =
> +            &lr_od->nbr->load_balancer[i]->header_.uuid;
> +        lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> +        ovs_assert(lb_dps);
> +        ovn_lb_datapaths_add_lr(lb_dps, 1, &lr_od);
> +        build_lrouter_lb_ips(lr_od->lb_ips, lb_dps->lb);
> +        lr_od->lb_uuids[i] = *lb_uuid;
> +    }
> +}
> +
> +static void
> +associate_lr_lb_groups(struct ovn_datapath *lr_od,
> +                       struct hmap *lb_group_datapaths_map,
> +                       struct hmap *lb_datapaths_map)
> +{
> +    const struct nbrec_load_balancer_group *nbrec_lb_group;
> +    struct ovn_lb_group_datapaths *lb_group_dps;
> +
> +    free(lr_od->lb_group_uuids);
> +    lr_od->lb_group_uuids = xcalloc(lr_od->nbr->n_load_balancer_group,
> +                                    sizeof *lr_od->lb_group_uuids);
> +    lr_od->n_lb_group_uuids = lr_od->nbr->n_load_balancer_group;
> +
> +    /* Checking load balancer groups first, starting from the largest
one,
> +     * to more efficiently copy IP sets. */
> +    size_t largest_group = 0;
> +
> +    for (size_t i = 1; i < lr_od->nbr->n_load_balancer_group; i++) {
> +        if (lr_od->nbr->load_balancer_group[i]->n_load_balancer >
> +
 lr_od->nbr->load_balancer_group[largest_group]->n_load_balancer) {
> +            largest_group = i;
> +        }
> +    }
> +
> +    for (size_t i = 0; i < lr_od->nbr->n_load_balancer_group; i++) {
> +        size_t idx = (i + largest_group) %
lr_od->nbr->n_load_balancer_group;
> +
> +        nbrec_lb_group = lr_od->nbr->load_balancer_group[idx];
> +        const struct uuid *lb_group_uuid = &nbrec_lb_group->header_.uuid;
> +
> +        lb_group_dps =
> +            ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> +                                        lb_group_uuid);
> +        ovs_assert(lb_group_dps);
> +        ovn_lb_group_datapaths_add_lr(lb_group_dps, lr_od);
> +
> +        if (!lr_od->lb_ips) {
> +            lr_od->lb_ips =
> +                ovn_lb_ip_set_clone(lb_group_dps->lb_group->lb_ips);
> +        } else {
> +            for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs; j++) {
> +                build_lrouter_lb_ips(lr_od->lb_ips,
> +                                     lb_group_dps->lb_group->lbs[j]);
> +            }
> +        }
> +
> +        lr_od->lb_group_uuids[i] = *lb_group_uuid;
> +
> +        struct ovn_lb_datapaths *lb_dps;
> +        for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs; j++) {
> +            const struct uuid *lb_uuid =
> +                &lb_group_dps->lb_group->lbs[j]->nlb->header_.uuid;
> +            lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> +            ovs_assert(lb_dps);
> +            ovn_lb_datapaths_add_lr(lb_dps, 1, &lr_od);
> +        }
> +    }
> +}
> +
>  static void
>  build_lb_datapaths(const struct hmap *lbs, const struct hmap *lb_groups,
>                     struct ovn_datapaths *ls_datapaths,
> @@ -4028,7 +4112,6 @@ build_lb_datapaths(const struct hmap *lbs, const
struct hmap *lb_groups,
>                     struct hmap *lb_datapaths_map,
>                     struct hmap *lb_group_datapaths_map)
>  {
> -    const struct nbrec_load_balancer_group *nbrec_lb_group;
>      struct ovn_lb_group_datapaths *lb_group_dps;
>      const struct ovn_lb_group *lb_group;
>      struct ovn_lb_datapaths *lb_dps;
> @@ -4062,53 +4145,8 @@ build_lb_datapaths(const struct hmap *lbs, const
struct hmap *lb_groups,
>
>      HMAP_FOR_EACH (od, key_node, &lr_datapaths->datapaths) {
>          ovs_assert(od->nbr);
> -
> -        /* Checking load balancer groups first, starting from the
largest one,
> -         * to more efficiently copy IP sets. */
> -        size_t largest_group = 0;
> -
> -        for (size_t i = 1; i < od->nbr->n_load_balancer_group; i++) {
> -            if (od->nbr->load_balancer_group[i]->n_load_balancer >
> -
 od->nbr->load_balancer_group[largest_group]->n_load_balancer) {
> -                largest_group = i;
> -            }
> -        }
> -
> -        for (size_t i = 0; i < od->nbr->n_load_balancer_group; i++) {
> -            size_t idx = (i + largest_group) %
od->nbr->n_load_balancer_group;
> -
> -            nbrec_lb_group = od->nbr->load_balancer_group[idx];
> -            const struct uuid *lb_group_uuid =
&nbrec_lb_group->header_.uuid;
> -
> -            lb_group_dps =
> -                ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> -                                            lb_group_uuid);
> -            ovs_assert(lb_group_dps);
> -            ovn_lb_group_datapaths_add_lr(lb_group_dps, od);
> -
> -            if (!od->lb_ips) {
> -                od->lb_ips =
> -                    ovn_lb_ip_set_clone(lb_group_dps->lb_group->lb_ips);
> -            } else {
> -                for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs;
j++) {
> -                    build_lrouter_lb_ips(od->lb_ips,
> -                                         lb_group_dps->lb_group->lbs[j]);
> -                }
> -            }
> -        }
> -
> -        if (!od->lb_ips) {
> -            od->lb_ips = ovn_lb_ip_set_create();
> -        }
> -
> -        for (size_t i = 0; i < od->nbr->n_load_balancer; i++) {
> -            const struct uuid *lb_uuid =
> -                &od->nbr->load_balancer[i]->header_.uuid;
> -            lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> -            ovs_assert(lb_dps);
> -            ovn_lb_datapaths_add_lr(lb_dps, 1, &od);
> -            build_lrouter_lb_ips(od->lb_ips, lb_dps->lb);
> -        }
> +        associate_lr_lb_groups(od, lb_group_datapaths_map,
lb_datapaths_map);
> +        associate_lr_lbs(od, lb_datapaths_map);
>      }
>
>      HMAP_FOR_EACH (lb_group_dps, hmap_node, lb_group_datapaths_map) {
> @@ -4978,6 +5016,7 @@ destroy_northd_data_tracked_changes(struct
northd_data *nd)
>      }
>
>      nd->change_tracked = false;
> +    nd->lrouters_changed = false;
>  }
>
>  /* Check if a changed LSP can be handled incrementally within the I-P
engine
> @@ -5513,6 +5552,263 @@ fail:
>      return false;
>  }
>
> +/* Returns true if the logical router has changes which are not
> + * incrementally handled.
> + * Presently supports i-p for the below changes:
> + *    - load balancers and load balancer groups.
> + */
> +static bool
> +check_unsupported_inc_proc_for_lr_changes(
> +    const struct nbrec_logical_router *lr)
> +{
> +    /* Check if the columns are changed in this row. */
> +    enum nbrec_logical_router_column_id col;
> +    for (col = 0; col < NBREC_LOGICAL_ROUTER_N_COLUMNS; col++) {
> +        if (nbrec_logical_router_is_updated(lr, col)) {
> +            if (col == NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER ||
> +                col == NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER_GROUP) {
> +                continue;
> +            }
> +            return true;
> +        }
> +    }
> +
> +    /* Check if the referenced rows are changed.
> +       XXX: Need a better OVSDB IDL interface for this check. */
> +    for (size_t i = 0; i < lr->n_ports; i++) {
> +        if (nbrec_logical_router_port_row_get_seqno(lr->ports[i],
> +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> +            return true;
> +        }
> +    }
> +    if (lr->copp && nbrec_copp_row_get_seqno(lr->copp,
> +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> +        return true;
> +    }
> +    for (size_t i = 0; i < lr->n_nat; i++) {
> +        if (nbrec_nat_row_get_seqno(lr->nat[i],
> +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> +            return true;
> +        }
> +    }
> +    for (size_t i = 0; i < lr->n_policies; i++) {
> +        if (nbrec_logical_router_policy_row_get_seqno(lr->policies[i],
> +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> +            return true;
> +        }
> +    }
> +    for (size_t i = 0; i < lr->n_static_routes; i++) {
> +        if (nbrec_logical_router_static_route_row_get_seqno(
> +            lr->static_routes[i], OVSDB_IDL_CHANGE_MODIFY) > 0) {
> +            return true;
> +        }
> +    }
> +    return false;
> +}
> +
> +static bool
> +lr_check_and_handle_lb_and_lbgrp_changes(
> +    const struct nbrec_logical_router *changed_lr,
> +    struct hmap *lb_datapaths_map, struct hmap *lb_group_datapaths_map,
> +    struct ovn_datapath *od, bool *updated)
> +{
> +    ovs_assert(od->nbr);
> +    bool lbs_changed = true;
> +    if (!nbrec_logical_router_is_updated(changed_lr,
> +                        NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER) &&
> +        !nbrec_logical_router_is_updated(changed_lr,
> +                        NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER_GROUP)) {
> +        lbs_changed = false;
> +        for (size_t i = 0; i < changed_lr->n_load_balancer_group; i++) {
> +            if (nbrec_load_balancer_group_row_get_seqno(
> +                    changed_lr->load_balancer_group[i],
> +                    OVSDB_IDL_CHANGE_MODIFY) > 0) {
> +                lbs_changed = true;
> +                break;
> +            }
> +        }
> +
> +        if (!lbs_changed) {
> +            for (size_t i = 0; i < changed_lr->n_load_balancer; i++) {
> +                if (nbrec_load_balancer_row_get_seqno(
> +                        changed_lr->load_balancer[i],
> +                        OVSDB_IDL_CHANGE_MODIFY) > 0) {
> +                    lbs_changed = true;
> +                    break;
> +                }
> +            }
> +        }
> +
> +        if (!lbs_changed) {
> +            return true;
> +        }
> +    }
> +
> +    /* We need to do 2 things to handle the router for load balancer or
> +     * load balancer groups.
> +     *
> +     * 1. Disassociate the logical router datapath from the already
associated
> +     *    load balancers/load balancer groups.
> +     * 2. Associate the logical router datapath with the updated
> +     *    load balancers/groups.
> +     * */
> +    struct ovn_lb_datapaths *lb_dps;
> +    size_t i, j;
> +    for (i = 0; i < od->n_lb_uuids; i++) {
> +        lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
&od->lb_uuids[i]);
> +        if (lb_dps) {
> +            if (lb_dps->lb->routable) {
> +                /* Can't handler if the load balancer has routable flag
set.
> +                 * This requires updating the router ports's routable
> +                 * addresses. */
> +                return false;
> +            }
> +            ovn_lb_datapaths_remove_lr(lb_dps, 1, &od);
> +        }
> +    }
> +
> +    struct ovn_lb_group_datapaths *lbg_dps;
> +    for (i = 0; i < od->n_lb_group_uuids; i++) {
> +        lbg_dps = ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> +                                              &od->lb_group_uuids[i]);
> +        if (lbg_dps) {
> +            ovn_lb_group_datapaths_remove_lr(lbg_dps, od);
> +
> +            if (install_ls_lb_from_router && od->n_ls_peers) {
> +                ovn_lb_group_datapaths_remove_ls(lbg_dps, od->n_ls_peers,
> +                                                 od->ls_peers);
> +            }
> +        }
> +
> +        for (j = 0; j < lbg_dps->lb_group->n_lbs; j++) {
> +            const struct uuid *lb_uuid =
> +                &lbg_dps->lb_group->lbs[j]->nlb->header_.uuid;
> +            lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> +            if (lb_dps) {
> +                if (lb_dps->lb->routable) {
> +                    /* Can't handler if the load balancer has routable
flag
> +                     * set.  This requires updating the router ports's
> +                     * routable addresses. */
> +                    return false;
> +                }
> +                ovn_lb_datapaths_remove_lr(lb_dps, 1, &od);
> +
> +                if (install_ls_lb_from_router && od->n_ls_peers) {
> +                    ovn_lb_datapaths_remove_ls(lb_dps, od->n_ls_peers,
> +                                               od->ls_peers);
> +                }
> +            }
> +        }
> +    }
> +
> +    ovn_lb_ip_set_destroy(od->lb_ips);
> +    od->lb_ips = NULL;
> +    associate_lr_lb_groups(od, lb_group_datapaths_map, lb_datapaths_map);
> +    associate_lr_lbs(od, lb_datapaths_map);
> +
> +    /* Do the job of build_lrouter_lbs_reachable_ips and
> +     * build_lswitch_lbs_from_lrouter for this lrouter datapath. */
> +    for (i = 0; i < od->nbr->n_load_balancer; i++) {
> +        lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> +
 &od->nbr->load_balancer[i]->header_.uuid);
> +        ovs_assert(lb_dps);
> +        if (lb_dps->lb->routable) {
> +            /* Can't handler if the load balancer has routable flag set.
> +             * This requires updating the router ports's routable
addresses. */
> +            return false;
> +        }
> +        build_lrouter_lb_reachable_ips(od, lb_dps->lb);
> +
> +        if (install_ls_lb_from_router && od->n_ls_peers) {
> +            ovn_lb_datapaths_add_ls(lb_dps, od->n_ls_peers,
od->ls_peers);
> +        }
> +    }
> +
> +    for (i = 0; i < od->nbr->n_load_balancer_group; i++) {
> +        const struct nbrec_load_balancer_group *nbrec_lb_group =
> +            od->nbr->load_balancer_group[i];
> +        lbg_dps = ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> +
 &nbrec_lb_group->header_.uuid);
> +        ovs_assert(lbg_dps);
> +        if (install_ls_lb_from_router && od->n_ls_peers) {
> +            ovn_lb_group_datapaths_add_ls(lbg_dps, od->n_ls_peers,
> +                                          od->ls_peers);
> +        }
> +
> +        for (j = 0; j < lbg_dps->lb_group->n_lbs; j++) {
> +            build_lrouter_lb_reachable_ips(od,
lbg_dps->lb_group->lbs[j]);
> +
> +            if (install_ls_lb_from_router && od->n_ls_peers) {
> +                const struct uuid *lb_uuid =
> +                    &lbg_dps->lb_group->lbs[j]->nlb->header_.uuid;
> +                lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
lb_uuid);
> +                ovs_assert(lb_dps);
> +                if (lb_dps->lb->routable) {
> +                    /* Can't handler if the load balancer has routable
flag
> +                     * set.  This requires updating the router ports's
> +                     * routable addresses. */
> +                    return false;
> +                }
> +                ovn_lb_datapaths_add_ls(lb_dps, od->n_ls_peers,
od->ls_peers);
> +            }
> +        }
> +    }
> +
> +    *updated = true;
> +    /* Re-evaluate 'od->has_lb_vip' */
> +    init_lb_for_datapath(od);
> +    return true;
> +}
> +
> +
> +/* Return true if changes are handled incrementally, false otherwise.
> + * When there are any changes, try to track what's exactly changed and
set
> + * northd_data->change_tracked accordingly: change tracked - true,
otherwise,
> + * false. */
> +bool
> +northd_handle_lr_changes(const struct northd_input *ni,
> +                         struct northd_data *nd)
> +{
> +    const struct nbrec_logical_router *changed_lr;
> +
> +    bool updated = false;
> +
> +    NBREC_LOGICAL_ROUTER_TABLE_FOR_EACH_TRACKED (changed_lr,
> +
ni->nbrec_logical_router_table) {
> +        if (nbrec_logical_router_is_new(changed_lr) ||
> +            nbrec_logical_router_is_deleted(changed_lr)) {
> +            goto fail;
> +        }
> +
> +        struct ovn_datapath *od = ovn_datapath_find(
> +                                    &nd->lr_datapaths.datapaths,
> +                                    &changed_lr->header_.uuid);
> +
> +        /* Presently only able to handle load balancer and
> +         * load balancer group changes. */
> +        if (check_unsupported_inc_proc_for_lr_changes(changed_lr)) {
> +            goto fail;
> +        }
> +
> +        if (!lr_check_and_handle_lb_and_lbgrp_changes(changed_lr,
> +                                                &nd->lb_datapaths_map,
> +
 &nd->lb_group_datapaths_map,
> +                                                od, &updated)) {
> +            goto fail;
> +        }
> +    }
> +
> +    if (updated) {
> +        nd->change_tracked = true;

Updated doesn't mean the change is tracked. Please see the earlier comments.

> +        nd->lrouters_changed = true;
> +    }
> +
> +    return true;
> +fail:
> +    destroy_northd_data_tracked_changes(nd);

I think we don't need "fail:" here. Instead, just return false and call the
destroy_northd_data_tracked_changes if lrouter is updated (but not tracked)
or can't be handled.

Thanks,
Han

> +    return false;
> +}
> +
>  bool
>  northd_handle_sb_port_binding_changes(
>      const struct sbrec_port_binding_table *sbrec_port_binding_table,
> diff --git a/northd/northd.h b/northd/northd.h
> index a79cc470c3..765720849a 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -119,8 +119,11 @@ struct northd_data {
>      struct chassis_features features;
>      struct sset svc_monitor_lsps;
>      struct hmap svc_monitor_map;
> +
> +    /* Tracked data. */
>      bool change_tracked;
>      struct tracked_ls_changes tracked_ls_changes;
> +    bool lrouters_changed;
>  };
>
>  struct lflow_data {
> @@ -342,6 +345,8 @@ void ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
>  bool northd_handle_ls_changes(struct ovsdb_idl_txn *,
>                                const struct northd_input *,
>                                struct northd_data *);
> +bool northd_handle_lr_changes(const struct northd_input *,
> +                              struct northd_data *);
>  void destroy_northd_data_tracked_changes(struct northd_data *);
>  void northd_destroy(struct northd_data *data);
>  void northd_init(struct northd_data *data);
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index d3a5851e35..286b311242 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -9760,6 +9760,14 @@ 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 norecompute recompute
>
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +check ovn-nbctl lr-lb-add lr0 lb1
> +check_engine_stats norecompute recompute
> +
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +check ovn-nbctl lr-lb-del lr0 lb1
> +check_engine_stats norecompute 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)
> @@ -9796,11 +9804,11 @@ check_engine_stats norecompute 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_engine_stats norecompute 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
> +check_engine_stats norecompute recompute
>
>  AT_CLEANUP
>  ])
> --
> 2.40.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On Tue, Jul 18, 2023 at 11:08 AM Han Zhou <hzhou@ovn.org> wrote:
>
> On Fri, Jul 7, 2023 at 1:56 PM <numans@ovn.org> wrote:
> >
> > From: Numan Siddique <numans@ovn.org>
> >
> > When a logical router gets updated due to load balancer
> > or load balancer groups changes, it is now incrementally
> > handled in the 'northd' engine node.  Other logical router
> > updates result in the full recompute of 'northd' engine node.
> >
> > lflow engine node still falls back to recompute due to
> > logical router changes.
> >
> > Signed-off-by: Numan Siddique <numans@ovn.org>
> > ---
> >  lib/lb.c                 |  32 +++-
> >  lib/lb.h                 |   5 +
> >  northd/en-lflow.c        |   5 +
> >  northd/en-northd.c       |  20 ++
> >  northd/en-northd.h       |   1 +
> >  northd/inc-proc-northd.c |   3 +-
> >  northd/northd.c          | 392 ++++++++++++++++++++++++++++++++++-----
> >  northd/northd.h          |   5 +
> >  tests/ovn-northd.at      |  12 +-
> >  9 files changed, 422 insertions(+), 53 deletions(-)
> >
> > diff --git a/lib/lb.c b/lib/lb.c
> > index a0426cc42e..a8b694d0b3 100644
> > --- a/lib/lb.c
> > +++ b/lib/lb.c
> > @@ -1082,7 +1082,10 @@ ovn_lb_datapaths_add_lr(struct ovn_lb_datapaths
> *lb_dps, size_t n,
> >                          struct ovn_datapath **ods)
> >  {
> >      for (size_t i = 0; i < n; i++) {
> > -        bitmap_set1(lb_dps->nb_lr_map, ods[i]->index);
> > +        if (!bitmap_is_set(lb_dps->nb_lr_map, ods[i]->index)) {
> > +            bitmap_set1(lb_dps->nb_lr_map, ods[i]->index);
> > +            lb_dps->n_nb_lr++;
> > +        }
> >      }
> >  }
> >
> > @@ -1110,6 +1113,18 @@ ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths
> *lb_dps, size_t n,
> >      }
> >  }
> >
> > +void
> > +ovn_lb_datapaths_remove_lr(struct ovn_lb_datapaths *lb_dps, size_t n,
> > +                           struct ovn_datapath **ods)
> > +{
> > +    for (size_t i = 0; i < n; i++) {
> > +        if (bitmap_is_set(lb_dps->nb_lr_map, ods[i]->index)) {
> > +            bitmap_set0(lb_dps->nb_lr_map, ods[i]->index);
> > +            lb_dps->n_nb_lr--;
> > +        }
> > +    }
> > +}
> > +
> >  struct ovn_lb_group_datapaths *
> >  ovn_lb_group_datapaths_create(const struct ovn_lb_group *lb_group,
> >                                size_t n_ls_datapaths,
> > @@ -1175,5 +1190,18 @@ void
> >  ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *lbg_dps,
> >                                struct ovn_datapath *lr)
> >  {
> > -    bitmap_set1(lbg_dps->nb_lr_map, lr->index);
> > +    if (!bitmap_is_set(lbg_dps->nb_lr_map, lr->index)) {
> > +        bitmap_set1(lbg_dps->nb_lr_map, lr->index);
> > +        lbg_dps->n_nb_lr++;
> > +    }
> > +}
> > +
> > +void
> > +ovn_lb_group_datapaths_remove_lr(struct ovn_lb_group_datapaths *lbg_dps,
> > +                                 struct ovn_datapath *lr)
> > +{
> > +    if (bitmap_is_set(lbg_dps->nb_lr_map, lr->index)) {
> > +        bitmap_set1(lbg_dps->nb_lr_map, lr->index);
> > +        lbg_dps->n_nb_lr--;
> > +    }
> >  }
> > diff --git a/lib/lb.h b/lib/lb.h
> > index 08723e8ef3..91eec0199b 100644
> > --- a/lib/lb.h
> > +++ b/lib/lb.h
> > @@ -185,6 +185,8 @@ void ovn_lb_datapaths_set_ls(struct ovn_lb_datapaths
> *, size_t n,
> >
> >  void ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *, size_t n,
> >                                  struct ovn_datapath **);
> > +void ovn_lb_datapaths_remove_lr(struct ovn_lb_datapaths *, size_t n,
> > +                                struct ovn_datapath **);
> >
> >  struct ovn_lb_group_datapaths {
> >      struct hmap_node hmap_node;
> > @@ -214,6 +216,9 @@ void ovn_lb_group_datapaths_remove_ls(struct
> ovn_lb_group_datapaths *,
> >
> >  void ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *,
> >                                     struct ovn_datapath *lr);
> > +void ovn_lb_group_datapaths_remove_lr(struct ovn_lb_group_datapaths *,
> > +                                      struct ovn_datapath *lr);
> > +
> >  struct ovn_controller_lb {
> >      struct hmap_node hmap_node;
> >
> > diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> > index db1bcbccd6..ea51f4e3e0 100644
> > --- a/northd/en-lflow.c
> > +++ b/northd/en-lflow.c
> > @@ -102,6 +102,11 @@ lflow_northd_handler(struct engine_node *node,
> >      if (!northd_data->change_tracked) {
> >          return false;
> >      }
> > +
> > +    if (northd_data->lrouters_changed) {
> > +        return false;
> > +    }
>
> I think lrouters_changed is not needed. If lrouters related data is
> changed, we should set change_tracked to false (call the
> destroy_northd_data_tracked_changes), because lrouter data is part of
> northd_data nad we don't track lrouter changes yet.


   -  With this patch we could are  handling router changes
incrementally in northd engine node if only the router's load_balancer
column has changed.
      If we set change_tracked to false, it means northd engine node
didn't handle the router changes due to load balancer column
      incrementally and it fell back to full recompute which is not
the cause.  This 'lrounters_changed'  indicates to the lflow engine
node
      that northd engine node did a compute (and not a recompute) and
logical routers have changed as part of this compute.

   -  Since lflow engine node doesn't yet handle the lrouter changes,
it falls back to full recompute.

   - It is also possible that in the same nb db update,  a load
balancer was updated which is associated with both logical switch and
logical router.
     In such cases we don't want to call -
destroy_northd_data_tracked_changes().  Even though lflow engine does
a recompute, from northd engine point of view,
     it is indicating what changed.  It is left to the dependent nodes
to decide if they want to handle incrementally or fall back to
recompute.

Let me know if you disagree with this.


 >
> > +
> >      const struct engine_context *eng_ctx = engine_get_context();
> >      struct lflow_data *lflow_data = data;
> >
> > diff --git a/northd/en-northd.c b/northd/en-northd.c
> > index b3c03c54bd..a321eff323 100644
> > --- a/northd/en-northd.c
> > +++ b/northd/en-northd.c
> > @@ -188,6 +188,26 @@ northd_nb_logical_switch_handler(struct engine_node
> *node,
> >      return true;
> >  }
> >
> > +bool
> > +northd_nb_logical_router_handler(struct engine_node *node,
> > +                                 void *data)
> > +{
> > +    struct northd_data *nd = data;
> > +    struct northd_input input_data;
> > +
> > +    northd_get_input_data(node, &input_data);
> > +
> > +    if (!northd_handle_lr_changes(&input_data, nd)) {
> > +        return false;
> > +    }
> > +
> > +    if (nd->change_tracked) {
> > +        engine_set_node_state(node, EN_UPDATED);
>
> change_tracked is supposed to indicate if the changes in the engine node is
> tracked, meaning the node depending on this one can potentailly
> incrementally process the changes.
> It is possible that the node is updated but the changes are not tracked. In
> this case most likely the nodes depending on it will need to be recomputed.
> Whether the node state is EN_UPDATED depends on if the node is updated. I
> think the northd_handle_lr_changes can take a parameter "updated" to
> indicate if it is updated.

Agree.  Will use "updated" parameter instead.

Thanks
Numan



>
> > +    }
> > +
> > +    return true;
> > +}
> > +
> >  bool
> >  northd_sb_port_binding_handler(struct engine_node *node,
> >                                 void *data)
> > diff --git a/northd/en-northd.h b/northd/en-northd.h
> > index 5674f4390c..739aa5e87f 100644
> > --- a/northd/en-northd.h
> > +++ b/northd/en-northd.h
> > @@ -16,6 +16,7 @@ void en_northd_cleanup(void *data);
> >  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_nb_logical_router_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);
> >
> > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> > index f5b7998f1f..362dd5e87a 100644
> > --- a/northd/inc-proc-northd.c
> > +++ b/northd/inc-proc-northd.c
> > @@ -156,7 +156,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> >
> >      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);
> >      engine_add_input(&en_northd, &en_nb_mirror, NULL);
> >      engine_add_input(&en_northd, &en_nb_meter, NULL);
> >      engine_add_input(&en_northd, &en_nb_static_mac_binding, NULL);
> > @@ -185,6 +184,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> >                       northd_nb_nb_global_handler);
> >      engine_add_input(&en_northd, &en_nb_logical_switch,
> >                       northd_nb_logical_switch_handler);
> > +    engine_add_input(&en_northd, &en_nb_logical_router,
> > +                     northd_nb_logical_router_handler);
> >
> >      engine_add_input(&en_mac_binding_aging, &en_nb_nb_global, NULL);
> >      engine_add_input(&en_mac_binding_aging, &en_sb_mac_binding, NULL);
> > diff --git a/northd/northd.c b/northd/northd.c
> > index a7761285de..4393adc2ad 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -4021,6 +4021,90 @@ associate_ls_lb_groups(struct ovn_datapath *ls_od,
> >      }
> >  }
> >
> > +static void
> > +associate_lr_lbs(struct ovn_datapath *lr_od, struct hmap
> *lb_datapaths_map)
> > +{
> > +    struct ovn_lb_datapaths *lb_dps;
> > +
> > +    free(lr_od->lb_uuids);
> > +    lr_od->lb_uuids = xcalloc(lr_od->nbr->n_load_balancer,
> > +                              sizeof *lr_od->lb_uuids);
> > +    lr_od->n_lb_uuids = lr_od->nbr->n_load_balancer;
> > +
> > +    if (!lr_od->lb_ips) {
> > +        lr_od->lb_ips = ovn_lb_ip_set_create();
> > +    }
> > +
> > +    for (size_t i = 0; i < lr_od->nbr->n_load_balancer; i++) {
> > +        const struct uuid *lb_uuid =
> > +            &lr_od->nbr->load_balancer[i]->header_.uuid;
> > +        lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> > +        ovs_assert(lb_dps);
> > +        ovn_lb_datapaths_add_lr(lb_dps, 1, &lr_od);
> > +        build_lrouter_lb_ips(lr_od->lb_ips, lb_dps->lb);
> > +        lr_od->lb_uuids[i] = *lb_uuid;
> > +    }
> > +}
> > +
> > +static void
> > +associate_lr_lb_groups(struct ovn_datapath *lr_od,
> > +                       struct hmap *lb_group_datapaths_map,
> > +                       struct hmap *lb_datapaths_map)
> > +{
> > +    const struct nbrec_load_balancer_group *nbrec_lb_group;
> > +    struct ovn_lb_group_datapaths *lb_group_dps;
> > +
> > +    free(lr_od->lb_group_uuids);
> > +    lr_od->lb_group_uuids = xcalloc(lr_od->nbr->n_load_balancer_group,
> > +                                    sizeof *lr_od->lb_group_uuids);
> > +    lr_od->n_lb_group_uuids = lr_od->nbr->n_load_balancer_group;
> > +
> > +    /* Checking load balancer groups first, starting from the largest
> one,
> > +     * to more efficiently copy IP sets. */
> > +    size_t largest_group = 0;
> > +
> > +    for (size_t i = 1; i < lr_od->nbr->n_load_balancer_group; i++) {
> > +        if (lr_od->nbr->load_balancer_group[i]->n_load_balancer >
> > +
>  lr_od->nbr->load_balancer_group[largest_group]->n_load_balancer) {
> > +            largest_group = i;
> > +        }
> > +    }
> > +
> > +    for (size_t i = 0; i < lr_od->nbr->n_load_balancer_group; i++) {
> > +        size_t idx = (i + largest_group) %
> lr_od->nbr->n_load_balancer_group;
> > +
> > +        nbrec_lb_group = lr_od->nbr->load_balancer_group[idx];
> > +        const struct uuid *lb_group_uuid = &nbrec_lb_group->header_.uuid;
> > +
> > +        lb_group_dps =
> > +            ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > +                                        lb_group_uuid);
> > +        ovs_assert(lb_group_dps);
> > +        ovn_lb_group_datapaths_add_lr(lb_group_dps, lr_od);
> > +
> > +        if (!lr_od->lb_ips) {
> > +            lr_od->lb_ips =
> > +                ovn_lb_ip_set_clone(lb_group_dps->lb_group->lb_ips);
> > +        } else {
> > +            for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs; j++) {
> > +                build_lrouter_lb_ips(lr_od->lb_ips,
> > +                                     lb_group_dps->lb_group->lbs[j]);
> > +            }
> > +        }
> > +
> > +        lr_od->lb_group_uuids[i] = *lb_group_uuid;
> > +
> > +        struct ovn_lb_datapaths *lb_dps;
> > +        for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs; j++) {
> > +            const struct uuid *lb_uuid =
> > +                &lb_group_dps->lb_group->lbs[j]->nlb->header_.uuid;
> > +            lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> > +            ovs_assert(lb_dps);
> > +            ovn_lb_datapaths_add_lr(lb_dps, 1, &lr_od);
> > +        }
> > +    }
> > +}
> > +
> >  static void
> >  build_lb_datapaths(const struct hmap *lbs, const struct hmap *lb_groups,
> >                     struct ovn_datapaths *ls_datapaths,
> > @@ -4028,7 +4112,6 @@ build_lb_datapaths(const struct hmap *lbs, const
> struct hmap *lb_groups,
> >                     struct hmap *lb_datapaths_map,
> >                     struct hmap *lb_group_datapaths_map)
> >  {
> > -    const struct nbrec_load_balancer_group *nbrec_lb_group;
> >      struct ovn_lb_group_datapaths *lb_group_dps;
> >      const struct ovn_lb_group *lb_group;
> >      struct ovn_lb_datapaths *lb_dps;
> > @@ -4062,53 +4145,8 @@ build_lb_datapaths(const struct hmap *lbs, const
> struct hmap *lb_groups,
> >
> >      HMAP_FOR_EACH (od, key_node, &lr_datapaths->datapaths) {
> >          ovs_assert(od->nbr);
> > -
> > -        /* Checking load balancer groups first, starting from the
> largest one,
> > -         * to more efficiently copy IP sets. */
> > -        size_t largest_group = 0;
> > -
> > -        for (size_t i = 1; i < od->nbr->n_load_balancer_group; i++) {
> > -            if (od->nbr->load_balancer_group[i]->n_load_balancer >
> > -
>  od->nbr->load_balancer_group[largest_group]->n_load_balancer) {
> > -                largest_group = i;
> > -            }
> > -        }
> > -
> > -        for (size_t i = 0; i < od->nbr->n_load_balancer_group; i++) {
> > -            size_t idx = (i + largest_group) %
> od->nbr->n_load_balancer_group;
> > -
> > -            nbrec_lb_group = od->nbr->load_balancer_group[idx];
> > -            const struct uuid *lb_group_uuid =
> &nbrec_lb_group->header_.uuid;
> > -
> > -            lb_group_dps =
> > -                ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > -                                            lb_group_uuid);
> > -            ovs_assert(lb_group_dps);
> > -            ovn_lb_group_datapaths_add_lr(lb_group_dps, od);
> > -
> > -            if (!od->lb_ips) {
> > -                od->lb_ips =
> > -                    ovn_lb_ip_set_clone(lb_group_dps->lb_group->lb_ips);
> > -            } else {
> > -                for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs;
> j++) {
> > -                    build_lrouter_lb_ips(od->lb_ips,
> > -                                         lb_group_dps->lb_group->lbs[j]);
> > -                }
> > -            }
> > -        }
> > -
> > -        if (!od->lb_ips) {
> > -            od->lb_ips = ovn_lb_ip_set_create();
> > -        }
> > -
> > -        for (size_t i = 0; i < od->nbr->n_load_balancer; i++) {
> > -            const struct uuid *lb_uuid =
> > -                &od->nbr->load_balancer[i]->header_.uuid;
> > -            lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> > -            ovs_assert(lb_dps);
> > -            ovn_lb_datapaths_add_lr(lb_dps, 1, &od);
> > -            build_lrouter_lb_ips(od->lb_ips, lb_dps->lb);
> > -        }
> > +        associate_lr_lb_groups(od, lb_group_datapaths_map,
> lb_datapaths_map);
> > +        associate_lr_lbs(od, lb_datapaths_map);
> >      }
> >
> >      HMAP_FOR_EACH (lb_group_dps, hmap_node, lb_group_datapaths_map) {
> > @@ -4978,6 +5016,7 @@ destroy_northd_data_tracked_changes(struct
> northd_data *nd)
> >      }
> >
> >      nd->change_tracked = false;
> > +    nd->lrouters_changed = false;
> >  }
> >
> >  /* Check if a changed LSP can be handled incrementally within the I-P
> engine
> > @@ -5513,6 +5552,263 @@ fail:
> >      return false;
> >  }
> >
> > +/* Returns true if the logical router has changes which are not
> > + * incrementally handled.
> > + * Presently supports i-p for the below changes:
> > + *    - load balancers and load balancer groups.
> > + */
> > +static bool
> > +check_unsupported_inc_proc_for_lr_changes(
> > +    const struct nbrec_logical_router *lr)
> > +{
> > +    /* Check if the columns are changed in this row. */
> > +    enum nbrec_logical_router_column_id col;
> > +    for (col = 0; col < NBREC_LOGICAL_ROUTER_N_COLUMNS; col++) {
> > +        if (nbrec_logical_router_is_updated(lr, col)) {
> > +            if (col == NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER ||
> > +                col == NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER_GROUP) {
> > +                continue;
> > +            }
> > +            return true;
> > +        }
> > +    }
> > +
> > +    /* Check if the referenced rows are changed.
> > +       XXX: Need a better OVSDB IDL interface for this check. */
> > +    for (size_t i = 0; i < lr->n_ports; i++) {
> > +        if (nbrec_logical_router_port_row_get_seqno(lr->ports[i],
> > +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > +            return true;
> > +        }
> > +    }
> > +    if (lr->copp && nbrec_copp_row_get_seqno(lr->copp,
> > +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > +        return true;
> > +    }
> > +    for (size_t i = 0; i < lr->n_nat; i++) {
> > +        if (nbrec_nat_row_get_seqno(lr->nat[i],
> > +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > +            return true;
> > +        }
> > +    }
> > +    for (size_t i = 0; i < lr->n_policies; i++) {
> > +        if (nbrec_logical_router_policy_row_get_seqno(lr->policies[i],
> > +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > +            return true;
> > +        }
> > +    }
> > +    for (size_t i = 0; i < lr->n_static_routes; i++) {
> > +        if (nbrec_logical_router_static_route_row_get_seqno(
> > +            lr->static_routes[i], OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > +            return true;
> > +        }
> > +    }
> > +    return false;
> > +}
> > +
> > +static bool
> > +lr_check_and_handle_lb_and_lbgrp_changes(
> > +    const struct nbrec_logical_router *changed_lr,
> > +    struct hmap *lb_datapaths_map, struct hmap *lb_group_datapaths_map,
> > +    struct ovn_datapath *od, bool *updated)
> > +{
> > +    ovs_assert(od->nbr);
> > +    bool lbs_changed = true;
> > +    if (!nbrec_logical_router_is_updated(changed_lr,
> > +                        NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER) &&
> > +        !nbrec_logical_router_is_updated(changed_lr,
> > +                        NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER_GROUP)) {
> > +        lbs_changed = false;
> > +        for (size_t i = 0; i < changed_lr->n_load_balancer_group; i++) {
> > +            if (nbrec_load_balancer_group_row_get_seqno(
> > +                    changed_lr->load_balancer_group[i],
> > +                    OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > +                lbs_changed = true;
> > +                break;
> > +            }
> > +        }
> > +
> > +        if (!lbs_changed) {
> > +            for (size_t i = 0; i < changed_lr->n_load_balancer; i++) {
> > +                if (nbrec_load_balancer_row_get_seqno(
> > +                        changed_lr->load_balancer[i],
> > +                        OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > +                    lbs_changed = true;
> > +                    break;
> > +                }
> > +            }
> > +        }
> > +
> > +        if (!lbs_changed) {
> > +            return true;
> > +        }
> > +    }
> > +
> > +    /* We need to do 2 things to handle the router for load balancer or
> > +     * load balancer groups.
> > +     *
> > +     * 1. Disassociate the logical router datapath from the already
> associated
> > +     *    load balancers/load balancer groups.
> > +     * 2. Associate the logical router datapath with the updated
> > +     *    load balancers/groups.
> > +     * */
> > +    struct ovn_lb_datapaths *lb_dps;
> > +    size_t i, j;
> > +    for (i = 0; i < od->n_lb_uuids; i++) {
> > +        lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> &od->lb_uuids[i]);
> > +        if (lb_dps) {
> > +            if (lb_dps->lb->routable) {
> > +                /* Can't handler if the load balancer has routable flag
> set.
> > +                 * This requires updating the router ports's routable
> > +                 * addresses. */
> > +                return false;
> > +            }
> > +            ovn_lb_datapaths_remove_lr(lb_dps, 1, &od);
> > +        }
> > +    }
> > +
> > +    struct ovn_lb_group_datapaths *lbg_dps;
> > +    for (i = 0; i < od->n_lb_group_uuids; i++) {
> > +        lbg_dps = ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > +                                              &od->lb_group_uuids[i]);
> > +        if (lbg_dps) {
> > +            ovn_lb_group_datapaths_remove_lr(lbg_dps, od);
> > +
> > +            if (install_ls_lb_from_router && od->n_ls_peers) {
> > +                ovn_lb_group_datapaths_remove_ls(lbg_dps, od->n_ls_peers,
> > +                                                 od->ls_peers);
> > +            }
> > +        }
> > +
> > +        for (j = 0; j < lbg_dps->lb_group->n_lbs; j++) {
> > +            const struct uuid *lb_uuid =
> > +                &lbg_dps->lb_group->lbs[j]->nlb->header_.uuid;
> > +            lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> > +            if (lb_dps) {
> > +                if (lb_dps->lb->routable) {
> > +                    /* Can't handler if the load balancer has routable
> flag
> > +                     * set.  This requires updating the router ports's
> > +                     * routable addresses. */
> > +                    return false;
> > +                }
> > +                ovn_lb_datapaths_remove_lr(lb_dps, 1, &od);
> > +
> > +                if (install_ls_lb_from_router && od->n_ls_peers) {
> > +                    ovn_lb_datapaths_remove_ls(lb_dps, od->n_ls_peers,
> > +                                               od->ls_peers);
> > +                }
> > +            }
> > +        }
> > +    }
> > +
> > +    ovn_lb_ip_set_destroy(od->lb_ips);
> > +    od->lb_ips = NULL;
> > +    associate_lr_lb_groups(od, lb_group_datapaths_map, lb_datapaths_map);
> > +    associate_lr_lbs(od, lb_datapaths_map);
> > +
> > +    /* Do the job of build_lrouter_lbs_reachable_ips and
> > +     * build_lswitch_lbs_from_lrouter for this lrouter datapath. */
> > +    for (i = 0; i < od->nbr->n_load_balancer; i++) {
> > +        lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> > +
>  &od->nbr->load_balancer[i]->header_.uuid);
> > +        ovs_assert(lb_dps);
> > +        if (lb_dps->lb->routable) {
> > +            /* Can't handler if the load balancer has routable flag set.
> > +             * This requires updating the router ports's routable
> addresses. */
> > +            return false;
> > +        }
> > +        build_lrouter_lb_reachable_ips(od, lb_dps->lb);
> > +
> > +        if (install_ls_lb_from_router && od->n_ls_peers) {
> > +            ovn_lb_datapaths_add_ls(lb_dps, od->n_ls_peers,
> od->ls_peers);
> > +        }
> > +    }
> > +
> > +    for (i = 0; i < od->nbr->n_load_balancer_group; i++) {
> > +        const struct nbrec_load_balancer_group *nbrec_lb_group =
> > +            od->nbr->load_balancer_group[i];
> > +        lbg_dps = ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > +
>  &nbrec_lb_group->header_.uuid);
> > +        ovs_assert(lbg_dps);
> > +        if (install_ls_lb_from_router && od->n_ls_peers) {
> > +            ovn_lb_group_datapaths_add_ls(lbg_dps, od->n_ls_peers,
> > +                                          od->ls_peers);
> > +        }
> > +
> > +        for (j = 0; j < lbg_dps->lb_group->n_lbs; j++) {
> > +            build_lrouter_lb_reachable_ips(od,
> lbg_dps->lb_group->lbs[j]);
> > +
> > +            if (install_ls_lb_from_router && od->n_ls_peers) {
> > +                const struct uuid *lb_uuid =
> > +                    &lbg_dps->lb_group->lbs[j]->nlb->header_.uuid;
> > +                lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> lb_uuid);
> > +                ovs_assert(lb_dps);
> > +                if (lb_dps->lb->routable) {
> > +                    /* Can't handler if the load balancer has routable
> flag
> > +                     * set.  This requires updating the router ports's
> > +                     * routable addresses. */
> > +                    return false;
> > +                }
> > +                ovn_lb_datapaths_add_ls(lb_dps, od->n_ls_peers,
> od->ls_peers);
> > +            }
> > +        }
> > +    }
> > +
> > +    *updated = true;
> > +    /* Re-evaluate 'od->has_lb_vip' */
> > +    init_lb_for_datapath(od);
> > +    return true;
> > +}
> > +
> > +
> > +/* Return true if changes are handled incrementally, false otherwise.
> > + * When there are any changes, try to track what's exactly changed and
> set
> > + * northd_data->change_tracked accordingly: change tracked - true,
> otherwise,
> > + * false. */
> > +bool
> > +northd_handle_lr_changes(const struct northd_input *ni,
> > +                         struct northd_data *nd)
> > +{
> > +    const struct nbrec_logical_router *changed_lr;
> > +
> > +    bool updated = false;
> > +
> > +    NBREC_LOGICAL_ROUTER_TABLE_FOR_EACH_TRACKED (changed_lr,
> > +
> ni->nbrec_logical_router_table) {
> > +        if (nbrec_logical_router_is_new(changed_lr) ||
> > +            nbrec_logical_router_is_deleted(changed_lr)) {
> > +            goto fail;
> > +        }
> > +
> > +        struct ovn_datapath *od = ovn_datapath_find(
> > +                                    &nd->lr_datapaths.datapaths,
> > +                                    &changed_lr->header_.uuid);
> > +
> > +        /* Presently only able to handle load balancer and
> > +         * load balancer group changes. */
> > +        if (check_unsupported_inc_proc_for_lr_changes(changed_lr)) {
> > +            goto fail;
> > +        }
> > +
> > +        if (!lr_check_and_handle_lb_and_lbgrp_changes(changed_lr,
> > +                                                &nd->lb_datapaths_map,
> > +
>  &nd->lb_group_datapaths_map,
> > +                                                od, &updated)) {
> > +            goto fail;
> > +        }
> > +    }
> > +
> > +    if (updated) {
> > +        nd->change_tracked = true;
>
> Updated doesn't mean the change is tracked. Please see the earlier comments.
>
> > +        nd->lrouters_changed = true;
> > +    }
> > +
> > +    return true;
> > +fail:
> > +    destroy_northd_data_tracked_changes(nd);
>
> I think we don't need "fail:" here. Instead, just return false and call the
> destroy_northd_data_tracked_changes if lrouter is updated (but not tracked)
> or can't be handled.
>
> Thanks,
> Han
>
> > +    return false;
> > +}
> > +
> >  bool
> >  northd_handle_sb_port_binding_changes(
> >      const struct sbrec_port_binding_table *sbrec_port_binding_table,
> > diff --git a/northd/northd.h b/northd/northd.h
> > index a79cc470c3..765720849a 100644
> > --- a/northd/northd.h
> > +++ b/northd/northd.h
> > @@ -119,8 +119,11 @@ struct northd_data {
> >      struct chassis_features features;
> >      struct sset svc_monitor_lsps;
> >      struct hmap svc_monitor_map;
> > +
> > +    /* Tracked data. */
> >      bool change_tracked;
> >      struct tracked_ls_changes tracked_ls_changes;
> > +    bool lrouters_changed;
> >  };
> >
> >  struct lflow_data {
> > @@ -342,6 +345,8 @@ void ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
> >  bool northd_handle_ls_changes(struct ovsdb_idl_txn *,
> >                                const struct northd_input *,
> >                                struct northd_data *);
> > +bool northd_handle_lr_changes(const struct northd_input *,
> > +                              struct northd_data *);
> >  void destroy_northd_data_tracked_changes(struct northd_data *);
> >  void northd_destroy(struct northd_data *data);
> >  void northd_init(struct northd_data *data);
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index d3a5851e35..286b311242 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -9760,6 +9760,14 @@ 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 norecompute recompute
> >
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl lr-lb-add lr0 lb1
> > +check_engine_stats norecompute recompute
> > +
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl lr-lb-del lr0 lb1
> > +check_engine_stats norecompute 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)
> > @@ -9796,11 +9804,11 @@ check_engine_stats norecompute 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_engine_stats norecompute 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
> > +check_engine_stats norecompute recompute
> >
> >  AT_CLEANUP
> >  ])
> > --
> > 2.40.1
> >
> > _______________________________________________
> > 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 Wed, Jul 19, 2023 at 2:01 AM Numan Siddique <numans@ovn.org> wrote:
>
> On Tue, Jul 18, 2023 at 11:08 AM Han Zhou <hzhou@ovn.org> wrote:
> >
> > On Fri, Jul 7, 2023 at 1:56 PM <numans@ovn.org> wrote:
> > >
> > > From: Numan Siddique <numans@ovn.org>
> > >
> > > When a logical router gets updated due to load balancer
> > > or load balancer groups changes, it is now incrementally
> > > handled in the 'northd' engine node.  Other logical router
> > > updates result in the full recompute of 'northd' engine node.
> > >
> > > lflow engine node still falls back to recompute due to
> > > logical router changes.
> > >
> > > Signed-off-by: Numan Siddique <numans@ovn.org>
> > > ---
> > >  lib/lb.c                 |  32 +++-
> > >  lib/lb.h                 |   5 +
> > >  northd/en-lflow.c        |   5 +
> > >  northd/en-northd.c       |  20 ++
> > >  northd/en-northd.h       |   1 +
> > >  northd/inc-proc-northd.c |   3 +-
> > >  northd/northd.c          | 392
++++++++++++++++++++++++++++++++++-----
> > >  northd/northd.h          |   5 +
> > >  tests/ovn-northd.at      |  12 +-
> > >  9 files changed, 422 insertions(+), 53 deletions(-)
> > >
> > > diff --git a/lib/lb.c b/lib/lb.c
> > > index a0426cc42e..a8b694d0b3 100644
> > > --- a/lib/lb.c
> > > +++ b/lib/lb.c
> > > @@ -1082,7 +1082,10 @@ ovn_lb_datapaths_add_lr(struct ovn_lb_datapaths
> > *lb_dps, size_t n,
> > >                          struct ovn_datapath **ods)
> > >  {
> > >      for (size_t i = 0; i < n; i++) {
> > > -        bitmap_set1(lb_dps->nb_lr_map, ods[i]->index);
> > > +        if (!bitmap_is_set(lb_dps->nb_lr_map, ods[i]->index)) {
> > > +            bitmap_set1(lb_dps->nb_lr_map, ods[i]->index);
> > > +            lb_dps->n_nb_lr++;
> > > +        }
> > >      }
> > >  }
> > >
> > > @@ -1110,6 +1113,18 @@ ovn_lb_datapaths_remove_ls(struct
ovn_lb_datapaths
> > *lb_dps, size_t n,
> > >      }
> > >  }
> > >
> > > +void
> > > +ovn_lb_datapaths_remove_lr(struct ovn_lb_datapaths *lb_dps, size_t n,
> > > +                           struct ovn_datapath **ods)
> > > +{
> > > +    for (size_t i = 0; i < n; i++) {
> > > +        if (bitmap_is_set(lb_dps->nb_lr_map, ods[i]->index)) {
> > > +            bitmap_set0(lb_dps->nb_lr_map, ods[i]->index);
> > > +            lb_dps->n_nb_lr--;
> > > +        }
> > > +    }
> > > +}
> > > +
> > >  struct ovn_lb_group_datapaths *
> > >  ovn_lb_group_datapaths_create(const struct ovn_lb_group *lb_group,
> > >                                size_t n_ls_datapaths,
> > > @@ -1175,5 +1190,18 @@ void
> > >  ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *lbg_dps,
> > >                                struct ovn_datapath *lr)
> > >  {
> > > -    bitmap_set1(lbg_dps->nb_lr_map, lr->index);
> > > +    if (!bitmap_is_set(lbg_dps->nb_lr_map, lr->index)) {
> > > +        bitmap_set1(lbg_dps->nb_lr_map, lr->index);
> > > +        lbg_dps->n_nb_lr++;
> > > +    }
> > > +}
> > > +
> > > +void
> > > +ovn_lb_group_datapaths_remove_lr(struct ovn_lb_group_datapaths
*lbg_dps,
> > > +                                 struct ovn_datapath *lr)
> > > +{
> > > +    if (bitmap_is_set(lbg_dps->nb_lr_map, lr->index)) {
> > > +        bitmap_set1(lbg_dps->nb_lr_map, lr->index);
> > > +        lbg_dps->n_nb_lr--;
> > > +    }
> > >  }
> > > diff --git a/lib/lb.h b/lib/lb.h
> > > index 08723e8ef3..91eec0199b 100644
> > > --- a/lib/lb.h
> > > +++ b/lib/lb.h
> > > @@ -185,6 +185,8 @@ void ovn_lb_datapaths_set_ls(struct
ovn_lb_datapaths
> > *, size_t n,
> > >
> > >  void ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *, size_t n,
> > >                                  struct ovn_datapath **);
> > > +void ovn_lb_datapaths_remove_lr(struct ovn_lb_datapaths *, size_t n,
> > > +                                struct ovn_datapath **);
> > >
> > >  struct ovn_lb_group_datapaths {
> > >      struct hmap_node hmap_node;
> > > @@ -214,6 +216,9 @@ void ovn_lb_group_datapaths_remove_ls(struct
> > ovn_lb_group_datapaths *,
> > >
> > >  void ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *,
> > >                                     struct ovn_datapath *lr);
> > > +void ovn_lb_group_datapaths_remove_lr(struct ovn_lb_group_datapaths
*,
> > > +                                      struct ovn_datapath *lr);
> > > +
> > >  struct ovn_controller_lb {
> > >      struct hmap_node hmap_node;
> > >
> > > diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> > > index db1bcbccd6..ea51f4e3e0 100644
> > > --- a/northd/en-lflow.c
> > > +++ b/northd/en-lflow.c
> > > @@ -102,6 +102,11 @@ lflow_northd_handler(struct engine_node *node,
> > >      if (!northd_data->change_tracked) {
> > >          return false;
> > >      }
> > > +
> > > +    if (northd_data->lrouters_changed) {
> > > +        return false;
> > > +    }
> >
> > I think lrouters_changed is not needed. If lrouters related data is
> > changed, we should set change_tracked to false (call the
> > destroy_northd_data_tracked_changes), because lrouter data is part of
> > northd_data nad we don't track lrouter changes yet.
>
>
>    -  With this patch we could are  handling router changes
> incrementally in northd engine node if only the router's load_balancer
> column has changed.
>       If we set change_tracked to false, it means northd engine node
> didn't handle the router changes due to load balancer column
>       incrementally and it fell back to full recompute which is not

"change_tracked", as the name suggests, only indicates if the changes are
tracked, and the purpose is for the dependent node to decide how to handle
the changes (usually if the changes are not tracked the dependent node
couldn't handle the changes incrementally). I think "change_tracked"
doesn't necessarily tell if the current node is recomputed or not.

> the cause.  This 'lrounters_changed'  indicates to the lflow engine
> node
>       that northd engine node did a compute (and not a recompute) and
> logical routers have changed as part of this compute.
>

Why do we need to maintain the status of whether the node is recomputed in
such a variable?

>    -  Since lflow engine node doesn't yet handle the lrouter changes,
> it falls back to full recompute.

I understand, but northd_data->change_tracked == false should just trigger
the lflow node recompute, because the input changed but the changes are not
tracked and obviously we don't know how to handle that in the lflow node.

>
>    - It is also possible that in the same nb db update,  a load
> balancer was updated which is associated with both logical switch and
> logical router.
>      In such cases we don't want to call -
> destroy_northd_data_tracked_changes().  Even though lflow engine does

Why don't we want to call destroy_northd_data_tracked_changes()? If there
are changes already handled incrementally and tracked, and now we know we
can't handle the LR/LS related changes (due to LB changes associated with
LR/LS), it is ok to release the "partially" tracked changes, right?

> a recompute, from northd engine point of view,
>      it is indicating what changed.  It is left to the dependent nodes
> to decide if they want to handle incrementally or fall back to
> recompute.
>
Yes, dependent nodes will decide, but I think change_tracked (and tracked
changes) is sufficient for them to make the decision.

(Sorry for my late response because I was just back from vacation)

Thanks,
Han

> Let me know if you disagree with this.
>
>
>  >
> > > +
> > >      const struct engine_context *eng_ctx = engine_get_context();
> > >      struct lflow_data *lflow_data = data;
> > >
> > > diff --git a/northd/en-northd.c b/northd/en-northd.c
> > > index b3c03c54bd..a321eff323 100644
> > > --- a/northd/en-northd.c
> > > +++ b/northd/en-northd.c
> > > @@ -188,6 +188,26 @@ northd_nb_logical_switch_handler(struct
engine_node
> > *node,
> > >      return true;
> > >  }
> > >
> > > +bool
> > > +northd_nb_logical_router_handler(struct engine_node *node,
> > > +                                 void *data)
> > > +{
> > > +    struct northd_data *nd = data;
> > > +    struct northd_input input_data;
> > > +
> > > +    northd_get_input_data(node, &input_data);
> > > +
> > > +    if (!northd_handle_lr_changes(&input_data, nd)) {
> > > +        return false;
> > > +    }
> > > +
> > > +    if (nd->change_tracked) {
> > > +        engine_set_node_state(node, EN_UPDATED);
> >
> > change_tracked is supposed to indicate if the changes in the engine
node is
> > tracked, meaning the node depending on this one can potentailly
> > incrementally process the changes.
> > It is possible that the node is updated but the changes are not
tracked. In
> > this case most likely the nodes depending on it will need to be
recomputed.
> > Whether the node state is EN_UPDATED depends on if the node is updated.
I
> > think the northd_handle_lr_changes can take a parameter "updated" to
> > indicate if it is updated.
>
> Agree.  Will use "updated" parameter instead.
>
> Thanks
> Numan
>
>
>
> >
> > > +    }
> > > +
> > > +    return true;
> > > +}
> > > +
> > >  bool
> > >  northd_sb_port_binding_handler(struct engine_node *node,
> > >                                 void *data)
> > > diff --git a/northd/en-northd.h b/northd/en-northd.h
> > > index 5674f4390c..739aa5e87f 100644
> > > --- a/northd/en-northd.h
> > > +++ b/northd/en-northd.h
> > > @@ -16,6 +16,7 @@ void en_northd_cleanup(void *data);
> > >  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_nb_logical_router_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);
> > >
> > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> > > index f5b7998f1f..362dd5e87a 100644
> > > --- a/northd/inc-proc-northd.c
> > > +++ b/northd/inc-proc-northd.c
> > > @@ -156,7 +156,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop
*nb,
> > >
> > >      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);
> > >      engine_add_input(&en_northd, &en_nb_mirror, NULL);
> > >      engine_add_input(&en_northd, &en_nb_meter, NULL);
> > >      engine_add_input(&en_northd, &en_nb_static_mac_binding, NULL);
> > > @@ -185,6 +184,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop
*nb,
> > >                       northd_nb_nb_global_handler);
> > >      engine_add_input(&en_northd, &en_nb_logical_switch,
> > >                       northd_nb_logical_switch_handler);
> > > +    engine_add_input(&en_northd, &en_nb_logical_router,
> > > +                     northd_nb_logical_router_handler);
> > >
> > >      engine_add_input(&en_mac_binding_aging, &en_nb_nb_global, NULL);
> > >      engine_add_input(&en_mac_binding_aging, &en_sb_mac_binding,
NULL);
> > > diff --git a/northd/northd.c b/northd/northd.c
> > > index a7761285de..4393adc2ad 100644
> > > --- a/northd/northd.c
> > > +++ b/northd/northd.c
> > > @@ -4021,6 +4021,90 @@ associate_ls_lb_groups(struct ovn_datapath
*ls_od,
> > >      }
> > >  }
> > >
> > > +static void
> > > +associate_lr_lbs(struct ovn_datapath *lr_od, struct hmap
> > *lb_datapaths_map)
> > > +{
> > > +    struct ovn_lb_datapaths *lb_dps;
> > > +
> > > +    free(lr_od->lb_uuids);
> > > +    lr_od->lb_uuids = xcalloc(lr_od->nbr->n_load_balancer,
> > > +                              sizeof *lr_od->lb_uuids);
> > > +    lr_od->n_lb_uuids = lr_od->nbr->n_load_balancer;
> > > +
> > > +    if (!lr_od->lb_ips) {
> > > +        lr_od->lb_ips = ovn_lb_ip_set_create();
> > > +    }
> > > +
> > > +    for (size_t i = 0; i < lr_od->nbr->n_load_balancer; i++) {
> > > +        const struct uuid *lb_uuid =
> > > +            &lr_od->nbr->load_balancer[i]->header_.uuid;
> > > +        lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> > > +        ovs_assert(lb_dps);
> > > +        ovn_lb_datapaths_add_lr(lb_dps, 1, &lr_od);
> > > +        build_lrouter_lb_ips(lr_od->lb_ips, lb_dps->lb);
> > > +        lr_od->lb_uuids[i] = *lb_uuid;
> > > +    }
> > > +}
> > > +
> > > +static void
> > > +associate_lr_lb_groups(struct ovn_datapath *lr_od,
> > > +                       struct hmap *lb_group_datapaths_map,
> > > +                       struct hmap *lb_datapaths_map)
> > > +{
> > > +    const struct nbrec_load_balancer_group *nbrec_lb_group;
> > > +    struct ovn_lb_group_datapaths *lb_group_dps;
> > > +
> > > +    free(lr_od->lb_group_uuids);
> > > +    lr_od->lb_group_uuids =
xcalloc(lr_od->nbr->n_load_balancer_group,
> > > +                                    sizeof *lr_od->lb_group_uuids);
> > > +    lr_od->n_lb_group_uuids = lr_od->nbr->n_load_balancer_group;
> > > +
> > > +    /* Checking load balancer groups first, starting from the largest
> > one,
> > > +     * to more efficiently copy IP sets. */
> > > +    size_t largest_group = 0;
> > > +
> > > +    for (size_t i = 1; i < lr_od->nbr->n_load_balancer_group; i++) {
> > > +        if (lr_od->nbr->load_balancer_group[i]->n_load_balancer >
> > > +
> >  lr_od->nbr->load_balancer_group[largest_group]->n_load_balancer) {
> > > +            largest_group = i;
> > > +        }
> > > +    }
> > > +
> > > +    for (size_t i = 0; i < lr_od->nbr->n_load_balancer_group; i++) {
> > > +        size_t idx = (i + largest_group) %
> > lr_od->nbr->n_load_balancer_group;
> > > +
> > > +        nbrec_lb_group = lr_od->nbr->load_balancer_group[idx];
> > > +        const struct uuid *lb_group_uuid =
&nbrec_lb_group->header_.uuid;
> > > +
> > > +        lb_group_dps =
> > > +            ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > > +                                        lb_group_uuid);
> > > +        ovs_assert(lb_group_dps);
> > > +        ovn_lb_group_datapaths_add_lr(lb_group_dps, lr_od);
> > > +
> > > +        if (!lr_od->lb_ips) {
> > > +            lr_od->lb_ips =
> > > +                ovn_lb_ip_set_clone(lb_group_dps->lb_group->lb_ips);
> > > +        } else {
> > > +            for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs;
j++) {
> > > +                build_lrouter_lb_ips(lr_od->lb_ips,
> > > +                                     lb_group_dps->lb_group->lbs[j]);
> > > +            }
> > > +        }
> > > +
> > > +        lr_od->lb_group_uuids[i] = *lb_group_uuid;
> > > +
> > > +        struct ovn_lb_datapaths *lb_dps;
> > > +        for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs; j++) {
> > > +            const struct uuid *lb_uuid =
> > > +                &lb_group_dps->lb_group->lbs[j]->nlb->header_.uuid;
> > > +            lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
lb_uuid);
> > > +            ovs_assert(lb_dps);
> > > +            ovn_lb_datapaths_add_lr(lb_dps, 1, &lr_od);
> > > +        }
> > > +    }
> > > +}
> > > +
> > >  static void
> > >  build_lb_datapaths(const struct hmap *lbs, const struct hmap
*lb_groups,
> > >                     struct ovn_datapaths *ls_datapaths,
> > > @@ -4028,7 +4112,6 @@ build_lb_datapaths(const struct hmap *lbs, const
> > struct hmap *lb_groups,
> > >                     struct hmap *lb_datapaths_map,
> > >                     struct hmap *lb_group_datapaths_map)
> > >  {
> > > -    const struct nbrec_load_balancer_group *nbrec_lb_group;
> > >      struct ovn_lb_group_datapaths *lb_group_dps;
> > >      const struct ovn_lb_group *lb_group;
> > >      struct ovn_lb_datapaths *lb_dps;
> > > @@ -4062,53 +4145,8 @@ build_lb_datapaths(const struct hmap *lbs,
const
> > struct hmap *lb_groups,
> > >
> > >      HMAP_FOR_EACH (od, key_node, &lr_datapaths->datapaths) {
> > >          ovs_assert(od->nbr);
> > > -
> > > -        /* Checking load balancer groups first, starting from the
> > largest one,
> > > -         * to more efficiently copy IP sets. */
> > > -        size_t largest_group = 0;
> > > -
> > > -        for (size_t i = 1; i < od->nbr->n_load_balancer_group; i++) {
> > > -            if (od->nbr->load_balancer_group[i]->n_load_balancer >
> > > -
> >  od->nbr->load_balancer_group[largest_group]->n_load_balancer) {
> > > -                largest_group = i;
> > > -            }
> > > -        }
> > > -
> > > -        for (size_t i = 0; i < od->nbr->n_load_balancer_group; i++) {
> > > -            size_t idx = (i + largest_group) %
> > od->nbr->n_load_balancer_group;
> > > -
> > > -            nbrec_lb_group = od->nbr->load_balancer_group[idx];
> > > -            const struct uuid *lb_group_uuid =
> > &nbrec_lb_group->header_.uuid;
> > > -
> > > -            lb_group_dps =
> > > -                ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > > -                                            lb_group_uuid);
> > > -            ovs_assert(lb_group_dps);
> > > -            ovn_lb_group_datapaths_add_lr(lb_group_dps, od);
> > > -
> > > -            if (!od->lb_ips) {
> > > -                od->lb_ips =
> > > -
 ovn_lb_ip_set_clone(lb_group_dps->lb_group->lb_ips);
> > > -            } else {
> > > -                for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs;
> > j++) {
> > > -                    build_lrouter_lb_ips(od->lb_ips,
> > > -
lb_group_dps->lb_group->lbs[j]);
> > > -                }
> > > -            }
> > > -        }
> > > -
> > > -        if (!od->lb_ips) {
> > > -            od->lb_ips = ovn_lb_ip_set_create();
> > > -        }
> > > -
> > > -        for (size_t i = 0; i < od->nbr->n_load_balancer; i++) {
> > > -            const struct uuid *lb_uuid =
> > > -                &od->nbr->load_balancer[i]->header_.uuid;
> > > -            lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
lb_uuid);
> > > -            ovs_assert(lb_dps);
> > > -            ovn_lb_datapaths_add_lr(lb_dps, 1, &od);
> > > -            build_lrouter_lb_ips(od->lb_ips, lb_dps->lb);
> > > -        }
> > > +        associate_lr_lb_groups(od, lb_group_datapaths_map,
> > lb_datapaths_map);
> > > +        associate_lr_lbs(od, lb_datapaths_map);
> > >      }
> > >
> > >      HMAP_FOR_EACH (lb_group_dps, hmap_node, lb_group_datapaths_map) {
> > > @@ -4978,6 +5016,7 @@ destroy_northd_data_tracked_changes(struct
> > northd_data *nd)
> > >      }
> > >
> > >      nd->change_tracked = false;
> > > +    nd->lrouters_changed = false;
> > >  }
> > >
> > >  /* Check if a changed LSP can be handled incrementally within the I-P
> > engine
> > > @@ -5513,6 +5552,263 @@ fail:
> > >      return false;
> > >  }
> > >
> > > +/* Returns true if the logical router has changes which are not
> > > + * incrementally handled.
> > > + * Presently supports i-p for the below changes:
> > > + *    - load balancers and load balancer groups.
> > > + */
> > > +static bool
> > > +check_unsupported_inc_proc_for_lr_changes(
> > > +    const struct nbrec_logical_router *lr)
> > > +{
> > > +    /* Check if the columns are changed in this row. */
> > > +    enum nbrec_logical_router_column_id col;
> > > +    for (col = 0; col < NBREC_LOGICAL_ROUTER_N_COLUMNS; col++) {
> > > +        if (nbrec_logical_router_is_updated(lr, col)) {
> > > +            if (col == NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER ||
> > > +                col == NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER_GROUP)
{
> > > +                continue;
> > > +            }
> > > +            return true;
> > > +        }
> > > +    }
> > > +
> > > +    /* Check if the referenced rows are changed.
> > > +       XXX: Need a better OVSDB IDL interface for this check. */
> > > +    for (size_t i = 0; i < lr->n_ports; i++) {
> > > +        if (nbrec_logical_router_port_row_get_seqno(lr->ports[i],
> > > +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > > +            return true;
> > > +        }
> > > +    }
> > > +    if (lr->copp && nbrec_copp_row_get_seqno(lr->copp,
> > > +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > > +        return true;
> > > +    }
> > > +    for (size_t i = 0; i < lr->n_nat; i++) {
> > > +        if (nbrec_nat_row_get_seqno(lr->nat[i],
> > > +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > > +            return true;
> > > +        }
> > > +    }
> > > +    for (size_t i = 0; i < lr->n_policies; i++) {
> > > +        if
(nbrec_logical_router_policy_row_get_seqno(lr->policies[i],
> > > +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > > +            return true;
> > > +        }
> > > +    }
> > > +    for (size_t i = 0; i < lr->n_static_routes; i++) {
> > > +        if (nbrec_logical_router_static_route_row_get_seqno(
> > > +            lr->static_routes[i], OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > > +            return true;
> > > +        }
> > > +    }
> > > +    return false;
> > > +}
> > > +
> > > +static bool
> > > +lr_check_and_handle_lb_and_lbgrp_changes(
> > > +    const struct nbrec_logical_router *changed_lr,
> > > +    struct hmap *lb_datapaths_map, struct hmap
*lb_group_datapaths_map,
> > > +    struct ovn_datapath *od, bool *updated)
> > > +{
> > > +    ovs_assert(od->nbr);
> > > +    bool lbs_changed = true;
> > > +    if (!nbrec_logical_router_is_updated(changed_lr,
> > > +                        NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER) &&
> > > +        !nbrec_logical_router_is_updated(changed_lr,
> > > +
 NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER_GROUP)) {
> > > +        lbs_changed = false;
> > > +        for (size_t i = 0; i < changed_lr->n_load_balancer_group;
i++) {
> > > +            if (nbrec_load_balancer_group_row_get_seqno(
> > > +                    changed_lr->load_balancer_group[i],
> > > +                    OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > > +                lbs_changed = true;
> > > +                break;
> > > +            }
> > > +        }
> > > +
> > > +        if (!lbs_changed) {
> > > +            for (size_t i = 0; i < changed_lr->n_load_balancer; i++)
{
> > > +                if (nbrec_load_balancer_row_get_seqno(
> > > +                        changed_lr->load_balancer[i],
> > > +                        OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > > +                    lbs_changed = true;
> > > +                    break;
> > > +                }
> > > +            }
> > > +        }
> > > +
> > > +        if (!lbs_changed) {
> > > +            return true;
> > > +        }
> > > +    }
> > > +
> > > +    /* We need to do 2 things to handle the router for load balancer
or
> > > +     * load balancer groups.
> > > +     *
> > > +     * 1. Disassociate the logical router datapath from the already
> > associated
> > > +     *    load balancers/load balancer groups.
> > > +     * 2. Associate the logical router datapath with the updated
> > > +     *    load balancers/groups.
> > > +     * */
> > > +    struct ovn_lb_datapaths *lb_dps;
> > > +    size_t i, j;
> > > +    for (i = 0; i < od->n_lb_uuids; i++) {
> > > +        lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> > &od->lb_uuids[i]);
> > > +        if (lb_dps) {
> > > +            if (lb_dps->lb->routable) {
> > > +                /* Can't handler if the load balancer has routable
flag
> > set.
> > > +                 * This requires updating the router ports's routable
> > > +                 * addresses. */
> > > +                return false;
> > > +            }
> > > +            ovn_lb_datapaths_remove_lr(lb_dps, 1, &od);
> > > +        }
> > > +    }
> > > +
> > > +    struct ovn_lb_group_datapaths *lbg_dps;
> > > +    for (i = 0; i < od->n_lb_group_uuids; i++) {
> > > +        lbg_dps = ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > > +
 &od->lb_group_uuids[i]);
> > > +        if (lbg_dps) {
> > > +            ovn_lb_group_datapaths_remove_lr(lbg_dps, od);
> > > +
> > > +            if (install_ls_lb_from_router && od->n_ls_peers) {
> > > +                ovn_lb_group_datapaths_remove_ls(lbg_dps,
od->n_ls_peers,
> > > +                                                 od->ls_peers);
> > > +            }
> > > +        }
> > > +
> > > +        for (j = 0; j < lbg_dps->lb_group->n_lbs; j++) {
> > > +            const struct uuid *lb_uuid =
> > > +                &lbg_dps->lb_group->lbs[j]->nlb->header_.uuid;
> > > +            lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
lb_uuid);
> > > +            if (lb_dps) {
> > > +                if (lb_dps->lb->routable) {
> > > +                    /* Can't handler if the load balancer has
routable
> > flag
> > > +                     * set.  This requires updating the router
ports's
> > > +                     * routable addresses. */
> > > +                    return false;
> > > +                }
> > > +                ovn_lb_datapaths_remove_lr(lb_dps, 1, &od);
> > > +
> > > +                if (install_ls_lb_from_router && od->n_ls_peers) {
> > > +                    ovn_lb_datapaths_remove_ls(lb_dps,
od->n_ls_peers,
> > > +                                               od->ls_peers);
> > > +                }
> > > +            }
> > > +        }
> > > +    }
> > > +
> > > +    ovn_lb_ip_set_destroy(od->lb_ips);
> > > +    od->lb_ips = NULL;
> > > +    associate_lr_lb_groups(od, lb_group_datapaths_map,
lb_datapaths_map);
> > > +    associate_lr_lbs(od, lb_datapaths_map);
> > > +
> > > +    /* Do the job of build_lrouter_lbs_reachable_ips and
> > > +     * build_lswitch_lbs_from_lrouter for this lrouter datapath. */
> > > +    for (i = 0; i < od->nbr->n_load_balancer; i++) {
> > > +        lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> > > +
> >  &od->nbr->load_balancer[i]->header_.uuid);
> > > +        ovs_assert(lb_dps);
> > > +        if (lb_dps->lb->routable) {
> > > +            /* Can't handler if the load balancer has routable flag
set.
> > > +             * This requires updating the router ports's routable
> > addresses. */
> > > +            return false;
> > > +        }
> > > +        build_lrouter_lb_reachable_ips(od, lb_dps->lb);
> > > +
> > > +        if (install_ls_lb_from_router && od->n_ls_peers) {
> > > +            ovn_lb_datapaths_add_ls(lb_dps, od->n_ls_peers,
> > od->ls_peers);
> > > +        }
> > > +    }
> > > +
> > > +    for (i = 0; i < od->nbr->n_load_balancer_group; i++) {
> > > +        const struct nbrec_load_balancer_group *nbrec_lb_group =
> > > +            od->nbr->load_balancer_group[i];
> > > +        lbg_dps = ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > > +
> >  &nbrec_lb_group->header_.uuid);
> > > +        ovs_assert(lbg_dps);
> > > +        if (install_ls_lb_from_router && od->n_ls_peers) {
> > > +            ovn_lb_group_datapaths_add_ls(lbg_dps, od->n_ls_peers,
> > > +                                          od->ls_peers);
> > > +        }
> > > +
> > > +        for (j = 0; j < lbg_dps->lb_group->n_lbs; j++) {
> > > +            build_lrouter_lb_reachable_ips(od,
> > lbg_dps->lb_group->lbs[j]);
> > > +
> > > +            if (install_ls_lb_from_router && od->n_ls_peers) {
> > > +                const struct uuid *lb_uuid =
> > > +                    &lbg_dps->lb_group->lbs[j]->nlb->header_.uuid;
> > > +                lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> > lb_uuid);
> > > +                ovs_assert(lb_dps);
> > > +                if (lb_dps->lb->routable) {
> > > +                    /* Can't handler if the load balancer has
routable
> > flag
> > > +                     * set.  This requires updating the router
ports's
> > > +                     * routable addresses. */
> > > +                    return false;
> > > +                }
> > > +                ovn_lb_datapaths_add_ls(lb_dps, od->n_ls_peers,
> > od->ls_peers);
> > > +            }
> > > +        }
> > > +    }
> > > +
> > > +    *updated = true;
> > > +    /* Re-evaluate 'od->has_lb_vip' */
> > > +    init_lb_for_datapath(od);
> > > +    return true;
> > > +}
> > > +
> > > +
> > > +/* Return true if changes are handled incrementally, false otherwise.
> > > + * When there are any changes, try to track what's exactly changed
and
> > set
> > > + * northd_data->change_tracked accordingly: change tracked - true,
> > otherwise,
> > > + * false. */
> > > +bool
> > > +northd_handle_lr_changes(const struct northd_input *ni,
> > > +                         struct northd_data *nd)
> > > +{
> > > +    const struct nbrec_logical_router *changed_lr;
> > > +
> > > +    bool updated = false;
> > > +
> > > +    NBREC_LOGICAL_ROUTER_TABLE_FOR_EACH_TRACKED (changed_lr,
> > > +
> > ni->nbrec_logical_router_table) {
> > > +        if (nbrec_logical_router_is_new(changed_lr) ||
> > > +            nbrec_logical_router_is_deleted(changed_lr)) {
> > > +            goto fail;
> > > +        }
> > > +
> > > +        struct ovn_datapath *od = ovn_datapath_find(
> > > +                                    &nd->lr_datapaths.datapaths,
> > > +                                    &changed_lr->header_.uuid);
> > > +
> > > +        /* Presently only able to handle load balancer and
> > > +         * load balancer group changes. */
> > > +        if (check_unsupported_inc_proc_for_lr_changes(changed_lr)) {
> > > +            goto fail;
> > > +        }
> > > +
> > > +        if (!lr_check_and_handle_lb_and_lbgrp_changes(changed_lr,
> > > +
 &nd->lb_datapaths_map,
> > > +
> >  &nd->lb_group_datapaths_map,
> > > +                                                od, &updated)) {
> > > +            goto fail;
> > > +        }
> > > +    }
> > > +
> > > +    if (updated) {
> > > +        nd->change_tracked = true;
> >
> > Updated doesn't mean the change is tracked. Please see the earlier
comments.
> >
> > > +        nd->lrouters_changed = true;
> > > +    }
> > > +
> > > +    return true;
> > > +fail:
> > > +    destroy_northd_data_tracked_changes(nd);
> >
> > I think we don't need "fail:" here. Instead, just return false and call
the
> > destroy_northd_data_tracked_changes if lrouter is updated (but not
tracked)
> > or can't be handled.
> >
> > Thanks,
> > Han
> >
> > > +    return false;
> > > +}
> > > +
> > >  bool
> > >  northd_handle_sb_port_binding_changes(
> > >      const struct sbrec_port_binding_table *sbrec_port_binding_table,
> > > diff --git a/northd/northd.h b/northd/northd.h
> > > index a79cc470c3..765720849a 100644
> > > --- a/northd/northd.h
> > > +++ b/northd/northd.h
> > > @@ -119,8 +119,11 @@ struct northd_data {
> > >      struct chassis_features features;
> > >      struct sset svc_monitor_lsps;
> > >      struct hmap svc_monitor_map;
> > > +
> > > +    /* Tracked data. */
> > >      bool change_tracked;
> > >      struct tracked_ls_changes tracked_ls_changes;
> > > +    bool lrouters_changed;
> > >  };
> > >
> > >  struct lflow_data {
> > > @@ -342,6 +345,8 @@ void ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
> > >  bool northd_handle_ls_changes(struct ovsdb_idl_txn *,
> > >                                const struct northd_input *,
> > >                                struct northd_data *);
> > > +bool northd_handle_lr_changes(const struct northd_input *,
> > > +                              struct northd_data *);
> > >  void destroy_northd_data_tracked_changes(struct northd_data *);
> > >  void northd_destroy(struct northd_data *data);
> > >  void northd_init(struct northd_data *data);
> > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > > index d3a5851e35..286b311242 100644
> > > --- a/tests/ovn-northd.at
> > > +++ b/tests/ovn-northd.at
> > > @@ -9760,6 +9760,14 @@ 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 norecompute recompute
> > >
> > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > +check ovn-nbctl lr-lb-add lr0 lb1
> > > +check_engine_stats norecompute recompute
> > > +
> > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > +check ovn-nbctl lr-lb-del lr0 lb1
> > > +check_engine_stats norecompute 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)
> > > @@ -9796,11 +9804,11 @@ check_engine_stats norecompute 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_engine_stats norecompute 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
> > > +check_engine_stats norecompute recompute
> > >
> > >  AT_CLEANUP
> > >  ])
> > > --
> > > 2.40.1
> > >
> > > _______________________________________________
> > > 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 Mon, Jul 31, 2023 at 8:18 PM Han Zhou <hzhou@ovn.org> wrote:
>
> On Wed, Jul 19, 2023 at 2:01 AM Numan Siddique <numans@ovn.org> wrote:
> >
> > On Tue, Jul 18, 2023 at 11:08 AM Han Zhou <hzhou@ovn.org> wrote:
> > >
> > > On Fri, Jul 7, 2023 at 1:56 PM <numans@ovn.org> wrote:
> > > >
> > > > From: Numan Siddique <numans@ovn.org>
> > > >
> > > > When a logical router gets updated due to load balancer
> > > > or load balancer groups changes, it is now incrementally
> > > > handled in the 'northd' engine node.  Other logical router
> > > > updates result in the full recompute of 'northd' engine node.
> > > >
> > > > lflow engine node still falls back to recompute due to
> > > > logical router changes.
> > > >
> > > > Signed-off-by: Numan Siddique <numans@ovn.org>
> > > > ---
> > > >  lib/lb.c                 |  32 +++-
> > > >  lib/lb.h                 |   5 +
> > > >  northd/en-lflow.c        |   5 +
> > > >  northd/en-northd.c       |  20 ++
> > > >  northd/en-northd.h       |   1 +
> > > >  northd/inc-proc-northd.c |   3 +-
> > > >  northd/northd.c          | 392
> ++++++++++++++++++++++++++++++++++-----
> > > >  northd/northd.h          |   5 +
> > > >  tests/ovn-northd.at      |  12 +-
> > > >  9 files changed, 422 insertions(+), 53 deletions(-)
> > > >
> > > > diff --git a/lib/lb.c b/lib/lb.c
> > > > index a0426cc42e..a8b694d0b3 100644
> > > > --- a/lib/lb.c
> > > > +++ b/lib/lb.c
> > > > @@ -1082,7 +1082,10 @@ ovn_lb_datapaths_add_lr(struct ovn_lb_datapaths
> > > *lb_dps, size_t n,
> > > >                          struct ovn_datapath **ods)
> > > >  {
> > > >      for (size_t i = 0; i < n; i++) {
> > > > -        bitmap_set1(lb_dps->nb_lr_map, ods[i]->index);
> > > > +        if (!bitmap_is_set(lb_dps->nb_lr_map, ods[i]->index)) {
> > > > +            bitmap_set1(lb_dps->nb_lr_map, ods[i]->index);
> > > > +            lb_dps->n_nb_lr++;
> > > > +        }
> > > >      }
> > > >  }
> > > >
> > > > @@ -1110,6 +1113,18 @@ ovn_lb_datapaths_remove_ls(struct
> ovn_lb_datapaths
> > > *lb_dps, size_t n,
> > > >      }
> > > >  }
> > > >
> > > > +void
> > > > +ovn_lb_datapaths_remove_lr(struct ovn_lb_datapaths *lb_dps, size_t n,
> > > > +                           struct ovn_datapath **ods)
> > > > +{
> > > > +    for (size_t i = 0; i < n; i++) {
> > > > +        if (bitmap_is_set(lb_dps->nb_lr_map, ods[i]->index)) {
> > > > +            bitmap_set0(lb_dps->nb_lr_map, ods[i]->index);
> > > > +            lb_dps->n_nb_lr--;
> > > > +        }
> > > > +    }
> > > > +}
> > > > +
> > > >  struct ovn_lb_group_datapaths *
> > > >  ovn_lb_group_datapaths_create(const struct ovn_lb_group *lb_group,
> > > >                                size_t n_ls_datapaths,
> > > > @@ -1175,5 +1190,18 @@ void
> > > >  ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *lbg_dps,
> > > >                                struct ovn_datapath *lr)
> > > >  {
> > > > -    bitmap_set1(lbg_dps->nb_lr_map, lr->index);
> > > > +    if (!bitmap_is_set(lbg_dps->nb_lr_map, lr->index)) {
> > > > +        bitmap_set1(lbg_dps->nb_lr_map, lr->index);
> > > > +        lbg_dps->n_nb_lr++;
> > > > +    }
> > > > +}
> > > > +
> > > > +void
> > > > +ovn_lb_group_datapaths_remove_lr(struct ovn_lb_group_datapaths
> *lbg_dps,
> > > > +                                 struct ovn_datapath *lr)
> > > > +{
> > > > +    if (bitmap_is_set(lbg_dps->nb_lr_map, lr->index)) {
> > > > +        bitmap_set1(lbg_dps->nb_lr_map, lr->index);
> > > > +        lbg_dps->n_nb_lr--;
> > > > +    }
> > > >  }
> > > > diff --git a/lib/lb.h b/lib/lb.h
> > > > index 08723e8ef3..91eec0199b 100644
> > > > --- a/lib/lb.h
> > > > +++ b/lib/lb.h
> > > > @@ -185,6 +185,8 @@ void ovn_lb_datapaths_set_ls(struct
> ovn_lb_datapaths
> > > *, size_t n,
> > > >
> > > >  void ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *, size_t n,
> > > >                                  struct ovn_datapath **);
> > > > +void ovn_lb_datapaths_remove_lr(struct ovn_lb_datapaths *, size_t n,
> > > > +                                struct ovn_datapath **);
> > > >
> > > >  struct ovn_lb_group_datapaths {
> > > >      struct hmap_node hmap_node;
> > > > @@ -214,6 +216,9 @@ void ovn_lb_group_datapaths_remove_ls(struct
> > > ovn_lb_group_datapaths *,
> > > >
> > > >  void ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *,
> > > >                                     struct ovn_datapath *lr);
> > > > +void ovn_lb_group_datapaths_remove_lr(struct ovn_lb_group_datapaths
> *,
> > > > +                                      struct ovn_datapath *lr);
> > > > +
> > > >  struct ovn_controller_lb {
> > > >      struct hmap_node hmap_node;
> > > >
> > > > diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> > > > index db1bcbccd6..ea51f4e3e0 100644
> > > > --- a/northd/en-lflow.c
> > > > +++ b/northd/en-lflow.c
> > > > @@ -102,6 +102,11 @@ lflow_northd_handler(struct engine_node *node,
> > > >      if (!northd_data->change_tracked) {
> > > >          return false;
> > > >      }
> > > > +
> > > > +    if (northd_data->lrouters_changed) {
> > > > +        return false;
> > > > +    }
> > >
> > > I think lrouters_changed is not needed. If lrouters related data is
> > > changed, we should set change_tracked to false (call the
> > > destroy_northd_data_tracked_changes), because lrouter data is part of
> > > northd_data nad we don't track lrouter changes yet.
> >
> >
> >    -  With this patch we could are  handling router changes
> > incrementally in northd engine node if only the router's load_balancer
> > column has changed.
> >       If we set change_tracked to false, it means northd engine node
> > didn't handle the router changes due to load balancer column
> >       incrementally and it fell back to full recompute which is not
>
> "change_tracked", as the name suggests, only indicates if the changes are
> tracked, and the purpose is for the dependent node to decide how to handle
> the changes (usually if the changes are not tracked the dependent node
> couldn't handle the changes incrementally). I think "change_tracked"
> doesn't necessarily tell if the current node is recomputed or not.

We can definitely find a better name to indicate to the dependent nodes
about it.

In the v4 there is no "lrouters_changed" but the northd tracking data has
a different field by the name - "lb_changes" which indicates to the
dependent nodes
that the northd engine data has changes to the "load balancers" in the
engine run.

Maybe we can discuss further in v4.  But a few points
    - northd engine node has  a handler for lb_data changes which sets
the northd_tracked_data->lb_changes = true
    - northd engine node has a handler for ls changes where the
handler tracks the changed VIF ports.

When lflow engine handler for the northd engine node is called,  it
can decide what to do with the provided data.

Thanks
Numan


>
> > the cause.  This 'lrounters_changed'  indicates to the lflow engine
> > node
> >       that northd engine node did a compute (and not a recompute) and
> > logical routers have changed as part of this compute.
> >
>
> Why do we need to maintain the status of whether the node is recomputed in
> such a variable?
>
> >    -  Since lflow engine node doesn't yet handle the lrouter changes,
> > it falls back to full recompute.
>
> I understand, but northd_data->change_tracked == false should just trigger
> the lflow node recompute, because the input changed but the changes are not
> tracked and obviously we don't know how to handle that in the lflow node.
>
> >
> >    - It is also possible that in the same nb db update,  a load
> > balancer was updated which is associated with both logical switch and
> > logical router.
> >      In such cases we don't want to call -
> > destroy_northd_data_tracked_changes().  Even though lflow engine does
>
> Why don't we want to call destroy_northd_data_tracked_changes()? If there
> are changes already handled incrementally and tracked, and now we know we
> can't handle the LR/LS related changes (due to LB changes associated with
> LR/LS), it is ok to release the "partially" tracked changes, right?
>
> > a recompute, from northd engine point of view,
> >      it is indicating what changed.  It is left to the dependent nodes
> > to decide if they want to handle incrementally or fall back to
> > recompute.
> >
> Yes, dependent nodes will decide, but I think change_tracked (and tracked
> changes) is sufficient for them to make the decision.
>
> (Sorry for my late response because I was just back from vacation)
>
> Thanks,
> Han
>
> > Let me know if you disagree with this.
> >
> >
> >  >
> > > > +
> > > >      const struct engine_context *eng_ctx = engine_get_context();
> > > >      struct lflow_data *lflow_data = data;
> > > >
> > > > diff --git a/northd/en-northd.c b/northd/en-northd.c
> > > > index b3c03c54bd..a321eff323 100644
> > > > --- a/northd/en-northd.c
> > > > +++ b/northd/en-northd.c
> > > > @@ -188,6 +188,26 @@ northd_nb_logical_switch_handler(struct
> engine_node
> > > *node,
> > > >      return true;
> > > >  }
> > > >
> > > > +bool
> > > > +northd_nb_logical_router_handler(struct engine_node *node,
> > > > +                                 void *data)
> > > > +{
> > > > +    struct northd_data *nd = data;
> > > > +    struct northd_input input_data;
> > > > +
> > > > +    northd_get_input_data(node, &input_data);
> > > > +
> > > > +    if (!northd_handle_lr_changes(&input_data, nd)) {
> > > > +        return false;
> > > > +    }
> > > > +
> > > > +    if (nd->change_tracked) {
> > > > +        engine_set_node_state(node, EN_UPDATED);
> > >
> > > change_tracked is supposed to indicate if the changes in the engine
> node is
> > > tracked, meaning the node depending on this one can potentailly
> > > incrementally process the changes.
> > > It is possible that the node is updated but the changes are not
> tracked. In
> > > this case most likely the nodes depending on it will need to be
> recomputed.
> > > Whether the node state is EN_UPDATED depends on if the node is updated.
> I
> > > think the northd_handle_lr_changes can take a parameter "updated" to
> > > indicate if it is updated.
> >
> > Agree.  Will use "updated" parameter instead.
> >
> > Thanks
> > Numan
> >
> >
> >
> > >
> > > > +    }
> > > > +
> > > > +    return true;
> > > > +}
> > > > +
> > > >  bool
> > > >  northd_sb_port_binding_handler(struct engine_node *node,
> > > >                                 void *data)
> > > > diff --git a/northd/en-northd.h b/northd/en-northd.h
> > > > index 5674f4390c..739aa5e87f 100644
> > > > --- a/northd/en-northd.h
> > > > +++ b/northd/en-northd.h
> > > > @@ -16,6 +16,7 @@ void en_northd_cleanup(void *data);
> > > >  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_nb_logical_router_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);
> > > >
> > > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> > > > index f5b7998f1f..362dd5e87a 100644
> > > > --- a/northd/inc-proc-northd.c
> > > > +++ b/northd/inc-proc-northd.c
> > > > @@ -156,7 +156,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop
> *nb,
> > > >
> > > >      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);
> > > >      engine_add_input(&en_northd, &en_nb_mirror, NULL);
> > > >      engine_add_input(&en_northd, &en_nb_meter, NULL);
> > > >      engine_add_input(&en_northd, &en_nb_static_mac_binding, NULL);
> > > > @@ -185,6 +184,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop
> *nb,
> > > >                       northd_nb_nb_global_handler);
> > > >      engine_add_input(&en_northd, &en_nb_logical_switch,
> > > >                       northd_nb_logical_switch_handler);
> > > > +    engine_add_input(&en_northd, &en_nb_logical_router,
> > > > +                     northd_nb_logical_router_handler);
> > > >
> > > >      engine_add_input(&en_mac_binding_aging, &en_nb_nb_global, NULL);
> > > >      engine_add_input(&en_mac_binding_aging, &en_sb_mac_binding,
> NULL);
> > > > diff --git a/northd/northd.c b/northd/northd.c
> > > > index a7761285de..4393adc2ad 100644
> > > > --- a/northd/northd.c
> > > > +++ b/northd/northd.c
> > > > @@ -4021,6 +4021,90 @@ associate_ls_lb_groups(struct ovn_datapath
> *ls_od,
> > > >      }
> > > >  }
> > > >
> > > > +static void
> > > > +associate_lr_lbs(struct ovn_datapath *lr_od, struct hmap
> > > *lb_datapaths_map)
> > > > +{
> > > > +    struct ovn_lb_datapaths *lb_dps;
> > > > +
> > > > +    free(lr_od->lb_uuids);
> > > > +    lr_od->lb_uuids = xcalloc(lr_od->nbr->n_load_balancer,
> > > > +                              sizeof *lr_od->lb_uuids);
> > > > +    lr_od->n_lb_uuids = lr_od->nbr->n_load_balancer;
> > > > +
> > > > +    if (!lr_od->lb_ips) {
> > > > +        lr_od->lb_ips = ovn_lb_ip_set_create();
> > > > +    }
> > > > +
> > > > +    for (size_t i = 0; i < lr_od->nbr->n_load_balancer; i++) {
> > > > +        const struct uuid *lb_uuid =
> > > > +            &lr_od->nbr->load_balancer[i]->header_.uuid;
> > > > +        lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> > > > +        ovs_assert(lb_dps);
> > > > +        ovn_lb_datapaths_add_lr(lb_dps, 1, &lr_od);
> > > > +        build_lrouter_lb_ips(lr_od->lb_ips, lb_dps->lb);
> > > > +        lr_od->lb_uuids[i] = *lb_uuid;
> > > > +    }
> > > > +}
> > > > +
> > > > +static void
> > > > +associate_lr_lb_groups(struct ovn_datapath *lr_od,
> > > > +                       struct hmap *lb_group_datapaths_map,
> > > > +                       struct hmap *lb_datapaths_map)
> > > > +{
> > > > +    const struct nbrec_load_balancer_group *nbrec_lb_group;
> > > > +    struct ovn_lb_group_datapaths *lb_group_dps;
> > > > +
> > > > +    free(lr_od->lb_group_uuids);
> > > > +    lr_od->lb_group_uuids =
> xcalloc(lr_od->nbr->n_load_balancer_group,
> > > > +                                    sizeof *lr_od->lb_group_uuids);
> > > > +    lr_od->n_lb_group_uuids = lr_od->nbr->n_load_balancer_group;
> > > > +
> > > > +    /* Checking load balancer groups first, starting from the largest
> > > one,
> > > > +     * to more efficiently copy IP sets. */
> > > > +    size_t largest_group = 0;
> > > > +
> > > > +    for (size_t i = 1; i < lr_od->nbr->n_load_balancer_group; i++) {
> > > > +        if (lr_od->nbr->load_balancer_group[i]->n_load_balancer >
> > > > +
> > >  lr_od->nbr->load_balancer_group[largest_group]->n_load_balancer) {
> > > > +            largest_group = i;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    for (size_t i = 0; i < lr_od->nbr->n_load_balancer_group; i++) {
> > > > +        size_t idx = (i + largest_group) %
> > > lr_od->nbr->n_load_balancer_group;
> > > > +
> > > > +        nbrec_lb_group = lr_od->nbr->load_balancer_group[idx];
> > > > +        const struct uuid *lb_group_uuid =
> &nbrec_lb_group->header_.uuid;
> > > > +
> > > > +        lb_group_dps =
> > > > +            ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > > > +                                        lb_group_uuid);
> > > > +        ovs_assert(lb_group_dps);
> > > > +        ovn_lb_group_datapaths_add_lr(lb_group_dps, lr_od);
> > > > +
> > > > +        if (!lr_od->lb_ips) {
> > > > +            lr_od->lb_ips =
> > > > +                ovn_lb_ip_set_clone(lb_group_dps->lb_group->lb_ips);
> > > > +        } else {
> > > > +            for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs;
> j++) {
> > > > +                build_lrouter_lb_ips(lr_od->lb_ips,
> > > > +                                     lb_group_dps->lb_group->lbs[j]);
> > > > +            }
> > > > +        }
> > > > +
> > > > +        lr_od->lb_group_uuids[i] = *lb_group_uuid;
> > > > +
> > > > +        struct ovn_lb_datapaths *lb_dps;
> > > > +        for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs; j++) {
> > > > +            const struct uuid *lb_uuid =
> > > > +                &lb_group_dps->lb_group->lbs[j]->nlb->header_.uuid;
> > > > +            lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> lb_uuid);
> > > > +            ovs_assert(lb_dps);
> > > > +            ovn_lb_datapaths_add_lr(lb_dps, 1, &lr_od);
> > > > +        }
> > > > +    }
> > > > +}
> > > > +
> > > >  static void
> > > >  build_lb_datapaths(const struct hmap *lbs, const struct hmap
> *lb_groups,
> > > >                     struct ovn_datapaths *ls_datapaths,
> > > > @@ -4028,7 +4112,6 @@ build_lb_datapaths(const struct hmap *lbs, const
> > > struct hmap *lb_groups,
> > > >                     struct hmap *lb_datapaths_map,
> > > >                     struct hmap *lb_group_datapaths_map)
> > > >  {
> > > > -    const struct nbrec_load_balancer_group *nbrec_lb_group;
> > > >      struct ovn_lb_group_datapaths *lb_group_dps;
> > > >      const struct ovn_lb_group *lb_group;
> > > >      struct ovn_lb_datapaths *lb_dps;
> > > > @@ -4062,53 +4145,8 @@ build_lb_datapaths(const struct hmap *lbs,
> const
> > > struct hmap *lb_groups,
> > > >
> > > >      HMAP_FOR_EACH (od, key_node, &lr_datapaths->datapaths) {
> > > >          ovs_assert(od->nbr);
> > > > -
> > > > -        /* Checking load balancer groups first, starting from the
> > > largest one,
> > > > -         * to more efficiently copy IP sets. */
> > > > -        size_t largest_group = 0;
> > > > -
> > > > -        for (size_t i = 1; i < od->nbr->n_load_balancer_group; i++) {
> > > > -            if (od->nbr->load_balancer_group[i]->n_load_balancer >
> > > > -
> > >  od->nbr->load_balancer_group[largest_group]->n_load_balancer) {
> > > > -                largest_group = i;
> > > > -            }
> > > > -        }
> > > > -
> > > > -        for (size_t i = 0; i < od->nbr->n_load_balancer_group; i++) {
> > > > -            size_t idx = (i + largest_group) %
> > > od->nbr->n_load_balancer_group;
> > > > -
> > > > -            nbrec_lb_group = od->nbr->load_balancer_group[idx];
> > > > -            const struct uuid *lb_group_uuid =
> > > &nbrec_lb_group->header_.uuid;
> > > > -
> > > > -            lb_group_dps =
> > > > -                ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > > > -                                            lb_group_uuid);
> > > > -            ovs_assert(lb_group_dps);
> > > > -            ovn_lb_group_datapaths_add_lr(lb_group_dps, od);
> > > > -
> > > > -            if (!od->lb_ips) {
> > > > -                od->lb_ips =
> > > > -
>  ovn_lb_ip_set_clone(lb_group_dps->lb_group->lb_ips);
> > > > -            } else {
> > > > -                for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs;
> > > j++) {
> > > > -                    build_lrouter_lb_ips(od->lb_ips,
> > > > -
> lb_group_dps->lb_group->lbs[j]);
> > > > -                }
> > > > -            }
> > > > -        }
> > > > -
> > > > -        if (!od->lb_ips) {
> > > > -            od->lb_ips = ovn_lb_ip_set_create();
> > > > -        }
> > > > -
> > > > -        for (size_t i = 0; i < od->nbr->n_load_balancer; i++) {
> > > > -            const struct uuid *lb_uuid =
> > > > -                &od->nbr->load_balancer[i]->header_.uuid;
> > > > -            lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> lb_uuid);
> > > > -            ovs_assert(lb_dps);
> > > > -            ovn_lb_datapaths_add_lr(lb_dps, 1, &od);
> > > > -            build_lrouter_lb_ips(od->lb_ips, lb_dps->lb);
> > > > -        }
> > > > +        associate_lr_lb_groups(od, lb_group_datapaths_map,
> > > lb_datapaths_map);
> > > > +        associate_lr_lbs(od, lb_datapaths_map);
> > > >      }
> > > >
> > > >      HMAP_FOR_EACH (lb_group_dps, hmap_node, lb_group_datapaths_map) {
> > > > @@ -4978,6 +5016,7 @@ destroy_northd_data_tracked_changes(struct
> > > northd_data *nd)
> > > >      }
> > > >
> > > >      nd->change_tracked = false;
> > > > +    nd->lrouters_changed = false;
> > > >  }
> > > >
> > > >  /* Check if a changed LSP can be handled incrementally within the I-P
> > > engine
> > > > @@ -5513,6 +5552,263 @@ fail:
> > > >      return false;
> > > >  }
> > > >
> > > > +/* Returns true if the logical router has changes which are not
> > > > + * incrementally handled.
> > > > + * Presently supports i-p for the below changes:
> > > > + *    - load balancers and load balancer groups.
> > > > + */
> > > > +static bool
> > > > +check_unsupported_inc_proc_for_lr_changes(
> > > > +    const struct nbrec_logical_router *lr)
> > > > +{
> > > > +    /* Check if the columns are changed in this row. */
> > > > +    enum nbrec_logical_router_column_id col;
> > > > +    for (col = 0; col < NBREC_LOGICAL_ROUTER_N_COLUMNS; col++) {
> > > > +        if (nbrec_logical_router_is_updated(lr, col)) {
> > > > +            if (col == NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER ||
> > > > +                col == NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER_GROUP)
> {
> > > > +                continue;
> > > > +            }
> > > > +            return true;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    /* Check if the referenced rows are changed.
> > > > +       XXX: Need a better OVSDB IDL interface for this check. */
> > > > +    for (size_t i = 0; i < lr->n_ports; i++) {
> > > > +        if (nbrec_logical_router_port_row_get_seqno(lr->ports[i],
> > > > +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > > > +            return true;
> > > > +        }
> > > > +    }
> > > > +    if (lr->copp && nbrec_copp_row_get_seqno(lr->copp,
> > > > +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > > > +        return true;
> > > > +    }
> > > > +    for (size_t i = 0; i < lr->n_nat; i++) {
> > > > +        if (nbrec_nat_row_get_seqno(lr->nat[i],
> > > > +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > > > +            return true;
> > > > +        }
> > > > +    }
> > > > +    for (size_t i = 0; i < lr->n_policies; i++) {
> > > > +        if
> (nbrec_logical_router_policy_row_get_seqno(lr->policies[i],
> > > > +                                OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > > > +            return true;
> > > > +        }
> > > > +    }
> > > > +    for (size_t i = 0; i < lr->n_static_routes; i++) {
> > > > +        if (nbrec_logical_router_static_route_row_get_seqno(
> > > > +            lr->static_routes[i], OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > > > +            return true;
> > > > +        }
> > > > +    }
> > > > +    return false;
> > > > +}
> > > > +
> > > > +static bool
> > > > +lr_check_and_handle_lb_and_lbgrp_changes(
> > > > +    const struct nbrec_logical_router *changed_lr,
> > > > +    struct hmap *lb_datapaths_map, struct hmap
> *lb_group_datapaths_map,
> > > > +    struct ovn_datapath *od, bool *updated)
> > > > +{
> > > > +    ovs_assert(od->nbr);
> > > > +    bool lbs_changed = true;
> > > > +    if (!nbrec_logical_router_is_updated(changed_lr,
> > > > +                        NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER) &&
> > > > +        !nbrec_logical_router_is_updated(changed_lr,
> > > > +
>  NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER_GROUP)) {
> > > > +        lbs_changed = false;
> > > > +        for (size_t i = 0; i < changed_lr->n_load_balancer_group;
> i++) {
> > > > +            if (nbrec_load_balancer_group_row_get_seqno(
> > > > +                    changed_lr->load_balancer_group[i],
> > > > +                    OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > > > +                lbs_changed = true;
> > > > +                break;
> > > > +            }
> > > > +        }
> > > > +
> > > > +        if (!lbs_changed) {
> > > > +            for (size_t i = 0; i < changed_lr->n_load_balancer; i++)
> {
> > > > +                if (nbrec_load_balancer_row_get_seqno(
> > > > +                        changed_lr->load_balancer[i],
> > > > +                        OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > > > +                    lbs_changed = true;
> > > > +                    break;
> > > > +                }
> > > > +            }
> > > > +        }
> > > > +
> > > > +        if (!lbs_changed) {
> > > > +            return true;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    /* We need to do 2 things to handle the router for load balancer
> or
> > > > +     * load balancer groups.
> > > > +     *
> > > > +     * 1. Disassociate the logical router datapath from the already
> > > associated
> > > > +     *    load balancers/load balancer groups.
> > > > +     * 2. Associate the logical router datapath with the updated
> > > > +     *    load balancers/groups.
> > > > +     * */
> > > > +    struct ovn_lb_datapaths *lb_dps;
> > > > +    size_t i, j;
> > > > +    for (i = 0; i < od->n_lb_uuids; i++) {
> > > > +        lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> > > &od->lb_uuids[i]);
> > > > +        if (lb_dps) {
> > > > +            if (lb_dps->lb->routable) {
> > > > +                /* Can't handler if the load balancer has routable
> flag
> > > set.
> > > > +                 * This requires updating the router ports's routable
> > > > +                 * addresses. */
> > > > +                return false;
> > > > +            }
> > > > +            ovn_lb_datapaths_remove_lr(lb_dps, 1, &od);
> > > > +        }
> > > > +    }
> > > > +
> > > > +    struct ovn_lb_group_datapaths *lbg_dps;
> > > > +    for (i = 0; i < od->n_lb_group_uuids; i++) {
> > > > +        lbg_dps = ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > > > +
>  &od->lb_group_uuids[i]);
> > > > +        if (lbg_dps) {
> > > > +            ovn_lb_group_datapaths_remove_lr(lbg_dps, od);
> > > > +
> > > > +            if (install_ls_lb_from_router && od->n_ls_peers) {
> > > > +                ovn_lb_group_datapaths_remove_ls(lbg_dps,
> od->n_ls_peers,
> > > > +                                                 od->ls_peers);
> > > > +            }
> > > > +        }
> > > > +
> > > > +        for (j = 0; j < lbg_dps->lb_group->n_lbs; j++) {
> > > > +            const struct uuid *lb_uuid =
> > > > +                &lbg_dps->lb_group->lbs[j]->nlb->header_.uuid;
> > > > +            lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> lb_uuid);
> > > > +            if (lb_dps) {
> > > > +                if (lb_dps->lb->routable) {
> > > > +                    /* Can't handler if the load balancer has
> routable
> > > flag
> > > > +                     * set.  This requires updating the router
> ports's
> > > > +                     * routable addresses. */
> > > > +                    return false;
> > > > +                }
> > > > +                ovn_lb_datapaths_remove_lr(lb_dps, 1, &od);
> > > > +
> > > > +                if (install_ls_lb_from_router && od->n_ls_peers) {
> > > > +                    ovn_lb_datapaths_remove_ls(lb_dps,
> od->n_ls_peers,
> > > > +                                               od->ls_peers);
> > > > +                }
> > > > +            }
> > > > +        }
> > > > +    }
> > > > +
> > > > +    ovn_lb_ip_set_destroy(od->lb_ips);
> > > > +    od->lb_ips = NULL;
> > > > +    associate_lr_lb_groups(od, lb_group_datapaths_map,
> lb_datapaths_map);
> > > > +    associate_lr_lbs(od, lb_datapaths_map);
> > > > +
> > > > +    /* Do the job of build_lrouter_lbs_reachable_ips and
> > > > +     * build_lswitch_lbs_from_lrouter for this lrouter datapath. */
> > > > +    for (i = 0; i < od->nbr->n_load_balancer; i++) {
> > > > +        lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> > > > +
> > >  &od->nbr->load_balancer[i]->header_.uuid);
> > > > +        ovs_assert(lb_dps);
> > > > +        if (lb_dps->lb->routable) {
> > > > +            /* Can't handler if the load balancer has routable flag
> set.
> > > > +             * This requires updating the router ports's routable
> > > addresses. */
> > > > +            return false;
> > > > +        }
> > > > +        build_lrouter_lb_reachable_ips(od, lb_dps->lb);
> > > > +
> > > > +        if (install_ls_lb_from_router && od->n_ls_peers) {
> > > > +            ovn_lb_datapaths_add_ls(lb_dps, od->n_ls_peers,
> > > od->ls_peers);
> > > > +        }
> > > > +    }
> > > > +
> > > > +    for (i = 0; i < od->nbr->n_load_balancer_group; i++) {
> > > > +        const struct nbrec_load_balancer_group *nbrec_lb_group =
> > > > +            od->nbr->load_balancer_group[i];
> > > > +        lbg_dps = ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > > > +
> > >  &nbrec_lb_group->header_.uuid);
> > > > +        ovs_assert(lbg_dps);
> > > > +        if (install_ls_lb_from_router && od->n_ls_peers) {
> > > > +            ovn_lb_group_datapaths_add_ls(lbg_dps, od->n_ls_peers,
> > > > +                                          od->ls_peers);
> > > > +        }
> > > > +
> > > > +        for (j = 0; j < lbg_dps->lb_group->n_lbs; j++) {
> > > > +            build_lrouter_lb_reachable_ips(od,
> > > lbg_dps->lb_group->lbs[j]);
> > > > +
> > > > +            if (install_ls_lb_from_router && od->n_ls_peers) {
> > > > +                const struct uuid *lb_uuid =
> > > > +                    &lbg_dps->lb_group->lbs[j]->nlb->header_.uuid;
> > > > +                lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> > > lb_uuid);
> > > > +                ovs_assert(lb_dps);
> > > > +                if (lb_dps->lb->routable) {
> > > > +                    /* Can't handler if the load balancer has
> routable
> > > flag
> > > > +                     * set.  This requires updating the router
> ports's
> > > > +                     * routable addresses. */
> > > > +                    return false;
> > > > +                }
> > > > +                ovn_lb_datapaths_add_ls(lb_dps, od->n_ls_peers,
> > > od->ls_peers);
> > > > +            }
> > > > +        }
> > > > +    }
> > > > +
> > > > +    *updated = true;
> > > > +    /* Re-evaluate 'od->has_lb_vip' */
> > > > +    init_lb_for_datapath(od);
> > > > +    return true;
> > > > +}
> > > > +
> > > > +
> > > > +/* Return true if changes are handled incrementally, false otherwise.
> > > > + * When there are any changes, try to track what's exactly changed
> and
> > > set
> > > > + * northd_data->change_tracked accordingly: change tracked - true,
> > > otherwise,
> > > > + * false. */
> > > > +bool
> > > > +northd_handle_lr_changes(const struct northd_input *ni,
> > > > +                         struct northd_data *nd)
> > > > +{
> > > > +    const struct nbrec_logical_router *changed_lr;
> > > > +
> > > > +    bool updated = false;
> > > > +
> > > > +    NBREC_LOGICAL_ROUTER_TABLE_FOR_EACH_TRACKED (changed_lr,
> > > > +
> > > ni->nbrec_logical_router_table) {
> > > > +        if (nbrec_logical_router_is_new(changed_lr) ||
> > > > +            nbrec_logical_router_is_deleted(changed_lr)) {
> > > > +            goto fail;
> > > > +        }
> > > > +
> > > > +        struct ovn_datapath *od = ovn_datapath_find(
> > > > +                                    &nd->lr_datapaths.datapaths,
> > > > +                                    &changed_lr->header_.uuid);
> > > > +
> > > > +        /* Presently only able to handle load balancer and
> > > > +         * load balancer group changes. */
> > > > +        if (check_unsupported_inc_proc_for_lr_changes(changed_lr)) {
> > > > +            goto fail;
> > > > +        }
> > > > +
> > > > +        if (!lr_check_and_handle_lb_and_lbgrp_changes(changed_lr,
> > > > +
>  &nd->lb_datapaths_map,
> > > > +
> > >  &nd->lb_group_datapaths_map,
> > > > +                                                od, &updated)) {
> > > > +            goto fail;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    if (updated) {
> > > > +        nd->change_tracked = true;
> > >
> > > Updated doesn't mean the change is tracked. Please see the earlier
> comments.
> > >
> > > > +        nd->lrouters_changed = true;
> > > > +    }
> > > > +
> > > > +    return true;
> > > > +fail:
> > > > +    destroy_northd_data_tracked_changes(nd);
> > >
> > > I think we don't need "fail:" here. Instead, just return false and call
> the
> > > destroy_northd_data_tracked_changes if lrouter is updated (but not
> tracked)
> > > or can't be handled.
> > >
> > > Thanks,
> > > Han
> > >
> > > > +    return false;
> > > > +}
> > > > +
> > > >  bool
> > > >  northd_handle_sb_port_binding_changes(
> > > >      const struct sbrec_port_binding_table *sbrec_port_binding_table,
> > > > diff --git a/northd/northd.h b/northd/northd.h
> > > > index a79cc470c3..765720849a 100644
> > > > --- a/northd/northd.h
> > > > +++ b/northd/northd.h
> > > > @@ -119,8 +119,11 @@ struct northd_data {
> > > >      struct chassis_features features;
> > > >      struct sset svc_monitor_lsps;
> > > >      struct hmap svc_monitor_map;
> > > > +
> > > > +    /* Tracked data. */
> > > >      bool change_tracked;
> > > >      struct tracked_ls_changes tracked_ls_changes;
> > > > +    bool lrouters_changed;
> > > >  };
> > > >
> > > >  struct lflow_data {
> > > > @@ -342,6 +345,8 @@ void ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
> > > >  bool northd_handle_ls_changes(struct ovsdb_idl_txn *,
> > > >                                const struct northd_input *,
> > > >                                struct northd_data *);
> > > > +bool northd_handle_lr_changes(const struct northd_input *,
> > > > +                              struct northd_data *);
> > > >  void destroy_northd_data_tracked_changes(struct northd_data *);
> > > >  void northd_destroy(struct northd_data *data);
> > > >  void northd_init(struct northd_data *data);
> > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > > > index d3a5851e35..286b311242 100644
> > > > --- a/tests/ovn-northd.at
> > > > +++ b/tests/ovn-northd.at
> > > > @@ -9760,6 +9760,14 @@ 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 norecompute recompute
> > > >
> > > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > +check ovn-nbctl lr-lb-add lr0 lb1
> > > > +check_engine_stats norecompute recompute
> > > > +
> > > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > +check ovn-nbctl lr-lb-del lr0 lb1
> > > > +check_engine_stats norecompute 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)
> > > > @@ -9796,11 +9804,11 @@ check_engine_stats norecompute 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_engine_stats norecompute 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
> > > > +check_engine_stats norecompute recompute
> > > >
> > > >  AT_CLEANUP
> > > >  ])
> > > > --
> > > > 2.40.1
> > > >
> > > > _______________________________________________
> > > > 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
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev