[ovs-dev,v6,04/16] northd: Refactor the 'northd' node code which handles logical switch changes.

From: Numan Siddique <numans@ovn.org>

This will help in handling other column changes of a logical switch.

Acked-by: Han Zhou <hzhou@ovn.org>
Signed-off-by: Numan Siddique <numans@ovn.org>
---
 northd/en-lb-data.c |   2 +-
 northd/northd.c     | 356 ++++++++++++++++++++++++++------------------
 2 files changed, 209 insertions(+), 149 deletions(-)

On Fri, Aug 18, 2023 at 1:57 AM <numans@ovn.org> wrote:
>
> From: Numan Siddique <numans@ovn.org>
>
> This will help in handling other column changes of a logical switch.
>
> Acked-by: Han Zhou <hzhou@ovn.org>
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>  northd/en-lb-data.c |   2 +-
>  northd/northd.c     | 356 ++++++++++++++++++++++++++------------------
>  2 files changed, 209 insertions(+), 149 deletions(-)
>
> diff --git a/northd/en-lb-data.c b/northd/en-lb-data.c
> index 328c003675..23f2cb1021 100644
> --- a/northd/en-lb-data.c
> +++ b/northd/en-lb-data.c
> @@ -58,7 +58,7 @@ static void add_deleted_lb_group_to_tracked_data(
>  /* 'lb_data' engine node manages the NB load balancers and load balancer
>   * groups.  For each NB LB, it creates 'struct ovn_northd_lb' and
>   * for each NB LB group, it creates 'struct ovn_lb_group' and stores in
> - * the respective hmaps in it data (ed_type_lb_data).
> + * the respective hmaps in it's data (ed_type_lb_data).

nit: this change belongs to a previous patch.

Other than this, LGTM (already acked :))

>   */
>  void *
>  en_lb_data_init(struct engine_node *node OVS_UNUSED,
> diff --git a/northd/northd.c b/northd/northd.c
> index e9afa07177..4cc9ef8c8d 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -4891,23 +4891,29 @@ build_ports(struct ovsdb_idl_txn *ovnsb_txn,
>      sset_destroy(&active_ha_chassis_grps);
>  }
>
> +static void
> +destroy_tracked_ls_change(struct ls_change *ls_change)
> +{
> +    struct ovn_port *op;
> +    LIST_FOR_EACH (op, list, &ls_change->added_ports) {
> +        ovs_list_remove(&op->list);
> +    }
> +    LIST_FOR_EACH (op, list, &ls_change->updated_ports) {
> +        ovs_list_remove(&op->list);
> +    }
> +    LIST_FOR_EACH_SAFE (op, list, &ls_change->deleted_ports) {
> +        ovs_list_remove(&op->list);
> +        ovn_port_destroy_orphan(op);
> +    }
> +}
> +
>  void
>  destroy_northd_data_tracked_changes(struct northd_data *nd)
>  {
>      struct ls_change *ls_change;
>      LIST_FOR_EACH_SAFE (ls_change, list_node,
>                          &nd->tracked_ls_changes.updated) {
> -        struct ovn_port *op;
> -        LIST_FOR_EACH (op, list, &ls_change->added_ports) {
> -            ovs_list_remove(&op->list);
> -        }
> -        LIST_FOR_EACH (op, list, &ls_change->updated_ports) {
> -            ovs_list_remove(&op->list);
> -        }
> -        LIST_FOR_EACH_SAFE (op, list, &ls_change->deleted_ports) {
> -            ovs_list_remove(&op->list);
> -            ovn_port_destroy_orphan(op);
> -        }
> +        destroy_tracked_ls_change(ls_change);
>          ovs_list_remove(&ls_change->list_node);
>          free(ls_change);
>      }
> @@ -5024,15 +5030,21 @@ ls_port_create(struct ovsdb_idl_txn *ovnsb_txn,
struct hmap *ls_ports,
>      return op;
>  }
>
> +/* Returns true if the logical switch has changes which can be
> + * incrementally handled.
> + * Presently supports i-p for the below changes:
> + *    - logical switch ports.
> + */
>  static bool
> -check_ls_changes_other_than_lsp(const struct nbrec_logical_switch *ls)
> +ls_changes_can_be_handled(
> +    const struct nbrec_logical_switch *ls)
>  {
>      /* Check if the columns are changed in this row. */
>      enum nbrec_logical_switch_column_id col;
>      for (col = 0; col < NBREC_LOGICAL_SWITCH_N_COLUMNS; col++) {
>          if (nbrec_logical_switch_is_updated(ls, col) &&
>              col != NBREC_LOGICAL_SWITCH_COL_PORTS) {
> -            return true;
> +            return false;
>          }
>      }
>
> @@ -5041,44 +5053,44 @@ check_ls_changes_other_than_lsp(const struct
nbrec_logical_switch *ls)
>      for (size_t i = 0; i < ls->n_acls; i++) {
>          if (nbrec_acl_row_get_seqno(ls->acls[i],
>                                  OVSDB_IDL_CHANGE_MODIFY) > 0) {
> -            return true;
> +            return false;
>          }
>      }
>      if (ls->copp && nbrec_copp_row_get_seqno(ls->copp,
>                                  OVSDB_IDL_CHANGE_MODIFY) > 0) {
> -        return true;
> +        return false;
>      }
>      for (size_t i = 0; i < ls->n_dns_records; i++) {
>          if (nbrec_dns_row_get_seqno(ls->dns_records[i],
>                                  OVSDB_IDL_CHANGE_MODIFY) > 0) {
> -            return true;
> +            return false;
>          }
>      }
>      for (size_t i = 0; i < ls->n_forwarding_groups; i++) {
>          if
(nbrec_forwarding_group_row_get_seqno(ls->forwarding_groups[i],
>                                  OVSDB_IDL_CHANGE_MODIFY) > 0) {
> -            return true;
> +            return false;
>          }
>      }
>      for (size_t i = 0; i < ls->n_load_balancer; i++) {
>          if (nbrec_load_balancer_row_get_seqno(ls->load_balancer[i],
>                                  OVSDB_IDL_CHANGE_MODIFY) > 0) {
> -            return true;
> +            return false;
>          }
>      }
>      for (size_t i = 0; i < ls->n_load_balancer_group; i++) {
>          if
(nbrec_load_balancer_group_row_get_seqno(ls->load_balancer_group[i],
>                                  OVSDB_IDL_CHANGE_MODIFY) > 0) {
> -            return true;
> +            return false;
>          }
>      }
>      for (size_t i = 0; i < ls->n_qos_rules; i++) {
>          if (nbrec_qos_row_get_seqno(ls->qos_rules[i],
>                                  OVSDB_IDL_CHANGE_MODIFY) > 0) {
> -            return true;
> +            return false;
>          }
>      }
> -    return false;
> +    return true;
>  }
>
>  static bool
> @@ -5119,6 +5131,172 @@ check_lsp_changes_other_than_up(const struct
nbrec_logical_switch_port *nbsp)
>      return false;
>  }
>
> +/* Handles logical switch port changes of a changed logical switch.
> + * Returns false, if any logical port can't be incrementally handled.
> + */
> +static bool
> +ls_handle_lsp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn,
> +                                const struct nbrec_logical_switch
*changed_ls,
> +                                const struct northd_input *ni,
> +                                struct northd_data *nd,
> +                                struct ovn_datapath *od,
> +                                struct ls_change *ls_change,
> +                                bool *updated)
> +{
> +    bool ls_ports_changed = false;
> +    if (!nbrec_logical_switch_is_updated(changed_ls,
> +
NBREC_LOGICAL_SWITCH_COL_PORTS)) {
> +
> +        for (size_t i = 0; i < changed_ls->n_ports; i++) {
> +            if (nbrec_logical_switch_port_row_get_seqno(
> +                changed_ls->ports[i], OVSDB_IDL_CHANGE_MODIFY) > 0) {
> +                ls_ports_changed = true;
> +                break;
> +            }
> +        }
> +    } else {
> +        ls_ports_changed = true;
> +    }
> +
> +    if (!ls_ports_changed) {
> +        return true;
> +    }
> +
> +    /* Check if the logical switch has only router ports before this
change.
> +     * If so, fall back to recompute.
> +     * lflow engine node while building the lflows checks if the logical
switch
> +     * has any router ports and depending on that it adds different
flows.
> +     * See build_lswitch_rport_arp_req_flow() for more details.
> +     * Note: We can definitely handle this scenario incrementally in the
> +     * northd engine node and fall back to recompute in lflow engine node
> +     * and even handle this incrementally in lflow node.  Until we do
that
> +     * resort to full recompute of northd node.
> +     */
> +    bool only_rports = (od->n_router_ports
> +                        && (od->n_router_ports ==
hmap_count(&od->ports)));
> +    if (only_rports) {
> +        return false;
> +    }
> +
> +    struct ovn_port *op;
> +    HMAP_FOR_EACH (op, dp_node, &od->ports) {
> +        op->visited = false;
> +    }
> +
> +    /* Compare the individual ports in the old and new Logical Switches
*/
> +    for (size_t j = 0; j < changed_ls->n_ports; ++j) {
> +        struct nbrec_logical_switch_port *new_nbsp =
changed_ls->ports[j];
> +        op = ovn_port_find_in_datapath(od, new_nbsp->name);
> +
> +        if (!op) {
> +            if (!lsp_can_be_inc_processed(new_nbsp)) {
> +                goto fail;
> +            }
> +            op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
> +                                new_nbsp->name, new_nbsp, od, NULL, NULL,
> +                                ni->sbrec_mirror_table,
> +                                ni->sbrec_chassis_table,
> +                                ni->sbrec_chassis_by_name,
> +                                ni->sbrec_chassis_by_hostname);
> +            if (!op) {
> +                goto fail;
> +            }
> +            ovs_list_push_back(&ls_change->added_ports,
> +                                &op->list);
> +        } else if (ls_port_has_changed(op->nbsp, new_nbsp)) {
> +            /* Existing port updated */
> +            bool temp = false;
> +            if (lsp_is_type_changed(op->sb, new_nbsp, &temp) ||
> +                !op->lsp_can_be_inc_processed ||
> +                !lsp_can_be_inc_processed(new_nbsp)) {
> +                goto fail;
> +            }
> +            const struct sbrec_port_binding *sb = op->sb;
> +            if (sset_contains(&nd->svc_monitor_lsps, new_nbsp->name)) {
> +                /* This port is used for svc monitor, which may be
impacted
> +                 * by this change. Fallback to recompute. */
> +                goto fail;
> +            }
> +            if (!check_lsp_is_up &&
> +                !check_lsp_changes_other_than_up(new_nbsp)) {
> +                /* If the only change is the "up" column while the
> +                 * "ignore_lsp_down" is set to true, just ignore this
> +                 * change. */
> +                op->visited = true;
> +                continue;
> +            }
> +            struct ovs_list lflows = OVS_LIST_INITIALIZER(&lflows);
> +            ovs_list_splice(&lflows, op->lflows.next, &op->lflows);
> +            ovn_port_destroy(&nd->ls_ports, op);
> +            op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
> +                                new_nbsp->name, new_nbsp, od, sb,
&lflows,
> +                                ni->sbrec_mirror_table,
> +                                ni->sbrec_chassis_table,
> +                                ni->sbrec_chassis_by_name,
> +                                ni->sbrec_chassis_by_hostname);
> +            ovs_assert(ovs_list_is_empty(&lflows));
> +            if (!op) {
> +                goto fail;
> +            }
> +            ovs_list_push_back(&ls_change->updated_ports, &op->list);
> +        }
> +        op->visited = true;
> +    }
> +
> +    /* Check for deleted ports */
> +    HMAP_FOR_EACH_SAFE (op, dp_node, &od->ports) {
> +        if (!op->visited) {
> +            if (!op->lsp_can_be_inc_processed) {
> +                goto fail_clean_deleted;
> +            }
> +            if (sset_contains(&nd->svc_monitor_lsps, op->key)) {
> +                /* This port was used for svc monitor, which may be
> +                 * impacted by this deletion. Fallback to recompute. */
> +                goto fail_clean_deleted;
> +            }
> +            ovs_list_push_back(&ls_change->deleted_ports,
> +                                &op->list);
> +            hmap_remove(&nd->ls_ports, &op->key_node);
> +            hmap_remove(&od->ports, &op->dp_node);
> +            sbrec_port_binding_delete(op->sb);
> +            delete_fdb_entry(ni->sbrec_fdb_by_dp_and_port,
od->tunnel_key,
> +                                op->tunnel_key);
> +        }
> +    }
> +
> +    /* Check if the logical switch has only router ports after this
change.
> +     * If so, fall back to recompute.
> +     * lflow engine node while building the lflows checks if the logical
switch
> +     * has any router ports and depending on that it adds different
flows.
> +     * See build_lswitch_rport_arp_req_flow() for more details.
> +     * Note: We can definitely handle this scenario incrementally in the
> +     * northd engine node and fall back to recompute in lflow engine node
> +     * and even handle this incrementally in lflow node.  Until we do
that
> +     * resort to full recompute of northd node.
> +     */
> +    only_rports = (od->n_router_ports
> +                    && (od->n_router_ports == hmap_count(&od->ports)));
> +    if (only_rports) {
> +        goto fail_clean_deleted;
> +    }
> +
> +    if (!ovs_list_is_empty(&ls_change->added_ports) ||
> +        !ovs_list_is_empty(&ls_change->updated_ports) ||
> +        !ovs_list_is_empty(&ls_change->deleted_ports)) {
> +        *updated = true;
> +    }
> +
> +    return true;
> +
> +fail_clean_deleted:
> +    LIST_FOR_EACH_POP (op, list, &ls_change->deleted_ports) {
> +        ovn_port_destroy_orphan(op);
> +    }
> +
> +fail:
> +    return false;
> +}
> +
>  /* 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,
> @@ -5130,11 +5308,9 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
*ovnsb_idl_txn,
>  {
>      const struct nbrec_logical_switch *changed_ls;
>      struct ls_change *ls_change = NULL;
> -    struct ovn_port *op;
>
>      NBREC_LOGICAL_SWITCH_TABLE_FOR_EACH_TRACKED (changed_ls,
>
ni->nbrec_logical_switch_table) {
> -        ls_change = NULL;
>          if (nbrec_logical_switch_is_new(changed_ls) ||
>              nbrec_logical_switch_is_deleted(changed_ls)) {
>              goto fail;
> @@ -5150,8 +5326,8 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
*ovnsb_idl_txn,
>              goto fail;
>          }
>
> -        /* Now only able to handle lsp changes. */
> -        if (check_ls_changes_other_than_lsp(changed_ls)) {
> +        /* Check if the ls changes can be handled or not. */
> +        if (!ls_changes_can_be_handled(changed_ls)) {
>              goto fail;
>          }
>
> @@ -5161,126 +5337,16 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
*ovnsb_idl_txn,
>          ovs_list_init(&ls_change->deleted_ports);
>          ovs_list_init(&ls_change->updated_ports);
>
> -        HMAP_FOR_EACH (op, dp_node, &od->ports) {
> -            op->visited = false;
> -        }
> -
> -        /* Check if the logical switch has only router ports before this
change.
> -         * If so, fall back to recompute.
> -         * lflow engine node while building the lflows checks if the
logical switch
> -         * has any router ports and depending on that it adds different
flows.
> -         * See build_lswitch_rport_arp_req_flow() for more details.
> -         * Note: We can definitely handle this scenario incrementally in
the
> -         * northd engine node and fall back to recompute in lflow engine
node
> -         * and even handle this incrementally in lflow node.  Until we
do that
> -         * resort to full recompute of northd node.
> -         */
> -        bool only_rports = (od->n_router_ports
> -                            && (od->n_router_ports ==
hmap_count(&od->ports)));
> -        if (only_rports) {
> +        bool updated = false;
> +        if (!ls_handle_lsp_changes(ovnsb_idl_txn, changed_ls,
> +                                   ni, nd, od, ls_change,
> +                                   &updated)) {
> +            destroy_tracked_ls_change(ls_change);
> +            free(ls_change);
>              goto fail;
>          }
>
> -        /* Compare the individual ports in the old and new Logical
Switches */
> -        for (size_t j = 0; j < changed_ls->n_ports; ++j) {
> -            struct nbrec_logical_switch_port *new_nbsp =
changed_ls->ports[j];
> -            op = ovn_port_find_in_datapath(od, new_nbsp->name);
> -
> -            if (!op) {
> -                if (!lsp_can_be_inc_processed(new_nbsp)) {
> -                    goto fail;
> -                }
> -                op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
> -                                    new_nbsp->name, new_nbsp, od, NULL,
NULL,
> -                                    ni->sbrec_mirror_table,
> -                                    ni->sbrec_chassis_table,
> -                                    ni->sbrec_chassis_by_name,
> -                                    ni->sbrec_chassis_by_hostname);
> -                if (!op) {
> -                    goto fail;
> -                }
> -                ovs_list_push_back(&ls_change->added_ports,
> -                                   &op->list);
> -            } else if (ls_port_has_changed(op->nbsp, new_nbsp)) {
> -                /* Existing port updated */
> -                bool temp = false;
> -                if (lsp_is_type_changed(op->sb, new_nbsp, &temp) ||
> -                    !op->lsp_can_be_inc_processed ||
> -                    !lsp_can_be_inc_processed(new_nbsp)) {
> -                    goto fail;
> -                }
> -                const struct sbrec_port_binding *sb = op->sb;
> -                if (sset_contains(&nd->svc_monitor_lsps,
new_nbsp->name)) {
> -                    /* This port is used for svc monitor, which may be
impacted
> -                     * by this change. Fallback to recompute. */
> -                    goto fail;
> -                }
> -                if (!check_lsp_is_up &&
> -                    !check_lsp_changes_other_than_up(new_nbsp)) {
> -                    /* If the only change is the "up" column while the
> -                     * "ignore_lsp_down" is set to true, just ignore this
> -                     * change. */
> -                    op->visited = true;
> -                    continue;
> -                }
> -                struct ovs_list lflows = OVS_LIST_INITIALIZER(&lflows);
> -                ovs_list_splice(&lflows, op->lflows.next, &op->lflows);
> -                ovn_port_destroy(&nd->ls_ports, op);
> -                op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
> -                                    new_nbsp->name, new_nbsp, od, sb,
&lflows,
> -                                    ni->sbrec_mirror_table,
> -                                    ni->sbrec_chassis_table,
> -                                    ni->sbrec_chassis_by_name,
> -                                    ni->sbrec_chassis_by_hostname);
> -                ovs_assert(ovs_list_is_empty(&lflows));
> -                if (!op) {
> -                    goto fail;
> -                }
> -                ovs_list_push_back(&ls_change->updated_ports, &op->list);
> -            }
> -            op->visited = true;
> -        }
> -
> -        /* Check for deleted ports */
> -        HMAP_FOR_EACH_SAFE (op, dp_node, &od->ports) {
> -            if (!op->visited) {
> -                if (!op->lsp_can_be_inc_processed) {
> -                    goto fail_clean_deleted;
> -                }
> -                if (sset_contains(&nd->svc_monitor_lsps, op->key)) {
> -                    /* This port was used for svc monitor, which may be
> -                     * impacted by this deletion. Fallback to recompute.
*/
> -                    goto fail_clean_deleted;
> -                }
> -                ovs_list_push_back(&ls_change->deleted_ports,
> -                                   &op->list);
> -                hmap_remove(&nd->ls_ports, &op->key_node);
> -                hmap_remove(&od->ports, &op->dp_node);
> -                sbrec_port_binding_delete(op->sb);
> -                delete_fdb_entry(ni->sbrec_fdb_by_dp_and_port,
od->tunnel_key,
> -                                 op->tunnel_key);
> -            }
> -        }
> -
> -        /* Check if the logical switch has only router ports after this
change.
> -         * If so, fall back to recompute.
> -         * lflow engine node while building the lflows checks if the
logical switch
> -         * has any router ports and depending on that it adds different
flows.
> -         * See build_lswitch_rport_arp_req_flow() for more details.
> -         * Note: We can definitely handle this scenario incrementally in
the
> -         * northd engine node and fall back to recompute in lflow engine
node
> -         * and even handle this incrementally in lflow node.  Until we
do that
> -         * resort to full recompute of northd node.
> -         */
> -        only_rports = (od->n_router_ports
> -                       && (od->n_router_ports ==
hmap_count(&od->ports)));
> -        if (only_rports) {
> -            goto fail_clean_deleted;
> -        }
> -
> -        if (!ovs_list_is_empty(&ls_change->added_ports) ||
> -            !ovs_list_is_empty(&ls_change->updated_ports) ||
> -            !ovs_list_is_empty(&ls_change->deleted_ports)) {
> +        if (updated) {
>              ovs_list_push_back(&nd->tracked_ls_changes.updated,
>                                 &ls_change->list_node);
>          } else {
> @@ -5293,13 +5359,7 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
*ovnsb_idl_txn,
>      }
>      return true;
>
> -fail_clean_deleted:
> -    LIST_FOR_EACH_POP (op, list, &ls_change->deleted_ports) {
> -        ovn_port_destroy_orphan(op);
> -    }
> -
>  fail:
> -    free(ls_change);
>      destroy_northd_data_tracked_changes(nd);
>      return false;
>  }
> --
> 2.40.1
>

