[ovs-dev,2/2] if-status: Add OVS interface status management module.

The initial implementation of the notification mechanism that indicates
if OVS flows corresponding to a logical switch port have been installed
is not resilient enough.  It didn't cover the case when transactions to
the local OVS DB or to the Southbound fail.

In order to deal with that, factor out the code that tracks the state
of the interfaces to a new module, 'if-status', and implement it with
a state machine that will retry setting Port_Binding.UP and
OVS.Interface.external_ids:ovn-installed in the Southbound and local
OVS databases.

Having a separate module makes sense because tracking the interface
state doesn't really depend on the rest of the binding module, except
for interacting with the Port_Binding and Interface IDL records.  For
this we add some additional APIs to binding.c.

Reported-at: https://bugzilla.redhat.com/1952846
Fixes: 4d3cb42b076b ("binding: Set Logical_Switch_Port.up when all OVS flows are installed.")
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
---
 controller/automake.mk      |    2 
 controller/binding.c        |  302 +++++++++++----------------------
 controller/binding.h        |   14 +-
 controller/if-status.c      |  395 +++++++++++++++++++++++++++++++++++++++++++
 controller/if-status.h      |   37 ++++
 controller/ovn-controller.c |   24 ++-
 6 files changed, 562 insertions(+), 212 deletions(-)
 create mode 100644 controller/if-status.c
 create mode 100644 controller/if-status.h

On Fri, Apr 23, 2021 at 10:18 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> The initial implementation of the notification mechanism that indicates
> if OVS flows corresponding to a logical switch port have been installed
> is not resilient enough.  It didn't cover the case when transactions to
> the local OVS DB or to the Southbound fail.
>
> In order to deal with that, factor out the code that tracks the state
> of the interfaces to a new module, 'if-status', and implement it with
> a state machine that will retry setting Port_Binding.UP and
> OVS.Interface.external_ids:ovn-installed in the Southbound and local
> OVS databases.
>
> Having a separate module makes sense because tracking the interface
> state doesn't really depend on the rest of the binding module, except
> for interacting with the Port_Binding and Interface IDL records.  For
> this we add some additional APIs to binding.c.
>
> Reported-at: https://bugzilla.redhat.com/1952846
> Fixes: 4d3cb42b076b ("binding: Set Logical_Switch_Port.up when all OVS flows are installed.")
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>

Hi Dumitru,

Thanks for fixing this issue.

During my testing, I found a regression.  If suppose an ovs port
"sw0p1" is bound on
a chassis for the logical port - sw0-port1, and if I delete the ovs port sw0p1,
ovn-controller clears the chassis column of Port_Binding but it
doesn't set the "up" column to "false".
It still remains "true".

All the tests passed for me locally but still I see this issue when I
ran in fake-multinode setup.

Can you please check if there are existing test cases which check for
the "up" column of port binding to be
false when it is released ?


I would also suggest adding a few test cases to replicate the issue it
is trying to address. If possible.

To test this scenario, I have one idea.  Make the SB ovsdb-server read-only
(ovn-appctl -t ovnsb_db.ctl ovsdb-server/set-active-ovsdb-server
tcp:192.0.0.1:6642 and
ovn-appctl -t ovnsb_db.ctl ovsdb-server/connect-active-ovsdb-server)
after a port is claimed, and then run ovs-vsctl commands to release
the ovs port. At this point ovn-controller will fail
to set the "chassis" and "up" column.  After a few seconds, make the
ovsdb-server active
and verify that the "chassis" and "up" columns are set properly.

Is this possible to do ?  Or is this a wrong configuration ? Because
ovn-controller can not come to
know when ovsdb-server becomes active. Unless it keeps trying continuously.

In my testing, ovn-controller doesn't set it properly when I make the
ovsdb-server active again.

What do you think ?

Thanks
Numan



