[ovs-dev] [PATCH v3 2/5] ovn: Add generic HA chassis group

Numan Siddique nusiddiq at redhat.com
Thu Mar 7 17:45:31 UTC 2019


On Thu, Mar 7, 2019, 10:46 PM Han Zhou <zhouhan at gmail.com> wrote:

> On Thu, Mar 7, 2019 at 8:37 AM Numan Siddique <nusiddiq at redhat.com> wrote:
> >
> >
> >
> > On Tue, Mar 5, 2019 at 11:34 PM Han Zhou <zhouhan at gmail.com> wrote:
> >>
> >> > > +
> >> > > +static void
> >> > > +add_to_ha_ref_chassis_info(struct ha_ref_chassis_info *ref_ch_info,
> >> > > +                           const struct sbrec_chassis *chassis)
> >> > > +{
> >> > > +    for (size_t j = 0; j < ref_ch_info->n_ref_chassis; j++) {
> >> > > +        if (ref_ch_info->ref_chassis[j] == chassis) {
> >> > > +           return;
> >> > > +        }
> >> > > +    }
> >> > > +
> >> > > +    ref_ch_info->ref_chassis = xrealloc(ref_ch_info->ref_chassis,
> >> > > +                                        sizeof
> *ref_ch_info->ref_chassis *
> >> > > +
> (++ref_ch_info->n_ref_chassis));
> >> >
> >> > This may be inefficient, considering the amount of ref chassises to be
> >> > added for each HA group. It is better to xrealloc for original_size *
> >> > 2 every time and expand only when more space is needed.
> >> >
> >> > > +    ref_ch_info->ref_chassis[ref_ch_info->n_ref_chassis - 1] =
> >> > > +        CONST_CAST(struct sbrec_chassis *, chassis);
> >> > > +}
> >> > > +
> >> > > +static void
> >> > > +update_sb_ha_group_ref_chassis(struct shash *ha_ref_chassis_map)
> >> > > +{
> >> > > +    struct shash_node *node, *next;
> >> > > +    SHASH_FOR_EACH_SAFE (node, next, ha_ref_chassis_map) {
> >> > > +        struct ha_ref_chassis_info *ha_ref_info = node->data;
> >> > > +
> sbrec_ha_chassis_group_set_ref_chassis(ha_ref_info->ha_chassis_group,
> >> > > +
>  ha_ref_info->ref_chassis,
> >> > > +
>  ha_ref_info->n_ref_chassis);
> >> > > +        free(ha_ref_info->ref_chassis);
> >> > > +        free(ha_ref_info);
> >> > > +        shash_delete(ha_ref_chassis_map, node);
> >> > > +    }
> >> > > +}
> >> > > +
> >> > > +/* This function returns logical router datapath (with a
> distributed
> >> > > + * gateway port) to which 'od' is connected to - either directly
> >> > > + * or indirectly (via transit logical switches).
> >> > > + * Returns NULL if no logical router with gw port found.
> >> > > + */
> >> > > +static struct ovn_datapath *
> >> > > +get_router_dp_with_gw_port(struct hmap *ports,
> >> > > +                           struct ovn_datapath *od,
> >> > > +                           struct ovn_datapath *peer_od)
> >> > > +{
> >> > > +    if (!od) {
> >> > > +        return NULL;
> >> > > +    }
> >> > > +
> >> > > +    if (od->nbs) {
> >> > > +        /* It's a logical switch datapath. */
> >> > > +        if (peer_od) {
> >> > > +            /* If peer datapath is not logical router, then
> >> > > +             * something is wrong. */
> >> > > +            ovs_assert(peer_od->nbr);
> >> > > +        }
> >> > > +
> >> > > +        for (size_t i = 0; i < od->n_router_ports; i++) {
> >> > > +            if (!od->router_ports[i]->peer) {
> >> > > +                /* If there is no peer port connecting to the
> >> > > +                 * router datapath, ignore it. */
> >> > > +                continue;
> >> > > +            }
> >> > > +
> >> > > +            struct ovn_datapath *router_dp =
> od->router_ports[i]->peer->od;
> >> > > +            if (router_dp->l3dgw_port &&
> router_dp->l3dgw_port->nbrp) {
> >> > > +                /* Router datapath has a distributed gateway
> router port. */
> >> > > +                return router_dp;
> >> >
> >> > I think we can't return when just one router_dp is found. There can be
> >> > more than one connected router that has gateway router ports. So the
> >> > return value of this function should be a set.
> >> >
> >> > > +            }
> >> > > +        }
> >> > > +
> >> > > +        /* The logical switch datapath is not connected to any
> >> > > +         * logical router with a distributed gateway port. Check
> if it
> >> > > +         * is indirectly connected to a logical router with a gw
> port. */
> >> > > +        for (size_t i = 0; i < od->n_router_ports; i++) {
> >> > > +            if (!od->router_ports[i]->peer) {
> >> > > +                continue;
> >> > > +            }
> >> > > +
> >> > > +            struct ovn_datapath *router_dp =
> >> > > +                od->router_ports[i]->peer->od;
> >> > > +
> >> > > +            /* If we don't check this, we will be in an infinite
> loop. */
> >> > > +            if (router_dp != peer_od) {
> >> >
> >> > peer_od should also be a set, it should skip checking any datapath
> >> > that has already been checked. Consider the case LR1 is connected with
> >> > LS1, LS2 and LS3.
> >> >
> >> > > +                router_dp = get_router_dp_with_gw_port(ports,
> router_dp,
> >> > > +                                                       od);
> >> > > +                if (router_dp) {
> >> > > +                    /* Found a logical router with gw port
> indirectly connected
> >> > > +                     * to 'od'. */
> >> > > +                    return router_dp;
> >> > > +                }
> >> > > +            }
> >> > > +        }
> >> > > +    } else if (od->nbr) {
> >> > > +        /* It's a logical router datapath. */
> >> > > +        if (peer_od) {
> >> > > +            /* If peer datapath is not logical switch, then
> >> > > +             * something is wrong. */
> >> > > +            ovs_assert(peer_od->nbs);
> >> >
> >> > A router port can be peered with another router port directly, so this
> >> > assert is not true.
> >> >
> >> > > +        }
> >> > > +
> >> > > +        /* Check if this logical router datapath is indirectly
> connected
> >> > > +         * to another logical router via a transit logical
> switch(es). */
> >> > > +        for (size_t i = 0; i < od->nbr->n_ports; i++) {
> >> > > +            struct ovn_port *router_port =
> >> > > +                ovn_port_find(ports, od->nbr->ports[i]->name);
> >> > > +
> >> > > +            if (!router_port || !router_port->peer) {
> >> > > +                continue;
> >> > > +            }
> >> > > +            /* If we don't check this, we will be in an infinite
> loop. */
> >> > > +            if (router_port->peer->od != peer_od) {
> >> > > +                struct ovn_datapath *router_dp;
> >> > > +                /* router_port->peer->od points a logical switch
> datapath. */
> >> > > +                router_dp = get_router_dp_with_gw_port(ports,
> >> > > +
>  router_port->peer->od,
> >> > > +                                                       od);
> >> > > +                if (router_dp) {
> >> > > +                    /* Found a logical router with gw port
> indirectly connected
> >> > > +                    * to 'od'. */
> >> > > +                    return router_dp;
> >> > > +                }
> >> > > +            }
> >> > > +        }
> >> > > +    }
> >> > > +
> >> > > +    return NULL;
> >> > > +}
> >> >
> >> > In general, it may refer to the flood-fill approach implemented in
> >> > ovn-controller for populating local datapaths in binding.c, which is
> >> > similar to the purpose here. However, we should consider an
> >> > optimization here since the flood-fill cost would apply for every
> >> > port-binding. It can be optimized with a cache, which maps between
> >> > each datapath and its related router_dps with gw ports, to avoid
> >> > repeated traversing. (In ovn-controller it can be optimized, too, but
> >> > the gain is not as big as here since only local port-bindings are
> >> > checked in ovn-controller.)
> >> >
> >
> >
> > Thanks for the comments Han. I will work on them.
> > I haven't thought of an optimization yet. I will try to explore based
> > on your suggestions. At the same time, I personally think the code
> > shouldn't get complicated because of the optimization. (I feel the
> present
> > code in ovn-controller [1] is a bit complicated -
> > [1] -
> https://github.com/openvswitch/ovs/blob/master/ovn/controller/bfd.c#L98
> > )
>
> Sorry, I should point out the location more clearly. I was talking
> about this code:
>
> https://github.com/openvswitch/ovs/blob/master/ovn/controller/binding.c#L113
> It is about the algorithm that calculates related datapaths. I think
> we need similar logic in northd, but since northd handles all port
> bindings, it is worth to optimize with a cache to avoid repeat this
> calculation for every port-binding.
>

