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

Han Zhou hzhou at ovn.org
Wed May 20 18:22:27 UTC 2020


On Wed, May 20, 2020 at 2:25 AM Numan Siddique <numans at ovn.org> wrote:
>
>
>
> On Wed, May 20, 2020 at 1:27 PM Numan Siddique <numans at ovn.org> wrote:
>>
>>
>>
>> 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.
>>
My bad, I can update it.

>>>
>>> > +};
>>> > +
>>> > +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 ?
>>
You are right, assert can cause upgrading problem. I think we can just
print a warning about unknown types from ovn-controller point of view, so
that we know an upgrade is needed, or it could be just corrupt data in SB
DB, or a bug.

>>
>>>
>>> > +        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

If binding_run() is triggered after this step, the consider_vif_lport()
won't be triggered because the PB is not created yet.

>> ovn-nbctl lsp-add ls0 sw0-p1

If binding_run() is triggered here, then the lbinding->pb should have
already been set in build_local_bindings(), and setting it here is useless.

>>
>> Please see the comments above about the "struct lbinding".
>
>
> I think we can remove the lport_lookup_by_name() in
build_local_bindings() as
> it is expensive and unnnecessary as binding_run() will eventually set it
in
> consider_vif_lport().
>
Yes, this might be better. It is better to have clear responsibility of
each function.

>
> Thanks
> Numan
>
>>
>>
>>>
>>> > +    }
>>> > +    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.
>>
I see. Yes, it would be better to add such code in next patch which would
help understanding each patch, and merge it incrementally.

>>>
>>> > +        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
>>
>>

By "duplicated port-binding" I meant misconfiguration with same iface-id in
different interfaces. I think a warning log can be printed here instead of
sliently overwritting the previous one.

>>>
>>>
>>> > +                    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 ?

Sure, it is ok if we ensure patch 1 and patch 2 are merged together.

>>
>> 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