> ---
>  controller/automake.mk      |    2
>  controller/binding.c        |  302 +++++++++++----------------------
>  controller/binding.h        |   14 +-
>  controller/if-status.c      |  395 +++++++++++++++++++++++++++++++++++++++++++
>  controller/if-status.h      |   37 ++++
>  controller/ovn-controller.c |   24 ++-
>  6 files changed, 562 insertions(+), 212 deletions(-)
>  create mode 100644 controller/if-status.c
>  create mode 100644 controller/if-status.h
>
> diff --git a/controller/automake.mk b/controller/automake.mk
> index e664f1980..2f6c50890 100644
> --- a/controller/automake.mk
> +++ b/controller/automake.mk
> @@ -10,6 +10,8 @@ controller_ovn_controller_SOURCES = \
>         controller/encaps.h \
>         controller/ha-chassis.c \
>         controller/ha-chassis.h \
> +       controller/if-status.c \
> +       controller/if-status.h \
>         controller/ip-mcast.c \
>         controller/ip-mcast.h \
>         controller/lflow.c \
> diff --git a/controller/binding.c b/controller/binding.c
> index 451f00e34..3baf4cf17 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -16,13 +16,14 @@
>  #include <config.h>
>  #include "binding.h"
>  #include "ha-chassis.h"
> +#include "if-status.h"
>  #include "lflow.h"
>  #include "lport.h"
> -#include "ofctrl-seqno.h"
>  #include "patch.h"
>
>  #include "lib/bitmap.h"
>  #include "openvswitch/poll-loop.h"
> +#include "lib/hmapx.h"
>  #include "lib/sset.h"
>  #include "lib/util.h"
>  #include "lib/netdev.h"
> @@ -41,32 +42,6 @@ VLOG_DEFINE_THIS_MODULE(binding);
>   */
>  #define OVN_INSTALLED_EXT_ID "ovn-installed"
>
> -/* Set of OVS interface IDs that have been released in the most recent
> - * processing iterations.  This gets updated in release_lport() and is
> - * periodically emptied in binding_seqno_run().
> - */
> -static struct sset binding_iface_released_set =
> -    SSET_INITIALIZER(&binding_iface_released_set);
> -
> -/* Set of OVS interface IDs that have been bound in the most recent
> - * processing iterations.  This gets updated in release_lport() and is
> - * periodically emptied in binding_seqno_run().
> - */
> -static struct sset binding_iface_bound_set =
> -    SSET_INITIALIZER(&binding_iface_bound_set);
> -
> -static void
> -binding_iface_released_add(const char *iface_id)
> -{
> -    sset_add(&binding_iface_released_set, iface_id);
> -}
> -
> -static void
> -binding_iface_bound_add(const char *iface_id)
> -{
> -    sset_add(&binding_iface_bound_set, iface_id);
> -}
> -
>  #define OVN_QOS_TYPE "linux-htb"
>
>  struct qos_queue {
> @@ -672,7 +647,8 @@ static void local_binding_destroy(struct local_binding *,
>                                    struct shash *binding_lports);
>  static void local_binding_delete(struct local_binding *,
>                                   struct shash *local_bindings,
> -                                 struct shash *binding_lports);
> +                                 struct shash *binding_lports,
> +                                 struct if_status_mgr *if_mgr);
>  static struct binding_lport *local_binding_add_lport(
>      struct shash *binding_lports,
>      struct local_binding *,
> @@ -739,6 +715,80 @@ local_binding_get_primary_pb(struct shash *local_bindings, const char *pb_name)
>      return b_lport ? b_lport->pb : NULL;
>  }
>
> +bool
> +local_binding_is_up(struct shash *local_bindings, const char *pb_name)
> +{
> +    struct local_binding *lbinding =
> +        local_binding_find(local_bindings, pb_name);
> +    struct binding_lport *b_lport = local_binding_get_primary_lport(lbinding);
> +    if (lbinding && b_lport && lbinding->iface) {
> +        if (b_lport->pb->n_up && !b_lport->pb->up[0]) {
> +            return false;
> +        }
> +        return smap_get_bool(&lbinding->iface->external_ids,
> +                             OVN_INSTALLED_EXT_ID, false);
> +    }
> +    return false;
> +}
> +
> +bool
> +local_binding_is_down(struct shash *local_bindings, const char *pb_name)
> +{
> +    struct local_binding *lbinding =
> +        local_binding_find(local_bindings, pb_name);
> +
> +    return !lbinding
> +           || !lbinding->iface
> +           || !smap_get_bool(&lbinding->iface->external_ids,
> +                            OVN_INSTALLED_EXT_ID, false);
> +}
> +
> +void
> +local_binding_set_up(struct shash *local_bindings, const char *pb_name,
> +                     bool sb_readonly, bool ovs_readonly)
> +{
> +    struct local_binding *lbinding =
> +        local_binding_find(local_bindings, pb_name);
> +    struct binding_lport *b_lport = local_binding_get_primary_lport(lbinding);
> +    if (lbinding && b_lport && lbinding->iface) {
> +        if (!ovs_readonly && !smap_get_bool(&lbinding->iface->external_ids,
> +                                            OVN_INSTALLED_EXT_ID, false)) {
> +            ovsrec_interface_update_external_ids_setkey(lbinding->iface,
> +                                                        OVN_INSTALLED_EXT_ID,
> +                                                        "true");
> +        }
> +        if (!sb_readonly && b_lport->pb->n_up) {
> +            bool up = true;
> +
> +            sbrec_port_binding_set_up(b_lport->pb, &up, 1);
> +            LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) {
> +                sbrec_port_binding_set_up(b_lport->pb, &up, 1);
> +            }
> +        }
> +    }
> +}
> +
> +void
> +local_binding_set_down(struct shash *local_bindings, const char *pb_name,
> +                       bool sb_readonly, bool ovs_readonly)
> +{
> +    struct local_binding *lbinding =
> +        local_binding_find(local_bindings, pb_name);
> +    struct binding_lport *b_lport = local_binding_get_primary_lport(lbinding);
> +
> +    if (!ovs_readonly && lbinding && lbinding->iface
> +            && smap_get_bool(&lbinding->iface->external_ids,
> +                             OVN_INSTALLED_EXT_ID, false)) {
> +        ovsrec_interface_update_external_ids_delkey(lbinding->iface,
> +                                                    OVN_INSTALLED_EXT_ID);
> +    }
> +
> +    if (!sb_readonly && b_lport && b_lport->pb->n_up) {
> +        bool up = false;
> +        sbrec_port_binding_set_up(b_lport->pb, &up, 1);
> +    }
> +}
> +
>  void
>  binding_dump_local_bindings(struct local_binding_data *lbinding_data,
>                              struct ds *out_data)
> @@ -959,7 +1009,7 @@ static void
>  claimed_lport_set_up(const struct sbrec_port_binding *pb,
>                       const struct sbrec_port_binding *parent_pb,
>                       const struct sbrec_chassis *chassis_rec,
> -                     bool notify_up)
> +                     bool notify_up, struct if_status_mgr *if_mgr)
>  {
>      if (!notify_up) {
>          bool up = true;
> @@ -970,7 +1020,7 @@ claimed_lport_set_up(const struct sbrec_port_binding *pb,
>      }
>
>      if (pb->chassis != chassis_rec || (pb->n_up && !pb->up[0])) {
> -        binding_iface_bound_add(pb->logical_port);
> +        if_status_mgr_claim_iface(if_mgr, pb->logical_port);
>      }
>  }
>
> @@ -983,10 +1033,11 @@ claim_lport(const struct sbrec_port_binding *pb,
>              const struct sbrec_chassis *chassis_rec,
>              const struct ovsrec_interface *iface_rec,
>              bool sb_readonly, bool notify_up,
> -            struct hmap *tracked_datapaths)
> +            struct hmap *tracked_datapaths,
> +            struct if_status_mgr *if_mgr)
>  {
>      if (!sb_readonly) {
> -        claimed_lport_set_up(pb, parent_pb, chassis_rec, notify_up);
> +        claimed_lport_set_up(pb, parent_pb, chassis_rec, notify_up, if_mgr);
>      }
>
>      if (pb->chassis != chassis_rec) {
> @@ -1034,7 +1085,7 @@ claim_lport(const struct sbrec_port_binding *pb,
>   */
>  static bool
>  release_lport(const struct sbrec_port_binding *pb, bool sb_readonly,
> -              struct hmap *tracked_datapaths)
> +              struct hmap *tracked_datapaths, struct if_status_mgr *if_mgr)
>  {
>      if (pb->encap) {
>          if (sb_readonly) {
> @@ -1057,12 +1108,8 @@ release_lport(const struct sbrec_port_binding *pb, bool sb_readonly,
>          sbrec_port_binding_set_virtual_parent(pb, NULL);
>      }
>
> -    if (pb->n_up) {
> -        bool up = false;
> -        sbrec_port_binding_set_up(pb, &up, 1);
> -    }
>      update_lport_tracking(pb, tracked_datapaths);
> -    binding_iface_released_add(pb->logical_port);
> +    if_status_mgr_release_iface(if_mgr, pb->logical_port);
>      VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port);
>      return true;
>  }
> @@ -1113,7 +1160,8 @@ release_binding_lport(const struct sbrec_chassis *chassis_rec,
>      if (is_binding_lport_this_chassis(b_lport, chassis_rec)) {
>          remove_local_lport_ids(b_lport->pb, b_ctx_out);
>          if (!release_lport(b_lport->pb, sb_readonly,
> -            b_ctx_out->tracked_dp_bindings)) {
> +                           b_ctx_out->tracked_dp_bindings,
> +                           b_ctx_out->if_mgr)) {
>              return false;
>          }
>      }
> @@ -1140,7 +1188,8 @@ consider_vif_lport_(const struct sbrec_port_binding *pb,
>              if (!claim_lport(pb, parent_pb, b_ctx_in->chassis_rec,
>                               b_lport->lbinding->iface,
>                               !b_ctx_in->ovnsb_idl_txn,
> -                             !parent_pb, b_ctx_out->tracked_dp_bindings)){
> +                             !parent_pb, b_ctx_out->tracked_dp_bindings,
> +                             b_ctx_out->if_mgr)){
>                  return false;
>              }
>
> @@ -1171,7 +1220,8 @@ consider_vif_lport_(const struct sbrec_port_binding *pb,
>          /* Release the lport if there is no lbinding. */
>          if (!lbinding_set || !can_bind) {
>              return release_lport(pb, !b_ctx_in->ovnsb_idl_txn,
> -                                 b_ctx_out->tracked_dp_bindings);
> +                                 b_ctx_out->tracked_dp_bindings,
> +                                 b_ctx_out->if_mgr);
>          }
>      }
>
> @@ -1269,7 +1319,8 @@ consider_container_lport(const struct sbrec_port_binding *pb,
>          if (is_binding_lport_this_chassis(container_b_lport,
>                                            b_ctx_in->chassis_rec)) {
>              return release_lport(pb, !b_ctx_in->ovnsb_idl_txn,
> -                                 b_ctx_out->tracked_dp_bindings);
> +                                 b_ctx_out->tracked_dp_bindings,
> +                                 b_ctx_out->if_mgr);
>          }
>
>          return true;
> @@ -1367,10 +1418,12 @@ consider_nonvif_lport_(const struct sbrec_port_binding *pb,
>          update_local_lport_ids(pb, b_ctx_out);
>          return claim_lport(pb, NULL, b_ctx_in->chassis_rec, NULL,
>                             !b_ctx_in->ovnsb_idl_txn, false,
> -                           b_ctx_out->tracked_dp_bindings);
> +                           b_ctx_out->tracked_dp_bindings,
> +                           b_ctx_out->if_mgr);
>      } else if (pb->chassis == b_ctx_in->chassis_rec) {
>          return release_lport(pb, !b_ctx_in->ovnsb_idl_txn,
> -                             b_ctx_out->tracked_dp_bindings);
> +                             b_ctx_out->tracked_dp_bindings,
> +                             b_ctx_out->if_mgr);
>      }
>
>      return true;
> @@ -1978,7 +2031,8 @@ consider_iface_release(const struct ovsrec_interface *iface_rec,
>      /* Check if the lbinding has children of type PB_CONTAINER.
>       * If so, don't delete the local_binding. */
>      if (lbinding && !is_lbinding_container_parent(lbinding)) {
> -        local_binding_delete(lbinding, local_bindings, binding_lports);
> +        local_binding_delete(lbinding, local_bindings, binding_lports,
> +                             b_ctx_out->if_mgr);
>      }
>
>      remove_local_lports(iface_id, b_ctx_out);
> @@ -2533,160 +2587,6 @@ delete_done:
>      return handled;
>  }
>
> -/* Registered ofctrl seqno type for port_binding flow installation. */
> -static size_t binding_seq_type_pb_cfg;
> -
> -/* Binding specific seqno to be acked by ofctrl when flows for new interfaces
> - * have been installed.
> - */
> -static uint32_t binding_iface_seqno = 0;
> -
> -/* Map indexed by iface-id containing the sequence numbers that when acked
> - * indicate that the OVS flows for the iface-id have been installed.
> - */
> -static struct simap binding_iface_seqno_map =
> -    SIMAP_INITIALIZER(&binding_iface_seqno_map);
> -
> -void
> -binding_init(void)
> -{
> -    binding_seq_type_pb_cfg = ofctrl_seqno_add_type();
> -}
> -
> -/* Processes new release/bind operations OVN ports.  For newly bound ports
> - * it creates ofctrl seqno update requests that will be acked when
> - * corresponding OVS flows have been installed.
> - *
> - * NOTE: Should be called only when valid SB and OVS transactions are
> - * available.
> - */
> -void
> -binding_seqno_run(struct local_binding_data *lbinding_data)
> -{
> -    const char *iface_id;
> -    const char *iface_id_next;
> -    struct shash *local_bindings = &lbinding_data->bindings;
> -    SSET_FOR_EACH_SAFE (iface_id, iface_id_next, &binding_iface_released_set) {
> -        struct shash_node *lb_node = shash_find(local_bindings, iface_id);
> -
> -        /* If the local binding still exists (i.e., the OVS interface is
> -         * still configured locally) then remove the external id and remove
> -         * it from the in-flight seqno map.
> -         */
> -        if (lb_node) {
> -            struct local_binding *lb = lb_node->data;
> -
> -            if (lb->iface && smap_get(&lb->iface->external_ids,
> -                                      OVN_INSTALLED_EXT_ID)) {
> -                ovsrec_interface_update_external_ids_delkey(
> -                    lb->iface, OVN_INSTALLED_EXT_ID);
> -            }
> -        }
> -        simap_find_and_delete(&binding_iface_seqno_map, iface_id);
> -        sset_delete(&binding_iface_released_set,
> -                    SSET_NODE_FROM_NAME(iface_id));
> -    }
> -
> -    bool new_ifaces = false;
> -    uint32_t new_seqno = binding_iface_seqno + 1;
> -
> -    SSET_FOR_EACH_SAFE (iface_id, iface_id_next, &binding_iface_bound_set) {
> -        struct shash_node *lb_node = shash_find(local_bindings, iface_id);
> -
> -        struct local_binding *lb = lb_node ? lb_node->data : NULL;
> -
> -        /* Make sure the binding is still complete, i.e., both SB port_binding
> -         * and OVS interface still exist.
> -         *
> -         * If so, then this is a newly bound interface, make sure we reset the
> -         * Port_Binding 'up' field and the OVS Interface 'external-id'.
> -         */
> -        struct binding_lport *b_lport = local_binding_get_primary_lport(lb);
> -        if (lb && b_lport && lb->iface
> -                && !simap_contains(&binding_iface_seqno_map, lb->name)) {
> -            new_ifaces = true;
> -
> -            if (smap_get(&lb->iface->external_ids, OVN_INSTALLED_EXT_ID)) {
> -                ovsrec_interface_update_external_ids_delkey(
> -                    lb->iface, OVN_INSTALLED_EXT_ID);
> -            }
> -            if (b_lport->pb->n_up) {
> -                bool up = false;
> -                sbrec_port_binding_set_up(b_lport->pb, &up, 1);
> -            }
> -            simap_put(&binding_iface_seqno_map, lb->name, new_seqno);
> -        }
> -        sset_delete(&binding_iface_bound_set, SSET_NODE_FROM_NAME(iface_id));
> -    }
> -
> -    /* Request a seqno update when the flows for new interfaces have been
> -     * installed in OVS.
> -     */
> -    if (new_ifaces) {
> -        binding_iface_seqno = new_seqno;
> -        ofctrl_seqno_update_create(binding_seq_type_pb_cfg, new_seqno);
> -    }
> -}
> -
> -/* Processes ofctrl seqno ACKs for new bindings.  Sets the
> - * 'OVN_INSTALLED_EXT_ID' external-id in the OVS interface and the
> - * Port_Binding.up field for all ports for which OVS flows have been
> - * installed.
> - *
> - * NOTE: Should be called only when valid SB and OVS transactions are
> - * available.
> - */
> -void
> -binding_seqno_install(struct local_binding_data *lbinding_data)
> -{
> -    struct ofctrl_acked_seqnos *acked_seqnos =
> -            ofctrl_acked_seqnos_get(binding_seq_type_pb_cfg);
> -    struct simap_node *node;
> -    struct simap_node *node_next;
> -    struct shash *local_bindings = &lbinding_data->bindings;
> -
> -    SIMAP_FOR_EACH_SAFE (node, node_next, &binding_iface_seqno_map) {
> -        struct shash_node *lb_node = shash_find(local_bindings, node->name);
> -
> -        if (!lb_node) {
> -            goto del_seqno;
> -        }
> -
> -        struct local_binding *lb = lb_node->data;
> -        struct binding_lport *b_lport = local_binding_get_primary_lport(lb);
> -        if (!b_lport || !lb->iface) {
> -            goto del_seqno;
> -        }
> -
> -        if (!ofctrl_acked_seqnos_contains(acked_seqnos, node->data)) {
> -            continue;
> -        }
> -
> -        ovsrec_interface_update_external_ids_setkey(lb->iface,
> -                                                    OVN_INSTALLED_EXT_ID,
> -                                                    "true");
> -        if (b_lport->pb->n_up) {
> -            bool up = true;
> -
> -            sbrec_port_binding_set_up(b_lport->pb, &up, 1);
> -            LIST_FOR_EACH (b_lport, list_node, &lb->binding_lports) {
> -                sbrec_port_binding_set_up(b_lport->pb, &up, 1);
> -            }
> -        }
> -
> -del_seqno:
> -        simap_delete(&binding_iface_seqno_map, node);
> -    }
> -
> -    ofctrl_acked_seqnos_destroy(acked_seqnos);
> -}
> -
> -void
> -binding_seqno_flush(void)
> -{
> -    simap_clear(&binding_iface_seqno_map);
> -}
> -
>  /* Static functions for local_lbindind and binding_lport. */
>  static struct local_binding *
>  local_binding_create(const char *name, const struct ovsrec_interface *iface)
> @@ -2728,9 +2628,11 @@ local_binding_destroy(struct local_binding *lbinding,
>  static void
>  local_binding_delete(struct local_binding *lbinding,
>                       struct shash *local_bindings,
> -                     struct shash *binding_lports)
> +                     struct shash *binding_lports,
> +                     struct if_status_mgr *if_mgr)
>  {
>      shash_find_and_delete(local_bindings, lbinding->name);
> +    if_status_mgr_delete_iface(if_mgr, lbinding->name);
>      local_binding_destroy(lbinding, binding_lports);
>  }
>
> diff --git a/controller/binding.h b/controller/binding.h
> index 4fc9ef207..4f733426b 100644
> --- a/controller/binding.h
> +++ b/controller/binding.h
> @@ -37,6 +37,7 @@ struct sbrec_port_binding_table;
>  struct sset;
>  struct sbrec_port_binding;
>  struct ds;
> +struct if_status_mgr;
>
>  struct binding_ctx_in {
>      struct ovsdb_idl_txn *ovnsb_idl_txn;
> @@ -85,6 +86,8 @@ struct binding_ctx_out {
>       * binding_handle_port_binding_changes) fills in for
>       * the changed datapaths and port bindings. */
>      struct hmap *tracked_dp_bindings;
> +
> +    struct if_status_mgr *if_mgr;
>  };
>
>  struct local_binding_data {
> @@ -97,6 +100,12 @@ void local_binding_data_destroy(struct local_binding_data *);
>
>  const struct sbrec_port_binding *local_binding_get_primary_pb(
>      struct shash *local_bindings, const char *pb_name);
> +bool local_binding_is_up(struct shash *local_bindings, const char *pb_name);
> +bool local_binding_is_down(struct shash *local_bindings, const char *pb_name);
> +void local_binding_set_up(struct shash *local_bindings, const char *pb_name,
> +                          bool sb_readonly, bool ovs_readonly);
> +void local_binding_set_down(struct shash *local_bindings, const char *pb_name,
> +                            bool sb_readonly, bool ovs_readonly);
>
>  /* Represents a tracked binding logical port. */
>  struct tracked_binding_lport {
> @@ -123,9 +132,6 @@ bool binding_handle_port_binding_changes(struct binding_ctx_in *,
>                                           struct binding_ctx_out *);
>  void binding_tracked_dp_destroy(struct hmap *tracked_datapaths);
>
> -void binding_init(void);
> -void binding_seqno_run(struct local_binding_data *lbinding_data);
> -void binding_seqno_install(struct local_binding_data *lbinding_data);
> -void binding_seqno_flush(void);
>  void binding_dump_local_bindings(struct local_binding_data *, struct ds *);
> +
>  #endif /* controller/binding.h */
> diff --git a/controller/if-status.c b/controller/if-status.c
> new file mode 100644
> index 000000000..c6b1a7fa6
> --- /dev/null
> +++ b/controller/if-status.c
> @@ -0,0 +1,395 @@
> +/* Copyright (c) 2021, Red Hat, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <config.h>
> +
> +#include "binding.h"
> +#include "if-status.h"
> +#include "ofctrl-seqno.h"
> +
> +#include "lib/hmapx.h"
> +#include "lib/util.h"
> +#include "openvswitch/vlog.h"
> +
> +VLOG_DEFINE_THIS_MODULE(if_status);
> +
> +/* This module implements an interface manager that maintains the state of
> + * the interfaces wrt. their flows being completely installed in OVS and
> + * their corresponding bindings being marked up/down.
> + *
> + * A state machine is maintained for each interface.
> + *
> + * Transitions are triggered between states by three types of events:
> + * A. Events received from the binding module:
> + * - interface is claimed: if_status_mgr_claim_iface()
> + * - interface is released: if_status_mgr_release_iface()
> + * - interface is deleted: if_status_mgr_delete_iface()
> + *
> + * B. At every iteration, based on SB/OVS updates, handled in
> + *    if_status_mgr_update():
> + * - an interface binding has been marked "up" both in the Southbound and OVS
> + *   databases.
> + * - an interface binding has been marked "down" both in the Southbound and OVS
> + *   databases.
> + * - new interface has been claimed.
> + *
> + * C. At every iteration, based on ofctrl_seqno updates, handled in
> + *    if_status_mgr_run():
> + * - the flows for a previously claimed interface have been installed in OVS.
> + */
> +
> +enum if_state {
> +    OIF_CLAIMED,       /* Newly claimed interface. */
> +    OIF_INSTALL_FLOWS, /* Already claimed interface 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_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",
> +};
> +
> +struct ovs_iface {
> +    char *id;               /* Extracted from OVS external_ids.iface_id. */
> +    enum if_state state;    /* State of the interface in the state machine. */
> +    uint32_t install_seqno; /* Seqno at which this interface is expected to
> +                             * be fully programmed in OVS.  Only used in state
> +                             * OIF_INSTALL_FLOWS.
> +                             */
> +};
> +
> +/* State machine manager for all local OVS interfaces. */
> +struct if_status_mgr {
> +    /* All local interfaces, mapping from 'iface-id' to 'struct ovs_iface'. */
> +    struct shash ifaces;
> +
> +    /* All local interfaces, stored per state. */
> +    struct hmapx ifaces_per_state[OIF_MAX];
> +
> +    /* Registered ofctrl seqno type for port_binding flow installation. */
> +    size_t iface_seq_type_pb_cfg;
> +
> +    /* Interface specific seqno to be acked by ofctrl when flows for new
> +     * interfaces have been installed.
> +     */
> +    uint32_t iface_seqno;
> +};
> +
> +static struct ovs_iface *ovs_iface_create(struct if_status_mgr *,
> +                                          const char *iface_id,
> +                                          enum if_state );
> +static void ovs_iface_destroy(struct if_status_mgr *, struct ovs_iface *);
> +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,
> +    bool sb_readonly, bool ovs_readonly);
> +
> +struct if_status_mgr *
> +if_status_mgr_create(void)
> +{
> +    struct if_status_mgr *mgr = xzalloc(sizeof *mgr);
> +
> +    mgr->iface_seq_type_pb_cfg = ofctrl_seqno_add_type();
> +    for (size_t i = 0; i < ARRAY_SIZE(mgr->ifaces_per_state); i++) {
> +        hmapx_init(&mgr->ifaces_per_state[i]);
> +    }
> +    shash_init(&mgr->ifaces);
> +    return mgr;
> +}
> +
> +void
> +if_status_mgr_clear(struct if_status_mgr *mgr)
> +{
> +    struct shash_node *node_next;
> +    struct shash_node *node;
> +
> +    SHASH_FOR_EACH_SAFE (node, node_next, &mgr->ifaces) {
> +        ovs_iface_destroy(mgr, node->data);
> +    }
> +    ovs_assert(shash_is_empty(&mgr->ifaces));
> +
> +    for (size_t i = 0; i < ARRAY_SIZE(mgr->ifaces_per_state); i++) {
> +        ovs_assert(hmapx_is_empty(&mgr->ifaces_per_state[i]));
> +    }
> +}
> +
> +void
> +if_status_mgr_destroy(struct if_status_mgr *mgr)
> +{
> +    if_status_mgr_clear(mgr);
> +    shash_destroy(&mgr->ifaces);
> +    for (size_t i = 0; i < ARRAY_SIZE(mgr->ifaces_per_state); i++) {
> +        hmapx_destroy(&mgr->ifaces_per_state[i]);
> +    }
> +    free(mgr);
> +}
> +
> +void
> +if_status_mgr_claim_iface(struct if_status_mgr *mgr, const char *iface_id)
> +{
> +    struct ovs_iface *iface = shash_find_data(&mgr->ifaces, iface_id);
> +
> +    if (!iface) {
> +        iface = ovs_iface_create(mgr, iface_id, OIF_CLAIMED);
> +    }
> +
> +    switch (iface->state) {
> +    case OIF_CLAIMED:
> +    case OIF_INSTALL_FLOWS:
> +    case OIF_MARK_UP:
> +        /* Nothing to do here. */
> +        break;
> +    case OIF_INSTALLED:
> +    case OIF_MARK_DOWN:
> +        ovs_iface_set_state(mgr, iface, OIF_CLAIMED);
> +        break;
> +    case OIF_MAX:
> +        OVS_NOT_REACHED();
> +        break;
> +    }
> +}
> +
> +void
> +if_status_mgr_release_iface(struct if_status_mgr *mgr, const char *iface_id)
> +{
> +    struct ovs_iface *iface = shash_find_data(&mgr->ifaces, iface_id);
> +
> +    if (!iface) {
> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> +        VLOG_WARN_RL(&rl, "Trying to release unknown interface %s", iface_id);
> +        return;
> +    }
> +
> +    switch (iface->state) {
> +    case OIF_CLAIMED:
> +    case OIF_INSTALL_FLOWS:
> +        /* Not yet fully installed interfaces can be safely deleted. */
> +        ovs_iface_destroy(mgr, iface);
> +        break;
> +    case OIF_MARK_UP:
> +    case OIF_INSTALLED:
> +        /* Properly mark interfaces "down" if their flows were already
> +         * programmed in OVS.
> +         */
> +        ovs_iface_set_state(mgr, iface, OIF_MARK_DOWN);
> +        break;
> +    case OIF_MARK_DOWN:
> +        /* Nothing to do here. */
> +        break;
> +    case OIF_MAX:
> +        OVS_NOT_REACHED();
> +        break;
> +    }
> +}
> +
> +void
> +if_status_mgr_delete_iface(struct if_status_mgr *mgr, const char *iface_id)
> +{
> +    struct ovs_iface *iface = shash_find_data(&mgr->ifaces, iface_id);
> +
> +    if (!iface) {
> +        return;
> +    }
> +    ovs_iface_destroy(mgr, iface);
> +}
> +
> +void
> +if_status_mgr_update(struct if_status_mgr *mgr,
> +                     struct local_binding_data *binding_data)
> +{
> +    if (!binding_data) {
> +        return;
> +    }
> +
> +    struct shash *bindings = &binding_data->bindings;
> +    struct hmapx_node *node_next;
> +    struct hmapx_node *node;
> +
> +    /* Move all interfaces that have been confirmed "up" by the binding module,
> +     * from OIF_MARK_UP to OIF_INSTALLED.
> +     */
> +    HMAPX_FOR_EACH_SAFE (node, node_next,
> +                         &mgr->ifaces_per_state[OIF_MARK_UP]) {
> +        struct ovs_iface *iface = node->data;
> +
> +        if (local_binding_is_up(bindings, iface->id)) {
> +            ovs_iface_set_state(mgr, iface, OIF_INSTALLED);
> +        }
> +    }
> +
> +    /* Cleanup all interfaces that have been confirmed "down" by the binding
> +     * module.
> +     */
> +    HMAPX_FOR_EACH_SAFE (node, node_next,
> +                         &mgr->ifaces_per_state[OIF_MARK_DOWN]) {
> +        struct ovs_iface *iface = node->data;
> +
> +        if (local_binding_is_down(bindings, iface->id)) {
> +            ovs_iface_destroy(mgr, iface);
> +        }
> +    }
> +
> +    /* Register for a notification about flows being installed in OVS for all
> +     * newly claimed interfaces.
> +     *
> +     * Move them from OIF_CLAIMED to OIF_INSTALL_FLOWS.
> +     */
> +    bool new_ifaces = false;
> +    HMAPX_FOR_EACH_SAFE (node, node_next,
> +                         &mgr->ifaces_per_state[OIF_CLAIMED]) {
> +        struct ovs_iface *iface = node->data;
> +
> +        ovs_iface_set_state(mgr, iface, OIF_INSTALL_FLOWS);
> +        iface->install_seqno = mgr->iface_seqno + 1;
> +        new_ifaces = true;
> +    }
> +
> +    /* Request a seqno update when the flows for new interfaces have been
> +     * installed in OVS.
> +     */
> +    if (new_ifaces) {
> +        mgr->iface_seqno++;
> +        ofctrl_seqno_update_create(mgr->iface_seq_type_pb_cfg,
> +                                   mgr->iface_seqno);
> +        VLOG_DBG("Seqno requested: %"PRIu32, mgr->iface_seqno);
> +    }
> +}
> +
> +void
> +if_status_mgr_run(struct if_status_mgr *mgr,
> +                  struct local_binding_data *binding_data,
> +                  bool sb_readonly, bool ovs_readonly)
> +{
> +    struct ofctrl_acked_seqnos *acked_seqnos =
> +            ofctrl_acked_seqnos_get(mgr->iface_seq_type_pb_cfg);
> +    struct hmapx_node *node_next;
> +    struct hmapx_node *node;
> +
> +    /* Move interfaces from state OIF_INSTALL_FLOWS to OIF_MARK_UP if a
> +     * notification has been received aabout their flows being installed
> +     * in OVS.
> +     */
> +    HMAPX_FOR_EACH_SAFE (node, node_next,
> +                         &mgr->ifaces_per_state[OIF_INSTALL_FLOWS]) {
> +        struct ovs_iface *iface = node->data;
> +
> +        if (!ofctrl_acked_seqnos_contains(acked_seqnos,
> +                                          iface->install_seqno)) {
> +            continue;
> +        }
> +        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, sb_readonly,
> +                                  ovs_readonly);
> +}
> +
> +static struct ovs_iface *
> +ovs_iface_create(struct if_status_mgr *mgr, const char *iface_id,
> +                 enum if_state state)
> +{
> +    struct ovs_iface *iface = xzalloc(sizeof *iface);
> +
> +    VLOG_DBG("Interface %s create.", iface->id);
> +    iface->id = xstrdup(iface_id);
> +    shash_add(&mgr->ifaces, iface_id, iface);
> +    ovs_iface_set_state(mgr, iface, state);
> +    return iface;
> +}
> +
> +static void
> +ovs_iface_destroy(struct if_status_mgr *mgr, struct ovs_iface *iface)
> +{
> +    VLOG_DBG("Interface %s destroy: state %s", iface->id,
> +             if_state_names[iface->state]);
> +    hmapx_find_and_delete(&mgr->ifaces_per_state[iface->state], iface);
> +    shash_find_and_delete(&mgr->ifaces, iface->id);
> +    free(iface->id);
> +    free(iface);
> +}
> +
> +static void
> +ovs_iface_set_state(struct if_status_mgr *mgr, struct ovs_iface *iface,
> +                    enum if_state state)
> +{
> +    VLOG_DBG("Interface %s set state: old %s, new %s", iface->id,
> +             if_state_names[iface->state],
> +             if_state_names[state]);
> +
> +    hmapx_find_and_delete(&mgr->ifaces_per_state[iface->state], iface);
> +    iface->state = state;
> +    hmapx_add(&mgr->ifaces_per_state[iface->state], iface);
> +    iface->install_seqno = 0;
> +}
> +
> +static void
> +if_status_mgr_update_bindings(struct if_status_mgr *mgr,
> +                              struct local_binding_data *binding_data,
> +                              bool sb_readonly, bool ovs_readonly)
> +{
> +    if (!binding_data) {
> +        return;
> +    }
> +
> +    struct shash *bindings = &binding_data->bindings;
> +    struct hmapx_node *node;
> +
> +    /* Notify the binding module to set "down" all bindings that are still
> +     * in the process of being installed in OVS, i.e., are not yet instsalled.
> +     */
> +    HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_INSTALL_FLOWS]) {
> +        struct ovs_iface *iface = node->data;
> +
> +        local_binding_set_down(bindings, iface->id, sb_readonly, ovs_readonly);
> +    }
> +
> +    /* Notifiy the binding module to set "up" all bindings that have had
> +     * their flows installed but are not yet marked "up" in the binding
> +     * module.
> +     */
> +    HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_MARK_UP]) {
> +        struct ovs_iface *iface = node->data;
> +
> +        local_binding_set_up(bindings, iface->id, sb_readonly, ovs_readonly);
> +    }
> +
> +    /* Notify the binding module to set "down" all bindings that have been
> +     * released but are not yet marked as "down" in the binding module.
> +     */
> +    HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_MARK_DOWN]) {
> +        struct ovs_iface *iface = node->data;
> +
> +        local_binding_set_down(bindings, iface->id, sb_readonly, ovs_readonly);
> +    }
> +}
> diff --git a/controller/if-status.h b/controller/if-status.h
> new file mode 100644
> index 000000000..51fe7c684
> --- /dev/null
> +++ b/controller/if-status.h
> @@ -0,0 +1,37 @@
> +/* Copyright (c) 2021, Red Hat, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#ifndef IF_STATUS_H
> +#define IF_STATUS_H 1
> +
> +#include "openvswitch/shash.h"
> +
> +#include "binding.h"
> +
> +struct if_status_mgr;
> +
> +struct if_status_mgr *if_status_mgr_create(void);
> +void if_status_mgr_clear(struct if_status_mgr *);
> +void if_status_mgr_destroy(struct if_status_mgr *);
> +
> +void if_status_mgr_claim_iface(struct if_status_mgr *, const char *iface_id);
> +void if_status_mgr_release_iface(struct if_status_mgr *, const char *iface_id);
> +void if_status_mgr_delete_iface(struct if_status_mgr *, const char *iface_id);
> +
> +void if_status_mgr_update(struct if_status_mgr *, struct local_binding_data *);
> +void if_status_mgr_run(struct if_status_mgr *mgr, struct local_binding_data *,
> +                       bool sb_readonly, bool ovs_readonly);
> +
> +# endif /* controller/if-status.h */
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 6f7c9ea61..e599ad1df 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -33,6 +33,7 @@
>  #include "openvswitch/dynamic-string.h"
>  #include "encaps.h"
>  #include "fatal-signal.h"
> +#include "if-status.h"
>  #include "ip-mcast.h"
>  #include "openvswitch/hmap.h"
>  #include "lflow.h"
> @@ -103,6 +104,7 @@ OVS_NO_RETURN static void usage(void);
>
>  struct controller_engine_ctx {
>      struct lflow_cache *lflow_cache;
> +    struct if_status_mgr *if_mgr;
>  };
>
>  /* Pending packet to be injected into connected OVS. */
> @@ -996,6 +998,7 @@ en_ofctrl_is_connected_cleanup(void *data OVS_UNUSED)
>  static void
>  en_ofctrl_is_connected_run(struct engine_node *node, void *data)
>  {
> +    struct controller_engine_ctx *ctrl_ctx = engine_get_context()->client_ctx;
>      struct ed_type_ofctrl_is_connected *of_data = data;
>      if (of_data->connected != ofctrl_is_connected()) {
>          of_data->connected = !of_data->connected;
> @@ -1003,7 +1006,7 @@ en_ofctrl_is_connected_run(struct engine_node *node, void *data)
>          /* Flush ofctrl seqno requests when the ofctrl connection goes down. */
>          if (!of_data->connected) {
>              ofctrl_seqno_flush();
> -            binding_seqno_flush();
> +            if_status_mgr_clear(ctrl_ctx->if_mgr);
>          }
>          engine_set_node_state(node, EN_UPDATED);
>          return;
> @@ -1375,6 +1378,8 @@ init_binding_ctx(struct engine_node *node,
>                  engine_get_input("SB_port_binding", node),
>                  "datapath");
>
> +    struct controller_engine_ctx *ctrl_ctx = engine_get_context()->client_ctx;
> +
>      b_ctx_in->ovnsb_idl_txn = engine_get_context()->ovnsb_idl_txn;
>      b_ctx_in->ovs_idl_txn = engine_get_context()->ovs_idl_txn;
>      b_ctx_in->sbrec_datapath_binding_by_key = sbrec_datapath_binding_by_key;
> @@ -1401,6 +1406,7 @@ init_binding_ctx(struct engine_node *node,
>      b_ctx_out->local_iface_ids = &rt_data->local_iface_ids;
>      b_ctx_out->tracked_dp_bindings = NULL;
>      b_ctx_out->local_lports_changed = NULL;
> +    b_ctx_out->if_mgr = ctrl_ctx->if_mgr;
>  }
>
>  static void
> @@ -2440,7 +2446,6 @@ main(int argc, char *argv[])
>      /* Register ofctrl seqno types. */
>      ofctrl_seq_type_nb_cfg = ofctrl_seqno_add_type();
>
> -    binding_init();
>      patch_init();
>      pinctrl_init();
>      lflow_init();
> @@ -2744,7 +2749,9 @@ main(int argc, char *argv[])
>
>      struct controller_engine_ctx ctrl_engine_ctx = {
>          .lflow_cache = lflow_cache_create(),
> +        .if_mgr = if_status_mgr_create(),
>      };
> +    struct if_status_mgr *if_mgr = ctrl_engine_ctx.if_mgr;
>
>      char *ovn_version = ovn_get_internal_version();
>      VLOG_INFO("OVN internal version is : [%s]", ovn_version);
> @@ -2954,9 +2961,10 @@ main(int argc, char *argv[])
>                                                         ovnsb_idl_loop.idl),
>                                                ovnsb_cond_seqno,
>                                                ovnsb_expected_cond_seqno));
> -                    if (runtime_data && ovs_idl_txn && ovnsb_idl_txn) {
> -                        binding_seqno_run(&runtime_data->lbinding_data);
> -                    }
> +
> +                    struct local_binding_data *binding_data =
> +                        runtime_data ? &runtime_data->lbinding_data : NULL;
> +                    if_status_mgr_update(if_mgr, binding_data);
>
>                      flow_output_data = engine_get_data(&en_flow_output);
>                      if (flow_output_data && ct_zones_data) {
> @@ -2967,9 +2975,8 @@ main(int argc, char *argv[])
>                                     engine_node_changed(&en_flow_output));
>                      }
>                      ofctrl_seqno_run(ofctrl_get_cur_cfg());
> -                    if (runtime_data && ovs_idl_txn && ovnsb_idl_txn) {
> -                        binding_seqno_install(&runtime_data->lbinding_data);
> -                    }
> +                    if_status_mgr_run(if_mgr, binding_data, !ovnsb_idl_txn,
> +                                      !ovs_idl_txn);
>                  }
>
>              }
> @@ -3135,6 +3142,7 @@ loop_done:
>      ofctrl_destroy();
>      pinctrl_destroy();
>      patch_destroy();
> +    if_status_mgr_destroy(if_mgr);
>
>      ovsdb_idl_loop_destroy(&ovs_idl_loop);
>      ovsdb_idl_loop_destroy(&ovnsb_idl_loop);
>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On 4/28/21 6:39 PM, Numan Siddique wrote:
> On Fri, Apr 23, 2021 at 10:18 AM Dumitru Ceara <dceara@redhat.com> wrote:
>>
>> The initial implementation of the notification mechanism that indicates
>> if OVS flows corresponding to a logical switch port have been installed
>> is not resilient enough.  It didn't cover the case when transactions to
>> the local OVS DB or to the Southbound fail.
>>
>> In order to deal with that, factor out the code that tracks the state
>> of the interfaces to a new module, 'if-status', and implement it with
>> a state machine that will retry setting Port_Binding.UP and
>> OVS.Interface.external_ids:ovn-installed in the Southbound and local
>> OVS databases.
>>
>> Having a separate module makes sense because tracking the interface
>> state doesn't really depend on the rest of the binding module, except
>> for interacting with the Port_Binding and Interface IDL records.  For
>> this we add some additional APIs to binding.c.
>>
>> Reported-at: https://bugzilla.redhat.com/1952846
>> Fixes: 4d3cb42b076b ("binding: Set Logical_Switch_Port.up when all OVS flows are installed.")
>> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> 
> Hi Dumitru,

