[ovs-dev,v2,2/5] binding: Add the localport port binding in the binding_lport information.

From: Numan Siddique <numans@ovn.org>

If there is an OVS interface present with the external_ids:iface-id set
to a localport port binding, we create a 'struct local_binding' for this
OVS interface but we do not associate the binding_lport to this
local_binding.  This patch now associates the binding_lport now.
Future patch will make use of this information in handling the
runtime data changes in the pflow engine node.

Signed-off-by: Numan Siddique <numans@ovn.org>
---
 controller/binding.c    | 154 ++++++++++++++++++++++++++++++++++------
 tests/ovn-controller.at |  35 ++++++++-
 2 files changed, 166 insertions(+), 23 deletions(-)

On Fri, Jul 16, 2021 at 4:45 AM <numans@ovn.org> wrote:
>
> From: Numan Siddique <numans@ovn.org>
>
> If there is an OVS interface present with the external_ids:iface-id set
> to a localport port binding, we create a 'struct local_binding' for this
> OVS interface but we do not associate the binding_lport to this
> local_binding.  This patch now associates the binding_lport now.
> Future patch will make use of this information in handling the
> runtime data changes in the pflow engine node.
>
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>  controller/binding.c    | 154 ++++++++++++++++++++++++++++++++++------
>  tests/ovn-controller.at |  35 ++++++++-
>  2 files changed, 166 insertions(+), 23 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index e68546ca9..39119c0c3 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -630,6 +630,11 @@ static struct binding_lport *local_binding_add_lport(
>      enum en_lport_type);
>  static struct binding_lport *local_binding_get_primary_lport(
>      struct local_binding *);
> +static struct binding_lport *local_binding_get_first_lport(
> +    struct local_binding *lbinding);
> +static struct binding_lport
*local_binding_get_primary_or_localport_lport(
> +    struct local_binding *lbinding);
> +
>  static bool local_binding_handle_stale_binding_lports(
>      struct local_binding *lbinding, struct binding_ctx_in *b_ctx_in,
>      struct binding_ctx_out *b_ctx_out, struct hmap *qos_map);
> @@ -815,12 +820,15 @@ binding_dump_local_bindings(struct
local_binding_data *lbinding_data,
>          if (num_lports) {
>              struct shash child_lports = SHASH_INITIALIZER(&child_lports);
>              struct binding_lport *primary_lport = NULL;
> +            struct binding_lport *localport_lport = NULL;
>              struct binding_lport *b_lport;
>              bool first_elem = true;
>
>              LIST_FOR_EACH (b_lport, list_node,
&lbinding->binding_lports) {
>                  if (first_elem && b_lport->type == LP_VIF) {
>                      primary_lport = b_lport;
> +                } else if (first_elem && b_lport->type == LP_LOCALPORT) {
> +                    localport_lport = b_lport;
>                  } else {
>                      shash_add(&child_lports, b_lport->name, b_lport);
>                  }
> @@ -830,6 +838,9 @@ binding_dump_local_bindings(struct local_binding_data
*lbinding_data,
>              if (primary_lport) {
>                  ds_put_format(out_data, "primary lport : [%s]\n",
>                                primary_lport->name);
> +            } else if (localport_lport) {
> +                ds_put_format(out_data, "localport lport : [%s]\n",
> +                              localport_lport->name);
>              } else {
>                  ds_put_format(out_data, "no primary lport\n");
>              }
> @@ -1019,8 +1030,7 @@ claim_lport(const struct sbrec_port_binding *pb,
>   * Caller should make sure that this is the case.
>   */
>  static bool
> -release_lport(const struct sbrec_port_binding *pb, bool sb_readonly,
> -              struct hmap *tracked_datapaths, struct if_status_mgr
*if_mgr)
> +release_lport_(const struct sbrec_port_binding *pb, bool sb_readonly)
>  {
>      if (pb->encap) {
>          if (sb_readonly) {
> @@ -1043,9 +1053,20 @@ release_lport(const struct sbrec_port_binding *pb,
bool sb_readonly,
>          sbrec_port_binding_set_virtual_parent(pb, NULL);
>      }
>
> +    VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port);
> +    return true;
> +}
> +
> +static bool
> +release_lport(const struct sbrec_port_binding *pb, bool sb_readonly,
> +              struct hmap *tracked_datapaths, struct if_status_mgr
*if_mgr)
> +{
> +    if (!release_lport_(pb, sb_readonly)) {
> +        return false;
> +    }
> +
>      update_lport_tracking(pb, tracked_datapaths, false);
>      if_status_mgr_release_iface(if_mgr, pb->logical_port);
> -    VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port);
>      return true;
>  }
>
> @@ -1332,6 +1353,36 @@ consider_virtual_lport(const struct
sbrec_port_binding *pb,
>      return true;
>  }
>
> +static bool
> +consider_localport(const struct sbrec_port_binding *pb,
> +                   struct binding_ctx_in *b_ctx_in,
> +                   struct binding_ctx_out *b_ctx_out)
> +{
> +    struct shash *local_bindings = &b_ctx_out->lbinding_data->bindings;
> +    struct local_binding *lbinding = local_binding_find(local_bindings,
> +
 pb->logical_port);
> +
> +    if (!lbinding) {
> +        return true;
> +    }
> +
> +    local_binding_add_lport(&b_ctx_out->lbinding_data->lports, lbinding,
pb,
> +                            LP_LOCALPORT);
> +
> +    /* If the port binding is claimed, then release it as localport is
claimed
> +     * by any ovn-controller. */
> +    if (pb->chassis == b_ctx_in->chassis_rec) {
> +        if (!release_lport_(pb, !b_ctx_in->ovnsb_idl_txn)) {
> +            return false;
> +        }
> +
> +        remove_related_lport(pb, b_ctx_out);
> +    }
> +
> +    update_related_lport(pb, b_ctx_out);
> +    return true;
> +}
> +
>  /* Considers either claiming the lport or releasing the lport
>   * for non VIF lports.
>   */
> @@ -1561,11 +1612,14 @@ binding_run(struct binding_ctx_in *b_ctx_in,
struct binding_ctx_out *b_ctx_out)
>
>          switch (lport_type) {
>          case LP_PATCH:
> -        case LP_LOCALPORT:
>          case LP_VTEP:
>              update_related_lport(pb, b_ctx_out);
>              break;
>
> +        case LP_LOCALPORT:
> +            consider_localport(pb, b_ctx_in, b_ctx_out);
> +            break;
> +
>          case LP_VIF:
>              consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL,
qos_map_ptr);
>              break;
> @@ -1793,14 +1847,18 @@ consider_iface_claim(const struct
ovsrec_interface *iface_rec,
>          lbinding->iface = iface_rec;
>      }
>
> -    struct binding_lport *b_lport =
local_binding_get_primary_lport(lbinding);
> +    struct binding_lport *b_lport =
> +        local_binding_get_primary_or_localport_lport(lbinding);
>      const struct sbrec_port_binding *pb = NULL;
>      if (!b_lport) {
>          pb = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name,
>                                    lbinding->name);
> -        if (pb && get_lport_type(pb) == LP_VIF) {
> -            b_lport = local_binding_add_lport(binding_lports, lbinding,
pb,
> -                                              LP_VIF);
> +        if (pb) {
> +            enum en_lport_type lport_type = get_lport_type(pb);
> +            if (lport_type == LP_VIF || lport_type == LP_LOCALPORT) {
> +                b_lport = local_binding_add_lport(binding_lports,
lbinding, pb,
> +                                                  lport_type);

This logic looks redundant. Both consider_localport() and
consider_vif_lport() below would call local_binding_add_lport(), so can
this inner if-block be removed?

> +            }
>          }
>      }
>
> @@ -1809,6 +1867,10 @@ consider_iface_claim(const struct ovsrec_interface
*iface_rec,
>          return true;
>      }
>
> +    if (b_lport->type == LP_LOCALPORT) {
> +        return consider_localport(pb, b_ctx_in, b_ctx_out);
> +    }
> +
>      if (!consider_vif_lport(b_lport->pb, b_ctx_in, b_ctx_out,
>                              lbinding, qos_map)) {
>          return false;
> @@ -1853,7 +1915,8 @@ consider_iface_release(const struct
ovsrec_interface *iface_rec,
>      struct shash *binding_lports = &b_ctx_out->lbinding_data->lports;
>
>      lbinding = local_binding_find(local_bindings, iface_id);
> -    struct binding_lport *b_lport =
local_binding_get_primary_lport(lbinding);
> +    struct binding_lport *b_lport =
> +        local_binding_get_primary_or_localport_lport(lbinding);
>      if (is_binding_lport_this_chassis(b_lport, b_ctx_in->chassis_rec)) {
>          struct local_datapath *ld =
>              get_local_datapath(b_ctx_out->local_datapaths,
> @@ -1874,6 +1937,10 @@ consider_iface_release(const struct
ovsrec_interface *iface_rec,
>              }
>          }
>
> +    } else if (lbinding && b_lport && b_lport->type == LP_LOCALPORT) {
> +        /* lbinding is associated with a localport.  Remove it from the
> +         * related lports. */
> +        remove_related_lport(b_lport->pb, b_ctx_out);
>      }
>
>      if (lbinding) {
> @@ -2209,6 +2276,8 @@ binding_handle_port_binding_changes(struct
binding_ctx_in *b_ctx_in,
>          SHASH_INITIALIZER(&deleted_virtual_pbs);
>      struct shash deleted_vif_pbs =
>          SHASH_INITIALIZER(&deleted_vif_pbs);
> +    struct shash deleted_localport_pbs =
> +        SHASH_INITIALIZER(&deleted_localport_pbs);
>      struct shash deleted_other_pbs =
>          SHASH_INITIALIZER(&deleted_other_pbs);
>      const struct sbrec_port_binding *pb;
> @@ -2243,6 +2312,8 @@ binding_handle_port_binding_changes(struct
binding_ctx_in *b_ctx_in,
>              shash_add(&deleted_container_pbs, pb->logical_port, pb);
>          } else if (lport_type == LP_VIRTUAL) {
>              shash_add(&deleted_virtual_pbs, pb->logical_port, pb);
> +        } else if (lport_type == LP_LOCALPORT) {
> +            shash_add(&deleted_localport_pbs, pb->logical_port, pb);
>          } else {
>              shash_add(&deleted_other_pbs, pb->logical_port, pb);
>          }
> @@ -2277,6 +2348,12 @@ binding_handle_port_binding_changes(struct
binding_ctx_in *b_ctx_in,
>          }
>      }
>
> +    SHASH_FOR_EACH_SAFE (node, node_next, &deleted_localport_pbs) {
> +        handle_deleted_vif_lport(node->data, LP_LOCALPORT, b_ctx_in,
> +                                 b_ctx_out);
> +        shash_delete(&deleted_localport_pbs, node);
> +    }
> +
>      SHASH_FOR_EACH_SAFE (node, node_next, &deleted_other_pbs) {
>          handle_deleted_lport(node->data, b_ctx_in, b_ctx_out);
>          shash_delete(&deleted_other_pbs, node);
> @@ -2286,6 +2363,7 @@ delete_done:
>      shash_destroy(&deleted_container_pbs);
>      shash_destroy(&deleted_virtual_pbs);
>      shash_destroy(&deleted_vif_pbs);
> +    shash_destroy(&deleted_localport_pbs);
>      shash_destroy(&deleted_other_pbs);
>
>      if (!handled) {
> @@ -2345,8 +2423,11 @@ delete_done:
>                                                 b_ctx_out, qos_map_ptr);
>              break;
>
> -        case LP_PATCH:
>          case LP_LOCALPORT:
> +            handled = consider_localport(pb, b_ctx_in, b_ctx_out);
> +            break;
> +
> +        case LP_PATCH:
>          case LP_VTEP:
>              update_related_lport(pb, b_ctx_out);
>              if (lport_type ==  LP_PATCH) {
> @@ -2501,6 +2582,24 @@ local_binding_delete(struct local_binding
*lbinding,
>      local_binding_destroy(lbinding, binding_lports);
>  }
>
> +static struct binding_lport *
> +local_binding_get_first_lport(struct local_binding *lbinding)
> +{
> +    if (!lbinding) {
> +        return NULL;
> +    }
> +
> +    if (!ovs_list_is_empty(&lbinding->binding_lports)) {
> +        struct binding_lport *b_lport = NULL;
> +        b_lport = CONTAINER_OF(ovs_list_front(&lbinding->binding_lports),
> +                               struct binding_lport, list_node);
> +
> +        return b_lport;
> +    }
> +
> +    return NULL;
> +}
> +
>  /* Returns the primary binding lport if present in lbinding's
>   * binding lports list.  A binding lport is considered primary
>   * if binding lport's type is LP_VIF and the name matches
> @@ -2513,15 +2612,26 @@ local_binding_get_primary_lport(struct
local_binding *lbinding)
>          return NULL;
>      }
>
> -    if (!ovs_list_is_empty(&lbinding->binding_lports)) {
> -        struct binding_lport *b_lport = NULL;
> -        b_lport = CONTAINER_OF(ovs_list_front(&lbinding->binding_lports),
> -                               struct binding_lport, list_node);
> -
> -        if (b_lport->type == LP_VIF &&
> +    struct binding_lport *b_lport =
local_binding_get_first_lport(lbinding);
> +    if (b_lport && b_lport->type == LP_VIF &&
>              !strcmp(lbinding->name, b_lport->name)) {
> -            return b_lport;
> -        }
> +        return b_lport;
> +    }
> +
> +    return NULL;
> +}
> +
> +static struct binding_lport *
> +local_binding_get_primary_or_localport_lport(struct local_binding
*lbinding)
> +{
> +    if (!lbinding) {
> +        return NULL;
> +    }
> +
> +    struct binding_lport *b_lport =
local_binding_get_first_lport(lbinding);
> +    if (b_lport && (b_lport->type == LP_VIF || b_lport->type ==
LP_LOCALPORT)
> +            && !strcmp(lbinding->name, b_lport->name)) {
> +        return b_lport;
>      }
>
>      return NULL;
> @@ -2569,8 +2679,10 @@ local_binding_handle_stale_binding_lports(struct
local_binding *lbinding,
>                                            struct binding_ctx_out
*b_ctx_out,
>                                            struct hmap *qos_map)
>  {
> -    /* Check if this lbinding has a primary binding_lport or not. */
> -    struct binding_lport *p_lport =
local_binding_get_primary_lport(lbinding);
> +    /* Check if this lbinding has a primary binding_lport or
> +     * localport binding_lport or not. */
> +    struct binding_lport *p_lport =
> +        local_binding_get_primary_or_localport_lport(lbinding);
>      if (p_lport) {
>          /* Nothing to be done. */
>          return true;
> @@ -2737,6 +2849,7 @@ binding_lport_check_and_cleanup(struct
binding_lport *b_lport,
>
>      switch (b_lport->type) {
>      case LP_VIF:
> +    case LP_LOCALPORT:
>          if (strcmp(b_lport->name, b_lport->lbinding->name)) {
>              cleanup_blport = true;
>          }
> @@ -2756,7 +2869,6 @@ binding_lport_check_and_cleanup(struct
binding_lport *b_lport,
>          break;
>
>      case LP_PATCH:
> -    case LP_LOCALPORT:
>      case LP_VTEP:
>      case LP_L2GATEWAY:
>      case LP_L3GATEWAY:
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 9a2ebd7ce..11661a2fd 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -510,6 +510,18 @@ primary lport : [[lsp1]]
>  ----------------------------------------
>  ])
>
> +# Set the port type to localport
> +check ovn-nbctl lsp-set-type lsp1 localport
> +check as hv1 ovs-vsctl set open . external_ids:ovn-cms-options=localport
> +OVS_WAIT_UNTIL([test localport = $(ovn-sbctl get chassis .
other_config:ovn-cms-options)])
> +
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller
debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[lsp1]], OVS interface name : [[vif1]], num binding lports : [[1]]
> +localport lport : [[lsp1]]
> +----------------------------------------
> +])
> +
>  # pause ovn-northd
>  check as northd ovn-appctl -t ovn-northd pause
>  check as northd-backup ovn-appctl -t ovn-northd pause
> @@ -527,22 +539,41 @@ do
>          check as hv1 ovs-vsctl set open .
external_ids:ovn-cms-options=$type
>          OVS_WAIT_UNTIL([test $type = $(ovn-sbctl get chassis .
other_config:ovn-cms-options)])
>
> -        AT_CHECK([as hv1 ovn-appctl -t ovn-controller
debug/dump-local-bindings], [0], [dnl
> +        if [[ "$type" == "localport" ]]; then
> +            AT_CHECK([as hv1 ovn-appctl -t ovn-controller
debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[lsp1]], OVS interface name : [[vif1]], num binding lports : [[1]]
> +localport lport : [[lsp1]]
> +----------------------------------------
> +])
> +        else
> +            AT_CHECK([as hv1 ovn-appctl -t ovn-controller
debug/dump-local-bindings], [0], [dnl
>  Local bindings:
>  name: [[lsp1]], OVS interface name : [[vif1]], num binding lports : [[0]]
>  ----------------------------------------
>  ])
> +        fi
>
>          echo "Updating to $update_type from $type"
>          check ovn-sbctl set port_binding lsp1 type=$update_type
>          check as hv1 ovs-vsctl set open .
external_ids:ovn-cms-options=$update_type
>          OVS_WAIT_UNTIL([test $update_type = $(ovn-sbctl get chassis .
other_config:ovn-cms-options)])
>
> -        AT_CHECK([as hv1 ovn-appctl -t ovn-controller
debug/dump-local-bindings], [0], [dnl
> +        if [[ "$update_type" == "localport" ]]; then
> +            AT_CHECK([as hv1 ovn-appctl -t ovn-controller
debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[lsp1]], OVS interface name : [[vif1]], num binding lports : [[1]]
> +localport lport : [[lsp1]]
> +----------------------------------------
> +])
> +        else
> +            AT_CHECK([as hv1 ovn-appctl -t ovn-controller
debug/dump-local-bindings], [0], [dnl
>  Local bindings:
>  name: [[lsp1]], OVS interface name : [[vif1]], num binding lports : [[0]]
>  ----------------------------------------
>  ])
> +        fi
> +
>          # Set the port binding type back to VIF.
>          check ovn-sbctl set port_binding lsp1 type=\"\"
>          check as hv1 ovs-vsctl set open .
external_ids:ovn-cms-options=foo
> --
> 2.31.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On Mon, Jul 26, 2021 at 10:33 PM Han Zhou <zhouhan@gmail.com> wrote:
>
> On Fri, Jul 16, 2021 at 4:45 AM <numans@ovn.org> wrote:
> >
> > From: Numan Siddique <numans@ovn.org>
> >
> > If there is an OVS interface present with the external_ids:iface-id set
> > to a localport port binding, we create a 'struct local_binding' for this
> > OVS interface but we do not associate the binding_lport to this
> > local_binding.  This patch now associates the binding_lport now.
> > Future patch will make use of this information in handling the
> > runtime data changes in the pflow engine node.
> >
> > Signed-off-by: Numan Siddique <numans@ovn.org>
> > ---
> >  controller/binding.c    | 154 ++++++++++++++++++++++++++++++++++------
> >  tests/ovn-controller.at |  35 ++++++++-
> >  2 files changed, 166 insertions(+), 23 deletions(-)
> >
> > diff --git a/controller/binding.c b/controller/binding.c
> > index e68546ca9..39119c0c3 100644
> > --- a/controller/binding.c
> > +++ b/controller/binding.c
> > @@ -630,6 +630,11 @@ static struct binding_lport *local_binding_add_lport(
> >      enum en_lport_type);
> >  static struct binding_lport *local_binding_get_primary_lport(
> >      struct local_binding *);
> > +static struct binding_lport *local_binding_get_first_lport(
> > +    struct local_binding *lbinding);
> > +static struct binding_lport
> *local_binding_get_primary_or_localport_lport(
> > +    struct local_binding *lbinding);
> > +
> >  static bool local_binding_handle_stale_binding_lports(
> >      struct local_binding *lbinding, struct binding_ctx_in *b_ctx_in,
> >      struct binding_ctx_out *b_ctx_out, struct hmap *qos_map);
> > @@ -815,12 +820,15 @@ binding_dump_local_bindings(struct
> local_binding_data *lbinding_data,
> >          if (num_lports) {
> >              struct shash child_lports = SHASH_INITIALIZER(&child_lports);
> >              struct binding_lport *primary_lport = NULL;
> > +            struct binding_lport *localport_lport = NULL;
> >              struct binding_lport *b_lport;
> >              bool first_elem = true;
> >
> >              LIST_FOR_EACH (b_lport, list_node,
> &lbinding->binding_lports) {
> >                  if (first_elem && b_lport->type == LP_VIF) {
> >                      primary_lport = b_lport;
> > +                } else if (first_elem && b_lport->type == LP_LOCALPORT) {
> > +                    localport_lport = b_lport;
> >                  } else {
> >                      shash_add(&child_lports, b_lport->name, b_lport);
> >                  }
> > @@ -830,6 +838,9 @@ binding_dump_local_bindings(struct local_binding_data
> *lbinding_data,
> >              if (primary_lport) {
> >                  ds_put_format(out_data, "primary lport : [%s]\n",
> >                                primary_lport->name);
> > +            } else if (localport_lport) {
> > +                ds_put_format(out_data, "localport lport : [%s]\n",
> > +                              localport_lport->name);
> >              } else {
> >                  ds_put_format(out_data, "no primary lport\n");
> >              }
> > @@ -1019,8 +1030,7 @@ claim_lport(const struct sbrec_port_binding *pb,
> >   * Caller should make sure that this is the case.
> >   */
> >  static bool
> > -release_lport(const struct sbrec_port_binding *pb, bool sb_readonly,
> > -              struct hmap *tracked_datapaths, struct if_status_mgr
> *if_mgr)
> > +release_lport_(const struct sbrec_port_binding *pb, bool sb_readonly)
> >  {
> >      if (pb->encap) {
> >          if (sb_readonly) {
> > @@ -1043,9 +1053,20 @@ release_lport(const struct sbrec_port_binding *pb,
> bool sb_readonly,
> >          sbrec_port_binding_set_virtual_parent(pb, NULL);
> >      }
> >
> > +    VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port);
> > +    return true;
> > +}
> > +
> > +static bool
> > +release_lport(const struct sbrec_port_binding *pb, bool sb_readonly,
> > +              struct hmap *tracked_datapaths, struct if_status_mgr
> *if_mgr)
> > +{
> > +    if (!release_lport_(pb, sb_readonly)) {
> > +        return false;
> > +    }
> > +
> >      update_lport_tracking(pb, tracked_datapaths, false);
> >      if_status_mgr_release_iface(if_mgr, pb->logical_port);
> > -    VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port);
> >      return true;
> >  }
> >
> > @@ -1332,6 +1353,36 @@ consider_virtual_lport(const struct
> sbrec_port_binding *pb,
> >      return true;
> >  }
> >
> > +static bool
> > +consider_localport(const struct sbrec_port_binding *pb,
> > +                   struct binding_ctx_in *b_ctx_in,
> > +                   struct binding_ctx_out *b_ctx_out)
> > +{
> > +    struct shash *local_bindings = &b_ctx_out->lbinding_data->bindings;
> > +    struct local_binding *lbinding = local_binding_find(local_bindings,
> > +
>  pb->logical_port);
> > +
> > +    if (!lbinding) {
> > +        return true;
> > +    }
> > +
> > +    local_binding_add_lport(&b_ctx_out->lbinding_data->lports, lbinding,
> pb,
> > +                            LP_LOCALPORT);
> > +
> > +    /* If the port binding is claimed, then release it as localport is
> claimed
> > +     * by any ovn-controller. */
> > +    if (pb->chassis == b_ctx_in->chassis_rec) {
> > +        if (!release_lport_(pb, !b_ctx_in->ovnsb_idl_txn)) {
> > +            return false;
> > +        }
> > +
> > +        remove_related_lport(pb, b_ctx_out);
> > +    }
> > +
> > +    update_related_lport(pb, b_ctx_out);
> > +    return true;
> > +}
> > +
> >  /* Considers either claiming the lport or releasing the lport
> >   * for non VIF lports.
> >   */
> > @@ -1561,11 +1612,14 @@ binding_run(struct binding_ctx_in *b_ctx_in,
> struct binding_ctx_out *b_ctx_out)
> >
> >          switch (lport_type) {
> >          case LP_PATCH:
> > -        case LP_LOCALPORT:
> >          case LP_VTEP:
> >              update_related_lport(pb, b_ctx_out);
> >              break;
> >
> > +        case LP_LOCALPORT:
> > +            consider_localport(pb, b_ctx_in, b_ctx_out);
> > +            break;
> > +
> >          case LP_VIF:
> >              consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL,
> qos_map_ptr);
> >              break;
> > @@ -1793,14 +1847,18 @@ consider_iface_claim(const struct
> ovsrec_interface *iface_rec,
> >          lbinding->iface = iface_rec;
> >      }
> >
> > -    struct binding_lport *b_lport =
> local_binding_get_primary_lport(lbinding);
> > +    struct binding_lport *b_lport =
> > +        local_binding_get_primary_or_localport_lport(lbinding);
> >      const struct sbrec_port_binding *pb = NULL;
> >      if (!b_lport) {
> >          pb = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name,
> >                                    lbinding->name);
> > -        if (pb && get_lport_type(pb) == LP_VIF) {
> > -            b_lport = local_binding_add_lport(binding_lports, lbinding,
> pb,
> > -                                              LP_VIF);
> > +        if (pb) {
> > +            enum en_lport_type lport_type = get_lport_type(pb);
> > +            if (lport_type == LP_VIF || lport_type == LP_LOCALPORT) {
> > +                b_lport = local_binding_add_lport(binding_lports,
> lbinding, pb,
> > +                                                  lport_type);
>
> This logic looks redundant. Both consider_localport() and
> consider_vif_lport() below would call local_binding_add_lport(), so can
> this inner if-block be removed?

I did the below changes as per your suggestion and it breaks quite a
few test cases.

*****************************************
diff --git a/controller/binding.c b/controller/binding.c
index 93ee0372b..2cd92f188 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -1838,7 +1838,6 @@ consider_iface_claim(const struct
ovsrec_interface *iface_rec,
     smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id);

     struct shash *local_bindings = &b_ctx_out->lbinding_data->bindings;
-    struct shash *binding_lports = &b_ctx_out->lbinding_data->lports;
     struct local_binding *lbinding = local_binding_find(local_bindings,
                                                         iface_id);

@@ -1855,29 +1854,27 @@ consider_iface_claim(const struct
ovsrec_interface *iface_rec,
     if (!b_lport) {
         pb = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name,
                                   lbinding->name);
-        if (pb) {
-            enum en_lport_type lport_type = get_lport_type(pb);
-            if (lport_type == LP_VIF || lport_type == LP_LOCALPORT) {
-                b_lport = local_binding_add_lport(binding_lports, lbinding, pb,
-                                                  lport_type);
-            }
-        }
+    } else {
+        pb = b_lport->pb;
     }

-    if (!b_lport) {
-        /* There is no binding lport for this local binding. */
+    if (!pb) {
+        /* There is no port_binding row for this local binding. */
         return true;
     }

-    if (b_lport->type == LP_LOCALPORT) {
+    enum en_lport_type lport_type = get_lport_type(pb);
+    if (lport_type == LP_LOCALPORT) {
         return consider_localport(pb, b_ctx_in, b_ctx_out);
     }

-    if (!consider_vif_lport(b_lport->pb, b_ctx_in, b_ctx_out,
-                            lbinding, qos_map)) {
+    if (!consider_vif_lport(pb, b_ctx_in, b_ctx_out, lbinding, qos_map)) {
         return false;
     }

+    /* Get the (updated) b_lport again for the lbinding. */
+    b_lport = local_binding_get_primary_lport(lbinding);
+
     /* Update the child local_binding's iface (if any children) and try to
      *  claim the container lbindings. */
     LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) {

************************************************
Unfortunately we can't remove it and it is not redundant for the
following reasons:

1.  Lets say there is a logical port - sw0-p1 and it is claimed by
ovn-controller.
     So there will be a "local_binding" entry for sw0-p1 and also a
binding_lport entry
     in the local_binding->binding_lports list.

2.  Suppose if I run this command (and makes sw0-p1 as a container port whose
     parent 'foo' doesn't exist.

     ovn-nbctl set logical_switch_port sw0-p1 parent_name=foo

3.  Then the port binding handler releases the lport - sw0-p1 and removes
     the external_ids:ovn-installed=true from the corresponding OVS port.

4.  When ovn-controller receives the update for the OVS port changes
done in the previous transaction,
     with your suggested change, ovn-controller claims back the ovs interface.

Please let me know If you're still not convinced.   But I'd leave it
AS IS for now :).

Thanks
Numan



>
> > +            }
> >          }
> >      }
> >
> > @@ -1809,6 +1867,10 @@ consider_iface_claim(const struct ovsrec_interface
> *iface_rec,
> >          return true;
> >      }
> >
> > +    if (b_lport->type == LP_LOCALPORT) {
> > +        return consider_localport(pb, b_ctx_in, b_ctx_out);
> > +    }
> > +
> >      if (!consider_vif_lport(b_lport->pb, b_ctx_in, b_ctx_out,
> >                              lbinding, qos_map)) {
> >          return false;
> > @@ -1853,7 +1915,8 @@ consider_iface_release(const struct
> ovsrec_interface *iface_rec,
> >      struct shash *binding_lports = &b_ctx_out->lbinding_data->lports;
> >
> >      lbinding = local_binding_find(local_bindings, iface_id);
> > -    struct binding_lport *b_lport =
> local_binding_get_primary_lport(lbinding);
> > +    struct binding_lport *b_lport =
> > +        local_binding_get_primary_or_localport_lport(lbinding);
> >      if (is_binding_lport_this_chassis(b_lport, b_ctx_in->chassis_rec)) {
> >          struct local_datapath *ld =
> >              get_local_datapath(b_ctx_out->local_datapaths,
> > @@ -1874,6 +1937,10 @@ consider_iface_release(const struct
> ovsrec_interface *iface_rec,
> >              }
> >          }
> >
> > +    } else if (lbinding && b_lport && b_lport->type == LP_LOCALPORT) {
> > +        /* lbinding is associated with a localport.  Remove it from the
> > +         * related lports. */
> > +        remove_related_lport(b_lport->pb, b_ctx_out);
> >      }
> >
> >      if (lbinding) {
> > @@ -2209,6 +2276,8 @@ binding_handle_port_binding_changes(struct
> binding_ctx_in *b_ctx_in,
> >          SHASH_INITIALIZER(&deleted_virtual_pbs);
> >      struct shash deleted_vif_pbs =
> >          SHASH_INITIALIZER(&deleted_vif_pbs);
> > +    struct shash deleted_localport_pbs =
> > +        SHASH_INITIALIZER(&deleted_localport_pbs);
> >      struct shash deleted_other_pbs =
> >          SHASH_INITIALIZER(&deleted_other_pbs);
> >      const struct sbrec_port_binding *pb;
> > @@ -2243,6 +2312,8 @@ binding_handle_port_binding_changes(struct
> binding_ctx_in *b_ctx_in,
> >              shash_add(&deleted_container_pbs, pb->logical_port, pb);
> >          } else if (lport_type == LP_VIRTUAL) {
> >              shash_add(&deleted_virtual_pbs, pb->logical_port, pb);
> > +        } else if (lport_type == LP_LOCALPORT) {
> > +            shash_add(&deleted_localport_pbs, pb->logical_port, pb);
> >          } else {
> >              shash_add(&deleted_other_pbs, pb->logical_port, pb);
> >          }
> > @@ -2277,6 +2348,12 @@ binding_handle_port_binding_changes(struct
> binding_ctx_in *b_ctx_in,
> >          }
> >      }
> >
> > +    SHASH_FOR_EACH_SAFE (node, node_next, &deleted_localport_pbs) {
> > +        handle_deleted_vif_lport(node->data, LP_LOCALPORT, b_ctx_in,
> > +                                 b_ctx_out);
> > +        shash_delete(&deleted_localport_pbs, node);
> > +    }
> > +
> >      SHASH_FOR_EACH_SAFE (node, node_next, &deleted_other_pbs) {
> >          handle_deleted_lport(node->data, b_ctx_in, b_ctx_out);
> >          shash_delete(&deleted_other_pbs, node);
> > @@ -2286,6 +2363,7 @@ delete_done:
> >      shash_destroy(&deleted_container_pbs);
> >      shash_destroy(&deleted_virtual_pbs);
> >      shash_destroy(&deleted_vif_pbs);
> > +    shash_destroy(&deleted_localport_pbs);
> >      shash_destroy(&deleted_other_pbs);
> >
> >      if (!handled) {
> > @@ -2345,8 +2423,11 @@ delete_done:
> >                                                 b_ctx_out, qos_map_ptr);
> >              break;
> >
> > -        case LP_PATCH:
> >          case LP_LOCALPORT:
> > +            handled = consider_localport(pb, b_ctx_in, b_ctx_out);
> > +            break;
> > +
> > +        case LP_PATCH:
> >          case LP_VTEP:
> >              update_related_lport(pb, b_ctx_out);
> >              if (lport_type ==  LP_PATCH) {
> > @@ -2501,6 +2582,24 @@ local_binding_delete(struct local_binding
> *lbinding,
> >      local_binding_destroy(lbinding, binding_lports);
> >  }
> >
> > +static struct binding_lport *
> > +local_binding_get_first_lport(struct local_binding *lbinding)
> > +{
> > +    if (!lbinding) {
> > +        return NULL;
> > +    }
> > +
> > +    if (!ovs_list_is_empty(&lbinding->binding_lports)) {
> > +        struct binding_lport *b_lport = NULL;
> > +        b_lport = CONTAINER_OF(ovs_list_front(&lbinding->binding_lports),
> > +                               struct binding_lport, list_node);
> > +
> > +        return b_lport;
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> >  /* Returns the primary binding lport if present in lbinding's
> >   * binding lports list.  A binding lport is considered primary
> >   * if binding lport's type is LP_VIF and the name matches
> > @@ -2513,15 +2612,26 @@ local_binding_get_primary_lport(struct
> local_binding *lbinding)
> >          return NULL;
> >      }
> >
> > -    if (!ovs_list_is_empty(&lbinding->binding_lports)) {
> > -        struct binding_lport *b_lport = NULL;
> > -        b_lport = CONTAINER_OF(ovs_list_front(&lbinding->binding_lports),
> > -                               struct binding_lport, list_node);
> > -
> > -        if (b_lport->type == LP_VIF &&
> > +    struct binding_lport *b_lport =
> local_binding_get_first_lport(lbinding);
> > +    if (b_lport && b_lport->type == LP_VIF &&
> >              !strcmp(lbinding->name, b_lport->name)) {
> > -            return b_lport;
> > -        }
> > +        return b_lport;
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> > +static struct binding_lport *
> > +local_binding_get_primary_or_localport_lport(struct local_binding
> *lbinding)
> > +{
> > +    if (!lbinding) {
> > +        return NULL;
> > +    }
> > +
> > +    struct binding_lport *b_lport =
> local_binding_get_first_lport(lbinding);
> > +    if (b_lport && (b_lport->type == LP_VIF || b_lport->type ==
> LP_LOCALPORT)
> > +            && !strcmp(lbinding->name, b_lport->name)) {
> > +        return b_lport;
> >      }
> >
> >      return NULL;
> > @@ -2569,8 +2679,10 @@ local_binding_handle_stale_binding_lports(struct
> local_binding *lbinding,
> >                                            struct binding_ctx_out
> *b_ctx_out,
> >                                            struct hmap *qos_map)
> >  {
> > -    /* Check if this lbinding has a primary binding_lport or not. */
> > -    struct binding_lport *p_lport =
> local_binding_get_primary_lport(lbinding);
> > +    /* Check if this lbinding has a primary binding_lport or
> > +     * localport binding_lport or not. */
> > +    struct binding_lport *p_lport =
> > +        local_binding_get_primary_or_localport_lport(lbinding);
> >      if (p_lport) {
> >          /* Nothing to be done. */
> >          return true;
> > @@ -2737,6 +2849,7 @@ binding_lport_check_and_cleanup(struct
> binding_lport *b_lport,
> >
> >      switch (b_lport->type) {
> >      case LP_VIF:
> > +    case LP_LOCALPORT:
> >          if (strcmp(b_lport->name, b_lport->lbinding->name)) {
> >              cleanup_blport = true;
> >          }
> > @@ -2756,7 +2869,6 @@ binding_lport_check_and_cleanup(struct
> binding_lport *b_lport,
> >          break;
> >
> >      case LP_PATCH:
> > -    case LP_LOCALPORT:
> >      case LP_VTEP:
> >      case LP_L2GATEWAY:
> >      case LP_L3GATEWAY:
> > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> > index 9a2ebd7ce..11661a2fd 100644
> > --- a/tests/ovn-controller.at
> > +++ b/tests/ovn-controller.at
> > @@ -510,6 +510,18 @@ primary lport : [[lsp1]]
> >  ----------------------------------------
> >  ])
> >
> > +# Set the port type to localport
> > +check ovn-nbctl lsp-set-type lsp1 localport
> > +check as hv1 ovs-vsctl set open . external_ids:ovn-cms-options=localport
> > +OVS_WAIT_UNTIL([test localport = $(ovn-sbctl get chassis .
> other_config:ovn-cms-options)])
> > +
> > +AT_CHECK([as hv1 ovn-appctl -t ovn-controller
> debug/dump-local-bindings], [0], [dnl
> > +Local bindings:
> > +name: [[lsp1]], OVS interface name : [[vif1]], num binding lports : [[1]]
> > +localport lport : [[lsp1]]
> > +----------------------------------------
> > +])
> > +
> >  # pause ovn-northd
> >  check as northd ovn-appctl -t ovn-northd pause
> >  check as northd-backup ovn-appctl -t ovn-northd pause
> > @@ -527,22 +539,41 @@ do
> >          check as hv1 ovs-vsctl set open .
> external_ids:ovn-cms-options=$type
> >          OVS_WAIT_UNTIL([test $type = $(ovn-sbctl get chassis .
> other_config:ovn-cms-options)])
> >
> > -        AT_CHECK([as hv1 ovn-appctl -t ovn-controller
> debug/dump-local-bindings], [0], [dnl
> > +        if [[ "$type" == "localport" ]]; then
> > +            AT_CHECK([as hv1 ovn-appctl -t ovn-controller
> debug/dump-local-bindings], [0], [dnl
> > +Local bindings:
> > +name: [[lsp1]], OVS interface name : [[vif1]], num binding lports : [[1]]
> > +localport lport : [[lsp1]]
> > +----------------------------------------
> > +])
> > +        else
> > +            AT_CHECK([as hv1 ovn-appctl -t ovn-controller
> debug/dump-local-bindings], [0], [dnl
> >  Local bindings:
> >  name: [[lsp1]], OVS interface name : [[vif1]], num binding lports : [[0]]
> >  ----------------------------------------
> >  ])
> > +        fi
> >
> >          echo "Updating to $update_type from $type"
> >          check ovn-sbctl set port_binding lsp1 type=$update_type
> >          check as hv1 ovs-vsctl set open .
> external_ids:ovn-cms-options=$update_type
> >          OVS_WAIT_UNTIL([test $update_type = $(ovn-sbctl get chassis .
> other_config:ovn-cms-options)])
> >
> > -        AT_CHECK([as hv1 ovn-appctl -t ovn-controller
> debug/dump-local-bindings], [0], [dnl
> > +        if [[ "$update_type" == "localport" ]]; then
> > +            AT_CHECK([as hv1 ovn-appctl -t ovn-controller
> debug/dump-local-bindings], [0], [dnl
> > +Local bindings:
> > +name: [[lsp1]], OVS interface name : [[vif1]], num binding lports : [[1]]
> > +localport lport : [[lsp1]]
> > +----------------------------------------
> > +])
> > +        else
> > +            AT_CHECK([as hv1 ovn-appctl -t ovn-controller
> debug/dump-local-bindings], [0], [dnl
> >  Local bindings:
> >  name: [[lsp1]], OVS interface name : [[vif1]], num binding lports : [[0]]
> >  ----------------------------------------
> >  ])
> > +        fi
> > +
> >          # Set the port binding type back to VIF.
> >          check ovn-sbctl set port_binding lsp1 type=\"\"
> >          check as hv1 ovs-vsctl set open .
> external_ids:ovn-cms-options=foo
> > --
> > 2.31.1
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On Tue, Jul 27, 2021 at 2:55 PM Numan Siddique <numans@ovn.org> wrote:
>
> On Mon, Jul 26, 2021 at 10:33 PM Han Zhou <zhouhan@gmail.com> wrote:
> >
> > On Fri, Jul 16, 2021 at 4:45 AM <numans@ovn.org> wrote:
> > >
> > > From: Numan Siddique <numans@ovn.org>
> > >
> > > If there is an OVS interface present with the external_ids:iface-id
set
> > > to a localport port binding, we create a 'struct local_binding' for
this
> > > OVS interface but we do not associate the binding_lport to this
> > > local_binding.  This patch now associates the binding_lport now.
> > > Future patch will make use of this information in handling the
> > > runtime data changes in the pflow engine node.
> > >
> > > Signed-off-by: Numan Siddique <numans@ovn.org>
> > > ---
> > >  controller/binding.c    | 154
++++++++++++++++++++++++++++++++++------
> > >  tests/ovn-controller.at |  35 ++++++++-
> > >  2 files changed, 166 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/controller/binding.c b/controller/binding.c
> > > index e68546ca9..39119c0c3 100644
> > > --- a/controller/binding.c
> > > +++ b/controller/binding.c
> > > @@ -630,6 +630,11 @@ static struct binding_lport
*local_binding_add_lport(
> > >      enum en_lport_type);
> > >  static struct binding_lport *local_binding_get_primary_lport(
> > >      struct local_binding *);
> > > +static struct binding_lport *local_binding_get_first_lport(
> > > +    struct local_binding *lbinding);
> > > +static struct binding_lport
> > *local_binding_get_primary_or_localport_lport(
> > > +    struct local_binding *lbinding);
> > > +
> > >  static bool local_binding_handle_stale_binding_lports(
> > >      struct local_binding *lbinding, struct binding_ctx_in *b_ctx_in,
> > >      struct binding_ctx_out *b_ctx_out, struct hmap *qos_map);
> > > @@ -815,12 +820,15 @@ binding_dump_local_bindings(struct
> > local_binding_data *lbinding_data,
> > >          if (num_lports) {
> > >              struct shash child_lports =
SHASH_INITIALIZER(&child_lports);
> > >              struct binding_lport *primary_lport = NULL;
> > > +            struct binding_lport *localport_lport = NULL;
> > >              struct binding_lport *b_lport;
> > >              bool first_elem = true;
> > >
> > >              LIST_FOR_EACH (b_lport, list_node,
> > &lbinding->binding_lports) {
> > >                  if (first_elem && b_lport->type == LP_VIF) {
> > >                      primary_lport = b_lport;
> > > +                } else if (first_elem && b_lport->type ==
LP_LOCALPORT) {
> > > +                    localport_lport = b_lport;
> > >                  } else {
> > >                      shash_add(&child_lports, b_lport->name, b_lport);
> > >                  }
> > > @@ -830,6 +838,9 @@ binding_dump_local_bindings(struct
local_binding_data
> > *lbinding_data,
> > >              if (primary_lport) {
> > >                  ds_put_format(out_data, "primary lport : [%s]\n",
> > >                                primary_lport->name);
> > > +            } else if (localport_lport) {
> > > +                ds_put_format(out_data, "localport lport : [%s]\n",
> > > +                              localport_lport->name);
> > >              } else {
> > >                  ds_put_format(out_data, "no primary lport\n");
> > >              }
> > > @@ -1019,8 +1030,7 @@ claim_lport(const struct sbrec_port_binding *pb,
> > >   * Caller should make sure that this is the case.
> > >   */
> > >  static bool
> > > -release_lport(const struct sbrec_port_binding *pb, bool sb_readonly,
> > > -              struct hmap *tracked_datapaths, struct if_status_mgr
> > *if_mgr)
> > > +release_lport_(const struct sbrec_port_binding *pb, bool sb_readonly)
> > >  {
> > >      if (pb->encap) {
> > >          if (sb_readonly) {
> > > @@ -1043,9 +1053,20 @@ release_lport(const struct sbrec_port_binding
*pb,
> > bool sb_readonly,
> > >          sbrec_port_binding_set_virtual_parent(pb, NULL);
> > >      }
> > >
> > > +    VLOG_INFO("Releasing lport %s from this chassis.",
pb->logical_port);
> > > +    return true;
> > > +}
> > > +
> > > +static bool
> > > +release_lport(const struct sbrec_port_binding *pb, bool sb_readonly,
> > > +              struct hmap *tracked_datapaths, struct if_status_mgr
> > *if_mgr)
> > > +{
> > > +    if (!release_lport_(pb, sb_readonly)) {
> > > +        return false;
> > > +    }
> > > +
> > >      update_lport_tracking(pb, tracked_datapaths, false);
> > >      if_status_mgr_release_iface(if_mgr, pb->logical_port);
> > > -    VLOG_INFO("Releasing lport %s from this chassis.",
pb->logical_port);
> > >      return true;
> > >  }
> > >
> > > @@ -1332,6 +1353,36 @@ consider_virtual_lport(const struct
> > sbrec_port_binding *pb,
> > >      return true;
> > >  }
> > >
> > > +static bool
> > > +consider_localport(const struct sbrec_port_binding *pb,
> > > +                   struct binding_ctx_in *b_ctx_in,
> > > +                   struct binding_ctx_out *b_ctx_out)
> > > +{
> > > +    struct shash *local_bindings =
&b_ctx_out->lbinding_data->bindings;
> > > +    struct local_binding *lbinding =
local_binding_find(local_bindings,
> > > +
> >  pb->logical_port);
> > > +
> > > +    if (!lbinding) {
> > > +        return true;
> > > +    }
> > > +
> > > +    local_binding_add_lport(&b_ctx_out->lbinding_data->lports,
lbinding,
> > pb,
> > > +                            LP_LOCALPORT);
> > > +
> > > +    /* If the port binding is claimed, then release it as localport
is
> > claimed
> > > +     * by any ovn-controller. */
> > > +    if (pb->chassis == b_ctx_in->chassis_rec) {
> > > +        if (!release_lport_(pb, !b_ctx_in->ovnsb_idl_txn)) {
> > > +            return false;
> > > +        }
> > > +
> > > +        remove_related_lport(pb, b_ctx_out);
> > > +    }
> > > +
> > > +    update_related_lport(pb, b_ctx_out);
> > > +    return true;
> > > +}
> > > +
> > >  /* Considers either claiming the lport or releasing the lport
> > >   * for non VIF lports.
> > >   */
> > > @@ -1561,11 +1612,14 @@ binding_run(struct binding_ctx_in *b_ctx_in,
> > struct binding_ctx_out *b_ctx_out)
> > >
> > >          switch (lport_type) {
> > >          case LP_PATCH:
> > > -        case LP_LOCALPORT:
> > >          case LP_VTEP:
> > >              update_related_lport(pb, b_ctx_out);
> > >              break;
> > >
> > > +        case LP_LOCALPORT:
> > > +            consider_localport(pb, b_ctx_in, b_ctx_out);
> > > +            break;
> > > +
> > >          case LP_VIF:
> > >              consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL,
> > qos_map_ptr);
> > >              break;
> > > @@ -1793,14 +1847,18 @@ consider_iface_claim(const struct
> > ovsrec_interface *iface_rec,
> > >          lbinding->iface = iface_rec;
> > >      }
> > >
> > > -    struct binding_lport *b_lport =
> > local_binding_get_primary_lport(lbinding);
> > > +    struct binding_lport *b_lport =
> > > +        local_binding_get_primary_or_localport_lport(lbinding);
> > >      const struct sbrec_port_binding *pb = NULL;
> > >      if (!b_lport) {
> > >          pb =
lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name,
> > >                                    lbinding->name);
> > > -        if (pb && get_lport_type(pb) == LP_VIF) {
> > > -            b_lport = local_binding_add_lport(binding_lports,
lbinding,
> > pb,
> > > -                                              LP_VIF);
> > > +        if (pb) {
> > > +            enum en_lport_type lport_type = get_lport_type(pb);
> > > +            if (lport_type == LP_VIF || lport_type == LP_LOCALPORT) {
> > > +                b_lport = local_binding_add_lport(binding_lports,
> > lbinding, pb,
> > > +                                                  lport_type);
> >
> > This logic looks redundant. Both consider_localport() and
> > consider_vif_lport() below would call local_binding_add_lport(), so can
> > this inner if-block be removed?
>
> I did the below changes as per your suggestion and it breaks quite a
> few test cases.
>
> *****************************************
> diff --git a/controller/binding.c b/controller/binding.c
> index 93ee0372b..2cd92f188 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -1838,7 +1838,6 @@ consider_iface_claim(const struct
> ovsrec_interface *iface_rec,
>      smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id);
>
>      struct shash *local_bindings = &b_ctx_out->lbinding_data->bindings;
> -    struct shash *binding_lports = &b_ctx_out->lbinding_data->lports;
>      struct local_binding *lbinding = local_binding_find(local_bindings,
>                                                          iface_id);
>
> @@ -1855,29 +1854,27 @@ consider_iface_claim(const struct
> ovsrec_interface *iface_rec,
>      if (!b_lport) {
>          pb = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name,
>                                    lbinding->name);
> -        if (pb) {
> -            enum en_lport_type lport_type = get_lport_type(pb);
> -            if (lport_type == LP_VIF || lport_type == LP_LOCALPORT) {
> -                b_lport = local_binding_add_lport(binding_lports,
lbinding, pb,
> -                                                  lport_type);
> -            }
> -        }
> +    } else {
> +        pb = b_lport->pb;
>      }
>
> -    if (!b_lport) {
> -        /* There is no binding lport for this local binding. */
> +    if (!pb) {
> +        /* There is no port_binding row for this local binding. */
>          return true;
>      }
>
> -    if (b_lport->type == LP_LOCALPORT) {
> +    enum en_lport_type lport_type = get_lport_type(pb);
> +    if (lport_type == LP_LOCALPORT) {
>          return consider_localport(pb, b_ctx_in, b_ctx_out);
>      }
>
> -    if (!consider_vif_lport(b_lport->pb, b_ctx_in, b_ctx_out,
> -                            lbinding, qos_map)) {
> +    if (!consider_vif_lport(pb, b_ctx_in, b_ctx_out, lbinding, qos_map))
{
>          return false;
>      }
>
> +    /* Get the (updated) b_lport again for the lbinding. */
> +    b_lport = local_binding_get_primary_lport(lbinding);
> +
>      /* Update the child local_binding's iface (if any children) and try
to
>       *  claim the container lbindings. */
>      LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) {
>
> ************************************************
> Unfortunately we can't remove it and it is not redundant for the
> following reasons:
>
> 1.  Lets say there is a logical port - sw0-p1 and it is claimed by
> ovn-controller.
>      So there will be a "local_binding" entry for sw0-p1 and also a
> binding_lport entry
>      in the local_binding->binding_lports list.
>
> 2.  Suppose if I run this command (and makes sw0-p1 as a container port
whose
>      parent 'foo' doesn't exist.
>
>      ovn-nbctl set logical_switch_port sw0-p1 parent_name=foo
>
> 3.  Then the port binding handler releases the lport - sw0-p1 and removes
>      the external_ids:ovn-installed=true from the corresponding OVS port.
>
> 4.  When ovn-controller receives the update for the OVS port changes
> done in the previous transaction,
>      with your suggested change, ovn-controller claims back the ovs
interface.
>
> Please let me know If you're still not convinced.   But I'd leave it
> AS IS for now :).
>
> Thanks
> Numan
>

I am not sure if I understand the logic completely, but the modified
version looks more reasonable. Shouldn't it fix the scenario just by adding
a check "if lport_type == LP_VIF" before calling consider_vif_lport()?
If it still has the problem and you have to keep the current logic, then I
think at least adding some comments to explain such scenarios and explain
the exact purpose of each of these functions. At least it is hard for me to
follow the code and reasoning about the scenario you mentioned above. But
if it is possible I'd prefer to simplify the logic without extra comments.

>
>
> >
> > > +            }
> > >          }
> > >      }
> > >
> > > @@ -1809,6 +1867,10 @@ consider_iface_claim(const struct
ovsrec_interface
> > *iface_rec,
> > >          return true;
> > >      }
> > >
> > > +    if (b_lport->type == LP_LOCALPORT) {
> > > +        return consider_localport(pb, b_ctx_in, b_ctx_out);
> > > +    }
> > > +
> > >      if (!consider_vif_lport(b_lport->pb, b_ctx_in, b_ctx_out,
> > >                              lbinding, qos_map)) {
> > >          return false;
> > > @@ -1853,7 +1915,8 @@ consider_iface_release(const struct
> > ovsrec_interface *iface_rec,
> > >      struct shash *binding_lports = &b_ctx_out->lbinding_data->lports;
> > >
> > >      lbinding = local_binding_find(local_bindings, iface_id);
> > > -    struct binding_lport *b_lport =
> > local_binding_get_primary_lport(lbinding);
> > > +    struct binding_lport *b_lport =
> > > +        local_binding_get_primary_or_localport_lport(lbinding);
> > >      if (is_binding_lport_this_chassis(b_lport,
b_ctx_in->chassis_rec)) {
> > >          struct local_datapath *ld =
> > >              get_local_datapath(b_ctx_out->local_datapaths,
> > > @@ -1874,6 +1937,10 @@ consider_iface_release(const struct
> > ovsrec_interface *iface_rec,
> > >              }
> > >          }
> > >
> > > +    } else if (lbinding && b_lport && b_lport->type == LP_LOCALPORT)
{
> > > +        /* lbinding is associated with a localport.  Remove it from
the
> > > +         * related lports. */
> > > +        remove_related_lport(b_lport->pb, b_ctx_out);
> > >      }
> > >
> > >      if (lbinding) {
> > > @@ -2209,6 +2276,8 @@ binding_handle_port_binding_changes(struct
> > binding_ctx_in *b_ctx_in,
> > >          SHASH_INITIALIZER(&deleted_virtual_pbs);
> > >      struct shash deleted_vif_pbs =
> > >          SHASH_INITIALIZER(&deleted_vif_pbs);
> > > +    struct shash deleted_localport_pbs =
> > > +        SHASH_INITIALIZER(&deleted_localport_pbs);
> > >      struct shash deleted_other_pbs =
> > >          SHASH_INITIALIZER(&deleted_other_pbs);
> > >      const struct sbrec_port_binding *pb;
> > > @@ -2243,6 +2312,8 @@ binding_handle_port_binding_changes(struct
> > binding_ctx_in *b_ctx_in,
> > >              shash_add(&deleted_container_pbs, pb->logical_port, pb);
> > >          } else if (lport_type == LP_VIRTUAL) {
> > >              shash_add(&deleted_virtual_pbs, pb->logical_port, pb);
> > > +        } else if (lport_type == LP_LOCALPORT) {
> > > +            shash_add(&deleted_localport_pbs, pb->logical_port, pb);
> > >          } else {
> > >              shash_add(&deleted_other_pbs, pb->logical_port, pb);
> > >          }
> > > @@ -2277,6 +2348,12 @@ binding_handle_port_binding_changes(struct
> > binding_ctx_in *b_ctx_in,
> > >          }
> > >      }
> > >
> > > +    SHASH_FOR_EACH_SAFE (node, node_next, &deleted_localport_pbs) {
> > > +        handle_deleted_vif_lport(node->data, LP_LOCALPORT, b_ctx_in,
> > > +                                 b_ctx_out);
> > > +        shash_delete(&deleted_localport_pbs, node);
> > > +    }
> > > +
> > >      SHASH_FOR_EACH_SAFE (node, node_next, &deleted_other_pbs) {
> > >          handle_deleted_lport(node->data, b_ctx_in, b_ctx_out);
> > >          shash_delete(&deleted_other_pbs, node);
> > > @@ -2286,6 +2363,7 @@ delete_done:
> > >      shash_destroy(&deleted_container_pbs);
> > >      shash_destroy(&deleted_virtual_pbs);
> > >      shash_destroy(&deleted_vif_pbs);
> > > +    shash_destroy(&deleted_localport_pbs);
> > >      shash_destroy(&deleted_other_pbs);
> > >
> > >      if (!handled) {
> > > @@ -2345,8 +2423,11 @@ delete_done:
> > >                                                 b_ctx_out,
qos_map_ptr);
> > >              break;
> > >
> > > -        case LP_PATCH:
> > >          case LP_LOCALPORT:
> > > +            handled = consider_localport(pb, b_ctx_in, b_ctx_out);
> > > +            break;
> > > +
> > > +        case LP_PATCH:
> > >          case LP_VTEP:
> > >              update_related_lport(pb, b_ctx_out);
> > >              if (lport_type ==  LP_PATCH) {
> > > @@ -2501,6 +2582,24 @@ local_binding_delete(struct local_binding
> > *lbinding,
> > >      local_binding_destroy(lbinding, binding_lports);
> > >  }
> > >
> > > +static struct binding_lport *
> > > +local_binding_get_first_lport(struct local_binding *lbinding)
> > > +{
> > > +    if (!lbinding) {
> > > +        return NULL;
> > > +    }
> > > +
> > > +    if (!ovs_list_is_empty(&lbinding->binding_lports)) {
> > > +        struct binding_lport *b_lport = NULL;
> > > +        b_lport =
CONTAINER_OF(ovs_list_front(&lbinding->binding_lports),
> > > +                               struct binding_lport, list_node);
> > > +
> > > +        return b_lport;
> > > +    }
> > > +
> > > +    return NULL;
> > > +}
> > > +
> > >  /* Returns the primary binding lport if present in lbinding's
> > >   * binding lports list.  A binding lport is considered primary
> > >   * if binding lport's type is LP_VIF and the name matches
> > > @@ -2513,15 +2612,26 @@ local_binding_get_primary_lport(struct
> > local_binding *lbinding)
> > >          return NULL;
> > >      }
> > >
> > > -    if (!ovs_list_is_empty(&lbinding->binding_lports)) {
> > > -        struct binding_lport *b_lport = NULL;
> > > -        b_lport =
CONTAINER_OF(ovs_list_front(&lbinding->binding_lports),
> > > -                               struct binding_lport, list_node);
> > > -
> > > -        if (b_lport->type == LP_VIF &&
> > > +    struct binding_lport *b_lport =
> > local_binding_get_first_lport(lbinding);
> > > +    if (b_lport && b_lport->type == LP_VIF &&
> > >              !strcmp(lbinding->name, b_lport->name)) {
> > > -            return b_lport;
> > > -        }
> > > +        return b_lport;
> > > +    }
> > > +
> > > +    return NULL;
> > > +}
> > > +
> > > +static struct binding_lport *
> > > +local_binding_get_primary_or_localport_lport(struct local_binding
> > *lbinding)
> > > +{
> > > +    if (!lbinding) {
> > > +        return NULL;
> > > +    }
> > > +
> > > +    struct binding_lport *b_lport =
> > local_binding_get_first_lport(lbinding);
> > > +    if (b_lport && (b_lport->type == LP_VIF || b_lport->type ==
> > LP_LOCALPORT)
> > > +            && !strcmp(lbinding->name, b_lport->name)) {
> > > +        return b_lport;
> > >      }
> > >
> > >      return NULL;
> > > @@ -2569,8 +2679,10 @@
local_binding_handle_stale_binding_lports(struct
> > local_binding *lbinding,
> > >                                            struct binding_ctx_out
> > *b_ctx_out,
> > >                                            struct hmap *qos_map)
> > >  {
> > > -    /* Check if this lbinding has a primary binding_lport or not. */
> > > -    struct binding_lport *p_lport =
> > local_binding_get_primary_lport(lbinding);
> > > +    /* Check if this lbinding has a primary binding_lport or
> > > +     * localport binding_lport or not. */
> > > +    struct binding_lport *p_lport =
> > > +        local_binding_get_primary_or_localport_lport(lbinding);
> > >      if (p_lport) {
> > >          /* Nothing to be done. */
> > >          return true;
> > > @@ -2737,6 +2849,7 @@ binding_lport_check_and_cleanup(struct
> > binding_lport *b_lport,
> > >
> > >      switch (b_lport->type) {
> > >      case LP_VIF:
> > > +    case LP_LOCALPORT:
> > >          if (strcmp(b_lport->name, b_lport->lbinding->name)) {
> > >              cleanup_blport = true;
> > >          }
> > > @@ -2756,7 +2869,6 @@ binding_lport_check_and_cleanup(struct
> > binding_lport *b_lport,
> > >          break;
> > >
> > >      case LP_PATCH:
> > > -    case LP_LOCALPORT:
> > >      case LP_VTEP:
> > >      case LP_L2GATEWAY:
> > >      case LP_L3GATEWAY:
> > > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> > > index 9a2ebd7ce..11661a2fd 100644
> > > --- a/tests/ovn-controller.at
> > > +++ b/tests/ovn-controller.at
> > > @@ -510,6 +510,18 @@ primary lport : [[lsp1]]
> > >  ----------------------------------------
> > >  ])
> > >
> > > +# Set the port type to localport
> > > +check ovn-nbctl lsp-set-type lsp1 localport
> > > +check as hv1 ovs-vsctl set open .
external_ids:ovn-cms-options=localport
> > > +OVS_WAIT_UNTIL([test localport = $(ovn-sbctl get chassis .
> > other_config:ovn-cms-options)])
> > > +
> > > +AT_CHECK([as hv1 ovn-appctl -t ovn-controller
> > debug/dump-local-bindings], [0], [dnl
> > > +Local bindings:
> > > +name: [[lsp1]], OVS interface name : [[vif1]], num binding lports :
[[1]]
> > > +localport lport : [[lsp1]]
> > > +----------------------------------------
> > > +])
> > > +
> > >  # pause ovn-northd
> > >  check as northd ovn-appctl -t ovn-northd pause
> > >  check as northd-backup ovn-appctl -t ovn-northd pause
> > > @@ -527,22 +539,41 @@ do
> > >          check as hv1 ovs-vsctl set open .
> > external_ids:ovn-cms-options=$type
> > >          OVS_WAIT_UNTIL([test $type = $(ovn-sbctl get chassis .
> > other_config:ovn-cms-options)])
> > >
> > > -        AT_CHECK([as hv1 ovn-appctl -t ovn-controller
> > debug/dump-local-bindings], [0], [dnl
> > > +        if [[ "$type" == "localport" ]]; then
> > > +            AT_CHECK([as hv1 ovn-appctl -t ovn-controller
> > debug/dump-local-bindings], [0], [dnl
> > > +Local bindings:
> > > +name: [[lsp1]], OVS interface name : [[vif1]], num binding lports :
[[1]]
> > > +localport lport : [[lsp1]]
> > > +----------------------------------------
> > > +])
> > > +        else
> > > +            AT_CHECK([as hv1 ovn-appctl -t ovn-controller
> > debug/dump-local-bindings], [0], [dnl
> > >  Local bindings:
> > >  name: [[lsp1]], OVS interface name : [[vif1]], num binding lports :
[[0]]
> > >  ----------------------------------------
> > >  ])
> > > +        fi
> > >
> > >          echo "Updating to $update_type from $type"
> > >          check ovn-sbctl set port_binding lsp1 type=$update_type
> > >          check as hv1 ovs-vsctl set open .
> > external_ids:ovn-cms-options=$update_type
> > >          OVS_WAIT_UNTIL([test $update_type = $(ovn-sbctl get chassis .
> > other_config:ovn-cms-options)])
> > >
> > > -        AT_CHECK([as hv1 ovn-appctl -t ovn-controller
> > debug/dump-local-bindings], [0], [dnl
> > > +        if [[ "$update_type" == "localport" ]]; then
> > > +            AT_CHECK([as hv1 ovn-appctl -t ovn-controller
> > debug/dump-local-bindings], [0], [dnl
> > > +Local bindings:
> > > +name: [[lsp1]], OVS interface name : [[vif1]], num binding lports :
[[1]]
> > > +localport lport : [[lsp1]]
> > > +----------------------------------------
> > > +])
> > > +        else
> > > +            AT_CHECK([as hv1 ovn-appctl -t ovn-controller
> > debug/dump-local-bindings], [0], [dnl
> > >  Local bindings:
> > >  name: [[lsp1]], OVS interface name : [[vif1]], num binding lports :
[[0]]
> > >  ----------------------------------------
> > >  ])
> > > +        fi
> > > +
> > >          # Set the port binding type back to VIF.
> > >          check ovn-sbctl set port_binding lsp1 type=\"\"
> > >          check as hv1 ovs-vsctl set open .
> > external_ids:ovn-cms-options=foo
> > > --
> > > 2.31.1
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >

On Tue, Jul 27, 2021 at 6:51 PM Han Zhou <zhouhan@gmail.com> wrote:
>
> On Tue, Jul 27, 2021 at 2:55 PM Numan Siddique <numans@ovn.org> wrote:
> >
> > On Mon, Jul 26, 2021 at 10:33 PM Han Zhou <zhouhan@gmail.com> wrote:
> > >
> > > On Fri, Jul 16, 2021 at 4:45 AM <numans@ovn.org> wrote:
> > > >
> > > > From: Numan Siddique <numans@ovn.org>
> > > >
> > > > If there is an OVS interface present with the external_ids:iface-id
> set
> > > > to a localport port binding, we create a 'struct local_binding' for
> this
> > > > OVS interface but we do not associate the binding_lport to this
> > > > local_binding.  This patch now associates the binding_lport now.
> > > > Future patch will make use of this information in handling the
> > > > runtime data changes in the pflow engine node.
> > > >
> > > > Signed-off-by: Numan Siddique <numans@ovn.org>
> > > > ---
> > > >  controller/binding.c    | 154
> ++++++++++++++++++++++++++++++++++------
> > > >  tests/ovn-controller.at |  35 ++++++++-
> > > >  2 files changed, 166 insertions(+), 23 deletions(-)
> > > >
> > > > diff --git a/controller/binding.c b/controller/binding.c
> > > > index e68546ca9..39119c0c3 100644
> > > > --- a/controller/binding.c
> > > > +++ b/controller/binding.c
> > > > @@ -630,6 +630,11 @@ static struct binding_lport
> *local_binding_add_lport(
> > > >      enum en_lport_type);
> > > >  static struct binding_lport *local_binding_get_primary_lport(
> > > >      struct local_binding *);
> > > > +static struct binding_lport *local_binding_get_first_lport(
> > > > +    struct local_binding *lbinding);
> > > > +static struct binding_lport
> > > *local_binding_get_primary_or_localport_lport(
> > > > +    struct local_binding *lbinding);
> > > > +
> > > >  static bool local_binding_handle_stale_binding_lports(
> > > >      struct local_binding *lbinding, struct binding_ctx_in *b_ctx_in,
> > > >      struct binding_ctx_out *b_ctx_out, struct hmap *qos_map);
> > > > @@ -815,12 +820,15 @@ binding_dump_local_bindings(struct
> > > local_binding_data *lbinding_data,
> > > >          if (num_lports) {
> > > >              struct shash child_lports =
> SHASH_INITIALIZER(&child_lports);
> > > >              struct binding_lport *primary_lport = NULL;
> > > > +            struct binding_lport *localport_lport = NULL;
> > > >              struct binding_lport *b_lport;
> > > >              bool first_elem = true;
> > > >
> > > >              LIST_FOR_EACH (b_lport, list_node,
> > > &lbinding->binding_lports) {
> > > >                  if (first_elem && b_lport->type == LP_VIF) {
> > > >                      primary_lport = b_lport;
> > > > +                } else if (first_elem && b_lport->type ==
> LP_LOCALPORT) {
> > > > +                    localport_lport = b_lport;
> > > >                  } else {
> > > >                      shash_add(&child_lports, b_lport->name, b_lport);
> > > >                  }
> > > > @@ -830,6 +838,9 @@ binding_dump_local_bindings(struct
> local_binding_data
> > > *lbinding_data,
> > > >              if (primary_lport) {
> > > >                  ds_put_format(out_data, "primary lport : [%s]\n",
> > > >                                primary_lport->name);
> > > > +            } else if (localport_lport) {
> > > > +                ds_put_format(out_data, "localport lport : [%s]\n",
> > > > +                              localport_lport->name);
> > > >              } else {
> > > >                  ds_put_format(out_data, "no primary lport\n");
> > > >              }
> > > > @@ -1019,8 +1030,7 @@ claim_lport(const struct sbrec_port_binding *pb,
> > > >   * Caller should make sure that this is the case.
> > > >   */
> > > >  static bool
> > > > -release_lport(const struct sbrec_port_binding *pb, bool sb_readonly,
> > > > -              struct hmap *tracked_datapaths, struct if_status_mgr
> > > *if_mgr)
> > > > +release_lport_(const struct sbrec_port_binding *pb, bool sb_readonly)
> > > >  {
> > > >      if (pb->encap) {
> > > >          if (sb_readonly) {
> > > > @@ -1043,9 +1053,20 @@ release_lport(const struct sbrec_port_binding
> *pb,
> > > bool sb_readonly,
> > > >          sbrec_port_binding_set_virtual_parent(pb, NULL);
> > > >      }
> > > >
> > > > +    VLOG_INFO("Releasing lport %s from this chassis.",
> pb->logical_port);
> > > > +    return true;
> > > > +}
> > > > +
> > > > +static bool
> > > > +release_lport(const struct sbrec_port_binding *pb, bool sb_readonly,
> > > > +              struct hmap *tracked_datapaths, struct if_status_mgr
> > > *if_mgr)
> > > > +{
> > > > +    if (!release_lport_(pb, sb_readonly)) {
> > > > +        return false;
> > > > +    }
> > > > +
> > > >      update_lport_tracking(pb, tracked_datapaths, false);
> > > >      if_status_mgr_release_iface(if_mgr, pb->logical_port);
> > > > -    VLOG_INFO("Releasing lport %s from this chassis.",
> pb->logical_port);
> > > >      return true;
> > > >  }
> > > >
> > > > @@ -1332,6 +1353,36 @@ consider_virtual_lport(const struct
> > > sbrec_port_binding *pb,
> > > >      return true;
> > > >  }
> > > >
> > > > +static bool
> > > > +consider_localport(const struct sbrec_port_binding *pb,
> > > > +                   struct binding_ctx_in *b_ctx_in,
> > > > +                   struct binding_ctx_out *b_ctx_out)
> > > > +{
> > > > +    struct shash *local_bindings =
> &b_ctx_out->lbinding_data->bindings;
> > > > +    struct local_binding *lbinding =
> local_binding_find(local_bindings,
> > > > +
> > >  pb->logical_port);
> > > > +
> > > > +    if (!lbinding) {
> > > > +        return true;
> > > > +    }
> > > > +
> > > > +    local_binding_add_lport(&b_ctx_out->lbinding_data->lports,
> lbinding,
> > > pb,
> > > > +                            LP_LOCALPORT);
> > > > +
> > > > +    /* If the port binding is claimed, then release it as localport
> is
> > > claimed
> > > > +     * by any ovn-controller. */
> > > > +    if (pb->chassis == b_ctx_in->chassis_rec) {
> > > > +        if (!release_lport_(pb, !b_ctx_in->ovnsb_idl_txn)) {
> > > > +            return false;
> > > > +        }
> > > > +
> > > > +        remove_related_lport(pb, b_ctx_out);
> > > > +    }
> > > > +
> > > > +    update_related_lport(pb, b_ctx_out);
> > > > +    return true;
> > > > +}
> > > > +
> > > >  /* Considers either claiming the lport or releasing the lport
> > > >   * for non VIF lports.
> > > >   */
> > > > @@ -1561,11 +1612,14 @@ binding_run(struct binding_ctx_in *b_ctx_in,
> > > struct binding_ctx_out *b_ctx_out)
> > > >
> > > >          switch (lport_type) {
> > > >          case LP_PATCH:
> > > > -        case LP_LOCALPORT:
> > > >          case LP_VTEP:
> > > >              update_related_lport(pb, b_ctx_out);
> > > >              break;
> > > >
> > > > +        case LP_LOCALPORT:
> > > > +            consider_localport(pb, b_ctx_in, b_ctx_out);
> > > > +            break;
> > > > +
> > > >          case LP_VIF:
> > > >              consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL,
> > > qos_map_ptr);
> > > >              break;
> > > > @@ -1793,14 +1847,18 @@ consider_iface_claim(const struct
> > > ovsrec_interface *iface_rec,
> > > >          lbinding->iface = iface_rec;
> > > >      }
> > > >
> > > > -    struct binding_lport *b_lport =
> > > local_binding_get_primary_lport(lbinding);
> > > > +    struct binding_lport *b_lport =
> > > > +        local_binding_get_primary_or_localport_lport(lbinding);
> > > >      const struct sbrec_port_binding *pb = NULL;
> > > >      if (!b_lport) {
> > > >          pb =
> lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name,
> > > >                                    lbinding->name);
> > > > -        if (pb && get_lport_type(pb) == LP_VIF) {
> > > > -            b_lport = local_binding_add_lport(binding_lports,
> lbinding,
> > > pb,
> > > > -                                              LP_VIF);
> > > > +        if (pb) {
> > > > +            enum en_lport_type lport_type = get_lport_type(pb);
> > > > +            if (lport_type == LP_VIF || lport_type == LP_LOCALPORT) {
> > > > +                b_lport = local_binding_add_lport(binding_lports,
> > > lbinding, pb,
> > > > +                                                  lport_type);
> > >
> > > This logic looks redundant. Both consider_localport() and
> > > consider_vif_lport() below would call local_binding_add_lport(), so can
> > > this inner if-block be removed?
> >
> > I did the below changes as per your suggestion and it breaks quite a
> > few test cases.
> >
> > *****************************************
> > diff --git a/controller/binding.c b/controller/binding.c
> > index 93ee0372b..2cd92f188 100644
> > --- a/controller/binding.c
> > +++ b/controller/binding.c
> > @@ -1838,7 +1838,6 @@ consider_iface_claim(const struct
> > ovsrec_interface *iface_rec,
> >      smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id);
> >
> >      struct shash *local_bindings = &b_ctx_out->lbinding_data->bindings;
> > -    struct shash *binding_lports = &b_ctx_out->lbinding_data->lports;
> >      struct local_binding *lbinding = local_binding_find(local_bindings,
> >                                                          iface_id);
> >
> > @@ -1855,29 +1854,27 @@ consider_iface_claim(const struct
> > ovsrec_interface *iface_rec,
> >      if (!b_lport) {
> >          pb = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name,
> >                                    lbinding->name);
> > -        if (pb) {
> > -            enum en_lport_type lport_type = get_lport_type(pb);
> > -            if (lport_type == LP_VIF || lport_type == LP_LOCALPORT) {
> > -                b_lport = local_binding_add_lport(binding_lports,
> lbinding, pb,
> > -                                                  lport_type);
> > -            }
> > -        }
> > +    } else {
> > +        pb = b_lport->pb;
> >      }
> >
> > -    if (!b_lport) {
> > -        /* There is no binding lport for this local binding. */
> > +    if (!pb) {
> > +        /* There is no port_binding row for this local binding. */
> >          return true;
> >      }
> >
> > -    if (b_lport->type == LP_LOCALPORT) {
> > +    enum en_lport_type lport_type = get_lport_type(pb);
> > +    if (lport_type == LP_LOCALPORT) {
> >          return consider_localport(pb, b_ctx_in, b_ctx_out);
> >      }
> >
> > -    if (!consider_vif_lport(b_lport->pb, b_ctx_in, b_ctx_out,
> > -                            lbinding, qos_map)) {
> > +    if (!consider_vif_lport(pb, b_ctx_in, b_ctx_out, lbinding, qos_map))
> {
> >          return false;
> >      }
> >
> > +    /* Get the (updated) b_lport again for the lbinding. */
> > +    b_lport = local_binding_get_primary_lport(lbinding);
> > +
> >      /* Update the child local_binding's iface (if any children) and try
> to
> >       *  claim the container lbindings. */
> >      LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) {
> >
> > ************************************************
> > Unfortunately we can't remove it and it is not redundant for the
> > following reasons:
> >
> > 1.  Lets say there is a logical port - sw0-p1 and it is claimed by
> > ovn-controller.
> >      So there will be a "local_binding" entry for sw0-p1 and also a
> > binding_lport entry
> >      in the local_binding->binding_lports list.
> >
> > 2.  Suppose if I run this command (and makes sw0-p1 as a container port
> whose
> >      parent 'foo' doesn't exist.
> >
> >      ovn-nbctl set logical_switch_port sw0-p1 parent_name=foo
> >
> > 3.  Then the port binding handler releases the lport - sw0-p1 and removes
> >      the external_ids:ovn-installed=true from the corresponding OVS port.
> >
> > 4.  When ovn-controller receives the update for the OVS port changes
> > done in the previous transaction,
> >      with your suggested change, ovn-controller claims back the ovs
> interface.
> >
> > Please let me know If you're still not convinced.   But I'd leave it
> > AS IS for now :).
> >
> > Thanks
> > Numan
> >
>
> I am not sure if I understand the logic completely, but the modified
> version looks more reasonable. Shouldn't it fix the scenario just by adding
> a check "if lport_type == LP_VIF" before calling consider_vif_lport()?
> If it still has the problem and you have to keep the current logic, then I
> think at least adding some comments to explain such scenarios and explain
> the exact purpose of each of these functions. At least it is hard for me to
> follow the code and reasoning about the scenario you mentioned above. But
> if it is possible I'd prefer to simplify the logic without extra comments.

You're right.  That works.  Thanks.
There's so many combinations of port types and changes that I'm a bit
hesitant to modify this
code (even though I've authored much of the code).

I'll update v3 with the changes you suggested.

Thanks
Numan


>
> >
> >
> > >
> > > > +            }
> > > >          }
> > > >      }
> > > >
> > > > @@ -1809,6 +1867,10 @@ consider_iface_claim(const struct
> ovsrec_interface
> > > *iface_rec,
> > > >          return true;
> > > >      }
> > > >
> > > > +    if (b_lport->type == LP_LOCALPORT) {
> > > > +        return consider_localport(pb, b_ctx_in, b_ctx_out);
> > > > +    }
> > > > +
> > > >      if (!consider_vif_lport(b_lport->pb, b_ctx_in, b_ctx_out,
> > > >                              lbinding, qos_map)) {
> > > >          return false;
> > > > @@ -1853,7 +1915,8 @@ consider_iface_release(const struct
> > > ovsrec_interface *iface_rec,
> > > >      struct shash *binding_lports = &b_ctx_out->lbinding_data->lports;
> > > >
> > > >      lbinding = local_binding_find(local_bindings, iface_id);
> > > > -    struct binding_lport *b_lport =
> > > local_binding_get_primary_lport(lbinding);
> > > > +    struct binding_lport *b_lport =
> > > > +        local_binding_get_primary_or_localport_lport(lbinding);
> > > >      if (is_binding_lport_this_chassis(b_lport,
> b_ctx_in->chassis_rec)) {
> > > >          struct local_datapath *ld =
> > > >              get_local_datapath(b_ctx_out->local_datapaths,
> > > > @@ -1874,6 +1937,10 @@ consider_iface_release(const struct
> > > ovsrec_interface *iface_rec,
> > > >              }
> > > >          }
> > > >
> > > > +    } else if (lbinding && b_lport && b_lport->type == LP_LOCALPORT)
> {
> > > > +        /* lbinding is associated with a localport.  Remove it from
> the
> > > > +         * related lports. */
> > > > +        remove_related_lport(b_lport->pb, b_ctx_out);
> > > >      }
> > > >
> > > >      if (lbinding) {
> > > > @@ -2209,6 +2276,8 @@ binding_handle_port_binding_changes(struct
> > > binding_ctx_in *b_ctx_in,
> > > >          SHASH_INITIALIZER(&deleted_virtual_pbs);
> > > >      struct shash deleted_vif_pbs =
> > > >          SHASH_INITIALIZER(&deleted_vif_pbs);
> > > > +    struct shash deleted_localport_pbs =
> > > > +        SHASH_INITIALIZER(&deleted_localport_pbs);
> > > >      struct shash deleted_other_pbs =
> > > >          SHASH_INITIALIZER(&deleted_other_pbs);
> > > >      const struct sbrec_port_binding *pb;
> > > > @@ -2243,6 +2312,8 @@ binding_handle_port_binding_changes(struct
> > > binding_ctx_in *b_ctx_in,
> > > >              shash_add(&deleted_container_pbs, pb->logical_port, pb);
> > > >          } else if (lport_type == LP_VIRTUAL) {
> > > >              shash_add(&deleted_virtual_pbs, pb->logical_port, pb);
> > > > +        } else if (lport_type == LP_LOCALPORT) {
> > > > +            shash_add(&deleted_localport_pbs, pb->logical_port, pb);
> > > >          } else {
> > > >              shash_add(&deleted_other_pbs, pb->logical_port, pb);
> > > >          }
> > > > @@ -2277,6 +2348,12 @@ binding_handle_port_binding_changes(struct
> > > binding_ctx_in *b_ctx_in,
> > > >          }
> > > >      }
> > > >
> > > > +    SHASH_FOR_EACH_SAFE (node, node_next, &deleted_localport_pbs) {
> > > > +        handle_deleted_vif_lport(node->data, LP_LOCALPORT, b_ctx_in,
> > > > +                                 b_ctx_out);
> > > > +        shash_delete(&deleted_localport_pbs, node);
> > > > +    }
> > > > +
> > > >      SHASH_FOR_EACH_SAFE (node, node_next, &deleted_other_pbs) {
> > > >          handle_deleted_lport(node->data, b_ctx_in, b_ctx_out);
> > > >          shash_delete(&deleted_other_pbs, node);
> > > > @@ -2286,6 +2363,7 @@ delete_done:
> > > >      shash_destroy(&deleted_container_pbs);
> > > >      shash_destroy(&deleted_virtual_pbs);
> > > >      shash_destroy(&deleted_vif_pbs);
> > > > +    shash_destroy(&deleted_localport_pbs);
> > > >      shash_destroy(&deleted_other_pbs);
> > > >
> > > >      if (!handled) {
> > > > @@ -2345,8 +2423,11 @@ delete_done:
> > > >                                                 b_ctx_out,
> qos_map_ptr);
> > > >              break;
> > > >
> > > > -        case LP_PATCH:
> > > >          case LP_LOCALPORT:
> > > > +            handled = consider_localport(pb, b_ctx_in, b_ctx_out);
> > > > +            break;
> > > > +
> > > > +        case LP_PATCH:
> > > >          case LP_VTEP:
> > > >              update_related_lport(pb, b_ctx_out);
> > > >              if (lport_type ==  LP_PATCH) {
> > > > @@ -2501,6 +2582,24 @@ local_binding_delete(struct local_binding
> > > *lbinding,
> > > >      local_binding_destroy(lbinding, binding_lports);
> > > >  }
> > > >
> > > > +static struct binding_lport *
> > > > +local_binding_get_first_lport(struct local_binding *lbinding)
> > > > +{
> > > > +    if (!lbinding) {
> > > > +        return NULL;
> > > > +    }
> > > > +
> > > > +    if (!ovs_list_is_empty(&lbinding->binding_lports)) {
> > > > +        struct binding_lport *b_lport = NULL;
> > > > +        b_lport =
> CONTAINER_OF(ovs_list_front(&lbinding->binding_lports),
> > > > +                               struct binding_lport, list_node);
> > > > +
> > > > +        return b_lport;
> > > > +    }
> > > > +
> > > > +    return NULL;
> > > > +}
> > > > +
> > > >  /* Returns the primary binding lport if present in lbinding's
> > > >   * binding lports list.  A binding lport is considered primary
> > > >   * if binding lport's type is LP_VIF and the name matches
> > > > @@ -2513,15 +2612,26 @@ local_binding_get_primary_lport(struct
> > > local_binding *lbinding)
> > > >          return NULL;
> > > >      }
> > > >
> > > > -    if (!ovs_list_is_empty(&lbinding->binding_lports)) {
> > > > -        struct binding_lport *b_lport = NULL;
> > > > -        b_lport =
> CONTAINER_OF(ovs_list_front(&lbinding->binding_lports),
> > > > -                               struct binding_lport, list_node);
> > > > -
> > > > -        if (b_lport->type == LP_VIF &&
> > > > +    struct binding_lport *b_lport =
> > > local_binding_get_first_lport(lbinding);
> > > > +    if (b_lport && b_lport->type == LP_VIF &&
> > > >              !strcmp(lbinding->name, b_lport->name)) {
> > > > -            return b_lport;
> > > > -        }
> > > > +        return b_lport;
> > > > +    }
> > > > +
> > > > +    return NULL;
> > > > +}
> > > > +
> > > > +static struct binding_lport *
> > > > +local_binding_get_primary_or_localport_lport(struct local_binding
> > > *lbinding)
> > > > +{
> > > > +    if (!lbinding) {
> > > > +        return NULL;
> > > > +    }
> > > > +
> > > > +    struct binding_lport *b_lport =
> > > local_binding_get_first_lport(lbinding);
> > > > +    if (b_lport && (b_lport->type == LP_VIF || b_lport->type ==
> > > LP_LOCALPORT)
> > > > +            && !strcmp(lbinding->name, b_lport->name)) {
> > > > +        return b_lport;
> > > >      }
> > > >
> > > >      return NULL;
> > > > @@ -2569,8 +2679,10 @@
> local_binding_handle_stale_binding_lports(struct
> > > local_binding *lbinding,
> > > >                                            struct binding_ctx_out
> > > *b_ctx_out,
> > > >                                            struct hmap *qos_map)
> > > >  {
> > > > -    /* Check if this lbinding has a primary binding_lport or not. */
> > > > -    struct binding_lport *p_lport =
> > > local_binding_get_primary_lport(lbinding);
> > > > +    /* Check if this lbinding has a primary binding_lport or
> > > > +     * localport binding_lport or not. */
> > > > +    struct binding_lport *p_lport =
> > > > +        local_binding_get_primary_or_localport_lport(lbinding);
> > > >      if (p_lport) {
> > > >          /* Nothing to be done. */
> > > >          return true;
> > > > @@ -2737,6 +2849,7 @@ binding_lport_check_and_cleanup(struct
> > > binding_lport *b_lport,
> > > >
> > > >      switch (b_lport->type) {
> > > >      case LP_VIF:
> > > > +    case LP_LOCALPORT:
> > > >          if (strcmp(b_lport->name, b_lport->lbinding->name)) {
> > > >              cleanup_blport = true;
> > > >          }
> > > > @@ -2756,7 +2869,6 @@ binding_lport_check_and_cleanup(struct
> > > binding_lport *b_lport,
> > > >          break;
> > > >
> > > >      case LP_PATCH:
> > > > -    case LP_LOCALPORT:
> > > >      case LP_VTEP:
> > > >      case LP_L2GATEWAY:
> > > >      case LP_L3GATEWAY:
> > > > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> > > > index 9a2ebd7ce..11661a2fd 100644
> > > > --- a/tests/ovn-controller.at
> > > > +++ b/tests/ovn-controller.at
> > > > @@ -510,6 +510,18 @@ primary lport : [[lsp1]]
> > > >  ----------------------------------------
> > > >  ])
> > > >
> > > > +# Set the port type to localport
> > > > +check ovn-nbctl lsp-set-type lsp1 localport
> > > > +check as hv1 ovs-vsctl set open .
> external_ids:ovn-cms-options=localport
> > > > +OVS_WAIT_UNTIL([test localport = $(ovn-sbctl get chassis .
> > > other_config:ovn-cms-options)])
> > > > +
> > > > +AT_CHECK([as hv1 ovn-appctl -t ovn-controller
> > > debug/dump-local-bindings], [0], [dnl
> > > > +Local bindings:
> > > > +name: [[lsp1]], OVS interface name : [[vif1]], num binding lports :
> [[1]]
> > > > +localport lport : [[lsp1]]
> > > > +----------------------------------------
> > > > +])
> > > > +
> > > >  # pause ovn-northd
> > > >  check as northd ovn-appctl -t ovn-northd pause
> > > >  check as northd-backup ovn-appctl -t ovn-northd pause
> > > > @@ -527,22 +539,41 @@ do
> > > >          check as hv1 ovs-vsctl set open .
> > > external_ids:ovn-cms-options=$type
> > > >          OVS_WAIT_UNTIL([test $type = $(ovn-sbctl get chassis .
> > > other_config:ovn-cms-options)])
> > > >
> > > > -        AT_CHECK([as hv1 ovn-appctl -t ovn-controller
> > > debug/dump-local-bindings], [0], [dnl
> > > > +        if [[ "$type" == "localport" ]]; then
> > > > +            AT_CHECK([as hv1 ovn-appctl -t ovn-controller
> > > debug/dump-local-bindings], [0], [dnl
> > > > +Local bindings:
> > > > +name: [[lsp1]], OVS interface name : [[vif1]], num binding lports :
> [[1]]
> > > > +localport lport : [[lsp1]]
> > > > +----------------------------------------
> > > > +])
> > > > +        else
> > > > +            AT_CHECK([as hv1 ovn-appctl -t ovn-controller
> > > debug/dump-local-bindings], [0], [dnl
> > > >  Local bindings:
> > > >  name: [[lsp1]], OVS interface name : [[vif1]], num binding lports :
> [[0]]
> > > >  ----------------------------------------
> > > >  ])
> > > > +        fi
> > > >
> > > >          echo "Updating to $update_type from $type"
> > > >          check ovn-sbctl set port_binding lsp1 type=$update_type
> > > >          check as hv1 ovs-vsctl set open .
> > > external_ids:ovn-cms-options=$update_type
> > > >          OVS_WAIT_UNTIL([test $update_type = $(ovn-sbctl get chassis .
> > > other_config:ovn-cms-options)])
> > > >
> > > > -        AT_CHECK([as hv1 ovn-appctl -t ovn-controller
> > > debug/dump-local-bindings], [0], [dnl
> > > > +        if [[ "$update_type" == "localport" ]]; then
> > > > +            AT_CHECK([as hv1 ovn-appctl -t ovn-controller
> > > debug/dump-local-bindings], [0], [dnl
> > > > +Local bindings:
> > > > +name: [[lsp1]], OVS interface name : [[vif1]], num binding lports :
> [[1]]
> > > > +localport lport : [[lsp1]]
> > > > +----------------------------------------
> > > > +])
> > > > +        else
> > > > +            AT_CHECK([as hv1 ovn-appctl -t ovn-controller
> > > debug/dump-local-bindings], [0], [dnl
> > > >  Local bindings:
> > > >  name: [[lsp1]], OVS interface name : [[vif1]], num binding lports :
> [[0]]
> > > >  ----------------------------------------
> > > >  ])
> > > > +        fi
> > > > +
> > > >          # Set the port binding type back to VIF.
> > > >          check ovn-sbctl set port_binding lsp1 type=\"\"
> > > >          check as hv1 ovs-vsctl set open .
> > > external_ids:ovn-cms-options=foo
> > > > --
> > > > 2.31.1
> > > >
> > > > _______________________________________________
> > > > dev mailing list
> > > > dev@openvswitch.org
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>