Thanks. Makes sense. I will work on it.



> >
> > I think it is uncommon to have indirect logical router connections which
> eventually
> > connect to a router with a gateway router port.
> > May be I am wrong. Do you think it's common to have such setups ?
> >
>
> It may be uncommon, but I think it is important to make it correct in
> these situations.
>

Agree.

Thanks
Numan


> >
> >> I forgot to mention, this implementation is for finding out the
> >> required BFD sessions, however, it can be reused for a more generic
> >> purpose - to figure out whether a tunnel is needed between each pair
> >> of chassises. There happens to be a related discussion ongoing here:
> >>
> https://mail.openvswitch.org/pipermail/ovs-discuss/2019-March/048281.html
> >>
> >
> > I have seen thread. I will try to make it generic.
> > If not I think we can revisit the code later and make it generic and
> more optimized.
> > As a first step I think it should be reasonable to aim to have the
> solution for the HA_Chassis_Group's ref_chassis calculation.
> > Does this sound good ?
> >
>
> I think it is fair to have separate patches to address the
> optimizations. For current patch, if it is only about making HA group
> generic for both GW and external port, I think we can simply put all
> chassises as ref_chassis for all GW HA-groups, and enable bfd for
> them, to make it work just like how it behaves today. Further
> optimizations can be added separately.
>
> Thanks,
> Han
>


More information about the dev mailing list