[ovs-dev,v5,03/16] northd: Move router ports SB PB options sync to sync_to_sb_pb node.

From: Numan Siddique <numans@ovn.org>

It also moves the logical router port IPv6 prefix delegation
updates to "sync-from-sb" engine node.

With these changes, northd engine node doesn't need to do much
for SB Port binding changes other than updating 'op->sb'.  And
there is no need to fall back to recompute.  Prior to this patch
northd_handle_sb_port_binding_changes() was returning false for
all SB port binding updates except for the VIF PBs.  This patch
now handles updates to all the SB port binding by setting 'op->sb'.

Signed-off-by: Numan Siddique <numans@ovn.org>
---
 northd/en-northd.c  |   2 +-
 northd/en-sync-sb.c |   3 +-
 northd/northd.c     | 296 ++++++++++++++++++++++++++------------------
 northd/northd.h     |   6 +-
 tests/ovn-northd.at | 158 +++++++++++++++++++++--
 5 files changed, 334 insertions(+), 131 deletions(-)

On 1/11/24 16:28, numans@ovn.org wrote:
> From: Numan Siddique <numans@ovn.org>
> 
> It also moves the logical router port IPv6 prefix delegation
> updates to "sync-from-sb" engine node.
> 
> With these changes, northd engine node doesn't need to do much
> for SB Port binding changes other than updating 'op->sb'.  And
> there is no need to fall back to recompute.  Prior to this patch
> northd_handle_sb_port_binding_changes() was returning false for
> all SB port binding updates except for the VIF PBs.  This patch
> now handles updates to all the SB port binding by setting 'op->sb'.
> 
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---

Hi Numan,

With this patch applied the "633: ovn-northd.at:10298 SB Port binding
incremental processing -- parallelization=yes" test occasionally fails,
e.g.:

https://github.com/dceara/ovn/actions/runs/7539663594/job/20524238508

I saw it locally too when running the test in a loop:

It fails with:
ovn-nbctl lrp-set-gateway-chassis lrp hv1
./ovn-macros.at:449: "$@"
northd recompute count - 2
./ovn-northd.at:10238: test x$northd_recomp = x$1
./ovn-northd.at:10238: exit code was 1, expected 0

Here:
https://github.com/dceara/ovn/blob/e1c76b6d347570618591fe6587f6af83445223ec/tests/ovn-northd.at#L10327

For some reason in some cases only 2 recomputes get triggered.

