[ovs-dev,v3,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
---
 controller/binding.c        |  81 ++++++++-
 controller/binding.h        |   9 +
 controller/if-status.c      | 199 ++++++++++++++++++----
 controller/if-status.h      |   6 +
 controller/ovn-controller.c |   3 +
 tests/system-ovn.at         | 321 ++++++++++++++++++++++++++++++++++++
 6 files changed, 587 insertions(+), 32 deletions(-)

As a note:
The newly added system test sometimes fails as it is finding some warnings
in the controller.log (e.g.
2023-04-19T13:18:29.536Z|00087|if_status|WARN|Trying to release unknown
interface lsp1)

This can happen in the following scenario:
- logical port is created
- OVS interfaces is added w/ iface-id
- Port is claimed, setting pb->chassis in SB. SB very slow
- Removing OVS interfaces
- iface deleted in if-status (if_status_mgr_release_iface destroying the
iface in CLAIMED state)
- pb->chassis update received from sb => port released, iface released from
iface-status

This issue existed before this patch and has been fixed in the next patch.

On Wed, Apr 19, 2023 at 2:41 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
> ---
>  controller/binding.c        |  81 ++++++++-
>  controller/binding.h        |   9 +
>  controller/if-status.c      | 199 ++++++++++++++++++----
>  controller/if-status.h      |   6 +
>  controller/ovn-controller.c |   3 +
>  tests/system-ovn.at         | 321 ++++++++++++++++++++++++++++++++++++
>  6 files changed, 587 insertions(+), 32 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 5df62baef..4e79c1c87 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,51 @@ local_binding_set_up(struct shash *local_bindings,
> const char *pb_name,
>      }
>  }
>
> +static void
> +remove_ovn_installed(struct local_binding *lbinding, const char *pb_name)
> +{
> +    if (lbinding && lbinding->iface &&
> +        smap_get_bool(&lbinding->iface->external_ids,
> +                      OVN_INSTALLED_EXT_ID, false)) {
> +        /* If iface has been deleted, do not try to delete a key from it
> */
> +        if (!ovsrec_interface_is_deleted(lbinding->iface)) {
> +            VLOG_INFO("Removing lport %s ovn-installed in OVS", pb_name);
> +            ovsrec_interface_update_external_ids_delkey(lbinding->iface,
> +
> OVN_INSTALLED_EXT_ID);
> +        }
> +    }
> +}
> +
> +void
> +local_binding_remove_ovn_installed(struct shash *local_bindings,
> +                                   const char *pb_name, bool ovs_readonly)
> +{
> +    if (ovs_readonly) {
> +        return;
> +    }
> +    struct local_binding *lbinding =
> +        local_binding_find(local_bindings, pb_name);
> +    remove_ovn_installed(lbinding, pb_name);
> +}
> +
> +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)) {
> +        /* If iface has been deleted, do not try to delete a key from it
> */
> +        if (!ovsrec_interface_is_deleted(iface_rec)) {
> +            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 +1298,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 +1525,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 +2351,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 +2626,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 +2650,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 +2697,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..fdf59b813 100644
> --- a/controller/binding.h
> +++ b/controller/binding.h
> @@ -159,6 +159,12 @@ 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 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 +201,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..7caa65aca 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,7 +193,10 @@ 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);
>
> @@ -179,6 +205,7 @@ static void if_status_mgr_update_bindings(
>      const struct sbrec_chassis *,
>      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 +216,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 +230,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 +245,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 +272,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 +309,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 +341,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 +383,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 +417,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,6 +519,18 @@ 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,
> @@ -471,7 +552,19 @@ 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);
>
> @@ -492,6 +585,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 +611,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 +636,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)
> @@ -558,7 +690,16 @@ 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->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..6341b5dc6 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,6 +36,8 @@ 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 *,
> @@ -48,5 +51,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 e170e9262..8ddc21f5e 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -5243,6 +5243,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());
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 96d1c295e..bf15d2aac 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -10784,3 +10784,324 @@ 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_interfaces() {
> +  echo Adding interfaces
> +  ovs-vsctl --no-wait -- add-port br-int vif1 \
> +                      -- set Interface vif1 external_ids:iface-id=lsp1 \
> +                      -- set Interface vif1 type=internal
> +  ovs-vsctl --no-wait -- add-port br-int vif2 \
> +                      -- set Interface vif2 external_ids:iface-id=lsp2 \
> +                      -- set Interface vif2 type=internal
> +}
> +remove_ovs_interfaces() {
> +  echo Removing interfaces
> +  check ovs-vsctl --no-wait -- del-port vif1
> +  check ovs-vsctl --no-wait -- del-port vif2
> +}
> +add_iface_ids() {
> +  echo Adding back iface-id
> +  ovs-vsctl --no-wait -- set Interface vif1 external_ids:iface-id=lsp1
> +  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() {
> +  echo Removing iface-id
> +  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
> +############################################################
> +################### 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
>
>

On Wed, Apr 19, 2023 at 2:41 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>
>

Hi Xavier,

I have two comments down below, other than that it looks good.


>
> ---
> 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
> ---
>  controller/binding.c        |  81 ++++++++-
>  controller/binding.h        |   9 +
>  controller/if-status.c      | 199 ++++++++++++++++++----
>  controller/if-status.h      |   6 +
>  controller/ovn-controller.c |   3 +
>  tests/system-ovn.at         | 321 ++++++++++++++++++++++++++++++++++++
>  6 files changed, 587 insertions(+), 32 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 5df62baef..4e79c1c87 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,51 @@ local_binding_set_up(struct shash *local_bindings,
> const char *pb_name,
>      }
>  }
>
> +static void
> +remove_ovn_installed(struct local_binding *lbinding, const char *pb_name)
> +{
> +    if (lbinding && lbinding->iface &&
> +        smap_get_bool(&lbinding->iface->external_ids,
> +                      OVN_INSTALLED_EXT_ID, false)) {
> +        /* If iface has been deleted, do not try to delete a key from it
> */
> +        if (!ovsrec_interface_is_deleted(lbinding->iface)) {
>

This call is not "safe" to use outside of a tracked loop AFAIU. Maybe it
could be actually
replaced with the updated "remove_ovn_installed_for_uuid". WDYT?


> +            VLOG_INFO("Removing lport %s ovn-installed in OVS", pb_name);
> +            ovsrec_interface_update_external_ids_delkey(lbinding->iface,
> +
> OVN_INSTALLED_EXT_ID);
> +        }
> +    }
> +}
> +
> +void
> +local_binding_remove_ovn_installed(struct shash *local_bindings,
> +                                   const char *pb_name, bool ovs_readonly)
> +{
> +    if (ovs_readonly) {
> +        return;
> +    }
> +    struct local_binding *lbinding =
> +        local_binding_find(local_bindings, pb_name);
> +    remove_ovn_installed(lbinding, pb_name);
> +}
> +
> +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)) {
> +        /* If iface has been deleted, do not try to delete a key from it
> */
> +        if (!ovsrec_interface_is_deleted(iface_rec)) {
>

Same as before this check is not "safe" to do outside of the tracked loop
AFAIU.
But IMO it is not needed. If the interface would be deleted the
"ovsrec_interface_table_get_for_uuid"
should return NULL so in this case this check is redundant.


> +            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 +1298,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 +1525,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 +2351,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 +2626,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 +2650,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 +2697,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..fdf59b813 100644
> --- a/controller/binding.h
> +++ b/controller/binding.h
> @@ -159,6 +159,12 @@ 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 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 +201,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..7caa65aca 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,7 +193,10 @@ 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);
>
> @@ -179,6 +205,7 @@ static void if_status_mgr_update_bindings(
>      const struct sbrec_chassis *,
>      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 +216,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 +230,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 +245,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 +272,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 +309,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 +341,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 +383,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 +417,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,6 +519,18 @@ 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,
> @@ -471,7 +552,19 @@ 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);
>
> @@ -492,6 +585,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 +611,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 +636,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)
> @@ -558,7 +690,16 @@ 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->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..6341b5dc6 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,6 +36,8 @@ 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 *,
> @@ -48,5 +51,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 e170e9262..8ddc21f5e 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -5243,6 +5243,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());
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 96d1c295e..bf15d2aac 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -10784,3 +10784,324 @@ 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_interfaces() {
> +  echo Adding interfaces
> +  ovs-vsctl --no-wait -- add-port br-int vif1 \
> +                      -- set Interface vif1 external_ids:iface-id=lsp1 \
> +                      -- set Interface vif1 type=internal
> +  ovs-vsctl --no-wait -- add-port br-int vif2 \
> +                      -- set Interface vif2 external_ids:iface-id=lsp2 \
> +                      -- set Interface vif2 type=internal
> +}
> +remove_ovs_interfaces() {
> +  echo Removing interfaces
> +  check ovs-vsctl --no-wait -- del-port vif1
> +  check ovs-vsctl --no-wait -- del-port vif2
> +}
> +add_iface_ids() {
> +  echo Adding back iface-id
> +  ovs-vsctl --no-wait -- set Interface vif1 external_ids:iface-id=lsp1
> +  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() {
> +  echo Removing iface-id
> +  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
> +############################################################
> +################### 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
>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Thanks,
Ales