Hi Numan,

> 
> Thanks for fixing this issue.
> 
> During my testing, I found a regression.  If suppose an ovs port
> "sw0p1" is bound on
> a chassis for the logical port - sw0-port1, and if I delete the ovs port sw0p1,
> ovn-controller clears the chassis column of Port_Binding but it
> doesn't set the "up" column to "false".
> It still remains "true".
> 
> All the tests passed for me locally but still I see this issue when I
> ran in fake-multinode setup.
> 
> Can you please check if there are existing test cases which check for
> the "up" column of port binding to be
> false when it is released ?

You're right, the "up" field of the port binding in the SB will stay
"true" but the "chassis" column is unset so the NB.LSP.up field will be
set to "false".

I agree this is a bit confusing though and I'll try to fix it in v2.

> 
> 
> I would also suggest adding a few test cases to replicate the issue it
> is trying to address. If possible.
> 
> To test this scenario, I have one idea.  Make the SB ovsdb-server read-only
> (ovn-appctl -t ovnsb_db.ctl ovsdb-server/set-active-ovsdb-server
> tcp:192.0.0.1:6642 and
> ovn-appctl -t ovnsb_db.ctl ovsdb-server/connect-active-ovsdb-server)
> after a port is claimed, and then run ovs-vsctl commands to release
> the ovs port. At this point ovn-controller will fail
> to set the "chassis" and "up" column.  After a few seconds, make the
> ovsdb-server active
> and verify that the "chassis" and "up" columns are set properly.