>  northd/en-northd.c  |   2 +-
>  northd/en-sync-sb.c |   3 +-
>  northd/northd.c     | 296 ++++++++++++++++++++++++++------------------
>  northd/northd.h     |   6 +-
>  tests/ovn-northd.at | 158 +++++++++++++++++++++--
>  5 files changed, 334 insertions(+), 131 deletions(-)
> 
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index 28559ed211..677b2b1ab0 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -189,7 +189,7 @@ northd_sb_port_binding_handler(struct engine_node *node,
>      northd_get_input_data(node, &input_data);
>  
>      if (!northd_handle_sb_port_binding_changes(
> -        input_data.sbrec_port_binding_table, &nd->ls_ports)) {
> +        input_data.sbrec_port_binding_table, &nd->ls_ports, &nd->lr_ports)) {
>          return false;
>      }
>  
> diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
> index 3aaab8d005..45be7ddbcb 100644
> --- a/northd/en-sync-sb.c
> +++ b/northd/en-sync-sb.c
> @@ -288,7 +288,8 @@ en_sync_to_sb_pb_run(struct engine_node *node, void *data OVS_UNUSED)
>      const struct engine_context *eng_ctx = engine_get_context();
>      struct northd_data *northd_data = engine_get_input_data("northd", node);
>  
> -    sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports);
> +    sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports,
> +             &northd_data->lr_ports);
>      engine_set_node_state(node, EN_UPDATED);
>  }
>  
> diff --git a/northd/northd.c b/northd/northd.c
> index f32e3bf21a..23f2dae26b 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -3435,6 +3435,9 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
>  {
>      sbrec_port_binding_set_datapath(op->sb, op->od->sb);
>      if (op->nbrp) {
> +        /* Note: SB port binding options for router ports are set in
> +         * sync_pbs(). */
> +
>          /* If the router is for l3 gateway, it resides on a chassis
>           * and its port type is "l3gateway". */
>          const char *chassis_name = smap_get(&op->od->nbr->options, "chassis");
> @@ -3446,15 +3449,11 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
>              sbrec_port_binding_set_type(op->sb, "patch");
>          }
>  
> -        struct smap new;
> -        smap_init(&new);
>          if (is_cr_port(op)) {
>              ovs_assert(sbrec_chassis_by_name);
>              ovs_assert(sbrec_chassis_by_hostname);
>              ovs_assert(sbrec_ha_chassis_grp_by_name);
>              ovs_assert(active_ha_chassis_grps);
> -            const char *redirect_type = smap_get(&op->nbrp->options,
> -                                                 "redirect-type");
>  
>              if (op->nbrp->ha_chassis_group) {
>                  if (op->nbrp->n_gateway_chassis) {
> @@ -3496,49 +3495,8 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
>                  /* Delete the legacy gateway_chassis from the pb. */
>                  sbrec_port_binding_set_gateway_chassis(op->sb, NULL, 0);
>              }
> -            smap_add(&new, "distributed-port", op->nbrp->name);
> -
> -            bool always_redirect =
> -                !op->od->has_distributed_nat &&
> -                !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port);
> -
> -            if (redirect_type) {
> -                smap_add(&new, "redirect-type", redirect_type);
> -                /* XXX Why can't we enable always-redirect when redirect-type
> -                 * is bridged? */
> -                if (!strcmp(redirect_type, "bridged")) {
> -                    always_redirect = false;
> -                }
> -            }
> -
> -            if (always_redirect) {
> -                smap_add(&new, "always-redirect", "true");
> -            }
> -        } else {
> -            if (op->peer) {
> -                smap_add(&new, "peer", op->peer->key);
> -                if (op->nbrp->ha_chassis_group ||
> -                    op->nbrp->n_gateway_chassis) {
> -                    char *redirect_name =
> -                        ovn_chassis_redirect_name(op->nbrp->name);
> -                    smap_add(&new, "chassis-redirect-port", redirect_name);
> -                    free(redirect_name);
> -                }
> -            }
> -            if (chassis_name) {
> -                smap_add(&new, "l3gateway-chassis", chassis_name);
> -            }
>          }
>  
> -        const char *ipv6_pd_list = smap_get(&op->sb->options,
> -                                            "ipv6_ra_pd_list");
> -        if (ipv6_pd_list) {
> -            smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list);
> -        }
> -
> -        sbrec_port_binding_set_options(op->sb, &new);
> -        smap_destroy(&new);
> -
>          sbrec_port_binding_set_parent_port(op->sb, NULL);
>          sbrec_port_binding_set_tag(op->sb, NULL, 0);
>  
> @@ -4768,12 +4726,14 @@ check_sb_lb_duplicates(const struct sbrec_load_balancer_table *table)
>      return duplicates;
>  }
>  
> -/* Syncs the SB port binding for the ovn_port 'op'.  Caller should make sure
> - * that the OVN SB IDL txn is not NULL.  Presently it only syncs the nat
> - * column of port binding corresponding to the 'op->nbsp' */
> +/* Syncs the SB port binding for the ovn_port 'op' of a logical switch port.
> + * Caller should make sure that the OVN SB IDL txn is not NULL.  Presently it
> + * only syncs the nat column of port binding corresponding to the 'op->nbsp' */
>  static void
> -sync_pb_for_op(struct ovn_port *op)
> +sync_pb_for_lsp(struct ovn_port *op)
>  {
> +    ovs_assert(op->nbsp);
> +
>      if (lsp_is_router(op->nbsp)) {
>          const char *chassis = NULL;
>          if (op->peer && op->peer->od && op->peer->od->nbr) {
> @@ -4895,18 +4855,86 @@ sync_pb_for_op(struct ovn_port *op)
>      }
>  }
>  
> +/* Syncs the SB port binding for the ovn_port 'op' of a logical router port.
> + * Caller should make sure that the OVN SB IDL txn is not NULL.  Presently it
> + * only sets the port binding options column for the router ports */
> +static void
> +sync_pb_for_lrp(struct ovn_port *op)
> +{
> +    ovs_assert(op->nbrp);
> +
> +    struct smap new;
> +    smap_init(&new);
> +
> +    const char *chassis_name = smap_get(&op->od->nbr->options, "chassis");
> +    if (is_cr_port(op)) {
> +        smap_add(&new, "distributed-port", op->nbrp->name);
> +
> +        bool always_redirect =
> +            !op->od->has_distributed_nat &&
> +            !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port);
> +
> +        const char *redirect_type = smap_get(&op->nbrp->options,
> +                                            "redirect-type");
> +        if (redirect_type) {
> +            smap_add(&new, "redirect-type", redirect_type);
> +            /* XXX Why can't we enable always-redirect when redirect-type
> +                * is bridged? */

Nit: indentation

> +            if (!strcmp(redirect_type, "bridged")) {
> +                always_redirect = false;
> +            }
> +        }
> +
> +        if (always_redirect) {
> +            smap_add(&new, "always-redirect", "true");
> +        }
> +    } else {
> +        if (op->peer) {
> +            smap_add(&new, "peer", op->peer->key);
> +            if (op->nbrp->ha_chassis_group ||
> +                op->nbrp->n_gateway_chassis) {
> +                char *redirect_name =
> +                    ovn_chassis_redirect_name(op->nbrp->name);
> +                smap_add(&new, "chassis-redirect-port", redirect_name);
> +                free(redirect_name);
> +            }
> +        }
> +        if (chassis_name) {
> +            smap_add(&new, "l3gateway-chassis", chassis_name);
> +        }
> +    }
> +
> +    const char *ipv6_pd_list = smap_get(&op->sb->options, "ipv6_ra_pd_list");
> +    if (ipv6_pd_list) {
> +        smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list);
> +    }
> +
> +    sbrec_port_binding_set_options(op->sb, &new);
> +    smap_destroy(&new);
> +}
> +
> +static void ovn_update_ipv6_options(struct hmap *lr_ports);
> +static void ovn_update_ipv6_opt_for_op(struct ovn_port *op);
> +
>  /* Sync the SB Port bindings which needs to be updated.
>   * Presently it syncs the nat column of port bindings corresponding to
>   * the logical switch ports. */
>  void
> -sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports)
> +sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports,
> +         struct hmap *lr_ports)
>  {
>      ovs_assert(ovnsb_idl_txn);
>  
>      struct ovn_port *op;
>      HMAP_FOR_EACH (op, key_node, ls_ports) {
> -        sync_pb_for_op(op);
> +        sync_pb_for_lsp(op);
> +    }
> +
> +    HMAP_FOR_EACH (op, key_node, lr_ports) {
> +        sync_pb_for_lrp(op);
>      }
> +
> +    ovn_update_ipv6_options(lr_ports);
>  }
>  
>  /* Sync the SB Port bindings for the added and updated logical switch ports
> @@ -4918,12 +4946,14 @@ sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports *trk_ovn_ports)
>      struct ovn_port *op;
>      HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->created) {
>          op = hmapx_node->data;
> -        sync_pb_for_op(op);
> +        ovs_assert(op->nbsp);

This is already checked inside sync_pb_for_lsp().  Let's remove it from
here.  Then what I mentioned in patch 01/16 still applies, we don't need
the local 'op' variable anymore.

> +        sync_pb_for_lsp(op);
>      }
>  
>      HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->updated) {
>          op = hmapx_node->data;
> -        sync_pb_for_op(op);
> +        ovs_assert(op->nbsp);
> +        sync_pb_for_lsp(op);

Same here.

>      }
>  
>      return true;
> @@ -5671,20 +5701,31 @@ fail:
>  bool
>  northd_handle_sb_port_binding_changes(
>      const struct sbrec_port_binding_table *sbrec_port_binding_table,
> -    struct hmap *ls_ports)
> +    struct hmap *ls_ports, struct hmap *lr_ports)
>  {
>      const struct sbrec_port_binding *pb;
>      static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>      SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, sbrec_port_binding_table) {
> -        /* Return false if the 'pb' belongs to a router port.  We don't handle
> -         * I-P for router ports yet. */
> -        if (is_pb_router_type(pb)) {
> -            return false;
> +        bool is_router_port = is_pb_router_type(pb);
> +        struct ovn_port *op = NULL;
> +
> +        if (is_router_port) {
> +            /* A router port binding 'pb' can belong to
> +             *   - a logical switch port of type router or
> +             *   - a logical router port.
> +             *
> +             * So, first search in lr_ports hmap.  If not found, search in
> +             * ls_ports hmap.
> +             * */
> +            op = ovn_port_find(lr_ports, pb->logical_port);
>          }
>  
> -        struct ovn_port *op = ovn_port_find(ls_ports, pb->logical_port);
> -        if (op && !op->lsp_can_be_inc_processed) {
> -            return false;
> +        if (!op) {
> +            op = ovn_port_find(ls_ports, pb->logical_port);
> +
> +            if (op) {
> +                is_router_port = false;
> +            }
>          }
>  
>          if (sbrec_port_binding_is_new(pb)) {
> @@ -5693,7 +5734,8 @@ northd_handle_sb_port_binding_changes(
>               * pointer in northd data. Fallback to recompute otherwise. */
>              if (!op) {
>                  VLOG_WARN_RL(&rl, "A port-binding for %s is created but the "
> -                            "LSP is not found.", pb->logical_port);
> +                            "%s is not found.", pb->logical_port,
> +                            is_router_port ? "LRP" : "LSP");
>                  return false;
>              }
>              op->sb = pb;
> @@ -5703,18 +5745,29 @@ northd_handle_sb_port_binding_changes(
>               * case. Fallback to recompute otherwise, to avoid dangling
>               * sb idl pointers and other unexpected behavior. */
>              if (op && op->sb == pb) {
> -                VLOG_WARN_RL(&rl, "A port-binding for %s is deleted but "
> -                            "the LSP still exists.", pb->logical_port);
> +                VLOG_WARN_RL(&rl, "A port-binding for %s is deleted but the "
> +                            "LSP/LRP still exists.", pb->logical_port);
>                  return false;
>              }
>          } else {
> -            /* The PB is updated, most likely because of binding/unbinding
> -             * to/from a chassis, and we can ignore the change (updating NB
> -             * "up" will be handled in the engine node "sync_from_sb").
> +            /* The PB is updated.
> +             * For an LSP PB it is most likely because of
> +             * binding/unbinding to/from a chassis, and we can ignore the
> +             * change (updating NB "up" will be handled in the engine node
> +             * "sync_from_sb").
> +             *
> +             * For an LRP PB, it is most likely because of
> +             *   - IPv6 prefix delagation updates from ovn-controller.
> +             *     This update is handled in "sync_from_sb" node.
> +             *   - ha chassis group and this can be ignored.
> +             *
> +             * All other changes can be ignored.
> +             *
>               * Fallback to recompute for anything unexpected. */
>              if (!op) {
>                  VLOG_WARN_RL(&rl, "A port-binding for %s is updated but the "
> -                            "LSP is not found.", pb->logical_port);
> +                            "%s is not found.", pb->logical_port,
> +                            is_router_port ? "LRP" : "LSP");
>                  return false;
>              }
>              if (op->sb != pb) {
> @@ -7855,67 +7908,70 @@ static void
>  copy_ra_to_sb(struct ovn_port *op, const char *address_mode);
>  
>  static void
> -ovn_update_ipv6_options(struct hmap *lr_ports)
> +ovn_update_ipv6_opt_for_op(struct ovn_port *op)
>  {
> -    struct ovn_port *op;
> -    HMAP_FOR_EACH (op, key_node, lr_ports) {
> -        ovs_assert(op->nbrp);
> -
> -        if (op->nbrp->peer || !op->peer) {
> -            continue;
> -        }
> +    if (op->nbrp->peer || !op->peer) {
> +        return;
> +    }
>  
> -        if (!op->lrp_networks.n_ipv6_addrs) {
> -            continue;
> -        }
> +    if (!op->lrp_networks.n_ipv6_addrs) {
> +        return;
> +    }
>  
> -        struct smap options;
> -        smap_clone(&options, &op->sb->options);
> +    struct smap options;
> +    smap_clone(&options, &op->sb->options);
>  
> -        /* enable IPv6 prefix delegation */
> -        bool prefix_delegation = smap_get_bool(&op->nbrp->options,
> +    /* enable IPv6 prefix delegation */
> +    bool prefix_delegation = smap_get_bool(&op->nbrp->options,
>                                             "prefix_delegation", false);
> -        if (!lrport_is_enabled(op->nbrp)) {
> -            prefix_delegation = false;
> -        }
> -        if (smap_get_bool(&options, "ipv6_prefix_delegation",
> -                          false) != prefix_delegation) {
> -            smap_add(&options, "ipv6_prefix_delegation",
> -                     prefix_delegation ? "true" : "false");
> -        }
> +    if (!lrport_is_enabled(op->nbrp)) {
> +        prefix_delegation = false;
> +    }
> +    if (smap_get_bool(&options, "ipv6_prefix_delegation",
> +                      false) != prefix_delegation) {
> +        smap_add(&options, "ipv6_prefix_delegation",
> +                 prefix_delegation ? "true" : "false");
> +    }
>  
> -        bool ipv6_prefix = smap_get_bool(&op->nbrp->options,
> +    bool ipv6_prefix = smap_get_bool(&op->nbrp->options,
>                                       "prefix", false);

Nit: this fits on a single line now.

> -        if (!lrport_is_enabled(op->nbrp)) {
> -            ipv6_prefix = false;
> -        }
> -        if (smap_get_bool(&options, "ipv6_prefix", false) != ipv6_prefix) {
> -            smap_add(&options, "ipv6_prefix",
> -                     ipv6_prefix ? "true" : "false");
> -        }
> -        sbrec_port_binding_set_options(op->sb, &options);
> +    if (!lrport_is_enabled(op->nbrp)) {
> +        ipv6_prefix = false;
> +    }
> +    if (smap_get_bool(&options, "ipv6_prefix", false) != ipv6_prefix) {
> +        smap_add(&options, "ipv6_prefix", ipv6_prefix ? "true" : "false");
> +    }
> +    sbrec_port_binding_set_options(op->sb, &options);
>  
> -        smap_destroy(&options);
> +    smap_destroy(&options);
>  
> -        const char *address_mode = smap_get(
> -            &op->nbrp->ipv6_ra_configs, "address_mode");
> +    const char *address_mode = smap_get(&op->nbrp->ipv6_ra_configs,
> +                                        "address_mode");
>  
> -        if (!address_mode) {
> -            continue;
> -        }
> -        if (strcmp(address_mode, "slaac") &&
> -            strcmp(address_mode, "dhcpv6_stateful") &&
> -            strcmp(address_mode, "dhcpv6_stateless")) {
> -            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> -            VLOG_WARN_RL(&rl, "Invalid address mode [%s] defined",
> -                         address_mode);
> -            continue;
> -        }
> +    if (!address_mode) {
> +        return;
> +    }
> +    if (strcmp(address_mode, "slaac") &&
> +        strcmp(address_mode, "dhcpv6_stateful") &&
> +        strcmp(address_mode, "dhcpv6_stateless")) {
> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> +        VLOG_WARN_RL(&rl, "Invalid address mode [%s] defined",
> +                     address_mode);
> +        return;
> +    }
>  
> -        if (smap_get_bool(&op->nbrp->ipv6_ra_configs, "send_periodic",
> -                          false)) {
> -            copy_ra_to_sb(op, address_mode);
> -        }
> +    if (smap_get_bool(&op->nbrp->ipv6_ra_configs, "send_periodic", false)) {
> +        copy_ra_to_sb(op, address_mode);
> +    }
> +}
> +
> +static void
> +ovn_update_ipv6_options(struct hmap *lr_ports)
> +{
> +    struct ovn_port *op;
> +    HMAP_FOR_EACH (op, key_node, lr_ports) {
> +        ovs_assert(op->nbrp);

I would move this assert inside ovn_update_ipv6_opt_for_op() just in
case we ever want to call it in multiple places in the future.

> +        ovn_update_ipv6_opt_for_op(op);
>      }
>  }
>  
> @@ -18007,8 +18063,6 @@ ovnnb_db_run(struct northd_input *input_data,
>          &data->lr_ports);
>      stopwatch_stop(BUILD_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
>      stopwatch_start(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
> -    ovn_update_ipv6_options(&data->lr_ports);
> -    ovn_update_ipv6_prefix(&data->lr_ports);
>  
>      sync_mirrors(ovnsb_txn, input_data->nbrec_mirror_table,
>                   input_data->sbrec_mirror_table);
> @@ -18339,6 +18393,8 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
>                                         &ha_ref_chassis_map);
>      }
>      shash_destroy(&ha_ref_chassis_map);
> +
> +    ovn_update_ipv6_prefix(lr_ports);
>  }
>  
>  const char *
> diff --git a/northd/northd.h b/northd/northd.h
> index 23521065e8..233dca8084 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -364,7 +364,8 @@ bool lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn,
>                                        struct lflow_input *,
>                                        struct hmap *lflows);
>  bool northd_handle_sb_port_binding_changes(
> -    const struct sbrec_port_binding_table *, struct hmap *ls_ports);
> +    const struct sbrec_port_binding_table *, struct hmap *ls_ports,
> +    struct hmap *lr_ports);
>  
>  struct tracked_lb_data;
>  bool northd_handle_lb_data_changes(struct tracked_lb_data *,
> @@ -394,7 +395,8 @@ void sync_lbs(struct ovsdb_idl_txn *, const struct sbrec_load_balancer_table *,
>                struct chassis_features *chassis_features);
>  bool check_sb_lb_duplicates(const struct sbrec_load_balancer_table *);
>  
> -void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports);
> +void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports,
> +              struct hmap *lr_ports);
>  bool sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports *);
>  
>  static inline bool
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 4a02dacf60..4edad24e53 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -10073,7 +10073,7 @@ check ovn-nbctl --wait=hv lsp-add ls0 lsp0-0 -- lsp-set-addresses lsp0-0 "unknow
>  ovs-vsctl add-port br-int lsp0-0 -- set interface lsp0-0 external_ids:iface-id=lsp0-0
>  wait_for_ports_up
>  check ovn-nbctl --wait=hv sync
> -check_recompute_counter 4 5 5 5 5 5
> +check_recompute_counter 3 4 3 4 3 4
>  
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=hv lsp-add ls0 lsp0-1 -- lsp-set-addresses lsp0-1 "aa:aa:aa:00:00:01 192.168.0.11"
> @@ -10235,6 +10235,126 @@ OVN_CLEANUP([hv1])
>  AT_CLEANUP
>  ])
>  
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([SB Port binding incremental processing])
> +ovn_start
> +
> +check_recompute_counter() {
> +    northd_recomp=$(as northd ovn-appctl -t ovn-northd inc-engine/show-stats northd recompute)
> +    echo "northd recompute count - $northd_recomp"
> +    AT_CHECK([test x$northd_recomp = x$1])
> +
> +    lflow_recomp=$(as northd ovn-appctl -t ovn-northd inc-engine/show-stats lflow recompute)
> +    echo "lflow recompute count - $lflow_recomp"
> +    AT_CHECK([test x$lflow_recomp = x$2])
> +}
> +
> +net_add n1
> +sim_add hv1
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.11
> +
> +check ovn-nbctl --wait=sb ls-add ls0
> +check ovn-nbctl --wait=sb lr-add lr0
> +
> +# Test normal VIF port
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lsp-add ls0 p1
> +check_recompute_counter 0 0
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lsp-set-addresses p1 "00:00:00:00:01:01 10.0.0.3"
> +check_recompute_counter 0 0
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +check as northd ovn-appctl -t ovn-northd vlog/set info
> +
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-sbctl lsp-bind p1 hv1
> +check_recompute_counter 0 0
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +# Test lsp of type router
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lsp-add ls0 rp -- lsp-set-type rp router
> +
> +# northd engine recomputes twice. Both the times for handling NB logical switch port
> +# changes and not because of SB port binding changes.  This is because ovn-northd
> +# sets the "up" to true.
> +check_recompute_counter 2 2
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +# Set some options to 'rp'.  northd should only recompute once.
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lsp-set-options rp foo=bar
> +check_recompute_counter 1 1
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +# Test lsp of type external
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lsp-add ls0 e1 -- lsp-set-type e1 external
> +check_recompute_counter 2 2
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +# Set some options to 'e1'.  northd should only recompute once.
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lsp-set-options e1 foo=bar
> +check_recompute_counter 1 1
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +# Test lrp
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lrp-add lr0 lrp 00:00:02:01:02:03 10.0.0.1/24
> +check_recompute_counter 1 1
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +# Set some options on 'lrp'.  northd should only recompute once.
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lrp-set-options lrp route_table=rtb-1
> +check_recompute_counter 1 1
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +# Make lrp a gateway port
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lrp-set-gateway-chassis lrp hv1
> +# There will be 3 recomputes of northd engine node
> +#   1. missing handler for input NB_logical_router
> +#   2. missing handler for input SB_ha_chassis_group
> +#   3. missing handler for input NB_logical_router when ovn-northd
> +#      updates the hosting-chassis option in NB_logical_router_port.
> +check_recompute_counter 3 3
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl clear logical_router_port lrp gateway_chassis
> +# There will be 2 recomputes of northd engine node
> +#   1. missing handler for input NB_logical_router
> +#   2. missing handler for input SB_ha_chassis_group
> +check_recompute_counter 2 2
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +# Delete some of the ports.
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lsp-del p1
> +check_recompute_counter 0 0
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lsp-del e1
> +check_recompute_counter 1 1
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lrp-del lrp
> +check_recompute_counter 1 1
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
> +
>  OVN_FOR_EACH_NORTHD_NO_HV([
>  AT_SETUP([ACL/Meter incremental processing - no northd recompute])
>  ovn_start
> @@ -10269,6 +10389,7 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE(1)
>  AT_CLEANUP
>  ])
>  
> +
>  OVN_FOR_EACH_NORTHD_NO_HV([
>  AT_SETUP([check fip and lb flows])
>  AT_KEYWORDS([fip-lb-flows])
> @@ -10471,7 +10592,7 @@ ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  ovn-nbctl --wait=sb lsp-set-options sw0-lr0 router-port=lr0-sw0
>  check_engine_stats lb_data norecompute compute
> -check_engine_stats northd recompute nocompute
> +check_engine_stats northd recompute compute
>  check_engine_stats lflow recompute nocompute
>  check_engine_stats sync_to_sb_lb recompute nocompute
>  
> @@ -11114,12 +11235,14 @@ ovn_attach n1 br-phys 192.168.0.11
>  ovn-sbctl chassis-add gw1 geneve 127.0.0.1
>  
>  check ovn-nbctl ls-add sw0
> -check ovn-nbctl lsp-add sw0 sw0p1 -- lsp-set-addresses sw0p1 "00:00:20:20:12:01 10.0.0.4"
> +check ovn-nbctl --wait=sb lsp-add sw0 sw0p1 -- lsp-set-addresses sw0p1 "00:00:20:20:12:01 10.0.0.4"
>  
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb lr-add lr0
>  check_engine_stats northd recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>  
>  # Adding a logical router port should result in recompute
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> @@ -11127,16 +11250,20 @@ check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24
>  # for northd engine there will be both recompute and compute
>  # first it will be recompute to handle lr0-sw0 and then a compute
>  # for the SB port binding change.
> -check_engine_stats northd recompute nocompute
> +check_engine_stats northd recompute compute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>  
>  ovn-nbctl lsp-add sw0 sw0-lr0
>  ovn-nbctl lsp-set-type sw0-lr0 router
>  ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb lsp-set-options sw0-lr0 router-port=lr0-sw0
> -check_engine_stats northd recompute nocompute
> +check_engine_stats northd recompute compute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>  
>  ovn-nbctl ls-add public
>  ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24
> @@ -11158,7 +11285,9 @@ ovn-nbctl --wait=hv lrp-set-gateway-chassis lr0-public hv1 20
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl set logical_router_port lr0-sw0 options:foo=bar
>  check_engine_stats northd recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>  
>  # Do checks for NATs.
>  # Add a NAT. This should not result in recompute of both northd and lflow
> @@ -11167,6 +11296,7 @@ check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat  172.168.0.110 10.0.0.4
>  check_engine_stats northd recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>  
>  # Update the NAT options column
> @@ -11174,6 +11304,7 @@ check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb set NAT . options:foo=bar
>  check_engine_stats northd recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>  
>  # Update the NAT external_ip column
> @@ -11181,6 +11312,7 @@ check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb set NAT . external_ip=172.168.0.120
>  check_engine_stats northd recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>  
>  # Update the NAT logical_ip column
> @@ -11188,6 +11320,7 @@ check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb set NAT . logical_ip=10.0.0.10
>  check_engine_stats northd recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>  
>  # Update the NAT type
> @@ -11195,13 +11328,15 @@ check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb set NAT . type=snat
>  check_engine_stats northd recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>  
>  # Create a dnat_and_snat NAT with external_mac and logical_port
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.110 10.0.0.4 sw0p1 30:54:00:00:00:03
> -check_engine_stats northd recompute nocompute
> +check_engine_stats northd recompute compute
>  check_engine_stats lflow recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>  
>  nat2_uuid=$(ovn-nbctl --bare --columns _uuid find nat logical_ip=10.0.0.4)
> @@ -11210,6 +11345,7 @@ check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb set NAT $nat2_uuid external_mac='"30:54:00:00:00:04"'
>  check_engine_stats northd recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>  
>  # Create a load balancer and add the lb vip as NAT
> @@ -11223,31 +11359,35 @@ check ovn-nbctl lr-lb-add lr0 lb2
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.140 10.0.0.20
>  check_engine_stats northd recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  check_engine_stats lflow recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>  
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.150 10.0.0.41
>  check_engine_stats northd recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  check_engine_stats lflow recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>  
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb lr-nat-del lr0 dnat_and_snat 172.168.0.150
>  check_engine_stats northd recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  check_engine_stats lflow recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>  
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb lr-nat-del lr0 dnat_and_snat 172.168.0.140
>  check_engine_stats northd recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  check_engine_stats lflow recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>  
>  # Delete the NAT
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb clear logical_router lr0 nat
> -check_engine_stats northd recompute nocompute
> +check_engine_stats northd recompute compute
>  check_engine_stats lflow recompute nocompute
>  check_engine_stats sync_to_sb_pb recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> @@ -11256,12 +11396,16 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb lr-policy-add lr0  10 "ip4.src == 10.0.0.3" reroute 172.168.0.101,172.168.0.102
>  check_engine_stats northd recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>  
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb lr-policy-del lr0  10 "ip4.src == 10.0.0.3"
>  check_engine_stats northd recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>  
>  OVN_CLEANUP([hv1])
>  AT_CLEANUP

The rest looks good to me but I'd prefer we figure out the test failure
before acking the patch.

Regards,
Dumitru

On Thu, Jan 11, 2024 at 7:29 AM <numans@ovn.org> wrote:
>
> From: Numan Siddique <numans@ovn.org>
>
> It also moves the logical router port IPv6 prefix delegation
> updates to "sync-from-sb" engine node.
>
> With these changes, northd engine node doesn't need to do much
> for SB Port binding changes other than updating 'op->sb'.  And
> there is no need to fall back to recompute.  Prior to this patch
> northd_handle_sb_port_binding_changes() was returning false for
> all SB port binding updates except for the VIF PBs.  This patch
> now handles updates to all the SB port binding by setting 'op->sb'.
>
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>  northd/en-northd.c  |   2 +-
>  northd/en-sync-sb.c |   3 +-
>  northd/northd.c     | 296 ++++++++++++++++++++++++++------------------
>  northd/northd.h     |   6 +-
>  tests/ovn-northd.at | 158 +++++++++++++++++++++--
>  5 files changed, 334 insertions(+), 131 deletions(-)
>
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index 28559ed211..677b2b1ab0 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -189,7 +189,7 @@ northd_sb_port_binding_handler(struct engine_node
*node,
>      northd_get_input_data(node, &input_data);
>
>      if (!northd_handle_sb_port_binding_changes(
> -        input_data.sbrec_port_binding_table, &nd->ls_ports)) {
> +        input_data.sbrec_port_binding_table, &nd->ls_ports,
&nd->lr_ports)) {
>          return false;
>      }
>
> diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
> index 3aaab8d005..45be7ddbcb 100644
> --- a/northd/en-sync-sb.c
> +++ b/northd/en-sync-sb.c
> @@ -288,7 +288,8 @@ en_sync_to_sb_pb_run(struct engine_node *node, void
*data OVS_UNUSED)
>      const struct engine_context *eng_ctx = engine_get_context();
>      struct northd_data *northd_data = engine_get_input_data("northd",
node);
>
> -    sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports);
> +    sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports,
> +             &northd_data->lr_ports);
>      engine_set_node_state(node, EN_UPDATED);
>  }
>
> diff --git a/northd/northd.c b/northd/northd.c
> index f32e3bf21a..23f2dae26b 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -3435,6 +3435,9 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn
*ovnsb_txn,
>  {
>      sbrec_port_binding_set_datapath(op->sb, op->od->sb);
>      if (op->nbrp) {
> +        /* Note: SB port binding options for router ports are set in
> +         * sync_pbs(). */
> +
>          /* If the router is for l3 gateway, it resides on a chassis
>           * and its port type is "l3gateway". */
>          const char *chassis_name = smap_get(&op->od->nbr->options,
"chassis");
> @@ -3446,15 +3449,11 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn
*ovnsb_txn,
>              sbrec_port_binding_set_type(op->sb, "patch");
>          }
>
> -        struct smap new;
> -        smap_init(&new);
>          if (is_cr_port(op)) {
>              ovs_assert(sbrec_chassis_by_name);
>              ovs_assert(sbrec_chassis_by_hostname);
>              ovs_assert(sbrec_ha_chassis_grp_by_name);
>              ovs_assert(active_ha_chassis_grps);
> -            const char *redirect_type = smap_get(&op->nbrp->options,
> -                                                 "redirect-type");
>
>              if (op->nbrp->ha_chassis_group) {
>                  if (op->nbrp->n_gateway_chassis) {
> @@ -3496,49 +3495,8 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn
*ovnsb_txn,
>                  /* Delete the legacy gateway_chassis from the pb. */
>                  sbrec_port_binding_set_gateway_chassis(op->sb, NULL, 0);
>              }
> -            smap_add(&new, "distributed-port", op->nbrp->name);
> -
> -            bool always_redirect =
> -                !op->od->has_distributed_nat &&
> -                !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port);
> -
> -            if (redirect_type) {
> -                smap_add(&new, "redirect-type", redirect_type);
> -                /* XXX Why can't we enable always-redirect when
redirect-type
> -                 * is bridged? */
> -                if (!strcmp(redirect_type, "bridged")) {
> -                    always_redirect = false;
> -                }
> -            }
> -
> -            if (always_redirect) {
> -                smap_add(&new, "always-redirect", "true");
> -            }
> -        } else {
> -            if (op->peer) {
> -                smap_add(&new, "peer", op->peer->key);
> -                if (op->nbrp->ha_chassis_group ||
> -                    op->nbrp->n_gateway_chassis) {
> -                    char *redirect_name =
> -                        ovn_chassis_redirect_name(op->nbrp->name);
> -                    smap_add(&new, "chassis-redirect-port",
redirect_name);
> -                    free(redirect_name);
> -                }
> -            }
> -            if (chassis_name) {
> -                smap_add(&new, "l3gateway-chassis", chassis_name);
> -            }
>          }
>
> -        const char *ipv6_pd_list = smap_get(&op->sb->options,
> -                                            "ipv6_ra_pd_list");
> -        if (ipv6_pd_list) {
> -            smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list);
> -        }
> -
> -        sbrec_port_binding_set_options(op->sb, &new);
> -        smap_destroy(&new);
> -
>          sbrec_port_binding_set_parent_port(op->sb, NULL);
>          sbrec_port_binding_set_tag(op->sb, NULL, 0);
>
> @@ -4768,12 +4726,14 @@ check_sb_lb_duplicates(const struct
sbrec_load_balancer_table *table)
>      return duplicates;
>  }
>
> -/* Syncs the SB port binding for the ovn_port 'op'.  Caller should make
sure
> - * that the OVN SB IDL txn is not NULL.  Presently it only syncs the nat
> - * column of port binding corresponding to the 'op->nbsp' */
> +/* Syncs the SB port binding for the ovn_port 'op' of a logical switch
port.
> + * Caller should make sure that the OVN SB IDL txn is not NULL.
Presently it
> + * only syncs the nat column of port binding corresponding to the
'op->nbsp' */
>  static void
> -sync_pb_for_op(struct ovn_port *op)
> +sync_pb_for_lsp(struct ovn_port *op)
>  {
> +    ovs_assert(op->nbsp);
> +
>      if (lsp_is_router(op->nbsp)) {
>          const char *chassis = NULL;
>          if (op->peer && op->peer->od && op->peer->od->nbr) {
> @@ -4895,18 +4855,86 @@ sync_pb_for_op(struct ovn_port *op)
>      }
>  }
>
> +/* Syncs the SB port binding for the ovn_port 'op' of a logical router
port.
> + * Caller should make sure that the OVN SB IDL txn is not NULL.
Presently it
> + * only sets the port binding options column for the router ports */
> +static void
> +sync_pb_for_lrp(struct ovn_port *op)
> +{
> +    ovs_assert(op->nbrp);
> +
> +    struct smap new;
> +    smap_init(&new);
> +
> +    const char *chassis_name = smap_get(&op->od->nbr->options,
"chassis");
> +    if (is_cr_port(op)) {
> +        smap_add(&new, "distributed-port", op->nbrp->name);
> +
> +        bool always_redirect =
> +            !op->od->has_distributed_nat &&
> +            !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port);
> +
> +        const char *redirect_type = smap_get(&op->nbrp->options,
> +                                            "redirect-type");
> +        if (redirect_type) {
> +            smap_add(&new, "redirect-type", redirect_type);
> +            /* XXX Why can't we enable always-redirect when redirect-type
> +                * is bridged? */
> +            if (!strcmp(redirect_type, "bridged")) {
> +                always_redirect = false;
> +            }
> +        }
> +
> +        if (always_redirect) {
> +            smap_add(&new, "always-redirect", "true");
> +        }
> +    } else {
> +        if (op->peer) {
> +            smap_add(&new, "peer", op->peer->key);
> +            if (op->nbrp->ha_chassis_group ||
> +                op->nbrp->n_gateway_chassis) {
> +                char *redirect_name =
> +                    ovn_chassis_redirect_name(op->nbrp->name);
> +                smap_add(&new, "chassis-redirect-port", redirect_name);
> +                free(redirect_name);
> +            }
> +        }
> +        if (chassis_name) {
> +            smap_add(&new, "l3gateway-chassis", chassis_name);
> +        }
> +    }
> +
> +    const char *ipv6_pd_list = smap_get(&op->sb->options,
"ipv6_ra_pd_list");
> +    if (ipv6_pd_list) {
> +        smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list);
> +    }
> +
> +    sbrec_port_binding_set_options(op->sb, &new);
> +    smap_destroy(&new);
> +}
> +
> +static void ovn_update_ipv6_options(struct hmap *lr_ports);
> +static void ovn_update_ipv6_opt_for_op(struct ovn_port *op);
> +
>  /* Sync the SB Port bindings which needs to be updated.
>   * Presently it syncs the nat column of port bindings corresponding to
>   * the logical switch ports. */
>  void
> -sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports)
> +sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports,
> +         struct hmap *lr_ports)
>  {
>      ovs_assert(ovnsb_idl_txn);
>
>      struct ovn_port *op;
>      HMAP_FOR_EACH (op, key_node, ls_ports) {
> -        sync_pb_for_op(op);
> +        sync_pb_for_lsp(op);
> +    }
> +
> +    HMAP_FOR_EACH (op, key_node, lr_ports) {
> +        sync_pb_for_lrp(op);
>      }
> +
> +    ovn_update_ipv6_options(lr_ports);
>  }
>
>  /* Sync the SB Port bindings for the added and updated logical switch
ports
> @@ -4918,12 +4946,14 @@ sync_pbs_for_northd_changed_ovn_ports( struct
tracked_ovn_ports *trk_ovn_ports)
>      struct ovn_port *op;
>      HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->created) {
>          op = hmapx_node->data;
> -        sync_pb_for_op(op);
> +        ovs_assert(op->nbsp);
> +        sync_pb_for_lsp(op);
>      }
>
>      HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->updated) {
>          op = hmapx_node->data;
> -        sync_pb_for_op(op);
> +        ovs_assert(op->nbsp);
> +        sync_pb_for_lsp(op);
>      }
>
>      return true;
> @@ -5671,20 +5701,31 @@ fail:
>  bool
>  northd_handle_sb_port_binding_changes(
>      const struct sbrec_port_binding_table *sbrec_port_binding_table,
> -    struct hmap *ls_ports)
> +    struct hmap *ls_ports, struct hmap *lr_ports)
>  {
>      const struct sbrec_port_binding *pb;
>      static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>      SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb,
sbrec_port_binding_table) {
> -        /* Return false if the 'pb' belongs to a router port.  We don't
handle
> -         * I-P for router ports yet. */
> -        if (is_pb_router_type(pb)) {
> -            return false;
> +        bool is_router_port = is_pb_router_type(pb);
> +        struct ovn_port *op = NULL;
> +
> +        if (is_router_port) {
> +            /* A router port binding 'pb' can belong to
> +             *   - a logical switch port of type router or
> +             *   - a logical router port.
> +             *
> +             * So, first search in lr_ports hmap.  If not found, search
in
> +             * ls_ports hmap.
> +             * */
> +            op = ovn_port_find(lr_ports, pb->logical_port);
>          }
>
> -        struct ovn_port *op = ovn_port_find(ls_ports, pb->logical_port);
> -        if (op && !op->lsp_can_be_inc_processed) {
> -            return false;
> +        if (!op) {
> +            op = ovn_port_find(ls_ports, pb->logical_port);
> +
> +            if (op) {
> +                is_router_port = false;
> +            }
>          }
>
>          if (sbrec_port_binding_is_new(pb)) {
> @@ -5693,7 +5734,8 @@ northd_handle_sb_port_binding_changes(
>               * pointer in northd data. Fallback to recompute otherwise.
*/
>              if (!op) {
>                  VLOG_WARN_RL(&rl, "A port-binding for %s is created but
the "
> -                            "LSP is not found.", pb->logical_port);
> +                            "%s is not found.", pb->logical_port,
> +                            is_router_port ? "LRP" : "LSP");
>                  return false;
>              }
>              op->sb = pb;
> @@ -5703,18 +5745,29 @@ northd_handle_sb_port_binding_changes(
>               * case. Fallback to recompute otherwise, to avoid dangling
>               * sb idl pointers and other unexpected behavior. */
>              if (op && op->sb == pb) {
> -                VLOG_WARN_RL(&rl, "A port-binding for %s is deleted but "
> -                            "the LSP still exists.", pb->logical_port);
> +                VLOG_WARN_RL(&rl, "A port-binding for %s is deleted but
the "
> +                            "LSP/LRP still exists.", pb->logical_port);
>                  return false;
>              }
>          } else {
> -            /* The PB is updated, most likely because of
binding/unbinding
> -             * to/from a chassis, and we can ignore the change (updating
NB
> -             * "up" will be handled in the engine node "sync_from_sb").
> +            /* The PB is updated.
> +             * For an LSP PB it is most likely because of
> +             * binding/unbinding to/from a chassis, and we can ignore the
> +             * change (updating NB "up" will be handled in the engine
node
> +             * "sync_from_sb").
> +             *
> +             * For an LRP PB, it is most likely because of
> +             *   - IPv6 prefix delagation updates from ovn-controller.
> +             *     This update is handled in "sync_from_sb" node.
> +             *   - ha chassis group and this can be ignored.
> +             *
> +             * All other changes can be ignored.
> +             *
>               * Fallback to recompute for anything unexpected. */
>              if (!op) {
>                  VLOG_WARN_RL(&rl, "A port-binding for %s is updated but
the "
> -                            "LSP is not found.", pb->logical_port);
> +                            "%s is not found.", pb->logical_port,
> +                            is_router_port ? "LRP" : "LSP");
>                  return false;
>              }
>              if (op->sb != pb) {
> @@ -7855,67 +7908,70 @@ static void
>  copy_ra_to_sb(struct ovn_port *op, const char *address_mode);
>
>  static void
> -ovn_update_ipv6_options(struct hmap *lr_ports)
> +ovn_update_ipv6_opt_for_op(struct ovn_port *op)
>  {
> -    struct ovn_port *op;
> -    HMAP_FOR_EACH (op, key_node, lr_ports) {
> -        ovs_assert(op->nbrp);
> -
> -        if (op->nbrp->peer || !op->peer) {
> -            continue;
> -        }
> +    if (op->nbrp->peer || !op->peer) {
> +        return;
> +    }
>
> -        if (!op->lrp_networks.n_ipv6_addrs) {
> -            continue;
> -        }
> +    if (!op->lrp_networks.n_ipv6_addrs) {
> +        return;
> +    }
>
> -        struct smap options;
> -        smap_clone(&options, &op->sb->options);
> +    struct smap options;
> +    smap_clone(&options, &op->sb->options);
>
> -        /* enable IPv6 prefix delegation */
> -        bool prefix_delegation = smap_get_bool(&op->nbrp->options,
> +    /* enable IPv6 prefix delegation */
> +    bool prefix_delegation = smap_get_bool(&op->nbrp->options,
>                                             "prefix_delegation", false);
> -        if (!lrport_is_enabled(op->nbrp)) {
> -            prefix_delegation = false;
> -        }
> -        if (smap_get_bool(&options, "ipv6_prefix_delegation",
> -                          false) != prefix_delegation) {
> -            smap_add(&options, "ipv6_prefix_delegation",
> -                     prefix_delegation ? "true" : "false");
> -        }
> +    if (!lrport_is_enabled(op->nbrp)) {
> +        prefix_delegation = false;
> +    }
> +    if (smap_get_bool(&options, "ipv6_prefix_delegation",
> +                      false) != prefix_delegation) {
> +        smap_add(&options, "ipv6_prefix_delegation",
> +                 prefix_delegation ? "true" : "false");
> +    }
>
> -        bool ipv6_prefix = smap_get_bool(&op->nbrp->options,
> +    bool ipv6_prefix = smap_get_bool(&op->nbrp->options,
>                                       "prefix", false);
> -        if (!lrport_is_enabled(op->nbrp)) {
> -            ipv6_prefix = false;
> -        }
> -        if (smap_get_bool(&options, "ipv6_prefix", false) !=
ipv6_prefix) {
> -            smap_add(&options, "ipv6_prefix",
> -                     ipv6_prefix ? "true" : "false");
> -        }
> -        sbrec_port_binding_set_options(op->sb, &options);
> +    if (!lrport_is_enabled(op->nbrp)) {
> +        ipv6_prefix = false;
> +    }
> +    if (smap_get_bool(&options, "ipv6_prefix", false) != ipv6_prefix) {
> +        smap_add(&options, "ipv6_prefix", ipv6_prefix ? "true" :
"false");
> +    }
> +    sbrec_port_binding_set_options(op->sb, &options);
>
> -        smap_destroy(&options);
> +    smap_destroy(&options);
>
> -        const char *address_mode = smap_get(
> -            &op->nbrp->ipv6_ra_configs, "address_mode");
> +    const char *address_mode = smap_get(&op->nbrp->ipv6_ra_configs,
> +                                        "address_mode");
>
> -        if (!address_mode) {
> -            continue;
> -        }
> -        if (strcmp(address_mode, "slaac") &&
> -            strcmp(address_mode, "dhcpv6_stateful") &&
> -            strcmp(address_mode, "dhcpv6_stateless")) {
> -            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1,
5);
> -            VLOG_WARN_RL(&rl, "Invalid address mode [%s] defined",
> -                         address_mode);
> -            continue;
> -        }
> +    if (!address_mode) {
> +        return;
> +    }
> +    if (strcmp(address_mode, "slaac") &&
> +        strcmp(address_mode, "dhcpv6_stateful") &&
> +        strcmp(address_mode, "dhcpv6_stateless")) {
> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> +        VLOG_WARN_RL(&rl, "Invalid address mode [%s] defined",
> +                     address_mode);
> +        return;
> +    }
>
> -        if (smap_get_bool(&op->nbrp->ipv6_ra_configs, "send_periodic",
> -                          false)) {
> -            copy_ra_to_sb(op, address_mode);
> -        }
> +    if (smap_get_bool(&op->nbrp->ipv6_ra_configs, "send_periodic",
false)) {
> +        copy_ra_to_sb(op, address_mode);
> +    }
> +}
> +
> +static void
> +ovn_update_ipv6_options(struct hmap *lr_ports)
> +{
> +    struct ovn_port *op;
> +    HMAP_FOR_EACH (op, key_node, lr_ports) {
> +        ovs_assert(op->nbrp);
> +        ovn_update_ipv6_opt_for_op(op);
>      }
>  }
>
> @@ -18007,8 +18063,6 @@ ovnnb_db_run(struct northd_input *input_data,
>          &data->lr_ports);
>      stopwatch_stop(BUILD_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
>      stopwatch_start(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
> -    ovn_update_ipv6_options(&data->lr_ports);
> -    ovn_update_ipv6_prefix(&data->lr_ports);
>
>      sync_mirrors(ovnsb_txn, input_data->nbrec_mirror_table,
>                   input_data->sbrec_mirror_table);
> @@ -18339,6 +18393,8 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
>                                         &ha_ref_chassis_map);
>      }
>      shash_destroy(&ha_ref_chassis_map);
> +
> +    ovn_update_ipv6_prefix(lr_ports);
>  }
>
>  const char *
> diff --git a/northd/northd.h b/northd/northd.h
> index 23521065e8..233dca8084 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -364,7 +364,8 @@ bool lflow_handle_northd_port_changes(struct
ovsdb_idl_txn *ovnsb_txn,
>                                        struct lflow_input *,
>                                        struct hmap *lflows);
>  bool northd_handle_sb_port_binding_changes(
> -    const struct sbrec_port_binding_table *, struct hmap *ls_ports);
> +    const struct sbrec_port_binding_table *, struct hmap *ls_ports,
> +    struct hmap *lr_ports);
>
>  struct tracked_lb_data;
>  bool northd_handle_lb_data_changes(struct tracked_lb_data *,
> @@ -394,7 +395,8 @@ void sync_lbs(struct ovsdb_idl_txn *, const struct
sbrec_load_balancer_table *,
>                struct chassis_features *chassis_features);
>  bool check_sb_lb_duplicates(const struct sbrec_load_balancer_table *);
>
> -void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports);
> +void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports,
> +              struct hmap *lr_ports);
>  bool sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports *);
>
>  static inline bool
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 4a02dacf60..4edad24e53 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -10073,7 +10073,7 @@ check ovn-nbctl --wait=hv lsp-add ls0 lsp0-0 --
lsp-set-addresses lsp0-0 "unknow
>  ovs-vsctl add-port br-int lsp0-0 -- set interface lsp0-0
external_ids:iface-id=lsp0-0
>  wait_for_ports_up
>  check ovn-nbctl --wait=hv sync
> -check_recompute_counter 4 5 5 5 5 5
> +check_recompute_counter 3 4 3 4 3 4
>
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=hv lsp-add ls0 lsp0-1 -- lsp-set-addresses lsp0-1
"aa:aa:aa:00:00:01 192.168.0.11"
> @@ -10235,6 +10235,126 @@ OVN_CLEANUP([hv1])
>  AT_CLEANUP
>  ])
>
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([SB Port binding incremental processing])
> +ovn_start
> +
> +check_recompute_counter() {
> +    northd_recomp=$(as northd ovn-appctl -t ovn-northd
inc-engine/show-stats northd recompute)
> +    echo "northd recompute count - $northd_recomp"
> +    AT_CHECK([test x$northd_recomp = x$1])
> +
> +    lflow_recomp=$(as northd ovn-appctl -t ovn-northd
inc-engine/show-stats lflow recompute)
> +    echo "lflow recompute count - $lflow_recomp"
> +    AT_CHECK([test x$lflow_recomp = x$2])
> +}
> +
> +net_add n1
> +sim_add hv1
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.11
> +
> +check ovn-nbctl --wait=sb ls-add ls0
> +check ovn-nbctl --wait=sb lr-add lr0
> +
> +# Test normal VIF port
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lsp-add ls0 p1
> +check_recompute_counter 0 0
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lsp-set-addresses p1 "00:00:00:00:01:01 10.0.0.3"
> +check_recompute_counter 0 0
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +check as northd ovn-appctl -t ovn-northd vlog/set info
> +
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-sbctl lsp-bind p1 hv1
> +check_recompute_counter 0 0
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +# Test lsp of type router
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lsp-add ls0 rp -- lsp-set-type rp router
> +
> +# northd engine recomputes twice. Both the times for handling NB logical
switch port
> +# changes and not because of SB port binding changes.  This is because
ovn-northd
> +# sets the "up" to true.
> +check_recompute_counter 2 2
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +# Set some options to 'rp'.  northd should only recompute once.
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lsp-set-options rp foo=bar
> +check_recompute_counter 1 1
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +# Test lsp of type external
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lsp-add ls0 e1 -- lsp-set-type e1 external
> +check_recompute_counter 2 2
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +# Set some options to 'e1'.  northd should only recompute once.
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lsp-set-options e1 foo=bar
> +check_recompute_counter 1 1
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +# Test lrp
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lrp-add lr0 lrp 00:00:02:01:02:03 10.0.0.1/24
> +check_recompute_counter 1 1
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +# Set some options on 'lrp'.  northd should only recompute once.
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lrp-set-options lrp route_table=rtb-1
> +check_recompute_counter 1 1
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +# Make lrp a gateway port
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lrp-set-gateway-chassis lrp hv1
> +# There will be 3 recomputes of northd engine node
> +#   1. missing handler for input NB_logical_router
> +#   2. missing handler for input SB_ha_chassis_group
> +#   3. missing handler for input NB_logical_router when ovn-northd
> +#      updates the hosting-chassis option in NB_logical_router_port.
> +check_recompute_counter 3 3
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl clear logical_router_port lrp gateway_chassis
> +# There will be 2 recomputes of northd engine node
> +#   1. missing handler for input NB_logical_router
> +#   2. missing handler for input SB_ha_chassis_group
> +check_recompute_counter 2 2
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +# Delete some of the ports.
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lsp-del p1
> +check_recompute_counter 0 0
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lsp-del e1
> +check_recompute_counter 1 1
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl lrp-del lrp
> +check_recompute_counter 1 1
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
> +
>  OVN_FOR_EACH_NORTHD_NO_HV([
>  AT_SETUP([ACL/Meter incremental processing - no northd recompute])
>  ovn_start
> @@ -10269,6 +10389,7 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE(1)
>  AT_CLEANUP
>  ])
>
> +
>  OVN_FOR_EACH_NORTHD_NO_HV([
>  AT_SETUP([check fip and lb flows])
>  AT_KEYWORDS([fip-lb-flows])
> @@ -10471,7 +10592,7 @@ ovn-nbctl lsp-set-addresses sw0-lr0
00:00:00:00:ff:01
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  ovn-nbctl --wait=sb lsp-set-options sw0-lr0 router-port=lr0-sw0
>  check_engine_stats lb_data norecompute compute
> -check_engine_stats northd recompute nocompute
> +check_engine_stats northd recompute compute
>  check_engine_stats lflow recompute nocompute
>  check_engine_stats sync_to_sb_lb recompute nocompute
>
> @@ -11114,12 +11235,14 @@ ovn_attach n1 br-phys 192.168.0.11
>  ovn-sbctl chassis-add gw1 geneve 127.0.0.1
>
>  check ovn-nbctl ls-add sw0
> -check ovn-nbctl lsp-add sw0 sw0p1 -- lsp-set-addresses sw0p1
"00:00:20:20:12:01 10.0.0.4"
> +check ovn-nbctl --wait=sb lsp-add sw0 sw0p1 -- lsp-set-addresses sw0p1
"00:00:20:20:12:01 10.0.0.4"
>
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb lr-add lr0
>  check_engine_stats northd recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
>  # Adding a logical router port should result in recompute
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> @@ -11127,16 +11250,20 @@ check ovn-nbctl lrp-add lr0 lr0-sw0
00:00:00:00:ff:01 10.0.0.1/24
>  # for northd engine there will be both recompute and compute
>  # first it will be recompute to handle lr0-sw0 and then a compute
>  # for the SB port binding change.
> -check_engine_stats northd recompute nocompute
> +check_engine_stats northd recompute compute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
>  ovn-nbctl lsp-add sw0 sw0-lr0
>  ovn-nbctl lsp-set-type sw0-lr0 router
>  ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb lsp-set-options sw0-lr0 router-port=lr0-sw0
> -check_engine_stats northd recompute nocompute
> +check_engine_stats northd recompute compute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
>  ovn-nbctl ls-add public
>  ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24
> @@ -11158,7 +11285,9 @@ ovn-nbctl --wait=hv lrp-set-gateway-chassis
lr0-public hv1 20
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl set logical_router_port lr0-sw0 options:foo=bar
>  check_engine_stats northd recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
>  # Do checks for NATs.
>  # Add a NAT. This should not result in recompute of both northd and lflow
> @@ -11167,6 +11296,7 @@ check as northd ovn-appctl -t ovn-northd
inc-engine/clear-stats
>  check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat  172.168.0.110
10.0.0.4
>  check_engine_stats northd recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
>  # Update the NAT options column
> @@ -11174,6 +11304,7 @@ check as northd ovn-appctl -t ovn-northd
inc-engine/clear-stats
>  check ovn-nbctl --wait=sb set NAT . options:foo=bar
>  check_engine_stats northd recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
>  # Update the NAT external_ip column
> @@ -11181,6 +11312,7 @@ check as northd ovn-appctl -t ovn-northd
inc-engine/clear-stats
>  check ovn-nbctl --wait=sb set NAT . external_ip=172.168.0.120
>  check_engine_stats northd recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
>  # Update the NAT logical_ip column
> @@ -11188,6 +11320,7 @@ check as northd ovn-appctl -t ovn-northd
inc-engine/clear-stats
>  check ovn-nbctl --wait=sb set NAT . logical_ip=10.0.0.10
>  check_engine_stats northd recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
>  # Update the NAT type
> @@ -11195,13 +11328,15 @@ check as northd ovn-appctl -t ovn-northd
inc-engine/clear-stats
>  check ovn-nbctl --wait=sb set NAT . type=snat
>  check_engine_stats northd recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
>  # Create a dnat_and_snat NAT with external_mac and logical_port
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.110
10.0.0.4 sw0p1 30:54:00:00:00:03
> -check_engine_stats northd recompute nocompute
> +check_engine_stats northd recompute compute
>  check_engine_stats lflow recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
>  nat2_uuid=$(ovn-nbctl --bare --columns _uuid find nat
logical_ip=10.0.0.4)
> @@ -11210,6 +11345,7 @@ check as northd ovn-appctl -t ovn-northd
inc-engine/clear-stats
>  check ovn-nbctl --wait=sb set NAT $nat2_uuid
external_mac='"30:54:00:00:00:04"'
>  check_engine_stats northd recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
>  # Create a load balancer and add the lb vip as NAT
> @@ -11223,31 +11359,35 @@ check ovn-nbctl lr-lb-add lr0 lb2
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.140
10.0.0.20
>  check_engine_stats northd recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  check_engine_stats lflow recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.150
10.0.0.41
>  check_engine_stats northd recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  check_engine_stats lflow recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb lr-nat-del lr0 dnat_and_snat 172.168.0.150
>  check_engine_stats northd recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  check_engine_stats lflow recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb lr-nat-del lr0 dnat_and_snat 172.168.0.140
>  check_engine_stats northd recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  check_engine_stats lflow recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
>  # Delete the NAT
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb clear logical_router lr0 nat
> -check_engine_stats northd recompute nocompute
> +check_engine_stats northd recompute compute
>  check_engine_stats lflow recompute nocompute
>  check_engine_stats sync_to_sb_pb recompute nocompute
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> @@ -11256,12 +11396,16 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb lr-policy-add lr0  10 "ip4.src == 10.0.0.3"
reroute 172.168.0.101,172.168.0.102
>  check_engine_stats northd recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-nbctl --wait=sb lr-policy-del lr0  10 "ip4.src == 10.0.0.3"
>  check_engine_stats northd recompute nocompute
> +check_engine_stats sync_to_sb_pb recompute nocompute
>  check_engine_stats lflow recompute nocompute
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
>  OVN_CLEANUP([hv1])
>  AT_CLEANUP
> --
> 2.43.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Acked-by: Han Zhou <hzhou@ovn.org>

On Tue, Jan 16, 2024 at 6:15 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 1/11/24 16:28, numans@ovn.org wrote:
> > From: Numan Siddique <numans@ovn.org>
> >
> > It also moves the logical router port IPv6 prefix delegation
> > updates to "sync-from-sb" engine node.
> >
> > With these changes, northd engine node doesn't need to do much
> > for SB Port binding changes other than updating 'op->sb'.  And
> > there is no need to fall back to recompute.  Prior to this patch
> > northd_handle_sb_port_binding_changes() was returning false for
> > all SB port binding updates except for the VIF PBs.  This patch
> > now handles updates to all the SB port binding by setting 'op->sb'.
> >
> > Signed-off-by: Numan Siddique <numans@ovn.org>
> > ---
>
> Hi Numan,
>
> With this patch applied the "633: ovn-northd.at:10298 SB Port binding
> incremental processing -- parallelization=yes" test occasionally fails,
> e.g.:
>
> https://github.com/dceara/ovn/actions/runs/7539663594/job/20524238508
>
> I saw it locally too when running the test in a loop:
>
> It fails with:
> ovn-nbctl lrp-set-gateway-chassis lrp hv1
> ./ovn-macros.at:449: "$@"
> northd recompute count - 2
> ./ovn-northd.at:10238: test x$northd_recomp = x$1
> ./ovn-northd.at:10238: exit code was 1, expected 0
>
> Here:
> https://github.com/dceara/ovn/blob/e1c76b6d347570618591fe6587f6af83445223ec/tests/ovn-northd.at#L10327
>
> For some reason in some cases only 2 recomputes get triggered.

Thanks Dumitru and Han for the reviews.

@Dumitru Ceara  I've addressed all the review comments below.

I think the test case is failing because of the missing "--wait=sb" in
the ovn-nbctl commands.

Will the below changes in ovn-northd.at look good to you  ?  If so,
and if you can provide your ack I can apply this patch.
Otherwise I'll include it in the v6.

----
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 72eb9e1173..492248b05d 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -10316,12 +10316,12 @@ check ovn-nbctl --wait=sb lr-add lr0

 # Test normal VIF port
 check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
-check ovn-nbctl lsp-add ls0 p1
+check ovn-nbctl --wait=sb lsp-add ls0 p1
 check_recompute_counter 0 0
 CHECK_NO_CHANGE_AFTER_RECOMPUTE

 check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
-check ovn-nbctl lsp-set-addresses p1 "00:00:00:00:01:01 10.0.0.3"
+check ovn-nbctl --wait=sb lsp-set-addresses p1 "00:00:00:00:01:01 10.0.0.3"
 check_recompute_counter 0 0
 CHECK_NO_CHANGE_AFTER_RECOMPUTE

@@ -10329,12 +10329,13 @@ check as northd ovn-appctl -t ovn-northd vlog/set info

 check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
 check ovn-sbctl lsp-bind p1 hv1
+check ovn-nbctl --wait=sb sync
 check_recompute_counter 0 0
 CHECK_NO_CHANGE_AFTER_RECOMPUTE

 # Test lsp of type router
 check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
-check ovn-nbctl lsp-add ls0 rp -- lsp-set-type rp router
+check ovn-nbctl --wait=sb lsp-add ls0 rp -- lsp-set-type rp router

 # northd engine recomputes twice. Both the times for handling NB
logical switch port
 # changes and not because of SB port binding changes.  This is
because ovn-northd
@@ -10344,37 +10345,37 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE

 # Set some options to 'rp'.  northd should only recompute once.
 check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
-check ovn-nbctl lsp-set-options rp foo=bar
+check ovn-nbctl --wait=sb lsp-set-options rp foo=bar
 check_recompute_counter 1 1
 CHECK_NO_CHANGE_AFTER_RECOMPUTE

 # Test lsp of type external
 check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
-check ovn-nbctl lsp-add ls0 e1 -- lsp-set-type e1 external
+check ovn-nbctl --wait=sb lsp-add ls0 e1 -- lsp-set-type e1 external
 check_recompute_counter 2 2
 CHECK_NO_CHANGE_AFTER_RECOMPUTE

 # Set some options to 'e1'.  northd should only recompute once.
 check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
-check ovn-nbctl lsp-set-options e1 foo=bar
+check ovn-nbctl --wait=sb lsp-set-options e1 foo=bar
 check_recompute_counter 1 1
 CHECK_NO_CHANGE_AFTER_RECOMPUTE

 # Test lrp
 check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
-check ovn-nbctl lrp-add lr0 lrp 00:00:02:01:02:03 10.0.0.1/24
+check ovn-nbctl --wait=sb lrp-add lr0 lrp 00:00:02:01:02:03 10.0.0.1/24
 check_recompute_counter 1 1
 CHECK_NO_CHANGE_AFTER_RECOMPUTE

 # Set some options on 'lrp'.  northd should only recompute once.
 check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
-check ovn-nbctl lrp-set-options lrp route_table=rtb-1
+check ovn-nbctl --wait=sb lrp-set-options lrp route_table=rtb-1
 check_recompute_counter 1 1
 CHECK_NO_CHANGE_AFTER_RECOMPUTE

 # Make lrp a gateway port
 check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
-check ovn-nbctl lrp-set-gateway-chassis lrp hv1
+check ovn-nbctl --wait=sb lrp-set-gateway-chassis lrp hv1
 # There will be 3 recomputes of northd engine node
 #   1. missing handler for input NB_logical_router
 #   2. missing handler for input SB_ha_chassis_group
@@ -10384,7 +10385,7 @@ check_recompute_counter 3 3
 CHECK_NO_CHANGE_AFTER_RECOMPUTE

 check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
-check ovn-nbctl clear logical_router_port lrp gateway_chassis
+check ovn-nbctl --wait=sb clear logical_router_port lrp gateway_chassis
 # There will be 2 recomputes of northd engine node
 #   1. missing handler for input NB_logical_router
 #   2. missing handler for input SB_ha_chassis_group
@@ -10393,17 +10394,17 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE

 # Delete some of the ports.
 check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
-check ovn-nbctl lsp-del p1
+check ovn-nbctl --wait=sb lsp-del p1
 check_recompute_counter 0 0
 CHECK_NO_CHANGE_AFTER_RECOMPUTE

 check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
-check ovn-nbctl lsp-del e1
+check ovn-nbctl --wait=sb lsp-del e1
 check_recompute_counter 1 1
 CHECK_NO_CHANGE_AFTER_RECOMPUTE

 check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
-check ovn-nbctl lrp-del lrp
+check ovn-nbctl --wait=sb lrp-del lrp
 check_recompute_counter 1 1
 CHECK_NO_CHANGE_AFTER_RECOMPUTE
--------------------------------

Thanks
Numan



>
> >  northd/en-northd.c  |   2 +-
> >  northd/en-sync-sb.c |   3 +-
> >  northd/northd.c     | 296 ++++++++++++++++++++++++++------------------
> >  northd/northd.h     |   6 +-
> >  tests/ovn-northd.at | 158 +++++++++++++++++++++--
> >  5 files changed, 334 insertions(+), 131 deletions(-)
> >
> > diff --git a/northd/en-northd.c b/northd/en-northd.c
> > index 28559ed211..677b2b1ab0 100644
> > --- a/northd/en-northd.c
> > +++ b/northd/en-northd.c
> > @@ -189,7 +189,7 @@ northd_sb_port_binding_handler(struct engine_node *node,
> >      northd_get_input_data(node, &input_data);
> >
> >      if (!northd_handle_sb_port_binding_changes(
> > -        input_data.sbrec_port_binding_table, &nd->ls_ports)) {
> > +        input_data.sbrec_port_binding_table, &nd->ls_ports, &nd->lr_ports)) {
> >          return false;
> >      }
> >
> > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
> > index 3aaab8d005..45be7ddbcb 100644
> > --- a/northd/en-sync-sb.c
> > +++ b/northd/en-sync-sb.c
> > @@ -288,7 +288,8 @@ en_sync_to_sb_pb_run(struct engine_node *node, void *data OVS_UNUSED)
> >      const struct engine_context *eng_ctx = engine_get_context();
> >      struct northd_data *northd_data = engine_get_input_data("northd", node);
> >
> > -    sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports);
> > +    sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports,
> > +             &northd_data->lr_ports);
> >      engine_set_node_state(node, EN_UPDATED);
> >  }
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index f32e3bf21a..23f2dae26b 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -3435,6 +3435,9 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
> >  {
> >      sbrec_port_binding_set_datapath(op->sb, op->od->sb);
> >      if (op->nbrp) {
> > +        /* Note: SB port binding options for router ports are set in
> > +         * sync_pbs(). */
> > +
> >          /* If the router is for l3 gateway, it resides on a chassis
> >           * and its port type is "l3gateway". */
> >          const char *chassis_name = smap_get(&op->od->nbr->options, "chassis");
> > @@ -3446,15 +3449,11 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
> >              sbrec_port_binding_set_type(op->sb, "patch");
> >          }
> >
> > -        struct smap new;
> > -        smap_init(&new);
> >          if (is_cr_port(op)) {
> >              ovs_assert(sbrec_chassis_by_name);
> >              ovs_assert(sbrec_chassis_by_hostname);
> >              ovs_assert(sbrec_ha_chassis_grp_by_name);
> >              ovs_assert(active_ha_chassis_grps);
> > -            const char *redirect_type = smap_get(&op->nbrp->options,
> > -                                                 "redirect-type");
> >
> >              if (op->nbrp->ha_chassis_group) {
> >                  if (op->nbrp->n_gateway_chassis) {
> > @@ -3496,49 +3495,8 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
> >                  /* Delete the legacy gateway_chassis from the pb. */
> >                  sbrec_port_binding_set_gateway_chassis(op->sb, NULL, 0);
> >              }
> > -            smap_add(&new, "distributed-port", op->nbrp->name);
> > -
> > -            bool always_redirect =
> > -                !op->od->has_distributed_nat &&
> > -                !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port);
> > -
> > -            if (redirect_type) {
> > -                smap_add(&new, "redirect-type", redirect_type);
> > -                /* XXX Why can't we enable always-redirect when redirect-type
> > -                 * is bridged? */
> > -                if (!strcmp(redirect_type, "bridged")) {
> > -                    always_redirect = false;
> > -                }
> > -            }
> > -
> > -            if (always_redirect) {
> > -                smap_add(&new, "always-redirect", "true");
> > -            }
> > -        } else {
> > -            if (op->peer) {
> > -                smap_add(&new, "peer", op->peer->key);
> > -                if (op->nbrp->ha_chassis_group ||
> > -                    op->nbrp->n_gateway_chassis) {
> > -                    char *redirect_name =
> > -                        ovn_chassis_redirect_name(op->nbrp->name);
> > -                    smap_add(&new, "chassis-redirect-port", redirect_name);
> > -                    free(redirect_name);
> > -                }
> > -            }
> > -            if (chassis_name) {
> > -                smap_add(&new, "l3gateway-chassis", chassis_name);
> > -            }
> >          }
> >
> > -        const char *ipv6_pd_list = smap_get(&op->sb->options,
> > -                                            "ipv6_ra_pd_list");
> > -        if (ipv6_pd_list) {
> > -            smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list);
> > -        }
> > -
> > -        sbrec_port_binding_set_options(op->sb, &new);
> > -        smap_destroy(&new);
> > -
> >          sbrec_port_binding_set_parent_port(op->sb, NULL);
> >          sbrec_port_binding_set_tag(op->sb, NULL, 0);
> >
> > @@ -4768,12 +4726,14 @@ check_sb_lb_duplicates(const struct sbrec_load_balancer_table *table)
> >      return duplicates;
> >  }
> >
> > -/* Syncs the SB port binding for the ovn_port 'op'.  Caller should make sure
> > - * that the OVN SB IDL txn is not NULL.  Presently it only syncs the nat
> > - * column of port binding corresponding to the 'op->nbsp' */
> > +/* Syncs the SB port binding for the ovn_port 'op' of a logical switch port.
> > + * Caller should make sure that the OVN SB IDL txn is not NULL.  Presently it
> > + * only syncs the nat column of port binding corresponding to the 'op->nbsp' */
> >  static void
> > -sync_pb_for_op(struct ovn_port *op)
> > +sync_pb_for_lsp(struct ovn_port *op)
> >  {
> > +    ovs_assert(op->nbsp);
> > +
> >      if (lsp_is_router(op->nbsp)) {
> >          const char *chassis = NULL;
> >          if (op->peer && op->peer->od && op->peer->od->nbr) {
> > @@ -4895,18 +4855,86 @@ sync_pb_for_op(struct ovn_port *op)
> >      }
> >  }
> >
> > +/* Syncs the SB port binding for the ovn_port 'op' of a logical router port.
> > + * Caller should make sure that the OVN SB IDL txn is not NULL.  Presently it
> > + * only sets the port binding options column for the router ports */
> > +static void
> > +sync_pb_for_lrp(struct ovn_port *op)
> > +{
> > +    ovs_assert(op->nbrp);
> > +
> > +    struct smap new;
> > +    smap_init(&new);
> > +
> > +    const char *chassis_name = smap_get(&op->od->nbr->options, "chassis");
> > +    if (is_cr_port(op)) {
> > +        smap_add(&new, "distributed-port", op->nbrp->name);
> > +
> > +        bool always_redirect =
> > +            !op->od->has_distributed_nat &&
> > +            !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port);
> > +
> > +        const char *redirect_type = smap_get(&op->nbrp->options,
> > +                                            "redirect-type");
> > +        if (redirect_type) {
> > +            smap_add(&new, "redirect-type", redirect_type);
> > +            /* XXX Why can't we enable always-redirect when redirect-type
> > +                * is bridged? */
>
> Nit: indentation
>
> > +            if (!strcmp(redirect_type, "bridged")) {
> > +                always_redirect = false;
> > +            }
> > +        }
> > +
> > +        if (always_redirect) {
> > +            smap_add(&new, "always-redirect", "true");
> > +        }
> > +    } else {
> > +        if (op->peer) {
> > +            smap_add(&new, "peer", op->peer->key);
> > +            if (op->nbrp->ha_chassis_group ||
> > +                op->nbrp->n_gateway_chassis) {
> > +                char *redirect_name =
> > +                    ovn_chassis_redirect_name(op->nbrp->name);
> > +                smap_add(&new, "chassis-redirect-port", redirect_name);
> > +                free(redirect_name);
> > +            }
> > +        }
> > +        if (chassis_name) {
> > +            smap_add(&new, "l3gateway-chassis", chassis_name);
> > +        }
> > +    }
> > +
> > +    const char *ipv6_pd_list = smap_get(&op->sb->options, "ipv6_ra_pd_list");
> > +    if (ipv6_pd_list) {
> > +        smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list);
> > +    }
> > +
> > +    sbrec_port_binding_set_options(op->sb, &new);
> > +    smap_destroy(&new);
> > +}
> > +
> > +static void ovn_update_ipv6_options(struct hmap *lr_ports);
> > +static void ovn_update_ipv6_opt_for_op(struct ovn_port *op);
> > +
> >  /* Sync the SB Port bindings which needs to be updated.
> >   * Presently it syncs the nat column of port bindings corresponding to
> >   * the logical switch ports. */
> >  void
> > -sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports)
> > +sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports,
> > +         struct hmap *lr_ports)
> >  {
> >      ovs_assert(ovnsb_idl_txn);
> >
> >      struct ovn_port *op;
> >      HMAP_FOR_EACH (op, key_node, ls_ports) {
> > -        sync_pb_for_op(op);
> > +        sync_pb_for_lsp(op);
> > +    }
> > +
> > +    HMAP_FOR_EACH (op, key_node, lr_ports) {
> > +        sync_pb_for_lrp(op);
> >      }
> > +
> > +    ovn_update_ipv6_options(lr_ports);
> >  }
> >
> >  /* Sync the SB Port bindings for the added and updated logical switch ports
> > @@ -4918,12 +4946,14 @@ sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports *trk_ovn_ports)
> >      struct ovn_port *op;
> >      HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->created) {
> >          op = hmapx_node->data;
> > -        sync_pb_for_op(op);
> > +        ovs_assert(op->nbsp);
>
> This is already checked inside sync_pb_for_lsp().  Let's remove it from
> here.  Then what I mentioned in patch 01/16 still applies, we don't need
> the local 'op' variable anymore.
>
> > +        sync_pb_for_lsp(op);
> >      }
> >
> >      HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->updated) {
> >          op = hmapx_node->data;
> > -        sync_pb_for_op(op);
> > +        ovs_assert(op->nbsp);
> > +        sync_pb_for_lsp(op);
>
> Same here.
>
> >      }
> >
> >      return true;
> > @@ -5671,20 +5701,31 @@ fail:
> >  bool
> >  northd_handle_sb_port_binding_changes(
> >      const struct sbrec_port_binding_table *sbrec_port_binding_table,
> > -    struct hmap *ls_ports)
> > +    struct hmap *ls_ports, struct hmap *lr_ports)
> >  {
> >      const struct sbrec_port_binding *pb;
> >      static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> >      SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, sbrec_port_binding_table) {
> > -        /* Return false if the 'pb' belongs to a router port.  We don't handle
> > -         * I-P for router ports yet. */
> > -        if (is_pb_router_type(pb)) {
> > -            return false;
> > +        bool is_router_port = is_pb_router_type(pb);
> > +        struct ovn_port *op = NULL;
> > +
> > +        if (is_router_port) {
> > +            /* A router port binding 'pb' can belong to
> > +             *   - a logical switch port of type router or
> > +             *   - a logical router port.
> > +             *
> > +             * So, first search in lr_ports hmap.  If not found, search in
> > +             * ls_ports hmap.
> > +             * */
> > +            op = ovn_port_find(lr_ports, pb->logical_port);
> >          }
> >
> > -        struct ovn_port *op = ovn_port_find(ls_ports, pb->logical_port);
> > -        if (op && !op->lsp_can_be_inc_processed) {
> > -            return false;
> > +        if (!op) {
> > +            op = ovn_port_find(ls_ports, pb->logical_port);
> > +
> > +            if (op) {
> > +                is_router_port = false;
> > +            }
> >          }
> >
> >          if (sbrec_port_binding_is_new(pb)) {
> > @@ -5693,7 +5734,8 @@ northd_handle_sb_port_binding_changes(
> >               * pointer in northd data. Fallback to recompute otherwise. */
> >              if (!op) {
> >                  VLOG_WARN_RL(&rl, "A port-binding for %s is created but the "
> > -                            "LSP is not found.", pb->logical_port);
> > +                            "%s is not found.", pb->logical_port,
> > +                            is_router_port ? "LRP" : "LSP");
> >                  return false;
> >              }
> >              op->sb = pb;
> > @@ -5703,18 +5745,29 @@ northd_handle_sb_port_binding_changes(
> >               * case. Fallback to recompute otherwise, to avoid dangling
> >               * sb idl pointers and other unexpected behavior. */
> >              if (op && op->sb == pb) {
> > -                VLOG_WARN_RL(&rl, "A port-binding for %s is deleted but "
> > -                            "the LSP still exists.", pb->logical_port);
> > +                VLOG_WARN_RL(&rl, "A port-binding for %s is deleted but the "
> > +                            "LSP/LRP still exists.", pb->logical_port);
> >                  return false;
> >              }
> >          } else {
> > -            /* The PB is updated, most likely because of binding/unbinding
> > -             * to/from a chassis, and we can ignore the change (updating NB
> > -             * "up" will be handled in the engine node "sync_from_sb").
> > +            /* The PB is updated.
> > +             * For an LSP PB it is most likely because of
> > +             * binding/unbinding to/from a chassis, and we can ignore the
> > +             * change (updating NB "up" will be handled in the engine node
> > +             * "sync_from_sb").
> > +             *
> > +             * For an LRP PB, it is most likely because of
> > +             *   - IPv6 prefix delagation updates from ovn-controller.
> > +             *     This update is handled in "sync_from_sb" node.
> > +             *   - ha chassis group and this can be ignored.
> > +             *
> > +             * All other changes can be ignored.
> > +             *
> >               * Fallback to recompute for anything unexpected. */
> >              if (!op) {
> >                  VLOG_WARN_RL(&rl, "A port-binding for %s is updated but the "
> > -                            "LSP is not found.", pb->logical_port);
> > +                            "%s is not found.", pb->logical_port,
> > +                            is_router_port ? "LRP" : "LSP");
> >                  return false;
> >              }
> >              if (op->sb != pb) {
> > @@ -7855,67 +7908,70 @@ static void
> >  copy_ra_to_sb(struct ovn_port *op, const char *address_mode);
> >
> >  static void
> > -ovn_update_ipv6_options(struct hmap *lr_ports)
> > +ovn_update_ipv6_opt_for_op(struct ovn_port *op)
> >  {
> > -    struct ovn_port *op;
> > -    HMAP_FOR_EACH (op, key_node, lr_ports) {
> > -        ovs_assert(op->nbrp);
> > -
> > -        if (op->nbrp->peer || !op->peer) {
> > -            continue;
> > -        }
> > +    if (op->nbrp->peer || !op->peer) {
> > +        return;
> > +    }
> >
> > -        if (!op->lrp_networks.n_ipv6_addrs) {
> > -            continue;
> > -        }
> > +    if (!op->lrp_networks.n_ipv6_addrs) {
> > +        return;
> > +    }
> >
> > -        struct smap options;
> > -        smap_clone(&options, &op->sb->options);
> > +    struct smap options;
> > +    smap_clone(&options, &op->sb->options);
> >
> > -        /* enable IPv6 prefix delegation */
> > -        bool prefix_delegation = smap_get_bool(&op->nbrp->options,
> > +    /* enable IPv6 prefix delegation */
> > +    bool prefix_delegation = smap_get_bool(&op->nbrp->options,
> >                                             "prefix_delegation", false);
> > -        if (!lrport_is_enabled(op->nbrp)) {
> > -            prefix_delegation = false;
> > -        }
> > -        if (smap_get_bool(&options, "ipv6_prefix_delegation",
> > -                          false) != prefix_delegation) {
> > -            smap_add(&options, "ipv6_prefix_delegation",
> > -                     prefix_delegation ? "true" : "false");
> > -        }
> > +    if (!lrport_is_enabled(op->nbrp)) {
> > +        prefix_delegation = false;
> > +    }
> > +    if (smap_get_bool(&options, "ipv6_prefix_delegation",
> > +                      false) != prefix_delegation) {
> > +        smap_add(&options, "ipv6_prefix_delegation",
> > +                 prefix_delegation ? "true" : "false");
> > +    }
> >
> > -        bool ipv6_prefix = smap_get_bool(&op->nbrp->options,
> > +    bool ipv6_prefix = smap_get_bool(&op->nbrp->options,
> >                                       "prefix", false);
>
> Nit: this fits on a single line now.
>
> > -        if (!lrport_is_enabled(op->nbrp)) {
> > -            ipv6_prefix = false;
> > -        }
> > -        if (smap_get_bool(&options, "ipv6_prefix", false) != ipv6_prefix) {
> > -            smap_add(&options, "ipv6_prefix",
> > -                     ipv6_prefix ? "true" : "false");
> > -        }
> > -        sbrec_port_binding_set_options(op->sb, &options);
> > +    if (!lrport_is_enabled(op->nbrp)) {
> > +        ipv6_prefix = false;
> > +    }
> > +    if (smap_get_bool(&options, "ipv6_prefix", false) != ipv6_prefix) {
> > +        smap_add(&options, "ipv6_prefix", ipv6_prefix ? "true" : "false");
> > +    }
> > +    sbrec_port_binding_set_options(op->sb, &options);
> >
> > -        smap_destroy(&options);
> > +    smap_destroy(&options);
> >
> > -        const char *address_mode = smap_get(
> > -            &op->nbrp->ipv6_ra_configs, "address_mode");
> > +    const char *address_mode = smap_get(&op->nbrp->ipv6_ra_configs,
> > +                                        "address_mode");
> >
> > -        if (!address_mode) {
> > -            continue;
> > -        }
> > -        if (strcmp(address_mode, "slaac") &&
> > -            strcmp(address_mode, "dhcpv6_stateful") &&
> > -            strcmp(address_mode, "dhcpv6_stateless")) {
> > -            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > -            VLOG_WARN_RL(&rl, "Invalid address mode [%s] defined",
> > -                         address_mode);
> > -            continue;
> > -        }
> > +    if (!address_mode) {
> > +        return;
> > +    }
> > +    if (strcmp(address_mode, "slaac") &&
> > +        strcmp(address_mode, "dhcpv6_stateful") &&
> > +        strcmp(address_mode, "dhcpv6_stateless")) {
> > +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > +        VLOG_WARN_RL(&rl, "Invalid address mode [%s] defined",
> > +                     address_mode);
> > +        return;
> > +    }
> >
> > -        if (smap_get_bool(&op->nbrp->ipv6_ra_configs, "send_periodic",
> > -                          false)) {
> > -            copy_ra_to_sb(op, address_mode);
> > -        }
> > +    if (smap_get_bool(&op->nbrp->ipv6_ra_configs, "send_periodic", false)) {
> > +        copy_ra_to_sb(op, address_mode);
> > +    }
> > +}
> > +
> > +static void
> > +ovn_update_ipv6_options(struct hmap *lr_ports)
> > +{
> > +    struct ovn_port *op;
> > +    HMAP_FOR_EACH (op, key_node, lr_ports) {
> > +        ovs_assert(op->nbrp);
>
> I would move this assert inside ovn_update_ipv6_opt_for_op() just in
> case we ever want to call it in multiple places in the future.
>
> > +        ovn_update_ipv6_opt_for_op(op);
> >      }
> >  }
> >
> > @@ -18007,8 +18063,6 @@ ovnnb_db_run(struct northd_input *input_data,
> >          &data->lr_ports);
> >      stopwatch_stop(BUILD_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
> >      stopwatch_start(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
> > -    ovn_update_ipv6_options(&data->lr_ports);
> > -    ovn_update_ipv6_prefix(&data->lr_ports);
> >
> >      sync_mirrors(ovnsb_txn, input_data->nbrec_mirror_table,
> >                   input_data->sbrec_mirror_table);
> > @@ -18339,6 +18393,8 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
> >                                         &ha_ref_chassis_map);
> >      }
> >      shash_destroy(&ha_ref_chassis_map);
> > +
> > +    ovn_update_ipv6_prefix(lr_ports);
> >  }
> >
> >  const char *
> > diff --git a/northd/northd.h b/northd/northd.h
> > index 23521065e8..233dca8084 100644
> > --- a/northd/northd.h
> > +++ b/northd/northd.h
> > @@ -364,7 +364,8 @@ bool lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn,
> >                                        struct lflow_input *,
> >                                        struct hmap *lflows);
> >  bool northd_handle_sb_port_binding_changes(
> > -    const struct sbrec_port_binding_table *, struct hmap *ls_ports);
> > +    const struct sbrec_port_binding_table *, struct hmap *ls_ports,
> > +    struct hmap *lr_ports);
> >
> >  struct tracked_lb_data;
> >  bool northd_handle_lb_data_changes(struct tracked_lb_data *,
> > @@ -394,7 +395,8 @@ void sync_lbs(struct ovsdb_idl_txn *, const struct sbrec_load_balancer_table *,
> >                struct chassis_features *chassis_features);
> >  bool check_sb_lb_duplicates(const struct sbrec_load_balancer_table *);
> >
> > -void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports);
> > +void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports,
> > +              struct hmap *lr_ports);
> >  bool sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports *);
> >
> >  static inline bool
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 4a02dacf60..4edad24e53 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -10073,7 +10073,7 @@ check ovn-nbctl --wait=hv lsp-add ls0 lsp0-0 -- lsp-set-addresses lsp0-0 "unknow
> >  ovs-vsctl add-port br-int lsp0-0 -- set interface lsp0-0 external_ids:iface-id=lsp0-0
> >  wait_for_ports_up
> >  check ovn-nbctl --wait=hv sync
> > -check_recompute_counter 4 5 5 5 5 5
> > +check_recompute_counter 3 4 3 4 3 4
> >
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >  check ovn-nbctl --wait=hv lsp-add ls0 lsp0-1 -- lsp-set-addresses lsp0-1 "aa:aa:aa:00:00:01 192.168.0.11"
> > @@ -10235,6 +10235,126 @@ OVN_CLEANUP([hv1])
> >  AT_CLEANUP
> >  ])
> >
> > +OVN_FOR_EACH_NORTHD_NO_HV([
> > +AT_SETUP([SB Port binding incremental processing])
> > +ovn_start
> > +
> > +check_recompute_counter() {
> > +    northd_recomp=$(as northd ovn-appctl -t ovn-northd inc-engine/show-stats northd recompute)
> > +    echo "northd recompute count - $northd_recomp"
> > +    AT_CHECK([test x$northd_recomp = x$1])
> > +
> > +    lflow_recomp=$(as northd ovn-appctl -t ovn-northd inc-engine/show-stats lflow recompute)
> > +    echo "lflow recompute count - $lflow_recomp"
> > +    AT_CHECK([test x$lflow_recomp = x$2])
> > +}
> > +
> > +net_add n1
> > +sim_add hv1
> > +as hv1
> > +ovs-vsctl add-br br-phys
> > +ovn_attach n1 br-phys 192.168.0.11
> > +
> > +check ovn-nbctl --wait=sb ls-add ls0
> > +check ovn-nbctl --wait=sb lr-add lr0
> > +
> > +# Test normal VIF port
> > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > +check ovn-nbctl lsp-add ls0 p1
> > +check_recompute_counter 0 0
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +
> > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > +check ovn-nbctl lsp-set-addresses p1 "00:00:00:00:01:01 10.0.0.3"
> > +check_recompute_counter 0 0
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +
> > +check as northd ovn-appctl -t ovn-northd vlog/set info
> > +
> > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > +check ovn-sbctl lsp-bind p1 hv1
> > +check_recompute_counter 0 0
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +
> > +# Test lsp of type router
> > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > +check ovn-nbctl lsp-add ls0 rp -- lsp-set-type rp router
> > +
> > +# northd engine recomputes twice. Both the times for handling NB logical switch port
> > +# changes and not because of SB port binding changes.  This is because ovn-northd
> > +# sets the "up" to true.
> > +check_recompute_counter 2 2
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +
> > +# Set some options to 'rp'.  northd should only recompute once.
> > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > +check ovn-nbctl lsp-set-options rp foo=bar
> > +check_recompute_counter 1 1
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +
> > +# Test lsp of type external
> > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > +check ovn-nbctl lsp-add ls0 e1 -- lsp-set-type e1 external
> > +check_recompute_counter 2 2
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +
> > +# Set some options to 'e1'.  northd should only recompute once.
> > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > +check ovn-nbctl lsp-set-options e1 foo=bar
> > +check_recompute_counter 1 1
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +
> > +# Test lrp
> > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > +check ovn-nbctl lrp-add lr0 lrp 00:00:02:01:02:03 10.0.0.1/24
> > +check_recompute_counter 1 1
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +
> > +# Set some options on 'lrp'.  northd should only recompute once.
> > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > +check ovn-nbctl lrp-set-options lrp route_table=rtb-1
> > +check_recompute_counter 1 1
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +
> > +# Make lrp a gateway port
> > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > +check ovn-nbctl lrp-set-gateway-chassis lrp hv1
> > +# There will be 3 recomputes of northd engine node
> > +#   1. missing handler for input NB_logical_router
> > +#   2. missing handler for input SB_ha_chassis_group
> > +#   3. missing handler for input NB_logical_router when ovn-northd
> > +#      updates the hosting-chassis option in NB_logical_router_port.
> > +check_recompute_counter 3 3
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +
> > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > +check ovn-nbctl clear logical_router_port lrp gateway_chassis
> > +# There will be 2 recomputes of northd engine node
> > +#   1. missing handler for input NB_logical_router
> > +#   2. missing handler for input SB_ha_chassis_group
> > +check_recompute_counter 2 2
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +
> > +# Delete some of the ports.
> > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > +check ovn-nbctl lsp-del p1
> > +check_recompute_counter 0 0
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +
> > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > +check ovn-nbctl lsp-del e1
> > +check_recompute_counter 1 1
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +
> > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > +check ovn-nbctl lrp-del lrp
> > +check_recompute_counter 1 1
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +
> > +OVN_CLEANUP([hv1])
> > +AT_CLEANUP
> > +])
> > +
> >  OVN_FOR_EACH_NORTHD_NO_HV([
> >  AT_SETUP([ACL/Meter incremental processing - no northd recompute])
> >  ovn_start
> > @@ -10269,6 +10389,7 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE(1)
> >  AT_CLEANUP
> >  ])
> >
> > +
> >  OVN_FOR_EACH_NORTHD_NO_HV([
> >  AT_SETUP([check fip and lb flows])
> >  AT_KEYWORDS([fip-lb-flows])
> > @@ -10471,7 +10592,7 @@ ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >  ovn-nbctl --wait=sb lsp-set-options sw0-lr0 router-port=lr0-sw0
> >  check_engine_stats lb_data norecompute compute
> > -check_engine_stats northd recompute nocompute
> > +check_engine_stats northd recompute compute
> >  check_engine_stats lflow recompute nocompute
> >  check_engine_stats sync_to_sb_lb recompute nocompute
> >
> > @@ -11114,12 +11235,14 @@ ovn_attach n1 br-phys 192.168.0.11
> >  ovn-sbctl chassis-add gw1 geneve 127.0.0.1
> >
> >  check ovn-nbctl ls-add sw0
> > -check ovn-nbctl lsp-add sw0 sw0p1 -- lsp-set-addresses sw0p1 "00:00:20:20:12:01 10.0.0.4"
> > +check ovn-nbctl --wait=sb lsp-add sw0 sw0p1 -- lsp-set-addresses sw0p1 "00:00:20:20:12:01 10.0.0.4"
> >
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >  check ovn-nbctl --wait=sb lr-add lr0
> >  check_engine_stats northd recompute nocompute
> > +check_engine_stats sync_to_sb_pb recompute nocompute
> >  check_engine_stats lflow recompute nocompute
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  # Adding a logical router port should result in recompute
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > @@ -11127,16 +11250,20 @@ check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24
> >  # for northd engine there will be both recompute and compute
> >  # first it will be recompute to handle lr0-sw0 and then a compute
> >  # for the SB port binding change.
> > -check_engine_stats northd recompute nocompute
> > +check_engine_stats northd recompute compute
> > +check_engine_stats sync_to_sb_pb recompute nocompute
> >  check_engine_stats lflow recompute nocompute
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  ovn-nbctl lsp-add sw0 sw0-lr0
> >  ovn-nbctl lsp-set-type sw0-lr0 router
> >  ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >  check ovn-nbctl --wait=sb lsp-set-options sw0-lr0 router-port=lr0-sw0
> > -check_engine_stats northd recompute nocompute
> > +check_engine_stats northd recompute compute
> > +check_engine_stats sync_to_sb_pb recompute nocompute
> >  check_engine_stats lflow recompute nocompute
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  ovn-nbctl ls-add public
> >  ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24
> > @@ -11158,7 +11285,9 @@ ovn-nbctl --wait=hv lrp-set-gateway-chassis lr0-public hv1 20
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >  check ovn-nbctl set logical_router_port lr0-sw0 options:foo=bar
> >  check_engine_stats northd recompute nocompute
> > +check_engine_stats sync_to_sb_pb recompute nocompute
> >  check_engine_stats lflow recompute nocompute
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  # Do checks for NATs.
> >  # Add a NAT. This should not result in recompute of both northd and lflow
> > @@ -11167,6 +11296,7 @@ check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >  check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat  172.168.0.110 10.0.0.4
> >  check_engine_stats northd recompute nocompute
> >  check_engine_stats lflow recompute nocompute
> > +check_engine_stats sync_to_sb_pb recompute nocompute
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  # Update the NAT options column
> > @@ -11174,6 +11304,7 @@ check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >  check ovn-nbctl --wait=sb set NAT . options:foo=bar
> >  check_engine_stats northd recompute nocompute
> >  check_engine_stats lflow recompute nocompute
> > +check_engine_stats sync_to_sb_pb recompute nocompute
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  # Update the NAT external_ip column
> > @@ -11181,6 +11312,7 @@ check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >  check ovn-nbctl --wait=sb set NAT . external_ip=172.168.0.120
> >  check_engine_stats northd recompute nocompute
> >  check_engine_stats lflow recompute nocompute
> > +check_engine_stats sync_to_sb_pb recompute nocompute
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  # Update the NAT logical_ip column
> > @@ -11188,6 +11320,7 @@ check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >  check ovn-nbctl --wait=sb set NAT . logical_ip=10.0.0.10
> >  check_engine_stats northd recompute nocompute
> >  check_engine_stats lflow recompute nocompute
> > +check_engine_stats sync_to_sb_pb recompute nocompute
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  # Update the NAT type
> > @@ -11195,13 +11328,15 @@ check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >  check ovn-nbctl --wait=sb set NAT . type=snat
> >  check_engine_stats northd recompute nocompute
> >  check_engine_stats lflow recompute nocompute
> > +check_engine_stats sync_to_sb_pb recompute nocompute
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  # Create a dnat_and_snat NAT with external_mac and logical_port
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >  check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.110 10.0.0.4 sw0p1 30:54:00:00:00:03
> > -check_engine_stats northd recompute nocompute
> > +check_engine_stats northd recompute compute
> >  check_engine_stats lflow recompute nocompute
> > +check_engine_stats sync_to_sb_pb recompute nocompute
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  nat2_uuid=$(ovn-nbctl --bare --columns _uuid find nat logical_ip=10.0.0.4)
> > @@ -11210,6 +11345,7 @@ check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >  check ovn-nbctl --wait=sb set NAT $nat2_uuid external_mac='"30:54:00:00:00:04"'
> >  check_engine_stats northd recompute nocompute
> >  check_engine_stats lflow recompute nocompute
> > +check_engine_stats sync_to_sb_pb recompute nocompute
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  # Create a load balancer and add the lb vip as NAT
> > @@ -11223,31 +11359,35 @@ check ovn-nbctl lr-lb-add lr0 lb2
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >  check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.140 10.0.0.20
> >  check_engine_stats northd recompute nocompute
> > +check_engine_stats sync_to_sb_pb recompute nocompute
> >  check_engine_stats lflow recompute nocompute
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >  check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.150 10.0.0.41
> >  check_engine_stats northd recompute nocompute
> > +check_engine_stats sync_to_sb_pb recompute nocompute
> >  check_engine_stats lflow recompute nocompute
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >  check ovn-nbctl --wait=sb lr-nat-del lr0 dnat_and_snat 172.168.0.150
> >  check_engine_stats northd recompute nocompute
> > +check_engine_stats sync_to_sb_pb recompute nocompute
> >  check_engine_stats lflow recompute nocompute
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >  check ovn-nbctl --wait=sb lr-nat-del lr0 dnat_and_snat 172.168.0.140
> >  check_engine_stats northd recompute nocompute
> > +check_engine_stats sync_to_sb_pb recompute nocompute
> >  check_engine_stats lflow recompute nocompute
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  # Delete the NAT
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >  check ovn-nbctl --wait=sb clear logical_router lr0 nat
> > -check_engine_stats northd recompute nocompute
> > +check_engine_stats northd recompute compute
> >  check_engine_stats lflow recompute nocompute
> >  check_engine_stats sync_to_sb_pb recompute nocompute
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > @@ -11256,12 +11396,16 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >  check ovn-nbctl --wait=sb lr-policy-add lr0  10 "ip4.src == 10.0.0.3" reroute 172.168.0.101,172.168.0.102
> >  check_engine_stats northd recompute nocompute
> > +check_engine_stats sync_to_sb_pb recompute nocompute
> >  check_engine_stats lflow recompute nocompute
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >  check ovn-nbctl --wait=sb lr-policy-del lr0  10 "ip4.src == 10.0.0.3"
> >  check_engine_stats northd recompute nocompute
> > +check_engine_stats sync_to_sb_pb recompute nocompute
> >  check_engine_stats lflow recompute nocompute
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  OVN_CLEANUP([hv1])
> >  AT_CLEANUP
>
> The rest looks good to me but I'd prefer we figure out the test failure
> before acking the patch.
>
> Regards,
> Dumitru
>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On 1/25/24 17:46, Numan Siddique wrote:
> On Tue, Jan 16, 2024 at 6:15 AM Dumitru Ceara <dceara@redhat.com> wrote:
>>
>> On 1/11/24 16:28, numans@ovn.org wrote:
>>> From: Numan Siddique <numans@ovn.org>
>>>
>>> It also moves the logical router port IPv6 prefix delegation
>>> updates to "sync-from-sb" engine node.
>>>
>>> With these changes, northd engine node doesn't need to do much
>>> for SB Port binding changes other than updating 'op->sb'.  And
>>> there is no need to fall back to recompute.  Prior to this patch
>>> northd_handle_sb_port_binding_changes() was returning false for
>>> all SB port binding updates except for the VIF PBs.  This patch
>>> now handles updates to all the SB port binding by setting 'op->sb'.
>>>
>>> Signed-off-by: Numan Siddique <numans@ovn.org>
>>> ---
>>
>> Hi Numan,
>>
>> With this patch applied the "633: ovn-northd.at:10298 SB Port binding
>> incremental processing -- parallelization=yes" test occasionally fails,
>> e.g.:
>>
>> https://github.com/dceara/ovn/actions/runs/7539663594/job/20524238508
>>
>> I saw it locally too when running the test in a loop:
>>
>> It fails with:
>> ovn-nbctl lrp-set-gateway-chassis lrp hv1
>> ./ovn-macros.at:449: "$@"
>> northd recompute count - 2
>> ./ovn-northd.at:10238: test x$northd_recomp = x$1
>> ./ovn-northd.at:10238: exit code was 1, expected 0
>>
>> Here:
>> https://github.com/dceara/ovn/blob/e1c76b6d347570618591fe6587f6af83445223ec/tests/ovn-northd.at#L10327
>>
>> For some reason in some cases only 2 recomputes get triggered.
> 
> Thanks Dumitru and Han for the reviews.
> 
> @Dumitru Ceara  I've addressed all the review comments below.
> 
> I think the test case is failing because of the missing "--wait=sb" in
> the ovn-nbctl commands.
> 

I don't think it's only that.  It still fails even with your updated
test changes, in the same way:

as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
./ovn-macros.at:449: "$@"
ovn-nbctl lrp-set-gateway-chassis lrp hv1
./ovn-macros.at:449: "$@"
northd recompute count - 2
./ovn-northd.at:10238: test x$northd_recomp = x$1
./ovn-northd.at:10238: exit code was 1, expected 0

> Will the below changes in ovn-northd.at look good to you  ?  If so,
> and if you can provide your ack I can apply this patch.
> Otherwise I'll include it in the v6.
> 

I would prefer if we don't apply it until we figure out this test, the
CI has been relatively stable the last few weeks, I'd rather not
destabilize it if possible.

Regards,
Dumitru

> ----
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 72eb9e1173..492248b05d 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -10316,12 +10316,12 @@ check ovn-nbctl --wait=sb lr-add lr0
> 
>  # Test normal VIF port
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> -check ovn-nbctl lsp-add ls0 p1
> +check ovn-nbctl --wait=sb lsp-add ls0 p1
>  check_recompute_counter 0 0
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> 
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> -check ovn-nbctl lsp-set-addresses p1 "00:00:00:00:01:01 10.0.0.3"
> +check ovn-nbctl --wait=sb lsp-set-addresses p1 "00:00:00:00:01:01 10.0.0.3"
>  check_recompute_counter 0 0
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> 
> @@ -10329,12 +10329,13 @@ check as northd ovn-appctl -t ovn-northd vlog/set info
> 
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>  check ovn-sbctl lsp-bind p1 hv1
> +check ovn-nbctl --wait=sb sync
>  check_recompute_counter 0 0
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> 
>  # Test lsp of type router
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> -check ovn-nbctl lsp-add ls0 rp -- lsp-set-type rp router
> +check ovn-nbctl --wait=sb lsp-add ls0 rp -- lsp-set-type rp router
> 
>  # northd engine recomputes twice. Both the times for handling NB
> logical switch port
>  # changes and not because of SB port binding changes.  This is
> because ovn-northd
> @@ -10344,37 +10345,37 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
> 
>  # Set some options to 'rp'.  northd should only recompute once.
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> -check ovn-nbctl lsp-set-options rp foo=bar
> +check ovn-nbctl --wait=sb lsp-set-options rp foo=bar
>  check_recompute_counter 1 1
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> 
>  # Test lsp of type external
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> -check ovn-nbctl lsp-add ls0 e1 -- lsp-set-type e1 external
> +check ovn-nbctl --wait=sb lsp-add ls0 e1 -- lsp-set-type e1 external
>  check_recompute_counter 2 2
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> 
>  # Set some options to 'e1'.  northd should only recompute once.
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> -check ovn-nbctl lsp-set-options e1 foo=bar
> +check ovn-nbctl --wait=sb lsp-set-options e1 foo=bar
>  check_recompute_counter 1 1
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> 
>  # Test lrp
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> -check ovn-nbctl lrp-add lr0 lrp 00:00:02:01:02:03 10.0.0.1/24
> +check ovn-nbctl --wait=sb lrp-add lr0 lrp 00:00:02:01:02:03 10.0.0.1/24
>  check_recompute_counter 1 1
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> 
>  # Set some options on 'lrp'.  northd should only recompute once.
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> -check ovn-nbctl lrp-set-options lrp route_table=rtb-1
> +check ovn-nbctl --wait=sb lrp-set-options lrp route_table=rtb-1
>  check_recompute_counter 1 1
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> 
>  # Make lrp a gateway port
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> -check ovn-nbctl lrp-set-gateway-chassis lrp hv1
> +check ovn-nbctl --wait=sb lrp-set-gateway-chassis lrp hv1
>  # There will be 3 recomputes of northd engine node
>  #   1. missing handler for input NB_logical_router
>  #   2. missing handler for input SB_ha_chassis_group
> @@ -10384,7 +10385,7 @@ check_recompute_counter 3 3
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> 
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> -check ovn-nbctl clear logical_router_port lrp gateway_chassis
> +check ovn-nbctl --wait=sb clear logical_router_port lrp gateway_chassis
>  # There will be 2 recomputes of northd engine node
>  #   1. missing handler for input NB_logical_router
>  #   2. missing handler for input SB_ha_chassis_group
> @@ -10393,17 +10394,17 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
> 
>  # Delete some of the ports.
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> -check ovn-nbctl lsp-del p1
> +check ovn-nbctl --wait=sb lsp-del p1
>  check_recompute_counter 0 0
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> 
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> -check ovn-nbctl lsp-del e1
> +check ovn-nbctl --wait=sb lsp-del e1
>  check_recompute_counter 1 1
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> 
>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> -check ovn-nbctl lrp-del lrp
> +check ovn-nbctl --wait=sb lrp-del lrp
>  check_recompute_counter 1 1
>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> --------------------------------
> 
> Thanks
> Numan
> 
> 
> 
>>
>>>  northd/en-northd.c  |   2 +-
>>>  northd/en-sync-sb.c |   3 +-
>>>  northd/northd.c     | 296 ++++++++++++++++++++++++++------------------
>>>  northd/northd.h     |   6 +-
>>>  tests/ovn-northd.at | 158 +++++++++++++++++++++--
>>>  5 files changed, 334 insertions(+), 131 deletions(-)
>>>
>>> diff --git a/northd/en-northd.c b/northd/en-northd.c
>>> index 28559ed211..677b2b1ab0 100644
>>> --- a/northd/en-northd.c
>>> +++ b/northd/en-northd.c
>>> @@ -189,7 +189,7 @@ northd_sb_port_binding_handler(struct engine_node *node,
>>>      northd_get_input_data(node, &input_data);
>>>
>>>      if (!northd_handle_sb_port_binding_changes(
>>> -        input_data.sbrec_port_binding_table, &nd->ls_ports)) {
>>> +        input_data.sbrec_port_binding_table, &nd->ls_ports, &nd->lr_ports)) {
>>>          return false;
>>>      }
>>>
>>> diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
>>> index 3aaab8d005..45be7ddbcb 100644
>>> --- a/northd/en-sync-sb.c
>>> +++ b/northd/en-sync-sb.c
>>> @@ -288,7 +288,8 @@ en_sync_to_sb_pb_run(struct engine_node *node, void *data OVS_UNUSED)
>>>      const struct engine_context *eng_ctx = engine_get_context();
>>>      struct northd_data *northd_data = engine_get_input_data("northd", node);
>>>
>>> -    sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports);
>>> +    sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports,
>>> +             &northd_data->lr_ports);
>>>      engine_set_node_state(node, EN_UPDATED);
>>>  }
>>>
>>> diff --git a/northd/northd.c b/northd/northd.c
>>> index f32e3bf21a..23f2dae26b 100644
>>> --- a/northd/northd.c
>>> +++ b/northd/northd.c
>>> @@ -3435,6 +3435,9 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
>>>  {
>>>      sbrec_port_binding_set_datapath(op->sb, op->od->sb);
>>>      if (op->nbrp) {
>>> +        /* Note: SB port binding options for router ports are set in
>>> +         * sync_pbs(). */
>>> +
>>>          /* If the router is for l3 gateway, it resides on a chassis
>>>           * and its port type is "l3gateway". */
>>>          const char *chassis_name = smap_get(&op->od->nbr->options, "chassis");
>>> @@ -3446,15 +3449,11 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
>>>              sbrec_port_binding_set_type(op->sb, "patch");
>>>          }
>>>
>>> -        struct smap new;
>>> -        smap_init(&new);
>>>          if (is_cr_port(op)) {
>>>              ovs_assert(sbrec_chassis_by_name);
>>>              ovs_assert(sbrec_chassis_by_hostname);
>>>              ovs_assert(sbrec_ha_chassis_grp_by_name);
>>>              ovs_assert(active_ha_chassis_grps);
>>> -            const char *redirect_type = smap_get(&op->nbrp->options,
>>> -                                                 "redirect-type");
>>>
>>>              if (op->nbrp->ha_chassis_group) {
>>>                  if (op->nbrp->n_gateway_chassis) {
>>> @@ -3496,49 +3495,8 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
>>>                  /* Delete the legacy gateway_chassis from the pb. */
>>>                  sbrec_port_binding_set_gateway_chassis(op->sb, NULL, 0);
>>>              }
>>> -            smap_add(&new, "distributed-port", op->nbrp->name);
>>> -
>>> -            bool always_redirect =
>>> -                !op->od->has_distributed_nat &&
>>> -                !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port);
>>> -
>>> -            if (redirect_type) {
>>> -                smap_add(&new, "redirect-type", redirect_type);
>>> -                /* XXX Why can't we enable always-redirect when redirect-type
>>> -                 * is bridged? */
>>> -                if (!strcmp(redirect_type, "bridged")) {
>>> -                    always_redirect = false;
>>> -                }
>>> -            }
>>> -
>>> -            if (always_redirect) {
>>> -                smap_add(&new, "always-redirect", "true");
>>> -            }
>>> -        } else {
>>> -            if (op->peer) {
>>> -                smap_add(&new, "peer", op->peer->key);
>>> -                if (op->nbrp->ha_chassis_group ||
>>> -                    op->nbrp->n_gateway_chassis) {
>>> -                    char *redirect_name =
>>> -                        ovn_chassis_redirect_name(op->nbrp->name);
>>> -                    smap_add(&new, "chassis-redirect-port", redirect_name);
>>> -                    free(redirect_name);
>>> -                }
>>> -            }
>>> -            if (chassis_name) {
>>> -                smap_add(&new, "l3gateway-chassis", chassis_name);
>>> -            }
>>>          }
>>>
>>> -        const char *ipv6_pd_list = smap_get(&op->sb->options,
>>> -                                            "ipv6_ra_pd_list");
>>> -        if (ipv6_pd_list) {
>>> -            smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list);
>>> -        }
>>> -
>>> -        sbrec_port_binding_set_options(op->sb, &new);
>>> -        smap_destroy(&new);
>>> -
>>>          sbrec_port_binding_set_parent_port(op->sb, NULL);
>>>          sbrec_port_binding_set_tag(op->sb, NULL, 0);
>>>
>>> @@ -4768,12 +4726,14 @@ check_sb_lb_duplicates(const struct sbrec_load_balancer_table *table)
>>>      return duplicates;
>>>  }
>>>
>>> -/* Syncs the SB port binding for the ovn_port 'op'.  Caller should make sure
>>> - * that the OVN SB IDL txn is not NULL.  Presently it only syncs the nat
>>> - * column of port binding corresponding to the 'op->nbsp' */
>>> +/* Syncs the SB port binding for the ovn_port 'op' of a logical switch port.
>>> + * Caller should make sure that the OVN SB IDL txn is not NULL.  Presently it
>>> + * only syncs the nat column of port binding corresponding to the 'op->nbsp' */
>>>  static void
>>> -sync_pb_for_op(struct ovn_port *op)
>>> +sync_pb_for_lsp(struct ovn_port *op)
>>>  {
>>> +    ovs_assert(op->nbsp);
>>> +
>>>      if (lsp_is_router(op->nbsp)) {
>>>          const char *chassis = NULL;
>>>          if (op->peer && op->peer->od && op->peer->od->nbr) {
>>> @@ -4895,18 +4855,86 @@ sync_pb_for_op(struct ovn_port *op)
>>>      }
>>>  }
>>>
>>> +/* Syncs the SB port binding for the ovn_port 'op' of a logical router port.
>>> + * Caller should make sure that the OVN SB IDL txn is not NULL.  Presently it
>>> + * only sets the port binding options column for the router ports */
>>> +static void
>>> +sync_pb_for_lrp(struct ovn_port *op)
>>> +{
>>> +    ovs_assert(op->nbrp);
>>> +
>>> +    struct smap new;
>>> +    smap_init(&new);
>>> +
>>> +    const char *chassis_name = smap_get(&op->od->nbr->options, "chassis");
>>> +    if (is_cr_port(op)) {
>>> +        smap_add(&new, "distributed-port", op->nbrp->name);
>>> +
>>> +        bool always_redirect =
>>> +            !op->od->has_distributed_nat &&
>>> +            !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port);
>>> +
>>> +        const char *redirect_type = smap_get(&op->nbrp->options,
>>> +                                            "redirect-type");
>>> +        if (redirect_type) {
>>> +            smap_add(&new, "redirect-type", redirect_type);
>>> +            /* XXX Why can't we enable always-redirect when redirect-type
>>> +                * is bridged? */
>>
>> Nit: indentation
>>
>>> +            if (!strcmp(redirect_type, "bridged")) {
>>> +                always_redirect = false;
>>> +            }
>>> +        }
>>> +
>>> +        if (always_redirect) {
>>> +            smap_add(&new, "always-redirect", "true");
>>> +        }
>>> +    } else {
>>> +        if (op->peer) {
>>> +            smap_add(&new, "peer", op->peer->key);
>>> +            if (op->nbrp->ha_chassis_group ||
>>> +                op->nbrp->n_gateway_chassis) {
>>> +                char *redirect_name =
>>> +                    ovn_chassis_redirect_name(op->nbrp->name);
>>> +                smap_add(&new, "chassis-redirect-port", redirect_name);
>>> +                free(redirect_name);
>>> +            }
>>> +        }
>>> +        if (chassis_name) {
>>> +            smap_add(&new, "l3gateway-chassis", chassis_name);
>>> +        }
>>> +    }
>>> +
>>> +    const char *ipv6_pd_list = smap_get(&op->sb->options, "ipv6_ra_pd_list");
>>> +    if (ipv6_pd_list) {
>>> +        smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list);
>>> +    }
>>> +
>>> +    sbrec_port_binding_set_options(op->sb, &new);
>>> +    smap_destroy(&new);
>>> +}
>>> +
>>> +static void ovn_update_ipv6_options(struct hmap *lr_ports);
>>> +static void ovn_update_ipv6_opt_for_op(struct ovn_port *op);
>>> +
>>>  /* Sync the SB Port bindings which needs to be updated.
>>>   * Presently it syncs the nat column of port bindings corresponding to
>>>   * the logical switch ports. */
>>>  void
>>> -sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports)
>>> +sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports,
>>> +         struct hmap *lr_ports)
>>>  {
>>>      ovs_assert(ovnsb_idl_txn);
>>>
>>>      struct ovn_port *op;
>>>      HMAP_FOR_EACH (op, key_node, ls_ports) {
>>> -        sync_pb_for_op(op);
>>> +        sync_pb_for_lsp(op);
>>> +    }
>>> +
>>> +    HMAP_FOR_EACH (op, key_node, lr_ports) {
>>> +        sync_pb_for_lrp(op);
>>>      }
>>> +
>>> +    ovn_update_ipv6_options(lr_ports);
>>>  }
>>>
>>>  /* Sync the SB Port bindings for the added and updated logical switch ports
>>> @@ -4918,12 +4946,14 @@ sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports *trk_ovn_ports)
>>>      struct ovn_port *op;
>>>      HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->created) {
>>>          op = hmapx_node->data;
>>> -        sync_pb_for_op(op);
>>> +        ovs_assert(op->nbsp);
>>
>> This is already checked inside sync_pb_for_lsp().  Let's remove it from
>> here.  Then what I mentioned in patch 01/16 still applies, we don't need
>> the local 'op' variable anymore.
>>
>>> +        sync_pb_for_lsp(op);
>>>      }
>>>
>>>      HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->updated) {
>>>          op = hmapx_node->data;
>>> -        sync_pb_for_op(op);
>>> +        ovs_assert(op->nbsp);
>>> +        sync_pb_for_lsp(op);
>>
>> Same here.
>>
>>>      }
>>>
>>>      return true;
>>> @@ -5671,20 +5701,31 @@ fail:
>>>  bool
>>>  northd_handle_sb_port_binding_changes(
>>>      const struct sbrec_port_binding_table *sbrec_port_binding_table,
>>> -    struct hmap *ls_ports)
>>> +    struct hmap *ls_ports, struct hmap *lr_ports)
>>>  {
>>>      const struct sbrec_port_binding *pb;
>>>      static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>>>      SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, sbrec_port_binding_table) {
>>> -        /* Return false if the 'pb' belongs to a router port.  We don't handle
>>> -         * I-P for router ports yet. */
>>> -        if (is_pb_router_type(pb)) {
>>> -            return false;
>>> +        bool is_router_port = is_pb_router_type(pb);
>>> +        struct ovn_port *op = NULL;
>>> +
>>> +        if (is_router_port) {
>>> +            /* A router port binding 'pb' can belong to
>>> +             *   - a logical switch port of type router or
>>> +             *   - a logical router port.
>>> +             *
>>> +             * So, first search in lr_ports hmap.  If not found, search in
>>> +             * ls_ports hmap.
>>> +             * */
>>> +            op = ovn_port_find(lr_ports, pb->logical_port);
>>>          }
>>>
>>> -        struct ovn_port *op = ovn_port_find(ls_ports, pb->logical_port);
>>> -        if (op && !op->lsp_can_be_inc_processed) {
>>> -            return false;
>>> +        if (!op) {
>>> +            op = ovn_port_find(ls_ports, pb->logical_port);
>>> +
>>> +            if (op) {
>>> +                is_router_port = false;
>>> +            }
>>>          }
>>>
>>>          if (sbrec_port_binding_is_new(pb)) {
>>> @@ -5693,7 +5734,8 @@ northd_handle_sb_port_binding_changes(
>>>               * pointer in northd data. Fallback to recompute otherwise. */
>>>              if (!op) {
>>>                  VLOG_WARN_RL(&rl, "A port-binding for %s is created but the "
>>> -                            "LSP is not found.", pb->logical_port);
>>> +                            "%s is not found.", pb->logical_port,
>>> +                            is_router_port ? "LRP" : "LSP");
>>>                  return false;
>>>              }
>>>              op->sb = pb;
>>> @@ -5703,18 +5745,29 @@ northd_handle_sb_port_binding_changes(
>>>               * case. Fallback to recompute otherwise, to avoid dangling
>>>               * sb idl pointers and other unexpected behavior. */
>>>              if (op && op->sb == pb) {
>>> -                VLOG_WARN_RL(&rl, "A port-binding for %s is deleted but "
>>> -                            "the LSP still exists.", pb->logical_port);
>>> +                VLOG_WARN_RL(&rl, "A port-binding for %s is deleted but the "
>>> +                            "LSP/LRP still exists.", pb->logical_port);
>>>                  return false;
>>>              }
>>>          } else {
>>> -            /* The PB is updated, most likely because of binding/unbinding
>>> -             * to/from a chassis, and we can ignore the change (updating NB
>>> -             * "up" will be handled in the engine node "sync_from_sb").
>>> +            /* The PB is updated.
>>> +             * For an LSP PB it is most likely because of
>>> +             * binding/unbinding to/from a chassis, and we can ignore the
>>> +             * change (updating NB "up" will be handled in the engine node
>>> +             * "sync_from_sb").
>>> +             *
>>> +             * For an LRP PB, it is most likely because of
>>> +             *   - IPv6 prefix delagation updates from ovn-controller.
>>> +             *     This update is handled in "sync_from_sb" node.
>>> +             *   - ha chassis group and this can be ignored.
>>> +             *
>>> +             * All other changes can be ignored.
>>> +             *
>>>               * Fallback to recompute for anything unexpected. */
>>>              if (!op) {
>>>                  VLOG_WARN_RL(&rl, "A port-binding for %s is updated but the "
>>> -                            "LSP is not found.", pb->logical_port);
>>> +                            "%s is not found.", pb->logical_port,
>>> +                            is_router_port ? "LRP" : "LSP");
>>>                  return false;
>>>              }
>>>              if (op->sb != pb) {
>>> @@ -7855,67 +7908,70 @@ static void
>>>  copy_ra_to_sb(struct ovn_port *op, const char *address_mode);
>>>
>>>  static void
>>> -ovn_update_ipv6_options(struct hmap *lr_ports)
>>> +ovn_update_ipv6_opt_for_op(struct ovn_port *op)
>>>  {
>>> -    struct ovn_port *op;
>>> -    HMAP_FOR_EACH (op, key_node, lr_ports) {
>>> -        ovs_assert(op->nbrp);
>>> -
>>> -        if (op->nbrp->peer || !op->peer) {
>>> -            continue;
>>> -        }
>>> +    if (op->nbrp->peer || !op->peer) {
>>> +        return;
>>> +    }
>>>
>>> -        if (!op->lrp_networks.n_ipv6_addrs) {
>>> -            continue;
>>> -        }
>>> +    if (!op->lrp_networks.n_ipv6_addrs) {
>>> +        return;
>>> +    }
>>>
>>> -        struct smap options;
>>> -        smap_clone(&options, &op->sb->options);
>>> +    struct smap options;
>>> +    smap_clone(&options, &op->sb->options);
>>>
>>> -        /* enable IPv6 prefix delegation */
>>> -        bool prefix_delegation = smap_get_bool(&op->nbrp->options,
>>> +    /* enable IPv6 prefix delegation */
>>> +    bool prefix_delegation = smap_get_bool(&op->nbrp->options,
>>>                                             "prefix_delegation", false);
>>> -        if (!lrport_is_enabled(op->nbrp)) {
>>> -            prefix_delegation = false;
>>> -        }
>>> -        if (smap_get_bool(&options, "ipv6_prefix_delegation",
>>> -                          false) != prefix_delegation) {
>>> -            smap_add(&options, "ipv6_prefix_delegation",
>>> -                     prefix_delegation ? "true" : "false");
>>> -        }
>>> +    if (!lrport_is_enabled(op->nbrp)) {
>>> +        prefix_delegation = false;
>>> +    }
>>> +    if (smap_get_bool(&options, "ipv6_prefix_delegation",
>>> +                      false) != prefix_delegation) {
>>> +        smap_add(&options, "ipv6_prefix_delegation",
>>> +                 prefix_delegation ? "true" : "false");
>>> +    }
>>>
>>> -        bool ipv6_prefix = smap_get_bool(&op->nbrp->options,
>>> +    bool ipv6_prefix = smap_get_bool(&op->nbrp->options,
>>>                                       "prefix", false);
>>
>> Nit: this fits on a single line now.
>>
>>> -        if (!lrport_is_enabled(op->nbrp)) {
>>> -            ipv6_prefix = false;
>>> -        }
>>> -        if (smap_get_bool(&options, "ipv6_prefix", false) != ipv6_prefix) {
>>> -            smap_add(&options, "ipv6_prefix",
>>> -                     ipv6_prefix ? "true" : "false");
>>> -        }
>>> -        sbrec_port_binding_set_options(op->sb, &options);
>>> +    if (!lrport_is_enabled(op->nbrp)) {
>>> +        ipv6_prefix = false;
>>> +    }
>>> +    if (smap_get_bool(&options, "ipv6_prefix", false) != ipv6_prefix) {
>>> +        smap_add(&options, "ipv6_prefix", ipv6_prefix ? "true" : "false");
>>> +    }
>>> +    sbrec_port_binding_set_options(op->sb, &options);
>>>
>>> -        smap_destroy(&options);
>>> +    smap_destroy(&options);
>>>
>>> -        const char *address_mode = smap_get(
>>> -            &op->nbrp->ipv6_ra_configs, "address_mode");
>>> +    const char *address_mode = smap_get(&op->nbrp->ipv6_ra_configs,
>>> +                                        "address_mode");
>>>
>>> -        if (!address_mode) {
>>> -            continue;
>>> -        }
>>> -        if (strcmp(address_mode, "slaac") &&
>>> -            strcmp(address_mode, "dhcpv6_stateful") &&
>>> -            strcmp(address_mode, "dhcpv6_stateless")) {
>>> -            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>>> -            VLOG_WARN_RL(&rl, "Invalid address mode [%s] defined",
>>> -                         address_mode);
>>> -            continue;
>>> -        }
>>> +    if (!address_mode) {
>>> +        return;
>>> +    }
>>> +    if (strcmp(address_mode, "slaac") &&
>>> +        strcmp(address_mode, "dhcpv6_stateful") &&
>>> +        strcmp(address_mode, "dhcpv6_stateless")) {
>>> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>>> +        VLOG_WARN_RL(&rl, "Invalid address mode [%s] defined",
>>> +                     address_mode);
>>> +        return;
>>> +    }
>>>
>>> -        if (smap_get_bool(&op->nbrp->ipv6_ra_configs, "send_periodic",
>>> -                          false)) {
>>> -            copy_ra_to_sb(op, address_mode);
>>> -        }
>>> +    if (smap_get_bool(&op->nbrp->ipv6_ra_configs, "send_periodic", false)) {
>>> +        copy_ra_to_sb(op, address_mode);
>>> +    }
>>> +}
>>> +
>>> +static void
>>> +ovn_update_ipv6_options(struct hmap *lr_ports)
>>> +{
>>> +    struct ovn_port *op;
>>> +    HMAP_FOR_EACH (op, key_node, lr_ports) {
>>> +        ovs_assert(op->nbrp);
>>
>> I would move this assert inside ovn_update_ipv6_opt_for_op() just in
>> case we ever want to call it in multiple places in the future.
>>
>>> +        ovn_update_ipv6_opt_for_op(op);
>>>      }
>>>  }
>>>
>>> @@ -18007,8 +18063,6 @@ ovnnb_db_run(struct northd_input *input_data,
>>>          &data->lr_ports);
>>>      stopwatch_stop(BUILD_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
>>>      stopwatch_start(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
>>> -    ovn_update_ipv6_options(&data->lr_ports);
>>> -    ovn_update_ipv6_prefix(&data->lr_ports);
>>>
>>>      sync_mirrors(ovnsb_txn, input_data->nbrec_mirror_table,
>>>                   input_data->sbrec_mirror_table);
>>> @@ -18339,6 +18393,8 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
>>>                                         &ha_ref_chassis_map);
>>>      }
>>>      shash_destroy(&ha_ref_chassis_map);
>>> +
>>> +    ovn_update_ipv6_prefix(lr_ports);
>>>  }
>>>
>>>  const char *
>>> diff --git a/northd/northd.h b/northd/northd.h
>>> index 23521065e8..233dca8084 100644
>>> --- a/northd/northd.h
>>> +++ b/northd/northd.h
>>> @@ -364,7 +364,8 @@ bool lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn,
>>>                                        struct lflow_input *,
>>>                                        struct hmap *lflows);
>>>  bool northd_handle_sb_port_binding_changes(
>>> -    const struct sbrec_port_binding_table *, struct hmap *ls_ports);
>>> +    const struct sbrec_port_binding_table *, struct hmap *ls_ports,
>>> +    struct hmap *lr_ports);
>>>
>>>  struct tracked_lb_data;
>>>  bool northd_handle_lb_data_changes(struct tracked_lb_data *,
>>> @@ -394,7 +395,8 @@ void sync_lbs(struct ovsdb_idl_txn *, const struct sbrec_load_balancer_table *,
>>>                struct chassis_features *chassis_features);
>>>  bool check_sb_lb_duplicates(const struct sbrec_load_balancer_table *);
>>>
>>> -void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports);
>>> +void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports,
>>> +              struct hmap *lr_ports);
>>>  bool sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports *);
>>>
>>>  static inline bool
>>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>>> index 4a02dacf60..4edad24e53 100644
>>> --- a/tests/ovn-northd.at
>>> +++ b/tests/ovn-northd.at
>>> @@ -10073,7 +10073,7 @@ check ovn-nbctl --wait=hv lsp-add ls0 lsp0-0 -- lsp-set-addresses lsp0-0 "unknow
>>>  ovs-vsctl add-port br-int lsp0-0 -- set interface lsp0-0 external_ids:iface-id=lsp0-0
>>>  wait_for_ports_up
>>>  check ovn-nbctl --wait=hv sync
>>> -check_recompute_counter 4 5 5 5 5 5
>>> +check_recompute_counter 3 4 3 4 3 4
>>>
>>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>  check ovn-nbctl --wait=hv lsp-add ls0 lsp0-1 -- lsp-set-addresses lsp0-1 "aa:aa:aa:00:00:01 192.168.0.11"
>>> @@ -10235,6 +10235,126 @@ OVN_CLEANUP([hv1])
>>>  AT_CLEANUP
>>>  ])
>>>
>>> +OVN_FOR_EACH_NORTHD_NO_HV([
>>> +AT_SETUP([SB Port binding incremental processing])
>>> +ovn_start
>>> +
>>> +check_recompute_counter() {
>>> +    northd_recomp=$(as northd ovn-appctl -t ovn-northd inc-engine/show-stats northd recompute)
>>> +    echo "northd recompute count - $northd_recomp"
>>> +    AT_CHECK([test x$northd_recomp = x$1])
>>> +
>>> +    lflow_recomp=$(as northd ovn-appctl -t ovn-northd inc-engine/show-stats lflow recompute)
>>> +    echo "lflow recompute count - $lflow_recomp"
>>> +    AT_CHECK([test x$lflow_recomp = x$2])
>>> +}
>>> +
>>> +net_add n1
>>> +sim_add hv1
>>> +as hv1
>>> +ovs-vsctl add-br br-phys
>>> +ovn_attach n1 br-phys 192.168.0.11
>>> +
>>> +check ovn-nbctl --wait=sb ls-add ls0
>>> +check ovn-nbctl --wait=sb lr-add lr0
>>> +
>>> +# Test normal VIF port
>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>> +check ovn-nbctl lsp-add ls0 p1
>>> +check_recompute_counter 0 0
>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>> +
>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>> +check ovn-nbctl lsp-set-addresses p1 "00:00:00:00:01:01 10.0.0.3"
>>> +check_recompute_counter 0 0
>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>> +
>>> +check as northd ovn-appctl -t ovn-northd vlog/set info
>>> +
>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>> +check ovn-sbctl lsp-bind p1 hv1
>>> +check_recompute_counter 0 0
>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>> +
>>> +# Test lsp of type router
>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>> +check ovn-nbctl lsp-add ls0 rp -- lsp-set-type rp router
>>> +
>>> +# northd engine recomputes twice. Both the times for handling NB logical switch port
>>> +# changes and not because of SB port binding changes.  This is because ovn-northd
>>> +# sets the "up" to true.
>>> +check_recompute_counter 2 2
>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>> +
>>> +# Set some options to 'rp'.  northd should only recompute once.
>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>> +check ovn-nbctl lsp-set-options rp foo=bar
>>> +check_recompute_counter 1 1
>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>> +
>>> +# Test lsp of type external
>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>> +check ovn-nbctl lsp-add ls0 e1 -- lsp-set-type e1 external
>>> +check_recompute_counter 2 2
>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>> +
>>> +# Set some options to 'e1'.  northd should only recompute once.
>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>> +check ovn-nbctl lsp-set-options e1 foo=bar
>>> +check_recompute_counter 1 1
>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>> +
>>> +# Test lrp
>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>> +check ovn-nbctl lrp-add lr0 lrp 00:00:02:01:02:03 10.0.0.1/24
>>> +check_recompute_counter 1 1
>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>> +
>>> +# Set some options on 'lrp'.  northd should only recompute once.
>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>> +check ovn-nbctl lrp-set-options lrp route_table=rtb-1
>>> +check_recompute_counter 1 1
>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>> +
>>> +# Make lrp a gateway port
>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>> +check ovn-nbctl lrp-set-gateway-chassis lrp hv1
>>> +# There will be 3 recomputes of northd engine node
>>> +#   1. missing handler for input NB_logical_router
>>> +#   2. missing handler for input SB_ha_chassis_group
>>> +#   3. missing handler for input NB_logical_router when ovn-northd
>>> +#      updates the hosting-chassis option in NB_logical_router_port.
>>> +check_recompute_counter 3 3
>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>> +
>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>> +check ovn-nbctl clear logical_router_port lrp gateway_chassis
>>> +# There will be 2 recomputes of northd engine node
>>> +#   1. missing handler for input NB_logical_router
>>> +#   2. missing handler for input SB_ha_chassis_group
>>> +check_recompute_counter 2 2
>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>> +
>>> +# Delete some of the ports.
>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>> +check ovn-nbctl lsp-del p1
>>> +check_recompute_counter 0 0
>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>> +
>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>> +check ovn-nbctl lsp-del e1
>>> +check_recompute_counter 1 1
>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>> +
>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>> +check ovn-nbctl lrp-del lrp
>>> +check_recompute_counter 1 1
>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>> +
>>> +OVN_CLEANUP([hv1])
>>> +AT_CLEANUP
>>> +])
>>> +
>>>  OVN_FOR_EACH_NORTHD_NO_HV([
>>>  AT_SETUP([ACL/Meter incremental processing - no northd recompute])
>>>  ovn_start
>>> @@ -10269,6 +10389,7 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE(1)
>>>  AT_CLEANUP
>>>  ])
>>>
>>> +
>>>  OVN_FOR_EACH_NORTHD_NO_HV([
>>>  AT_SETUP([check fip and lb flows])
>>>  AT_KEYWORDS([fip-lb-flows])
>>> @@ -10471,7 +10592,7 @@ ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01
>>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>  ovn-nbctl --wait=sb lsp-set-options sw0-lr0 router-port=lr0-sw0
>>>  check_engine_stats lb_data norecompute compute
>>> -check_engine_stats northd recompute nocompute
>>> +check_engine_stats northd recompute compute
>>>  check_engine_stats lflow recompute nocompute
>>>  check_engine_stats sync_to_sb_lb recompute nocompute
>>>
>>> @@ -11114,12 +11235,14 @@ ovn_attach n1 br-phys 192.168.0.11
>>>  ovn-sbctl chassis-add gw1 geneve 127.0.0.1
>>>
>>>  check ovn-nbctl ls-add sw0
>>> -check ovn-nbctl lsp-add sw0 sw0p1 -- lsp-set-addresses sw0p1 "00:00:20:20:12:01 10.0.0.4"
>>> +check ovn-nbctl --wait=sb lsp-add sw0 sw0p1 -- lsp-set-addresses sw0p1 "00:00:20:20:12:01 10.0.0.4"
>>>
>>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>  check ovn-nbctl --wait=sb lr-add lr0
>>>  check_engine_stats northd recompute nocompute
>>> +check_engine_stats sync_to_sb_pb recompute nocompute
>>>  check_engine_stats lflow recompute nocompute
>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>
>>>  # Adding a logical router port should result in recompute
>>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>> @@ -11127,16 +11250,20 @@ check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24
>>>  # for northd engine there will be both recompute and compute
>>>  # first it will be recompute to handle lr0-sw0 and then a compute
>>>  # for the SB port binding change.
>>> -check_engine_stats northd recompute nocompute
>>> +check_engine_stats northd recompute compute
>>> +check_engine_stats sync_to_sb_pb recompute nocompute
>>>  check_engine_stats lflow recompute nocompute
>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>
>>>  ovn-nbctl lsp-add sw0 sw0-lr0
>>>  ovn-nbctl lsp-set-type sw0-lr0 router
>>>  ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01
>>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>  check ovn-nbctl --wait=sb lsp-set-options sw0-lr0 router-port=lr0-sw0
>>> -check_engine_stats northd recompute nocompute
>>> +check_engine_stats northd recompute compute
>>> +check_engine_stats sync_to_sb_pb recompute nocompute
>>>  check_engine_stats lflow recompute nocompute
>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>
>>>  ovn-nbctl ls-add public
>>>  ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24
>>> @@ -11158,7 +11285,9 @@ ovn-nbctl --wait=hv lrp-set-gateway-chassis lr0-public hv1 20
>>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>  check ovn-nbctl set logical_router_port lr0-sw0 options:foo=bar
>>>  check_engine_stats northd recompute nocompute
>>> +check_engine_stats sync_to_sb_pb recompute nocompute
>>>  check_engine_stats lflow recompute nocompute
>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>
>>>  # Do checks for NATs.
>>>  # Add a NAT. This should not result in recompute of both northd and lflow
>>> @@ -11167,6 +11296,7 @@ check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>  check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat  172.168.0.110 10.0.0.4
>>>  check_engine_stats northd recompute nocompute
>>>  check_engine_stats lflow recompute nocompute
>>> +check_engine_stats sync_to_sb_pb recompute nocompute
>>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>
>>>  # Update the NAT options column
>>> @@ -11174,6 +11304,7 @@ check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>  check ovn-nbctl --wait=sb set NAT . options:foo=bar
>>>  check_engine_stats northd recompute nocompute
>>>  check_engine_stats lflow recompute nocompute
>>> +check_engine_stats sync_to_sb_pb recompute nocompute
>>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>
>>>  # Update the NAT external_ip column
>>> @@ -11181,6 +11312,7 @@ check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>  check ovn-nbctl --wait=sb set NAT . external_ip=172.168.0.120
>>>  check_engine_stats northd recompute nocompute
>>>  check_engine_stats lflow recompute nocompute
>>> +check_engine_stats sync_to_sb_pb recompute nocompute
>>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>
>>>  # Update the NAT logical_ip column
>>> @@ -11188,6 +11320,7 @@ check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>  check ovn-nbctl --wait=sb set NAT . logical_ip=10.0.0.10
>>>  check_engine_stats northd recompute nocompute
>>>  check_engine_stats lflow recompute nocompute
>>> +check_engine_stats sync_to_sb_pb recompute nocompute
>>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>
>>>  # Update the NAT type
>>> @@ -11195,13 +11328,15 @@ check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>  check ovn-nbctl --wait=sb set NAT . type=snat
>>>  check_engine_stats northd recompute nocompute
>>>  check_engine_stats lflow recompute nocompute
>>> +check_engine_stats sync_to_sb_pb recompute nocompute
>>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>
>>>  # Create a dnat_and_snat NAT with external_mac and logical_port
>>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>  check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.110 10.0.0.4 sw0p1 30:54:00:00:00:03
>>> -check_engine_stats northd recompute nocompute
>>> +check_engine_stats northd recompute compute
>>>  check_engine_stats lflow recompute nocompute
>>> +check_engine_stats sync_to_sb_pb recompute nocompute
>>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>
>>>  nat2_uuid=$(ovn-nbctl --bare --columns _uuid find nat logical_ip=10.0.0.4)
>>> @@ -11210,6 +11345,7 @@ check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>  check ovn-nbctl --wait=sb set NAT $nat2_uuid external_mac='"30:54:00:00:00:04"'
>>>  check_engine_stats northd recompute nocompute
>>>  check_engine_stats lflow recompute nocompute
>>> +check_engine_stats sync_to_sb_pb recompute nocompute
>>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>
>>>  # Create a load balancer and add the lb vip as NAT
>>> @@ -11223,31 +11359,35 @@ check ovn-nbctl lr-lb-add lr0 lb2
>>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>  check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.140 10.0.0.20
>>>  check_engine_stats northd recompute nocompute
>>> +check_engine_stats sync_to_sb_pb recompute nocompute
>>>  check_engine_stats lflow recompute nocompute
>>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>
>>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>  check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.150 10.0.0.41
>>>  check_engine_stats northd recompute nocompute
>>> +check_engine_stats sync_to_sb_pb recompute nocompute
>>>  check_engine_stats lflow recompute nocompute
>>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>
>>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>  check ovn-nbctl --wait=sb lr-nat-del lr0 dnat_and_snat 172.168.0.150
>>>  check_engine_stats northd recompute nocompute
>>> +check_engine_stats sync_to_sb_pb recompute nocompute
>>>  check_engine_stats lflow recompute nocompute
>>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>
>>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>  check ovn-nbctl --wait=sb lr-nat-del lr0 dnat_and_snat 172.168.0.140
>>>  check_engine_stats northd recompute nocompute
>>> +check_engine_stats sync_to_sb_pb recompute nocompute
>>>  check_engine_stats lflow recompute nocompute
>>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>
>>>  # Delete the NAT
>>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>  check ovn-nbctl --wait=sb clear logical_router lr0 nat
>>> -check_engine_stats northd recompute nocompute
>>> +check_engine_stats northd recompute compute
>>>  check_engine_stats lflow recompute nocompute
>>>  check_engine_stats sync_to_sb_pb recompute nocompute
>>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>> @@ -11256,12 +11396,16 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>  check ovn-nbctl --wait=sb lr-policy-add lr0  10 "ip4.src == 10.0.0.3" reroute 172.168.0.101,172.168.0.102
>>>  check_engine_stats northd recompute nocompute
>>> +check_engine_stats sync_to_sb_pb recompute nocompute
>>>  check_engine_stats lflow recompute nocompute
>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>
>>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>  check ovn-nbctl --wait=sb lr-policy-del lr0  10 "ip4.src == 10.0.0.3"
>>>  check_engine_stats northd recompute nocompute
>>> +check_engine_stats sync_to_sb_pb recompute nocompute
>>>  check_engine_stats lflow recompute nocompute
>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>
>>>  OVN_CLEANUP([hv1])
>>>  AT_CLEANUP
>>
>> The rest looks good to me but I'd prefer we figure out the test failure
>> before acking the patch.
>>
>> Regards,
>> Dumitru
>>
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>

On Thu, Jan 25, 2024, 3:29 PM Dumitru Ceara <dceara@redhat.com> wrote:

> On 1/25/24 17:46, Numan Siddique wrote:
> > On Tue, Jan 16, 2024 at 6:15 AM Dumitru Ceara <dceara@redhat.com> wrote:
> >>
> >> On 1/11/24 16:28, numans@ovn.org wrote:
> >>> From: Numan Siddique <numans@ovn.org>
> >>>
> >>> It also moves the logical router port IPv6 prefix delegation
> >>> updates to "sync-from-sb" engine node.
> >>>
> >>> With these changes, northd engine node doesn't need to do much
> >>> for SB Port binding changes other than updating 'op->sb'.  And
> >>> there is no need to fall back to recompute.  Prior to this patch
> >>> northd_handle_sb_port_binding_changes() was returning false for
> >>> all SB port binding updates except for the VIF PBs.  This patch
> >>> now handles updates to all the SB port binding by setting 'op->sb'.
> >>>
> >>> Signed-off-by: Numan Siddique <numans@ovn.org>
> >>> ---
> >>
> >> Hi Numan,
> >>
> >> With this patch applied the "633: ovn-northd.at:10298 SB Port binding
> >> incremental processing -- parallelization=yes" test occasionally fails,
> >> e.g.:
> >>
> >> https://github.com/dceara/ovn/actions/runs/7539663594/job/20524238508
> >>
> >> I saw it locally too when running the test in a loop:
> >>
> >> It fails with:
> >> ovn-nbctl lrp-set-gateway-chassis lrp hv1
> >> ./ovn-macros.at:449: "$@"
> >> northd recompute count - 2
> >> ./ovn-northd.at:10238: test x$northd_recomp = x$1
> >> ./ovn-northd.at:10238: exit code was 1, expected 0
> >>
> >> Here:
> >>
> https://github.com/dceara/ovn/blob/e1c76b6d347570618591fe6587f6af83445223ec/tests/ovn-northd.at#L10327
> >>
> >> For some reason in some cases only 2 recomputes get triggered.
> >
> > Thanks Dumitru and Han for the reviews.
> >
> > @Dumitru Ceara  I've addressed all the review comments below.
> >
> > I think the test case is failing because of the missing "--wait=sb" in
> > the ovn-nbctl commands.
> >
>
> I don't think it's only that.  It still fails even with your updated
> test changes, in the same way:
>
> as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> ./ovn-macros.at:449: "$@"
> ovn-nbctl lrp-set-gateway-chassis lrp hv1
> ./ovn-macros.at:449: "$@"
> northd recompute count - 2
> ./ovn-northd.at:10238: test x$northd_recomp = x$1
> ./ovn-northd.at:10238: exit code was 1, expected 0
>
> > Will the below changes in ovn-northd.at look good to you  ?  If so,
> > and if you can provide your ack I can apply this patch.
> > Otherwise I'll include it in the v6.
> >
>
> I would prefer if we don't apply it until we figure out this test, the
> CI has been relatively stable the last few weeks, I'd rather not
> destabilize it if possible.
>

Sure.  Definitely.

Its passing for me locally when run in a loop.

Let me dig into it further.


Thanks
Numan


> Regards,
> Dumitru
>
> > ----
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 72eb9e1173..492248b05d 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -10316,12 +10316,12 @@ check ovn-nbctl --wait=sb lr-add lr0
> >
> >  # Test normal VIF port
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > -check ovn-nbctl lsp-add ls0 p1
> > +check ovn-nbctl --wait=sb lsp-add ls0 p1
> >  check_recompute_counter 0 0
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > -check ovn-nbctl lsp-set-addresses p1 "00:00:00:00:01:01 10.0.0.3"
> > +check ovn-nbctl --wait=sb lsp-set-addresses p1 "00:00:00:00:01:01
> 10.0.0.3"
> >  check_recompute_counter 0 0
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> > @@ -10329,12 +10329,13 @@ check as northd ovn-appctl -t ovn-northd
> vlog/set info
> >
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >  check ovn-sbctl lsp-bind p1 hv1
> > +check ovn-nbctl --wait=sb sync
> >  check_recompute_counter 0 0
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  # Test lsp of type router
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > -check ovn-nbctl lsp-add ls0 rp -- lsp-set-type rp router
> > +check ovn-nbctl --wait=sb lsp-add ls0 rp -- lsp-set-type rp router
> >
> >  # northd engine recomputes twice. Both the times for handling NB
> > logical switch port
> >  # changes and not because of SB port binding changes.  This is
> > because ovn-northd
> > @@ -10344,37 +10345,37 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  # Set some options to 'rp'.  northd should only recompute once.
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > -check ovn-nbctl lsp-set-options rp foo=bar
> > +check ovn-nbctl --wait=sb lsp-set-options rp foo=bar
> >  check_recompute_counter 1 1
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  # Test lsp of type external
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > -check ovn-nbctl lsp-add ls0 e1 -- lsp-set-type e1 external
> > +check ovn-nbctl --wait=sb lsp-add ls0 e1 -- lsp-set-type e1 external
> >  check_recompute_counter 2 2
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  # Set some options to 'e1'.  northd should only recompute once.
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > -check ovn-nbctl lsp-set-options e1 foo=bar
> > +check ovn-nbctl --wait=sb lsp-set-options e1 foo=bar
> >  check_recompute_counter 1 1
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  # Test lrp
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > -check ovn-nbctl lrp-add lr0 lrp 00:00:02:01:02:03 10.0.0.1/24
> > +check ovn-nbctl --wait=sb lrp-add lr0 lrp 00:00:02:01:02:03 10.0.0.1/24
> >  check_recompute_counter 1 1
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  # Set some options on 'lrp'.  northd should only recompute once.
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > -check ovn-nbctl lrp-set-options lrp route_table=rtb-1
> > +check ovn-nbctl --wait=sb lrp-set-options lrp route_table=rtb-1
> >  check_recompute_counter 1 1
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  # Make lrp a gateway port
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > -check ovn-nbctl lrp-set-gateway-chassis lrp hv1
> > +check ovn-nbctl --wait=sb lrp-set-gateway-chassis lrp hv1
> >  # There will be 3 recomputes of northd engine node
> >  #   1. missing handler for input NB_logical_router
> >  #   2. missing handler for input SB_ha_chassis_group
> > @@ -10384,7 +10385,7 @@ check_recompute_counter 3 3
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > -check ovn-nbctl clear logical_router_port lrp gateway_chassis
> > +check ovn-nbctl --wait=sb clear logical_router_port lrp gateway_chassis
> >  # There will be 2 recomputes of northd engine node
> >  #   1. missing handler for input NB_logical_router
> >  #   2. missing handler for input SB_ha_chassis_group
> > @@ -10393,17 +10394,17 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  # Delete some of the ports.
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > -check ovn-nbctl lsp-del p1
> > +check ovn-nbctl --wait=sb lsp-del p1
> >  check_recompute_counter 0 0
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > -check ovn-nbctl lsp-del e1
> > +check ovn-nbctl --wait=sb lsp-del e1
> >  check_recompute_counter 1 1
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > -check ovn-nbctl lrp-del lrp
> > +check ovn-nbctl --wait=sb lrp-del lrp
> >  check_recompute_counter 1 1
> >  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > --------------------------------
> >
> > Thanks
> > Numan
> >
> >
> >
> >>
> >>>  northd/en-northd.c  |   2 +-
> >>>  northd/en-sync-sb.c |   3 +-
> >>>  northd/northd.c     | 296 ++++++++++++++++++++++++++------------------
> >>>  northd/northd.h     |   6 +-
> >>>  tests/ovn-northd.at | 158 +++++++++++++++++++++--
> >>>  5 files changed, 334 insertions(+), 131 deletions(-)
> >>>
> >>> diff --git a/northd/en-northd.c b/northd/en-northd.c
> >>> index 28559ed211..677b2b1ab0 100644
> >>> --- a/northd/en-northd.c
> >>> +++ b/northd/en-northd.c
> >>> @@ -189,7 +189,7 @@ northd_sb_port_binding_handler(struct engine_node
> *node,
> >>>      northd_get_input_data(node, &input_data);
> >>>
> >>>      if (!northd_handle_sb_port_binding_changes(
> >>> -        input_data.sbrec_port_binding_table, &nd->ls_ports)) {
> >>> +        input_data.sbrec_port_binding_table, &nd->ls_ports,
> &nd->lr_ports)) {
> >>>          return false;
> >>>      }
> >>>
> >>> diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
> >>> index 3aaab8d005..45be7ddbcb 100644
> >>> --- a/northd/en-sync-sb.c
> >>> +++ b/northd/en-sync-sb.c
> >>> @@ -288,7 +288,8 @@ en_sync_to_sb_pb_run(struct engine_node *node,
> void *data OVS_UNUSED)
> >>>      const struct engine_context *eng_ctx = engine_get_context();
> >>>      struct northd_data *northd_data = engine_get_input_data("northd",
> node);
> >>>
> >>> -    sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports);
> >>> +    sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports,
> >>> +             &northd_data->lr_ports);
> >>>      engine_set_node_state(node, EN_UPDATED);
> >>>  }
> >>>
> >>> diff --git a/northd/northd.c b/northd/northd.c
> >>> index f32e3bf21a..23f2dae26b 100644
> >>> --- a/northd/northd.c
> >>> +++ b/northd/northd.c
> >>> @@ -3435,6 +3435,9 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn
> *ovnsb_txn,
> >>>  {
> >>>      sbrec_port_binding_set_datapath(op->sb, op->od->sb);
> >>>      if (op->nbrp) {
> >>> +        /* Note: SB port binding options for router ports are set in
> >>> +         * sync_pbs(). */
> >>> +
> >>>          /* If the router is for l3 gateway, it resides on a chassis
> >>>           * and its port type is "l3gateway". */
> >>>          const char *chassis_name = smap_get(&op->od->nbr->options,
> "chassis");
> >>> @@ -3446,15 +3449,11 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn
> *ovnsb_txn,
> >>>              sbrec_port_binding_set_type(op->sb, "patch");
> >>>          }
> >>>
> >>> -        struct smap new;
> >>> -        smap_init(&new);
> >>>          if (is_cr_port(op)) {
> >>>              ovs_assert(sbrec_chassis_by_name);
> >>>              ovs_assert(sbrec_chassis_by_hostname);
> >>>              ovs_assert(sbrec_ha_chassis_grp_by_name);
> >>>              ovs_assert(active_ha_chassis_grps);
> >>> -            const char *redirect_type = smap_get(&op->nbrp->options,
> >>> -                                                 "redirect-type");
> >>>
> >>>              if (op->nbrp->ha_chassis_group) {
> >>>                  if (op->nbrp->n_gateway_chassis) {
> >>> @@ -3496,49 +3495,8 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn
> *ovnsb_txn,
> >>>                  /* Delete the legacy gateway_chassis from the pb. */
> >>>                  sbrec_port_binding_set_gateway_chassis(op->sb, NULL,
> 0);
> >>>              }
> >>> -            smap_add(&new, "distributed-port", op->nbrp->name);
> >>> -
> >>> -            bool always_redirect =
> >>> -                !op->od->has_distributed_nat &&
> >>> -
> !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port);
> >>> -
> >>> -            if (redirect_type) {
> >>> -                smap_add(&new, "redirect-type", redirect_type);
> >>> -                /* XXX Why can't we enable always-redirect when
> redirect-type
> >>> -                 * is bridged? */
> >>> -                if (!strcmp(redirect_type, "bridged")) {
> >>> -                    always_redirect = false;
> >>> -                }
> >>> -            }
> >>> -
> >>> -            if (always_redirect) {
> >>> -                smap_add(&new, "always-redirect", "true");
> >>> -            }
> >>> -        } else {
> >>> -            if (op->peer) {
> >>> -                smap_add(&new, "peer", op->peer->key);
> >>> -                if (op->nbrp->ha_chassis_group ||
> >>> -                    op->nbrp->n_gateway_chassis) {
> >>> -                    char *redirect_name =
> >>> -                        ovn_chassis_redirect_name(op->nbrp->name);
> >>> -                    smap_add(&new, "chassis-redirect-port",
> redirect_name);
> >>> -                    free(redirect_name);
> >>> -                }
> >>> -            }
> >>> -            if (chassis_name) {
> >>> -                smap_add(&new, "l3gateway-chassis", chassis_name);
> >>> -            }
> >>>          }
> >>>
> >>> -        const char *ipv6_pd_list = smap_get(&op->sb->options,
> >>> -                                            "ipv6_ra_pd_list");
> >>> -        if (ipv6_pd_list) {
> >>> -            smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list);
> >>> -        }
> >>> -
> >>> -        sbrec_port_binding_set_options(op->sb, &new);
> >>> -        smap_destroy(&new);
> >>> -
> >>>          sbrec_port_binding_set_parent_port(op->sb, NULL);
> >>>          sbrec_port_binding_set_tag(op->sb, NULL, 0);
> >>>
> >>> @@ -4768,12 +4726,14 @@ check_sb_lb_duplicates(const struct
> sbrec_load_balancer_table *table)
> >>>      return duplicates;
> >>>  }
> >>>
> >>> -/* Syncs the SB port binding for the ovn_port 'op'.  Caller should
> make sure
> >>> - * that the OVN SB IDL txn is not NULL.  Presently it only syncs the
> nat
> >>> - * column of port binding corresponding to the 'op->nbsp' */
> >>> +/* Syncs the SB port binding for the ovn_port 'op' of a logical
> switch port.
> >>> + * Caller should make sure that the OVN SB IDL txn is not NULL.
> Presently it
> >>> + * only syncs the nat column of port binding corresponding to the
> 'op->nbsp' */
> >>>  static void
> >>> -sync_pb_for_op(struct ovn_port *op)
> >>> +sync_pb_for_lsp(struct ovn_port *op)
> >>>  {
> >>> +    ovs_assert(op->nbsp);
> >>> +
> >>>      if (lsp_is_router(op->nbsp)) {
> >>>          const char *chassis = NULL;
> >>>          if (op->peer && op->peer->od && op->peer->od->nbr) {
> >>> @@ -4895,18 +4855,86 @@ sync_pb_for_op(struct ovn_port *op)
> >>>      }
> >>>  }
> >>>
> >>> +/* Syncs the SB port binding for the ovn_port 'op' of a logical
> router port.
> >>> + * Caller should make sure that the OVN SB IDL txn is not NULL.
> Presently it
> >>> + * only sets the port binding options column for the router ports */
> >>> +static void
> >>> +sync_pb_for_lrp(struct ovn_port *op)
> >>> +{
> >>> +    ovs_assert(op->nbrp);
> >>> +
> >>> +    struct smap new;
> >>> +    smap_init(&new);
> >>> +
> >>> +    const char *chassis_name = smap_get(&op->od->nbr->options,
> "chassis");
> >>> +    if (is_cr_port(op)) {
> >>> +        smap_add(&new, "distributed-port", op->nbrp->name);
> >>> +
> >>> +        bool always_redirect =
> >>> +            !op->od->has_distributed_nat &&
> >>> +            !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port);
> >>> +
> >>> +        const char *redirect_type = smap_get(&op->nbrp->options,
> >>> +                                            "redirect-type");
> >>> +        if (redirect_type) {
> >>> +            smap_add(&new, "redirect-type", redirect_type);
> >>> +            /* XXX Why can't we enable always-redirect when
> redirect-type
> >>> +                * is bridged? */
> >>
> >> Nit: indentation
> >>
> >>> +            if (!strcmp(redirect_type, "bridged")) {
> >>> +                always_redirect = false;
> >>> +            }
> >>> +        }
> >>> +
> >>> +        if (always_redirect) {
> >>> +            smap_add(&new, "always-redirect", "true");
> >>> +        }
> >>> +    } else {
> >>> +        if (op->peer) {
> >>> +            smap_add(&new, "peer", op->peer->key);
> >>> +            if (op->nbrp->ha_chassis_group ||
> >>> +                op->nbrp->n_gateway_chassis) {
> >>> +                char *redirect_name =
> >>> +                    ovn_chassis_redirect_name(op->nbrp->name);
> >>> +                smap_add(&new, "chassis-redirect-port",
> redirect_name);
> >>> +                free(redirect_name);
> >>> +            }
> >>> +        }
> >>> +        if (chassis_name) {
> >>> +            smap_add(&new, "l3gateway-chassis", chassis_name);
> >>> +        }
> >>> +    }
> >>> +
> >>> +    const char *ipv6_pd_list = smap_get(&op->sb->options,
> "ipv6_ra_pd_list");
> >>> +    if (ipv6_pd_list) {
> >>> +        smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list);
> >>> +    }
> >>> +
> >>> +    sbrec_port_binding_set_options(op->sb, &new);
> >>> +    smap_destroy(&new);
> >>> +}
> >>> +
> >>> +static void ovn_update_ipv6_options(struct hmap *lr_ports);
> >>> +static void ovn_update_ipv6_opt_for_op(struct ovn_port *op);
> >>> +
> >>>  /* Sync the SB Port bindings which needs to be updated.
> >>>   * Presently it syncs the nat column of port bindings corresponding to
> >>>   * the logical switch ports. */
> >>>  void
> >>> -sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports)
> >>> +sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports,
> >>> +         struct hmap *lr_ports)
> >>>  {
> >>>      ovs_assert(ovnsb_idl_txn);
> >>>
> >>>      struct ovn_port *op;
> >>>      HMAP_FOR_EACH (op, key_node, ls_ports) {
> >>> -        sync_pb_for_op(op);
> >>> +        sync_pb_for_lsp(op);
> >>> +    }
> >>> +
> >>> +    HMAP_FOR_EACH (op, key_node, lr_ports) {
> >>> +        sync_pb_for_lrp(op);
> >>>      }
> >>> +
> >>> +    ovn_update_ipv6_options(lr_ports);
> >>>  }
> >>>
> >>>  /* Sync the SB Port bindings for the added and updated logical switch
> ports
> >>> @@ -4918,12 +4946,14 @@ sync_pbs_for_northd_changed_ovn_ports( struct
> tracked_ovn_ports *trk_ovn_ports)
> >>>      struct ovn_port *op;
> >>>      HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->created) {
> >>>          op = hmapx_node->data;
> >>> -        sync_pb_for_op(op);
> >>> +        ovs_assert(op->nbsp);
> >>
> >> This is already checked inside sync_pb_for_lsp().  Let's remove it from
> >> here.  Then what I mentioned in patch 01/16 still applies, we don't need
> >> the local 'op' variable anymore.
> >>
> >>> +        sync_pb_for_lsp(op);
> >>>      }
> >>>
> >>>      HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->updated) {
> >>>          op = hmapx_node->data;
> >>> -        sync_pb_for_op(op);
> >>> +        ovs_assert(op->nbsp);
> >>> +        sync_pb_for_lsp(op);
> >>
> >> Same here.
> >>
> >>>      }
> >>>
> >>>      return true;
> >>> @@ -5671,20 +5701,31 @@ fail:
> >>>  bool
> >>>  northd_handle_sb_port_binding_changes(
> >>>      const struct sbrec_port_binding_table *sbrec_port_binding_table,
> >>> -    struct hmap *ls_ports)
> >>> +    struct hmap *ls_ports, struct hmap *lr_ports)
> >>>  {
> >>>      const struct sbrec_port_binding *pb;
> >>>      static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> >>>      SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb,
> sbrec_port_binding_table) {
> >>> -        /* Return false if the 'pb' belongs to a router port.  We
> don't handle
> >>> -         * I-P for router ports yet. */
> >>> -        if (is_pb_router_type(pb)) {
> >>> -            return false;
> >>> +        bool is_router_port = is_pb_router_type(pb);
> >>> +        struct ovn_port *op = NULL;
> >>> +
> >>> +        if (is_router_port) {
> >>> +            /* A router port binding 'pb' can belong to
> >>> +             *   - a logical switch port of type router or
> >>> +             *   - a logical router port.
> >>> +             *
> >>> +             * So, first search in lr_ports hmap.  If not found,
> search in
> >>> +             * ls_ports hmap.
> >>> +             * */
> >>> +            op = ovn_port_find(lr_ports, pb->logical_port);
> >>>          }
> >>>
> >>> -        struct ovn_port *op = ovn_port_find(ls_ports,
> pb->logical_port);
> >>> -        if (op && !op->lsp_can_be_inc_processed) {
> >>> -            return false;
> >>> +        if (!op) {
> >>> +            op = ovn_port_find(ls_ports, pb->logical_port);
> >>> +
> >>> +            if (op) {
> >>> +                is_router_port = false;
> >>> +            }
> >>>          }
> >>>
> >>>          if (sbrec_port_binding_is_new(pb)) {
> >>> @@ -5693,7 +5734,8 @@ northd_handle_sb_port_binding_changes(
> >>>               * pointer in northd data. Fallback to recompute
> otherwise. */
> >>>              if (!op) {
> >>>                  VLOG_WARN_RL(&rl, "A port-binding for %s is created
> but the "
> >>> -                            "LSP is not found.", pb->logical_port);
> >>> +                            "%s is not found.", pb->logical_port,
> >>> +                            is_router_port ? "LRP" : "LSP");
> >>>                  return false;
> >>>              }
> >>>              op->sb = pb;
> >>> @@ -5703,18 +5745,29 @@ northd_handle_sb_port_binding_changes(
> >>>               * case. Fallback to recompute otherwise, to avoid
> dangling
> >>>               * sb idl pointers and other unexpected behavior. */
> >>>              if (op && op->sb == pb) {
> >>> -                VLOG_WARN_RL(&rl, "A port-binding for %s is deleted
> but "
> >>> -                            "the LSP still exists.",
> pb->logical_port);
> >>> +                VLOG_WARN_RL(&rl, "A port-binding for %s is deleted
> but the "
> >>> +                            "LSP/LRP still exists.",
> pb->logical_port);
> >>>                  return false;
> >>>              }
> >>>          } else {
> >>> -            /* The PB is updated, most likely because of
> binding/unbinding
> >>> -             * to/from a chassis, and we can ignore the change
> (updating NB
> >>> -             * "up" will be handled in the engine node
> "sync_from_sb").
> >>> +            /* The PB is updated.
> >>> +             * For an LSP PB it is most likely because of
> >>> +             * binding/unbinding to/from a chassis, and we can ignore
> the
> >>> +             * change (updating NB "up" will be handled in the engine
> node
> >>> +             * "sync_from_sb").
> >>> +             *
> >>> +             * For an LRP PB, it is most likely because of
> >>> +             *   - IPv6 prefix delagation updates from ovn-controller.
> >>> +             *     This update is handled in "sync_from_sb" node.
> >>> +             *   - ha chassis group and this can be ignored.
> >>> +             *
> >>> +             * All other changes can be ignored.
> >>> +             *
> >>>               * Fallback to recompute for anything unexpected. */
> >>>              if (!op) {
> >>>                  VLOG_WARN_RL(&rl, "A port-binding for %s is updated
> but the "
> >>> -                            "LSP is not found.", pb->logical_port);
> >>> +                            "%s is not found.", pb->logical_port,
> >>> +                            is_router_port ? "LRP" : "LSP");
> >>>                  return false;
> >>>              }
> >>>              if (op->sb != pb) {
> >>> @@ -7855,67 +7908,70 @@ static void
> >>>  copy_ra_to_sb(struct ovn_port *op, const char *address_mode);
> >>>
> >>>  static void
> >>> -ovn_update_ipv6_options(struct hmap *lr_ports)
> >>> +ovn_update_ipv6_opt_for_op(struct ovn_port *op)
> >>>  {
> >>> -    struct ovn_port *op;
> >>> -    HMAP_FOR_EACH (op, key_node, lr_ports) {
> >>> -        ovs_assert(op->nbrp);
> >>> -
> >>> -        if (op->nbrp->peer || !op->peer) {
> >>> -            continue;
> >>> -        }
> >>> +    if (op->nbrp->peer || !op->peer) {
> >>> +        return;
> >>> +    }
> >>>
> >>> -        if (!op->lrp_networks.n_ipv6_addrs) {
> >>> -            continue;
> >>> -        }
> >>> +    if (!op->lrp_networks.n_ipv6_addrs) {
> >>> +        return;
> >>> +    }
> >>>
> >>> -        struct smap options;
> >>> -        smap_clone(&options, &op->sb->options);
> >>> +    struct smap options;
> >>> +    smap_clone(&options, &op->sb->options);
> >>>
> >>> -        /* enable IPv6 prefix delegation */
> >>> -        bool prefix_delegation = smap_get_bool(&op->nbrp->options,
> >>> +    /* enable IPv6 prefix delegation */
> >>> +    bool prefix_delegation = smap_get_bool(&op->nbrp->options,
> >>>                                             "prefix_delegation",
> false);
> >>> -        if (!lrport_is_enabled(op->nbrp)) {
> >>> -            prefix_delegation = false;
> >>> -        }
> >>> -        if (smap_get_bool(&options, "ipv6_prefix_delegation",
> >>> -                          false) != prefix_delegation) {
> >>> -            smap_add(&options, "ipv6_prefix_delegation",
> >>> -                     prefix_delegation ? "true" : "false");
> >>> -        }
> >>> +    if (!lrport_is_enabled(op->nbrp)) {
> >>> +        prefix_delegation = false;
> >>> +    }
> >>> +    if (smap_get_bool(&options, "ipv6_prefix_delegation",
> >>> +                      false) != prefix_delegation) {
> >>> +        smap_add(&options, "ipv6_prefix_delegation",
> >>> +                 prefix_delegation ? "true" : "false");
> >>> +    }
> >>>
> >>> -        bool ipv6_prefix = smap_get_bool(&op->nbrp->options,
> >>> +    bool ipv6_prefix = smap_get_bool(&op->nbrp->options,
> >>>                                       "prefix", false);
> >>
> >> Nit: this fits on a single line now.
> >>
> >>> -        if (!lrport_is_enabled(op->nbrp)) {
> >>> -            ipv6_prefix = false;
> >>> -        }
> >>> -        if (smap_get_bool(&options, "ipv6_prefix", false) !=
> ipv6_prefix) {
> >>> -            smap_add(&options, "ipv6_prefix",
> >>> -                     ipv6_prefix ? "true" : "false");
> >>> -        }
> >>> -        sbrec_port_binding_set_options(op->sb, &options);
> >>> +    if (!lrport_is_enabled(op->nbrp)) {
> >>> +        ipv6_prefix = false;
> >>> +    }
> >>> +    if (smap_get_bool(&options, "ipv6_prefix", false) != ipv6_prefix)
> {
> >>> +        smap_add(&options, "ipv6_prefix", ipv6_prefix ? "true" :
> "false");
> >>> +    }
> >>> +    sbrec_port_binding_set_options(op->sb, &options);
> >>>
> >>> -        smap_destroy(&options);
> >>> +    smap_destroy(&options);
> >>>
> >>> -        const char *address_mode = smap_get(
> >>> -            &op->nbrp->ipv6_ra_configs, "address_mode");
> >>> +    const char *address_mode = smap_get(&op->nbrp->ipv6_ra_configs,
> >>> +                                        "address_mode");
> >>>
> >>> -        if (!address_mode) {
> >>> -            continue;
> >>> -        }
> >>> -        if (strcmp(address_mode, "slaac") &&
> >>> -            strcmp(address_mode, "dhcpv6_stateful") &&
> >>> -            strcmp(address_mode, "dhcpv6_stateless")) {
> >>> -            static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(1, 5);
> >>> -            VLOG_WARN_RL(&rl, "Invalid address mode [%s] defined",
> >>> -                         address_mode);
> >>> -            continue;
> >>> -        }
> >>> +    if (!address_mode) {
> >>> +        return;
> >>> +    }
> >>> +    if (strcmp(address_mode, "slaac") &&
> >>> +        strcmp(address_mode, "dhcpv6_stateful") &&
> >>> +        strcmp(address_mode, "dhcpv6_stateless")) {
> >>> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> >>> +        VLOG_WARN_RL(&rl, "Invalid address mode [%s] defined",
> >>> +                     address_mode);
> >>> +        return;
> >>> +    }
> >>>
> >>> -        if (smap_get_bool(&op->nbrp->ipv6_ra_configs, "send_periodic",
> >>> -                          false)) {
> >>> -            copy_ra_to_sb(op, address_mode);
> >>> -        }
> >>> +    if (smap_get_bool(&op->nbrp->ipv6_ra_configs, "send_periodic",
> false)) {
> >>> +        copy_ra_to_sb(op, address_mode);
> >>> +    }
> >>> +}
> >>> +
> >>> +static void
> >>> +ovn_update_ipv6_options(struct hmap *lr_ports)
> >>> +{
> >>> +    struct ovn_port *op;
> >>> +    HMAP_FOR_EACH (op, key_node, lr_ports) {
> >>> +        ovs_assert(op->nbrp);
> >>
> >> I would move this assert inside ovn_update_ipv6_opt_for_op() just in
> >> case we ever want to call it in multiple places in the future.
> >>
> >>> +        ovn_update_ipv6_opt_for_op(op);
> >>>      }
> >>>  }
> >>>
> >>> @@ -18007,8 +18063,6 @@ ovnnb_db_run(struct northd_input *input_data,
> >>>          &data->lr_ports);
> >>>      stopwatch_stop(BUILD_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
> >>>      stopwatch_start(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
> >>> -    ovn_update_ipv6_options(&data->lr_ports);
> >>> -    ovn_update_ipv6_prefix(&data->lr_ports);
> >>>
> >>>      sync_mirrors(ovnsb_txn, input_data->nbrec_mirror_table,
> >>>                   input_data->sbrec_mirror_table);
> >>> @@ -18339,6 +18393,8 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
> >>>                                         &ha_ref_chassis_map);
> >>>      }
> >>>      shash_destroy(&ha_ref_chassis_map);
> >>> +
> >>> +    ovn_update_ipv6_prefix(lr_ports);
> >>>  }
> >>>
> >>>  const char *
> >>> diff --git a/northd/northd.h b/northd/northd.h
> >>> index 23521065e8..233dca8084 100644
> >>> --- a/northd/northd.h
> >>> +++ b/northd/northd.h
> >>> @@ -364,7 +364,8 @@ bool lflow_handle_northd_port_changes(struct
> ovsdb_idl_txn *ovnsb_txn,
> >>>                                        struct lflow_input *,
> >>>                                        struct hmap *lflows);
> >>>  bool northd_handle_sb_port_binding_changes(
> >>> -    const struct sbrec_port_binding_table *, struct hmap *ls_ports);
> >>> +    const struct sbrec_port_binding_table *, struct hmap *ls_ports,
> >>> +    struct hmap *lr_ports);
> >>>
> >>>  struct tracked_lb_data;
> >>>  bool northd_handle_lb_data_changes(struct tracked_lb_data *,
> >>> @@ -394,7 +395,8 @@ void sync_lbs(struct ovsdb_idl_txn *, const struct
> sbrec_load_balancer_table *,
> >>>                struct chassis_features *chassis_features);
> >>>  bool check_sb_lb_duplicates(const struct sbrec_load_balancer_table *);
> >>>
> >>> -void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports);
> >>> +void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports,
> >>> +              struct hmap *lr_ports);
> >>>  bool sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports
> *);
> >>>
> >>>  static inline bool
> >>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> >>> index 4a02dacf60..4edad24e53 100644
> >>> --- a/tests/ovn-northd.at
> >>> +++ b/tests/ovn-northd.at
> >>> @@ -10073,7 +10073,7 @@ check ovn-nbctl --wait=hv lsp-add ls0 lsp0-0
> -- lsp-set-addresses lsp0-0 "unknow
> >>>  ovs-vsctl add-port br-int lsp0-0 -- set interface lsp0-0
> external_ids:iface-id=lsp0-0
> >>>  wait_for_ports_up
> >>>  check ovn-nbctl --wait=hv sync
> >>> -check_recompute_counter 4 5 5 5 5 5
> >>> +check_recompute_counter 3 4 3 4 3 4
> >>>
> >>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>>  check ovn-nbctl --wait=hv lsp-add ls0 lsp0-1 -- lsp-set-addresses
> lsp0-1 "aa:aa:aa:00:00:01 192.168.0.11"
> >>> @@ -10235,6 +10235,126 @@ OVN_CLEANUP([hv1])
> >>>  AT_CLEANUP
> >>>  ])
> >>>
> >>> +OVN_FOR_EACH_NORTHD_NO_HV([
> >>> +AT_SETUP([SB Port binding incremental processing])
> >>> +ovn_start
> >>> +
> >>> +check_recompute_counter() {
> >>> +    northd_recomp=$(as northd ovn-appctl -t ovn-northd
> inc-engine/show-stats northd recompute)
> >>> +    echo "northd recompute count - $northd_recomp"
> >>> +    AT_CHECK([test x$northd_recomp = x$1])
> >>> +
> >>> +    lflow_recomp=$(as northd ovn-appctl -t ovn-northd
> inc-engine/show-stats lflow recompute)
> >>> +    echo "lflow recompute count - $lflow_recomp"
> >>> +    AT_CHECK([test x$lflow_recomp = x$2])
> >>> +}
> >>> +
> >>> +net_add n1
> >>> +sim_add hv1
> >>> +as hv1
> >>> +ovs-vsctl add-br br-phys
> >>> +ovn_attach n1 br-phys 192.168.0.11
> >>> +
> >>> +check ovn-nbctl --wait=sb ls-add ls0
> >>> +check ovn-nbctl --wait=sb lr-add lr0
> >>> +
> >>> +# Test normal VIF port
> >>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>> +check ovn-nbctl lsp-add ls0 p1
> >>> +check_recompute_counter 0 0
> >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>> +
> >>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>> +check ovn-nbctl lsp-set-addresses p1 "00:00:00:00:01:01 10.0.0.3"
> >>> +check_recompute_counter 0 0
> >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>> +
> >>> +check as northd ovn-appctl -t ovn-northd vlog/set info
> >>> +
> >>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>> +check ovn-sbctl lsp-bind p1 hv1
> >>> +check_recompute_counter 0 0
> >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>> +
> >>> +# Test lsp of type router
> >>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>> +check ovn-nbctl lsp-add ls0 rp -- lsp-set-type rp router
> >>> +
> >>> +# northd engine recomputes twice. Both the times for handling NB
> logical switch port
> >>> +# changes and not because of SB port binding changes.  This is
> because ovn-northd
> >>> +# sets the "up" to true.
> >>> +check_recompute_counter 2 2
> >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>> +
> >>> +# Set some options to 'rp'.  northd should only recompute once.
> >>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>> +check ovn-nbctl lsp-set-options rp foo=bar
> >>> +check_recompute_counter 1 1
> >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>> +
> >>> +# Test lsp of type external
> >>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>> +check ovn-nbctl lsp-add ls0 e1 -- lsp-set-type e1 external
> >>> +check_recompute_counter 2 2
> >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>> +
> >>> +# Set some options to 'e1'.  northd should only recompute once.
> >>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>> +check ovn-nbctl lsp-set-options e1 foo=bar
> >>> +check_recompute_counter 1 1
> >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>> +
> >>> +# Test lrp
> >>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>> +check ovn-nbctl lrp-add lr0 lrp 00:00:02:01:02:03 10.0.0.1/24
> >>> +check_recompute_counter 1 1
> >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>> +
> >>> +# Set some options on 'lrp'.  northd should only recompute once.
> >>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>> +check ovn-nbctl lrp-set-options lrp route_table=rtb-1
> >>> +check_recompute_counter 1 1
> >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>> +
> >>> +# Make lrp a gateway port
> >>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>> +check ovn-nbctl lrp-set-gateway-chassis lrp hv1
> >>> +# There will be 3 recomputes of northd engine node
> >>> +#   1. missing handler for input NB_logical_router
> >>> +#   2. missing handler for input SB_ha_chassis_group
> >>> +#   3. missing handler for input NB_logical_router when ovn-northd
> >>> +#      updates the hosting-chassis option in NB_logical_router_port.
> >>> +check_recompute_counter 3 3
> >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>> +
> >>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>> +check ovn-nbctl clear logical_router_port lrp gateway_chassis
> >>> +# There will be 2 recomputes of northd engine node
> >>> +#   1. missing handler for input NB_logical_router
> >>> +#   2. missing handler for input SB_ha_chassis_group
> >>> +check_recompute_counter 2 2
> >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>> +
> >>> +# Delete some of the ports.
> >>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>> +check ovn-nbctl lsp-del p1
> >>> +check_recompute_counter 0 0
> >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>> +
> >>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>> +check ovn-nbctl lsp-del e1
> >>> +check_recompute_counter 1 1
> >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>> +
> >>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>> +check ovn-nbctl lrp-del lrp
> >>> +check_recompute_counter 1 1
> >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>> +
> >>> +OVN_CLEANUP([hv1])
> >>> +AT_CLEANUP
> >>> +])
> >>> +
> >>>  OVN_FOR_EACH_NORTHD_NO_HV([
> >>>  AT_SETUP([ACL/Meter incremental processing - no northd recompute])
> >>>  ovn_start
> >>> @@ -10269,6 +10389,7 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE(1)
> >>>  AT_CLEANUP
> >>>  ])
> >>>
> >>> +
> >>>  OVN_FOR_EACH_NORTHD_NO_HV([
> >>>  AT_SETUP([check fip and lb flows])
> >>>  AT_KEYWORDS([fip-lb-flows])
> >>> @@ -10471,7 +10592,7 @@ ovn-nbctl lsp-set-addresses sw0-lr0
> 00:00:00:00:ff:01
> >>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>>  ovn-nbctl --wait=sb lsp-set-options sw0-lr0 router-port=lr0-sw0
> >>>  check_engine_stats lb_data norecompute compute
> >>> -check_engine_stats northd recompute nocompute
> >>> +check_engine_stats northd recompute compute
> >>>  check_engine_stats lflow recompute nocompute
> >>>  check_engine_stats sync_to_sb_lb recompute nocompute
> >>>
> >>> @@ -11114,12 +11235,14 @@ ovn_attach n1 br-phys 192.168.0.11
> >>>  ovn-sbctl chassis-add gw1 geneve 127.0.0.1
> >>>
> >>>  check ovn-nbctl ls-add sw0
> >>> -check ovn-nbctl lsp-add sw0 sw0p1 -- lsp-set-addresses sw0p1
> "00:00:20:20:12:01 10.0.0.4"
> >>> +check ovn-nbctl --wait=sb lsp-add sw0 sw0p1 -- lsp-set-addresses
> sw0p1 "00:00:20:20:12:01 10.0.0.4"
> >>>
> >>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>>  check ovn-nbctl --wait=sb lr-add lr0
> >>>  check_engine_stats northd recompute nocompute
> >>> +check_engine_stats sync_to_sb_pb recompute nocompute
> >>>  check_engine_stats lflow recompute nocompute
> >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>>
> >>>  # Adding a logical router port should result in recompute
> >>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>> @@ -11127,16 +11250,20 @@ check ovn-nbctl lrp-add lr0 lr0-sw0
> 00:00:00:00:ff:01 10.0.0.1/24
> >>>  # for northd engine there will be both recompute and compute
> >>>  # first it will be recompute to handle lr0-sw0 and then a compute
> >>>  # for the SB port binding change.
> >>> -check_engine_stats northd recompute nocompute
> >>> +check_engine_stats northd recompute compute
> >>> +check_engine_stats sync_to_sb_pb recompute nocompute
> >>>  check_engine_stats lflow recompute nocompute
> >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>>
> >>>  ovn-nbctl lsp-add sw0 sw0-lr0
> >>>  ovn-nbctl lsp-set-type sw0-lr0 router
> >>>  ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01
> >>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>>  check ovn-nbctl --wait=sb lsp-set-options sw0-lr0 router-port=lr0-sw0
> >>> -check_engine_stats northd recompute nocompute
> >>> +check_engine_stats northd recompute compute
> >>> +check_engine_stats sync_to_sb_pb recompute nocompute
> >>>  check_engine_stats lflow recompute nocompute
> >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>>
> >>>  ovn-nbctl ls-add public
> >>>  ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24
> >>> @@ -11158,7 +11285,9 @@ ovn-nbctl --wait=hv lrp-set-gateway-chassis
> lr0-public hv1 20
> >>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>>  check ovn-nbctl set logical_router_port lr0-sw0 options:foo=bar
> >>>  check_engine_stats northd recompute nocompute
> >>> +check_engine_stats sync_to_sb_pb recompute nocompute
> >>>  check_engine_stats lflow recompute nocompute
> >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>>
> >>>  # Do checks for NATs.
> >>>  # Add a NAT. This should not result in recompute of both northd and
> lflow
> >>> @@ -11167,6 +11296,7 @@ check as northd ovn-appctl -t ovn-northd
> inc-engine/clear-stats
> >>>  check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat  172.168.0.110
> 10.0.0.4
> >>>  check_engine_stats northd recompute nocompute
> >>>  check_engine_stats lflow recompute nocompute
> >>> +check_engine_stats sync_to_sb_pb recompute nocompute
> >>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>>
> >>>  # Update the NAT options column
> >>> @@ -11174,6 +11304,7 @@ check as northd ovn-appctl -t ovn-northd
> inc-engine/clear-stats
> >>>  check ovn-nbctl --wait=sb set NAT . options:foo=bar
> >>>  check_engine_stats northd recompute nocompute
> >>>  check_engine_stats lflow recompute nocompute
> >>> +check_engine_stats sync_to_sb_pb recompute nocompute
> >>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>>
> >>>  # Update the NAT external_ip column
> >>> @@ -11181,6 +11312,7 @@ check as northd ovn-appctl -t ovn-northd
> inc-engine/clear-stats
> >>>  check ovn-nbctl --wait=sb set NAT . external_ip=172.168.0.120
> >>>  check_engine_stats northd recompute nocompute
> >>>  check_engine_stats lflow recompute nocompute
> >>> +check_engine_stats sync_to_sb_pb recompute nocompute
> >>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>>
> >>>  # Update the NAT logical_ip column
> >>> @@ -11188,6 +11320,7 @@ check as northd ovn-appctl -t ovn-northd
> inc-engine/clear-stats
> >>>  check ovn-nbctl --wait=sb set NAT . logical_ip=10.0.0.10
> >>>  check_engine_stats northd recompute nocompute
> >>>  check_engine_stats lflow recompute nocompute
> >>> +check_engine_stats sync_to_sb_pb recompute nocompute
> >>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>>
> >>>  # Update the NAT type
> >>> @@ -11195,13 +11328,15 @@ check as northd ovn-appctl -t ovn-northd
> inc-engine/clear-stats
> >>>  check ovn-nbctl --wait=sb set NAT . type=snat
> >>>  check_engine_stats northd recompute nocompute
> >>>  check_engine_stats lflow recompute nocompute
> >>> +check_engine_stats sync_to_sb_pb recompute nocompute
> >>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>>
> >>>  # Create a dnat_and_snat NAT with external_mac and logical_port
> >>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>>  check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.110
> 10.0.0.4 sw0p1 30:54:00:00:00:03
> >>> -check_engine_stats northd recompute nocompute
> >>> +check_engine_stats northd recompute compute
> >>>  check_engine_stats lflow recompute nocompute
> >>> +check_engine_stats sync_to_sb_pb recompute nocompute
> >>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>>
> >>>  nat2_uuid=$(ovn-nbctl --bare --columns _uuid find nat
> logical_ip=10.0.0.4)
> >>> @@ -11210,6 +11345,7 @@ check as northd ovn-appctl -t ovn-northd
> inc-engine/clear-stats
> >>>  check ovn-nbctl --wait=sb set NAT $nat2_uuid
> external_mac='"30:54:00:00:00:04"'
> >>>  check_engine_stats northd recompute nocompute
> >>>  check_engine_stats lflow recompute nocompute
> >>> +check_engine_stats sync_to_sb_pb recompute nocompute
> >>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>>
> >>>  # Create a load balancer and add the lb vip as NAT
> >>> @@ -11223,31 +11359,35 @@ check ovn-nbctl lr-lb-add lr0 lb2
> >>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>>  check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.140
> 10.0.0.20
> >>>  check_engine_stats northd recompute nocompute
> >>> +check_engine_stats sync_to_sb_pb recompute nocompute
> >>>  check_engine_stats lflow recompute nocompute
> >>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>>
> >>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>>  check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.150
> 10.0.0.41
> >>>  check_engine_stats northd recompute nocompute
> >>> +check_engine_stats sync_to_sb_pb recompute nocompute
> >>>  check_engine_stats lflow recompute nocompute
> >>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>>
> >>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>>  check ovn-nbctl --wait=sb lr-nat-del lr0 dnat_and_snat 172.168.0.150
> >>>  check_engine_stats northd recompute nocompute
> >>> +check_engine_stats sync_to_sb_pb recompute nocompute
> >>>  check_engine_stats lflow recompute nocompute
> >>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>>
> >>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>>  check ovn-nbctl --wait=sb lr-nat-del lr0 dnat_and_snat 172.168.0.140
> >>>  check_engine_stats northd recompute nocompute
> >>> +check_engine_stats sync_to_sb_pb recompute nocompute
> >>>  check_engine_stats lflow recompute nocompute
> >>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>>
> >>>  # Delete the NAT
> >>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>>  check ovn-nbctl --wait=sb clear logical_router lr0 nat
> >>> -check_engine_stats northd recompute nocompute
> >>> +check_engine_stats northd recompute compute
> >>>  check_engine_stats lflow recompute nocompute
> >>>  check_engine_stats sync_to_sb_pb recompute nocompute
> >>>  CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>> @@ -11256,12 +11396,16 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>>  check ovn-nbctl --wait=sb lr-policy-add lr0  10 "ip4.src == 10.0.0.3"
> reroute 172.168.0.101,172.168.0.102
> >>>  check_engine_stats northd recompute nocompute
> >>> +check_engine_stats sync_to_sb_pb recompute nocompute
> >>>  check_engine_stats lflow recompute nocompute
> >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>>
> >>>  check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>>  check ovn-nbctl --wait=sb lr-policy-del lr0  10 "ip4.src == 10.0.0.3"
> >>>  check_engine_stats northd recompute nocompute
> >>> +check_engine_stats sync_to_sb_pb recompute nocompute
> >>>  check_engine_stats lflow recompute nocompute
> >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>>
> >>>  OVN_CLEANUP([hv1])
> >>>  AT_CLEANUP
> >>
> >> The rest looks good to me but I'd prefer we figure out the test failure
> >> before acking the patch.
> >>
> >> Regards,
> >> Dumitru
> >>
> >>
> >> _______________________________________________
> >> 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 1/25/24 22:01, Numan Siddique wrote:
> On Thu, Jan 25, 2024, 3:29 PM Dumitru Ceara <dceara@redhat.com> wrote:
> 
>> On 1/25/24 17:46, Numan Siddique wrote:
>>> On Tue, Jan 16, 2024 at 6:15 AM Dumitru Ceara <dceara@redhat.com> wrote:
>>>>
>>>> On 1/11/24 16:28, numans@ovn.org wrote:
>>>>> From: Numan Siddique <numans@ovn.org>
>>>>>
>>>>> It also moves the logical router port IPv6 prefix delegation
>>>>> updates to "sync-from-sb" engine node.
>>>>>
>>>>> With these changes, northd engine node doesn't need to do much
>>>>> for SB Port binding changes other than updating 'op->sb'.  And
>>>>> there is no need to fall back to recompute.  Prior to this patch
>>>>> northd_handle_sb_port_binding_changes() was returning false for
>>>>> all SB port binding updates except for the VIF PBs.  This patch
>>>>> now handles updates to all the SB port binding by setting 'op->sb'.
>>>>>
>>>>> Signed-off-by: Numan Siddique <numans@ovn.org>
>>>>> ---
>>>>
>>>> Hi Numan,
>>>>
>>>> With this patch applied the "633: ovn-northd.at:10298 SB Port binding
>>>> incremental processing -- parallelization=yes" test occasionally fails,
>>>> e.g.:
>>>>
>>>> https://github.com/dceara/ovn/actions/runs/7539663594/job/20524238508
>>>>
>>>> I saw it locally too when running the test in a loop:
>>>>
>>>> It fails with:
>>>> ovn-nbctl lrp-set-gateway-chassis lrp hv1
>>>> ./ovn-macros.at:449: "$@"
>>>> northd recompute count - 2
>>>> ./ovn-northd.at:10238: test x$northd_recomp = x$1
>>>> ./ovn-northd.at:10238: exit code was 1, expected 0
>>>>
>>>> Here:
>>>>
>> https://github.com/dceara/ovn/blob/e1c76b6d347570618591fe6587f6af83445223ec/tests/ovn-northd.at#L10327
>>>>
>>>> For some reason in some cases only 2 recomputes get triggered.
>>>
>>> Thanks Dumitru and Han for the reviews.
>>>
>>> @Dumitru Ceara  I've addressed all the review comments below.
>>>
>>> I think the test case is failing because of the missing "--wait=sb" in
>>> the ovn-nbctl commands.
>>>
>>
>> I don't think it's only that.  It still fails even with your updated
>> test changes, in the same way:
>>
>> as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>> ./ovn-macros.at:449: "$@"
>> ovn-nbctl lrp-set-gateway-chassis lrp hv1
>> ./ovn-macros.at:449: "$@"
>> northd recompute count - 2
>> ./ovn-northd.at:10238: test x$northd_recomp = x$1
>> ./ovn-northd.at:10238: exit code was 1, expected 0
>>
>>> Will the below changes in ovn-northd.at look good to you  ?  If so,
>>> and if you can provide your ack I can apply this patch.
>>> Otherwise I'll include it in the v6.
>>>
>>
>> I would prefer if we don't apply it until we figure out this test, the
>> CI has been relatively stable the last few weeks, I'd rather not
>> destabilize it if possible.
>>
> 
> Sure.  Definitely.
> 
> Its passing for me locally when run in a loop.
> 
> Let me dig into it further.
> 
> 

Sorry, Numan, I had made a mistake when applying your incremental patch.
 With the right changes applied it passes now for me too.

Acked-by: Dumitru Ceara <dceara@redhat.com>

Thanks,
Dumitru

On Fri, Jan 26, 2024 at 4:30 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 1/25/24 22:01, Numan Siddique wrote:
> > On Thu, Jan 25, 2024, 3:29 PM Dumitru Ceara <dceara@redhat.com> wrote:
> >
> >> On 1/25/24 17:46, Numan Siddique wrote:
> >>> On Tue, Jan 16, 2024 at 6:15 AM Dumitru Ceara <dceara@redhat.com> wrote:
> >>>>
> >>>> On 1/11/24 16:28, numans@ovn.org wrote:
> >>>>> From: Numan Siddique <numans@ovn.org>
> >>>>>
> >>>>> It also moves the logical router port IPv6 prefix delegation
> >>>>> updates to "sync-from-sb" engine node.
> >>>>>
> >>>>> With these changes, northd engine node doesn't need to do much
> >>>>> for SB Port binding changes other than updating 'op->sb'.  And
> >>>>> there is no need to fall back to recompute.  Prior to this patch
> >>>>> northd_handle_sb_port_binding_changes() was returning false for
> >>>>> all SB port binding updates except for the VIF PBs.  This patch
> >>>>> now handles updates to all the SB port binding by setting 'op->sb'.
> >>>>>
> >>>>> Signed-off-by: Numan Siddique <numans@ovn.org>
> >>>>> ---
> >>>>
> >>>> Hi Numan,
> >>>>
> >>>> With this patch applied the "633: ovn-northd.at:10298 SB Port binding
> >>>> incremental processing -- parallelization=yes" test occasionally fails,
> >>>> e.g.:
> >>>>
> >>>> https://github.com/dceara/ovn/actions/runs/7539663594/job/20524238508
> >>>>
> >>>> I saw it locally too when running the test in a loop:
> >>>>
> >>>> It fails with:
> >>>> ovn-nbctl lrp-set-gateway-chassis lrp hv1
> >>>> ./ovn-macros.at:449: "$@"
> >>>> northd recompute count - 2
> >>>> ./ovn-northd.at:10238: test x$northd_recomp = x$1
> >>>> ./ovn-northd.at:10238: exit code was 1, expected 0
> >>>>
> >>>> Here:
> >>>>
> >> https://github.com/dceara/ovn/blob/e1c76b6d347570618591fe6587f6af83445223ec/tests/ovn-northd.at#L10327
> >>>>
> >>>> For some reason in some cases only 2 recomputes get triggered.
> >>>
> >>> Thanks Dumitru and Han for the reviews.
> >>>
> >>> @Dumitru Ceara  I've addressed all the review comments below.
> >>>
> >>> I think the test case is failing because of the missing "--wait=sb" in
> >>> the ovn-nbctl commands.
> >>>
> >>
> >> I don't think it's only that.  It still fails even with your updated
> >> test changes, in the same way:
> >>
> >> as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >> ./ovn-macros.at:449: "$@"
> >> ovn-nbctl lrp-set-gateway-chassis lrp hv1
> >> ./ovn-macros.at:449: "$@"
> >> northd recompute count - 2
> >> ./ovn-northd.at:10238: test x$northd_recomp = x$1
> >> ./ovn-northd.at:10238: exit code was 1, expected 0
> >>
> >>> Will the below changes in ovn-northd.at look good to you  ?  If so,
> >>> and if you can provide your ack I can apply this patch.
> >>> Otherwise I'll include it in the v6.
> >>>
> >>
> >> I would prefer if we don't apply it until we figure out this test, the
> >> CI has been relatively stable the last few weeks, I'd rather not
> >> destabilize it if possible.
> >>
> >
> > Sure.  Definitely.
> >
> > Its passing for me locally when run in a loop.
> >
> > Let me dig into it further.
> >
> >
>
> Sorry, Numan, I had made a mistake when applying your incremental patch.
>  With the right changes applied it passes now for me too.
>
> Acked-by: Dumitru Ceara <dceara@redhat.com>

Thanks.  I applied this patch to the main.

Patches 1,2 and 3 are merged.  I'll submit the rest of the patches in
v6 and will apply those as one series
after all the reviews and acks.

Thanks
Numan

>
> Thanks,
> Dumitru
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev