[ovs-dev] [PATCH ovn v6 1/9] ovn-controller: Store the local port bindings in the runtime data I-P state.

Numan Siddique numans at ovn.org
Wed May 20 07:57:59 UTC 2020


On Wed, May 20, 2020 at 6:35 AM Han Zhou <hzhou at ovn.org> wrote:

> Hi Numan,
>
> Please see my comments below.
>
> Thanks,
> Han
>
> On Sat, May 16, 2020 at 10:56 AM <numans at ovn.org> wrote:
> >
> > From: Numan Siddique <numans at ovn.org>
> >
> > This patch adds a new data structure - 'struct local_binding' which
> represents
> > a probable local port binding. A new object of this structure is creared
> for
> > each interface present in the integration bridge (br-int) with the
> > external_ids:iface-id set. This struct has 2 main fields
> >  - 'struct ovsrec_interface *' and 'struct sbrec_port_binding *'. These
> fields
> > are updated during port claim and release.
> >
> > A shash of the local bindings is maintained with the 'iface-id' as the
> hash key
> > in the runtime data of the incremental processing engine.
> >
> > This patch helps in the upcoming patch to add incremental processing
> support
> > for OVS interface and SB port binding changes.
> >
> > This patch also refactors the port binding code by adding a port binding
> function
> > for each port binding type.
> >
> > Signed-off-by: Numan Siddique <numans at ovn.org>
> > ---
> >  controller/binding.c        | 993 ++++++++++++++++++++++++------------
> >  controller/binding.h        |  14 +-
> >  controller/ovn-controller.c |  49 +-
> >  3 files changed, 700 insertions(+), 356 deletions(-)
> >
> > diff --git a/controller/binding.c b/controller/binding.c
> > index a5525a310..0c215d8d6 100644
> > --- a/controller/binding.c
> > +++ b/controller/binding.c
> > @@ -69,47 +69,6 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl)
> >      ovsdb_idl_add_column(ovs_idl, &ovsrec_qos_col_type);
> >  }
> >
> > -static void
> > -get_local_iface_ids(const struct ovsrec_bridge *br_int,
> > -                    struct shash *lport_to_iface,
> > -                    struct sset *local_lports,
> > -                    struct sset *egress_ifaces)
> > -{
> > -    int i;
> > -
> > -    for (i = 0; i < br_int->n_ports; i++) {
> > -        const struct ovsrec_port *port_rec = br_int->ports[i];
> > -        const char *iface_id;
> > -        int j;
> > -
> > -        if (!strcmp(port_rec->name, br_int->name)) {
> > -            continue;
> > -        }
> > -
> > -        for (j = 0; j < port_rec->n_interfaces; j++) {
> > -            const struct ovsrec_interface *iface_rec;
> > -
> > -            iface_rec = port_rec->interfaces[j];
> > -            iface_id = smap_get(&iface_rec->external_ids, "iface-id");
> > -            int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport :
> 0;
> > -
> > -            if (iface_id && ofport > 0) {
> > -                shash_add(lport_to_iface, iface_id, iface_rec);
> > -                sset_add(local_lports, iface_id);
> > -            }
> > -
> > -            /* Check if this is a tunnel interface. */
> > -            if (smap_get(&iface_rec->options, "remote_ip")) {
> > -                const char *tunnel_iface
> > -                    = smap_get(&iface_rec->status,
> "tunnel_egress_iface");
> > -                if (tunnel_iface) {
> > -                    sset_add(egress_ifaces, tunnel_iface);
> > -                }
> > -            }
> > -        }
> > -    }
> > -}
> > -
> >  static void
> >  add_local_datapath__(struct ovsdb_idl_index
> *sbrec_datapath_binding_by_key,
> >                       struct ovsdb_idl_index
> *sbrec_port_binding_by_datapath,
> > @@ -448,219 +407,6 @@ sbrec_get_port_encap(const struct sbrec_chassis
> *chassis_rec,
> >      return best_encap;
> >  }
> >
> > -static bool
> > -is_our_chassis(const struct sbrec_chassis *chassis_rec,
> > -               const struct sbrec_port_binding *binding_rec,
> > -               const struct sset *active_tunnels,
> > -               const struct ovsrec_interface *iface_rec,
> > -               const struct sset *local_lports)
> > -{
> > -    /* Ports of type "virtual" should never be explicitly bound to an
> OVS
> > -     * port in the integration bridge. If that's the case, ignore the
> binding
> > -     * and log a warning.
> > -     */
> > -    if (iface_rec && !strcmp(binding_rec->type, "virtual")) {
> > -        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > -        VLOG_WARN_RL(&rl,
> > -                     "Virtual port %s should not be bound to OVS port
> %s",
> > -                     binding_rec->logical_port, iface_rec->name);
> > -        return false;
> > -    }
> > -
> > -    bool our_chassis = false;
> > -    if (iface_rec
> > -        || (binding_rec->parent_port && binding_rec->parent_port[0] &&
> > -            sset_contains(local_lports, binding_rec->parent_port))) {
> > -        /* This port is in our chassis unless it is a localport. */
> > -        our_chassis = strcmp(binding_rec->type, "localport");
> > -    } else if (!strcmp(binding_rec->type, "l2gateway")) {
> > -        const char *chassis_id = smap_get(&binding_rec->options,
> > -                                          "l2gateway-chassis");
> > -        our_chassis = chassis_id && !strcmp(chassis_id,
> chassis_rec->name);
> > -    } else if (!strcmp(binding_rec->type, "chassisredirect") ||
> > -               !strcmp(binding_rec->type, "external")) {
> > -        our_chassis =
> ha_chassis_group_contains(binding_rec->ha_chassis_group,
> > -                                                chassis_rec) &&
> > -
>  ha_chassis_group_is_active(binding_rec->ha_chassis_group,
> > -                                                 active_tunnels,
> chassis_rec);
> > -    } else if (!strcmp(binding_rec->type, "l3gateway")) {
> > -        const char *chassis_id = smap_get(&binding_rec->options,
> > -                                          "l3gateway-chassis");
> > -        our_chassis = chassis_id && !strcmp(chassis_id,
> chassis_rec->name);
> > -    }
> > -
> > -    return our_chassis;
> > -}
> > -
> > -/* Updates 'binding_rec' and if the port binding is local also updates
> the
> > - * local datapaths and ports.
> > - * Updates and returns the array of local virtual ports that will
> require
> > - * additional processing.
> > - */
> > -static const struct sbrec_port_binding **
> > -consider_local_datapath(const struct sbrec_port_binding *binding_rec,
> > -                        const struct ovsrec_interface *iface_rec,
> > -                        struct binding_ctx_in *b_ctx_in,
> > -                        struct binding_ctx_out *b_ctx_out,
> > -                        struct hmap *qos_map,
> > -                        const struct sbrec_port_binding **vpbs,
> > -                        size_t *n_vpbs, size_t *n_allocated_vpbs)
> > -{
> > -    bool our_chassis = is_our_chassis(b_ctx_in->chassis_rec,
> binding_rec,
> > -                                      b_ctx_in->active_tunnels,
> iface_rec,
> > -                                      b_ctx_out->local_lports);
> > -    if (iface_rec
> > -        || (binding_rec->parent_port && binding_rec->parent_port[0] &&
> > -            sset_contains(b_ctx_out->local_lports,
> > -                          binding_rec->parent_port))) {
> > -        if (binding_rec->parent_port && binding_rec->parent_port[0]) {
> > -            /* Add child logical port to the set of all local ports. */
> > -            sset_add(b_ctx_out->local_lports,
> binding_rec->logical_port);
> > -        }
> > -        add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key,
> > -                           b_ctx_in->sbrec_port_binding_by_datapath,
> > -                           b_ctx_in->sbrec_port_binding_by_name,
> > -                           binding_rec->datapath, false,
> > -                           b_ctx_out->local_datapaths);
> > -        if (iface_rec && qos_map && b_ctx_in->ovs_idl_txn) {
> > -            get_qos_params(binding_rec, qos_map);
> > -        }
> > -    } else if (!strcmp(binding_rec->type, "l2gateway")) {
> > -        if (our_chassis) {
> > -            sset_add(b_ctx_out->local_lports,
> binding_rec->logical_port);
> > -            add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key,
> > -                               b_ctx_in->sbrec_port_binding_by_datapath,
> > -                               b_ctx_in->sbrec_port_binding_by_name,
> > -                               binding_rec->datapath, false,
> > -                               b_ctx_out->local_datapaths);
> > -        }
> > -    } else if (!strcmp(binding_rec->type, "chassisredirect")) {
> > -        if (ha_chassis_group_contains(binding_rec->ha_chassis_group,
> > -                                      b_ctx_in->chassis_rec)) {
> > -            add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key,
> > -                               b_ctx_in->sbrec_port_binding_by_datapath,
> > -                               b_ctx_in->sbrec_port_binding_by_name,
> > -                               binding_rec->datapath, false,
> > -                               b_ctx_out->local_datapaths);
> > -        }
> > -    } else if (!strcmp(binding_rec->type, "l3gateway")) {
> > -        if (our_chassis) {
> > -            add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key,
> > -                               b_ctx_in->sbrec_port_binding_by_datapath,
> > -                               b_ctx_in->sbrec_port_binding_by_name,
> > -                               binding_rec->datapath, true,
> > -                               b_ctx_out->local_datapaths);
> > -        }
> > -    } else if (!strcmp(binding_rec->type, "localnet")) {
> > -        /* Add all localnet ports to local_lports so that we allocate ct
> zones
> > -         * for them. */
> > -        sset_add(b_ctx_out->local_lports, binding_rec->logical_port);
> > -        if (qos_map && b_ctx_in->ovs_idl_txn) {
> > -            get_qos_params(binding_rec, qos_map);
> > -        }
> > -    } else if (!strcmp(binding_rec->type, "external")) {
> > -        if (ha_chassis_group_contains(binding_rec->ha_chassis_group,
> > -                                      b_ctx_in->chassis_rec)) {
> > -            add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key,
> > -                               b_ctx_in->sbrec_port_binding_by_datapath,
> > -                               b_ctx_in->sbrec_port_binding_by_name,
> > -                               binding_rec->datapath, false,
> > -                               b_ctx_out->local_datapaths);
> > -        }
> > -    }
> > -
> > -    if (our_chassis
> > -        || !strcmp(binding_rec->type, "patch")
> > -        || !strcmp(binding_rec->type, "localport")
> > -        || !strcmp(binding_rec->type, "vtep")
> > -        || !strcmp(binding_rec->type, "localnet")) {
> > -        update_local_lport_ids(b_ctx_out->local_lport_ids, binding_rec);
> > -    }
> > -
> > -    ovs_assert(b_ctx_in->ovnsb_idl_txn);
> > -    const char *vif_chassis = smap_get(&binding_rec->options,
> > -                                           "requested-chassis");
> > -    bool can_bind = !vif_chassis || !vif_chassis[0]
> > -                    || !strcmp(vif_chassis, b_ctx_in->chassis_rec->name)
> > -                    || !strcmp(vif_chassis,
> b_ctx_in->chassis_rec->hostname);
> > -
> > -    if (can_bind && our_chassis) {
> > -        if (binding_rec->chassis != b_ctx_in->chassis_rec) {
> > -            if (binding_rec->chassis) {
> > -                VLOG_INFO("Changing chassis for lport %s from %s to
> %s.",
> > -                          binding_rec->logical_port,
> > -                          binding_rec->chassis->name,
> > -                          b_ctx_in->chassis_rec->name);
> > -            } else {
> > -                VLOG_INFO("Claiming lport %s for this chassis.",
> > -                          binding_rec->logical_port);
> > -            }
> > -            for (int i = 0; i < binding_rec->n_mac; i++) {
> > -                VLOG_INFO("%s: Claiming %s",
> > -                          binding_rec->logical_port,
> binding_rec->mac[i]);
> > -            }
> > -            sbrec_port_binding_set_chassis(binding_rec,
> b_ctx_in->chassis_rec);
> > -        }
> > -        /* Check if the port encap binding, if any, has changed */
> > -        struct sbrec_encap *encap_rec =
> > -            sbrec_get_port_encap(b_ctx_in->chassis_rec, iface_rec);
> > -        if (encap_rec && binding_rec->encap != encap_rec) {
> > -            sbrec_port_binding_set_encap(binding_rec, encap_rec);
> > -        }
> > -    } else if (binding_rec->chassis == b_ctx_in->chassis_rec) {
> > -        if (!strcmp(binding_rec->type, "virtual")) {
> > -            if (*n_vpbs == *n_allocated_vpbs) {
> > -                vpbs = x2nrealloc(vpbs, n_allocated_vpbs, sizeof *vpbs);
> > -            }
> > -            vpbs[(*n_vpbs)] = binding_rec;
> > -            (*n_vpbs)++;
> > -        } else {
> > -            VLOG_INFO("Releasing lport %s from this chassis.",
> > -                      binding_rec->logical_port);
> > -            if (binding_rec->encap) {
> > -                sbrec_port_binding_set_encap(binding_rec, NULL);
> > -            }
> > -            sbrec_port_binding_set_chassis(binding_rec, NULL);
> > -        }
> > -    } else if (our_chassis) {
> > -        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > -        VLOG_INFO_RL(&rl,
> > -                     "Not claiming lport %s, chassis %s "
> > -                     "requested-chassis %s",
> > -                     binding_rec->logical_port,
> b_ctx_in->chassis_rec->name,
> > -                     vif_chassis);
> > -    }
> > -
> > -    return vpbs;
> > -}
> > -
> > -static void
> > -consider_local_virtual_port(struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
> > -                            const struct sbrec_chassis *chassis_rec,
> > -                            const struct sbrec_port_binding
> *binding_rec)
> > -{
> > -    if (binding_rec->virtual_parent) {
> > -        const struct sbrec_port_binding *parent =
> > -            lport_lookup_by_name(sbrec_port_binding_by_name,
> > -                                 binding_rec->virtual_parent);
> > -        if (parent && parent->chassis == chassis_rec) {
> > -            return;
> > -        }
> > -    }
> > -
> > -    /* pinctrl module takes care of binding the ports of type 'virtual'.
> > -     * Release such ports if their virtual parents are no longer claimed
> by
> > -     * this chassis.
> > -     */
> > -    VLOG_INFO("Releasing lport %s from this chassis.",
> > -              binding_rec->logical_port);
> > -    if (binding_rec->encap) {
> > -        sbrec_port_binding_set_encap(binding_rec, NULL);
> > -    }
> > -    sbrec_port_binding_set_chassis(binding_rec, NULL);
> > -    sbrec_port_binding_set_virtual_parent(binding_rec, NULL);
> > -}
> > -
> >  static void
> >  add_localnet_egress_interface_mappings(
> >          const struct sbrec_port_binding *port_binding,
> > @@ -723,6 +469,586 @@ consider_localnet_port(const struct
> sbrec_port_binding *binding_rec,
> >      ld->localnet_port = binding_rec;
> >  }
> >
> > +/* Local bindings. binding.c module binds the logical port (represented
> by
> > + * Port_Binding rows) and sets the 'chassis' column when it is sees the
> > + * OVS interface row (of type "" or "internal") with the
> > + * external_ids:iface-id=<logical_port name> set.
> > + *
> > + * This module also manages the other port_bindings.
> > + *
> > + * To better manage the local bindings with the associated OVS
> interfaces,
> > + * 'struct local_binding' is used. A shash of these local bindings is
> > + * maintained with the 'external_ids:iface-id' as the key to the shash.
> > + *
> > + * struct local_binding has 3 main fields:
> > + *    - type
> > + *    - OVS interface row object
> > + *    - Port_Binding row object
> > + *
> > + * An instance of 'struct local_binding' can be one of 3 types.
> > + *
> > + *  BT_VIF:     Represent a local binding for an OVS interface of
> > + *              type "" or "internal" with the external_ids:iface-id
> > + *              set.
> > + *
> > + *              This can be a
> > + *                 * probable local binding - external_ids:iface-id is
> > + *                   set, but the corresponding Port_Binding row is not
> > + *                   created or is not visible to the local
> ovn-controller
> > + *                   instance.
> > + *
> > + *                 * a local binding - external_ids:iface-id is set and
> > + *                   which is already bound to the corresponding
> Port_Binding
> > + *                   row.
> > + *
> > + *              It maintains a list of children
> > + *              (of type BT_CONTAINER/BT_VIRTUAL) if any.
> > + *
> > + *  BT_CONTAINER:   Represents a local binding which has a parent of
> type
> > + *                  BT_VIF. Its Port_Binding row's 'parent' column is
> set to
> > + *                  its parent's Port_Binding. It shares the OVS
> interface row
> > + *                  with the parent.
> > + *
> > + *  BT_VIRTUAL: Represents a local binding which has a parent of type
> BT_VIF.
> > + *              Its Port_Binding type is "virtual" and it shares the OVS
> > + *              interface row with the parent.
> > + *              Port_Binding of type "virtual" is claimed by pinctrl
> module
> > + *              when it sees the ARP packet from the parent's VIF.
> > + *
> > + */
> > +enum local_binding_type {
> > +    BT_VIF,
> > +    BT_CONTAINER,
> > +    BT_VIRTUAL
> > +};
> > +
> > +struct local_binding {
> > +    char *name;
> > +    enum local_binding_type type;
> > +    const struct ovsrec_interface *iface;
> > +    const struct sbrec_port_binding *pb;
> > +
> > +    /* shash of 'struct local_binding' representing children. */
> > +    struct shash children;
> > +};
> > +
> > +static struct local_binding *
> > +local_binding_create(const char *name, const struct ovsrec_interface
> *iface,
> > +                     const struct sbrec_port_binding *pb,
> > +                     enum local_binding_type type)
> > +{
> > +    struct local_binding *lbinding = xzalloc(sizeof *lbinding);
> > +    lbinding->name = xstrdup(name);
> > +    lbinding->type = type;
> > +    lbinding->pb = pb;
> > +    lbinding->iface = iface;
> > +    shash_init(&lbinding->children);
> > +    return lbinding;
> > +}
> > +
> > +static void
> > +local_binding_add(struct shash *local_bindings, struct local_binding
> *lbinding)
> > +{
> > +    shash_add(local_bindings, lbinding->name, lbinding);
> > +}
> > +
> > +static struct local_binding *
> > +local_binding_find(struct shash *local_bindings, const char *name)
> > +{
> > +    return shash_find_data(local_bindings, name);
> > +}
> > +
> > +static void
> > +local_binding_destroy(struct local_binding *lbinding)
> > +{
> > +    local_bindings_destroy(&lbinding->children);
> > +
> > +    free(lbinding->name);
> > +    free(lbinding);
> > +}
> > +
> > +void
> > +local_bindings_init(struct shash *local_bindings)
> > +{
> > +    shash_init(local_bindings);
> > +}
> > +
> > +void
> > +local_bindings_destroy(struct shash *local_bindings)
> > +{
> > +    struct shash_node *node, *next;
> > +    SHASH_FOR_EACH_SAFE (node, next, local_bindings) {
> > +        struct local_binding *lbinding = node->data;
> > +        local_binding_destroy(lbinding);
> > +    }
> > +
> > +    shash_destroy(local_bindings);
> > +}
> > +
> > +static void
> > +local_binding_add_child(struct local_binding *lbinding,
> > +                        struct local_binding *child)
> > +{
> > +    local_binding_add(&lbinding->children, child);
> > +}
> > +
> > +static struct local_binding *
> > +local_binding_find_child(struct local_binding *lbinding,
> > +                         const char *child_name)
> > +{
> > +    return local_binding_find(&lbinding->children, child_name);
> > +}
> > +
> > +static bool
> > +is_lport_vif(const struct sbrec_port_binding *pb)
> > +{
> > +    return !pb->type[0];
> > +}
> > +
> > +static bool
> > +is_lport_container(const struct sbrec_port_binding *pb)
> > +{
> > +    return !pb->type[0] && pb->parent_port && pb->parent_port[0];
> > +}
> > +
> > +enum en_lport_type {
> > +    LP_VIF,
> > +    LP_PATCH,
> > +    LP_L3GATEWAY,
> > +    LP_LOCALNET,
> > +    LP_LOCALPORT,
> > +    LP_L2GATEWAY,
> > +    LP_VTEP,
> > +    LP_CHASSISREDIRECT,
> > +    LP_VIRTUAL,
> > +    LP_EXTERNAL
>
> LP_REMOTE is missing here.
>

Thanks for pointing this out.

Looks like ovn-sb.xml is missing the documentation of "remote" type (also
"external" type). I referred that to be sure
that I'm capturing all the types.

We need to update the documentation.


> > +};
> > +
> > +static enum en_lport_type
> > +get_lport_type(const struct sbrec_port_binding *pb)
> > +{
> > +    if (is_lport_vif(pb)) {
> > +        return LP_VIF;
> > +    } else if (!strcmp(pb->type, "patch")) {
> > +        return LP_PATCH;
> > +    } else if (!strcmp(pb->type, "chassisredirect")) {
> > +        return LP_CHASSISREDIRECT;
> > +    } else if (!strcmp(pb->type, "l3gateway")) {
> > +        return LP_L3GATEWAY;
> > +    } else if (!strcmp(pb->type, "localnet")) {
> > +        return LP_LOCALNET;
> > +    } else if (!strcmp(pb->type, "localport")) {
> > +        return LP_LOCALPORT;
> > +    } else if (!strcmp(pb->type, "l2gateway")) {
> > +        return LP_L2GATEWAY;
> > +    } else if (!strcmp(pb->type, "virtual")) {
> > +        return LP_VIRTUAL;
> > +    } else if (!strcmp(pb->type, "external")) {
> > +        return LP_EXTERNAL;
> > +    } else {
>
> 1. "remote" is missing here.
> 2. It is better to check "vtep" explicitly to avoid returning LP_VTEP for
> the types that we missed checking. We should let the default branch hit the
> OVS_NOT_REACHED line.
>


There is a big issue if assert here for unknown types. When ovn-northd is
updated with a new PB type,
ovn-controller will assert and the user has to be
update/upgade ovn-controller.

How shall we handle this ? Is it Ok to assert ? What do you think ?



> > +        return LP_VTEP;
> > +    }
> > +
> > +    OVS_NOT_REACHED();
> > +}
> > +
> > +static void
> > +claim_lport(const struct sbrec_port_binding *pb,
> > +            const struct sbrec_chassis *chassis_rec,
> > +            const struct ovsrec_interface *iface_rec)
> > +{
> > +    if (pb->chassis != chassis_rec) {
> > +        if (pb->chassis) {
> > +            VLOG_INFO("Changing chassis for lport %s from %s to %s.",
> > +                    pb->logical_port, pb->chassis->name,
> > +                    chassis_rec->name);
> > +        } else {
> > +            VLOG_INFO("Claiming lport %s for this chassis.",
> pb->logical_port);
> > +        }
> > +        for (int i = 0; i < pb->n_mac; i++) {
> > +            VLOG_INFO("%s: Claiming %s", pb->logical_port, pb->mac[i]);
> > +        }
> > +
> > +        sbrec_port_binding_set_chassis(pb, chassis_rec);
> > +    }
> > +
> > +    /* Check if the port encap binding, if any, has changed */
> > +    struct sbrec_encap *encap_rec =
> > +        sbrec_get_port_encap(chassis_rec, iface_rec);
> > +    if (encap_rec && pb->encap != encap_rec) {
> > +        sbrec_port_binding_set_encap(pb, encap_rec);
> > +    }
> > +}
> > +
> > +static void
> > +release_lport(const struct sbrec_port_binding *pb)
> > +{
> > +    if (!pb) {
> > +        return;
> > +    }
> > +
> > +    VLOG_INFO("Releasing lport %s from this chassis.",
> pb->logical_port);
> > +    if (pb->encap) {
> > +        sbrec_port_binding_set_encap(pb, NULL);
> > +    }
> > +    sbrec_port_binding_set_chassis(pb, NULL);
> > +
> > +    if (pb->virtual_parent) {
> > +        sbrec_port_binding_set_virtual_parent(pb, NULL);
> > +    }
> > +}
> > +
> > +static bool
> > +is_lbinding_set(struct local_binding *lbinding)
> > +{
> > +    return lbinding && lbinding->pb && lbinding->iface;
> > +}
> > +
> > +static bool
> > +is_lbinding_this_chassis(struct local_binding *lbinding,
> > +                         const struct sbrec_chassis *chassis)
> > +{
> > +    return lbinding && lbinding->pb && lbinding->pb->chassis == chassis;
> > +}
> > +
> > +static bool
> > +can_bind_on_this_chassis(const struct sbrec_chassis *chassis_rec,
> > +                         const char *requested_chassis)
> > +{
> > +    return !requested_chassis || !requested_chassis[0]
> > +           || !strcmp(requested_chassis, chassis_rec->name)
> > +           || !strcmp(requested_chassis, chassis_rec->hostname);
> > +}
> > +
> > +static void
> > +consider_vif_lport_(const struct sbrec_port_binding *pb,
> > +                    bool can_bind, const char *vif_chassis,
> > +                    struct binding_ctx_in *b_ctx_in,
> > +                    struct binding_ctx_out *b_ctx_out,
> > +                    struct local_binding *lbinding,
> > +                    struct hmap *qos_map)
> > +{
> > +    bool lbinding_set = is_lbinding_set(lbinding);
> > +    if (lbinding_set) {
> > +        if (can_bind) {
> > +            /* We can claim the lport. */
> > +            claim_lport(pb, b_ctx_in->chassis_rec, lbinding->iface);
> > +
> > +            add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key,
> > +                            b_ctx_in->sbrec_port_binding_by_datapath,
> > +                            b_ctx_in->sbrec_port_binding_by_name,
> > +                            pb->datapath, false,
> b_ctx_out->local_datapaths);
> > +            update_local_lport_ids(b_ctx_out->local_lport_ids, pb);
> > +            if (lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) {
> > +                get_qos_params(pb, qos_map);
> > +            }
> > +        } else {
> > +            /* We could, but can't claim the lport. */
> > +            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
> 1);
> > +                VLOG_INFO_RL(&rl,
> > +                            "Not claiming lport %s, chassis %s "
> > +                            "requested-chassis %s",
> > +                            pb->logical_port,
> > +                            b_ctx_in->chassis_rec->name,
> > +                            vif_chassis);
> > +        }
> > +    }
> > +
> > +    if (pb->chassis == b_ctx_in->chassis_rec) {
> > +        /* Release the lport if there is no lbinding. */
> > +        if (!lbinding_set || !can_bind) {
> > +            release_lport(pb);
> > +        }
> > +    }
> > +
> > +}
> > +
> > +static void
> > +consider_vif_lport(const struct sbrec_port_binding *pb,
> > +                   struct binding_ctx_in *b_ctx_in,
> > +                   struct binding_ctx_out *b_ctx_out,
> > +                   struct hmap *qos_map)
> > +{
> > +    const char *vif_chassis = smap_get(&pb->options,
> "requested-chassis");
> > +    bool can_bind = can_bind_on_this_chassis(b_ctx_in->chassis_rec,
> > +                                             vif_chassis);
> > +
> > +    struct local_binding *lbinding =
> > +        local_binding_find(b_ctx_out->local_bindings, pb->logical_port);
> > +
> > +    if (lbinding) {
> > +        lbinding->pb = pb;
>
> The lbinding should have been created with the correct pb in
> build_local_bindings(), so we should just assert here instead of
> reassigning it, which is confusing.
>

Not always. It is very much likely that ovs interface is created with
iface-id set
without a  port_binding row created.

Eg.

ovs-vsctl add port p1 -- set interface p1 external_ids:iface-id=sw0-p1
ovn-nbctl lsp-add ls0 sw0-p1

Please see the comments above about the "struct lbinding".



> > +    }
> > +    consider_vif_lport_(pb, can_bind, vif_chassis, b_ctx_in,
> > +                        b_ctx_out, lbinding, qos_map);
> > +}
> > +
> > +static void
> > +consider_container_lport(const struct sbrec_port_binding *pb,
> > +                         struct binding_ctx_in *b_ctx_in,
> > +                         struct binding_ctx_out *b_ctx_out,
> > +                         struct hmap *qos_map)
> > +{
> > +    struct local_binding *parent_lbinding;
> > +    parent_lbinding = local_binding_find(b_ctx_out->local_bindings,
> > +                                         pb->parent_port);
> > +
> > +    if (!parent_lbinding) {
> > +        /* There is no local_binding for parent port. Create it
> > +         * without OVS interface row. This is the only exception
> > +         * for creating the 'struct local_binding' object without
> > +         * corresponding OVS interface row.
> > +         *
> > +         * This is required for the following reasons:
> > +         *   - If a logical port P1 is created and then
> > +         *     few container ports - C1, C2, .. are created first by
> CMS.
> > +         *   - And later when OVS interface row  is created for P1, then
> > +         *     we want the these container ports also be claimed by the
> > +         *     chassis.
> > +         *
> > +         * Right now we don't handle OVS interface and SB Port_Binding
> > +         * changes incrementally. Once we do that, this book keeping
> > +         * is required.
> > +         * */
> > +        const struct sbrec_port_binding *parent =
> > +            lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name,
> > +                                 pb->parent_port);
> > +
> > +        parent_lbinding = local_binding_create(pb->parent_port, NULL,
> parent,
> > +                                               BT_VIF);
> > +        local_binding_add(b_ctx_out->local_bindings, parent_lbinding);
> > +    }
> > +
> > +    struct local_binding *container_lbinding =
> > +        local_binding_find_child(parent_lbinding, pb->logical_port);
> > +    if (!container_lbinding) {
> > +        container_lbinding = local_binding_create(pb->logical_port,
> > +
> parent_lbinding->iface,
> > +                                                  pb, BT_CONTAINER);
> > +        local_binding_add_child(parent_lbinding, container_lbinding);
> > +    } else {
>
> This seems impossible. We are just building the table, so if it is found
> already, then something must be wrong.
>
>
This can happen when we handle I-P for interface and port binding changes
in patch 2 as this function will be called from those handlers too.

Keeping that in mind, I added the code here. I'm fine removing this code in
patch 1
and then adding it in patch 2. Let me know what you think.


> > +        ovs_assert(container_lbinding->type == BT_CONTAINER);
> > +        container_lbinding->pb = pb;
> > +        container_lbinding->iface = parent_lbinding->iface;
> > +    }
> > +
> > +    if (!parent_lbinding->pb) {
>
> I didn't see how could this happen. Should it be assertion?
>

Same comment as above. It is more applicable in patch 2.
It can happen when the parent lport is deleted.



>
> > +        /* Call release_lport, to release the container lport, if
> > +         * it was bound earlier. */
> > +        if (is_lbinding_this_chassis(container_lbinding,
> > +                                     b_ctx_in->chassis_rec)) {
> > +            release_lport(pb);
> > +        }
> > +        return;
> > +    }
> > +
> > +    const char *vif_chassis = smap_get(&parent_lbinding->pb->options,
> > +                                       "requested-chassis");
> > +    bool can_bind = can_bind_on_this_chassis(b_ctx_in->chassis_rec,
> > +                                             vif_chassis);
> > +
> > +    consider_vif_lport_(pb, can_bind, vif_chassis, b_ctx_in, b_ctx_out,
> > +                        container_lbinding, qos_map);
> > +}
> > +
> > +static void
> > +consider_virtual_lport(const struct sbrec_port_binding *pb,
> > +                       struct binding_ctx_in *b_ctx_in,
> > +                       struct binding_ctx_out *b_ctx_out,
> > +                       struct hmap *qos_map)
> > +{
> > +    struct local_binding * parent_lbinding =
> > +        pb->virtual_parent ?
> local_binding_find(b_ctx_out->local_bindings,
> > +                                                pb->virtual_parent)
> > +        : NULL;
> > +
> > +    struct local_binding *virtual_lbinding = NULL;
> > +    if (is_lbinding_this_chassis(parent_lbinding,
> b_ctx_in->chassis_rec)) {
> > +        virtual_lbinding =
> > +            local_binding_find_child(parent_lbinding, pb->logical_port);
> > +        if (!virtual_lbinding) {
> > +            virtual_lbinding = local_binding_create(pb->logical_port,
> > +
>  parent_lbinding->iface,
> > +                                                    pb, BT_VIRTUAL);
> > +            local_binding_add_child(parent_lbinding, virtual_lbinding);
> > +        } else {
> > +            ovs_assert(virtual_lbinding->type == BT_VIRTUAL);
> > +            virtual_lbinding->pb = pb;
> > +            virtual_lbinding->iface = parent_lbinding->iface;
> > +        }
> > +    }
> > +
> > +    return consider_vif_lport_(pb, true, NULL, b_ctx_in, b_ctx_out,
> > +                               virtual_lbinding, qos_map);
> > +}
> > +
> > +/* Considers either claiming the lport or releasing the lport
> > + * for non VIF lports.
> > + */
> > +static void
> > +consider_nonvif_lport_(const struct sbrec_port_binding *pb,
> > +                       bool our_chassis,
> > +                       bool has_local_l3gateway,
> > +                       struct binding_ctx_in *b_ctx_in,
> > +                       struct binding_ctx_out *b_ctx_out)
> > +{
> > +    ovs_assert(b_ctx_in->ovnsb_idl_txn);
> > +    if (our_chassis) {
> > +        sset_add(b_ctx_out->local_lports, pb->logical_port);
> > +        add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key,
> > +                           b_ctx_in->sbrec_port_binding_by_datapath,
> > +                           b_ctx_in->sbrec_port_binding_by_name,
> > +                           pb->datapath, has_local_l3gateway,
> > +                           b_ctx_out->local_datapaths);
> > +
> > +        update_local_lport_ids(b_ctx_out->local_lport_ids, pb);
> > +        claim_lport(pb, b_ctx_in->chassis_rec, NULL);
> > +    } else if (pb->chassis == b_ctx_in->chassis_rec) {
> > +        release_lport(pb);
> > +    }
> > +}
> > +
> > +static void
> > +consider_l2gw_lport(const struct sbrec_port_binding *pb,
> > +                    struct binding_ctx_in *b_ctx_in,
> > +                    struct binding_ctx_out *b_ctx_out)
> > +{
> > +    const char *chassis_id = smap_get(&pb->options,
> "l2gateway-chassis");
> > +    bool our_chassis = chassis_id && !strcmp(chassis_id,
> > +
> b_ctx_in->chassis_rec->name);
> > +
> > +    consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in, b_ctx_out);
> > +}
> > +
> > +static void
> > +consider_l3gw_lport(const struct sbrec_port_binding *pb,
> > +                    struct binding_ctx_in *b_ctx_in,
> > +                    struct binding_ctx_out *b_ctx_out)
> > +{
> > +    const char *chassis_id = smap_get(&pb->options,
> "l3gateway-chassis");
> > +    bool our_chassis = chassis_id && !strcmp(chassis_id,
> > +
> b_ctx_in->chassis_rec->name);
> > +
> > +    consider_nonvif_lport_(pb, our_chassis, true, b_ctx_in, b_ctx_out);
> > +}
> > +
> > +static void
> > +consider_localnet_lport(const struct sbrec_port_binding *pb,
> > +                        struct binding_ctx_in *b_ctx_in,
> > +                        struct binding_ctx_out *b_ctx_out,
> > +                        struct hmap *qos_map)
> > +{
> > +    /* Add all localnet ports to local_lports so that we allocate ct
> zones
> > +     * for them. */
> > +    sset_add(b_ctx_out->local_lports, pb->logical_port);
> > +    if (qos_map && b_ctx_in->ovs_idl_txn) {
> > +        get_qos_params(pb, qos_map);
> > +    }
> > +
> > +    update_local_lport_ids(b_ctx_out->local_lport_ids, pb);
> > +}
> > +
> > +static void
> > +consider_ha_lport(const struct sbrec_port_binding *pb,
> > +                  struct binding_ctx_in *b_ctx_in,
> > +                  struct binding_ctx_out *b_ctx_out)
> > +{
> > +    bool our_chassis = false;
> > +    bool is_ha_chassis = ha_chassis_group_contains(pb->ha_chassis_group,
> > +
> b_ctx_in->chassis_rec);
> > +    our_chassis = is_ha_chassis &&
> > +                  ha_chassis_group_is_active(pb->ha_chassis_group,
> > +                                             b_ctx_in->active_tunnels,
> > +                                             b_ctx_in->chassis_rec);
> > +
> > +    if (is_ha_chassis && !our_chassis) {
> > +        /* If the chassis_rec is part of ha_chassis_group associated
> with
> > +         * the port_binding 'pb', we need to add to the local_datapath
> > +         * in even if its not active.
> > +         *
> > +         * If the chassis is active, consider_nonvif_lport_() takes care
> > +         * of adding the datapath of this 'pb' to local datapaths.
> > +         * */
> > +        add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key,
> > +                           b_ctx_in->sbrec_port_binding_by_datapath,
> > +                           b_ctx_in->sbrec_port_binding_by_name,
> > +                           pb->datapath, false,
> > +                           b_ctx_out->local_datapaths);
> > +    }
> > +
> > +    consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in, b_ctx_out);
> > +}
> > +
> > +static void
> > +consider_cr_lport(const struct sbrec_port_binding *pb,
> > +                  struct binding_ctx_in *b_ctx_in,
> > +                  struct binding_ctx_out *b_ctx_out)
> > +{
> > +    consider_ha_lport(pb, b_ctx_in, b_ctx_out);
> > +}
> > +
> > +static void
> > +consider_external_lport(const struct sbrec_port_binding *pb,
> > +                        struct binding_ctx_in *b_ctx_in,
> > +                        struct binding_ctx_out *b_ctx_out)
> > +{
> > +    return consider_ha_lport(pb, b_ctx_in, b_ctx_out);
> > +}
> > +
> > +/*
> > + * Builds local_bindings from the OVS interfaces.
> > + */
> > +static void
> > +build_local_bindings(struct binding_ctx_in *b_ctx_in,
> > +                     struct binding_ctx_out *b_ctx_out)
> > +{
> > +    int i;
> > +    for (i = 0; i < b_ctx_in->br_int->n_ports; i++) {
> > +        const struct ovsrec_port *port_rec = b_ctx_in->br_int->ports[i];
> > +        const char *iface_id;
> > +        int j;
> > +
> > +        if (!strcmp(port_rec->name, b_ctx_in->br_int->name)) {
> > +            continue;
> > +        }
> > +
> > +        for (j = 0; j < port_rec->n_interfaces; j++) {
> > +            const struct ovsrec_interface *iface_rec;
> > +
> > +            iface_rec = port_rec->interfaces[j];
> > +            iface_id = smap_get(&iface_rec->external_ids, "iface-id");
> > +            int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport :
> 0;
> > +
> > +            if (iface_id && ofport > 0) {
> > +                const struct sbrec_port_binding *pb
> > +                    = lport_lookup_by_name(
> > +                        b_ctx_in->sbrec_port_binding_by_name, iface_id);
> > +                struct local_binding *lbinding =
> > +                    local_binding_find(b_ctx_out->local_bindings,
> iface_id);
> > +                if (!lbinding) {
> > +                    lbinding = local_binding_create(iface_id, iface_rec,
> pb,
> > +                                                    BT_VIF);
> > +                    local_binding_add(b_ctx_out->local_bindings,
> lbinding);
> > +                } else {
>
> We are just building the table, so it shouldn't have existed. If it exists,
> it means something must be wrong, such as duplicated port-bindings on this
> chassis for same logical port.
>

I don't understand completely what you mean by duplicated port-bindings.
But this can happen in the below scenario -

ovs-vsctl add port p1 -- set interface p1 external_ids:iface-id=sw0-p1
ovs-vsctl add port p2 -- set interface p2 external_ids:iface-id=sw0-p1

Note that interface p1 and p2 has same iface-id set. And we create lbinding
object
with the iface-id/logical port as the key.

Dumitru had asked about this use case when we reviewed the p1 and we
discussed about
it here -
https://mail.openvswitch.org/pipermail/ovs-dev/2020-May/370379.html



>
> > +                    lbinding->type = BT_VIF;
> > +                    lbinding->pb = pb;
> > +                }
> > +
> > +                sset_add(b_ctx_out->local_lports, iface_id);
> > +            }
> > +
> > +            /* Check if this is a tunnel interface. */
> > +            if (smap_get(&iface_rec->options, "remote_ip")) {
> > +                const char *tunnel_iface
> > +                    = smap_get(&iface_rec->status,
> "tunnel_egress_iface");
> > +                if (tunnel_iface) {
> > +                    sset_add(b_ctx_out->egress_ifaces, tunnel_iface);
> > +                }
> > +            }
> > +        }
> > +    }
> > +}
> > +
> >  void
> >  binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out
> *b_ctx_out)
> >  {
> > @@ -730,106 +1056,122 @@ binding_run(struct binding_ctx_in *b_ctx_in,
> struct binding_ctx_out *b_ctx_out)
> >          return;
> >      }
> >
> > -    const struct sbrec_port_binding *binding_rec;
> >      struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings);
> > -    struct shash lport_to_iface = SHASH_INITIALIZER(&lport_to_iface);
> > -    struct sset egress_ifaces = SSET_INITIALIZER(&egress_ifaces);
> >      struct hmap qos_map;
> >
> >      hmap_init(&qos_map);
> >      if (b_ctx_in->br_int) {
> > -        get_local_iface_ids(b_ctx_in->br_int, &lport_to_iface,
> > -                            b_ctx_out->local_lports, &egress_ifaces);
> > +        build_local_bindings(b_ctx_in, b_ctx_out);
> >      }
> >
> > -    /* Array to store pointers to local virtual ports. It is populated
> by
> > -     * consider_local_datapath.
> > -     */
> > -    const struct sbrec_port_binding **vpbs = NULL;
> > -    size_t n_vpbs = 0;
> > -    size_t n_allocated_vpbs = 0;
> > +    struct hmap *qos_map_ptr =
> > +        !sset_is_empty(b_ctx_out->egress_ifaces) ? &qos_map : NULL;
> > +
> > +    struct ovs_list localnet_lports =
> OVS_LIST_INITIALIZER(&localnet_lports);
> > +
> > +    struct localnet_lport {
> > +        struct ovs_list list_node;
> > +        const struct sbrec_port_binding *pb;
> > +    };
> >
> >      /* Run through each binding record to see if it is resident on this
> >       * chassis and update the binding accordingly.  This includes both
> > -     * directly connected logical ports and children of those ports.
> > -     * Virtual ports are just added to vpbs array and will be processed
> > -     * later. This is special case for virtual ports is needed in order
> to
> > -     * make sure we update the virtual_parent port bindings first.
> > +     * directly connected logical ports and children of those ports
> > +     * (which also includes virtual ports).
> >       */
> > -    SBREC_PORT_BINDING_TABLE_FOR_EACH (binding_rec,
> > +    const struct sbrec_port_binding *pb;
> > +    SBREC_PORT_BINDING_TABLE_FOR_EACH (pb,
> >                                         b_ctx_in->port_binding_table) {
> > -        const struct ovsrec_interface *iface_rec
> > -            = shash_find_data(&lport_to_iface,
> binding_rec->logical_port);
> > -        vpbs =
> > -            consider_local_datapath(binding_rec,
> > -                                    iface_rec, b_ctx_in, b_ctx_out,
> > -                                    sset_is_empty(&egress_ifaces) ? NULL
> :
> > -                                    &qos_map,
> > -                                    vpbs, &n_vpbs, &n_allocated_vpbs);
> > -    }
> > -
> > -    /* Now also update the virtual ports in case their parent ports were
> > -     * updated above.
> > -     */
> > -    for (size_t i = 0; i < n_vpbs; i++) {
> > -
> consider_local_virtual_port(b_ctx_in->sbrec_port_binding_by_name,
> > -                                    b_ctx_in->chassis_rec, vpbs[i]);
> > +        enum en_lport_type lport_type = get_lport_type(pb);
> > +
> > +        switch (lport_type) {
> > +        case LP_PATCH:
> > +        case LP_LOCALPORT:
> > +        case LP_VTEP:
> > +            update_local_lport_ids(b_ctx_out->local_lport_ids, pb);
> > +            break;
> > +
> > +        case LP_VIF:
> > +            if (is_lport_container(pb)) {
> > +                consider_container_lport(pb, b_ctx_in, b_ctx_out,
> qos_map_ptr);
> > +            } else {
> > +                consider_vif_lport(pb, b_ctx_in, b_ctx_out,
> qos_map_ptr);
> > +            }
> > +            break;
> > +
> > +        case LP_VIRTUAL:
> > +            consider_virtual_lport(pb, b_ctx_in, b_ctx_out,
> qos_map_ptr);
> > +            break;
> > +
> > +        case LP_L2GATEWAY:
> > +            consider_l2gw_lport(pb, b_ctx_in, b_ctx_out);
> > +            break;
> > +
> > +        case LP_L3GATEWAY:
> > +            consider_l3gw_lport(pb, b_ctx_in, b_ctx_out);
> > +            break;
> > +
> > +        case LP_CHASSISREDIRECT:
> > +            consider_cr_lport(pb, b_ctx_in, b_ctx_out);
> > +            break;
> > +
> > +        case LP_EXTERNAL:
> > +            consider_external_lport(pb, b_ctx_in, b_ctx_out);
> > +            break;
> > +
> > +        case LP_LOCALNET: {
> > +            consider_localnet_lport(pb, b_ctx_in, b_ctx_out,
> qos_map_ptr);
> > +            struct localnet_lport *lnet_lport = xmalloc(sizeof
> *lnet_lport);
> > +            lnet_lport->pb = pb;
> > +            ovs_list_push_back(&localnet_lports,
> &lnet_lport->list_node);
> > +            break;
> > +        }
> > +        }
> >      }
> > -    free(vpbs);
> >
> >      add_ovs_bridge_mappings(b_ctx_in->ovs_table, b_ctx_in->bridge_table,
> >                              &bridge_mappings);
> >
> > -    /* Run through each binding record to see if it is a localnet port
> > +    /* Run through each localnet lport list to see if it is a localnet
> port
> >       * on local datapaths discovered from above loop, and update the
> >       * corresponding local datapath accordingly. */
> > -    SBREC_PORT_BINDING_TABLE_FOR_EACH (binding_rec,
> > -                                       b_ctx_in->port_binding_table) {
> > -        if (!strcmp(binding_rec->type, "localnet")) {
> > -            consider_localnet_port(binding_rec, &bridge_mappings,
> > -                                   &egress_ifaces,
> b_ctx_out->local_datapaths);
> > -        }
> > +    struct localnet_lport *lnet_lport;
> > +    LIST_FOR_EACH_POP (lnet_lport, list_node, &localnet_lports) {
> > +        consider_localnet_port(lnet_lport->pb, &bridge_mappings,
> > +                               b_ctx_out->egress_ifaces,
> > +                               b_ctx_out->local_datapaths);
> > +        free(lnet_lport);
> >      }
> > +
> >      shash_destroy(&bridge_mappings);
> >
> > -    if (!sset_is_empty(&egress_ifaces)
> > +    if (!sset_is_empty(b_ctx_out->egress_ifaces)
> >          && set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table,
> > -                        b_ctx_in->qos_table, &egress_ifaces)) {
> > +                        b_ctx_in->qos_table, b_ctx_out->egress_ifaces))
> {
> >          const char *entry;
> > -        SSET_FOR_EACH (entry, &egress_ifaces) {
> > +        SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) {
> >              setup_qos(entry, &qos_map);
> >          }
> >      }
> >
> > -    shash_destroy(&lport_to_iface);
> > -    sset_destroy(&egress_ifaces);
> >      destroy_qos_map(&qos_map);
> >  }
> >
> >  /* Returns true if port-binding changes potentially require flow changes
> on
> >   * the current chassis. Returns false if we are sure there is no impact.
> */
> >  bool
> > -binding_evaluate_port_binding_changes(
> > -        const struct sbrec_port_binding_table *pb_table,
> > -        const struct ovsrec_bridge *br_int,
> > -        const struct sbrec_chassis *chassis_rec,
> > -        struct sset *active_tunnels,
> > -        struct sset *local_lports)
> > +binding_evaluate_port_binding_changes(struct binding_ctx_in *b_ctx_in,
> > +                                      struct binding_ctx_out *b_ctx_out)
> >  {
> > -    if (!chassis_rec) {
> > +    if (!b_ctx_in->chassis_rec) {
> >          return true;
> >      }
> >
> >      bool changed = false;
> >
> >      const struct sbrec_port_binding *binding_rec;
> > -    struct shash lport_to_iface = SHASH_INITIALIZER(&lport_to_iface);
> > -    struct sset egress_ifaces = SSET_INITIALIZER(&egress_ifaces);
> > -    if (br_int) {
> > -        get_local_iface_ids(br_int, &lport_to_iface, local_lports,
> > -                            &egress_ifaces);
> > -    }
> > -    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (binding_rec, pb_table) {
> > +    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (binding_rec,
> > +
> b_ctx_in->port_binding_table) {
> >          /* XXX: currently OVSDB change tracking doesn't support getting
> old
> >           * data when the operation is update, so if a port-binding moved
> from
> >           * this chassis to another, there is no easy way to find out the
> > @@ -841,24 +1183,31 @@ binding_evaluate_port_binding_changes(
> >           *   interface table will be updated, which will trigger
> recompute.
> >           *
> >           * - If the port is not a regular VIF, always trigger recompute.
> */
> > -        if (binding_rec->chassis == chassis_rec) {
> > +        if (binding_rec->chassis == b_ctx_in->chassis_rec) {
> >              changed = true;
> >              break;
> >          }
> >
> > -        const struct ovsrec_interface *iface_rec
> > -            = shash_find_data(&lport_to_iface,
> binding_rec->logical_port);
> > -        if (is_our_chassis(chassis_rec, binding_rec, active_tunnels,
> iface_rec,
> > -                           local_lports) ||
> > -            (strcmp(binding_rec->type, "") && strcmp(binding_rec->type,
> > -                                                     "remote"))) {
> > +        if (strcmp(binding_rec->type, "")) {
>
> This condition should include "remote" because "remote" port change has no
> impact on runtime data: if (strcmp(binding_rec->type, "") &&
> strcmp(binding_rec->type, "remote"))
>

Ack. I misunderstood earlier. Dumitru also had a similar comment earlier.

I'll fix it in v7. But please note this function is deleted in patch 2.


>
> > +            changed = true;
> > +            break;
> > +        }
> > +
> > +        struct local_binding *lbinding = NULL;
> > +        if (!binding_rec->parent_port || !binding_rec->parent_port[0]) {
> > +            lbinding = local_binding_find(b_ctx_out->local_bindings,
> > +                                          binding_rec->logical_port);
> > +        } else {
> > +            lbinding = local_binding_find(b_ctx_out->local_bindings,
> > +                                          binding_rec->parent_port);
> > +        }
>
> With this, container port binding changes will always trigger recompute
> because lbinding is created for container port (and its parent port)
> regardless of the existence in interfaces. In fact, it is still not clear
> to me why we have to create lbindings for container ports. At least for
> this patch it seems not necessary. Maybe I will understand when I review
> the next patches.
>
>
In patch 2 this function is deleted.  So I think  it should be Ok for now ?

In patch 2 I've added a test case for this scenario.

Let's say the user creates P1, c1 and c2 (with c1 and c2 as container
ports).

When ovs port for P1 is created we have to bind P1, c1 and c2. And with I-P
for
port_binding changes this is required.

Although this book keeping of container ports is not required for this
patch, I added
it here keeping in mind the I-P for port binding/ovs interface changes.

Thanks
Numan


> +
> > +        if (lbinding) {
> >              changed = true;
> >              break;
> >          }
> >      }
> >
> > -    shash_destroy(&lport_to_iface);
> > -    sset_destroy(&egress_ifaces);
> >      return changed;
> >  }
> >
> > diff --git a/controller/binding.h b/controller/binding.h
> > index d0b8331af..9affc9a96 100644
> > --- a/controller/binding.h
> > +++ b/controller/binding.h
> > @@ -31,6 +31,8 @@ struct ovsrec_open_vswitch_table;
> >  struct sbrec_chassis;
> >  struct sbrec_port_binding_table;
> >  struct sset;
> > +struct sbrec_port_binding;
> > +struct shash;
> >
> >  struct binding_ctx_in {
> >      struct ovsdb_idl_txn *ovnsb_idl_txn;
> > @@ -51,8 +53,10 @@ struct binding_ctx_in {
> >
> >  struct binding_ctx_out {
> >      struct hmap *local_datapaths;
> > +    struct shash *local_bindings;
> >      struct sset *local_lports;
> >      struct sset *local_lport_ids;
> > +    struct sset *egress_ifaces;
> >  };
> >
> >  void binding_register_ovs_idl(struct ovsdb_idl *);
> > @@ -60,11 +64,9 @@ void binding_run(struct binding_ctx_in *, struct
> binding_ctx_out *);
> >  bool binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
> >                       const struct sbrec_port_binding_table *,
> >                       const struct sbrec_chassis *);
> > -bool binding_evaluate_port_binding_changes(
> > -        const struct sbrec_port_binding_table *,
> > -        const struct ovsrec_bridge *br_int,
> > -        const struct sbrec_chassis *,
> > -        struct sset *active_tunnels,
> > -        struct sset *local_lports);
> > +bool binding_evaluate_port_binding_changes(struct binding_ctx_in *,
> > +                                           struct binding_ctx_out *);
> >
> > +void local_bindings_init(struct shash *local_bindings);
> > +void local_bindings_destroy(struct shash *local_bindings);
> >  #endif /* controller/binding.h */
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index c0a97a265..3bda1065c 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -958,6 +958,9 @@ struct ed_type_runtime_data {
> >      /* Contains "struct local_datapath" nodes. */
> >      struct hmap local_datapaths;
> >
> > +    /* Contains "struct local_binding" nodes. */
> > +    struct shash local_bindings;
> > +
> >      /* Contains the name of each logical port resident on the local
> >       * hypervisor.  These logical ports include the VIFs (and their
> child
> >       * logical ports, if any) that belong to VMs running on the
> hypervisor,
> > @@ -969,6 +972,8 @@ struct ed_type_runtime_data {
> >       * <datapath-tunnel-key>_<port-tunnel-key> */
> >      struct sset local_lport_ids;
> >      struct sset active_tunnels;
> > +
> > +    struct sset egress_ifaces;
> >  };
> >
> >  static void *
> > @@ -981,6 +986,8 @@ en_runtime_data_init(struct engine_node *node
> OVS_UNUSED,
> >      sset_init(&data->local_lports);
> >      sset_init(&data->local_lport_ids);
> >      sset_init(&data->active_tunnels);
> > +    sset_init(&data->egress_ifaces);
> > +    local_bindings_init(&data->local_bindings);
> >      return data;
> >  }
> >
> > @@ -992,6 +999,8 @@ en_runtime_data_cleanup(void *data)
> >      sset_destroy(&rt_data->local_lports);
> >      sset_destroy(&rt_data->local_lport_ids);
> >      sset_destroy(&rt_data->active_tunnels);
> > +    sset_destroy(&rt_data->egress_ifaces);
> > +    sset_init(&rt_data->egress_ifaces);
>
> This init is unnecessary here, although not causing any issue.
>

Ack. I'll remove it.


Thanks for the reviews.

Numan


>
> >      struct local_datapath *cur_node, *next_node;
> >      HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node,
> >                          &rt_data->local_datapaths) {
> > @@ -1000,6 +1009,7 @@ en_runtime_data_cleanup(void *data)
> >          free(cur_node);
> >      }
> >      hmap_destroy(&rt_data->local_datapaths);
> > +    local_bindings_destroy(&rt_data->local_bindings);
> >  }
> >
> >  static void
> > @@ -1072,6 +1082,8 @@ init_binding_ctx(struct engine_node *node,
> >      b_ctx_out->local_datapaths = &rt_data->local_datapaths;
> >      b_ctx_out->local_lports = &rt_data->local_lports;
> >      b_ctx_out->local_lport_ids = &rt_data->local_lport_ids;
> > +    b_ctx_out->egress_ifaces = &rt_data->egress_ifaces;
> > +    b_ctx_out->local_bindings = &rt_data->local_bindings;
> >  }
> >
> >  static void
> > @@ -1095,12 +1107,16 @@ en_runtime_data_run(struct engine_node *node,
> void *data)
> >              free(cur_node);
> >          }
> >          hmap_clear(local_datapaths);
> > +        local_bindings_destroy(&rt_data->local_bindings);
> >          sset_destroy(local_lports);
> >          sset_destroy(local_lport_ids);
> >          sset_destroy(active_tunnels);
> > +        sset_destroy(&rt_data->egress_ifaces);
> >          sset_init(local_lports);
> >          sset_init(local_lport_ids);
> >          sset_init(active_tunnels);
> > +        sset_init(&rt_data->egress_ifaces);
> > +        local_bindings_init(&rt_data->local_bindings);
> >      }
> >
> >      struct binding_ctx_in b_ctx_in;
> > @@ -1129,35 +1145,12 @@ static bool
> >  runtime_data_sb_port_binding_handler(struct engine_node *node, void
> *data)
> >  {
> >      struct ed_type_runtime_data *rt_data = data;
> > -    struct sset *local_lports = &rt_data->local_lports;
> > -    struct sset *active_tunnels = &rt_data->active_tunnels;
> > -
> > -    struct ovsrec_open_vswitch_table *ovs_table =
> > -        (struct ovsrec_open_vswitch_table *)EN_OVSDB_GET(
> > -            engine_get_input("OVS_open_vswitch", node));
> > -    struct ovsrec_bridge_table *bridge_table =
> > -        (struct ovsrec_bridge_table *)EN_OVSDB_GET(
> > -            engine_get_input("OVS_bridge", node));
> > -    const char *chassis_id = chassis_get_id();
> > -    const struct ovsrec_bridge *br_int = get_br_int(bridge_table,
> ovs_table);
> > -
> > -    ovs_assert(br_int && chassis_id);
> > -
> > -    struct ovsdb_idl_index *sbrec_chassis_by_name =
> > -        engine_ovsdb_node_get_index(
> > -                engine_get_input("SB_chassis", node),
> > -                "name");
> > -
> > -    const struct sbrec_chassis *chassis
> > -        = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id);
> > -    ovs_assert(chassis);
> > -
> > -    struct sbrec_port_binding_table *pb_table =
> > -        (struct sbrec_port_binding_table *)EN_OVSDB_GET(
> > -            engine_get_input("SB_port_binding", node));
> > +    struct binding_ctx_in b_ctx_in;
> > +    struct binding_ctx_out b_ctx_out;
> > +    init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out);
> >
> > -    bool changed = binding_evaluate_port_binding_changes(
> > -        pb_table, br_int, chassis, active_tunnels, local_lports);
> > +    bool changed = binding_evaluate_port_binding_changes(&b_ctx_in,
> > +                                                         &b_ctx_out);
> >
> >      return !changed;
> >  }
> > --
> > 2.26.2
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>


More information about the dev mailing list