[ovs-dev,v5] controller: avoid recomputes triggered by SBDB Port_Binding updates.

When VIF ports are claimed on a chassis, SBDB Port_Binding table is updated.
If the SBDB IDL is still is read-only ("in transaction") when such a update
is required, the update is not possible and recompute is triggered through
I+P failure.

This situation can happen:
- after updating Port_Binding->chassis to SBDB for one port, in a following
  iteration, ovn-controller handles Interface:external_ids:ovn-installed
  (for the same port) while SBDB is still read-only.
- after updating Port_Binding->chassis to SBDB for one port, in a following
  iteration, ovn-controller updates Port_Binding->chassis for another port,
  while SBDB is still read-only.

This patch prevent the recompute, by having the if-status module
updating the Port_Binding chassis (if needed) when possible.
This does not delay Port_Binding chassis update compared to before this patch.
- With the patch, Port_Binding chassis will be updated as soon as SBDB is
again writable, without recompute.
- Without the patch, Port_Binding chassis was updated as soon as SBDB was
again writable, through a recompute.

As part of this patch, ovn-installed will not be updated for additional chassis;
it will only be updated when the migration is completed.

Note that this patch also fixes an issue where port was not always properly
reported up for virtual ports (causing flaky failures in "virtual ports"
test case).

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2059253
Signed-off-by: Xavier Simonart <xsimonar@redhat.com>

---
v2:  - handled Dumitru's comments.
     - handled Han's comments, mainly ensure we moved out of CLAIMED state
       only after updating pb->chassis to guarentee physical flows are installed
       when ovn-installed is updated in OVS.
     - slighly reorganize the code to isolate 'notify_up = false' cases in
       claim_port (i.e. ports such as virtual ports), in the idea of making
       future patch preventing recomputes when virtual ports are claimed.
     - updated test case to cause more race conditions.
     - rebased on origin/main
     - note that "additional chassis" as now supported by
       "Support LSP:options:requested-chassis as a list" might still cause
       recomputes.
     - fixed missing flows when Port_Binding chassis was updated by mgr_update
       w/o any lflow recalculation.
v3:  - handled Dumitru's comments on v2, mainly have runtime_data handler
       handling pb_claims when sb becomes writable (instead of a lflow handler).
     - fixed test as it was not checking recomputes on all hv, as well as a flaky
v4:  - handled Han's comments, mainly update pb->chassis until it's confirmed.
     - updated doc to reflect that pb->chassis update can happen at any state, and
       the CLAIMED state is only for the initial attempt.
v5:  - rebased on origin/main.

Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
---
 controller/binding.c        | 174 +++++++++++++++++++++++++---------
 controller/binding.h        |  18 +++-
 controller/if-status.c      | 183 ++++++++++++++++++++++++++++++++----
 controller/if-status.h      |  17 +++-
 controller/ovn-controller.c |  72 +++++++++++++-
 tests/ovn-macros.at         |  11 +++
 tests/ovn.at                | 134 +++++++++++++++++++++++++-
 tests/perf-northd.at        |  19 +---
 8 files changed, 540 insertions(+), 88 deletions(-)

Bleep bloop.  Greetings Xavier Simonart, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


git-am:
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0001 controller: avoid recomputes triggered by SBDB Port_Binding updates.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot

On Fri, Aug 19, 2022 at 8:40 AM Xavier Simonart <xsimonar@redhat.com> wrote:
>
> When VIF ports are claimed on a chassis, SBDB Port_Binding table is
updated.
> If the SBDB IDL is still is read-only ("in transaction") when such a
update
> is required, the update is not possible and recompute is triggered through
> I+P failure.
>
> This situation can happen:
> - after updating Port_Binding->chassis to SBDB for one port, in a
following
>   iteration, ovn-controller handles Interface:external_ids:ovn-installed
>   (for the same port) while SBDB is still read-only.
> - after updating Port_Binding->chassis to SBDB for one port, in a
following
>   iteration, ovn-controller updates Port_Binding->chassis for another
port,
>   while SBDB is still read-only.
>
> This patch prevent the recompute, by having the if-status module
> updating the Port_Binding chassis (if needed) when possible.
> This does not delay Port_Binding chassis update compared to before this
patch.
> - With the patch, Port_Binding chassis will be updated as soon as SBDB is
> again writable, without recompute.
> - Without the patch, Port_Binding chassis was updated as soon as SBDB was
> again writable, through a recompute.
>
> As part of this patch, ovn-installed will not be updated for additional
chassis;
> it will only be updated when the migration is completed.
>
> Note that this patch also fixes an issue where port was not always
properly
> reported up for virtual ports (causing flaky failures in "virtual ports"
> test case).
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2059253
> Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
>
> ---
> v2:  - handled Dumitru's comments.
>      - handled Han's comments, mainly ensure we moved out of CLAIMED state
>        only after updating pb->chassis to guarentee physical flows are
installed
>        when ovn-installed is updated in OVS.
>      - slighly reorganize the code to isolate 'notify_up = false' cases in
>        claim_port (i.e. ports such as virtual ports), in the idea of
making
>        future patch preventing recomputes when virtual ports are claimed.
>      - updated test case to cause more race conditions.
>      - rebased on origin/main
>      - note that "additional chassis" as now supported by
>        "Support LSP:options:requested-chassis as a list" might still cause
>        recomputes.
>      - fixed missing flows when Port_Binding chassis was updated by
mgr_update
>        w/o any lflow recalculation.
> v3:  - handled Dumitru's comments on v2, mainly have runtime_data handler
>        handling pb_claims when sb becomes writable (instead of a lflow
handler).
>      - fixed test as it was not checking recomputes on all hv, as well as
a flaky
> v4:  - handled Han's comments, mainly update pb->chassis until it's
confirmed.
>      - updated doc to reflect that pb->chassis update can happen at any
state, and
>        the CLAIMED state is only for the initial attempt.

Thanks Xavier. All looks good to me except that it seems you updated the
diagram but forgot to update the description, I have the below small
incremental change, and if you are ok, I can merge it without the need for
v6:

diff --git a/controller/if-status.c b/controller/if-status.c
index 2590ec27f..d1c14ac30 100644
--- a/controller/if-status.c
+++ b/controller/if-status.c
@@ -54,11 +54,12 @@ VLOG_DEFINE_THIS_MODULE(if_status);
  */

 enum if_state {
-    OIF_CLAIMED,       /* Newly claimed interface. pb->chassis not yet
updated.
-                        */
-    OIF_INSTALL_FLOWS, /* Claimed interface with pb->chassis successfully
-                        * updated in SB and for which flows are still being
-                        * installed.
+    OIF_CLAIMED,       /* Newly claimed interface. pb->chassis update not
yet
+                          initiated. */
+    OIF_INSTALL_FLOWS, /* Claimed interface with pb->chassis update sent to
+                        * SB (but update notification not confirmed, so the
+                        * update may be resent in any of the following
states)
+                        * and for which flows are still being installed.
                         */
     OIF_MARK_UP,       /* Interface with flows successfully installed in
OVS
                         * but not yet marked "up" in the binding module (in

Thanks,
Han

> v5:  - rebased on origin/main.
>
> Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
> ---
>  controller/binding.c        | 174 +++++++++++++++++++++++++---------
>  controller/binding.h        |  18 +++-
>  controller/if-status.c      | 183 ++++++++++++++++++++++++++++++++----
>  controller/if-status.h      |  17 +++-
>  controller/ovn-controller.c |  72 +++++++++++++-
>  tests/ovn-macros.at         |  11 +++
>  tests/ovn.at                | 134 +++++++++++++++++++++++++-
>  tests/perf-northd.at        |  19 +---
>  8 files changed, 540 insertions(+), 88 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 9f5393a92..74a156525 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -707,11 +707,17 @@ local_binding_get_lport_ofport(const struct shash
*local_bindings,
>  }
>
>  bool
> -local_binding_is_up(struct shash *local_bindings, const char *pb_name)
> +local_binding_is_up(struct shash *local_bindings, const char *pb_name,
> +                    const struct sbrec_chassis *chassis_rec)
>  {
>      struct local_binding *lbinding =
>          local_binding_find(local_bindings, pb_name);
>      struct binding_lport *b_lport =
local_binding_get_primary_lport(lbinding);
> +
> +    if (b_lport && b_lport->pb->chassis != chassis_rec) {
> +        return false;
> +    }
> +
>      if (lbinding && b_lport && lbinding->iface) {
>          if (b_lport->pb->n_up && !b_lport->pb->up[0]) {
>              return false;
> @@ -723,13 +729,23 @@ local_binding_is_up(struct shash *local_bindings,
const char *pb_name)
>  }
>
>  bool
> -local_binding_is_down(struct shash *local_bindings, const char *pb_name)
> +local_binding_is_down(struct shash *local_bindings, const char *pb_name,
> +                      const struct sbrec_chassis *chassis_rec)
>  {
>      struct local_binding *lbinding =
>          local_binding_find(local_bindings, pb_name);
>
>      struct binding_lport *b_lport =
local_binding_get_primary_lport(lbinding);
>
> +    if (b_lport) {
> +        if (b_lport->pb->chassis == chassis_rec) {
> +            return false;
> +        } else if (b_lport->pb->chassis) {
> +            VLOG_DBG("lport %s already claimed by other chassis",
> +                     b_lport->pb->logical_port);
> +        }
> +    }
> +
>      if (!lbinding) {
>          return true;
>      }
> @@ -947,37 +963,95 @@ get_lport_type_str(enum en_lport_type lport_type)
>      OVS_NOT_REACHED();
>  }
>
> -/* For newly claimed ports, if 'notify_up' is 'false':
> +void
> +set_pb_chassis_in_sbrec(const struct sbrec_port_binding *pb,
> +                        const struct sbrec_chassis *chassis_rec,
> +                        bool is_set)
> +{
> +    if (pb->chassis != chassis_rec) {
> +         if (is_set) {
> +            if (pb->chassis) {
> +                VLOG_INFO("Changing chassis for lport %s from %s to %s.",
> +                          pb->logical_port, pb->chassis->name,
> +                          chassis_rec->name);
> +            } else {
> +                VLOG_INFO("Claiming lport %s for this chassis.",
> +                          pb->logical_port);
> +            }
> +            for (int i = 0; i < pb->n_mac; i++) {
> +                VLOG_INFO("%s: Claiming %s", pb->logical_port,
pb->mac[i]);
> +            }
> +            sbrec_port_binding_set_chassis(pb, chassis_rec);
> +        }
> +    } else if (!is_set) {
> +        sbrec_port_binding_set_chassis(pb, NULL);
> +    }
> +}
> +
> +bool
> +local_bindings_pb_chassis_is_set(struct shash *local_bindings,
> +                                 const char *pb_name,
> +                                 const struct sbrec_chassis *chassis_rec)
> +{
> +    struct local_binding *lbinding =
> +        local_binding_find(local_bindings, pb_name);
> +    struct binding_lport *b_lport =
local_binding_get_primary_lport(lbinding);
> +
> +    if (b_lport && b_lport->pb && b_lport->pb->chassis == chassis_rec) {
> +        return true;
> +    }
> +    return false;
> +}
> +
> +void
> +local_binding_set_pb(struct shash *local_bindings, const char *pb_name,
> +                     const struct sbrec_chassis *chassis_rec,
> +                     struct hmap *tracked_datapaths, bool is_set)
> +{
> +    struct local_binding *lbinding =
> +        local_binding_find(local_bindings, pb_name);
> +    struct binding_lport *b_lport =
local_binding_get_primary_lport(lbinding);
> +
> +    if (b_lport) {
> +        set_pb_chassis_in_sbrec(b_lport->pb, chassis_rec, is_set);
> +        if (tracked_datapaths) {
> +            update_lport_tracking(b_lport->pb, tracked_datapaths, true);
> +        }
> +    }
> +}
> +
> +/* For newly claimed ports:
>   * - set the 'pb.up' field to true if 'pb' has no 'parent_pb'.
>   * - set the 'pb.up' field to true if 'parent_pb.up' is 'true' (e.g., for
>   *   container and virtual ports).
> - * Otherwise request a notification to be sent when the OVS flows
> - * corresponding to 'pb' have been installed.
> + *
> + * Returns false if lport is not claimed due to 'sb_readonly'.
> + * Returns true otherwise.
>   *
>   * Note:
> - *   Updates (directly or through a notification) the 'pb->up' field
only if
> - *   it's explicitly set to 'false'.
> + *   Updates the 'pb->up' field only if it's explicitly set to 'false'.
>   *   This is to ensure compatibility with older versions of ovn-northd.
>   */
> -static void
> +static bool
>  claimed_lport_set_up(const struct sbrec_port_binding *pb,
>                       const struct sbrec_port_binding *parent_pb,
> -                     const struct sbrec_chassis *chassis_rec,
> -                     bool notify_up, struct if_status_mgr *if_mgr)
> +                     bool sb_readonly)
>  {
> -    if (!notify_up) {
> -        bool up = true;
> -        if (!parent_pb || (parent_pb->n_up && parent_pb->up[0])) {
> +    /* When notify_up is false in claim_port(), no state is created
> +     * by if_status_mgr. In such cases, return false (i.e. trigger
recompute)
> +     * if we can't update sb (because it is readonly).
> +     */
> +    bool up = true;
> +    if (!parent_pb || (parent_pb->n_up && parent_pb->up[0])) {
> +        if (!sb_readonly) {
>              if (pb->n_up) {
>                  sbrec_port_binding_set_up(pb, &up, 1);
>              }
> +        } else if (pb->n_up && !pb->up[0]) {
> +            return false;
>          }
> -        return;
> -    }
> -
> -    if (pb->chassis != chassis_rec || (pb->n_up && !pb->up[0])) {
> -        if_status_mgr_claim_iface(if_mgr, pb->logical_port);
>      }
> +    return true;
>  }
>
>  typedef void (*set_func)(const struct sbrec_port_binding *pb,
> @@ -1085,44 +1159,51 @@ claim_lport(const struct sbrec_port_binding *pb,
>              struct if_status_mgr *if_mgr,
>              struct sset *postponed_ports)
>  {
> -    if (!sb_readonly) {
> -        claimed_lport_set_up(pb, parent_pb, chassis_rec, notify_up,
if_mgr);
> -    }
> -
>      enum can_bind can_bind = lport_can_bind_on_this_chassis(chassis_rec,
pb);
>      bool update_tracked = false;
>
>      if (can_bind == CAN_BIND_AS_MAIN) {
>          if (pb->chassis != chassis_rec) {
> -            if (sb_readonly) {
> -                return false;
> -            }
> -
>              long long int now = time_msec();
>              if (pb->chassis) {
>                  if (lport_maybe_postpone(pb->logical_port, now,
>                                           postponed_ports)) {
>                      return true;
>                  }
> -                VLOG_INFO("Changing chassis for lport %s from %s to %s.",
> -                        pb->logical_port, pb->chassis->name,
> -                        chassis_rec->name);
> -            } else {
> -                VLOG_INFO("Claiming lport %s for this chassis.",
> -                          pb->logical_port);
> -            }
> -            for (size_t i = 0; i < pb->n_mac; i++) {
> -                VLOG_INFO("%s: Claiming %s", pb->logical_port,
pb->mac[i]);
>              }
> -
> -            sbrec_port_binding_set_chassis(pb, chassis_rec);
>              if (is_additional_chassis(pb, chassis_rec)) {
> +                if (sb_readonly) {
> +                    return false;
> +                }
>                  remove_additional_chassis(pb, chassis_rec);
>              }
>              update_tracked = true;
>
> +            if (!notify_up) {
> +                if (!claimed_lport_set_up(pb, parent_pb, sb_readonly)) {
> +                    return false;
> +                }
> +                if (sb_readonly) {
> +                    return false;
> +                }
> +                set_pb_chassis_in_sbrec(pb, chassis_rec, true);
> +            } else {
> +                if_status_mgr_claim_iface(if_mgr, pb, chassis_rec,
> +                                          sb_readonly);
> +            }
>              register_claim_timestamp(pb->logical_port, now);
>              sset_find_and_delete(postponed_ports, pb->logical_port);
> +        } else {
> +            if (!notify_up) {
> +                if (!claimed_lport_set_up(pb, parent_pb, sb_readonly)) {
> +                    return false;
> +                }
> +            } else {
> +                if (pb->n_up && !pb->up[0]) {
> +                    if_status_mgr_claim_iface(if_mgr, pb, chassis_rec,
> +                                              sb_readonly);
> +                }
> +            }
>          }
>      } else if (can_bind == CAN_BIND_AS_ADDITIONAL) {
>          if (!is_additional_chassis(pb, chassis_rec)) {
> @@ -1171,7 +1252,8 @@ claim_lport(const struct sbrec_port_binding *pb,
>   */
>  static bool
>  release_lport_main_chassis(const struct sbrec_port_binding *pb,
> -                           bool sb_readonly)
> +                           bool sb_readonly,
> +                           struct if_status_mgr *if_mgr)
>  {
>      if (pb->encap) {
>          if (sb_readonly) {
> @@ -1180,11 +1262,14 @@ release_lport_main_chassis(const struct
sbrec_port_binding *pb,
>          sbrec_port_binding_set_encap(pb, NULL);
>      }
>
> +    /* If sb is readonly, pb->chassis is unset through if-status if
present. */
> +
>      if (pb->chassis) {
> -        if (sb_readonly) {
> +        if (!sb_readonly) {
> +            sbrec_port_binding_set_chassis(pb, NULL);
> +        } else if (!if_status_mgr_iface_is_present(if_mgr,
pb->logical_port)) {
>              return false;
>          }
> -        sbrec_port_binding_set_chassis(pb, NULL);
>      }
>
>      if (pb->virtual_parent) {
> @@ -1194,7 +1279,9 @@ release_lport_main_chassis(const struct
sbrec_port_binding *pb,
>          sbrec_port_binding_set_virtual_parent(pb, NULL);
>      }
>
> -    VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port);
> +    VLOG_INFO("Releasing lport %s from this chassis (sb_readonly=%d)",
> +              pb->logical_port, sb_readonly);
> +
>      return true;
>  }
>
> @@ -1228,7 +1315,7 @@ release_lport(const struct sbrec_port_binding *pb,
>                struct hmap *tracked_datapaths, struct if_status_mgr
*if_mgr)
>  {
>      if (pb->chassis == chassis_rec) {
> -        if (!release_lport_main_chassis(pb, sb_readonly)) {
> +        if (!release_lport_main_chassis(pb, sb_readonly, if_mgr)) {
>              return false;
>          }
>      } else if (is_additional_chassis(pb, chassis_rec)) {
> @@ -1567,7 +1654,8 @@ consider_localport(const struct sbrec_port_binding
*pb,
>      enum can_bind can_bind = lport_can_bind_on_this_chassis(
>          b_ctx_in->chassis_rec, pb);
>      if (can_bind == CAN_BIND_AS_MAIN) {
> -        if (!release_lport_main_chassis(pb, !b_ctx_in->ovnsb_idl_txn)) {
> +        if (!release_lport_main_chassis(pb, !b_ctx_in->ovnsb_idl_txn,
> +            b_ctx_out->if_mgr)) {
>              return false;
>          }
>      } else if (can_bind == CAN_BIND_AS_ADDITIONAL) {
> diff --git a/controller/binding.h b/controller/binding.h
> index b2360bac2..ad959a9e6 100644
> --- a/controller/binding.h
> +++ b/controller/binding.h
> @@ -153,8 +153,11 @@ const struct sbrec_port_binding
*local_binding_get_primary_pb(
>  ofp_port_t local_binding_get_lport_ofport(const struct shash
*local_bindings,
>                                            const char *pb_name);
>
> -bool local_binding_is_up(struct shash *local_bindings, const char
*pb_name);
> -bool local_binding_is_down(struct shash *local_bindings, const char
*pb_name);
> +bool local_binding_is_up(struct shash *local_bindings, const char
*pb_name,
> +                         const struct sbrec_chassis *);
> +bool local_binding_is_down(struct shash *local_bindings, const char
*pb_name,
> +                           const struct sbrec_chassis *);
> +
>  void local_binding_set_up(struct shash *local_bindings, const char
*pb_name,
>                            const struct sbrec_chassis *chassis_rec,
>                            const char *ts_now_str, bool sb_readonly,
> @@ -162,6 +165,13 @@ void local_binding_set_up(struct shash
*local_bindings, const char *pb_name,
>  void local_binding_set_down(struct shash *local_bindings, const char
*pb_name,
>                              const struct sbrec_chassis *chassis_rec,
>                              bool sb_readonly, bool ovs_readonly);
> +void local_binding_set_pb(struct shash *local_bindings, const char
*pb_name,
> +                          const struct sbrec_chassis *chassis_rec,
> +                          struct hmap *tracked_datapaths,
> +                          bool is_set);
> +bool local_bindings_pb_chassis_is_set(struct shash *local_bindings,
> +                                      const char *pb_name,
> +                                      const struct sbrec_chassis
*chassis_rec);
>
>  void binding_register_ovs_idl(struct ovsdb_idl *);
>  void binding_run(struct binding_ctx_in *, struct binding_ctx_out *);
> @@ -180,6 +190,10 @@ void binding_dump_local_bindings(struct
local_binding_data *, struct ds *);
>  bool is_additional_chassis(const struct sbrec_port_binding *pb,
>                             const struct sbrec_chassis *chassis_rec);
>
> +void set_pb_chassis_in_sbrec(const struct sbrec_port_binding *pb,
> +                             const struct sbrec_chassis *chassis_rec,
> +                             bool is_set);
> +
>  /* Corresponds to each Port_Binding.type. */
>  enum en_lport_type {
>      LP_UNKNOWN,
> diff --git a/controller/if-status.c b/controller/if-status.c
> index ad61844d8..2590ec27f 100644
> --- a/controller/if-status.c
> +++ b/controller/if-status.c
> @@ -24,6 +24,7 @@
>  #include "lib/util.h"
>  #include "timeval.h"
>  #include "openvswitch/vlog.h"
> +#include "lib/ovn-sb-idl.h"
>
>  VLOG_DEFINE_THIS_MODULE(if_status);
>
> @@ -53,9 +54,11 @@ VLOG_DEFINE_THIS_MODULE(if_status);
>   */
>
>  enum if_state {
> -    OIF_CLAIMED,       /* Newly claimed interface. */
> -    OIF_INSTALL_FLOWS, /* Already claimed interface for which flows are
still
> -                        * being installed.
> +    OIF_CLAIMED,       /* Newly claimed interface. pb->chassis not yet
updated.
> +                        */
> +    OIF_INSTALL_FLOWS, /* Claimed interface with pb->chassis successfully
> +                        * updated in SB and for which flows are still
being
> +                        * installed.
>                          */
>      OIF_MARK_UP,       /* Interface with flows successfully installed in
OVS
>                          * but not yet marked "up" in the binding module
(in
> @@ -78,6 +81,63 @@ static const char *if_state_names[] = {
>      [OIF_INSTALLED]     = "INSTALLED",
>  };
>
> +/*
> + *       +----------------------+
> + * +---> |                      |
> + * | +-> |         NULL         |
<--------------------------------------+++-+
> + * | |   +----------------------+
     |
> + * | |     ^ release_iface   | claim_iface()
    |
> + * | |     |                 V - sbrec_update_chassis(if sb is rw)
    |
> + * | |   +----------------------+
     |
> + * | |   |                      |
<----------------------------------------+ |
> + * | |   |       CLAIMED        |
<--------------------------------------+ | |
> + * | |   +----------------------+
 | | |
> + * | |                 |  V  ^
| | |
> + * | |                 |  |  | handle_claims()
| | |
> + * | |                 |  |  | - sbrec_update_chassis(if sb is rw)
| | |
> + * | |                 |  +--+
| | |
> + * | |                 |
| | |
> + * | |                 | mgr_update(when sb is rw i.e. pb->chassis)
 | | |
> + * | |                 |            has been updated
| | |
> + * | | release_iface   | - request seqno
| | |
> + * | |                 |
| | |
> + * | |                 V
| | |
> + * | |   +----------------------+
 | | |
> + * | +-- |                      |  mgr_run(seqno not rcvd)
| | |
> + * |     |    INSTALL_FLOWS     |   - set port down in sb
 | | |
> + * |     |                      |   - remove ovn-installed from ovsdb
 | | |
> + * |     |                      |  mgr_update()
 | | |
> + * |     +----------------------+   - sbrec_update_chassis if needed
| | |
> + * |                    |
 | | |
> + * |                    |  mgr_run(seqno rcvd)
| | |
> + * |                    |  - set port up in sb
| | |
> + * | release_iface      |  - set ovn-installed in ovs
 | | |
> + * |                    V
 | | |
> + * |   +----------------------+
 | | |
> + * |   |                      |  mgr_run()
| | |
> + * +-- |       MARK_UP        |  - set port up in sb
| | |
> + *     |                      |  - set ovn-installed in ovs
 | | |
> + *     |                      |  mgr_update()
 | | |
> + *     +----------------------+  - sbrec_update_chassis if needed
 | | |
> + *              |
 | | |
> + *              | mgr_update(rcvd port up / ovn_installed & chassis set)
| | |
> + *              V
 | | |
> + *     +----------------------+
 | | |
> + *     |      INSTALLED       | ------------> claim_iface
---------------+ | |
> + *     +----------------------+
   | |
> + *              |
   | |
> + *              | release_iface
   | |
> + *              V
   | |
> + *     +----------------------+
   | |
> + *     |                      | ------------> claim_iface
-----------------+ |
> + *     |      MARK_DOWN       | ------> mgr_update(rcvd port down)
----------+
> + *     |                      | mgr_run()
> + *     |                      | - set port down in sb
> + *     |                      | mgr_update()
> + *     +----------------------+ - sbrec_update_chassis(NULL)
> + */
> +
> +
>  struct ovs_iface {
>      char *id;               /* Extracted from OVS external_ids.iface_id.
*/
>      enum if_state state;    /* State of the interface in the state
machine. */
> @@ -158,14 +218,22 @@ if_status_mgr_destroy(struct if_status_mgr *mgr)
>  }
>
>  void
> -if_status_mgr_claim_iface(struct if_status_mgr *mgr, const char
*iface_id)
> +if_status_mgr_claim_iface(struct if_status_mgr *mgr,
> +                          const struct sbrec_port_binding *pb,
> +                          const struct sbrec_chassis *chassis_rec,
> +                          bool sb_readonly)
>  {
> +    const char *iface_id = pb->logical_port;
>      struct ovs_iface *iface = shash_find_data(&mgr->ifaces, iface_id);
>
>      if (!iface) {
>          iface = ovs_iface_create(mgr, iface_id, OIF_CLAIMED);
>      }
>
> +    if (!sb_readonly) {
> +        set_pb_chassis_in_sbrec(pb, chassis_rec, true);
> +    }
> +
>      switch (iface->state) {
>      case OIF_CLAIMED:
>      case OIF_INSTALL_FLOWS:
> @@ -182,6 +250,12 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr,
const char *iface_id)
>      }
>  }
>
> +bool
> +if_status_mgr_iface_is_present(struct if_status_mgr *mgr, const char
*iface_id)
> +{
> +    return !!shash_find_data(&mgr->ifaces, iface_id);
> +}
> +
>  void
>  if_status_mgr_release_iface(struct if_status_mgr *mgr, const char
*iface_id)
>  {
> @@ -246,9 +320,36 @@ if_status_mgr_delete_iface(struct if_status_mgr
*mgr, const char *iface_id)
>      }
>  }
>
> +bool
> +if_status_handle_claims(struct if_status_mgr *mgr,
> +                        struct local_binding_data *binding_data,
> +                        const struct sbrec_chassis *chassis_rec,
> +                        struct hmap *tracked_datapath,
> +                        bool sb_readonly)
> +{
> +    if (!binding_data || sb_readonly) {
> +        return false;
> +    }
> +
> +    struct shash *bindings = &binding_data->bindings;
> +    struct hmapx_node *node;
> +
> +    bool rc = false;
> +    HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_CLAIMED]) {
> +        struct ovs_iface *iface = node->data;
> +        VLOG_INFO("if_status_handle_claims for %s", iface->id);
> +        local_binding_set_pb(bindings, iface->id, chassis_rec,
> +                             tracked_datapath, true);
> +        rc = true;
> +    }
> +    return rc;
> +}
> +
>  void
>  if_status_mgr_update(struct if_status_mgr *mgr,
> -                     struct local_binding_data *binding_data)
> +                     struct local_binding_data *binding_data,
> +                     const struct sbrec_chassis *chassis_rec,
> +                     bool sb_readonly)
>  {
>      if (!binding_data) {
>          return;
> @@ -257,13 +358,27 @@ if_status_mgr_update(struct if_status_mgr *mgr,
>      struct shash *bindings = &binding_data->bindings;
>      struct hmapx_node *node;
>
> +    /* Interfaces in OIF_MARK_UP/INSTALL_FLOWS state have already set
their
> +     * pb->chassis. However, the update might still be in fly
(confirmation
> +     * not received yet) or pb->chassis was overwitten by another
chassis.
> +     */
> +
>      /* Move all interfaces that have been confirmed "up" by the binding
module,
>       * from OIF_MARK_UP to OIF_INSTALLED.
>       */
>      HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_MARK_UP]) {
>          struct ovs_iface *iface = node->data;
>
> -        if (local_binding_is_up(bindings, iface->id)) {
> +        if (!local_bindings_pb_chassis_is_set(bindings, iface->id,
> +            chassis_rec)) {
> +            if (!sb_readonly) {
> +                local_binding_set_pb(bindings, iface->id, chassis_rec,
> +                                     NULL, true);
> +            } else {
> +                continue;
> +            }
> +        }
> +        if (local_binding_is_up(bindings, iface->id, chassis_rec)) {
>              ovs_iface_set_state(mgr, iface, OIF_INSTALLED);
>          }
>      }
> @@ -274,26 +389,56 @@ if_status_mgr_update(struct if_status_mgr *mgr,
>      HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_MARK_DOWN]) {
>          struct ovs_iface *iface = node->data;
>
> -        if (local_binding_is_down(bindings, iface->id)) {
> +        if (!sb_readonly) {
> +            local_binding_set_pb(bindings, iface->id, chassis_rec,
> +                                 NULL, false);
> +        }
> +        if (local_binding_is_down(bindings, iface->id, chassis_rec)) {
>              ovs_iface_destroy(mgr, iface);
>          }
>      }
>
> -    /* Register for a notification about flows being installed in OVS
for all
> -     * newly claimed interfaces.
> -     *
> -     * Move them from OIF_CLAIMED to OIF_INSTALL_FLOWS.
> +    /* Update pb->chassis in case it's not set (previous update still in
fly
> +     * or pb->chassis was overwitten by another chassis.
>       */
> -    bool new_ifaces = false;
> -    HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_CLAIMED]) {
> -        struct ovs_iface *iface = node->data;
> +    if (!sb_readonly) {
> +        HMAPX_FOR_EACH_SAFE (node,
&mgr->ifaces_per_state[OIF_INSTALL_FLOWS]) {
> +            struct ovs_iface *iface = node->data;
> +
> +            if (!local_bindings_pb_chassis_is_set(bindings, iface->id,
> +                chassis_rec)) {
> +                local_binding_set_pb(bindings, iface->id, chassis_rec,
> +                                     NULL, true);
> +            }
> +        }
> +    }
>
> -        ovs_iface_set_state(mgr, iface, OIF_INSTALL_FLOWS);
> -        iface->install_seqno = mgr->iface_seqno + 1;
> -        new_ifaces = true;
> +    /* Move newly claimed interfaces from OIF_CLAIMED to
OIF_INSTALL_FLOWS.
> +     */
> +    bool new_ifaces = false;
> +    if (!sb_readonly) {
> +        HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_CLAIMED]) {
> +            struct ovs_iface *iface = node->data;
> +            /* No need to to update pb->chassis as already done
> +             * in if_status_handle_claims or if_status_mgr_claim_iface
> +             */
> +            ovs_iface_set_state(mgr, iface, OIF_INSTALL_FLOWS);
> +            iface->install_seqno = mgr->iface_seqno + 1;
> +            new_ifaces = true;
> +        }
> +    } else {
> +        HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_CLAIMED]) {
> +            struct ovs_iface *iface = node->data;
> +            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
1);
> +            VLOG_INFO_RL(&rl,
> +                         "Not updating pb chassis for %s now as "
> +                         "sb is readonly", iface->id);
> +        }
>      }
>
> -    /* Request a seqno update when the flows for new interfaces have been
> +    /* Register for a notification about flows being installed in OVS
for all
> +     * newly claimed interfaces for which pb->chassis has been updated.
> +     * Request a seqno update when the flows for new interfaces have been
>       * installed in OVS.
>       */
>      if (new_ifaces) {
> @@ -403,7 +548,7 @@ if_status_mgr_update_bindings(struct if_status_mgr
*mgr,
>      struct hmapx_node *node;
>
>      /* Notify the binding module to set "down" all bindings that are
still
> -     * in the process of being installed in OVS, i.e., are not yet
instsalled.
> +     * in the process of being installed in OVS, i.e., are not yet
installed.
>       */
>      HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_INSTALL_FLOWS]) {
>          struct ovs_iface *iface = node->data;
> diff --git a/controller/if-status.h b/controller/if-status.h
> index bb8a3950d..5bd187a25 100644
> --- a/controller/if-status.h
> +++ b/controller/if-status.h
> @@ -26,16 +26,27 @@ struct simap;
>  struct if_status_mgr *if_status_mgr_create(void);
>  void if_status_mgr_clear(struct if_status_mgr *);
>  void if_status_mgr_destroy(struct if_status_mgr *);
> -
> -void if_status_mgr_claim_iface(struct if_status_mgr *, const char
*iface_id);
> +void if_status_mgr_claim_iface(struct if_status_mgr *,
> +                               const struct sbrec_port_binding *pb,
> +                               const struct sbrec_chassis *chassis_rec,
> +                               bool sb_readonly);
>  void if_status_mgr_release_iface(struct if_status_mgr *, const char
*iface_id);
>  void if_status_mgr_delete_iface(struct if_status_mgr *, const char
*iface_id);
>
> -void if_status_mgr_update(struct if_status_mgr *, struct
local_binding_data *);
> +void if_status_mgr_update(struct if_status_mgr *, struct
local_binding_data *,
> +                          const struct sbrec_chassis *chassis,
> +                          bool sb_readonly);
>  void if_status_mgr_run(struct if_status_mgr *mgr, struct
local_binding_data *,
>                         const struct sbrec_chassis *,
>                         bool sb_readonly, bool ovs_readonly);
>  void if_status_mgr_get_memory_usage(struct if_status_mgr *mgr,
>                                      struct simap *usage);
> +bool if_status_mgr_iface_is_present(struct if_status_mgr *mgr,
> +                                    const char *iface_id);
> +bool if_status_handle_claims(struct if_status_mgr *mgr,
> +                             struct local_binding_data *binding_data,
> +                             const struct sbrec_chassis *chassis_rec,
> +                             struct hmap *tracked_datapath,
> +                             bool sb_readonly);
>
>  # endif /* controller/if-status.h */
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index c97744d57..b2b2a4434 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1509,6 +1509,73 @@ en_runtime_data_run(struct engine_node *node, void
*data)
>      engine_set_node_state(node, EN_UPDATED);
>  }
>
> +struct ed_type_sb_ro {
> +    bool sb_readonly;
> +};
> +
> +static void *
> +en_sb_ro_init(struct engine_node *node OVS_UNUSED,
> +              struct engine_arg *arg OVS_UNUSED)
> +{
> +    struct ed_type_sb_ro *data = xzalloc(sizeof *data);
> +    return data;
> +}
> +
> +static void
> +en_sb_ro_run(struct engine_node *node, void *data)
> +{
> +    struct ed_type_sb_ro *sb_ro_data = data;
> +    bool sb_readonly = !engine_get_context()->ovnsb_idl_txn;
> +    if (sb_ro_data->sb_readonly != sb_readonly) {
> +        sb_ro_data->sb_readonly = sb_readonly;
> +        if (!sb_ro_data->sb_readonly) {
> +            engine_set_node_state(node, EN_UPDATED);
> +        }
> +    }
> +}
> +
> +static void
> +en_sb_ro_cleanup(void *data OVS_UNUSED)
> +{
> +}
> +
> +static bool
> +runtime_data_sb_ro_handler(struct engine_node *node, void *data)
> +{
> +    const struct sbrec_chassis *chassis = NULL;
> +
> +    struct ovsrec_open_vswitch_table *ovs_table =
> +        (struct ovsrec_open_vswitch_table *)EN_OVSDB_GET(
> +            engine_get_input("OVS_open_vswitch", node));
> +
> +    const char *chassis_id = get_ovs_chassis_id(ovs_table);
> +
> +    struct ovsdb_idl_index *sbrec_chassis_by_name =
> +        engine_ovsdb_node_get_index(
> +                engine_get_input("SB_chassis", node),
> +                "name");
> +
> +    if (chassis_id) {
> +        chassis = chassis_lookup_by_name(sbrec_chassis_by_name,
chassis_id);
> +    }
> +    if (chassis) {
> +        struct ed_type_runtime_data *rt_data = data;
> +        bool sb_readonly = !engine_get_context()->ovnsb_idl_txn;
> +        struct controller_engine_ctx *ctrl_ctx =
> +            engine_get_context()->client_ctx;
> +
> +        if (if_status_handle_claims(ctrl_ctx->if_mgr,
> +                                    &rt_data->lbinding_data,
> +                                    chassis,
> +                                    &rt_data->tracked_dp_bindings,
> +                                    sb_readonly)) {
> +            engine_set_node_state(node, EN_UPDATED);
> +            rt_data->tracked = true;
> +        }
> +    }
> +    return true;
> +}
> +
>  static bool
>  runtime_data_ovs_interface_shadow_handler(struct engine_node *node, void
*data)
>  {
> @@ -3592,6 +3659,7 @@ main(int argc, char *argv[])
>      stopwatch_create(VIF_PLUG_RUN_STOPWATCH_NAME, SW_MS);
>
>      /* Define inc-proc-engine nodes. */
> +    ENGINE_NODE(sb_ro, "sb_ro");
>      ENGINE_NODE_WITH_CLEAR_TRACK_DATA_IS_VALID(ct_zones, "ct_zones");
>      ENGINE_NODE_WITH_CLEAR_TRACK_DATA(ovs_interface_shadow,
>                                        "ovs_interface_shadow");
> @@ -3729,6 +3797,7 @@ main(int argc, char *argv[])
>      engine_add_input(&en_ovs_interface_shadow, &en_ovs_interface,
>                       ovs_interface_shadow_ovs_interface_handler);
>
> +    engine_add_input(&en_runtime_data, &en_sb_ro,
runtime_data_sb_ro_handler);
>      engine_add_input(&en_runtime_data, &en_ofctrl_is_connected, NULL);
>
>      engine_add_input(&en_runtime_data, &en_ovs_open_vswitch, NULL);
> @@ -4166,7 +4235,8 @@ main(int argc, char *argv[])
>                          runtime_data ? &runtime_data->lbinding_data :
NULL;
>                      stopwatch_start(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME,
>                                      time_msec());
> -                    if_status_mgr_update(if_mgr, binding_data);
> +                    if_status_mgr_update(if_mgr, binding_data, chassis,
> +                                         !ovnsb_idl_txn);
>                      stopwatch_stop(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME,
>                                     time_msec());
>
> diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
> index 2ba930a85..a3c7f8125 100644
> --- a/tests/ovn-macros.at
> +++ b/tests/ovn-macros.at
> @@ -814,3 +814,14 @@ m4_define([OVN_FOR_EACH_NORTHD],
>       [m4_foreach([NORTHD_USE_PARALLELIZATION], [yes, no],
>         [m4_foreach([OVN_MONITOR_ALL], [yes, no], [$1
>  ])])])])
> +
> +# OVN_NBCTL(NBCTL_COMMAND) adds NBCTL_COMMAND to list of commands to be
run by RUN_OVN_NBCTL().
> +m4_define([OVN_NBCTL], [
> +    command="${command} -- $1"
> +])
> +
> +# RUN_OVN_NBCTL() executes list of commands built by the OVN_NBCTL()
macro.
> +m4_define([RUN_OVN_NBCTL], [
> +    check ovn-nbctl ${command}
> +    unset command
> +])
> diff --git a/tests/ovn.at b/tests/ovn.at
> index bbad0f194..d0ea87952 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -130,6 +130,8 @@ m4_define([OVN_WAIT_PATCH_PORT_FLOWS],
>  m4_define([OVN_WAIT_REMOTE_OUTPUT_FLOWS],
>    [ovn_wait_remote_output_flows "$1" "$2" "__file__:__line__"])
>
> +m4_define([OVN_WAIT_REMOTE_INPUT_FLOWS],
> +  [ovn_wait_remote_input_flows "$1" "$2" "__file__:__line__"])
>
>  AT_BANNER([OVN components])
>
> @@ -13936,6 +13938,11 @@ wait_column "$hv1_uuid" Port_Binding
requested_chassis logical_port=lsp0
>  wait_column "$hv2_uuid" Port_Binding additional_chassis logical_port=lsp0
>  wait_column "$hv2_uuid" Port_Binding requested_additional_chassis
logical_port=lsp0
>
> +# Check ovn-installed updated for main chassis
> +wait_for_ports_up
> +OVS_WAIT_UNTIL([test `as hv1 ovs-vsctl get Interface lsp0
external_ids:ovn-installed` = '"true"'])
> +OVS_WAIT_UNTIL([test x`as hv2 ovs-vsctl get Interface lsp0
external_ids:ovn-installed` = x])
> +
>  # Check that setting iface:encap-ip populates
Port_Binding:additional_encap
>  wait_row_count Encap 2 chassis_name=hv1
>  wait_row_count Encap 2 chassis_name=hv2
> @@ -13961,6 +13968,11 @@ wait_column "$hv2_uuid" Port_Binding
requested_chassis logical_port=lsp0
>  wait_column "" Port_Binding additional_chassis logical_port=lsp0
>  wait_column "" Port_Binding requested_additional_chassis
logical_port=lsp0
>
> +# Check ovn-installed updated for main chassis and not for other chassis
> +wait_for_ports_up
> +OVS_WAIT_UNTIL([test `as hv2 ovs-vsctl get Interface lsp0
external_ids:ovn-installed` = '"true"'])
> +OVS_WAIT_UNTIL([test x`as hv1 ovs-vsctl get Interface lsp0
external_ids:ovn-installed` = x])
> +
>  # Check that additional_encap is cleared
>  wait_column "" Port_Binding additional_encap logical_port=lsp0
>
> @@ -15259,7 +15271,9 @@ AT_CHECK([as hv1 ovs-ofctl dump-flows br-int
table=65 | grep actions=output:1],
>  echo "verifying that lsp0 binding moves when requested-chassis is
changed"
>
>  ovn-nbctl lsp-set-options lsp0 requested-chassis=hv2
> -OVS_WAIT_UNTIL([test 1 = $(grep -c "Releasing lport lsp0 from this
chassis" hv1/ovn-controller.log)])
> +# We might see multiple "Releasing lport ...", when sb is read only
> +OVS_WAIT_UNTIL([test 1 -le $(grep -c "Releasing lport lsp0 from this
chassis" hv1/ovn-controller.log)])
> +
>  wait_column "$hv2_uuid" Port_Binding chassis logical_port=lsp0
>
>  # (6) Chassis hv2 should add flows and hv1 should not.
> @@ -15346,7 +15360,7 @@ AT_CHECK([as hv1 ovs-ofctl dump-flows br-int
table=0 | grep in_port=1], [0], [ig
>  AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=65 | grep
actions=output:1], [0], [ignore])
>
>  check ovn-nbctl --wait=hv lsp-set-options lsp0
requested-chassis=non-existant-chassis
> -OVS_WAIT_UNTIL([test 1 = $(grep -c "Releasing lport lsp0 from this
chassis" hv1/ovn-controller.log)])
> +OVS_WAIT_UNTIL([test 1 -le $(grep -c "Releasing lport lsp0 from this
chassis" hv1/ovn-controller.log)])
>  check ovn-nbctl --wait=hv sync
>  wait_column '' Port_Binding chasssi logical_port=lsp0
>  AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=0 | grep in_port=1],
[1], [])
> @@ -32517,3 +32531,119 @@ OVS_WAIT_UNTIL([
>  OVN_CLEANUP([hv1], [hv2])
>  AT_CLEANUP
>  ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([recomputes])
> +ovn_start
> +
> +n_hv=4
> +
> +# Add chassis
> +net_add n1
> +for i in $(seq 1 $n_hv); do
> +    sim_add hv$i
> +    as hv$i
> +    check ovs-vsctl add-br br-phys
> +    ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
> +    ovn_attach n1 br-phys 192.168.0.$i 24 geneve
> +done
> +
> +add_switch_ports() {
> +    start_port=$1
> +    end_port=$2
> +    nb_hv=$3
> +    bulk_size=$4
> +    for ((i=start_port; i<end_port; )) do
> +        start_bulk=$i
> +        for hv in $(seq 1 $nb_hv); do
> +            end_bulk=$((start_bulk+bulk_size-1))
> +            for port in $(seq $start_bulk $end_bulk); do
> +                logical_switch_port=lsp${port}
> +                OVN_NBCTL(lsp-add ls1 $logical_switch_port)
> +                OVN_NBCTL(lsp-set-addresses $logical_switch_port dynamic)
> +            done
> +            start_bulk=$((end_bulk+1))
> +        done
> +        RUN_OVN_NBCTL()
> +
> +        start_bulk=$i
> +        for hv in $(seq 1 $nb_hv); do
> +            end_bulk=$((start_bulk+bulk_size-1))
> +            for port in $(seq $start_bulk $end_bulk); do
> +                logical_switch_port=lsp${port}
> +                as hv$hv ovs-vsctl \
> +                    --no-wait -- add-port br-int vif${port} \
> +                    -- set Interface vif${port}
external_ids:iface-id=$logical_switch_port
> +            done
> +            start_bulk=$((end_bulk+1))
> +        done
> +        i=$((end_bulk+1))
> +    done
> +}
> +check ovn-nbctl ls-add ls1
> +check ovn-nbctl set Logical_Switch ls1 other_config:subnet=10.1.0.0/16
> +check ovn-nbctl set Logical_Switch ls1
other_config:exclude_ips=10.1.255.254
> +
> +check ovn-nbctl lr-add lr1
> +check ovn-nbctl lsp-add ls1 lsp0 -- set Logical_Switch_Port lsp0
type=router options:router-port=lrp0 addresses=dynamic
> +check ovn-nbctl lrp-add lr1 lrp0 "f0:00:00:01:00:01" 10.1.255.254/16
> +check ovn-nbctl lr-nat-add lr1 snat 10.2.0.1 10.1.0.0/16
> +
> +lflow_run=0
> +check ovn-nbctl --wait=hv sync
> +
> +# Tunnel ports might not be added (yet) at this point on slow system.
> +# Wait for flows related to such ports to ensure those ports have been
added
> +# before we measure recomputes. Otherwise, ovs_interface handler might
be run
> +# afterwards for tunnel ports, causing recomputes.
> +for i in $(seq 1 $n_hv); do
> +    for j in $(seq 1 $n_hv); do
> +        if test $i != $j; then
> +            OVN_WAIT_REMOTE_INPUT_FLOWS(["hv$i"],["hv$j"])
> +        fi
> +    done
> +done
> +
> +for i in $(seq 1 $n_hv); do
> +    as hv$i
> +    lflow_run1=$(ovn-appctl -t ovn-controller coverage/read-counter
lflow_run)
> +    lflow_run=`expr $lflow_run1 + $lflow_run`
> +done
> +
> +add_switch_ports 1 1000 $n_hv 5
> +
> +wait_for_ports_up
> +check ovn-nbctl --wait=hv sync
> +
> +for i in $(seq 1 $n_hv); do
> +    pid=$(cat hv${i}/ovn-controller.pid)
> +    u=$((u+$(cat "/proc/$pid/stat" | awk '{print $14}')))
> +    s=$((s+$(cat "/proc/$pid/stat" | awk '{print $15}')))
> +done
> +
> +n_pid=$(cat northd/ovn-northd.pid)
> +n_u=$(cat "/proc/$pid/stat" | awk '{print $14}')
> +n_s=$(cat "/proc/$pid/stat" | awk '{print $15}')
> +
> +echo "Total Northd User Time: $n_u"
> +echo "Total Northd System Time: $n_s"
> +echo "Total Controller User Time: $u"
> +echo "Total Controller System Time: $s"
> +
> +lflow_run_end=0
> +for i in $(seq 1 $n_hv); do
> +    as hv$i
> +    lflow_run1=$(ovn-appctl -t ovn-controller coverage/read-counter
lflow_run)
> +    lflow_run_end=`expr $lflow_run1 + $lflow_run_end`
> +done
> +n_recomputes=`expr $lflow_run_end - $lflow_run`
> +echo "$n_recomputes recomputes"
> +
> +AT_CHECK([test $lflow_run_end == $lflow_run])
> +
> +for i in $(seq 2 $n_hv); do
> +    OVN_CLEANUP_SBOX([hv$i])
> +done
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
> diff --git a/tests/perf-northd.at b/tests/perf-northd.at
> index 74b69e9d4..ca115dadc 100644
> --- a/tests/perf-northd.at
> +++ b/tests/perf-northd.at
> @@ -76,23 +76,6 @@ m4_define([PERF_RECORD_STOP], [
>      PERF_RECORD_STOPWATCH(ovn-northd-loop, ["Short term average"],
[Average (northd-loop in msec)])
>  ])
>
> -# OVN_NBCTL([NBCTL_COMMAND])
> -#
> -# Add NBCTL_COMMAND to list of commands to be run by RUN_OVN_NBCTL().
> -#
> -m4_define([OVN_NBCTL], [
> -    command="${command} -- $1"
> -])
> -
> -# RUN_OVN_NBCTL()
> -#
> -# Execute list of commands built by the OVN_NBCTL() macro.
> -#
> -m4_define([RUN_OVN_NBCTL], [
> -    check ovn-nbctl ${command}
> -    unset command
> -])
> -
>  OVS_START_SHELL_HELPERS
>  generate_subnet () {
>      local a=$(printf %d $(expr $1 / 256 + 10))
> @@ -208,4 +191,4 @@ BUILD_NBDB(OVN_BASIC_SCALE_CONFIG(500, 50))
>
>  PERF_RECORD_STOP()
>  AT_CLEANUP
> -])
> \ No newline at end of file
> +])
> --
> 2.31.1
>

On Mon, Aug 22, 2022 at 9:41 AM Han Zhou <hzhou@ovn.org> wrote:
>
>
>
> On Fri, Aug 19, 2022 at 8:40 AM Xavier Simonart <xsimonar@redhat.com>
wrote:
> >
> > When VIF ports are claimed on a chassis, SBDB Port_Binding table is
updated.
> > If the SBDB IDL is still is read-only ("in transaction") when such a
update
> > is required, the update is not possible and recompute is triggered
through
> > I+P failure.
> >
> > This situation can happen:
> > - after updating Port_Binding->chassis to SBDB for one port, in a
following
> >   iteration, ovn-controller handles Interface:external_ids:ovn-installed
> >   (for the same port) while SBDB is still read-only.
> > - after updating Port_Binding->chassis to SBDB for one port, in a
following
> >   iteration, ovn-controller updates Port_Binding->chassis for another
port,
> >   while SBDB is still read-only.
> >
> > This patch prevent the recompute, by having the if-status module
> > updating the Port_Binding chassis (if needed) when possible.
> > This does not delay Port_Binding chassis update compared to before this
patch.
> > - With the patch, Port_Binding chassis will be updated as soon as SBDB
is
> > again writable, without recompute.
> > - Without the patch, Port_Binding chassis was updated as soon as SBDB
was
> > again writable, through a recompute.
> >
> > As part of this patch, ovn-installed will not be updated for additional
chassis;
> > it will only be updated when the migration is completed.
> >
> > Note that this patch also fixes an issue where port was not always
properly
> > reported up for virtual ports (causing flaky failures in "virtual ports"
> > test case).
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2059253
> > Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
> >
> > ---
> > v2:  - handled Dumitru's comments.
> >      - handled Han's comments, mainly ensure we moved out of CLAIMED
state
> >        only after updating pb->chassis to guarentee physical flows are
installed
> >        when ovn-installed is updated in OVS.
> >      - slighly reorganize the code to isolate 'notify_up = false' cases
in
> >        claim_port (i.e. ports such as virtual ports), in the idea of
making
> >        future patch preventing recomputes when virtual ports are
claimed.
> >      - updated test case to cause more race conditions.
> >      - rebased on origin/main
> >      - note that "additional chassis" as now supported by
> >        "Support LSP:options:requested-chassis as a list" might still
cause
> >        recomputes.
> >      - fixed missing flows when Port_Binding chassis was updated by
mgr_update
> >        w/o any lflow recalculation.
> > v3:  - handled Dumitru's comments on v2, mainly have runtime_data
handler
> >        handling pb_claims when sb becomes writable (instead of a lflow
handler).
> >      - fixed test as it was not checking recomputes on all hv, as well
as a flaky
> > v4:  - handled Han's comments, mainly update pb->chassis until it's
confirmed.
> >      - updated doc to reflect that pb->chassis update can happen at any
state, and
> >        the CLAIMED state is only for the initial attempt.
>
> Thanks Xavier. All looks good to me except that it seems you updated the
diagram but forgot to update the description, I have the below small
incremental change, and if you are ok, I can merge it without the need for
v6:
>
> diff --git a/controller/if-status.c b/controller/if-status.c
> index 2590ec27f..d1c14ac30 100644
> --- a/controller/if-status.c
> +++ b/controller/if-status.c
> @@ -54,11 +54,12 @@ VLOG_DEFINE_THIS_MODULE(if_status);
>   */
>
>  enum if_state {
> -    OIF_CLAIMED,       /* Newly claimed interface. pb->chassis not yet
updated.
> -                        */
> -    OIF_INSTALL_FLOWS, /* Claimed interface with pb->chassis successfully
> -                        * updated in SB and for which flows are still
being
> -                        * installed.
> +    OIF_CLAIMED,       /* Newly claimed interface. pb->chassis update
not yet
> +                          initiated. */
> +    OIF_INSTALL_FLOWS, /* Claimed interface with pb->chassis update sent
to
> +                        * SB (but update notification not confirmed, so
the
> +                        * update may be resent in any of the following
states)
> +                        * and for which flows are still being installed.
>                          */
>      OIF_MARK_UP,       /* Interface with flows successfully installed in
OVS
>                          * but not yet marked "up" in the binding module
(in
>
> Thanks,
> Han

I heard no objection to the above and this is really minor, so applied it
to main. Thanks Xavier!
(Just realized after pushing, Dumitru had Acked one of the previous
versions but I forgot to add his name. I apologize!)

Han
>
>
> > v5:  - rebased on origin/main.
> >
> > Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
> > ---
> >  controller/binding.c        | 174 +++++++++++++++++++++++++---------
> >  controller/binding.h        |  18 +++-
> >  controller/if-status.c      | 183 ++++++++++++++++++++++++++++++++----
> >  controller/if-status.h      |  17 +++-
> >  controller/ovn-controller.c |  72 +++++++++++++-
> >  tests/ovn-macros.at         |  11 +++
> >  tests/ovn.at                | 134 +++++++++++++++++++++++++-
> >  tests/perf-northd.at        |  19 +---
> >  8 files changed, 540 insertions(+), 88 deletions(-)
> >
> > diff --git a/controller/binding.c b/controller/binding.c
> > index 9f5393a92..74a156525 100644
> > --- a/controller/binding.c
> > +++ b/controller/binding.c
> > @@ -707,11 +707,17 @@ local_binding_get_lport_ofport(const struct shash
*local_bindings,
> >  }
> >
> >  bool
> > -local_binding_is_up(struct shash *local_bindings, const char *pb_name)
> > +local_binding_is_up(struct shash *local_bindings, const char *pb_name,
> > +                    const struct sbrec_chassis *chassis_rec)
> >  {
> >      struct local_binding *lbinding =
> >          local_binding_find(local_bindings, pb_name);
> >      struct binding_lport *b_lport =
local_binding_get_primary_lport(lbinding);
> > +
> > +    if (b_lport && b_lport->pb->chassis != chassis_rec) {
> > +        return false;
> > +    }
> > +
> >      if (lbinding && b_lport && lbinding->iface) {
> >          if (b_lport->pb->n_up && !b_lport->pb->up[0]) {
> >              return false;
> > @@ -723,13 +729,23 @@ local_binding_is_up(struct shash *local_bindings,
const char *pb_name)
> >  }
> >
> >  bool
> > -local_binding_is_down(struct shash *local_bindings, const char
*pb_name)
> > +local_binding_is_down(struct shash *local_bindings, const char
*pb_name,
> > +                      const struct sbrec_chassis *chassis_rec)
> >  {
> >      struct local_binding *lbinding =
> >          local_binding_find(local_bindings, pb_name);
> >
> >      struct binding_lport *b_lport =
local_binding_get_primary_lport(lbinding);
> >
> > +    if (b_lport) {
> > +        if (b_lport->pb->chassis == chassis_rec) {
> > +            return false;
> > +        } else if (b_lport->pb->chassis) {
> > +            VLOG_DBG("lport %s already claimed by other chassis",
> > +                     b_lport->pb->logical_port);
> > +        }
> > +    }
> > +
> >      if (!lbinding) {
> >          return true;
> >      }
> > @@ -947,37 +963,95 @@ get_lport_type_str(enum en_lport_type lport_type)
> >      OVS_NOT_REACHED();
> >  }
> >
> > -/* For newly claimed ports, if 'notify_up' is 'false':
> > +void
> > +set_pb_chassis_in_sbrec(const struct sbrec_port_binding *pb,
> > +                        const struct sbrec_chassis *chassis_rec,
> > +                        bool is_set)
> > +{
> > +    if (pb->chassis != chassis_rec) {
> > +         if (is_set) {
> > +            if (pb->chassis) {
> > +                VLOG_INFO("Changing chassis for lport %s from %s to
%s.",
> > +                          pb->logical_port, pb->chassis->name,
> > +                          chassis_rec->name);
> > +            } else {
> > +                VLOG_INFO("Claiming lport %s for this chassis.",
> > +                          pb->logical_port);
> > +            }
> > +            for (int i = 0; i < pb->n_mac; i++) {
> > +                VLOG_INFO("%s: Claiming %s", pb->logical_port,
pb->mac[i]);
> > +            }
> > +            sbrec_port_binding_set_chassis(pb, chassis_rec);
> > +        }
> > +    } else if (!is_set) {
> > +        sbrec_port_binding_set_chassis(pb, NULL);
> > +    }
> > +}
> > +
> > +bool
> > +local_bindings_pb_chassis_is_set(struct shash *local_bindings,
> > +                                 const char *pb_name,
> > +                                 const struct sbrec_chassis
*chassis_rec)
> > +{
> > +    struct local_binding *lbinding =
> > +        local_binding_find(local_bindings, pb_name);
> > +    struct binding_lport *b_lport =
local_binding_get_primary_lport(lbinding);
> > +
> > +    if (b_lport && b_lport->pb && b_lport->pb->chassis == chassis_rec)
{
> > +        return true;
> > +    }
> > +    return false;
> > +}
> > +
> > +void
> > +local_binding_set_pb(struct shash *local_bindings, const char *pb_name,
> > +                     const struct sbrec_chassis *chassis_rec,
> > +                     struct hmap *tracked_datapaths, bool is_set)
> > +{
> > +    struct local_binding *lbinding =
> > +        local_binding_find(local_bindings, pb_name);
> > +    struct binding_lport *b_lport =
local_binding_get_primary_lport(lbinding);
> > +
> > +    if (b_lport) {
> > +        set_pb_chassis_in_sbrec(b_lport->pb, chassis_rec, is_set);
> > +        if (tracked_datapaths) {
> > +            update_lport_tracking(b_lport->pb, tracked_datapaths,
true);
> > +        }
> > +    }
> > +}
> > +
> > +/* For newly claimed ports:
> >   * - set the 'pb.up' field to true if 'pb' has no 'parent_pb'.
> >   * - set the 'pb.up' field to true if 'parent_pb.up' is 'true' (e.g.,
for
> >   *   container and virtual ports).
> > - * Otherwise request a notification to be sent when the OVS flows
> > - * corresponding to 'pb' have been installed.
> > + *
> > + * Returns false if lport is not claimed due to 'sb_readonly'.
> > + * Returns true otherwise.
> >   *
> >   * Note:
> > - *   Updates (directly or through a notification) the 'pb->up' field
only if
> > - *   it's explicitly set to 'false'.
> > + *   Updates the 'pb->up' field only if it's explicitly set to 'false'.
> >   *   This is to ensure compatibility with older versions of ovn-northd.
> >   */
> > -static void
> > +static bool
> >  claimed_lport_set_up(const struct sbrec_port_binding *pb,
> >                       const struct sbrec_port_binding *parent_pb,
> > -                     const struct sbrec_chassis *chassis_rec,
> > -                     bool notify_up, struct if_status_mgr *if_mgr)
> > +                     bool sb_readonly)
> >  {
> > -    if (!notify_up) {
> > -        bool up = true;
> > -        if (!parent_pb || (parent_pb->n_up && parent_pb->up[0])) {
> > +    /* When notify_up is false in claim_port(), no state is created
> > +     * by if_status_mgr. In such cases, return false (i.e. trigger
recompute)
> > +     * if we can't update sb (because it is readonly).
> > +     */
> > +    bool up = true;
> > +    if (!parent_pb || (parent_pb->n_up && parent_pb->up[0])) {
> > +        if (!sb_readonly) {
> >              if (pb->n_up) {
> >                  sbrec_port_binding_set_up(pb, &up, 1);
> >              }
> > +        } else if (pb->n_up && !pb->up[0]) {
> > +            return false;
> >          }
> > -        return;
> > -    }
> > -
> > -    if (pb->chassis != chassis_rec || (pb->n_up && !pb->up[0])) {
> > -        if_status_mgr_claim_iface(if_mgr, pb->logical_port);
> >      }
> > +    return true;
> >  }
> >
> >  typedef void (*set_func)(const struct sbrec_port_binding *pb,
> > @@ -1085,44 +1159,51 @@ claim_lport(const struct sbrec_port_binding *pb,
> >              struct if_status_mgr *if_mgr,
> >              struct sset *postponed_ports)
> >  {
> > -    if (!sb_readonly) {
> > -        claimed_lport_set_up(pb, parent_pb, chassis_rec, notify_up,
if_mgr);
> > -    }
> > -
> >      enum can_bind can_bind =
lport_can_bind_on_this_chassis(chassis_rec, pb);
> >      bool update_tracked = false;
> >
> >      if (can_bind == CAN_BIND_AS_MAIN) {
> >          if (pb->chassis != chassis_rec) {
> > -            if (sb_readonly) {
> > -                return false;
> > -            }
> > -
> >              long long int now = time_msec();
> >              if (pb->chassis) {
> >                  if (lport_maybe_postpone(pb->logical_port, now,
> >                                           postponed_ports)) {
> >                      return true;
> >                  }
> > -                VLOG_INFO("Changing chassis for lport %s from %s to
%s.",
> > -                        pb->logical_port, pb->chassis->name,
> > -                        chassis_rec->name);
> > -            } else {
> > -                VLOG_INFO("Claiming lport %s for this chassis.",
> > -                          pb->logical_port);
> > -            }
> > -            for (size_t i = 0; i < pb->n_mac; i++) {
> > -                VLOG_INFO("%s: Claiming %s", pb->logical_port,
pb->mac[i]);
> >              }
> > -
> > -            sbrec_port_binding_set_chassis(pb, chassis_rec);
> >              if (is_additional_chassis(pb, chassis_rec)) {
> > +                if (sb_readonly) {
> > +                    return false;
> > +                }
> >                  remove_additional_chassis(pb, chassis_rec);
> >              }
> >              update_tracked = true;
> >
> > +            if (!notify_up) {
> > +                if (!claimed_lport_set_up(pb, parent_pb, sb_readonly))
{
> > +                    return false;
> > +                }
> > +                if (sb_readonly) {
> > +                    return false;
> > +                }
> > +                set_pb_chassis_in_sbrec(pb, chassis_rec, true);
> > +            } else {
> > +                if_status_mgr_claim_iface(if_mgr, pb, chassis_rec,
> > +                                          sb_readonly);
> > +            }
> >              register_claim_timestamp(pb->logical_port, now);
> >              sset_find_and_delete(postponed_ports, pb->logical_port);
> > +        } else {
> > +            if (!notify_up) {
> > +                if (!claimed_lport_set_up(pb, parent_pb, sb_readonly))
{
> > +                    return false;
> > +                }
> > +            } else {
> > +                if (pb->n_up && !pb->up[0]) {
> > +                    if_status_mgr_claim_iface(if_mgr, pb, chassis_rec,
> > +                                              sb_readonly);
> > +                }
> > +            }
> >          }
> >      } else if (can_bind == CAN_BIND_AS_ADDITIONAL) {
> >          if (!is_additional_chassis(pb, chassis_rec)) {
> > @@ -1171,7 +1252,8 @@ claim_lport(const struct sbrec_port_binding *pb,
> >   */
> >  static bool
> >  release_lport_main_chassis(const struct sbrec_port_binding *pb,
> > -                           bool sb_readonly)
> > +                           bool sb_readonly,
> > +                           struct if_status_mgr *if_mgr)
> >  {
> >      if (pb->encap) {
> >          if (sb_readonly) {
> > @@ -1180,11 +1262,14 @@ release_lport_main_chassis(const struct
sbrec_port_binding *pb,
> >          sbrec_port_binding_set_encap(pb, NULL);
> >      }
> >
> > +    /* If sb is readonly, pb->chassis is unset through if-status if
present. */
> > +
> >      if (pb->chassis) {
> > -        if (sb_readonly) {
> > +        if (!sb_readonly) {
> > +            sbrec_port_binding_set_chassis(pb, NULL);
> > +        } else if (!if_status_mgr_iface_is_present(if_mgr,
pb->logical_port)) {
> >              return false;
> >          }
> > -        sbrec_port_binding_set_chassis(pb, NULL);
> >      }
> >
> >      if (pb->virtual_parent) {
> > @@ -1194,7 +1279,9 @@ release_lport_main_chassis(const struct
sbrec_port_binding *pb,
> >          sbrec_port_binding_set_virtual_parent(pb, NULL);
> >      }
> >
> > -    VLOG_INFO("Releasing lport %s from this chassis.",
pb->logical_port);
> > +    VLOG_INFO("Releasing lport %s from this chassis (sb_readonly=%d)",
> > +              pb->logical_port, sb_readonly);
> > +
> >      return true;
> >  }
> >
> > @@ -1228,7 +1315,7 @@ release_lport(const struct sbrec_port_binding *pb,
> >                struct hmap *tracked_datapaths, struct if_status_mgr
*if_mgr)
> >  {
> >      if (pb->chassis == chassis_rec) {
> > -        if (!release_lport_main_chassis(pb, sb_readonly)) {
> > +        if (!release_lport_main_chassis(pb, sb_readonly, if_mgr)) {
> >              return false;
> >          }
> >      } else if (is_additional_chassis(pb, chassis_rec)) {
> > @@ -1567,7 +1654,8 @@ consider_localport(const struct
sbrec_port_binding *pb,
> >      enum can_bind can_bind = lport_can_bind_on_this_chassis(
> >          b_ctx_in->chassis_rec, pb);
> >      if (can_bind == CAN_BIND_AS_MAIN) {
> > -        if (!release_lport_main_chassis(pb, !b_ctx_in->ovnsb_idl_txn))
{
> > +        if (!release_lport_main_chassis(pb, !b_ctx_in->ovnsb_idl_txn,
> > +            b_ctx_out->if_mgr)) {
> >              return false;
> >          }
> >      } else if (can_bind == CAN_BIND_AS_ADDITIONAL) {
> > diff --git a/controller/binding.h b/controller/binding.h
> > index b2360bac2..ad959a9e6 100644
> > --- a/controller/binding.h
> > +++ b/controller/binding.h
> > @@ -153,8 +153,11 @@ const struct sbrec_port_binding
*local_binding_get_primary_pb(
> >  ofp_port_t local_binding_get_lport_ofport(const struct shash
*local_bindings,
> >                                            const char *pb_name);
> >
> > -bool local_binding_is_up(struct shash *local_bindings, const char
*pb_name);
> > -bool local_binding_is_down(struct shash *local_bindings, const char
*pb_name);
> > +bool local_binding_is_up(struct shash *local_bindings, const char
*pb_name,
> > +                         const struct sbrec_chassis *);
> > +bool local_binding_is_down(struct shash *local_bindings, const char
*pb_name,
> > +                           const struct sbrec_chassis *);
> > +
> >  void local_binding_set_up(struct shash *local_bindings, const char
*pb_name,
> >                            const struct sbrec_chassis *chassis_rec,
> >                            const char *ts_now_str, bool sb_readonly,
> > @@ -162,6 +165,13 @@ void local_binding_set_up(struct shash
*local_bindings, const char *pb_name,
> >  void local_binding_set_down(struct shash *local_bindings, const char
*pb_name,
> >                              const struct sbrec_chassis *chassis_rec,
> >                              bool sb_readonly, bool ovs_readonly);
> > +void local_binding_set_pb(struct shash *local_bindings, const char
*pb_name,
> > +                          const struct sbrec_chassis *chassis_rec,
> > +                          struct hmap *tracked_datapaths,
> > +                          bool is_set);
> > +bool local_bindings_pb_chassis_is_set(struct shash *local_bindings,
> > +                                      const char *pb_name,
> > +                                      const struct sbrec_chassis
*chassis_rec);
> >
> >  void binding_register_ovs_idl(struct ovsdb_idl *);
> >  void binding_run(struct binding_ctx_in *, struct binding_ctx_out *);
> > @@ -180,6 +190,10 @@ void binding_dump_local_bindings(struct
local_binding_data *, struct ds *);
> >  bool is_additional_chassis(const struct sbrec_port_binding *pb,
> >                             const struct sbrec_chassis *chassis_rec);
> >
> > +void set_pb_chassis_in_sbrec(const struct sbrec_port_binding *pb,
> > +                             const struct sbrec_chassis *chassis_rec,
> > +                             bool is_set);
> > +
> >  /* Corresponds to each Port_Binding.type. */
> >  enum en_lport_type {
> >      LP_UNKNOWN,
> > diff --git a/controller/if-status.c b/controller/if-status.c
> > index ad61844d8..2590ec27f 100644
> > --- a/controller/if-status.c
> > +++ b/controller/if-status.c
> > @@ -24,6 +24,7 @@
> >  #include "lib/util.h"
> >  #include "timeval.h"
> >  #include "openvswitch/vlog.h"
> > +#include "lib/ovn-sb-idl.h"
> >
> >  VLOG_DEFINE_THIS_MODULE(if_status);
> >
> > @@ -53,9 +54,11 @@ VLOG_DEFINE_THIS_MODULE(if_status);
> >   */
> >
> >  enum if_state {
> > -    OIF_CLAIMED,       /* Newly claimed interface. */
> > -    OIF_INSTALL_FLOWS, /* Already claimed interface for which flows
are still
> > -                        * being installed.
> > +    OIF_CLAIMED,       /* Newly claimed interface. pb->chassis not yet
updated.
> > +                        */
> > +    OIF_INSTALL_FLOWS, /* Claimed interface with pb->chassis
successfully
> > +                        * updated in SB and for which flows are still
being
> > +                        * installed.
> >                          */
> >      OIF_MARK_UP,       /* Interface with flows successfully installed
in OVS
> >                          * but not yet marked "up" in the binding
module (in
> > @@ -78,6 +81,63 @@ static const char *if_state_names[] = {
> >      [OIF_INSTALLED]     = "INSTALLED",
> >  };
> >
> > +/*
> > + *       +----------------------+
> > + * +---> |                      |
> > + * | +-> |         NULL         |
<--------------------------------------+++-+
> > + * | |   +----------------------+
       |
> > + * | |     ^ release_iface   | claim_iface()
      |
> > + * | |     |                 V - sbrec_update_chassis(if sb is rw)
      |
> > + * | |   +----------------------+
       |
> > + * | |   |                      |
<----------------------------------------+ |
> > + * | |   |       CLAIMED        |
<--------------------------------------+ | |
> > + * | |   +----------------------+
   | | |
> > + * | |                 |  V  ^
  | | |
> > + * | |                 |  |  | handle_claims()
  | | |
> > + * | |                 |  |  | - sbrec_update_chassis(if sb is rw)
  | | |
> > + * | |                 |  +--+
  | | |
> > + * | |                 |
  | | |
> > + * | |                 | mgr_update(when sb is rw i.e. pb->chassis)
   | | |
> > + * | |                 |            has been updated
  | | |
> > + * | | release_iface   | - request seqno
  | | |
> > + * | |                 |
  | | |
> > + * | |                 V
  | | |
> > + * | |   +----------------------+
   | | |
> > + * | +-- |                      |  mgr_run(seqno not rcvd)
  | | |
> > + * |     |    INSTALL_FLOWS     |   - set port down in sb
   | | |
> > + * |     |                      |   - remove ovn-installed from ovsdb
   | | |
> > + * |     |                      |  mgr_update()
   | | |
> > + * |     +----------------------+   - sbrec_update_chassis if needed
  | | |
> > + * |                    |
   | | |
> > + * |                    |  mgr_run(seqno rcvd)
  | | |
> > + * |                    |  - set port up in sb
  | | |
> > + * | release_iface      |  - set ovn-installed in ovs
   | | |
> > + * |                    V
   | | |
> > + * |   +----------------------+
   | | |
> > + * |   |                      |  mgr_run()
  | | |
> > + * +-- |       MARK_UP        |  - set port up in sb
  | | |
> > + *     |                      |  - set ovn-installed in ovs
   | | |
> > + *     |                      |  mgr_update()
   | | |
> > + *     +----------------------+  - sbrec_update_chassis if needed
   | | |
> > + *              |
   | | |
> > + *              | mgr_update(rcvd port up / ovn_installed & chassis
set) | | |
> > + *              V
   | | |
> > + *     +----------------------+
   | | |
> > + *     |      INSTALLED       | ------------> claim_iface
---------------+ | |
> > + *     +----------------------+
     | |
> > + *              |
     | |
> > + *              | release_iface
     | |
> > + *              V
     | |
> > + *     +----------------------+
     | |
> > + *     |                      | ------------> claim_iface
-----------------+ |
> > + *     |      MARK_DOWN       | ------> mgr_update(rcvd port down)
----------+
> > + *     |                      | mgr_run()
> > + *     |                      | - set port down in sb
> > + *     |                      | mgr_update()
> > + *     +----------------------+ - sbrec_update_chassis(NULL)
> > + */
> > +
> > +
> >  struct ovs_iface {
> >      char *id;               /* Extracted from OVS
external_ids.iface_id. */
> >      enum if_state state;    /* State of the interface in the state
machine. */
> > @@ -158,14 +218,22 @@ if_status_mgr_destroy(struct if_status_mgr *mgr)
> >  }
> >
> >  void
> > -if_status_mgr_claim_iface(struct if_status_mgr *mgr, const char
*iface_id)
> > +if_status_mgr_claim_iface(struct if_status_mgr *mgr,
> > +                          const struct sbrec_port_binding *pb,
> > +                          const struct sbrec_chassis *chassis_rec,
> > +                          bool sb_readonly)
> >  {
> > +    const char *iface_id = pb->logical_port;
> >      struct ovs_iface *iface = shash_find_data(&mgr->ifaces, iface_id);
> >
> >      if (!iface) {
> >          iface = ovs_iface_create(mgr, iface_id, OIF_CLAIMED);
> >      }
> >
> > +    if (!sb_readonly) {
> > +        set_pb_chassis_in_sbrec(pb, chassis_rec, true);
> > +    }
> > +
> >      switch (iface->state) {
> >      case OIF_CLAIMED:
> >      case OIF_INSTALL_FLOWS:
> > @@ -182,6 +250,12 @@ if_status_mgr_claim_iface(struct if_status_mgr
*mgr, const char *iface_id)
> >      }
> >  }
> >
> > +bool
> > +if_status_mgr_iface_is_present(struct if_status_mgr *mgr, const char
*iface_id)
> > +{
> > +    return !!shash_find_data(&mgr->ifaces, iface_id);
> > +}
> > +
> >  void
> >  if_status_mgr_release_iface(struct if_status_mgr *mgr, const char
*iface_id)
> >  {
> > @@ -246,9 +320,36 @@ if_status_mgr_delete_iface(struct if_status_mgr
*mgr, const char *iface_id)
> >      }
> >  }
> >
> > +bool
> > +if_status_handle_claims(struct if_status_mgr *mgr,
> > +                        struct local_binding_data *binding_data,
> > +                        const struct sbrec_chassis *chassis_rec,
> > +                        struct hmap *tracked_datapath,
> > +                        bool sb_readonly)
> > +{
> > +    if (!binding_data || sb_readonly) {
> > +        return false;
> > +    }
> > +
> > +    struct shash *bindings = &binding_data->bindings;
> > +    struct hmapx_node *node;
> > +
> > +    bool rc = false;
> > +    HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_CLAIMED]) {
> > +        struct ovs_iface *iface = node->data;
> > +        VLOG_INFO("if_status_handle_claims for %s", iface->id);
> > +        local_binding_set_pb(bindings, iface->id, chassis_rec,
> > +                             tracked_datapath, true);
> > +        rc = true;
> > +    }
> > +    return rc;
> > +}
> > +
> >  void
> >  if_status_mgr_update(struct if_status_mgr *mgr,
> > -                     struct local_binding_data *binding_data)
> > +                     struct local_binding_data *binding_data,
> > +                     const struct sbrec_chassis *chassis_rec,
> > +                     bool sb_readonly)
> >  {
> >      if (!binding_data) {
> >          return;
> > @@ -257,13 +358,27 @@ if_status_mgr_update(struct if_status_mgr *mgr,
> >      struct shash *bindings = &binding_data->bindings;
> >      struct hmapx_node *node;
> >
> > +    /* Interfaces in OIF_MARK_UP/INSTALL_FLOWS state have already set
their
> > +     * pb->chassis. However, the update might still be in fly
(confirmation
> > +     * not received yet) or pb->chassis was overwitten by another
chassis.
> > +     */
> > +
> >      /* Move all interfaces that have been confirmed "up" by the
binding module,
> >       * from OIF_MARK_UP to OIF_INSTALLED.
> >       */
> >      HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_MARK_UP]) {
> >          struct ovs_iface *iface = node->data;
> >
> > -        if (local_binding_is_up(bindings, iface->id)) {
> > +        if (!local_bindings_pb_chassis_is_set(bindings, iface->id,
> > +            chassis_rec)) {
> > +            if (!sb_readonly) {
> > +                local_binding_set_pb(bindings, iface->id, chassis_rec,
> > +                                     NULL, true);
> > +            } else {
> > +                continue;
> > +            }
> > +        }
> > +        if (local_binding_is_up(bindings, iface->id, chassis_rec)) {
> >              ovs_iface_set_state(mgr, iface, OIF_INSTALLED);
> >          }
> >      }
> > @@ -274,26 +389,56 @@ if_status_mgr_update(struct if_status_mgr *mgr,
> >      HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_MARK_DOWN]) {
> >          struct ovs_iface *iface = node->data;
> >
> > -        if (local_binding_is_down(bindings, iface->id)) {
> > +        if (!sb_readonly) {
> > +            local_binding_set_pb(bindings, iface->id, chassis_rec,
> > +                                 NULL, false);
> > +        }
> > +        if (local_binding_is_down(bindings, iface->id, chassis_rec)) {
> >              ovs_iface_destroy(mgr, iface);
> >          }
> >      }
> >
> > -    /* Register for a notification about flows being installed in OVS
for all
> > -     * newly claimed interfaces.
> > -     *
> > -     * Move them from OIF_CLAIMED to OIF_INSTALL_FLOWS.
> > +    /* Update pb->chassis in case it's not set (previous update still
in fly
> > +     * or pb->chassis was overwitten by another chassis.
> >       */
> > -    bool new_ifaces = false;
> > -    HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_CLAIMED]) {
> > -        struct ovs_iface *iface = node->data;
> > +    if (!sb_readonly) {
> > +        HMAPX_FOR_EACH_SAFE (node,
&mgr->ifaces_per_state[OIF_INSTALL_FLOWS]) {
> > +            struct ovs_iface *iface = node->data;
> > +
> > +            if (!local_bindings_pb_chassis_is_set(bindings, iface->id,
> > +                chassis_rec)) {
> > +                local_binding_set_pb(bindings, iface->id, chassis_rec,
> > +                                     NULL, true);
> > +            }
> > +        }
> > +    }
> >
> > -        ovs_iface_set_state(mgr, iface, OIF_INSTALL_FLOWS);
> > -        iface->install_seqno = mgr->iface_seqno + 1;
> > -        new_ifaces = true;
> > +    /* Move newly claimed interfaces from OIF_CLAIMED to
OIF_INSTALL_FLOWS.
> > +     */
> > +    bool new_ifaces = false;
> > +    if (!sb_readonly) {
> > +        HMAPX_FOR_EACH_SAFE (node,
&mgr->ifaces_per_state[OIF_CLAIMED]) {
> > +            struct ovs_iface *iface = node->data;
> > +            /* No need to to update pb->chassis as already done
> > +             * in if_status_handle_claims or if_status_mgr_claim_iface
> > +             */
> > +            ovs_iface_set_state(mgr, iface, OIF_INSTALL_FLOWS);
> > +            iface->install_seqno = mgr->iface_seqno + 1;
> > +            new_ifaces = true;
> > +        }
> > +    } else {
> > +        HMAPX_FOR_EACH_SAFE (node,
&mgr->ifaces_per_state[OIF_CLAIMED]) {
> > +            struct ovs_iface *iface = node->data;
> > +            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
1);
> > +            VLOG_INFO_RL(&rl,
> > +                         "Not updating pb chassis for %s now as "
> > +                         "sb is readonly", iface->id);
> > +        }
> >      }
> >
> > -    /* Request a seqno update when the flows for new interfaces have
been
> > +    /* Register for a notification about flows being installed in OVS
for all
> > +     * newly claimed interfaces for which pb->chassis has been updated.
> > +     * Request a seqno update when the flows for new interfaces have
been
> >       * installed in OVS.
> >       */
> >      if (new_ifaces) {
> > @@ -403,7 +548,7 @@ if_status_mgr_update_bindings(struct if_status_mgr
*mgr,
> >      struct hmapx_node *node;
> >
> >      /* Notify the binding module to set "down" all bindings that are
still
> > -     * in the process of being installed in OVS, i.e., are not yet
instsalled.
> > +     * in the process of being installed in OVS, i.e., are not yet
installed.
> >       */
> >      HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_INSTALL_FLOWS]) {
> >          struct ovs_iface *iface = node->data;
> > diff --git a/controller/if-status.h b/controller/if-status.h
> > index bb8a3950d..5bd187a25 100644
> > --- a/controller/if-status.h
> > +++ b/controller/if-status.h
> > @@ -26,16 +26,27 @@ struct simap;
> >  struct if_status_mgr *if_status_mgr_create(void);
> >  void if_status_mgr_clear(struct if_status_mgr *);
> >  void if_status_mgr_destroy(struct if_status_mgr *);
> > -
> > -void if_status_mgr_claim_iface(struct if_status_mgr *, const char
*iface_id);
> > +void if_status_mgr_claim_iface(struct if_status_mgr *,
> > +                               const struct sbrec_port_binding *pb,
> > +                               const struct sbrec_chassis *chassis_rec,
> > +                               bool sb_readonly);
> >  void if_status_mgr_release_iface(struct if_status_mgr *, const char
*iface_id);
> >  void if_status_mgr_delete_iface(struct if_status_mgr *, const char
*iface_id);
> >
> > -void if_status_mgr_update(struct if_status_mgr *, struct
local_binding_data *);
> > +void if_status_mgr_update(struct if_status_mgr *, struct
local_binding_data *,
> > +                          const struct sbrec_chassis *chassis,
> > +                          bool sb_readonly);
> >  void if_status_mgr_run(struct if_status_mgr *mgr, struct
local_binding_data *,
> >                         const struct sbrec_chassis *,
> >                         bool sb_readonly, bool ovs_readonly);
> >  void if_status_mgr_get_memory_usage(struct if_status_mgr *mgr,
> >                                      struct simap *usage);
> > +bool if_status_mgr_iface_is_present(struct if_status_mgr *mgr,
> > +                                    const char *iface_id);
> > +bool if_status_handle_claims(struct if_status_mgr *mgr,
> > +                             struct local_binding_data *binding_data,
> > +                             const struct sbrec_chassis *chassis_rec,
> > +                             struct hmap *tracked_datapath,
> > +                             bool sb_readonly);
> >
> >  # endif /* controller/if-status.h */
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index c97744d57..b2b2a4434 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -1509,6 +1509,73 @@ en_runtime_data_run(struct engine_node *node,
void *data)
> >      engine_set_node_state(node, EN_UPDATED);
> >  }
> >
> > +struct ed_type_sb_ro {
> > +    bool sb_readonly;
> > +};
> > +
> > +static void *
> > +en_sb_ro_init(struct engine_node *node OVS_UNUSED,
> > +              struct engine_arg *arg OVS_UNUSED)
> > +{
> > +    struct ed_type_sb_ro *data = xzalloc(sizeof *data);
> > +    return data;
> > +}
> > +
> > +static void
> > +en_sb_ro_run(struct engine_node *node, void *data)
> > +{
> > +    struct ed_type_sb_ro *sb_ro_data = data;
> > +    bool sb_readonly = !engine_get_context()->ovnsb_idl_txn;
> > +    if (sb_ro_data->sb_readonly != sb_readonly) {
> > +        sb_ro_data->sb_readonly = sb_readonly;
> > +        if (!sb_ro_data->sb_readonly) {
> > +            engine_set_node_state(node, EN_UPDATED);
> > +        }
> > +    }
> > +}
> > +
> > +static void
> > +en_sb_ro_cleanup(void *data OVS_UNUSED)
> > +{
> > +}
> > +
> > +static bool
> > +runtime_data_sb_ro_handler(struct engine_node *node, void *data)
> > +{
> > +    const struct sbrec_chassis *chassis = NULL;
> > +
> > +    struct ovsrec_open_vswitch_table *ovs_table =
> > +        (struct ovsrec_open_vswitch_table *)EN_OVSDB_GET(
> > +            engine_get_input("OVS_open_vswitch", node));
> > +
> > +    const char *chassis_id = get_ovs_chassis_id(ovs_table);
> > +
> > +    struct ovsdb_idl_index *sbrec_chassis_by_name =
> > +        engine_ovsdb_node_get_index(
> > +                engine_get_input("SB_chassis", node),
> > +                "name");
> > +
> > +    if (chassis_id) {
> > +        chassis = chassis_lookup_by_name(sbrec_chassis_by_name,
chassis_id);
> > +    }
> > +    if (chassis) {
> > +        struct ed_type_runtime_data *rt_data = data;
> > +        bool sb_readonly = !engine_get_context()->ovnsb_idl_txn;
> > +        struct controller_engine_ctx *ctrl_ctx =
> > +            engine_get_context()->client_ctx;
> > +
> > +        if (if_status_handle_claims(ctrl_ctx->if_mgr,
> > +                                    &rt_data->lbinding_data,
> > +                                    chassis,
> > +                                    &rt_data->tracked_dp_bindings,
> > +                                    sb_readonly)) {
> > +            engine_set_node_state(node, EN_UPDATED);
> > +            rt_data->tracked = true;
> > +        }
> > +    }
> > +    return true;
> > +}
> > +
> >  static bool
> >  runtime_data_ovs_interface_shadow_handler(struct engine_node *node,
void *data)
> >  {
> > @@ -3592,6 +3659,7 @@ main(int argc, char *argv[])
> >      stopwatch_create(VIF_PLUG_RUN_STOPWATCH_NAME, SW_MS);
> >
> >      /* Define inc-proc-engine nodes. */
> > +    ENGINE_NODE(sb_ro, "sb_ro");
> >      ENGINE_NODE_WITH_CLEAR_TRACK_DATA_IS_VALID(ct_zones, "ct_zones");
> >      ENGINE_NODE_WITH_CLEAR_TRACK_DATA(ovs_interface_shadow,
> >                                        "ovs_interface_shadow");
> > @@ -3729,6 +3797,7 @@ main(int argc, char *argv[])
> >      engine_add_input(&en_ovs_interface_shadow, &en_ovs_interface,
> >                       ovs_interface_shadow_ovs_interface_handler);
> >
> > +    engine_add_input(&en_runtime_data, &en_sb_ro,
runtime_data_sb_ro_handler);
> >      engine_add_input(&en_runtime_data, &en_ofctrl_is_connected, NULL);
> >
> >      engine_add_input(&en_runtime_data, &en_ovs_open_vswitch, NULL);
> > @@ -4166,7 +4235,8 @@ main(int argc, char *argv[])
> >                          runtime_data ? &runtime_data->lbinding_data :
NULL;
> >
 stopwatch_start(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME,
> >                                      time_msec());
> > -                    if_status_mgr_update(if_mgr, binding_data);
> > +                    if_status_mgr_update(if_mgr, binding_data, chassis,
> > +                                         !ovnsb_idl_txn);
> >                      stopwatch_stop(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME,
> >                                     time_msec());
> >
> > diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
> > index 2ba930a85..a3c7f8125 100644
> > --- a/tests/ovn-macros.at
> > +++ b/tests/ovn-macros.at
> > @@ -814,3 +814,14 @@ m4_define([OVN_FOR_EACH_NORTHD],
> >       [m4_foreach([NORTHD_USE_PARALLELIZATION], [yes, no],
> >         [m4_foreach([OVN_MONITOR_ALL], [yes, no], [$1
> >  ])])])])
> > +
> > +# OVN_NBCTL(NBCTL_COMMAND) adds NBCTL_COMMAND to list of commands to
be run by RUN_OVN_NBCTL().
> > +m4_define([OVN_NBCTL], [
> > +    command="${command} -- $1"
> > +])
> > +
> > +# RUN_OVN_NBCTL() executes list of commands built by the OVN_NBCTL()
macro.
> > +m4_define([RUN_OVN_NBCTL], [
> > +    check ovn-nbctl ${command}
> > +    unset command
> > +])
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index bbad0f194..d0ea87952 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -130,6 +130,8 @@ m4_define([OVN_WAIT_PATCH_PORT_FLOWS],
> >  m4_define([OVN_WAIT_REMOTE_OUTPUT_FLOWS],
> >    [ovn_wait_remote_output_flows "$1" "$2" "__file__:__line__"])
> >
> > +m4_define([OVN_WAIT_REMOTE_INPUT_FLOWS],
> > +  [ovn_wait_remote_input_flows "$1" "$2" "__file__:__line__"])
> >
> >  AT_BANNER([OVN components])
> >
> > @@ -13936,6 +13938,11 @@ wait_column "$hv1_uuid" Port_Binding
requested_chassis logical_port=lsp0
> >  wait_column "$hv2_uuid" Port_Binding additional_chassis
logical_port=lsp0
> >  wait_column "$hv2_uuid" Port_Binding requested_additional_chassis
logical_port=lsp0
> >
> > +# Check ovn-installed updated for main chassis
> > +wait_for_ports_up
> > +OVS_WAIT_UNTIL([test `as hv1 ovs-vsctl get Interface lsp0
external_ids:ovn-installed` = '"true"'])
> > +OVS_WAIT_UNTIL([test x`as hv2 ovs-vsctl get Interface lsp0
external_ids:ovn-installed` = x])
> > +
> >  # Check that setting iface:encap-ip populates
Port_Binding:additional_encap
> >  wait_row_count Encap 2 chassis_name=hv1
> >  wait_row_count Encap 2 chassis_name=hv2
> > @@ -13961,6 +13968,11 @@ wait_column "$hv2_uuid" Port_Binding
requested_chassis logical_port=lsp0
> >  wait_column "" Port_Binding additional_chassis logical_port=lsp0
> >  wait_column "" Port_Binding requested_additional_chassis
logical_port=lsp0
> >
> > +# Check ovn-installed updated for main chassis and not for other
chassis
> > +wait_for_ports_up
> > +OVS_WAIT_UNTIL([test `as hv2 ovs-vsctl get Interface lsp0
external_ids:ovn-installed` = '"true"'])
> > +OVS_WAIT_UNTIL([test x`as hv1 ovs-vsctl get Interface lsp0
external_ids:ovn-installed` = x])
> > +
> >  # Check that additional_encap is cleared
> >  wait_column "" Port_Binding additional_encap logical_port=lsp0
> >
> > @@ -15259,7 +15271,9 @@ AT_CHECK([as hv1 ovs-ofctl dump-flows br-int
table=65 | grep actions=output:1],
> >  echo "verifying that lsp0 binding moves when requested-chassis is
changed"
> >
> >  ovn-nbctl lsp-set-options lsp0 requested-chassis=hv2
> > -OVS_WAIT_UNTIL([test 1 = $(grep -c "Releasing lport lsp0 from this
chassis" hv1/ovn-controller.log)])
> > +# We might see multiple "Releasing lport ...", when sb is read only
> > +OVS_WAIT_UNTIL([test 1 -le $(grep -c "Releasing lport lsp0 from this
chassis" hv1/ovn-controller.log)])
> > +
> >  wait_column "$hv2_uuid" Port_Binding chassis logical_port=lsp0
> >
> >  # (6) Chassis hv2 should add flows and hv1 should not.
> > @@ -15346,7 +15360,7 @@ AT_CHECK([as hv1 ovs-ofctl dump-flows br-int
table=0 | grep in_port=1], [0], [ig
> >  AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=65 | grep
actions=output:1], [0], [ignore])
> >
> >  check ovn-nbctl --wait=hv lsp-set-options lsp0
requested-chassis=non-existant-chassis
> > -OVS_WAIT_UNTIL([test 1 = $(grep -c "Releasing lport lsp0 from this
chassis" hv1/ovn-controller.log)])
> > +OVS_WAIT_UNTIL([test 1 -le $(grep -c "Releasing lport lsp0 from this
chassis" hv1/ovn-controller.log)])
> >  check ovn-nbctl --wait=hv sync
> >  wait_column '' Port_Binding chasssi logical_port=lsp0
> >  AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=0 | grep
in_port=1], [1], [])
> > @@ -32517,3 +32531,119 @@ OVS_WAIT_UNTIL([
> >  OVN_CLEANUP([hv1], [hv2])
> >  AT_CLEANUP
> >  ])
> > +
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([recomputes])
> > +ovn_start
> > +
> > +n_hv=4
> > +
> > +# Add chassis
> > +net_add n1
> > +for i in $(seq 1 $n_hv); do
> > +    sim_add hv$i
> > +    as hv$i
> > +    check ovs-vsctl add-br br-phys
> > +    ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
> > +    ovn_attach n1 br-phys 192.168.0.$i 24 geneve
> > +done
> > +
> > +add_switch_ports() {
> > +    start_port=$1
> > +    end_port=$2
> > +    nb_hv=$3
> > +    bulk_size=$4
> > +    for ((i=start_port; i<end_port; )) do
> > +        start_bulk=$i
> > +        for hv in $(seq 1 $nb_hv); do
> > +            end_bulk=$((start_bulk+bulk_size-1))
> > +            for port in $(seq $start_bulk $end_bulk); do
> > +                logical_switch_port=lsp${port}
> > +                OVN_NBCTL(lsp-add ls1 $logical_switch_port)
> > +                OVN_NBCTL(lsp-set-addresses $logical_switch_port
dynamic)
> > +            done
> > +            start_bulk=$((end_bulk+1))
> > +        done
> > +        RUN_OVN_NBCTL()
> > +
> > +        start_bulk=$i
> > +        for hv in $(seq 1 $nb_hv); do
> > +            end_bulk=$((start_bulk+bulk_size-1))
> > +            for port in $(seq $start_bulk $end_bulk); do
> > +                logical_switch_port=lsp${port}
> > +                as hv$hv ovs-vsctl \
> > +                    --no-wait -- add-port br-int vif${port} \
> > +                    -- set Interface vif${port}
external_ids:iface-id=$logical_switch_port
> > +            done
> > +            start_bulk=$((end_bulk+1))
> > +        done
> > +        i=$((end_bulk+1))
> > +    done
> > +}
> > +check ovn-nbctl ls-add ls1
> > +check ovn-nbctl set Logical_Switch ls1 other_config:subnet=10.1.0.0/16
> > +check ovn-nbctl set Logical_Switch ls1
other_config:exclude_ips=10.1.255.254
> > +
> > +check ovn-nbctl lr-add lr1
> > +check ovn-nbctl lsp-add ls1 lsp0 -- set Logical_Switch_Port lsp0
type=router options:router-port=lrp0 addresses=dynamic
> > +check ovn-nbctl lrp-add lr1 lrp0 "f0:00:00:01:00:01" 10.1.255.254/16
> > +check ovn-nbctl lr-nat-add lr1 snat 10.2.0.1 10.1.0.0/16
> > +
> > +lflow_run=0
> > +check ovn-nbctl --wait=hv sync
> > +
> > +# Tunnel ports might not be added (yet) at this point on slow system.
> > +# Wait for flows related to such ports to ensure those ports have been
added
> > +# before we measure recomputes. Otherwise, ovs_interface handler might
be run
> > +# afterwards for tunnel ports, causing recomputes.
> > +for i in $(seq 1 $n_hv); do
> > +    for j in $(seq 1 $n_hv); do
> > +        if test $i != $j; then
> > +            OVN_WAIT_REMOTE_INPUT_FLOWS(["hv$i"],["hv$j"])
> > +        fi
> > +    done
> > +done
> > +
> > +for i in $(seq 1 $n_hv); do
> > +    as hv$i
> > +    lflow_run1=$(ovn-appctl -t ovn-controller coverage/read-counter
lflow_run)
> > +    lflow_run=`expr $lflow_run1 + $lflow_run`
> > +done
> > +
> > +add_switch_ports 1 1000 $n_hv 5
> > +
> > +wait_for_ports_up
> > +check ovn-nbctl --wait=hv sync
> > +
> > +for i in $(seq 1 $n_hv); do
> > +    pid=$(cat hv${i}/ovn-controller.pid)
> > +    u=$((u+$(cat "/proc/$pid/stat" | awk '{print $14}')))
> > +    s=$((s+$(cat "/proc/$pid/stat" | awk '{print $15}')))
> > +done
> > +
> > +n_pid=$(cat northd/ovn-northd.pid)
> > +n_u=$(cat "/proc/$pid/stat" | awk '{print $14}')
> > +n_s=$(cat "/proc/$pid/stat" | awk '{print $15}')
> > +
> > +echo "Total Northd User Time: $n_u"
> > +echo "Total Northd System Time: $n_s"
> > +echo "Total Controller User Time: $u"
> > +echo "Total Controller System Time: $s"
> > +
> > +lflow_run_end=0
> > +for i in $(seq 1 $n_hv); do
> > +    as hv$i
> > +    lflow_run1=$(ovn-appctl -t ovn-controller coverage/read-counter
lflow_run)
> > +    lflow_run_end=`expr $lflow_run1 + $lflow_run_end`
> > +done
> > +n_recomputes=`expr $lflow_run_end - $lflow_run`
> > +echo "$n_recomputes recomputes"
> > +
> > +AT_CHECK([test $lflow_run_end == $lflow_run])
> > +
> > +for i in $(seq 2 $n_hv); do
> > +    OVN_CLEANUP_SBOX([hv$i])
> > +done
> > +OVN_CLEANUP([hv1])
> > +AT_CLEANUP
> > +])
> > diff --git a/tests/perf-northd.at b/tests/perf-northd.at
> > index 74b69e9d4..ca115dadc 100644
> > --- a/tests/perf-northd.at
> > +++ b/tests/perf-northd.at
> > @@ -76,23 +76,6 @@ m4_define([PERF_RECORD_STOP], [
> >      PERF_RECORD_STOPWATCH(ovn-northd-loop, ["Short term average"],
[Average (northd-loop in msec)])
> >  ])
> >
> > -# OVN_NBCTL([NBCTL_COMMAND])
> > -#
> > -# Add NBCTL_COMMAND to list of commands to be run by RUN_OVN_NBCTL().
> > -#
> > -m4_define([OVN_NBCTL], [
> > -    command="${command} -- $1"
> > -])
> > -
> > -# RUN_OVN_NBCTL()
> > -#
> > -# Execute list of commands built by the OVN_NBCTL() macro.
> > -#
> > -m4_define([RUN_OVN_NBCTL], [
> > -    check ovn-nbctl ${command}
> > -    unset command
> > -])
> > -
> >  OVS_START_SHELL_HELPERS
> >  generate_subnet () {
> >      local a=$(printf %d $(expr $1 / 256 + 10))
> > @@ -208,4 +191,4 @@ BUILD_NBDB(OVN_BASIC_SCALE_CONFIG(500, 50))
> >
> >  PERF_RECORD_STOP()
> >  AT_CLEANUP
> > -])
> > \ No newline at end of file
> > +])
> > --
> > 2.31.1
> >
>

On 8/25/22 08:35, Han Zhou wrote:
> On Mon, Aug 22, 2022 at 9:41 AM Han Zhou <hzhou@ovn.org> wrote:
>>
>>
>>
>> On Fri, Aug 19, 2022 at 8:40 AM Xavier Simonart <xsimonar@redhat.com>
> wrote:
>>>
>>> When VIF ports are claimed on a chassis, SBDB Port_Binding table is
> updated.
>>> If the SBDB IDL is still is read-only ("in transaction") when such a
> update
>>> is required, the update is not possible and recompute is triggered
> through
>>> I+P failure.
>>>
>>> This situation can happen:
>>> - after updating Port_Binding->chassis to SBDB for one port, in a
> following
>>>   iteration, ovn-controller handles Interface:external_ids:ovn-installed
>>>   (for the same port) while SBDB is still read-only.
>>> - after updating Port_Binding->chassis to SBDB for one port, in a
> following
>>>   iteration, ovn-controller updates Port_Binding->chassis for another
> port,
>>>   while SBDB is still read-only.
>>>
>>> This patch prevent the recompute, by having the if-status module
>>> updating the Port_Binding chassis (if needed) when possible.
>>> This does not delay Port_Binding chassis update compared to before this
> patch.
>>> - With the patch, Port_Binding chassis will be updated as soon as SBDB
> is
>>> again writable, without recompute.
>>> - Without the patch, Port_Binding chassis was updated as soon as SBDB
> was
>>> again writable, through a recompute.
>>>
>>> As part of this patch, ovn-installed will not be updated for additional
> chassis;
>>> it will only be updated when the migration is completed.
>>>
>>> Note that this patch also fixes an issue where port was not always
> properly
>>> reported up for virtual ports (causing flaky failures in "virtual ports"
>>> test case).
>>>
>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2059253
>>> Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
>>>
>>> ---
>>> v2:  - handled Dumitru's comments.
>>>      - handled Han's comments, mainly ensure we moved out of CLAIMED
> state
>>>        only after updating pb->chassis to guarentee physical flows are
> installed
>>>        when ovn-installed is updated in OVS.
>>>      - slighly reorganize the code to isolate 'notify_up = false' cases
> in
>>>        claim_port (i.e. ports such as virtual ports), in the idea of
> making
>>>        future patch preventing recomputes when virtual ports are
> claimed.
>>>      - updated test case to cause more race conditions.
>>>      - rebased on origin/main
>>>      - note that "additional chassis" as now supported by
>>>        "Support LSP:options:requested-chassis as a list" might still
> cause
>>>        recomputes.
>>>      - fixed missing flows when Port_Binding chassis was updated by
> mgr_update
>>>        w/o any lflow recalculation.
>>> v3:  - handled Dumitru's comments on v2, mainly have runtime_data
> handler
>>>        handling pb_claims when sb becomes writable (instead of a lflow
> handler).
>>>      - fixed test as it was not checking recomputes on all hv, as well
> as a flaky
>>> v4:  - handled Han's comments, mainly update pb->chassis until it's
> confirmed.
>>>      - updated doc to reflect that pb->chassis update can happen at any
> state, and
>>>        the CLAIMED state is only for the initial attempt.
>>
>> Thanks Xavier. All looks good to me except that it seems you updated the
> diagram but forgot to update the description, I have the below small
> incremental change, and if you are ok, I can merge it without the need for
> v6:
>>
>> diff --git a/controller/if-status.c b/controller/if-status.c
>> index 2590ec27f..d1c14ac30 100644
>> --- a/controller/if-status.c
>> +++ b/controller/if-status.c
>> @@ -54,11 +54,12 @@ VLOG_DEFINE_THIS_MODULE(if_status);
>>   */
>>
>>  enum if_state {
>> -    OIF_CLAIMED,       /* Newly claimed interface. pb->chassis not yet
> updated.
>> -                        */
>> -    OIF_INSTALL_FLOWS, /* Claimed interface with pb->chassis successfully
>> -                        * updated in SB and for which flows are still
> being
>> -                        * installed.
>> +    OIF_CLAIMED,       /* Newly claimed interface. pb->chassis update
> not yet
>> +                          initiated. */
>> +    OIF_INSTALL_FLOWS, /* Claimed interface with pb->chassis update sent
> to
>> +                        * SB (but update notification not confirmed, so
> the
>> +                        * update may be resent in any of the following
> states)
>> +                        * and for which flows are still being installed.
>>                          */
>>      OIF_MARK_UP,       /* Interface with flows successfully installed in
> OVS
>>                          * but not yet marked "up" in the binding module
> (in
>>
>> Thanks,
>> Han
> 
> I heard no objection to the above and this is really minor, so applied it
> to main. Thanks Xavier!

Thanks Xavier and Han!  I'm glad to see the if-status/ovn-installed code
getting in better shape.

> (Just realized after pushing, Dumitru had Acked one of the previous
> versions but I forgot to add his name. I apologize!)

No problem whatsoever. :)

Regards,
Dumitru

Hi Han

Sorry for not replying earlier - I wanted to give a last chance for Dumitru
or others to comment
Thanks for applying the patch and all you feedback

Thanks
Xavier

On Thu, Aug 25, 2022 at 10:42 AM Dumitru Ceara <dceara@redhat.com> wrote:

> On 8/25/22 08:35, Han Zhou wrote:
> > On Mon, Aug 22, 2022 at 9:41 AM Han Zhou <hzhou@ovn.org> wrote:
> >>
> >>
> >>
> >> On Fri, Aug 19, 2022 at 8:40 AM Xavier Simonart <xsimonar@redhat.com>
> > wrote:
> >>>
> >>> When VIF ports are claimed on a chassis, SBDB Port_Binding table is
> > updated.
> >>> If the SBDB IDL is still is read-only ("in transaction") when such a
> > update
> >>> is required, the update is not possible and recompute is triggered
> > through
> >>> I+P failure.
> >>>
> >>> This situation can happen:
> >>> - after updating Port_Binding->chassis to SBDB for one port, in a
> > following
> >>>   iteration, ovn-controller handles
> Interface:external_ids:ovn-installed
> >>>   (for the same port) while SBDB is still read-only.
> >>> - after updating Port_Binding->chassis to SBDB for one port, in a
> > following
> >>>   iteration, ovn-controller updates Port_Binding->chassis for another
> > port,
> >>>   while SBDB is still read-only.
> >>>
> >>> This patch prevent the recompute, by having the if-status module
> >>> updating the Port_Binding chassis (if needed) when possible.
> >>> This does not delay Port_Binding chassis update compared to before this
> > patch.
> >>> - With the patch, Port_Binding chassis will be updated as soon as SBDB
> > is
> >>> again writable, without recompute.
> >>> - Without the patch, Port_Binding chassis was updated as soon as SBDB
> > was
> >>> again writable, through a recompute.
> >>>
> >>> As part of this patch, ovn-installed will not be updated for additional
> > chassis;
> >>> it will only be updated when the migration is completed.
> >>>
> >>> Note that this patch also fixes an issue where port was not always
> > properly
> >>> reported up for virtual ports (causing flaky failures in "virtual
> ports"
> >>> test case).
> >>>
> >>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2059253
> >>> Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
> >>>
> >>> ---
> >>> v2:  - handled Dumitru's comments.
> >>>      - handled Han's comments, mainly ensure we moved out of CLAIMED
> > state
> >>>        only after updating pb->chassis to guarentee physical flows are
> > installed
> >>>        when ovn-installed is updated in OVS.
> >>>      - slighly reorganize the code to isolate 'notify_up = false' cases
> > in
> >>>        claim_port (i.e. ports such as virtual ports), in the idea of
> > making
> >>>        future patch preventing recomputes when virtual ports are
> > claimed.
> >>>      - updated test case to cause more race conditions.
> >>>      - rebased on origin/main
> >>>      - note that "additional chassis" as now supported by
> >>>        "Support LSP:options:requested-chassis as a list" might still
> > cause
> >>>        recomputes.
> >>>      - fixed missing flows when Port_Binding chassis was updated by
> > mgr_update
> >>>        w/o any lflow recalculation.
> >>> v3:  - handled Dumitru's comments on v2, mainly have runtime_data
> > handler
> >>>        handling pb_claims when sb becomes writable (instead of a lflow
> > handler).
> >>>      - fixed test as it was not checking recomputes on all hv, as well
> > as a flaky
> >>> v4:  - handled Han's comments, mainly update pb->chassis until it's
> > confirmed.
> >>>      - updated doc to reflect that pb->chassis update can happen at any
> > state, and
> >>>        the CLAIMED state is only for the initial attempt.
> >>
> >> Thanks Xavier. All looks good to me except that it seems you updated the
> > diagram but forgot to update the description, I have the below small
> > incremental change, and if you are ok, I can merge it without the need
> for
> > v6:
> >>
> >> diff --git a/controller/if-status.c b/controller/if-status.c
> >> index 2590ec27f..d1c14ac30 100644
> >> --- a/controller/if-status.c
> >> +++ b/controller/if-status.c
> >> @@ -54,11 +54,12 @@ VLOG_DEFINE_THIS_MODULE(if_status);
> >>   */
> >>
> >>  enum if_state {
> >> -    OIF_CLAIMED,       /* Newly claimed interface. pb->chassis not yet
> > updated.
> >> -                        */
> >> -    OIF_INSTALL_FLOWS, /* Claimed interface with pb->chassis
> successfully
> >> -                        * updated in SB and for which flows are still
> > being
> >> -                        * installed.
> >> +    OIF_CLAIMED,       /* Newly claimed interface. pb->chassis update
> > not yet
> >> +                          initiated. */
> >> +    OIF_INSTALL_FLOWS, /* Claimed interface with pb->chassis update
> sent
> > to
> >> +                        * SB (but update notification not confirmed, so
> > the
> >> +                        * update may be resent in any of the following
> > states)
> >> +                        * and for which flows are still being
> installed.
> >>                          */
> >>      OIF_MARK_UP,       /* Interface with flows successfully installed
> in
> > OVS
> >>                          * but not yet marked "up" in the binding module
> > (in
> >>
> >> Thanks,
> >> Han
> >
> > I heard no objection to the above and this is really minor, so applied it
> > to main. Thanks Xavier!
>
> Thanks Xavier and Han!  I'm glad to see the if-status/ovn-installed code
> getting in better shape.
>
> > (Just realized after pushing, Dumitru had Acked one of the previous
> > versions but I forgot to add his name. I apologize!)
>
> No problem whatsoever. :)
>
> Regards,
> Dumitru
>
>