This is a great idea, thanks for the suggestion, I'll try to add some
tests like this.

> 
> Is this possible to do ?  Or is this a wrong configuration ? Because
> ovn-controller can not come to
> know when ovsdb-server becomes active. Unless it keeps trying continuously.
> 
> In my testing, ovn-controller doesn't set it properly when I make the
> ovsdb-server active again.
> 
> What do you think ?

Actually, I think this just uncovered an already existing bug.  Even
without my changes, changing the iface-id while ovn-controller is
connected to a backup server triggers a transaction error but when the
SB becomes active again, ovn-controller will not retry the transaction
and won't even clear the chassis column of the port binding.

The only thing that helps is a full recompute.

I suspect there's a problem either with the way ovn-controller checks
for the transaction commit result or within the IDL.

I'll investigate some more but likely this will be a separate patch in v2.

> 
> Thanks
> Numan
> 

Thanks,
Dumitru

On 4/29/21 9:51 AM, Dumitru Ceara wrote:
> On 4/28/21 6:39 PM, Numan Siddique wrote:
>> On Fri, Apr 23, 2021 at 10:18 AM Dumitru Ceara <dceara@redhat.com> wrote:
>>>
>>> The initial implementation of the notification mechanism that indicates
>>> if OVS flows corresponding to a logical switch port have been installed
>>> is not resilient enough.  It didn't cover the case when transactions to
>>> the local OVS DB or to the Southbound fail.
>>>
>>> In order to deal with that, factor out the code that tracks the state
>>> of the interfaces to a new module, 'if-status', and implement it with
>>> a state machine that will retry setting Port_Binding.UP and
>>> OVS.Interface.external_ids:ovn-installed in the Southbound and local
>>> OVS databases.
>>>
>>> Having a separate module makes sense because tracking the interface
>>> state doesn't really depend on the rest of the binding module, except
>>> for interacting with the Port_Binding and Interface IDL records.  For
>>> this we add some additional APIs to binding.c.
>>>
>>> Reported-at: https://bugzilla.redhat.com/1952846
>>> Fixes: 4d3cb42b076b ("binding: Set Logical_Switch_Port.up when all OVS flows are installed.")
>>> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
>>
>> Hi Dumitru,
> 
> Hi Numan,
> 
>>
>> Thanks for fixing this issue.
>>
>> During my testing, I found a regression.  If suppose an ovs port
>> "sw0p1" is bound on
>> a chassis for the logical port - sw0-port1, and if I delete the ovs port sw0p1,
>> ovn-controller clears the chassis column of Port_Binding but it
>> doesn't set the "up" column to "false".
>> It still remains "true".
>>
>> All the tests passed for me locally but still I see this issue when I
>> ran in fake-multinode setup.
>>
>> Can you please check if there are existing test cases which check for
>> the "up" column of port binding to be
>> false when it is released ?
> 
> You're right, the "up" field of the port binding in the SB will stay
> "true" but the "chassis" column is unset so the NB.LSP.up field will be
> set to "false".
> 
> I agree this is a bit confusing though and I'll try to fix it in v2.
> 
>>
>>
>> I would also suggest adding a few test cases to replicate the issue it
>> is trying to address. If possible.
>>
>> To test this scenario, I have one idea.  Make the SB ovsdb-server read-only
>> (ovn-appctl -t ovnsb_db.ctl ovsdb-server/set-active-ovsdb-server
>> tcp:192.0.0.1:6642 and
>> ovn-appctl -t ovnsb_db.ctl ovsdb-server/connect-active-ovsdb-server)
>> after a port is claimed, and then run ovs-vsctl commands to release
>> the ovs port. At this point ovn-controller will fail
>> to set the "chassis" and "up" column.  After a few seconds, make the
>> ovsdb-server active
>> and verify that the "chassis" and "up" columns are set properly.
> 
> This is a great idea, thanks for the suggestion, I'll try to add some
> tests like this.
> 
>>
>> Is this possible to do ?  Or is this a wrong configuration ? Because
>> ovn-controller can not come to
>> know when ovsdb-server becomes active. Unless it keeps trying continuously.
>>
>> In my testing, ovn-controller doesn't set it properly when I make the
>> ovsdb-server active again.
>>
>> What do you think ?
> 
> Actually, I think this just uncovered an already existing bug.  Even
> without my changes, changing the iface-id while ovn-controller is
> connected to a backup server triggers a transaction error but when the
> SB becomes active again, ovn-controller will not retry the transaction
> and won't even clear the chassis column of the port binding.
> 
> The only thing that helps is a full recompute.
> 
> I suspect there's a problem either with the way ovn-controller checks
> for the transaction commit result or within the IDL.
> 
> I'll investigate some more but likely this will be a separate patch in v2.

This is happening due to 0401cf5f9e06 ("ovsdb idl: Try committing the
pending txn in ovsdb_idl_loop_run.").

Since this commit we try to commit the transaction early in the run
(processing it's result) but if the transaction errored out we don't
return the result to the caller of ovsdb_idl_loop_run().

> 
>>
>> Thanks
>> Numan
>>
> 
> Thanks,
> Dumitru
>

On 5/4/21 10:20 AM, Dumitru Ceara wrote:
> On 4/29/21 9:51 AM, Dumitru Ceara wrote:
>> On 4/28/21 6:39 PM, Numan Siddique wrote:
>>> On Fri, Apr 23, 2021 at 10:18 AM Dumitru Ceara <dceara@redhat.com> wrote:
>>>>
>>>> The initial implementation of the notification mechanism that indicates
>>>> if OVS flows corresponding to a logical switch port have been installed
>>>> is not resilient enough.  It didn't cover the case when transactions to
>>>> the local OVS DB or to the Southbound fail.
>>>>
>>>> In order to deal with that, factor out the code that tracks the state
>>>> of the interfaces to a new module, 'if-status', and implement it with
>>>> a state machine that will retry setting Port_Binding.UP and
>>>> OVS.Interface.external_ids:ovn-installed in the Southbound and local
>>>> OVS databases.
>>>>
>>>> Having a separate module makes sense because tracking the interface
>>>> state doesn't really depend on the rest of the binding module, except
>>>> for interacting with the Port_Binding and Interface IDL records.  For
>>>> this we add some additional APIs to binding.c.
>>>>
>>>> Reported-at: https://bugzilla.redhat.com/1952846
>>>> Fixes: 4d3cb42b076b ("binding: Set Logical_Switch_Port.up when all OVS flows are installed.")
>>>> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
>>>
>>> Hi Dumitru,
>>
>> Hi Numan,
>>
>>>
>>> Thanks for fixing this issue.
>>>
>>> During my testing, I found a regression.  If suppose an ovs port
>>> "sw0p1" is bound on
>>> a chassis for the logical port - sw0-port1, and if I delete the ovs port sw0p1,
>>> ovn-controller clears the chassis column of Port_Binding but it
>>> doesn't set the "up" column to "false".
>>> It still remains "true".
>>>
>>> All the tests passed for me locally but still I see this issue when I
>>> ran in fake-multinode setup.
>>>
>>> Can you please check if there are existing test cases which check for
>>> the "up" column of port binding to be
>>> false when it is released ?
>>
>> You're right, the "up" field of the port binding in the SB will stay
>> "true" but the "chassis" column is unset so the NB.LSP.up field will be
>> set to "false".
>>
>> I agree this is a bit confusing though and I'll try to fix it in v2.
>>
>>>
>>>
>>> I would also suggest adding a few test cases to replicate the issue it
>>> is trying to address. If possible.
>>>
>>> To test this scenario, I have one idea.  Make the SB ovsdb-server read-only
>>> (ovn-appctl -t ovnsb_db.ctl ovsdb-server/set-active-ovsdb-server
>>> tcp:192.0.0.1:6642 and
>>> ovn-appctl -t ovnsb_db.ctl ovsdb-server/connect-active-ovsdb-server)
>>> after a port is claimed, and then run ovs-vsctl commands to release
>>> the ovs port. At this point ovn-controller will fail
>>> to set the "chassis" and "up" column.  After a few seconds, make the
>>> ovsdb-server active
>>> and verify that the "chassis" and "up" columns are set properly.
>>
>> This is a great idea, thanks for the suggestion, I'll try to add some
>> tests like this.
>>
>>>
>>> Is this possible to do ?  Or is this a wrong configuration ? Because
>>> ovn-controller can not come to
>>> know when ovsdb-server becomes active. Unless it keeps trying continuously.
>>>
>>> In my testing, ovn-controller doesn't set it properly when I make the
>>> ovsdb-server active again.
>>>
>>> What do you think ?
>>
>> Actually, I think this just uncovered an already existing bug.  Even
>> without my changes, changing the iface-id while ovn-controller is
>> connected to a backup server triggers a transaction error but when the
>> SB becomes active again, ovn-controller will not retry the transaction
>> and won't even clear the chassis column of the port binding.
>>
>> The only thing that helps is a full recompute.
>>
>> I suspect there's a problem either with the way ovn-controller checks
>> for the transaction commit result or within the IDL.
>>
>> I'll investigate some more but likely this will be a separate patch in v2.
> 
> This is happening due to 0401cf5f9e06 ("ovsdb idl: Try committing the
> pending txn in ovsdb_idl_loop_run.").
> 
> Since this commit we try to commit the transaction early in the run
> (processing it's result) but if the transaction errored out we don't
> return the result to the caller of ovsdb_idl_loop_run().
> 

I discussed a bit with Ilya offline about 0401cf5f9e06 ("ovsdb idl: Try
committing the pending txn in ovsdb_idl_loop_run.") and how
ovn-controller should behave.

I think it's best to revert 0401cf5f9e06 and change ovn-controller
behavior so it doesn't process updates/clear IDL tracked data when
transactions are in progress.  But that's a separate change and likely
outside the scope of this fix.

However, without it, I can't use the approach you suggested for the self
test as writing to a backup ovsdb-server will not trigger recomputes.
Do you maybe have a different suggestion in order to test the scenario
this patch is trying to fix?

Otherwise I can add a few sanity tests and send a v2.

Thanks,
Dumitru

On Tue, May 4, 2021 at 8:35 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 5/4/21 10:20 AM, Dumitru Ceara wrote:
> > On 4/29/21 9:51 AM, Dumitru Ceara wrote:
> >> On 4/28/21 6:39 PM, Numan Siddique wrote:
> >>> On Fri, Apr 23, 2021 at 10:18 AM Dumitru Ceara <dceara@redhat.com> wrote:
> >>>>
> >>>> The initial implementation of the notification mechanism that indicates
> >>>> if OVS flows corresponding to a logical switch port have been installed
> >>>> is not resilient enough.  It didn't cover the case when transactions to
> >>>> the local OVS DB or to the Southbound fail.
> >>>>
> >>>> In order to deal with that, factor out the code that tracks the state
> >>>> of the interfaces to a new module, 'if-status', and implement it with
> >>>> a state machine that will retry setting Port_Binding.UP and
> >>>> OVS.Interface.external_ids:ovn-installed in the Southbound and local
> >>>> OVS databases.
> >>>>
> >>>> Having a separate module makes sense because tracking the interface
> >>>> state doesn't really depend on the rest of the binding module, except
> >>>> for interacting with the Port_Binding and Interface IDL records.  For
> >>>> this we add some additional APIs to binding.c.
> >>>>
> >>>> Reported-at: https://bugzilla.redhat.com/1952846
> >>>> Fixes: 4d3cb42b076b ("binding: Set Logical_Switch_Port.up when all OVS flows are installed.")
> >>>> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> >>>
> >>> Hi Dumitru,
> >>
> >> Hi Numan,
> >>
> >>>
> >>> Thanks for fixing this issue.
> >>>
> >>> During my testing, I found a regression.  If suppose an ovs port
> >>> "sw0p1" is bound on
> >>> a chassis for the logical port - sw0-port1, and if I delete the ovs port sw0p1,
> >>> ovn-controller clears the chassis column of Port_Binding but it
> >>> doesn't set the "up" column to "false".
> >>> It still remains "true".
> >>>
> >>> All the tests passed for me locally but still I see this issue when I
> >>> ran in fake-multinode setup.
> >>>
> >>> Can you please check if there are existing test cases which check for
> >>> the "up" column of port binding to be
> >>> false when it is released ?
> >>
> >> You're right, the "up" field of the port binding in the SB will stay
> >> "true" but the "chassis" column is unset so the NB.LSP.up field will be
> >> set to "false".
> >>
> >> I agree this is a bit confusing though and I'll try to fix it in v2.
> >>
> >>>
> >>>
> >>> I would also suggest adding a few test cases to replicate the issue it
> >>> is trying to address. If possible.
> >>>
> >>> To test this scenario, I have one idea.  Make the SB ovsdb-server read-only
> >>> (ovn-appctl -t ovnsb_db.ctl ovsdb-server/set-active-ovsdb-server
> >>> tcp:192.0.0.1:6642 and
> >>> ovn-appctl -t ovnsb_db.ctl ovsdb-server/connect-active-ovsdb-server)
> >>> after a port is claimed, and then run ovs-vsctl commands to release
> >>> the ovs port. At this point ovn-controller will fail
> >>> to set the "chassis" and "up" column.  After a few seconds, make the
> >>> ovsdb-server active
> >>> and verify that the "chassis" and "up" columns are set properly.
> >>
> >> This is a great idea, thanks for the suggestion, I'll try to add some
> >> tests like this.
> >>
> >>>
> >>> Is this possible to do ?  Or is this a wrong configuration ? Because
> >>> ovn-controller can not come to
> >>> know when ovsdb-server becomes active. Unless it keeps trying continuously.
> >>>
> >>> In my testing, ovn-controller doesn't set it properly when I make the
> >>> ovsdb-server active again.
> >>>
> >>> What do you think ?
> >>
> >> Actually, I think this just uncovered an already existing bug.  Even
> >> without my changes, changing the iface-id while ovn-controller is
> >> connected to a backup server triggers a transaction error but when the
> >> SB becomes active again, ovn-controller will not retry the transaction
> >> and won't even clear the chassis column of the port binding.
> >>
> >> The only thing that helps is a full recompute.
> >>
> >> I suspect there's a problem either with the way ovn-controller checks
> >> for the transaction commit result or within the IDL.
> >>
> >> I'll investigate some more but likely this will be a separate patch in v2.
> >
> > This is happening due to 0401cf5f9e06 ("ovsdb idl: Try committing the
> > pending txn in ovsdb_idl_loop_run.").
> >
> > Since this commit we try to commit the transaction early in the run
> > (processing it's result) but if the transaction errored out we don't
> > return the result to the caller of ovsdb_idl_loop_run().
> >
>
> I discussed a bit with Ilya offline about 0401cf5f9e06 ("ovsdb idl: Try
> committing the pending txn in ovsdb_idl_loop_run.") and how
> ovn-controller should behave.
>
> I think it's best to revert 0401cf5f9e06 and change ovn-controller
> behavior so it doesn't process updates/clear IDL tracked data when
> transactions are in progress.  But that's a separate change and likely
> outside the scope of this fix.

Ok.  Thanks for the investigation.

>
> However, without it, I can't use the approach you suggested for the self
> test as writing to a backup ovsdb-server will not trigger recomputes.
> Do you maybe have a different suggestion in order to test the scenario
> this patch is trying to fix?
>
I can't think of any other ideas.

> Otherwise I can add a few sanity tests and send a v2.

Sounds good to me.

Regards
Numan

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