On Fri, Aug 18, 2023 at 10:58 AM <numans@ovn.org> wrote:
>
> From: Numan Siddique <numans@ovn.org>
>
> This will help in handling other column changes of a logical switch.
>
> Acked-by: Han Zhou <hzhou@ovn.org>
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>  northd/en-lb-data.c |   2 +-
>  northd/northd.c     | 356 ++++++++++++++++++++++++++------------------
>  2 files changed, 209 insertions(+), 149 deletions(-)
>
> diff --git a/northd/en-lb-data.c b/northd/en-lb-data.c
> index 328c003675..23f2cb1021 100644
> --- a/northd/en-lb-data.c
> +++ b/northd/en-lb-data.c
> @@ -58,7 +58,7 @@ static void add_deleted_lb_group_to_tracked_data(
>  /* 'lb_data' engine node manages the NB load balancers and load balancer
>   * groups.  For each NB LB, it creates 'struct ovn_northd_lb' and
>   * for each NB LB group, it creates 'struct ovn_lb_group' and stores in
> - * the respective hmaps in it data (ed_type_lb_data).
> + * the respective hmaps in it's data (ed_type_lb_data).
>   */
>  void *
>  en_lb_data_init(struct engine_node *node OVS_UNUSED,
> diff --git a/northd/northd.c b/northd/northd.c
> index e9afa07177..4cc9ef8c8d 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -4891,23 +4891,29 @@ build_ports(struct ovsdb_idl_txn *ovnsb_txn,
>      sset_destroy(&active_ha_chassis_grps);
>  }
>
> +static void
> +destroy_tracked_ls_change(struct ls_change *ls_change)
> +{
> +    struct ovn_port *op;
> +    LIST_FOR_EACH (op, list, &ls_change->added_ports) {
> +        ovs_list_remove(&op->list);
> +    }
> +    LIST_FOR_EACH (op, list, &ls_change->updated_ports) {
> +        ovs_list_remove(&op->list);
> +    }
> +    LIST_FOR_EACH_SAFE (op, list, &ls_change->deleted_ports) {
> +        ovs_list_remove(&op->list);
> +        ovn_port_destroy_orphan(op);
> +    }
> +}
> +
>  void
>  destroy_northd_data_tracked_changes(struct northd_data *nd)
>  {
>      struct ls_change *ls_change;
>      LIST_FOR_EACH_SAFE (ls_change, list_node,
>                          &nd->tracked_ls_changes.updated) {
> -        struct ovn_port *op;
> -        LIST_FOR_EACH (op, list, &ls_change->added_ports) {
> -            ovs_list_remove(&op->list);
> -        }
> -        LIST_FOR_EACH (op, list, &ls_change->updated_ports) {
> -            ovs_list_remove(&op->list);
> -        }
> -        LIST_FOR_EACH_SAFE (op, list, &ls_change->deleted_ports) {
> -            ovs_list_remove(&op->list);
> -            ovn_port_destroy_orphan(op);
> -        }
> +        destroy_tracked_ls_change(ls_change);
>          ovs_list_remove(&ls_change->list_node);
>          free(ls_change);
>      }
> @@ -5024,15 +5030,21 @@ ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct hmap *ls_ports,
>      return op;
>  }
>
> +/* Returns true if the logical switch has changes which can be
> + * incrementally handled.
> + * Presently supports i-p for the below changes:
> + *    - logical switch ports.
> + */
>  static bool
> -check_ls_changes_other_than_lsp(const struct nbrec_logical_switch *ls)
> +ls_changes_can_be_handled(
> +    const struct nbrec_logical_switch *ls)
>  {
>      /* Check if the columns are changed in this row. */
>      enum nbrec_logical_switch_column_id col;
>      for (col = 0; col < NBREC_LOGICAL_SWITCH_N_COLUMNS; col++) {
>          if (nbrec_logical_switch_is_updated(ls, col) &&
>              col != NBREC_LOGICAL_SWITCH_COL_PORTS) {
> -            return true;
> +            return false;
>          }
>      }
>
> @@ -5041,44 +5053,44 @@ check_ls_changes_other_than_lsp(const struct nbrec_logical_switch *ls)
>      for (size_t i = 0; i < ls->n_acls; i++) {
>          if (nbrec_acl_row_get_seqno(ls->acls[i],
>                                  OVSDB_IDL_CHANGE_MODIFY) > 0) {
> -            return true;
> +            return false;
>          }
>      }
>      if (ls->copp && nbrec_copp_row_get_seqno(ls->copp,
>                                  OVSDB_IDL_CHANGE_MODIFY) > 0) {
> -        return true;
> +        return false;
>      }
>      for (size_t i = 0; i < ls->n_dns_records; i++) {
>          if (nbrec_dns_row_get_seqno(ls->dns_records[i],
>                                  OVSDB_IDL_CHANGE_MODIFY) > 0) {
> -            return true;
> +            return false;
>          }
>      }
>      for (size_t i = 0; i < ls->n_forwarding_groups; i++) {
>          if (nbrec_forwarding_group_row_get_seqno(ls->forwarding_groups[i],
>                                  OVSDB_IDL_CHANGE_MODIFY) > 0) {
> -            return true;
> +            return false;
>          }
>      }
>      for (size_t i = 0; i < ls->n_load_balancer; i++) {
>          if (nbrec_load_balancer_row_get_seqno(ls->load_balancer[i],
>                                  OVSDB_IDL_CHANGE_MODIFY) > 0) {
> -            return true;
> +            return false;
>          }
>      }
>      for (size_t i = 0; i < ls->n_load_balancer_group; i++) {
>          if (nbrec_load_balancer_group_row_get_seqno(ls->load_balancer_group[i],
>                                  OVSDB_IDL_CHANGE_MODIFY) > 0) {
> -            return true;
> +            return false;
>          }
>      }
>      for (size_t i = 0; i < ls->n_qos_rules; i++) {
>          if (nbrec_qos_row_get_seqno(ls->qos_rules[i],
>                                  OVSDB_IDL_CHANGE_MODIFY) > 0) {
> -            return true;
> +            return false;
>          }
>      }
> -    return false;
> +    return true;
>  }
>
>  static bool
> @@ -5119,6 +5131,172 @@ check_lsp_changes_other_than_up(const struct nbrec_logical_switch_port *nbsp)
>      return false;
>  }
>
> +/* Handles logical switch port changes of a changed logical switch.
> + * Returns false, if any logical port can't be incrementally handled.
> + */
> +static bool
> +ls_handle_lsp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn,
> +                                const struct nbrec_logical_switch *changed_ls,
> +                                const struct northd_input *ni,
> +                                struct northd_data *nd,
> +                                struct ovn_datapath *od,
> +                                struct ls_change *ls_change,
> +                                bool *updated)
> +{
> +    bool ls_ports_changed = false;
> +    if (!nbrec_logical_switch_is_updated(changed_ls,
> +                                         NBREC_LOGICAL_SWITCH_COL_PORTS)) {
> +
> +        for (size_t i = 0; i < changed_ls->n_ports; i++) {
> +            if (nbrec_logical_switch_port_row_get_seqno(
> +                changed_ls->ports[i], OVSDB_IDL_CHANGE_MODIFY) > 0) {
> +                ls_ports_changed = true;
> +                break;
> +            }
> +        }
> +    } else {
> +        ls_ports_changed = true;
> +    }
> +
> +    if (!ls_ports_changed) {
> +        return true;
> +    }
> +
> +    /* Check if the logical switch has only router ports before this change.
> +     * If so, fall back to recompute.
> +     * lflow engine node while building the lflows checks if the logical switch
> +     * has any router ports and depending on that it adds different flows.
> +     * See build_lswitch_rport_arp_req_flow() for more details.
> +     * Note: We can definitely handle this scenario incrementally in the
> +     * northd engine node and fall back to recompute in lflow engine node
> +     * and even handle this incrementally in lflow node.  Until we do that
> +     * resort to full recompute of northd node.
> +     */
> +    bool only_rports = (od->n_router_ports
> +                        && (od->n_router_ports == hmap_count(&od->ports)));
> +    if (only_rports) {
> +        return false;
> +    }
> +
> +    struct ovn_port *op;
> +    HMAP_FOR_EACH (op, dp_node, &od->ports) {
> +        op->visited = false;
> +    }
> +
> +    /* Compare the individual ports in the old and new Logical Switches */
> +    for (size_t j = 0; j < changed_ls->n_ports; ++j) {
> +        struct nbrec_logical_switch_port *new_nbsp = changed_ls->ports[j];
> +        op = ovn_port_find_in_datapath(od, new_nbsp->name);
> +
> +        if (!op) {
> +            if (!lsp_can_be_inc_processed(new_nbsp)) {
> +                goto fail;
> +            }
> +            op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
> +                                new_nbsp->name, new_nbsp, od, NULL, NULL,
> +                                ni->sbrec_mirror_table,
> +                                ni->sbrec_chassis_table,
> +                                ni->sbrec_chassis_by_name,
> +                                ni->sbrec_chassis_by_hostname);
> +            if (!op) {
> +                goto fail;
> +            }
> +            ovs_list_push_back(&ls_change->added_ports,
> +                                &op->list);
> +        } else if (ls_port_has_changed(op->nbsp, new_nbsp)) {
> +            /* Existing port updated */
> +            bool temp = false;
> +            if (lsp_is_type_changed(op->sb, new_nbsp, &temp) ||
> +                !op->lsp_can_be_inc_processed ||
> +                !lsp_can_be_inc_processed(new_nbsp)) {
> +                goto fail;
> +            }
> +            const struct sbrec_port_binding *sb = op->sb;
> +            if (sset_contains(&nd->svc_monitor_lsps, new_nbsp->name)) {
> +                /* This port is used for svc monitor, which may be impacted
> +                 * by this change. Fallback to recompute. */
> +                goto fail;
> +            }
> +            if (!check_lsp_is_up &&
> +                !check_lsp_changes_other_than_up(new_nbsp)) {
> +                /* If the only change is the "up" column while the
> +                 * "ignore_lsp_down" is set to true, just ignore this
> +                 * change. */
> +                op->visited = true;
> +                continue;
> +            }
> +            struct ovs_list lflows = OVS_LIST_INITIALIZER(&lflows);
> +            ovs_list_splice(&lflows, op->lflows.next, &op->lflows);
> +            ovn_port_destroy(&nd->ls_ports, op);
> +            op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
> +                                new_nbsp->name, new_nbsp, od, sb, &lflows,
> +                                ni->sbrec_mirror_table,
> +                                ni->sbrec_chassis_table,
> +                                ni->sbrec_chassis_by_name,
> +                                ni->sbrec_chassis_by_hostname);
> +            ovs_assert(ovs_list_is_empty(&lflows));
> +            if (!op) {
> +                goto fail;
> +            }
> +            ovs_list_push_back(&ls_change->updated_ports, &op->list);
> +        }
> +        op->visited = true;
> +    }
> +
> +    /* Check for deleted ports */
> +    HMAP_FOR_EACH_SAFE (op, dp_node, &od->ports) {
> +        if (!op->visited) {
> +            if (!op->lsp_can_be_inc_processed) {
> +                goto fail_clean_deleted;
> +            }
> +            if (sset_contains(&nd->svc_monitor_lsps, op->key)) {
> +                /* This port was used for svc monitor, which may be
> +                 * impacted by this deletion. Fallback to recompute. */
> +                goto fail_clean_deleted;
> +            }
> +            ovs_list_push_back(&ls_change->deleted_ports,
> +                                &op->list);
> +            hmap_remove(&nd->ls_ports, &op->key_node);
> +            hmap_remove(&od->ports, &op->dp_node);
> +            sbrec_port_binding_delete(op->sb);
> +            delete_fdb_entry(ni->sbrec_fdb_by_dp_and_port, od->tunnel_key,
> +                                op->tunnel_key);
> +        }
> +    }
> +
> +    /* Check if the logical switch has only router ports after this change.
> +     * If so, fall back to recompute.
> +     * lflow engine node while building the lflows checks if the logical switch
> +     * has any router ports and depending on that it adds different flows.
> +     * See build_lswitch_rport_arp_req_flow() for more details.
> +     * Note: We can definitely handle this scenario incrementally in the
> +     * northd engine node and fall back to recompute in lflow engine node
> +     * and even handle this incrementally in lflow node.  Until we do that
> +     * resort to full recompute of northd node.
> +     */
> +    only_rports = (od->n_router_ports
> +                    && (od->n_router_ports == hmap_count(&od->ports)));
> +    if (only_rports) {
> +        goto fail_clean_deleted;
> +    }
> +
> +    if (!ovs_list_is_empty(&ls_change->added_ports) ||
> +        !ovs_list_is_empty(&ls_change->updated_ports) ||
> +        !ovs_list_is_empty(&ls_change->deleted_ports)) {
> +        *updated = true;
> +    }
> +
> +    return true;
> +
> +fail_clean_deleted:
> +    LIST_FOR_EACH_POP (op, list, &ls_change->deleted_ports) {
> +        ovn_port_destroy_orphan(op);
> +    }
> +
> +fail:
> +    return false;
> +}
> +
>  /* 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,
> @@ -5130,11 +5308,9 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn,
>  {
>      const struct nbrec_logical_switch *changed_ls;
>      struct ls_change *ls_change = NULL;
> -    struct ovn_port *op;
>
>      NBREC_LOGICAL_SWITCH_TABLE_FOR_EACH_TRACKED (changed_ls,
>                                               ni->nbrec_logical_switch_table) {
> -        ls_change = NULL;
>          if (nbrec_logical_switch_is_new(changed_ls) ||
>              nbrec_logical_switch_is_deleted(changed_ls)) {
>              goto fail;
> @@ -5150,8 +5326,8 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn,
>              goto fail;
>          }
>
> -        /* Now only able to handle lsp changes. */
> -        if (check_ls_changes_other_than_lsp(changed_ls)) {
> +        /* Check if the ls changes can be handled or not. */
> +        if (!ls_changes_can_be_handled(changed_ls)) {
>              goto fail;
>          }
>
> @@ -5161,126 +5337,16 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn,
>          ovs_list_init(&ls_change->deleted_ports);
>          ovs_list_init(&ls_change->updated_ports);
>
> -        HMAP_FOR_EACH (op, dp_node, &od->ports) {
> -            op->visited = false;
> -        }
> -
> -        /* Check if the logical switch has only router ports before this change.
> -         * If so, fall back to recompute.
> -         * lflow engine node while building the lflows checks if the logical switch
> -         * has any router ports and depending on that it adds different flows.
> -         * See build_lswitch_rport_arp_req_flow() for more details.
> -         * Note: We can definitely handle this scenario incrementally in the
> -         * northd engine node and fall back to recompute in lflow engine node
> -         * and even handle this incrementally in lflow node.  Until we do that
> -         * resort to full recompute of northd node.
> -         */
> -        bool only_rports = (od->n_router_ports
> -                            && (od->n_router_ports == hmap_count(&od->ports)));
> -        if (only_rports) {
> +        bool updated = false;
> +        if (!ls_handle_lsp_changes(ovnsb_idl_txn, changed_ls,
> +                                   ni, nd, od, ls_change,
> +                                   &updated)) {
> +            destroy_tracked_ls_change(ls_change);
> +            free(ls_change);
>              goto fail;
>          }
>
> -        /* Compare the individual ports in the old and new Logical Switches */
> -        for (size_t j = 0; j < changed_ls->n_ports; ++j) {
> -            struct nbrec_logical_switch_port *new_nbsp = changed_ls->ports[j];
> -            op = ovn_port_find_in_datapath(od, new_nbsp->name);
> -
> -            if (!op) {
> -                if (!lsp_can_be_inc_processed(new_nbsp)) {
> -                    goto fail;
> -                }
> -                op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
> -                                    new_nbsp->name, new_nbsp, od, NULL, NULL,
> -                                    ni->sbrec_mirror_table,
> -                                    ni->sbrec_chassis_table,
> -                                    ni->sbrec_chassis_by_name,
> -                                    ni->sbrec_chassis_by_hostname);
> -                if (!op) {
> -                    goto fail;
> -                }
> -                ovs_list_push_back(&ls_change->added_ports,
> -                                   &op->list);
> -            } else if (ls_port_has_changed(op->nbsp, new_nbsp)) {
> -                /* Existing port updated */
> -                bool temp = false;
> -                if (lsp_is_type_changed(op->sb, new_nbsp, &temp) ||
> -                    !op->lsp_can_be_inc_processed ||
> -                    !lsp_can_be_inc_processed(new_nbsp)) {
> -                    goto fail;
> -                }
> -                const struct sbrec_port_binding *sb = op->sb;
> -                if (sset_contains(&nd->svc_monitor_lsps, new_nbsp->name)) {
> -                    /* This port is used for svc monitor, which may be impacted
> -                     * by this change. Fallback to recompute. */
> -                    goto fail;
> -                }
> -                if (!check_lsp_is_up &&
> -                    !check_lsp_changes_other_than_up(new_nbsp)) {
> -                    /* If the only change is the "up" column while the
> -                     * "ignore_lsp_down" is set to true, just ignore this
> -                     * change. */
> -                    op->visited = true;
> -                    continue;
> -                }
> -                struct ovs_list lflows = OVS_LIST_INITIALIZER(&lflows);
> -                ovs_list_splice(&lflows, op->lflows.next, &op->lflows);
> -                ovn_port_destroy(&nd->ls_ports, op);
> -                op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
> -                                    new_nbsp->name, new_nbsp, od, sb, &lflows,
> -                                    ni->sbrec_mirror_table,
> -                                    ni->sbrec_chassis_table,
> -                                    ni->sbrec_chassis_by_name,
> -                                    ni->sbrec_chassis_by_hostname);
> -                ovs_assert(ovs_list_is_empty(&lflows));
> -                if (!op) {
> -                    goto fail;
> -                }
> -                ovs_list_push_back(&ls_change->updated_ports, &op->list);
> -            }
> -            op->visited = true;
> -        }
> -
> -        /* Check for deleted ports */
> -        HMAP_FOR_EACH_SAFE (op, dp_node, &od->ports) {
> -            if (!op->visited) {
> -                if (!op->lsp_can_be_inc_processed) {
> -                    goto fail_clean_deleted;
> -                }
> -                if (sset_contains(&nd->svc_monitor_lsps, op->key)) {
> -                    /* This port was used for svc monitor, which may be
> -                     * impacted by this deletion. Fallback to recompute. */
> -                    goto fail_clean_deleted;
> -                }
> -                ovs_list_push_back(&ls_change->deleted_ports,
> -                                   &op->list);
> -                hmap_remove(&nd->ls_ports, &op->key_node);
> -                hmap_remove(&od->ports, &op->dp_node);
> -                sbrec_port_binding_delete(op->sb);
> -                delete_fdb_entry(ni->sbrec_fdb_by_dp_and_port, od->tunnel_key,
> -                                 op->tunnel_key);
> -            }
> -        }
> -
> -        /* Check if the logical switch has only router ports after this change.
> -         * If so, fall back to recompute.
> -         * lflow engine node while building the lflows checks if the logical switch
> -         * has any router ports and depending on that it adds different flows.
> -         * See build_lswitch_rport_arp_req_flow() for more details.
> -         * Note: We can definitely handle this scenario incrementally in the
> -         * northd engine node and fall back to recompute in lflow engine node
> -         * and even handle this incrementally in lflow node.  Until we do that
> -         * resort to full recompute of northd node.
> -         */
> -        only_rports = (od->n_router_ports
> -                       && (od->n_router_ports == hmap_count(&od->ports)));
> -        if (only_rports) {
> -            goto fail_clean_deleted;
> -        }
> -
> -        if (!ovs_list_is_empty(&ls_change->added_ports) ||
> -            !ovs_list_is_empty(&ls_change->updated_ports) ||
> -            !ovs_list_is_empty(&ls_change->deleted_ports)) {
> +        if (updated) {
>              ovs_list_push_back(&nd->tracked_ls_changes.updated,
>                                 &ls_change->list_node);
>          } else {
> @@ -5293,13 +5359,7 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn,
>      }
>      return true;
>
> -fail_clean_deleted:
> -    LIST_FOR_EACH_POP (op, list, &ls_change->deleted_ports) {
> -        ovn_port_destroy_orphan(op);
> -    }
> -
>  fail:
> -    free(ls_change);
>      destroy_northd_data_tracked_changes(nd);
>      return false;
>  }
> --
> 2.40.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


Looks good to me, thanks.

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