[ovs-dev,v4,1/2] ovn-controller: fixed ovn-installed not always properly added or removed.

OVN checks whether ovn-installed is already present (in OVS) before updating it.
This might cause ovn-installed related issues in the following case:
- (1) ovn-installed is present
- (2) we claim the interface
- (3) we update ovs, removing ovn-installed and start installing flows
- (4) (next loop), after flows installed, we check if ovn-installed is absent,
  and if yes, we update OVS with ovn-installed.
However, in step4, if OVS is still busy from step 3, ovn-installed is read as
present; hence OVN controller does not update it and move to INSTALLED state.

ovn-installed was also not properly deleted on port or binding removal.

Note that port->up is not properly removed on external_ids:iface-id removal when
sb is read-only. This will be fixed in a further patch.

Fixes: a7c7d4519e50 ("controller: avoid recomputes triggered by SBDB Port_Binding updates.")

Signed-off-by: Xavier Simonart <xsimonar@redhat.com>

---
v2: - handled Dumitru's comments
    - rebased on main
    - updated state machine diagram as commented in v1 commit message
    - remove ovn-installed on port deletion or external_ids:iface-id removal.
    - added test case
v3: - handled more comment from Dumitru
    - fixed ovn-install not removed when ovs is read-only
    - moved test case from unit (ovn.at) to system (system-ovn).
    - test case connects to OVS db through tcp and uses iptables to momentarly block the idl update
      to simulate read-only ovsdb
v4: - handled comments from Ales: avoid using is_deleted outside of tracked loops.
---
 controller/binding.c        |  68 ++++++-
 controller/binding.h        |  11 ++
 controller/if-status.c      | 204 ++++++++++++++++++---
 controller/if-status.h      |   7 +
 controller/ovn-controller.c |   5 +
 tests/system-ovn.at         | 349 ++++++++++++++++++++++++++++++++++++
 6 files changed, 612 insertions(+), 32 deletions(-)

On Fri, Apr 28, 2023 at 1:59 PM Xavier Simonart <xsimonar@redhat.com> wrote:

> OVN checks whether ovn-installed is already present (in OVS) before
> updating it.
> This might cause ovn-installed related issues in the following case:
> - (1) ovn-installed is present
> - (2) we claim the interface
> - (3) we update ovs, removing ovn-installed and start installing flows
> - (4) (next loop), after flows installed, we check if ovn-installed is
> absent,
>   and if yes, we update OVS with ovn-installed.
> However, in step4, if OVS is still busy from step 3, ovn-installed is read
> as
> present; hence OVN controller does not update it and move to INSTALLED
> state.
>
> ovn-installed was also not properly deleted on port or binding removal.
>
> Note that port->up is not properly removed on external_ids:iface-id
> removal when
> sb is read-only. This will be fixed in a further patch.
>
> Fixes: a7c7d4519e50 ("controller: avoid recomputes triggered by SBDB
> Port_Binding updates.")
>
> Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
>
> ---
> v2: - handled Dumitru's comments
>     - rebased on main
>     - updated state machine diagram as commented in v1 commit message
>     - remove ovn-installed on port deletion or external_ids:iface-id
> removal.
>     - added test case
> v3: - handled more comment from Dumitru
>     - fixed ovn-install not removed when ovs is read-only
>     - moved test case from unit (ovn.at) to system (system-ovn).
>     - test case connects to OVS db through tcp and uses iptables to
> momentarly block the idl update
>       to simulate read-only ovsdb
> v4: - handled comments from Ales: avoid using is_deleted outside of
> tracked loops.
> ---
>  controller/binding.c        |  68 ++++++-
>  controller/binding.h        |  11 ++
>  controller/if-status.c      | 204 ++++++++++++++++++---
>  controller/if-status.h      |   7 +
>  controller/ovn-controller.c |   5 +
>  tests/system-ovn.at         | 349 ++++++++++++++++++++++++++++++++++++
>  6 files changed, 612 insertions(+), 32 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 5df62baef..357e77609 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -746,6 +746,19 @@ local_binding_get_lport_ofport(const struct shash
> *local_bindings,
>              u16_to_ofp(lbinding->iface->ofport[0]) : 0;
>  }
>
> +bool
> +local_binding_is_ovn_installed(struct shash *local_bindings,
> +                               const char *pb_name)
> +{
> +    struct local_binding *lbinding =
> +        local_binding_find(local_bindings, pb_name);
> +    if (lbinding && lbinding->iface) {
> +        return smap_get_bool(&lbinding->iface->external_ids,
> +                             OVN_INSTALLED_EXT_ID, false);
> +    }
> +    return false;
> +}
> +
>  bool
>  local_binding_is_up(struct shash *local_bindings, const char *pb_name,
>                      const struct sbrec_chassis *chassis_rec)
> @@ -783,6 +796,7 @@ local_binding_is_down(struct shash *local_bindings,
> const char *pb_name,
>          } else if (b_lport->pb->chassis) {
>              VLOG_DBG("lport %s already claimed by other chassis",
>                       b_lport->pb->logical_port);
> +            return true;
>          }
>      }
>
> @@ -834,6 +848,38 @@ local_binding_set_up(struct shash *local_bindings,
> const char *pb_name,
>      }
>  }
>
> +void
> +local_binding_remove_ovn_installed(
> +        struct shash *local_bindings,
> +        const struct ovsrec_interface_table *iface_table,
> +        const char *pb_name, bool ovs_readonly)
> +{
> +    if (ovs_readonly) {
> +        return;
> +    }
> +    struct local_binding *lbinding =
> +        local_binding_find(local_bindings, pb_name);
> +    if (lbinding && lbinding->iface) {
> +        const struct uuid *iface_uuid = &lbinding->iface->header_.uuid;
> +        remove_ovn_installed_for_uuid(iface_table, iface_uuid);
> +    }
> +}
> +
> +void
> +remove_ovn_installed_for_uuid(const struct ovsrec_interface_table
> *iface_table,
> +                              const struct uuid *iface_uuid)
> +{
> +    const struct ovsrec_interface *iface_rec =
> +        ovsrec_interface_table_get_for_uuid(iface_table, iface_uuid);
> +    if (iface_rec && smap_get_bool(&iface_rec->external_ids,
> +                                   OVN_INSTALLED_EXT_ID, false)) {
> +        VLOG_INFO("Removing iface %s ovn-installed in OVS",
> +                  iface_rec->name);
> +        ovsrec_interface_update_external_ids_delkey(iface_rec,
> +                                                    OVN_INSTALLED_EXT_ID);
> +    }
> +}
> +
>  void
>  local_binding_set_down(struct shash *local_bindings, const char *pb_name,
>                         const struct sbrec_chassis *chassis_rec,
> @@ -1239,7 +1285,9 @@ claim_lport(const struct sbrec_port_binding *pb,
>                      return false;
>                  }
>              } else {
> -                if (pb->n_up && !pb->up[0]) {
> +                if ((pb->n_up && !pb->up[0]) ||
> +                    !smap_get_bool(&iface_rec->external_ids,
> +                                   OVN_INSTALLED_EXT_ID, false)) {
>                      if_status_mgr_claim_iface(if_mgr, pb, chassis_rec,
>                                                sb_readonly);
>                  }
> @@ -1464,9 +1512,11 @@ consider_vif_lport_(const struct sbrec_port_binding
> *pb,
>              const char *requested_chassis_option = smap_get(
>                  &pb->options, "requested-chassis");
>              VLOG_INFO_RL(&rl,
> -                "Not claiming lport %s, chassis %s requested-chassis %s",
> +                "Not claiming lport %s, chassis %s requested-chassis %s "
> +                "pb->chassis %s",
>                  pb->logical_port, b_ctx_in->chassis_rec->name,
> -                requested_chassis_option ? requested_chassis_option :
> "[]");
> +                requested_chassis_option ? requested_chassis_option :
> "[]",
> +                pb->chassis ? pb->chassis->name: "");
>          }
>      }
>
> @@ -2288,6 +2338,11 @@ consider_iface_release(const struct
> ovsrec_interface *iface_rec,
>                  return false;
>              }
>          }
> +        if (lbinding->iface && lbinding->iface->name) {
> +            if_status_mgr_remove_ovn_installed(b_ctx_out->if_mgr,
> +                                               lbinding->iface->name,
> +
>  &lbinding->iface->header_.uuid);
> +        }
>
>      } else if (lbinding && b_lport && b_lport->type == LP_LOCALPORT) {
>          /* lbinding is associated with a localport.  Remove it from the
> @@ -2558,6 +2613,7 @@ handle_deleted_lport(const struct sbrec_port_binding
> *pb,
>      if (ld) {
>          remove_pb_from_local_datapath(pb,
>                                        b_ctx_out, ld);
> +        if_status_mgr_release_iface(b_ctx_out->if_mgr, pb->logical_port);
>          return;
>      }
>
> @@ -2581,6 +2637,7 @@ handle_deleted_lport(const struct sbrec_port_binding
> *pb,
>              remove_pb_from_local_datapath(pb, b_ctx_out,
>                                            ld);
>          }
> +        if_status_mgr_release_iface(b_ctx_out->if_mgr, pb->logical_port);
>      }
>  }
>
> @@ -2627,6 +2684,11 @@ handle_deleted_vif_lport(const struct
> sbrec_port_binding *pb,
>      }
>
>      handle_deleted_lport(pb, b_ctx_in, b_ctx_out);
> +    if (lbinding && lbinding->iface && lbinding->iface->name) {
> +        if_status_mgr_remove_ovn_installed(b_ctx_out->if_mgr,
> +                                           lbinding->iface->name,
> +
>  &lbinding->iface->header_.uuid);
> +    }
>      return true;
>  }
>
> diff --git a/controller/binding.h b/controller/binding.h
> index 6c3a98b02..0b9c6994f 100644
> --- a/controller/binding.h
> +++ b/controller/binding.h
> @@ -159,6 +159,14 @@ 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,
>                             const struct sbrec_chassis *);
>
> +bool local_binding_is_ovn_installed(struct shash *local_bindings,
> +                                    const char *pb_name);
> +void local_binding_remove_ovn_installed(
> +        struct shash *local_bindings,
> +        const struct ovsrec_interface_table *iface_table,
> +        const char *pb_name,
> +        bool ovs_readonly);
> +
>  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,
> @@ -195,6 +203,9 @@ void set_pb_chassis_in_sbrec(const struct
> sbrec_port_binding *pb,
>                               const struct sbrec_chassis *chassis_rec,
>                               bool is_set);
>
> +void remove_ovn_installed_for_uuid(const struct ovsrec_interface_table *,
> +                                   const struct uuid *);
> +
>  /* 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 d1c14ac30..ac36a9fb9 100644
> --- a/controller/if-status.c
> +++ b/controller/if-status.c
> @@ -54,32 +54,37 @@ VLOG_DEFINE_THIS_MODULE(if_status);
>   */
>
>  enum if_state {
> -    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
> -                        * SB and OVS databases).
> -                        */
> -    OIF_MARK_DOWN,     /* Released interface but not yet marked "down" in
> the
> -                        * binding module (in SB and/or OVS databases).
> -                        */
> -    OIF_INSTALLED,     /* Interface flows programmed in OVS and binding
> marked
> -                        * "up" in the binding module.
> -                        */
> +    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_REM_OLD_OVN_INST, /* Interface with flows successfully installed
> in OVS
> +                           * but with ovn-installed still in OVSDB.
> +                           */
> +    OIF_MARK_UP,          /* Interface with flows successfully installed
> in OVS
> +                           * but not yet marked "up" in the binding
> module (in
> +                           * SB and OVS databases).
> +                           */
> +    OIF_MARK_DOWN,        /* Released interface but not yet marked "down"
> in
> +                           * the binding module (in SB and/or OVS
> databases).
> +                           */
> +    OIF_INSTALLED,        /* Interface flows programmed in OVS and binding
> +                           * marked "up" in the binding module.
> +                           */
>      OIF_MAX,
>  };
>
>  static const char *if_state_names[] = {
> -    [OIF_CLAIMED]       = "CLAIMED",
> -    [OIF_INSTALL_FLOWS] = "INSTALL_FLOWS",
> -    [OIF_MARK_UP]       = "MARK_UP",
> -    [OIF_MARK_DOWN]     = "MARK_DOWN",
> -    [OIF_INSTALLED]     = "INSTALLED",
> +    [OIF_CLAIMED]          = "CLAIMED",
> +    [OIF_INSTALL_FLOWS]    = "INSTALL_FLOWS",
> +    [OIF_REM_OLD_OVN_INST] = "REM_OLD_OVN_INST",
> +    [OIF_MARK_UP]          = "MARK_UP",
> +    [OIF_MARK_DOWN]        = "MARK_DOWN",
> +    [OIF_INSTALLED]        = "INSTALLED",
>  };
>
>  /*
> @@ -109,11 +114,26 @@ static const char *if_state_names[] = {
>   * |     |                      |   - 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(seqno rcvd, ovn-installed present)          |
> | | |
> + * |        V                                                     |
> | | |
> + * |    +--------------------+                                    |
> | | |
> + * |    |                    |  mgr_run()                         |
> | | |
> + * +--- | REM_OLD_OVN_INST   |  - remove ovn-installed in ovs     |
> | | |
> + * |    +--------------------+                                    |
> | | |
> + * |               |                                              |
> | | |
> + * |               |                                              |
> | | |
> + * |               | mgr_update( ovn_installed not present)       |
> | | |
> + * |               |                                              |
> | | |
> + * |               |  +-------------------------------------------+
> | | |
> + * |               |  |
> | | |
> + * |               |  |  mgr_run(seqno rcvd, ovn-installed not present)
> | | |
> + * |               |  |  - set port up in sb
>  | | |
> + * |               |  |  - set ovn-installed in ovs
> | | |
> + * |release_iface  |  |
> | | |
> + * |               V  V
> | | |
>   * |   +----------------------+
> | | |
>   * |   |                      |  mgr_run()
>  | | |
>   * +-- |       MARK_UP        |  - set port up in sb
>  | | |
> @@ -155,6 +175,9 @@ struct if_status_mgr {
>      /* All local interfaces, mapping from 'iface-id' to 'struct
> ovs_iface'. */
>      struct shash ifaces;
>
> +    /* local interfaces which need ovn-install removal */
> +    struct shash ovn_uninstall_hash;
> +
>      /* All local interfaces, stored per state. */
>      struct hmapx ifaces_per_state[OIF_MAX];
>
> @@ -170,15 +193,20 @@ struct if_status_mgr {
>  static struct ovs_iface *ovs_iface_create(struct if_status_mgr *,
>                                            const char *iface_id,
>                                            enum if_state );
> +static void add_to_ovn_uninstall_hash(struct if_status_mgr *, const char
> *,
> +                                      const struct uuid *);
>  static void ovs_iface_destroy(struct if_status_mgr *, struct ovs_iface *);
> +static void ovn_uninstall_hash_destroy(struct if_status_mgr *mgr, char
> *name);
>  static void ovs_iface_set_state(struct if_status_mgr *, struct ovs_iface
> *,
>                                  enum if_state);
>
>  static void if_status_mgr_update_bindings(
>      struct if_status_mgr *mgr, struct local_binding_data *binding_data,
>      const struct sbrec_chassis *,
> +    const struct ovsrec_interface_table *iface_table,
>      bool sb_readonly, bool ovs_readonly);
>
> +static void ovn_uninstall_hash_account_mem(const char *name, bool erase);
>  struct if_status_mgr *
>  if_status_mgr_create(void)
>  {
> @@ -189,6 +217,7 @@ if_status_mgr_create(void)
>          hmapx_init(&mgr->ifaces_per_state[i]);
>      }
>      shash_init(&mgr->ifaces);
> +    shash_init(&mgr->ovn_uninstall_hash);
>      return mgr;
>  }
>
> @@ -202,6 +231,11 @@ if_status_mgr_clear(struct if_status_mgr *mgr)
>      }
>      ovs_assert(shash_is_empty(&mgr->ifaces));
>
> +    SHASH_FOR_EACH_SAFE (node, &mgr->ovn_uninstall_hash) {
> +        ovn_uninstall_hash_destroy(mgr, node->data);
> +    }
> +    ovs_assert(shash_is_empty(&mgr->ovn_uninstall_hash));
> +
>      for (size_t i = 0; i < ARRAY_SIZE(mgr->ifaces_per_state); i++) {
>          ovs_assert(hmapx_is_empty(&mgr->ifaces_per_state[i]));
>      }
> @@ -212,6 +246,7 @@ if_status_mgr_destroy(struct if_status_mgr *mgr)
>  {
>      if_status_mgr_clear(mgr);
>      shash_destroy(&mgr->ifaces);
> +    shash_destroy(&mgr->ovn_uninstall_hash);
>      for (size_t i = 0; i < ARRAY_SIZE(mgr->ifaces_per_state); i++) {
>          hmapx_destroy(&mgr->ifaces_per_state[i]);
>      }
> @@ -238,6 +273,7 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr,
>      switch (iface->state) {
>      case OIF_CLAIMED:
>      case OIF_INSTALL_FLOWS:
> +    case OIF_REM_OLD_OVN_INST:
>      case OIF_MARK_UP:
>          /* Nothing to do here. */
>          break;
> @@ -274,6 +310,7 @@ if_status_mgr_release_iface(struct if_status_mgr *mgr,
> const char *iface_id)
>          /* Not yet fully installed interfaces can be safely deleted. */
>          ovs_iface_destroy(mgr, iface);
>          break;
> +    case OIF_REM_OLD_OVN_INST:
>      case OIF_MARK_UP:
>      case OIF_INSTALLED:
>          /* Properly mark interfaces "down" if their flows were already
> @@ -305,6 +342,7 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr,
> const char *iface_id)
>          /* Not yet fully installed interfaces can be safely deleted. */
>          ovs_iface_destroy(mgr, iface);
>          break;
> +    case OIF_REM_OLD_OVN_INST:
>      case OIF_MARK_UP:
>      case OIF_INSTALLED:
>          /* Properly mark interfaces "down" if their flows were already
> @@ -346,12 +384,33 @@ if_status_handle_claims(struct if_status_mgr *mgr,
>      return rc;
>  }
>
> +static void
> +clean_ovn_installed(struct if_status_mgr *mgr,
> +                    const struct ovsrec_interface_table *iface_table)
> +{
> +    struct shash_node *node;
> +
> +    SHASH_FOR_EACH_SAFE (node, &mgr->ovn_uninstall_hash) {
> +        const struct uuid *iface_uuid = node->data;
> +        remove_ovn_installed_for_uuid(iface_table, iface_uuid);
> +        free(node->data);
> +        char *node_name = shash_steal(&mgr->ovn_uninstall_hash, node);
> +        ovn_uninstall_hash_account_mem(node_name, true);
> +        free(node_name);
> +    }
> +}
> +
>  void
>  if_status_mgr_update(struct if_status_mgr *mgr,
>                       struct local_binding_data *binding_data,
>                       const struct sbrec_chassis *chassis_rec,
> +                     const struct ovsrec_interface_table *iface_table,
> +                     bool ovs_readonly,
>                       bool sb_readonly)
>  {
> +    if (!ovs_readonly) {
> +        clean_ovn_installed(mgr, iface_table);
> +    }
>      if (!binding_data) {
>          return;
>      }
> @@ -359,6 +418,17 @@ if_status_mgr_update(struct if_status_mgr *mgr,
>      struct shash *bindings = &binding_data->bindings;
>      struct hmapx_node *node;
>
> +    /* Move all interfaces that have been confirmed without ovn-installed,
> +     * from OIF_REM_OLD_OVN_INST to OIF_MARK_UP.
> +     */
> +    HMAPX_FOR_EACH_SAFE (node,
> &mgr->ifaces_per_state[OIF_REM_OLD_OVN_INST]) {
> +        struct ovs_iface *iface = node->data;
> +
> +        if (!local_binding_is_ovn_installed(bindings, iface->id)) {
> +            ovs_iface_set_state(mgr, iface, OIF_MARK_UP);
> +        }
> +    }
> +
>      /* 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.
> @@ -450,10 +520,23 @@ if_status_mgr_update(struct if_status_mgr *mgr,
>      }
>  }
>
> +void
> +if_status_mgr_remove_ovn_installed(struct if_status_mgr *mgr,
> +                                   const char *name,
> +                                   const struct uuid *uuid)
> +{
> +    VLOG_DBG("Adding %s to list of interfaces for which to remove "
> +              "ovn-installed", name);
> +    if (!shash_find_data(&mgr->ovn_uninstall_hash, name)) {
> +        add_to_ovn_uninstall_hash(mgr, name, uuid);
> +    }
> +}
> +
>  void
>  if_status_mgr_run(struct if_status_mgr *mgr,
>                    struct local_binding_data *binding_data,
>                    const struct sbrec_chassis *chassis_rec,
> +                  const struct ovsrec_interface_table *iface_table,
>                    bool sb_readonly, bool ovs_readonly)
>  {
>      struct ofctrl_acked_seqnos *acked_seqnos =
> @@ -471,12 +554,25 @@ if_status_mgr_run(struct if_status_mgr *mgr,
>                                            iface->install_seqno)) {
>              continue;
>          }
> -        ovs_iface_set_state(mgr, iface, OIF_MARK_UP);
> +        /* Wait for ovn-installed to be absent before moving to MARK_UP
> state.
> +         * Most of the times ovn-installed is already absent and hence we
> will
> +         * not have to wait.
> +         * If there is no binding_data, we can't determine if
> ovn-installed is
> +         * present or not; hence also go to the OIF_REM_OLD_OVN_INST
> state.
> +         */
> +        if (!binding_data ||
> +            local_binding_is_ovn_installed(&binding_data->bindings,
> +                                           iface->id)) {
> +            ovs_iface_set_state(mgr, iface, OIF_REM_OLD_OVN_INST);
> +        } else {
> +            ovs_iface_set_state(mgr, iface, OIF_MARK_UP);
> +        }
>      }
>      ofctrl_acked_seqnos_destroy(acked_seqnos);
>
>      /* Update binding states. */
>      if_status_mgr_update_bindings(mgr, binding_data, chassis_rec,
> +                                  iface_table,
>                                    sb_readonly, ovs_readonly);
>  }
>
> @@ -492,6 +588,18 @@ ovs_iface_account_mem(const char *iface_id, bool
> erase)
>      }
>  }
>
> +static void
> +ovn_uninstall_hash_account_mem(const char *name, bool erase)
> +{
> +    uint32_t size = (strlen(name) + sizeof(struct uuid) +
> +                     sizeof(struct shash_node));
> +    if (erase) {
> +        ifaces_usage -= size;
> +    } else {
> +        ifaces_usage += size;
> +    }
> +}
> +
>  static struct ovs_iface *
>  ovs_iface_create(struct if_status_mgr *mgr, const char *iface_id,
>                   enum if_state state)
> @@ -506,6 +614,16 @@ ovs_iface_create(struct if_status_mgr *mgr, const
> char *iface_id,
>      return iface;
>  }
>
> +static void
> +add_to_ovn_uninstall_hash(struct if_status_mgr *mgr, const char *name,
> +                          const struct uuid *uuid)
> +{
> +    struct uuid *new_uuid = xzalloc(sizeof *new_uuid);
> +    memcpy(new_uuid, uuid, sizeof(*new_uuid));
> +    shash_add(&mgr->ovn_uninstall_hash, name, new_uuid);
> +    ovn_uninstall_hash_account_mem(name, false);
> +}
> +
>  static void
>  ovs_iface_destroy(struct if_status_mgr *mgr, struct ovs_iface *iface)
>  {
> @@ -521,6 +639,23 @@ ovs_iface_destroy(struct if_status_mgr *mgr, struct
> ovs_iface *iface)
>      free(iface);
>  }
>
> +static void
> +ovn_uninstall_hash_destroy(struct if_status_mgr *mgr, char *name)
> +{
> +    struct shash_node *node = shash_find(&mgr->ovn_uninstall_hash, name);
> +    char *node_name = NULL;
> +    if (node) {
> +        free(node->data);
> +        VLOG_DBG("Interface name %s destroy", name);
> +        node_name = shash_steal(&mgr->ovn_uninstall_hash, node);
> +        ovn_uninstall_hash_account_mem(name, true);
> +        free(node_name);
> +    } else {
> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> +        VLOG_WARN_RL(&rl, "Interface name %s not found", name);
> +    }
> +}
> +
>  static void
>  ovs_iface_set_state(struct if_status_mgr *mgr, struct ovs_iface *iface,
>                      enum if_state state)
> @@ -539,6 +674,7 @@ static void
>  if_status_mgr_update_bindings(struct if_status_mgr *mgr,
>                                struct local_binding_data *binding_data,
>                                const struct sbrec_chassis *chassis_rec,
> +                              const struct ovsrec_interface_table
> *iface_table,
>                                bool sb_readonly, bool ovs_readonly)
>  {
>      if (!binding_data) {
> @@ -558,7 +694,17 @@ if_status_mgr_update_bindings(struct if_status_mgr
> *mgr,
>                                 sb_readonly, ovs_readonly);
>      }
>
> -    /* Notifiy the binding module to set "up" all bindings that have had
> +    /* Notify the binding module to remove "ovn-installed" for all
> bindings
> +     * in the OIF_REM_OLD_OVN_INST state.
> +     */
> +    HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_REM_OLD_OVN_INST]) {
> +        struct ovs_iface *iface = node->data;
> +
> +        local_binding_remove_ovn_installed(bindings, iface_table,
> iface->id,
> +                                           ovs_readonly);
> +    }
> +
> +    /* Notify the binding module to set "up" all bindings that have had
>       * their flows installed but are not yet marked "up" in the binding
>       * module.
>       */
> diff --git a/controller/if-status.h b/controller/if-status.h
> index 5bd187a25..203764946 100644
> --- a/controller/if-status.h
> +++ b/controller/if-status.h
> @@ -17,6 +17,7 @@
>  #define IF_STATUS_H 1
>
>  #include "openvswitch/shash.h"
> +#include "lib/vswitch-idl.h"
>
>  #include "binding.h"
>
> @@ -35,9 +36,12 @@ 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 *,
>                            const struct sbrec_chassis *chassis,
> +                          const struct ovsrec_interface_table
> *iface_table,
> +                          bool ovs_readonly,
>                            bool sb_readonly);
>  void if_status_mgr_run(struct if_status_mgr *mgr, struct
> local_binding_data *,
>                         const struct sbrec_chassis *,
> +                       const struct ovsrec_interface_table *iface_table,
>                         bool sb_readonly, bool ovs_readonly);
>  void if_status_mgr_get_memory_usage(struct if_status_mgr *mgr,
>                                      struct simap *usage);
> @@ -48,5 +52,8 @@ bool if_status_handle_claims(struct if_status_mgr *mgr,
>                               const struct sbrec_chassis *chassis_rec,
>                               struct hmap *tracked_datapath,
>                               bool sb_readonly);
> +void if_status_mgr_remove_ovn_installed(struct if_status_mgr *mgr,
> +                                        const char *name,
> +                                        const struct uuid *uuid);
>
>  # endif /* controller/if-status.h */
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index c094cb74d..2d8fee4d6 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -5225,6 +5225,9 @@ main(int argc, char *argv[])
>                      stopwatch_start(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME,
>                                      time_msec());
>                      if_status_mgr_update(if_mgr, binding_data, chassis,
> +                                         ovsrec_interface_table_get(
> +                                                    ovs_idl_loop.idl),
> +                                         !ovs_idl_txn,
>                                           !ovnsb_idl_txn);
>                      stopwatch_stop(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME,
>                                     time_msec());
> @@ -5254,6 +5257,8 @@ main(int argc, char *argv[])
>                      stopwatch_start(IF_STATUS_MGR_RUN_STOPWATCH_NAME,
>                                      time_msec());
>                      if_status_mgr_run(if_mgr, binding_data, chassis,
> +                                      ovsrec_interface_table_get(
> +                                                  ovs_idl_loop.idl),
>                                        !ovnsb_idl_txn, !ovs_idl_txn);
>                      stopwatch_stop(IF_STATUS_MGR_RUN_STOPWATCH_NAME,
>                                     time_msec());
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index b46f67636..cae44edee 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -10667,3 +10667,352 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query
> port patch-.*/d
>  /connection dropped.*/d"])
>  AT_CLEANUP
>  ])
> +
> +# This tests port->up/down and ovn-installed after adding and removing
> Ports and Interfaces.
> +# 3 Conditions x 3 tests:
> +# - 3 Conditions:
> +#   - In normal conditions
> +#   - Remove interface while starting and stopping SB and Controller
> +#   - Remove and add back interface while starting and stopping SB and
> Controller
> +# - 3 tests:
> +#   - Add/Remove Logical Port
> +#   - Add/Remove iface-id
> +#   - Add/Remove Interface
> +# Each tests/conditions checks for
> +# - Port_binding->chassis
> +# - Port up or down
> +# - ovn-installed
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn-install on slow ovsdb])
> +AT_KEYWORDS([ovn-install])
> +
> +OVS_TRAFFIC_VSWITCHD_START()
> +# Restart ovsdb-server, this time with tcp
> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> +start_daemon ovsdb-server --remote=punix:"$OVS_RUNDIR"/db.sock
> --remote=ptcp:0:127.0.0.1
> +
> +ovn_start
> +ADD_BR([br-int])
> +
> +# Set external-ids in br-int needed for ovn-controller
> +check ovs-vsctl \
> +        -- set Open_vSwitch . external-ids:system-id=hv1 \
> +        -- set Open_vSwitch .
> external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
> +        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
> +        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
> +        -- set bridge br-int fail-mode=secure
> other-config:disable-in-band=true
> +
> +# Start ovn-controller
> +PARSE_LISTENING_PORT([$ovs_base/ovsdb-server.log], [TCP_PORT])
> +start_daemon ovn-controller tcp:127.0.0.1:$TCP_PORT
> +
> +check ovn-nbctl ls-add ls1
> +check ovn-nbctl set Logical_Switch ls1 other_config:subnet=10.1.0.0/16
> +
> +check ovn-nbctl --wait=hv sync
> +
> +add_logical_ports() {
> +  echo Adding logical ports
> +  check ovn-nbctl lsp-add ls1 lsp1
> +  check ovn-nbctl lsp-add ls1 lsp2
> +}
> +
> +remove_logical_ports() {
> +  echo Removing logical ports
> +  check ovn-nbctl lsp-del lsp1
> +  check ovn-nbctl lsp-del lsp2
> +}
> +
> +add_ovs_interface() {
> +  echo Adding interface $1 $2
> +  ovs-vsctl --no-wait -- add-port br-int $1 \
> +                      -- set Interface $1 external_ids:iface-id=$2 \
> +                      -- set Interface $1 type=internal
> +}
> +add_ovs_interfaces() {
> +  add_ovs_interface vif1 lsp1
> +  add_ovs_interface vif2 lsp2
> +}
> +remove_ovs_interface() {
> +  echo Removing interface $1
> +  check ovs-vsctl --no-wait -- del-port $1
> +}
> +remove_ovs_interfaces() {
> +  remove_ovs_interface vif1
> +  remove_ovs_interface vif2
> +}
> +add_iface_ids() {
> +  echo Adding iface-id vif1 lsp1
> +  ovs-vsctl --no-wait -- set Interface vif1 external_ids:iface-id=lsp1
> +  echo Adding iface-id vif2 lsp2
> +  ovs-vsctl --no-wait -- set Interface vif2 external_ids:iface-id=lsp2
> +}
> +remove_iface_id() {
> +  echo Removing iface-id $1
> +  check ovs-vsctl remove Interface $1 external_ids iface-id
> +}
> +remove_iface_ids() {
> +  remove_iface_id vif1
> +  remove_iface_id vif2
> +}
> +wait_for_local_bindings() {
> +  OVS_WAIT_UNTIL(
> +      [test `ovs-appctl -t ovn-controller debug/dump-local-bindings |
> grep interface | wc -l` -eq 2],
> +      [kill -CONT $(cat ovn-sb/ovsdb-server.pid)]
> +  )
> +}
> +sleep_sb() {
> +  echo SB going to sleep
> +  AT_CHECK([kill -STOP $(cat ovn-sb/ovsdb-server.pid)])
> +}
> +wake_up_sb() {
> +  echo SB waking up
> +  AT_CHECK([kill -CONT $(cat ovn-sb/ovsdb-server.pid)])
> +}
> +sleep_controller() {
> +  echo Controller going to sleep
> +  ovn-appctl debug/pause
> +  OVS_WAIT_UNTIL([test x$(ovn-appctl -t ovn-controller debug/status) =
> "xpaused"])
> +}
> +
> +stop_ovsdb_controller_updates() {
> +  TCP_PORT=$1
> +  echo Stopping updates from ovn-controller to ovsdb using port $TCP_PORT
> +  on_exit 'iptables -C INPUT -p tcp --destination-port $TCP_PORT -j DROP
> 2>/dev/null && iptables -D INPUT -p tcp --destination-port $TCP_PORT -j
> DROP'
> +  iptables -A INPUT -p tcp --destination-port $TCP_PORT -j DROP
> +}
> +restart_ovsdb_controller_updates() {
> +  TCP_PORT=$1
> +  echo Restarting updates from ovn-controller to ovsdb
> +  iptables -D INPUT -p tcp --destination-port $TCP_PORT  -j DROP
> +}
> +wake_up_controller() {
> +  echo Controller waking up
> +  ovn-appctl debug/resume
> +}
> +ensure_controller_run() {
> +# We want to make sure controller could run at least one full loop.
> +# We can't use wait=hv as sb might be sleeping.
> +# Use 2 ovn-appctl to guarentee that ovn-controller run the full loop,
> and not just the unixctl handling
> +  OVS_WAIT_UNTIL([test x$(ovn-appctl -t ovn-controller debug/status) =
> "xrunning"])
> +  OVS_WAIT_UNTIL([test x$(ovn-appctl -t ovn-controller debug/status) =
> "xrunning"])
> +}
> +sleep_ovsdb() {
> +  echo OVSDB going to sleep
> +  AT_CHECK([kill -STOP $(cat ovsdb-server.pid)])
> +}
> +wake_up_ovsdb() {
> +  echo OVSDB waking up
> +  AT_CHECK([kill -CONT $(cat ovsdb-server.pid)])
> +}
> +check_ovn_installed() {
> +  OVS_WAIT_UNTIL([test `ovs-vsctl get Interface vif1
> external_ids:ovn-installed` = '"true"'])
> +  OVS_WAIT_UNTIL([test `ovs-vsctl get Interface vif2
> external_ids:ovn-installed` = '"true"'])
> +}
> +check_ovn_uninstalled() {
> +  OVS_WAIT_UNTIL([test x`ovs-vsctl get Interface vif2
> external_ids:ovn-installed` = x])
> +  OVS_WAIT_UNTIL([test x`ovs-vsctl get Interface vif1
> external_ids:ovn-installed` = x])
> +}
> +check_ports_up() {
> +  OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp1 up` = 'true'])
> +  OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp2 up` = 'true'])
> +}
> +check_ports_down() {
> +  OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp1 up` = 'false'])
> +  OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp2 up` = 'false'])
> +}
> +
> +check_ports_bound() {
> +  ch=$(fetch_column Chassis _uuid name=hv1)
> +  wait_row_count Port_Binding 1 logical_port=lsp1 chassis=$ch
> +  wait_row_count Port_Binding 1 logical_port=lsp2 chassis=$ch
> +}
> +check_ports_unbound() {
> +  wait_column "" Port_Binding chassis logical_port=lsp1
> +  wait_column "" Port_Binding chassis logical_port=lsp2
> +}
> +add_logical_ports
> +add_ovs_interfaces
> +wait_for_local_bindings
> +wait_for_ports_up
> +check ovn-nbctl --wait=hv sync
> +############################################################
> +########## Remove interface while removing iface-id ########
> +############################################################
> +AS_BOX(["Remove interface while removing iface-id"])
> +stop_ovsdb_controller_updates $TCP_PORT
> +remove_iface_id vif1
> +ensure_controller_run
> +# OVSDB should be seen as ro now
> +remove_iface_id vif2
> +ensure_controller_run
> +# Controller delaying ovn-install removal for vif2 as ovsdb ro
> +sleep_controller
> +restart_ovsdb_controller_updates $TCP_PORT
> +remove_ovs_interface vif2
> +# vif2, for which we want to remove ovn-install, is deleted
> +wake_up_controller
> +check_ovn_uninstalled
> +check_ports_down
> +check_ports_unbound
> +add_ovs_interface vif2 lsp2
> +add_iface_ids
> +check_ovn_installed
> +check_ports_up
> +check_ports_bound
> +############################################################
> +################### Add/Remove iface-id ####################
> +############################################################
> +AS_BOX(["iface-id removal and added back (no sleeping sb or controller)"])
> +remove_iface_ids
> +check_ovn_uninstalled
> +check_ports_down
> +check_ports_unbound
> +add_iface_ids
> +check_ovn_installed
> +check_ports_up
> +check_ports_bound
> +
> +AS_BOX(["iface-id removal"])
> +sleep_sb
> +remove_iface_ids
> +ensure_controller_run
> +sleep_controller
> +wake_up_sb
> +wake_up_controller
> +check_ovn_uninstalled
> +# Port_down not always set on iface-id removal
> +# check_ports_down
> +# Port_Binding(chassis) not always removed on iface-id removal
> +# check_ports_unbound
> +add_iface_ids
> +check ovn-nbctl --wait=hv sync
> +
> +AS_BOX(["iface-id removal 2"])
> +# Block IDL from ovn-controller to OVSDB
> +stop_ovsdb_controller_updates $TCP_PORT
> +remove_iface_id vif2
> +ensure_controller_run
> +
> +# OVSDB should now be seen as read-only by ovn-controller
> +remove_iface_id vif1
> +ensure_controller_run
> +
> +# Restart connection from ovn-controller to OVSDB
> +restart_ovsdb_controller_updates $TCP_PORT
> +check_ovn_uninstalled
> +check_ports_down
> +check_ports_unbound
> +
> +add_iface_ids
> +check ovn-nbctl --wait=hv sync
> +
> +AS_BOX(["iface-id removal and added back"])
> +sleep_sb
> +remove_iface_ids
> +ensure_controller_run
> +sleep_controller
> +add_iface_ids
> +wake_up_sb
> +wake_up_controller
> +check_ovn_installed
> +check_ports_up
> +check_ports_bound
> +############################################################
> +###################### Add/Remove Interface ################
> +############################################################
> +AS_BOX(["Interface removal and added back (no sleeping sb or
> controller)"])
> +remove_ovs_interfaces
> +check_ovn_uninstalled
> +check_ports_down
> +check_ports_unbound
> +add_ovs_interfaces
> +check_ovn_installed
> +check_ports_up
> +check_ports_bound
> +check ovn-nbctl --wait=hv sync
> +
> +AS_BOX(["Interface removal"])
> +sleep_sb
> +remove_ovs_interfaces
> +ensure_controller_run
> +sleep_controller
> +wake_up_sb
> +wake_up_controller
> +check_ovn_uninstalled
> +# Port_down not always set on Interface removal
> +# check_ports_down
> +# Port_Binding(chassis) not always removed on Interface removal
> +# check_ports_unbound
> +add_ovs_interfaces
> +check ovn-nbctl --wait=hv sync
> +
> +AS_BOX(["Interface removal and added back"])
> +sleep_sb
> +remove_ovs_interfaces
> +ensure_controller_run
> +sleep_controller
> +add_ovs_interfaces
> +wake_up_sb
> +wake_up_controller
> +check_ovn_installed
> +check_ports_up
> +check_ports_bound
> +check ovn-nbctl --wait=hv sync
> +############################################################
> +###################### Add/Remove Logical Port #############
> +############################################################
> +AS_BOX(["Logical port removal and added back (no sleeping sb or
> controller)"])
> +remove_logical_ports
> +check_ovn_uninstalled
> +check_ports_unbound
> +sleep_ovsdb
> +add_logical_ports
> +ensure_controller_run
> +wake_up_ovsdb
> +check_ovn_installed
> +check_ports_up
> +check_ports_bound
> +check ovn-nbctl --wait=hv sync
> +
> +AS_BOX(["Logical port removal"])
> +sleep_sb
> +remove_logical_ports
> +ensure_controller_run
> +sleep_controller
> +wake_up_sb
> +wake_up_controller
> +check_ovn_uninstalled
> +check_ports_unbound
> +add_logical_ports
> +check ovn-nbctl --wait=hv sync
> +
> +AS_BOX(["Logical port removal and added back"])
> +sleep_sb
> +remove_logical_ports
> +ensure_controller_run
> +sleep_controller
> +add_logical_ports
> +wake_up_sb
> +wake_up_controller
> +check_ovn_installed
> +check_ports_up
> +check_ports_bound
> +
> +OVS_APP_EXIT_AND_WAIT([ovn-controller])
> +
> +as ovn-sb
> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> +
> +as ovn-nb
> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> +
> +as northd
> +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE])
> +
> +as
> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
> +/connection dropped.*/d"])
> +AT_CLEANUP
> +])
> +
> --
> 2.31.1
>
>
Looks good to me, thanks.

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

On 5/5/23 11:21, Ales Musil wrote:
> On Fri, Apr 28, 2023 at 1:59 PM Xavier Simonart <xsimonar@redhat.com> wrote:
> 
>> OVN checks whether ovn-installed is already present (in OVS) before
>> updating it.
>> This might cause ovn-installed related issues in the following case:
>> - (1) ovn-installed is present
>> - (2) we claim the interface
>> - (3) we update ovs, removing ovn-installed and start installing flows
>> - (4) (next loop), after flows installed, we check if ovn-installed is
>> absent,
>>   and if yes, we update OVS with ovn-installed.
>> However, in step4, if OVS is still busy from step 3, ovn-installed is read
>> as
>> present; hence OVN controller does not update it and move to INSTALLED
>> state.
>>
>> ovn-installed was also not properly deleted on port or binding removal.
>>
>> Note that port->up is not properly removed on external_ids:iface-id
>> removal when
>> sb is read-only. This will be fixed in a further patch.
>>
>> Fixes: a7c7d4519e50 ("controller: avoid recomputes triggered by SBDB
>> Port_Binding updates.")
>>
>> Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
>>
>> ---
>> v2: - handled Dumitru's comments
>>     - rebased on main
>>     - updated state machine diagram as commented in v1 commit message
>>     - remove ovn-installed on port deletion or external_ids:iface-id
>> removal.
>>     - added test case
>> v3: - handled more comment from Dumitru
>>     - fixed ovn-install not removed when ovs is read-only
>>     - moved test case from unit (ovn.at) to system (system-ovn).
>>     - test case connects to OVS db through tcp and uses iptables to
>> momentarly block the idl update
>>       to simulate read-only ovsdb
>> v4: - handled comments from Ales: avoid using is_deleted outside of
>> tracked loops.
>> ---
>>  controller/binding.c        |  68 ++++++-
>>  controller/binding.h        |  11 ++
>>  controller/if-status.c      | 204 ++++++++++++++++++---
>>  controller/if-status.h      |   7 +
>>  controller/ovn-controller.c |   5 +
>>  tests/system-ovn.at         | 349 ++++++++++++++++++++++++++++++++++++
>>  6 files changed, 612 insertions(+), 32 deletions(-)
>>
>> diff --git a/controller/binding.c b/controller/binding.c
>> index 5df62baef..357e77609 100644
>> --- a/controller/binding.c
>> +++ b/controller/binding.c
>> @@ -746,6 +746,19 @@ local_binding_get_lport_ofport(const struct shash
>> *local_bindings,
>>              u16_to_ofp(lbinding->iface->ofport[0]) : 0;
>>  }
>>
>> +bool
>> +local_binding_is_ovn_installed(struct shash *local_bindings,
>> +                               const char *pb_name)
>> +{
>> +    struct local_binding *lbinding =
>> +        local_binding_find(local_bindings, pb_name);
>> +    if (lbinding && lbinding->iface) {
>> +        return smap_get_bool(&lbinding->iface->external_ids,
>> +                             OVN_INSTALLED_EXT_ID, false);
>> +    }
>> +    return false;
>> +}
>> +
>>  bool
>>  local_binding_is_up(struct shash *local_bindings, const char *pb_name,
>>                      const struct sbrec_chassis *chassis_rec)
>> @@ -783,6 +796,7 @@ local_binding_is_down(struct shash *local_bindings,
>> const char *pb_name,
>>          } else if (b_lport->pb->chassis) {
>>              VLOG_DBG("lport %s already claimed by other chassis",
>>                       b_lport->pb->logical_port);
>> +            return true;
>>          }
>>      }
>>
>> @@ -834,6 +848,38 @@ local_binding_set_up(struct shash *local_bindings,
>> const char *pb_name,
>>      }
>>  }
>>
>> +void
>> +local_binding_remove_ovn_installed(
>> +        struct shash *local_bindings,
>> +        const struct ovsrec_interface_table *iface_table,
>> +        const char *pb_name, bool ovs_readonly)
>> +{
>> +    if (ovs_readonly) {
>> +        return;
>> +    }
>> +    struct local_binding *lbinding =
>> +        local_binding_find(local_bindings, pb_name);
>> +    if (lbinding && lbinding->iface) {
>> +        const struct uuid *iface_uuid = &lbinding->iface->header_.uuid;
>> +        remove_ovn_installed_for_uuid(iface_table, iface_uuid);
>> +    }
>> +}
>> +
>> +void
>> +remove_ovn_installed_for_uuid(const struct ovsrec_interface_table
>> *iface_table,
>> +                              const struct uuid *iface_uuid)
>> +{
>> +    const struct ovsrec_interface *iface_rec =
>> +        ovsrec_interface_table_get_for_uuid(iface_table, iface_uuid);
>> +    if (iface_rec && smap_get_bool(&iface_rec->external_ids,
>> +                                   OVN_INSTALLED_EXT_ID, false)) {
>> +        VLOG_INFO("Removing iface %s ovn-installed in OVS",
>> +                  iface_rec->name);
>> +        ovsrec_interface_update_external_ids_delkey(iface_rec,
>> +                                                    OVN_INSTALLED_EXT_ID);
>> +    }
>> +}
>> +
>>  void
>>  local_binding_set_down(struct shash *local_bindings, const char *pb_name,
>>                         const struct sbrec_chassis *chassis_rec,
>> @@ -1239,7 +1285,9 @@ claim_lport(const struct sbrec_port_binding *pb,
>>                      return false;
>>                  }
>>              } else {
>> -                if (pb->n_up && !pb->up[0]) {
>> +                if ((pb->n_up && !pb->up[0]) ||
>> +                    !smap_get_bool(&iface_rec->external_ids,
>> +                                   OVN_INSTALLED_EXT_ID, false)) {
>>                      if_status_mgr_claim_iface(if_mgr, pb, chassis_rec,
>>                                                sb_readonly);
>>                  }
>> @@ -1464,9 +1512,11 @@ consider_vif_lport_(const struct sbrec_port_binding
>> *pb,
>>              const char *requested_chassis_option = smap_get(
>>                  &pb->options, "requested-chassis");
>>              VLOG_INFO_RL(&rl,
>> -                "Not claiming lport %s, chassis %s requested-chassis %s",
>> +                "Not claiming lport %s, chassis %s requested-chassis %s "
>> +                "pb->chassis %s",
>>                  pb->logical_port, b_ctx_in->chassis_rec->name,
>> -                requested_chassis_option ? requested_chassis_option :
>> "[]");
>> +                requested_chassis_option ? requested_chassis_option :
>> "[]",
>> +                pb->chassis ? pb->chassis->name: "");
>>          }
>>      }
>>
>> @@ -2288,6 +2338,11 @@ consider_iface_release(const struct
>> ovsrec_interface *iface_rec,
>>                  return false;
>>              }
>>          }
>> +        if (lbinding->iface && lbinding->iface->name) {
>> +            if_status_mgr_remove_ovn_installed(b_ctx_out->if_mgr,
>> +                                               lbinding->iface->name,
>> +
>>  &lbinding->iface->header_.uuid);
>> +        }
>>
>>      } else if (lbinding && b_lport && b_lport->type == LP_LOCALPORT) {
>>          /* lbinding is associated with a localport.  Remove it from the
>> @@ -2558,6 +2613,7 @@ handle_deleted_lport(const struct sbrec_port_binding
>> *pb,
>>      if (ld) {
>>          remove_pb_from_local_datapath(pb,
>>                                        b_ctx_out, ld);
>> +        if_status_mgr_release_iface(b_ctx_out->if_mgr, pb->logical_port);
>>          return;
>>      }
>>
>> @@ -2581,6 +2637,7 @@ handle_deleted_lport(const struct sbrec_port_binding
>> *pb,
>>              remove_pb_from_local_datapath(pb, b_ctx_out,
>>                                            ld);
>>          }
>> +        if_status_mgr_release_iface(b_ctx_out->if_mgr, pb->logical_port);
>>      }
>>  }
>>
>> @@ -2627,6 +2684,11 @@ handle_deleted_vif_lport(const struct
>> sbrec_port_binding *pb,
>>      }
>>
>>      handle_deleted_lport(pb, b_ctx_in, b_ctx_out);
>> +    if (lbinding && lbinding->iface && lbinding->iface->name) {
>> +        if_status_mgr_remove_ovn_installed(b_ctx_out->if_mgr,
>> +                                           lbinding->iface->name,
>> +
>>  &lbinding->iface->header_.uuid);
>> +    }
>>      return true;
>>  }
>>
>> diff --git a/controller/binding.h b/controller/binding.h
>> index 6c3a98b02..0b9c6994f 100644
>> --- a/controller/binding.h
>> +++ b/controller/binding.h
>> @@ -159,6 +159,14 @@ 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,
>>                             const struct sbrec_chassis *);
>>
>> +bool local_binding_is_ovn_installed(struct shash *local_bindings,
>> +                                    const char *pb_name);
>> +void local_binding_remove_ovn_installed(
>> +        struct shash *local_bindings,
>> +        const struct ovsrec_interface_table *iface_table,
>> +        const char *pb_name,
>> +        bool ovs_readonly);
>> +
>>  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,
>> @@ -195,6 +203,9 @@ void set_pb_chassis_in_sbrec(const struct
>> sbrec_port_binding *pb,
>>                               const struct sbrec_chassis *chassis_rec,
>>                               bool is_set);
>>
>> +void remove_ovn_installed_for_uuid(const struct ovsrec_interface_table *,
>> +                                   const struct uuid *);
>> +
>>  /* 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 d1c14ac30..ac36a9fb9 100644
>> --- a/controller/if-status.c
>> +++ b/controller/if-status.c
>> @@ -54,32 +54,37 @@ VLOG_DEFINE_THIS_MODULE(if_status);
>>   */
>>
>>  enum if_state {
>> -    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
>> -                        * SB and OVS databases).
>> -                        */
>> -    OIF_MARK_DOWN,     /* Released interface but not yet marked "down" in
>> the
>> -                        * binding module (in SB and/or OVS databases).
>> -                        */
>> -    OIF_INSTALLED,     /* Interface flows programmed in OVS and binding
>> marked
>> -                        * "up" in the binding module.
>> -                        */
>> +    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_REM_OLD_OVN_INST, /* Interface with flows successfully installed
>> in OVS
>> +                           * but with ovn-installed still in OVSDB.
>> +                           */
>> +    OIF_MARK_UP,          /* Interface with flows successfully installed
>> in OVS
>> +                           * but not yet marked "up" in the binding
>> module (in
>> +                           * SB and OVS databases).
>> +                           */
>> +    OIF_MARK_DOWN,        /* Released interface but not yet marked "down"
>> in
>> +                           * the binding module (in SB and/or OVS
>> databases).
>> +                           */
>> +    OIF_INSTALLED,        /* Interface flows programmed in OVS and binding
>> +                           * marked "up" in the binding module.
>> +                           */
>>      OIF_MAX,
>>  };
>>
>>  static const char *if_state_names[] = {
>> -    [OIF_CLAIMED]       = "CLAIMED",
>> -    [OIF_INSTALL_FLOWS] = "INSTALL_FLOWS",
>> -    [OIF_MARK_UP]       = "MARK_UP",
>> -    [OIF_MARK_DOWN]     = "MARK_DOWN",
>> -    [OIF_INSTALLED]     = "INSTALLED",
>> +    [OIF_CLAIMED]          = "CLAIMED",
>> +    [OIF_INSTALL_FLOWS]    = "INSTALL_FLOWS",
>> +    [OIF_REM_OLD_OVN_INST] = "REM_OLD_OVN_INST",
>> +    [OIF_MARK_UP]          = "MARK_UP",
>> +    [OIF_MARK_DOWN]        = "MARK_DOWN",
>> +    [OIF_INSTALLED]        = "INSTALLED",
>>  };
>>
>>  /*
>> @@ -109,11 +114,26 @@ static const char *if_state_names[] = {
>>   * |     |                      |   - 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(seqno rcvd, ovn-installed present)          |
>> | | |
>> + * |        V                                                     |
>> | | |
>> + * |    +--------------------+                                    |
>> | | |
>> + * |    |                    |  mgr_run()                         |
>> | | |
>> + * +--- | REM_OLD_OVN_INST   |  - remove ovn-installed in ovs     |
>> | | |
>> + * |    +--------------------+                                    |
>> | | |
>> + * |               |                                              |
>> | | |
>> + * |               |                                              |
>> | | |
>> + * |               | mgr_update( ovn_installed not present)       |
>> | | |
>> + * |               |                                              |
>> | | |
>> + * |               |  +-------------------------------------------+
>> | | |
>> + * |               |  |
>> | | |
>> + * |               |  |  mgr_run(seqno rcvd, ovn-installed not present)
>> | | |
>> + * |               |  |  - set port up in sb
>>  | | |
>> + * |               |  |  - set ovn-installed in ovs
>> | | |
>> + * |release_iface  |  |
>> | | |
>> + * |               V  V
>> | | |
>>   * |   +----------------------+
>> | | |
>>   * |   |                      |  mgr_run()
>>  | | |
>>   * +-- |       MARK_UP        |  - set port up in sb
>>  | | |
>> @@ -155,6 +175,9 @@ struct if_status_mgr {
>>      /* All local interfaces, mapping from 'iface-id' to 'struct
>> ovs_iface'. */
>>      struct shash ifaces;
>>
>> +    /* local interfaces which need ovn-install removal */
>> +    struct shash ovn_uninstall_hash;
>> +
>>      /* All local interfaces, stored per state. */
>>      struct hmapx ifaces_per_state[OIF_MAX];
>>
>> @@ -170,15 +193,20 @@ struct if_status_mgr {
>>  static struct ovs_iface *ovs_iface_create(struct if_status_mgr *,
>>                                            const char *iface_id,
>>                                            enum if_state );
>> +static void add_to_ovn_uninstall_hash(struct if_status_mgr *, const char
>> *,
>> +                                      const struct uuid *);
>>  static void ovs_iface_destroy(struct if_status_mgr *, struct ovs_iface *);
>> +static void ovn_uninstall_hash_destroy(struct if_status_mgr *mgr, char
>> *name);
>>  static void ovs_iface_set_state(struct if_status_mgr *, struct ovs_iface
>> *,
>>                                  enum if_state);
>>
>>  static void if_status_mgr_update_bindings(
>>      struct if_status_mgr *mgr, struct local_binding_data *binding_data,
>>      const struct sbrec_chassis *,
>> +    const struct ovsrec_interface_table *iface_table,
>>      bool sb_readonly, bool ovs_readonly);
>>
>> +static void ovn_uninstall_hash_account_mem(const char *name, bool erase);
>>  struct if_status_mgr *
>>  if_status_mgr_create(void)
>>  {
>> @@ -189,6 +217,7 @@ if_status_mgr_create(void)
>>          hmapx_init(&mgr->ifaces_per_state[i]);
>>      }
>>      shash_init(&mgr->ifaces);
>> +    shash_init(&mgr->ovn_uninstall_hash);
>>      return mgr;
>>  }
>>
>> @@ -202,6 +231,11 @@ if_status_mgr_clear(struct if_status_mgr *mgr)
>>      }
>>      ovs_assert(shash_is_empty(&mgr->ifaces));
>>
>> +    SHASH_FOR_EACH_SAFE (node, &mgr->ovn_uninstall_hash) {
>> +        ovn_uninstall_hash_destroy(mgr, node->data);
>> +    }
>> +    ovs_assert(shash_is_empty(&mgr->ovn_uninstall_hash));
>> +
>>      for (size_t i = 0; i < ARRAY_SIZE(mgr->ifaces_per_state); i++) {
>>          ovs_assert(hmapx_is_empty(&mgr->ifaces_per_state[i]));
>>      }
>> @@ -212,6 +246,7 @@ if_status_mgr_destroy(struct if_status_mgr *mgr)
>>  {
>>      if_status_mgr_clear(mgr);
>>      shash_destroy(&mgr->ifaces);
>> +    shash_destroy(&mgr->ovn_uninstall_hash);
>>      for (size_t i = 0; i < ARRAY_SIZE(mgr->ifaces_per_state); i++) {
>>          hmapx_destroy(&mgr->ifaces_per_state[i]);
>>      }
>> @@ -238,6 +273,7 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr,
>>      switch (iface->state) {
>>      case OIF_CLAIMED:
>>      case OIF_INSTALL_FLOWS:
>> +    case OIF_REM_OLD_OVN_INST:
>>      case OIF_MARK_UP:
>>          /* Nothing to do here. */
>>          break;
>> @@ -274,6 +310,7 @@ if_status_mgr_release_iface(struct if_status_mgr *mgr,
>> const char *iface_id)
>>          /* Not yet fully installed interfaces can be safely deleted. */
>>          ovs_iface_destroy(mgr, iface);
>>          break;
>> +    case OIF_REM_OLD_OVN_INST:
>>      case OIF_MARK_UP:
>>      case OIF_INSTALLED:
>>          /* Properly mark interfaces "down" if their flows were already
>> @@ -305,6 +342,7 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr,
>> const char *iface_id)
>>          /* Not yet fully installed interfaces can be safely deleted. */
>>          ovs_iface_destroy(mgr, iface);
>>          break;
>> +    case OIF_REM_OLD_OVN_INST:
>>      case OIF_MARK_UP:
>>      case OIF_INSTALLED:
>>          /* Properly mark interfaces "down" if their flows were already
>> @@ -346,12 +384,33 @@ if_status_handle_claims(struct if_status_mgr *mgr,
>>      return rc;
>>  }
>>
>> +static void
>> +clean_ovn_installed(struct if_status_mgr *mgr,
>> +                    const struct ovsrec_interface_table *iface_table)
>> +{
>> +    struct shash_node *node;
>> +
>> +    SHASH_FOR_EACH_SAFE (node, &mgr->ovn_uninstall_hash) {
>> +        const struct uuid *iface_uuid = node->data;
>> +        remove_ovn_installed_for_uuid(iface_table, iface_uuid);
>> +        free(node->data);
>> +        char *node_name = shash_steal(&mgr->ovn_uninstall_hash, node);
>> +        ovn_uninstall_hash_account_mem(node_name, true);
>> +        free(node_name);
>> +    }
>> +}
>> +
>>  void
>>  if_status_mgr_update(struct if_status_mgr *mgr,
>>                       struct local_binding_data *binding_data,
>>                       const struct sbrec_chassis *chassis_rec,
>> +                     const struct ovsrec_interface_table *iface_table,
>> +                     bool ovs_readonly,
>>                       bool sb_readonly)
>>  {
>> +    if (!ovs_readonly) {
>> +        clean_ovn_installed(mgr, iface_table);
>> +    }
>>      if (!binding_data) {
>>          return;
>>      }
>> @@ -359,6 +418,17 @@ if_status_mgr_update(struct if_status_mgr *mgr,
>>      struct shash *bindings = &binding_data->bindings;
>>      struct hmapx_node *node;
>>
>> +    /* Move all interfaces that have been confirmed without ovn-installed,
>> +     * from OIF_REM_OLD_OVN_INST to OIF_MARK_UP.
>> +     */
>> +    HMAPX_FOR_EACH_SAFE (node,
>> &mgr->ifaces_per_state[OIF_REM_OLD_OVN_INST]) {
>> +        struct ovs_iface *iface = node->data;
>> +
>> +        if (!local_binding_is_ovn_installed(bindings, iface->id)) {
>> +            ovs_iface_set_state(mgr, iface, OIF_MARK_UP);
>> +        }
>> +    }
>> +
>>      /* 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.
>> @@ -450,10 +520,23 @@ if_status_mgr_update(struct if_status_mgr *mgr,
>>      }
>>  }
>>
>> +void
>> +if_status_mgr_remove_ovn_installed(struct if_status_mgr *mgr,
>> +                                   const char *name,
>> +                                   const struct uuid *uuid)
>> +{
>> +    VLOG_DBG("Adding %s to list of interfaces for which to remove "
>> +              "ovn-installed", name);
>> +    if (!shash_find_data(&mgr->ovn_uninstall_hash, name)) {
>> +        add_to_ovn_uninstall_hash(mgr, name, uuid);
>> +    }
>> +}
>> +
>>  void
>>  if_status_mgr_run(struct if_status_mgr *mgr,
>>                    struct local_binding_data *binding_data,
>>                    const struct sbrec_chassis *chassis_rec,
>> +                  const struct ovsrec_interface_table *iface_table,
>>                    bool sb_readonly, bool ovs_readonly)
>>  {
>>      struct ofctrl_acked_seqnos *acked_seqnos =
>> @@ -471,12 +554,25 @@ if_status_mgr_run(struct if_status_mgr *mgr,
>>                                            iface->install_seqno)) {
>>              continue;
>>          }
>> -        ovs_iface_set_state(mgr, iface, OIF_MARK_UP);
>> +        /* Wait for ovn-installed to be absent before moving to MARK_UP
>> state.
>> +         * Most of the times ovn-installed is already absent and hence we
>> will
>> +         * not have to wait.
>> +         * If there is no binding_data, we can't determine if
>> ovn-installed is
>> +         * present or not; hence also go to the OIF_REM_OLD_OVN_INST
>> state.
>> +         */
>> +        if (!binding_data ||
>> +            local_binding_is_ovn_installed(&binding_data->bindings,
>> +                                           iface->id)) {
>> +            ovs_iface_set_state(mgr, iface, OIF_REM_OLD_OVN_INST);
>> +        } else {
>> +            ovs_iface_set_state(mgr, iface, OIF_MARK_UP);
>> +        }
>>      }
>>      ofctrl_acked_seqnos_destroy(acked_seqnos);
>>
>>      /* Update binding states. */
>>      if_status_mgr_update_bindings(mgr, binding_data, chassis_rec,
>> +                                  iface_table,
>>                                    sb_readonly, ovs_readonly);
>>  }
>>
>> @@ -492,6 +588,18 @@ ovs_iface_account_mem(const char *iface_id, bool
>> erase)
>>      }
>>  }
>>
>> +static void
>> +ovn_uninstall_hash_account_mem(const char *name, bool erase)
>> +{
>> +    uint32_t size = (strlen(name) + sizeof(struct uuid) +
>> +                     sizeof(struct shash_node));
>> +    if (erase) {
>> +        ifaces_usage -= size;
>> +    } else {
>> +        ifaces_usage += size;
>> +    }
>> +}
>> +
>>  static struct ovs_iface *
>>  ovs_iface_create(struct if_status_mgr *mgr, const char *iface_id,
>>                   enum if_state state)
>> @@ -506,6 +614,16 @@ ovs_iface_create(struct if_status_mgr *mgr, const
>> char *iface_id,
>>      return iface;
>>  }
>>
>> +static void
>> +add_to_ovn_uninstall_hash(struct if_status_mgr *mgr, const char *name,
>> +                          const struct uuid *uuid)
>> +{
>> +    struct uuid *new_uuid = xzalloc(sizeof *new_uuid);
>> +    memcpy(new_uuid, uuid, sizeof(*new_uuid));
>> +    shash_add(&mgr->ovn_uninstall_hash, name, new_uuid);
>> +    ovn_uninstall_hash_account_mem(name, false);
>> +}
>> +
>>  static void
>>  ovs_iface_destroy(struct if_status_mgr *mgr, struct ovs_iface *iface)
>>  {
>> @@ -521,6 +639,23 @@ ovs_iface_destroy(struct if_status_mgr *mgr, struct
>> ovs_iface *iface)
>>      free(iface);
>>  }
>>
>> +static void
>> +ovn_uninstall_hash_destroy(struct if_status_mgr *mgr, char *name)
>> +{
>> +    struct shash_node *node = shash_find(&mgr->ovn_uninstall_hash, name);
>> +    char *node_name = NULL;
>> +    if (node) {
>> +        free(node->data);
>> +        VLOG_DBG("Interface name %s destroy", name);
>> +        node_name = shash_steal(&mgr->ovn_uninstall_hash, node);
>> +        ovn_uninstall_hash_account_mem(name, true);
>> +        free(node_name);
>> +    } else {
>> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>> +        VLOG_WARN_RL(&rl, "Interface name %s not found", name);
>> +    }
>> +}
>> +
>>  static void
>>  ovs_iface_set_state(struct if_status_mgr *mgr, struct ovs_iface *iface,
>>                      enum if_state state)
>> @@ -539,6 +674,7 @@ static void
>>  if_status_mgr_update_bindings(struct if_status_mgr *mgr,
>>                                struct local_binding_data *binding_data,
>>                                const struct sbrec_chassis *chassis_rec,
>> +                              const struct ovsrec_interface_table
>> *iface_table,
>>                                bool sb_readonly, bool ovs_readonly)
>>  {
>>      if (!binding_data) {
>> @@ -558,7 +694,17 @@ if_status_mgr_update_bindings(struct if_status_mgr
>> *mgr,
>>                                 sb_readonly, ovs_readonly);
>>      }
>>
>> -    /* Notifiy the binding module to set "up" all bindings that have had
>> +    /* Notify the binding module to remove "ovn-installed" for all
>> bindings
>> +     * in the OIF_REM_OLD_OVN_INST state.
>> +     */
>> +    HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_REM_OLD_OVN_INST]) {
>> +        struct ovs_iface *iface = node->data;
>> +
>> +        local_binding_remove_ovn_installed(bindings, iface_table,
>> iface->id,
>> +                                           ovs_readonly);
>> +    }
>> +
>> +    /* Notify the binding module to set "up" all bindings that have had
>>       * their flows installed but are not yet marked "up" in the binding
>>       * module.
>>       */
>> diff --git a/controller/if-status.h b/controller/if-status.h
>> index 5bd187a25..203764946 100644
>> --- a/controller/if-status.h
>> +++ b/controller/if-status.h
>> @@ -17,6 +17,7 @@
>>  #define IF_STATUS_H 1
>>
>>  #include "openvswitch/shash.h"
>> +#include "lib/vswitch-idl.h"
>>
>>  #include "binding.h"
>>
>> @@ -35,9 +36,12 @@ 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 *,
>>                            const struct sbrec_chassis *chassis,
>> +                          const struct ovsrec_interface_table
>> *iface_table,
>> +                          bool ovs_readonly,
>>                            bool sb_readonly);
>>  void if_status_mgr_run(struct if_status_mgr *mgr, struct
>> local_binding_data *,
>>                         const struct sbrec_chassis *,
>> +                       const struct ovsrec_interface_table *iface_table,
>>                         bool sb_readonly, bool ovs_readonly);
>>  void if_status_mgr_get_memory_usage(struct if_status_mgr *mgr,
>>                                      struct simap *usage);
>> @@ -48,5 +52,8 @@ bool if_status_handle_claims(struct if_status_mgr *mgr,
>>                               const struct sbrec_chassis *chassis_rec,
>>                               struct hmap *tracked_datapath,
>>                               bool sb_readonly);
>> +void if_status_mgr_remove_ovn_installed(struct if_status_mgr *mgr,
>> +                                        const char *name,
>> +                                        const struct uuid *uuid);
>>
>>  # endif /* controller/if-status.h */
>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> index c094cb74d..2d8fee4d6 100644
>> --- a/controller/ovn-controller.c
>> +++ b/controller/ovn-controller.c
>> @@ -5225,6 +5225,9 @@ main(int argc, char *argv[])
>>                      stopwatch_start(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME,
>>                                      time_msec());
>>                      if_status_mgr_update(if_mgr, binding_data, chassis,
>> +                                         ovsrec_interface_table_get(
>> +                                                    ovs_idl_loop.idl),
>> +                                         !ovs_idl_txn,
>>                                           !ovnsb_idl_txn);
>>                      stopwatch_stop(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME,
>>                                     time_msec());
>> @@ -5254,6 +5257,8 @@ main(int argc, char *argv[])
>>                      stopwatch_start(IF_STATUS_MGR_RUN_STOPWATCH_NAME,
>>                                      time_msec());
>>                      if_status_mgr_run(if_mgr, binding_data, chassis,
>> +                                      ovsrec_interface_table_get(
>> +                                                  ovs_idl_loop.idl),
>>                                        !ovnsb_idl_txn, !ovs_idl_txn);
>>                      stopwatch_stop(IF_STATUS_MGR_RUN_STOPWATCH_NAME,
>>                                     time_msec());
>> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
>> index b46f67636..cae44edee 100644
>> --- a/tests/system-ovn.at
>> +++ b/tests/system-ovn.at
>> @@ -10667,3 +10667,352 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query
>> port patch-.*/d
>>  /connection dropped.*/d"])
>>  AT_CLEANUP
>>  ])
>> +
>> +# This tests port->up/down and ovn-installed after adding and removing
>> Ports and Interfaces.
>> +# 3 Conditions x 3 tests:
>> +# - 3 Conditions:
>> +#   - In normal conditions
>> +#   - Remove interface while starting and stopping SB and Controller
>> +#   - Remove and add back interface while starting and stopping SB and
>> Controller
>> +# - 3 tests:
>> +#   - Add/Remove Logical Port
>> +#   - Add/Remove iface-id
>> +#   - Add/Remove Interface
>> +# Each tests/conditions checks for
>> +# - Port_binding->chassis
>> +# - Port up or down
>> +# - ovn-installed
>> +OVN_FOR_EACH_NORTHD([
>> +AT_SETUP([ovn-install on slow ovsdb])
>> +AT_KEYWORDS([ovn-install])
>> +
>> +OVS_TRAFFIC_VSWITCHD_START()
>> +# Restart ovsdb-server, this time with tcp
>> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>> +start_daemon ovsdb-server --remote=punix:"$OVS_RUNDIR"/db.sock
>> --remote=ptcp:0:127.0.0.1
>> +
>> +ovn_start
>> +ADD_BR([br-int])
>> +
>> +# Set external-ids in br-int needed for ovn-controller
>> +check ovs-vsctl \
>> +        -- set Open_vSwitch . external-ids:system-id=hv1 \
>> +        -- set Open_vSwitch .
>> external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
>> +        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
>> +        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
>> +        -- set bridge br-int fail-mode=secure
>> other-config:disable-in-band=true
>> +
>> +# Start ovn-controller
>> +PARSE_LISTENING_PORT([$ovs_base/ovsdb-server.log], [TCP_PORT])
>> +start_daemon ovn-controller tcp:127.0.0.1:$TCP_PORT
>> +
>> +check ovn-nbctl ls-add ls1
>> +check ovn-nbctl set Logical_Switch ls1 other_config:subnet=10.1.0.0/16
>> +
>> +check ovn-nbctl --wait=hv sync
>> +
>> +add_logical_ports() {
>> +  echo Adding logical ports
>> +  check ovn-nbctl lsp-add ls1 lsp1
>> +  check ovn-nbctl lsp-add ls1 lsp2
>> +}
>> +
>> +remove_logical_ports() {
>> +  echo Removing logical ports
>> +  check ovn-nbctl lsp-del lsp1
>> +  check ovn-nbctl lsp-del lsp2
>> +}
>> +
>> +add_ovs_interface() {
>> +  echo Adding interface $1 $2
>> +  ovs-vsctl --no-wait -- add-port br-int $1 \
>> +                      -- set Interface $1 external_ids:iface-id=$2 \
>> +                      -- set Interface $1 type=internal
>> +}
>> +add_ovs_interfaces() {
>> +  add_ovs_interface vif1 lsp1
>> +  add_ovs_interface vif2 lsp2
>> +}
>> +remove_ovs_interface() {
>> +  echo Removing interface $1
>> +  check ovs-vsctl --no-wait -- del-port $1
>> +}
>> +remove_ovs_interfaces() {
>> +  remove_ovs_interface vif1
>> +  remove_ovs_interface vif2
>> +}
>> +add_iface_ids() {
>> +  echo Adding iface-id vif1 lsp1
>> +  ovs-vsctl --no-wait -- set Interface vif1 external_ids:iface-id=lsp1
>> +  echo Adding iface-id vif2 lsp2
>> +  ovs-vsctl --no-wait -- set Interface vif2 external_ids:iface-id=lsp2
>> +}
>> +remove_iface_id() {
>> +  echo Removing iface-id $1
>> +  check ovs-vsctl remove Interface $1 external_ids iface-id
>> +}
>> +remove_iface_ids() {
>> +  remove_iface_id vif1
>> +  remove_iface_id vif2
>> +}
>> +wait_for_local_bindings() {
>> +  OVS_WAIT_UNTIL(
>> +      [test `ovs-appctl -t ovn-controller debug/dump-local-bindings |
>> grep interface | wc -l` -eq 2],
>> +      [kill -CONT $(cat ovn-sb/ovsdb-server.pid)]
>> +  )
>> +}
>> +sleep_sb() {
>> +  echo SB going to sleep
>> +  AT_CHECK([kill -STOP $(cat ovn-sb/ovsdb-server.pid)])
>> +}
>> +wake_up_sb() {
>> +  echo SB waking up
>> +  AT_CHECK([kill -CONT $(cat ovn-sb/ovsdb-server.pid)])
>> +}
>> +sleep_controller() {
>> +  echo Controller going to sleep
>> +  ovn-appctl debug/pause
>> +  OVS_WAIT_UNTIL([test x$(ovn-appctl -t ovn-controller debug/status) =
>> "xpaused"])
>> +}
>> +
>> +stop_ovsdb_controller_updates() {
>> +  TCP_PORT=$1
>> +  echo Stopping updates from ovn-controller to ovsdb using port $TCP_PORT
>> +  on_exit 'iptables -C INPUT -p tcp --destination-port $TCP_PORT -j DROP
>> 2>/dev/null && iptables -D INPUT -p tcp --destination-port $TCP_PORT -j
>> DROP'
>> +  iptables -A INPUT -p tcp --destination-port $TCP_PORT -j DROP
>> +}
>> +restart_ovsdb_controller_updates() {
>> +  TCP_PORT=$1
>> +  echo Restarting updates from ovn-controller to ovsdb
>> +  iptables -D INPUT -p tcp --destination-port $TCP_PORT  -j DROP
>> +}
>> +wake_up_controller() {
>> +  echo Controller waking up
>> +  ovn-appctl debug/resume
>> +}
>> +ensure_controller_run() {
>> +# We want to make sure controller could run at least one full loop.
>> +# We can't use wait=hv as sb might be sleeping.
>> +# Use 2 ovn-appctl to guarentee that ovn-controller run the full loop,
>> and not just the unixctl handling
>> +  OVS_WAIT_UNTIL([test x$(ovn-appctl -t ovn-controller debug/status) =
>> "xrunning"])
>> +  OVS_WAIT_UNTIL([test x$(ovn-appctl -t ovn-controller debug/status) =
>> "xrunning"])
>> +}
>> +sleep_ovsdb() {
>> +  echo OVSDB going to sleep
>> +  AT_CHECK([kill -STOP $(cat ovsdb-server.pid)])
>> +}
>> +wake_up_ovsdb() {
>> +  echo OVSDB waking up
>> +  AT_CHECK([kill -CONT $(cat ovsdb-server.pid)])
>> +}
>> +check_ovn_installed() {
>> +  OVS_WAIT_UNTIL([test `ovs-vsctl get Interface vif1
>> external_ids:ovn-installed` = '"true"'])
>> +  OVS_WAIT_UNTIL([test `ovs-vsctl get Interface vif2
>> external_ids:ovn-installed` = '"true"'])
>> +}
>> +check_ovn_uninstalled() {
>> +  OVS_WAIT_UNTIL([test x`ovs-vsctl get Interface vif2
>> external_ids:ovn-installed` = x])
>> +  OVS_WAIT_UNTIL([test x`ovs-vsctl get Interface vif1
>> external_ids:ovn-installed` = x])
>> +}
>> +check_ports_up() {
>> +  OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp1 up` = 'true'])
>> +  OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp2 up` = 'true'])
>> +}
>> +check_ports_down() {
>> +  OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp1 up` = 'false'])
>> +  OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp2 up` = 'false'])
>> +}
>> +
>> +check_ports_bound() {
>> +  ch=$(fetch_column Chassis _uuid name=hv1)
>> +  wait_row_count Port_Binding 1 logical_port=lsp1 chassis=$ch
>> +  wait_row_count Port_Binding 1 logical_port=lsp2 chassis=$ch
>> +}
>> +check_ports_unbound() {
>> +  wait_column "" Port_Binding chassis logical_port=lsp1
>> +  wait_column "" Port_Binding chassis logical_port=lsp2
>> +}
>> +add_logical_ports
>> +add_ovs_interfaces
>> +wait_for_local_bindings
>> +wait_for_ports_up
>> +check ovn-nbctl --wait=hv sync
>> +############################################################
>> +########## Remove interface while removing iface-id ########
>> +############################################################
>> +AS_BOX(["Remove interface while removing iface-id"])
>> +stop_ovsdb_controller_updates $TCP_PORT
>> +remove_iface_id vif1
>> +ensure_controller_run
>> +# OVSDB should be seen as ro now
>> +remove_iface_id vif2
>> +ensure_controller_run
>> +# Controller delaying ovn-install removal for vif2 as ovsdb ro
>> +sleep_controller
>> +restart_ovsdb_controller_updates $TCP_PORT
>> +remove_ovs_interface vif2
>> +# vif2, for which we want to remove ovn-install, is deleted
>> +wake_up_controller
>> +check_ovn_uninstalled
>> +check_ports_down
>> +check_ports_unbound
>> +add_ovs_interface vif2 lsp2
>> +add_iface_ids
>> +check_ovn_installed
>> +check_ports_up
>> +check_ports_bound
>> +############################################################
>> +################### Add/Remove iface-id ####################
>> +############################################################
>> +AS_BOX(["iface-id removal and added back (no sleeping sb or controller)"])
>> +remove_iface_ids
>> +check_ovn_uninstalled
>> +check_ports_down
>> +check_ports_unbound
>> +add_iface_ids
>> +check_ovn_installed
>> +check_ports_up
>> +check_ports_bound
>> +
>> +AS_BOX(["iface-id removal"])
>> +sleep_sb
>> +remove_iface_ids
>> +ensure_controller_run
>> +sleep_controller
>> +wake_up_sb
>> +wake_up_controller
>> +check_ovn_uninstalled
>> +# Port_down not always set on iface-id removal
>> +# check_ports_down
>> +# Port_Binding(chassis) not always removed on iface-id removal
>> +# check_ports_unbound
>> +add_iface_ids
>> +check ovn-nbctl --wait=hv sync
>> +
>> +AS_BOX(["iface-id removal 2"])
>> +# Block IDL from ovn-controller to OVSDB
>> +stop_ovsdb_controller_updates $TCP_PORT
>> +remove_iface_id vif2
>> +ensure_controller_run
>> +
>> +# OVSDB should now be seen as read-only by ovn-controller
>> +remove_iface_id vif1
>> +ensure_controller_run
>> +
>> +# Restart connection from ovn-controller to OVSDB
>> +restart_ovsdb_controller_updates $TCP_PORT
>> +check_ovn_uninstalled
>> +check_ports_down
>> +check_ports_unbound
>> +
>> +add_iface_ids
>> +check ovn-nbctl --wait=hv sync
>> +
>> +AS_BOX(["iface-id removal and added back"])
>> +sleep_sb
>> +remove_iface_ids
>> +ensure_controller_run
>> +sleep_controller
>> +add_iface_ids
>> +wake_up_sb
>> +wake_up_controller
>> +check_ovn_installed
>> +check_ports_up
>> +check_ports_bound
>> +############################################################
>> +###################### Add/Remove Interface ################
>> +############################################################
>> +AS_BOX(["Interface removal and added back (no sleeping sb or
>> controller)"])
>> +remove_ovs_interfaces
>> +check_ovn_uninstalled
>> +check_ports_down
>> +check_ports_unbound
>> +add_ovs_interfaces
>> +check_ovn_installed
>> +check_ports_up
>> +check_ports_bound
>> +check ovn-nbctl --wait=hv sync
>> +
>> +AS_BOX(["Interface removal"])
>> +sleep_sb
>> +remove_ovs_interfaces
>> +ensure_controller_run
>> +sleep_controller
>> +wake_up_sb
>> +wake_up_controller
>> +check_ovn_uninstalled
>> +# Port_down not always set on Interface removal
>> +# check_ports_down
>> +# Port_Binding(chassis) not always removed on Interface removal
>> +# check_ports_unbound
>> +add_ovs_interfaces
>> +check ovn-nbctl --wait=hv sync
>> +
>> +AS_BOX(["Interface removal and added back"])
>> +sleep_sb
>> +remove_ovs_interfaces
>> +ensure_controller_run
>> +sleep_controller
>> +add_ovs_interfaces
>> +wake_up_sb
>> +wake_up_controller
>> +check_ovn_installed
>> +check_ports_up
>> +check_ports_bound
>> +check ovn-nbctl --wait=hv sync
>> +############################################################
>> +###################### Add/Remove Logical Port #############
>> +############################################################
>> +AS_BOX(["Logical port removal and added back (no sleeping sb or
>> controller)"])
>> +remove_logical_ports
>> +check_ovn_uninstalled
>> +check_ports_unbound
>> +sleep_ovsdb
>> +add_logical_ports
>> +ensure_controller_run
>> +wake_up_ovsdb
>> +check_ovn_installed
>> +check_ports_up
>> +check_ports_bound
>> +check ovn-nbctl --wait=hv sync
>> +
>> +AS_BOX(["Logical port removal"])
>> +sleep_sb
>> +remove_logical_ports
>> +ensure_controller_run
>> +sleep_controller
>> +wake_up_sb
>> +wake_up_controller
>> +check_ovn_uninstalled
>> +check_ports_unbound
>> +add_logical_ports
>> +check ovn-nbctl --wait=hv sync
>> +
>> +AS_BOX(["Logical port removal and added back"])
>> +sleep_sb
>> +remove_logical_ports
>> +ensure_controller_run
>> +sleep_controller
>> +add_logical_ports
>> +wake_up_sb
>> +wake_up_controller
>> +check_ovn_installed
>> +check_ports_up
>> +check_ports_bound
>> +
>> +OVS_APP_EXIT_AND_WAIT([ovn-controller])
>> +
>> +as ovn-sb
>> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>> +
>> +as ovn-nb
>> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>> +
>> +as northd
>> +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE])
>> +
>> +as
>> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
>> +/connection dropped.*/d"])
>> +AT_CLEANUP
>> +])
>> +
>> --
>> 2.31.1
>>
>>
> Looks good to me, thanks.
> 
> Reviewed-by: Ales Musil <amusil@redhat.com>
> 

Thanks, Xavier and Ales!

I applied this to the main branch and backported it to stable branches
down to 22.09.  Xavier, the patch didn't apply cleanly to 22.06 and
22.03.  Do you have time to prepare a custom backport?  Otherwise I'll
look into it.

Regards,
Dumitru

Thanks Ales, Dumitru !

Dumitru, I'll work on the custom backport for 22.03 and 22.06. Thanks for
applying to main.

Thanks
Xavier


On Mon, May 8, 2023 at 3:12 PM Dumitru Ceara <dceara@redhat.com> wrote:

> On 5/5/23 11:21, Ales Musil wrote:
> > On Fri, Apr 28, 2023 at 1:59 PM Xavier Simonart <xsimonar@redhat.com>
> wrote:
> >
> >> OVN checks whether ovn-installed is already present (in OVS) before
> >> updating it.
> >> This might cause ovn-installed related issues in the following case:
> >> - (1) ovn-installed is present
> >> - (2) we claim the interface
> >> - (3) we update ovs, removing ovn-installed and start installing flows
> >> - (4) (next loop), after flows installed, we check if ovn-installed is
> >> absent,
> >>   and if yes, we update OVS with ovn-installed.
> >> However, in step4, if OVS is still busy from step 3, ovn-installed is
> read
> >> as
> >> present; hence OVN controller does not update it and move to INSTALLED
> >> state.
> >>
> >> ovn-installed was also not properly deleted on port or binding removal.
> >>
> >> Note that port->up is not properly removed on external_ids:iface-id
> >> removal when
> >> sb is read-only. This will be fixed in a further patch.
> >>
> >> Fixes: a7c7d4519e50 ("controller: avoid recomputes triggered by SBDB
> >> Port_Binding updates.")
> >>
> >> Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
> >>
> >> ---
> >> v2: - handled Dumitru's comments
> >>     - rebased on main
> >>     - updated state machine diagram as commented in v1 commit message
> >>     - remove ovn-installed on port deletion or external_ids:iface-id
> >> removal.
> >>     - added test case
> >> v3: - handled more comment from Dumitru
> >>     - fixed ovn-install not removed when ovs is read-only
> >>     - moved test case from unit (ovn.at) to system (system-ovn).
> >>     - test case connects to OVS db through tcp and uses iptables to
> >> momentarly block the idl update
> >>       to simulate read-only ovsdb
> >> v4: - handled comments from Ales: avoid using is_deleted outside of
> >> tracked loops.
> >> ---
> >>  controller/binding.c        |  68 ++++++-
> >>  controller/binding.h        |  11 ++
> >>  controller/if-status.c      | 204 ++++++++++++++++++---
> >>  controller/if-status.h      |   7 +
> >>  controller/ovn-controller.c |   5 +
> >>  tests/system-ovn.at         | 349 ++++++++++++++++++++++++++++++++++++
> >>  6 files changed, 612 insertions(+), 32 deletions(-)
> >>
> >> diff --git a/controller/binding.c b/controller/binding.c
> >> index 5df62baef..357e77609 100644
> >> --- a/controller/binding.c
> >> +++ b/controller/binding.c
> >> @@ -746,6 +746,19 @@ local_binding_get_lport_ofport(const struct shash
> >> *local_bindings,
> >>              u16_to_ofp(lbinding->iface->ofport[0]) : 0;
> >>  }
> >>
> >> +bool
> >> +local_binding_is_ovn_installed(struct shash *local_bindings,
> >> +                               const char *pb_name)
> >> +{
> >> +    struct local_binding *lbinding =
> >> +        local_binding_find(local_bindings, pb_name);
> >> +    if (lbinding && lbinding->iface) {
> >> +        return smap_get_bool(&lbinding->iface->external_ids,
> >> +                             OVN_INSTALLED_EXT_ID, false);
> >> +    }
> >> +    return false;
> >> +}
> >> +
> >>  bool
> >>  local_binding_is_up(struct shash *local_bindings, const char *pb_name,
> >>                      const struct sbrec_chassis *chassis_rec)
> >> @@ -783,6 +796,7 @@ local_binding_is_down(struct shash *local_bindings,
> >> const char *pb_name,
> >>          } else if (b_lport->pb->chassis) {
> >>              VLOG_DBG("lport %s already claimed by other chassis",
> >>                       b_lport->pb->logical_port);
> >> +            return true;
> >>          }
> >>      }
> >>
> >> @@ -834,6 +848,38 @@ local_binding_set_up(struct shash *local_bindings,
> >> const char *pb_name,
> >>      }
> >>  }
> >>
> >> +void
> >> +local_binding_remove_ovn_installed(
> >> +        struct shash *local_bindings,
> >> +        const struct ovsrec_interface_table *iface_table,
> >> +        const char *pb_name, bool ovs_readonly)
> >> +{
> >> +    if (ovs_readonly) {
> >> +        return;
> >> +    }
> >> +    struct local_binding *lbinding =
> >> +        local_binding_find(local_bindings, pb_name);
> >> +    if (lbinding && lbinding->iface) {
> >> +        const struct uuid *iface_uuid = &lbinding->iface->header_.uuid;
> >> +        remove_ovn_installed_for_uuid(iface_table, iface_uuid);
> >> +    }
> >> +}
> >> +
> >> +void
> >> +remove_ovn_installed_for_uuid(const struct ovsrec_interface_table
> >> *iface_table,
> >> +                              const struct uuid *iface_uuid)
> >> +{
> >> +    const struct ovsrec_interface *iface_rec =
> >> +        ovsrec_interface_table_get_for_uuid(iface_table, iface_uuid);
> >> +    if (iface_rec && smap_get_bool(&iface_rec->external_ids,
> >> +                                   OVN_INSTALLED_EXT_ID, false)) {
> >> +        VLOG_INFO("Removing iface %s ovn-installed in OVS",
> >> +                  iface_rec->name);
> >> +        ovsrec_interface_update_external_ids_delkey(iface_rec,
> >> +
> OVN_INSTALLED_EXT_ID);
> >> +    }
> >> +}
> >> +
> >>  void
> >>  local_binding_set_down(struct shash *local_bindings, const char
> *pb_name,
> >>                         const struct sbrec_chassis *chassis_rec,
> >> @@ -1239,7 +1285,9 @@ claim_lport(const struct sbrec_port_binding *pb,
> >>                      return false;
> >>                  }
> >>              } else {
> >> -                if (pb->n_up && !pb->up[0]) {
> >> +                if ((pb->n_up && !pb->up[0]) ||
> >> +                    !smap_get_bool(&iface_rec->external_ids,
> >> +                                   OVN_INSTALLED_EXT_ID, false)) {
> >>                      if_status_mgr_claim_iface(if_mgr, pb, chassis_rec,
> >>                                                sb_readonly);
> >>                  }
> >> @@ -1464,9 +1512,11 @@ consider_vif_lport_(const struct
> sbrec_port_binding
> >> *pb,
> >>              const char *requested_chassis_option = smap_get(
> >>                  &pb->options, "requested-chassis");
> >>              VLOG_INFO_RL(&rl,
> >> -                "Not claiming lport %s, chassis %s requested-chassis
> %s",
> >> +                "Not claiming lport %s, chassis %s requested-chassis
> %s "
> >> +                "pb->chassis %s",
> >>                  pb->logical_port, b_ctx_in->chassis_rec->name,
> >> -                requested_chassis_option ? requested_chassis_option :
> >> "[]");
> >> +                requested_chassis_option ? requested_chassis_option :
> >> "[]",
> >> +                pb->chassis ? pb->chassis->name: "");
> >>          }
> >>      }
> >>
> >> @@ -2288,6 +2338,11 @@ consider_iface_release(const struct
> >> ovsrec_interface *iface_rec,
> >>                  return false;
> >>              }
> >>          }
> >> +        if (lbinding->iface && lbinding->iface->name) {
> >> +            if_status_mgr_remove_ovn_installed(b_ctx_out->if_mgr,
> >> +                                               lbinding->iface->name,
> >> +
> >>  &lbinding->iface->header_.uuid);
> >> +        }
> >>
> >>      } else if (lbinding && b_lport && b_lport->type == LP_LOCALPORT) {
> >>          /* lbinding is associated with a localport.  Remove it from the
> >> @@ -2558,6 +2613,7 @@ handle_deleted_lport(const struct
> sbrec_port_binding
> >> *pb,
> >>      if (ld) {
> >>          remove_pb_from_local_datapath(pb,
> >>                                        b_ctx_out, ld);
> >> +        if_status_mgr_release_iface(b_ctx_out->if_mgr,
> pb->logical_port);
> >>          return;
> >>      }
> >>
> >> @@ -2581,6 +2637,7 @@ handle_deleted_lport(const struct
> sbrec_port_binding
> >> *pb,
> >>              remove_pb_from_local_datapath(pb, b_ctx_out,
> >>                                            ld);
> >>          }
> >> +        if_status_mgr_release_iface(b_ctx_out->if_mgr,
> pb->logical_port);
> >>      }
> >>  }
> >>
> >> @@ -2627,6 +2684,11 @@ handle_deleted_vif_lport(const struct
> >> sbrec_port_binding *pb,
> >>      }
> >>
> >>      handle_deleted_lport(pb, b_ctx_in, b_ctx_out);
> >> +    if (lbinding && lbinding->iface && lbinding->iface->name) {
> >> +        if_status_mgr_remove_ovn_installed(b_ctx_out->if_mgr,
> >> +                                           lbinding->iface->name,
> >> +
> >>  &lbinding->iface->header_.uuid);
> >> +    }
> >>      return true;
> >>  }
> >>
> >> diff --git a/controller/binding.h b/controller/binding.h
> >> index 6c3a98b02..0b9c6994f 100644
> >> --- a/controller/binding.h
> >> +++ b/controller/binding.h
> >> @@ -159,6 +159,14 @@ 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,
> >>                             const struct sbrec_chassis *);
> >>
> >> +bool local_binding_is_ovn_installed(struct shash *local_bindings,
> >> +                                    const char *pb_name);
> >> +void local_binding_remove_ovn_installed(
> >> +        struct shash *local_bindings,
> >> +        const struct ovsrec_interface_table *iface_table,
> >> +        const char *pb_name,
> >> +        bool ovs_readonly);
> >> +
> >>  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,
> >> @@ -195,6 +203,9 @@ void set_pb_chassis_in_sbrec(const struct
> >> sbrec_port_binding *pb,
> >>                               const struct sbrec_chassis *chassis_rec,
> >>                               bool is_set);
> >>
> >> +void remove_ovn_installed_for_uuid(const struct ovsrec_interface_table
> *,
> >> +                                   const struct uuid *);
> >> +
> >>  /* 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 d1c14ac30..ac36a9fb9 100644
> >> --- a/controller/if-status.c
> >> +++ b/controller/if-status.c
> >> @@ -54,32 +54,37 @@ VLOG_DEFINE_THIS_MODULE(if_status);
> >>   */
> >>
> >>  enum if_state {
> >> -    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
> >> -                        * SB and OVS databases).
> >> -                        */
> >> -    OIF_MARK_DOWN,     /* Released interface but not yet marked "down"
> in
> >> the
> >> -                        * binding module (in SB and/or OVS databases).
> >> -                        */
> >> -    OIF_INSTALLED,     /* Interface flows programmed in OVS and binding
> >> marked
> >> -                        * "up" in the binding module.
> >> -                        */
> >> +    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_REM_OLD_OVN_INST, /* Interface with flows successfully
> installed
> >> in OVS
> >> +                           * but with ovn-installed still in OVSDB.
> >> +                           */
> >> +    OIF_MARK_UP,          /* Interface with flows successfully
> installed
> >> in OVS
> >> +                           * but not yet marked "up" in the binding
> >> module (in
> >> +                           * SB and OVS databases).
> >> +                           */
> >> +    OIF_MARK_DOWN,        /* Released interface but not yet marked
> "down"
> >> in
> >> +                           * the binding module (in SB and/or OVS
> >> databases).
> >> +                           */
> >> +    OIF_INSTALLED,        /* Interface flows programmed in OVS and
> binding
> >> +                           * marked "up" in the binding module.
> >> +                           */
> >>      OIF_MAX,
> >>  };
> >>
> >>  static const char *if_state_names[] = {
> >> -    [OIF_CLAIMED]       = "CLAIMED",
> >> -    [OIF_INSTALL_FLOWS] = "INSTALL_FLOWS",
> >> -    [OIF_MARK_UP]       = "MARK_UP",
> >> -    [OIF_MARK_DOWN]     = "MARK_DOWN",
> >> -    [OIF_INSTALLED]     = "INSTALLED",
> >> +    [OIF_CLAIMED]          = "CLAIMED",
> >> +    [OIF_INSTALL_FLOWS]    = "INSTALL_FLOWS",
> >> +    [OIF_REM_OLD_OVN_INST] = "REM_OLD_OVN_INST",
> >> +    [OIF_MARK_UP]          = "MARK_UP",
> >> +    [OIF_MARK_DOWN]        = "MARK_DOWN",
> >> +    [OIF_INSTALLED]        = "INSTALLED",
> >>  };
> >>
> >>  /*
> >> @@ -109,11 +114,26 @@ static const char *if_state_names[] = {
> >>   * |     |                      |   - 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(seqno rcvd, ovn-installed present)          |
> >> | | |
> >> + * |        V                                                     |
> >> | | |
> >> + * |    +--------------------+                                    |
> >> | | |
> >> + * |    |                    |  mgr_run()                         |
> >> | | |
> >> + * +--- | REM_OLD_OVN_INST   |  - remove ovn-installed in ovs     |
> >> | | |
> >> + * |    +--------------------+                                    |
> >> | | |
> >> + * |               |                                              |
> >> | | |
> >> + * |               |                                              |
> >> | | |
> >> + * |               | mgr_update( ovn_installed not present)       |
> >> | | |
> >> + * |               |                                              |
> >> | | |
> >> + * |               |  +-------------------------------------------+
> >> | | |
> >> + * |               |  |
> >> | | |
> >> + * |               |  |  mgr_run(seqno rcvd, ovn-installed not present)
> >> | | |
> >> + * |               |  |  - set port up in sb
> >>  | | |
> >> + * |               |  |  - set ovn-installed in ovs
> >> | | |
> >> + * |release_iface  |  |
> >> | | |
> >> + * |               V  V
> >> | | |
> >>   * |   +----------------------+
> >> | | |
> >>   * |   |                      |  mgr_run()
> >>  | | |
> >>   * +-- |       MARK_UP        |  - set port up in sb
> >>  | | |
> >> @@ -155,6 +175,9 @@ struct if_status_mgr {
> >>      /* All local interfaces, mapping from 'iface-id' to 'struct
> >> ovs_iface'. */
> >>      struct shash ifaces;
> >>
> >> +    /* local interfaces which need ovn-install removal */
> >> +    struct shash ovn_uninstall_hash;
> >> +
> >>      /* All local interfaces, stored per state. */
> >>      struct hmapx ifaces_per_state[OIF_MAX];
> >>
> >> @@ -170,15 +193,20 @@ struct if_status_mgr {
> >>  static struct ovs_iface *ovs_iface_create(struct if_status_mgr *,
> >>                                            const char *iface_id,
> >>                                            enum if_state );
> >> +static void add_to_ovn_uninstall_hash(struct if_status_mgr *, const
> char
> >> *,
> >> +                                      const struct uuid *);
> >>  static void ovs_iface_destroy(struct if_status_mgr *, struct ovs_iface
> *);
> >> +static void ovn_uninstall_hash_destroy(struct if_status_mgr *mgr, char
> >> *name);
> >>  static void ovs_iface_set_state(struct if_status_mgr *, struct
> ovs_iface
> >> *,
> >>                                  enum if_state);
> >>
> >>  static void if_status_mgr_update_bindings(
> >>      struct if_status_mgr *mgr, struct local_binding_data *binding_data,
> >>      const struct sbrec_chassis *,
> >> +    const struct ovsrec_interface_table *iface_table,
> >>      bool sb_readonly, bool ovs_readonly);
> >>
> >> +static void ovn_uninstall_hash_account_mem(const char *name, bool
> erase);
> >>  struct if_status_mgr *
> >>  if_status_mgr_create(void)
> >>  {
> >> @@ -189,6 +217,7 @@ if_status_mgr_create(void)
> >>          hmapx_init(&mgr->ifaces_per_state[i]);
> >>      }
> >>      shash_init(&mgr->ifaces);
> >> +    shash_init(&mgr->ovn_uninstall_hash);
> >>      return mgr;
> >>  }
> >>
> >> @@ -202,6 +231,11 @@ if_status_mgr_clear(struct if_status_mgr *mgr)
> >>      }
> >>      ovs_assert(shash_is_empty(&mgr->ifaces));
> >>
> >> +    SHASH_FOR_EACH_SAFE (node, &mgr->ovn_uninstall_hash) {
> >> +        ovn_uninstall_hash_destroy(mgr, node->data);
> >> +    }
> >> +    ovs_assert(shash_is_empty(&mgr->ovn_uninstall_hash));
> >> +
> >>      for (size_t i = 0; i < ARRAY_SIZE(mgr->ifaces_per_state); i++) {
> >>          ovs_assert(hmapx_is_empty(&mgr->ifaces_per_state[i]));
> >>      }
> >> @@ -212,6 +246,7 @@ if_status_mgr_destroy(struct if_status_mgr *mgr)
> >>  {
> >>      if_status_mgr_clear(mgr);
> >>      shash_destroy(&mgr->ifaces);
> >> +    shash_destroy(&mgr->ovn_uninstall_hash);
> >>      for (size_t i = 0; i < ARRAY_SIZE(mgr->ifaces_per_state); i++) {
> >>          hmapx_destroy(&mgr->ifaces_per_state[i]);
> >>      }
> >> @@ -238,6 +273,7 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr,
> >>      switch (iface->state) {
> >>      case OIF_CLAIMED:
> >>      case OIF_INSTALL_FLOWS:
> >> +    case OIF_REM_OLD_OVN_INST:
> >>      case OIF_MARK_UP:
> >>          /* Nothing to do here. */
> >>          break;
> >> @@ -274,6 +310,7 @@ if_status_mgr_release_iface(struct if_status_mgr
> *mgr,
> >> const char *iface_id)
> >>          /* Not yet fully installed interfaces can be safely deleted. */
> >>          ovs_iface_destroy(mgr, iface);
> >>          break;
> >> +    case OIF_REM_OLD_OVN_INST:
> >>      case OIF_MARK_UP:
> >>      case OIF_INSTALLED:
> >>          /* Properly mark interfaces "down" if their flows were already
> >> @@ -305,6 +342,7 @@ if_status_mgr_delete_iface(struct if_status_mgr
> *mgr,
> >> const char *iface_id)
> >>          /* Not yet fully installed interfaces can be safely deleted. */
> >>          ovs_iface_destroy(mgr, iface);
> >>          break;
> >> +    case OIF_REM_OLD_OVN_INST:
> >>      case OIF_MARK_UP:
> >>      case OIF_INSTALLED:
> >>          /* Properly mark interfaces "down" if their flows were already
> >> @@ -346,12 +384,33 @@ if_status_handle_claims(struct if_status_mgr *mgr,
> >>      return rc;
> >>  }
> >>
> >> +static void
> >> +clean_ovn_installed(struct if_status_mgr *mgr,
> >> +                    const struct ovsrec_interface_table *iface_table)
> >> +{
> >> +    struct shash_node *node;
> >> +
> >> +    SHASH_FOR_EACH_SAFE (node, &mgr->ovn_uninstall_hash) {
> >> +        const struct uuid *iface_uuid = node->data;
> >> +        remove_ovn_installed_for_uuid(iface_table, iface_uuid);
> >> +        free(node->data);
> >> +        char *node_name = shash_steal(&mgr->ovn_uninstall_hash, node);
> >> +        ovn_uninstall_hash_account_mem(node_name, true);
> >> +        free(node_name);
> >> +    }
> >> +}
> >> +
> >>  void
> >>  if_status_mgr_update(struct if_status_mgr *mgr,
> >>                       struct local_binding_data *binding_data,
> >>                       const struct sbrec_chassis *chassis_rec,
> >> +                     const struct ovsrec_interface_table *iface_table,
> >> +                     bool ovs_readonly,
> >>                       bool sb_readonly)
> >>  {
> >> +    if (!ovs_readonly) {
> >> +        clean_ovn_installed(mgr, iface_table);
> >> +    }
> >>      if (!binding_data) {
> >>          return;
> >>      }
> >> @@ -359,6 +418,17 @@ if_status_mgr_update(struct if_status_mgr *mgr,
> >>      struct shash *bindings = &binding_data->bindings;
> >>      struct hmapx_node *node;
> >>
> >> +    /* Move all interfaces that have been confirmed without
> ovn-installed,
> >> +     * from OIF_REM_OLD_OVN_INST to OIF_MARK_UP.
> >> +     */
> >> +    HMAPX_FOR_EACH_SAFE (node,
> >> &mgr->ifaces_per_state[OIF_REM_OLD_OVN_INST]) {
> >> +        struct ovs_iface *iface = node->data;
> >> +
> >> +        if (!local_binding_is_ovn_installed(bindings, iface->id)) {
> >> +            ovs_iface_set_state(mgr, iface, OIF_MARK_UP);
> >> +        }
> >> +    }
> >> +
> >>      /* 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.
> >> @@ -450,10 +520,23 @@ if_status_mgr_update(struct if_status_mgr *mgr,
> >>      }
> >>  }
> >>
> >> +void
> >> +if_status_mgr_remove_ovn_installed(struct if_status_mgr *mgr,
> >> +                                   const char *name,
> >> +                                   const struct uuid *uuid)
> >> +{
> >> +    VLOG_DBG("Adding %s to list of interfaces for which to remove "
> >> +              "ovn-installed", name);
> >> +    if (!shash_find_data(&mgr->ovn_uninstall_hash, name)) {
> >> +        add_to_ovn_uninstall_hash(mgr, name, uuid);
> >> +    }
> >> +}
> >> +
> >>  void
> >>  if_status_mgr_run(struct if_status_mgr *mgr,
> >>                    struct local_binding_data *binding_data,
> >>                    const struct sbrec_chassis *chassis_rec,
> >> +                  const struct ovsrec_interface_table *iface_table,
> >>                    bool sb_readonly, bool ovs_readonly)
> >>  {
> >>      struct ofctrl_acked_seqnos *acked_seqnos =
> >> @@ -471,12 +554,25 @@ if_status_mgr_run(struct if_status_mgr *mgr,
> >>                                            iface->install_seqno)) {
> >>              continue;
> >>          }
> >> -        ovs_iface_set_state(mgr, iface, OIF_MARK_UP);
> >> +        /* Wait for ovn-installed to be absent before moving to MARK_UP
> >> state.
> >> +         * Most of the times ovn-installed is already absent and hence
> we
> >> will
> >> +         * not have to wait.
> >> +         * If there is no binding_data, we can't determine if
> >> ovn-installed is
> >> +         * present or not; hence also go to the OIF_REM_OLD_OVN_INST
> >> state.
> >> +         */
> >> +        if (!binding_data ||
> >> +            local_binding_is_ovn_installed(&binding_data->bindings,
> >> +                                           iface->id)) {
> >> +            ovs_iface_set_state(mgr, iface, OIF_REM_OLD_OVN_INST);
> >> +        } else {
> >> +            ovs_iface_set_state(mgr, iface, OIF_MARK_UP);
> >> +        }
> >>      }
> >>      ofctrl_acked_seqnos_destroy(acked_seqnos);
> >>
> >>      /* Update binding states. */
> >>      if_status_mgr_update_bindings(mgr, binding_data, chassis_rec,
> >> +                                  iface_table,
> >>                                    sb_readonly, ovs_readonly);
> >>  }
> >>
> >> @@ -492,6 +588,18 @@ ovs_iface_account_mem(const char *iface_id, bool
> >> erase)
> >>      }
> >>  }
> >>
> >> +static void
> >> +ovn_uninstall_hash_account_mem(const char *name, bool erase)
> >> +{
> >> +    uint32_t size = (strlen(name) + sizeof(struct uuid) +
> >> +                     sizeof(struct shash_node));
> >> +    if (erase) {
> >> +        ifaces_usage -= size;
> >> +    } else {
> >> +        ifaces_usage += size;
> >> +    }
> >> +}
> >> +
> >>  static struct ovs_iface *
> >>  ovs_iface_create(struct if_status_mgr *mgr, const char *iface_id,
> >>                   enum if_state state)
> >> @@ -506,6 +614,16 @@ ovs_iface_create(struct if_status_mgr *mgr, const
> >> char *iface_id,
> >>      return iface;
> >>  }
> >>
> >> +static void
> >> +add_to_ovn_uninstall_hash(struct if_status_mgr *mgr, const char *name,
> >> +                          const struct uuid *uuid)
> >> +{
> >> +    struct uuid *new_uuid = xzalloc(sizeof *new_uuid);
> >> +    memcpy(new_uuid, uuid, sizeof(*new_uuid));
> >> +    shash_add(&mgr->ovn_uninstall_hash, name, new_uuid);
> >> +    ovn_uninstall_hash_account_mem(name, false);
> >> +}
> >> +
> >>  static void
> >>  ovs_iface_destroy(struct if_status_mgr *mgr, struct ovs_iface *iface)
> >>  {
> >> @@ -521,6 +639,23 @@ ovs_iface_destroy(struct if_status_mgr *mgr, struct
> >> ovs_iface *iface)
> >>      free(iface);
> >>  }
> >>
> >> +static void
> >> +ovn_uninstall_hash_destroy(struct if_status_mgr *mgr, char *name)
> >> +{
> >> +    struct shash_node *node = shash_find(&mgr->ovn_uninstall_hash,
> name);
> >> +    char *node_name = NULL;
> >> +    if (node) {
> >> +        free(node->data);
> >> +        VLOG_DBG("Interface name %s destroy", name);
> >> +        node_name = shash_steal(&mgr->ovn_uninstall_hash, node);
> >> +        ovn_uninstall_hash_account_mem(name, true);
> >> +        free(node_name);
> >> +    } else {
> >> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> >> +        VLOG_WARN_RL(&rl, "Interface name %s not found", name);
> >> +    }
> >> +}
> >> +
> >>  static void
> >>  ovs_iface_set_state(struct if_status_mgr *mgr, struct ovs_iface *iface,
> >>                      enum if_state state)
> >> @@ -539,6 +674,7 @@ static void
> >>  if_status_mgr_update_bindings(struct if_status_mgr *mgr,
> >>                                struct local_binding_data *binding_data,
> >>                                const struct sbrec_chassis *chassis_rec,
> >> +                              const struct ovsrec_interface_table
> >> *iface_table,
> >>                                bool sb_readonly, bool ovs_readonly)
> >>  {
> >>      if (!binding_data) {
> >> @@ -558,7 +694,17 @@ if_status_mgr_update_bindings(struct if_status_mgr
> >> *mgr,
> >>                                 sb_readonly, ovs_readonly);
> >>      }
> >>
> >> -    /* Notifiy the binding module to set "up" all bindings that have
> had
> >> +    /* Notify the binding module to remove "ovn-installed" for all
> >> bindings
> >> +     * in the OIF_REM_OLD_OVN_INST state.
> >> +     */
> >> +    HMAPX_FOR_EACH (node,
> &mgr->ifaces_per_state[OIF_REM_OLD_OVN_INST]) {
> >> +        struct ovs_iface *iface = node->data;
> >> +
> >> +        local_binding_remove_ovn_installed(bindings, iface_table,
> >> iface->id,
> >> +                                           ovs_readonly);
> >> +    }
> >> +
> >> +    /* Notify the binding module to set "up" all bindings that have had
> >>       * their flows installed but are not yet marked "up" in the binding
> >>       * module.
> >>       */
> >> diff --git a/controller/if-status.h b/controller/if-status.h
> >> index 5bd187a25..203764946 100644
> >> --- a/controller/if-status.h
> >> +++ b/controller/if-status.h
> >> @@ -17,6 +17,7 @@
> >>  #define IF_STATUS_H 1
> >>
> >>  #include "openvswitch/shash.h"
> >> +#include "lib/vswitch-idl.h"
> >>
> >>  #include "binding.h"
> >>
> >> @@ -35,9 +36,12 @@ 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 *,
> >>                            const struct sbrec_chassis *chassis,
> >> +                          const struct ovsrec_interface_table
> >> *iface_table,
> >> +                          bool ovs_readonly,
> >>                            bool sb_readonly);
> >>  void if_status_mgr_run(struct if_status_mgr *mgr, struct
> >> local_binding_data *,
> >>                         const struct sbrec_chassis *,
> >> +                       const struct ovsrec_interface_table
> *iface_table,
> >>                         bool sb_readonly, bool ovs_readonly);
> >>  void if_status_mgr_get_memory_usage(struct if_status_mgr *mgr,
> >>                                      struct simap *usage);
> >> @@ -48,5 +52,8 @@ bool if_status_handle_claims(struct if_status_mgr
> *mgr,
> >>                               const struct sbrec_chassis *chassis_rec,
> >>                               struct hmap *tracked_datapath,
> >>                               bool sb_readonly);
> >> +void if_status_mgr_remove_ovn_installed(struct if_status_mgr *mgr,
> >> +                                        const char *name,
> >> +                                        const struct uuid *uuid);
> >>
> >>  # endif /* controller/if-status.h */
> >> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> >> index c094cb74d..2d8fee4d6 100644
> >> --- a/controller/ovn-controller.c
> >> +++ b/controller/ovn-controller.c
> >> @@ -5225,6 +5225,9 @@ main(int argc, char *argv[])
> >>
> stopwatch_start(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME,
> >>                                      time_msec());
> >>                      if_status_mgr_update(if_mgr, binding_data, chassis,
> >> +                                         ovsrec_interface_table_get(
> >> +                                                    ovs_idl_loop.idl),
> >> +                                         !ovs_idl_txn,
> >>                                           !ovnsb_idl_txn);
> >>                      stopwatch_stop(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME,
> >>                                     time_msec());
> >> @@ -5254,6 +5257,8 @@ main(int argc, char *argv[])
> >>                      stopwatch_start(IF_STATUS_MGR_RUN_STOPWATCH_NAME,
> >>                                      time_msec());
> >>                      if_status_mgr_run(if_mgr, binding_data, chassis,
> >> +                                      ovsrec_interface_table_get(
> >> +                                                  ovs_idl_loop.idl),
> >>                                        !ovnsb_idl_txn, !ovs_idl_txn);
> >>                      stopwatch_stop(IF_STATUS_MGR_RUN_STOPWATCH_NAME,
> >>                                     time_msec());
> >> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> >> index b46f67636..cae44edee 100644
> >> --- a/tests/system-ovn.at
> >> +++ b/tests/system-ovn.at
> >> @@ -10667,3 +10667,352 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query
> >> port patch-.*/d
> >>  /connection dropped.*/d"])
> >>  AT_CLEANUP
> >>  ])
> >> +
> >> +# This tests port->up/down and ovn-installed after adding and removing
> >> Ports and Interfaces.
> >> +# 3 Conditions x 3 tests:
> >> +# - 3 Conditions:
> >> +#   - In normal conditions
> >> +#   - Remove interface while starting and stopping SB and Controller
> >> +#   - Remove and add back interface while starting and stopping SB and
> >> Controller
> >> +# - 3 tests:
> >> +#   - Add/Remove Logical Port
> >> +#   - Add/Remove iface-id
> >> +#   - Add/Remove Interface
> >> +# Each tests/conditions checks for
> >> +# - Port_binding->chassis
> >> +# - Port up or down
> >> +# - ovn-installed
> >> +OVN_FOR_EACH_NORTHD([
> >> +AT_SETUP([ovn-install on slow ovsdb])
> >> +AT_KEYWORDS([ovn-install])
> >> +
> >> +OVS_TRAFFIC_VSWITCHD_START()
> >> +# Restart ovsdb-server, this time with tcp
> >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> >> +start_daemon ovsdb-server --remote=punix:"$OVS_RUNDIR"/db.sock
> >> --remote=ptcp:0:127.0.0.1
> >> +
> >> +ovn_start
> >> +ADD_BR([br-int])
> >> +
> >> +# Set external-ids in br-int needed for ovn-controller
> >> +check ovs-vsctl \
> >> +        -- set Open_vSwitch . external-ids:system-id=hv1 \
> >> +        -- set Open_vSwitch .
> >> external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
> >> +        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
> >> +        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
> >> +        -- set bridge br-int fail-mode=secure
> >> other-config:disable-in-band=true
> >> +
> >> +# Start ovn-controller
> >> +PARSE_LISTENING_PORT([$ovs_base/ovsdb-server.log], [TCP_PORT])
> >> +start_daemon ovn-controller tcp:127.0.0.1:$TCP_PORT
> >> +
> >> +check ovn-nbctl ls-add ls1
> >> +check ovn-nbctl set Logical_Switch ls1 other_config:subnet=10.1.0.0/16
> >> +
> >> +check ovn-nbctl --wait=hv sync
> >> +
> >> +add_logical_ports() {
> >> +  echo Adding logical ports
> >> +  check ovn-nbctl lsp-add ls1 lsp1
> >> +  check ovn-nbctl lsp-add ls1 lsp2
> >> +}
> >> +
> >> +remove_logical_ports() {
> >> +  echo Removing logical ports
> >> +  check ovn-nbctl lsp-del lsp1
> >> +  check ovn-nbctl lsp-del lsp2
> >> +}
> >> +
> >> +add_ovs_interface() {
> >> +  echo Adding interface $1 $2
> >> +  ovs-vsctl --no-wait -- add-port br-int $1 \
> >> +                      -- set Interface $1 external_ids:iface-id=$2 \
> >> +                      -- set Interface $1 type=internal
> >> +}
> >> +add_ovs_interfaces() {
> >> +  add_ovs_interface vif1 lsp1
> >> +  add_ovs_interface vif2 lsp2
> >> +}
> >> +remove_ovs_interface() {
> >> +  echo Removing interface $1
> >> +  check ovs-vsctl --no-wait -- del-port $1
> >> +}
> >> +remove_ovs_interfaces() {
> >> +  remove_ovs_interface vif1
> >> +  remove_ovs_interface vif2
> >> +}
> >> +add_iface_ids() {
> >> +  echo Adding iface-id vif1 lsp1
> >> +  ovs-vsctl --no-wait -- set Interface vif1 external_ids:iface-id=lsp1
> >> +  echo Adding iface-id vif2 lsp2
> >> +  ovs-vsctl --no-wait -- set Interface vif2 external_ids:iface-id=lsp2
> >> +}
> >> +remove_iface_id() {
> >> +  echo Removing iface-id $1
> >> +  check ovs-vsctl remove Interface $1 external_ids iface-id
> >> +}
> >> +remove_iface_ids() {
> >> +  remove_iface_id vif1
> >> +  remove_iface_id vif2
> >> +}
> >> +wait_for_local_bindings() {
> >> +  OVS_WAIT_UNTIL(
> >> +      [test `ovs-appctl -t ovn-controller debug/dump-local-bindings |
> >> grep interface | wc -l` -eq 2],
> >> +      [kill -CONT $(cat ovn-sb/ovsdb-server.pid)]
> >> +  )
> >> +}
> >> +sleep_sb() {
> >> +  echo SB going to sleep
> >> +  AT_CHECK([kill -STOP $(cat ovn-sb/ovsdb-server.pid)])
> >> +}
> >> +wake_up_sb() {
> >> +  echo SB waking up
> >> +  AT_CHECK([kill -CONT $(cat ovn-sb/ovsdb-server.pid)])
> >> +}
> >> +sleep_controller() {
> >> +  echo Controller going to sleep
> >> +  ovn-appctl debug/pause
> >> +  OVS_WAIT_UNTIL([test x$(ovn-appctl -t ovn-controller debug/status) =
> >> "xpaused"])
> >> +}
> >> +
> >> +stop_ovsdb_controller_updates() {
> >> +  TCP_PORT=$1
> >> +  echo Stopping updates from ovn-controller to ovsdb using port
> $TCP_PORT
> >> +  on_exit 'iptables -C INPUT -p tcp --destination-port $TCP_PORT -j
> DROP
> >> 2>/dev/null && iptables -D INPUT -p tcp --destination-port $TCP_PORT -j
> >> DROP'
> >> +  iptables -A INPUT -p tcp --destination-port $TCP_PORT -j DROP
> >> +}
> >> +restart_ovsdb_controller_updates() {
> >> +  TCP_PORT=$1
> >> +  echo Restarting updates from ovn-controller to ovsdb
> >> +  iptables -D INPUT -p tcp --destination-port $TCP_PORT  -j DROP
> >> +}
> >> +wake_up_controller() {
> >> +  echo Controller waking up
> >> +  ovn-appctl debug/resume
> >> +}
> >> +ensure_controller_run() {
> >> +# We want to make sure controller could run at least one full loop.
> >> +# We can't use wait=hv as sb might be sleeping.
> >> +# Use 2 ovn-appctl to guarentee that ovn-controller run the full loop,
> >> and not just the unixctl handling
> >> +  OVS_WAIT_UNTIL([test x$(ovn-appctl -t ovn-controller debug/status) =
> >> "xrunning"])
> >> +  OVS_WAIT_UNTIL([test x$(ovn-appctl -t ovn-controller debug/status) =
> >> "xrunning"])
> >> +}
> >> +sleep_ovsdb() {
> >> +  echo OVSDB going to sleep
> >> +  AT_CHECK([kill -STOP $(cat ovsdb-server.pid)])
> >> +}
> >> +wake_up_ovsdb() {
> >> +  echo OVSDB waking up
> >> +  AT_CHECK([kill -CONT $(cat ovsdb-server.pid)])
> >> +}
> >> +check_ovn_installed() {
> >> +  OVS_WAIT_UNTIL([test `ovs-vsctl get Interface vif1
> >> external_ids:ovn-installed` = '"true"'])
> >> +  OVS_WAIT_UNTIL([test `ovs-vsctl get Interface vif2
> >> external_ids:ovn-installed` = '"true"'])
> >> +}
> >> +check_ovn_uninstalled() {
> >> +  OVS_WAIT_UNTIL([test x`ovs-vsctl get Interface vif2
> >> external_ids:ovn-installed` = x])
> >> +  OVS_WAIT_UNTIL([test x`ovs-vsctl get Interface vif1
> >> external_ids:ovn-installed` = x])
> >> +}
> >> +check_ports_up() {
> >> +  OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp1 up` = 'true'])
> >> +  OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp2 up` = 'true'])
> >> +}
> >> +check_ports_down() {
> >> +  OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp1 up` = 'false'])
> >> +  OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp2 up` = 'false'])
> >> +}
> >> +
> >> +check_ports_bound() {
> >> +  ch=$(fetch_column Chassis _uuid name=hv1)
> >> +  wait_row_count Port_Binding 1 logical_port=lsp1 chassis=$ch
> >> +  wait_row_count Port_Binding 1 logical_port=lsp2 chassis=$ch
> >> +}
> >> +check_ports_unbound() {
> >> +  wait_column "" Port_Binding chassis logical_port=lsp1
> >> +  wait_column "" Port_Binding chassis logical_port=lsp2
> >> +}
> >> +add_logical_ports
> >> +add_ovs_interfaces
> >> +wait_for_local_bindings
> >> +wait_for_ports_up
> >> +check ovn-nbctl --wait=hv sync
> >> +############################################################
> >> +########## Remove interface while removing iface-id ########
> >> +############################################################
> >> +AS_BOX(["Remove interface while removing iface-id"])
> >> +stop_ovsdb_controller_updates $TCP_PORT
> >> +remove_iface_id vif1
> >> +ensure_controller_run
> >> +# OVSDB should be seen as ro now
> >> +remove_iface_id vif2
> >> +ensure_controller_run
> >> +# Controller delaying ovn-install removal for vif2 as ovsdb ro
> >> +sleep_controller
> >> +restart_ovsdb_controller_updates $TCP_PORT
> >> +remove_ovs_interface vif2
> >> +# vif2, for which we want to remove ovn-install, is deleted
> >> +wake_up_controller
> >> +check_ovn_uninstalled
> >> +check_ports_down
> >> +check_ports_unbound
> >> +add_ovs_interface vif2 lsp2
> >> +add_iface_ids
> >> +check_ovn_installed
> >> +check_ports_up
> >> +check_ports_bound
> >> +############################################################
> >> +################### Add/Remove iface-id ####################
> >> +############################################################
> >> +AS_BOX(["iface-id removal and added back (no sleeping sb or
> controller)"])
> >> +remove_iface_ids
> >> +check_ovn_uninstalled
> >> +check_ports_down
> >> +check_ports_unbound
> >> +add_iface_ids
> >> +check_ovn_installed
> >> +check_ports_up
> >> +check_ports_bound
> >> +
> >> +AS_BOX(["iface-id removal"])
> >> +sleep_sb
> >> +remove_iface_ids
> >> +ensure_controller_run
> >> +sleep_controller
> >> +wake_up_sb
> >> +wake_up_controller
> >> +check_ovn_uninstalled
> >> +# Port_down not always set on iface-id removal
> >> +# check_ports_down
> >> +# Port_Binding(chassis) not always removed on iface-id removal
> >> +# check_ports_unbound
> >> +add_iface_ids
> >> +check ovn-nbctl --wait=hv sync
> >> +
> >> +AS_BOX(["iface-id removal 2"])
> >> +# Block IDL from ovn-controller to OVSDB
> >> +stop_ovsdb_controller_updates $TCP_PORT
> >> +remove_iface_id vif2
> >> +ensure_controller_run
> >> +
> >> +# OVSDB should now be seen as read-only by ovn-controller
> >> +remove_iface_id vif1
> >> +ensure_controller_run
> >> +
> >> +# Restart connection from ovn-controller to OVSDB
> >> +restart_ovsdb_controller_updates $TCP_PORT
> >> +check_ovn_uninstalled
> >> +check_ports_down
> >> +check_ports_unbound
> >> +
> >> +add_iface_ids
> >> +check ovn-nbctl --wait=hv sync
> >> +
> >> +AS_BOX(["iface-id removal and added back"])
> >> +sleep_sb
> >> +remove_iface_ids
> >> +ensure_controller_run
> >> +sleep_controller
> >> +add_iface_ids
> >> +wake_up_sb
> >> +wake_up_controller
> >> +check_ovn_installed
> >> +check_ports_up
> >> +check_ports_bound
> >> +############################################################
> >> +###################### Add/Remove Interface ################
> >> +############################################################
> >> +AS_BOX(["Interface removal and added back (no sleeping sb or
> >> controller)"])
> >> +remove_ovs_interfaces
> >> +check_ovn_uninstalled
> >> +check_ports_down
> >> +check_ports_unbound
> >> +add_ovs_interfaces
> >> +check_ovn_installed
> >> +check_ports_up
> >> +check_ports_bound
> >> +check ovn-nbctl --wait=hv sync
> >> +
> >> +AS_BOX(["Interface removal"])
> >> +sleep_sb
> >> +remove_ovs_interfaces
> >> +ensure_controller_run
> >> +sleep_controller
> >> +wake_up_sb
> >> +wake_up_controller
> >> +check_ovn_uninstalled
> >> +# Port_down not always set on Interface removal
> >> +# check_ports_down
> >> +# Port_Binding(chassis) not always removed on Interface removal
> >> +# check_ports_unbound
> >> +add_ovs_interfaces
> >> +check ovn-nbctl --wait=hv sync
> >> +
> >> +AS_BOX(["Interface removal and added back"])
> >> +sleep_sb
> >> +remove_ovs_interfaces
> >> +ensure_controller_run
> >> +sleep_controller
> >> +add_ovs_interfaces
> >> +wake_up_sb
> >> +wake_up_controller
> >> +check_ovn_installed
> >> +check_ports_up
> >> +check_ports_bound
> >> +check ovn-nbctl --wait=hv sync
> >> +############################################################
> >> +###################### Add/Remove Logical Port #############
> >> +############################################################
> >> +AS_BOX(["Logical port removal and added back (no sleeping sb or
> >> controller)"])
> >> +remove_logical_ports
> >> +check_ovn_uninstalled
> >> +check_ports_unbound
> >> +sleep_ovsdb
> >> +add_logical_ports
> >> +ensure_controller_run
> >> +wake_up_ovsdb
> >> +check_ovn_installed
> >> +check_ports_up
> >> +check_ports_bound
> >> +check ovn-nbctl --wait=hv sync
> >> +
> >> +AS_BOX(["Logical port removal"])
> >> +sleep_sb
> >> +remove_logical_ports
> >> +ensure_controller_run
> >> +sleep_controller
> >> +wake_up_sb
> >> +wake_up_controller
> >> +check_ovn_uninstalled
> >> +check_ports_unbound
> >> +add_logical_ports
> >> +check ovn-nbctl --wait=hv sync
> >> +
> >> +AS_BOX(["Logical port removal and added back"])
> >> +sleep_sb
> >> +remove_logical_ports
> >> +ensure_controller_run
> >> +sleep_controller
> >> +add_logical_ports
> >> +wake_up_sb
> >> +wake_up_controller
> >> +check_ovn_installed
> >> +check_ports_up
> >> +check_ports_bound
> >> +
> >> +OVS_APP_EXIT_AND_WAIT([ovn-controller])
> >> +
> >> +as ovn-sb
> >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> >> +
> >> +as ovn-nb
> >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> >> +
> >> +as northd
> >> +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE])
> >> +
> >> +as
> >> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
> >> +/connection dropped.*/d"])
> >> +AT_CLEANUP
> >> +])
> >> +
> >> --
> >> 2.31.1
> >>
> >>
> > Looks good to me, thanks.
> >
> > Reviewed-by: Ales Musil <amusil@redhat.com>
> >
>
> Thanks, Xavier and Ales!
>
> I applied this to the main branch and backported it to stable branches
> down to 22.09.  Xavier, the patch didn't apply cleanly to 22.06 and
> 22.03.  Do you have time to prepare a custom backport?  Otherwise I'll
> look into it.
>
> Regards,
> Dumitru
>
>