[ovs-dev] [PATCH v3 2/5] ovn: Add generic HA chassis group
Han Zhou
zhouhan at gmail.com
Tue Mar 5 18:03:51 UTC 2019
> > +
> > +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.)
>
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
Thanks,
Han
More information about the dev
mailing list